1501608 - Investigate reducing size of ScriptSourceObject by moving DOM related fields outside the engine

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Jon Coppeard (:jonco)]

Bug 1482153 increased memory usage slightly (see bug 1482153 comment 18) by adding an extra slot to ScriptSourceObject which now requires an 8 slot object rather than a 4 slot object as before.

It's possible that we could move the slots containing the DOM element proxy and DOM attribute name out of the engine and into a script loader data structure.  This would make these objects smaller but also mean that we don't have to create DOM proxy objects for elements unless they are required (usually for the debugger).  This does require a DOM data structure to hold these, but following bug 1342012 I think this will always be the case.  This is also nicer conceptually to not have the JS engine know about these things (although the debugger will still have to).

Comment 1

[Jon Coppeard (:jonco)]

The idea is to associate JSScripts with the DOM LoadScript data structure via JS::SetScriptPrivate / SetModulePrivate. The JS API receives this as a private Value. The browser would supply callback functions to get the required data out of this structure.

smaug pointed out on IRC that this association needs to happen as soon possible for scripts. At the moment this only happens for scripts when they are executed (see ExecuteCompiledScript) so this would need to happen sooner. Possibly this would require us to merge off thread compiled scripts as soon as they finish compiling, rather than waiting until we need to execute them. This would be an additional complication, but I think this makes sense to do anyway.

Comment 3

[Denis Palmeiro [:denispal]]

I can look into this.

Comment 4

[Jon Coppeard (:jonco)]

(In reply to Denis Palmeiro [:denispal] from comment #3)
You might want to coordinate with mgaudet as I think he was looking at it too. Or at least I was talking to him about it last week.

Comment 5

[Denis Palmeiro [:denispal]]

(In reply to Jon Coppeard (:jonco) from comment #4)

(In reply to Denis Palmeiro [:denispal] from comment #3)
You might want to coordinate with mgaudet as I think he was looking at it too. Or at least I was talking to him about it last week.

Thanks! Yes, I did ask him beforehand and he said he was happy to offload this.

Comment 6

[Denis Palmeiro [:denispal]]

I'm not really sure on how to use a callback function when we set the element from mozilla::EventListenerManager::CompileEventHandlerInternal. One approach that might work is if we share the private slot in the script source object to either store the LoadedScript or the element if there is no associated LoadedScript object available. I think we can distinguish them by checking if privateValue.isObjectOrNull() is true or not. This will let us store the element directly whenever we have it available, or otherwise get it from the LoadedScript through a callback. We can still remove the ELEMENT_SLOT from the SSO this way to save memory. However, it will not work if we ever need to store a script element + Loadedscript that is not related but I'm not sure if that is ever the case.

Jon, does this approach sound feasible to you?

Comment 7

[Jon Coppeard (:jonco)]

(In reply to Denis Palmeiro [:denispal] from comment #6)
You could also store the wrapped element here, but that does make things more complicated. I think I was imagining that you would create a LoadedScript in CompileEventHandlerInternal to hold this information. Although that would add to memory use a little we still avoid wrapping the element when it's not used.

Comment 8

[Denis Palmeiro [:denispal]]

Attachment: WIP - Use a callback to retrieve the script element

Instead of saving the script element in the SSO, retrieve the script element using a callback function that takes the LoadedScript which is already saved in the SSO private slot.

Comment 9

[Denis Palmeiro [:denispal]]

Posting what I have at the moment. It currently works and passes most tests using the method Jon described in comment 7, but I am facing one issue that I can only reproduce on try where the CC keeps deleting the mFetchOptions member from the LoadedScript whenever it's created from the ELM and when we try to get the element from the debugger later on with the callback we can't because mFetchOptions is null.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=45b24178d91c2a13b73d57d424218511e6e25cee&selectedJob=264809810 has
https://hg.mozilla.org/try/rev/2e6278a24e4d899fbd0e8f45c7d14322883bd00d in it, and at least atm looks ok.
I haven't managed to reproduce the crash locally, but based on the stack traces FetchOptions() returns a null object and we crash at
address null+offset.
And null FetchOptions() may happen if CC has run, but GC hasn't yet killed JSScript.
https://mozilla.logbot.info/jsapi/20190903#c16587049 has some background.

Assuming this is actually the reason (which seems quite likely), I'd say the crash is a debugger issue.
Adding a null-check might not be too bad for now.

jonco may have an opinion

Comment 11

[Jim Blandy :jimb]

There was some speculation in #jsapi that the Debugger's weak maps mapping JSScripts to Debugger.Script objects might have something to do with the problem. I can't rule that out, but for what it's worth, Debugger's weak maps are registered with the CC in the same way as all the other weak maps:

js::Debugger::scripts is a js::Debugger::ScriptWeakMap, which is a js::DebuggerWeakMap, which inherits from WeakMap, which inherits from WeakMapBase, which inherits from mozilla::LinkedListElement<WeakMapBase>, so I think the Debugger's weak maps should appear in that list. The WeakMap constructor calls zone()->gcWeakMapList().insertFront(this) here:

https://searchfox.org/mozilla-central/source/js/src/gc/WeakMap-inl.h#57

Comment 12

[Jim Blandy :jimb]

Oh, hmm. Could you see if the crash started when b65e48539b71 landed (around 2019-6-5)? That changeset adds another edge from a Debugger.Frame object to a JSScript. I thought I'd gotten it right, but the frame table is not a weak map.

Comment 13

[Jon Coppeard (:jonco)]

The setup looks OK to me. I agree with Olli that it looks like the CC has unlinked the LoadedScript object as being part of a garbage cycle but the debugger has found a JSScript pointer into it. Is there a stack trace for the crash? I'd like to see where the JSScript comes from.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=eea5eebbe121bc883b23e3c5dcb5b7d41a751b9c dt5
but that isn't too useful.

I'm going to push a test patch to try to see if we see similar issue even currently - do we end up dealing with unlinked Element object.
(Elements mostly just work after unlinking, they don't have parent or children, but they do have ownerDocument. So one needs to add a flag to tell whether unlinking has happened or so.)

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Totally crazy patch to try to check if we already somehow deal with unlinked elements.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ff93866e646dec911138c677bd215f216808281

Comment 16

[Denis Palmeiro [:denispal]]

Attachment: Remove the ELEMENT_SLOT in the ScriptSourceObject and instead use a callback function to return the script element based on the value of the privateValue in the SSO.

We can reduce the size of the SSO by removing the element slot entirely, and instead retrieve the element through a callback function. The callback will take in the value in the private slot of the SSO, which is either a LoadedScript* (from the browser) or a JSObject* (from the shell). In addition, this removes the requirement of having a script dom element ready when parsing a JS script which can open up new opportunities for performance.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=93ffb20fa18826febc8f3f7b375b2a1bde65b0d0

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4107b758e7ae
Remove the ELEMENT_SLOT in the ScriptSourceObject and instead use a callback function to return the script element based on the value of the privateValue in the SSO. r=jonco,smaug

Comment 19

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/4107b758e7ae

Comment 20

[Tom S [:evilpie]]

We should back this out. This patch is certainly non trivial and landed during or just before the soft-freeze.

Comment 21

[Dorel Luca [:dluca]]

Backed out changeset 4107b758e7ae (bug 1501608) as requested by dev

Backout:
https://hg.mozilla.org/integration/autoland/rev/e1690bafaf05f42caa8d24da9ba1364775c1c258

Comment 22

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backout got merged: https://hg.mozilla.org/mozilla-central/rev/e1690bafaf05

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/554b7637fe60
Remove the ELEMENT_SLOT in the ScriptSourceObject and instead use a callback function to return the script element based on the value of the privateValue in the SSO. r=jonco,smaug

Comment 24

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/554b7637fe60

Comment 25

[Logan Smyth [:loganfsmyth]]

I'm curious, could we remove the callbacks too by treating the new private slot as opaque to SpiderMonkey and then leaving it up to the devtools to work cooperatively with Gecko to decide what the value is?

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

Could you file a new bug to figure out if we could change the setup.
(It isn't super clear to me what the opaque slot would mean.)

Comment 27

[Logan Smyth [:loganfsmyth]]

I think I'm going to skip this for now because I realize that currently the private slot is propagated to sources created via eval and Function, so if anything this patch has complicated things because now evals within scripts will also have an element even though they are evals and not scripts. Not the end of the world, but also means that I can't use this to build what I had in mind. I don't think anything will break because of the weird element behavior so we probably don't need to worry about that for now, but it's certainly not what I'd expect.

If we wanted to move any more fields onto this private object, it seems like there's need to be more work to separate state meant to pass through to eval/Function() and state that is explicitly attached to an individual source.