Closed Bug 1501608 Opened 2 years ago Closed 6 months ago

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

Categories

(Core :: JavaScript Engine, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla78
Tracking Status
firefox78 --- fixed

People

(Reporter: jonco, Assigned: denispal)

References

Details

Attachments

(1 file, 1 obsolete file)

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).
Priority: -- → P3

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.

Duplicate of this bug: 1548379

I can look into this.

Assignee: nobody → dpalmeiro

(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.

(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.

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?

Flags: needinfo?(jcoppeard)

(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.

Flags: needinfo?(jcoppeard)

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.

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.

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

Flags: needinfo?(jcoppeard)

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

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.

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.

Flags: needinfo?(jcoppeard)

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.)

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

See Also: → 1582160
Blocks: 1606652
Attachment #9081651 - Attachment is obsolete: true

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.

Depends on: 1634182
Pushed by dpalmeiro@mozilla.com:
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
Status: NEW → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla77

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

Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Regressions: 1634845

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

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

Target Milestone: mozilla77 → ---
Pushed by dpalmeiro@mozilla.com:
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
Status: REOPENED → RESOLVED
Closed: 6 months ago6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla78
See Also: → 1637618
Regressions: 1637886

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?

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.)

Flags: needinfo?(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.

Flags: needinfo?(loganfsmyth)
Depends on: 1653974
No longer depends on: 1653974
Regressions: 1653974
You need to log in before you can comment on or make changes to this bug.