Clean up external object references when they are garbage collected, …

[oliverbock]

…rather than when the context is destroyed.

[spahnke]

I wish I could give you a more detailed analysis but I'm occupied with another project until at least the end of October 😞 All I can say for now after integrating your change into our fork and then into our project, is that a crucial benchmark test where we execute a script that operates on a large object tree (i.e. many nested objects)[*] more than doubles in execution time (~2.2x). The only change is this one commit. On top of that three of our tests fail when executed in conjunction with said benchmark test, but pass if executed on their own or only with other "pure" engine tests. I didn't have the time to run our whole test suite to see if other tests break, but something is fishy there.

Again, sorry for the vague analysis 😞

[*] The script has a lot of back and forth between C# and JavaScript and the object tree is rooted in one global object that gets passed to the context and is set to null after the script has executed. If we run said script 100 times we get an average execution time of 233.84ms without the change and 525.67ms with the proposed change. The script is roughly 500 lines where half of it is actual logic and the other half is filling a transfer object with mostly primitive values that gets passed back to C# (basically a plain JS object that acts as a dictionary).

[oliverbock]

Hi Sebastian, thanks for the feedback. I guess that the performance problems relate to the increased complexity in JavascriptExternal::GetMethod(). I have changed this to copy the mMethods dictionary reference into each JavascriptExternal, which should return that method to its old cost, while preserving the other gains.

(This pull request has two benefits:

• We no longer hang onto all objects passed into SetParameter() for the lifetime of the JavascriptContext. (They were stored in JavascriptContext.mExternals.) This was causing us memory leaks when we started reusing JavascriptContexts to save on startup costs.
• The dictionary of methods looked up via reflection in JavascriptExternal::GetMethod() is now shared per type, rather than recalculated for every object. This makes things faster when multiple instances of the same C# class are used.)

Another cost of this change might be that we no longer cache JavascriptExternals in mExternals. Is the same object instance passed many times to SetParameter()?

[spahnke]

The new commit didn't improve the execution time unfortunately (still ~520ms) and we still get random test failures.

Is the same object instance passed many times to SetParameter()?

For us it's a new instance every time we execute but it's always the same type. To elaborate a bit further how our basic setup looks like: Creating a new JavascriptContext per execution is way too expensive for us in batch processes so we reuse a couple of those in a pool. We can't use them indefinitely because of the memory leak this PR seems to address (i.e. objects are strongly referenced until the context is disposed). So we keep a context for a while and then dispose it and create a new one so that the C# instances can be garbage collected. That means to execute a script we

• get a context from the pool
• if it is too old (~1 minute) we dispose it and create a new one

  The amortized cost of this has proven to be efficient enough for us.

• create a new instance of the (complex) global object and pass it to the context context.SetParameter("myGlobal", myGlobal)
• execute the code
• set the global object to null again `context.SetParameter("myGlobal", null)

  One reason for that is that we have a couple of global objects but only a subset of them is used in different scenarios. And since we reuse contexts we have to always clean these up. For the purpose of our benchmark this doesn't matter. Here it is always the same global that gets set but always a new instance.

• put the context back into the pool

As a test I set the lifetime of our contexts to infinity but that made the execution time even worse (~690ms) and I saw an increase in memory consumption of the test. Though to be fair, the memory measurement isn't exactly accurate and we only use it to make sure nothing explodes from the get go.

[oliverbock]

Then I guess that the performance problem relates to me getting rid of the mExternals cache of JavascriptExternals. In particular, if you are traversing the same tree of objects many times then you are probably repeatedly triggering WrapObject for the same object.

I could propose a different fix for my problems (leaking memory) that would just provide a ClearExternalObjectCache() to clear mExternals, but it wouldn't be safe if any references remained in v8.

I guess we'll just do something like you do, and recycle our JavascriptContext occasionally.