Open Bug 1501608 Opened Last year Updated 3 months ago

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


(Core :: JavaScript Engine, enhancement, P3)





(Reporter: jonco, Assigned: denispal)




(1 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. has 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. 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:

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

See Also: → 1582160
You need to log in before you can comment on or make changes to this bug.