BHR reports that the majority of "medium hangs" between 512ms and 2s: https://arewesmoothyet.com/?category=all&durationSpec=512_2048&mode=explore&payloadID=7c318db92afd491eace690b5b62d363b&search=devtools&thread=0 Are related to Debugger API's findScripts method. The main call site is from the debugger actor, right here: https://searchfox.org/mozilla-central/rev/97cb0aa64ae51adcabff76fb3b5eb18368f5f8ab/devtools/server/actors/script.js#1339 const scripts = this.dbg.findScripts(); It looks like we do call this only once, when we open the debugger against an already opened website. So I haven't found immediately something wrong with the call site. Looking at BHR stacks as well as a profile: https://perfht.ml/2E18rr2 (records debugger opening against a large website [lemonde.fr]) it looks like findScripts "delazify" the scripts and parse them all! If that's really the case and it does that for all the scripts and synchronously, BHR results make a lot of sense.

Jim, Any input? May be this bug should be moved to JS Engine, but I have not found any component specific for Debugger API.

I think the fix here would be for Debugger.Script not to eagerly delazify scripts. Many properties, including url, startLine, and lineCount, should be able to be accessed without delazifying, but anything that looks at the byte code or deals with source locations will need to delazify the script. If it's the case that the server would immediately de-lazify the scripts anyway, then permitting lazy Debugger.Script objects wouldn't help much. (Although it would make it possible in principle to break up the work into smaller chunks.)

Here's partially working patch: (there's remaining crash because of delazification somehow doesn't work tho) https://treeherder.mozilla.org/#/jobs?repo=try&revision=b4bc5c9dac2e4a0c5a8c5af82f6157aacd9c9759 and here's the statistics for OS X debug mochitest-dt there: Script property/method access count (get lineCount) 10: JSScript 2: LazyScript => delazify (get source) 94024: JSScript 70: LazyScript without SourceObject => delazify all scripts in the component 61654: LazyScript (get sourceLength) 8: LazyScript (get sourceStart) 8: LazyScript (get startLine) 706: JSScript 1604: LazyScript (get url) 693: JSScript 1578: LazyScript clearBreakpoint 104: JSScript 115: LazyScript => delazify getAllColumnOffsets 8: JSScript 13: LazyScript => delazify getLineOffsets 288: JSScript 155: LazyScript => delazify getOffsetLocation 599: JSScript 1287: LazyScript => delazify isInCatchScope 2: JSScript 6: LazyScript => delazify setBreakpoint 104: JSScript 115: LazyScript => delazify getAllOffsets 1: LazyScript => delazify Debugger::findScripts call count 568: does not need delazification 25: needs delazification (hasLine, urlCString.ptr()) 247: needs delazification (hasLine, hasSource) Debugger::wrapVariantReferent call count 48880: JSScript 14866: LazyScript retrieved from JSScript 17376: LazyScript with SourceObject 4296: LazyScript without SourceObject Looks like we can keep many scripts lazified while running those tests

(In reply to Tooru Fujisawa [:arai] from comment #3) > Here's partially working patch: > (there's remaining crash because of delazification somehow doesn't work tho) > > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=b4bc5c9dac2e4a0c5a8c5af82f6157aacd9c9759 \o/ Thanks for working on this! I imagine this is mostly debugger test requiring delazification while other panel tests doesn't. Or are you looking only at debugger panel mochitests?

the statistics is for entire mochitest-dt (dt1...dt8) (actually I just printed the activity into stderr and grepped the log, so I don't know which call is in which test) now I'm checking the statistics when opening debugger in GMail.

Thanks that looks great!

just to be clear about "LazyScript without SourceObject => delazify all scripts in the component" part. this is tentative solution for that situation and now I'm working on fixing it to delazify necessary scripts only (I think, just need to delazify all ancestor scripts of the given script) and in GMail, such situation happens and now my patch is delazifying all scripts, so it should be fixed before getting better statistics on GMail.

Note that right now, findScripts is called when you open the toolbox no matter which tool is selected. So all scripts will be delazified anytime you open DevTools. If your patch stops that for all tools but the debugger, it will be already *a very significant win*! So that if it is easy to proceed with a first patch and followup to optimize the debugger case I would suggest doing so. (Inspector is by far the most used tool and suffer from this performance hit)

updated to delazify only enclosing scripts https://treeherder.mozilla.org/#/jobs?repo=try&revision=65eef633c13ad925b741896ffda258143d17b0f4&selectedJob=176768088 the statistics for linux64 debug mochitest-dt: Script property/method access count (get lineCount) 6: JSScript 2: LazyScript without SourceObject => delazify (get source) 94409: JSScript 2214: LazyScript without SourceObject => delazify 26955: LazyScript with JSScript 31258: LazyScript without JSScript (get sourceLength) 8: LazyScript with JSScript (get sourceStart) 8: LazyScript with JSScript (get startLine) 705: JSScript 1595: LazyScript with JSScript (get url) 693: JSScript 1568: LazyScript with JSScript clearBreakpoint 103: JSScript 113: LazyScript without SourceObject => delazify getAllOffsets 1: LazyScript without SourceObject => delazify getAllColumnOffsets 5: JSScript 13: LazyScript without SourceObject => delazify getLineOffsets 261: JSScript 152: LazyScript without SourceObject => delazify getOffsetLocation 588: JSScript 1270: LazyScript without SourceObject => delazify isInCatchScope 2: JSScript 6: LazyScript without SourceObject => delazify setBreakpoint 103: JSScript 113: LazyScript without SourceObject => delazify The number of enclosing LazyScript that gets delazified while delazification: 2214: LazyScript with SourceObject 299: LazyScript without SourceObject (which needs recursive delazification) Debugger::findScripts call count 572: does not need delazification 25: needs delazification (hasLine, urlCString.ptr()) 244: needs delazification (hasLine, hasSource) Debugger::wrapVariantReferent call count 48991: wrap JSScript 14312: wrap LazyScript retrieved from JSScript 17737: wrap LazyScript with SourceObject 4244: wrap LazyScript without SourceObject and the statistics for GMail: Script property/method access count (get source) 9039: JSScript 4227: LazyScript without SourceObject => delazify 45715: LazyScript with JSScript 84189: LazyScript without JSScript The number of enclosing LazyScript that gets delazified while delazification: 4227: LazyScript with SourceObject 544: LazyScript without SourceObject (which needs recursive delazification) Debugger::findScripts call count 1: does not need delazification Debugger::wrapVariantReferent call count 2671: wrap JSScript 17196: wrap LazyScript retrieved from JSScript 44483: wrap LazyScript with SourceObject 6508: wrap LazyScript without SourceObject

(In reply to Tooru Fujisawa [:arai] from comment #9) > and the statistics for GMail: when opening Inspector for Inbox page

Changed .source getter not to delazify, but look for ancestor LazyScript which has the reference to ScriptSourceObject. https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c40dce69363a188f8f25ebdf13767260743fca0 Now the number of delazification is reduced, but it takes longer to open debugger, because it's currently looping on all scripts to find ancestor LazyScript. We should figure out why LazyScript inside LazyScript doesn't have the reference to ScriptSourceObject, and if possible, store ScriptSourceObject reference into LazyScript in all case, so that the time taken by accessing .source property will be reduced. the statistics for linux64 debug mochitest-dt: Script property/method access count (get lineCount) 10: JSScript 2: LazyScript without SourceObject => delazify (get source) 94281: JSScript 19543: LazyScript with JSScript 44605: LazyScript without JSScript (get sourceLength) 8: LazyScript with JSScript (get sourceStart) 8: LazyScript with JSScript (get startLine) 708: JSScript 1630: LazyScript with JSScript (get url) 695: JSScript 1604: LazyScript with JSScript clearBreakpoint 104: JSScript 115: LazyScript without SourceObject => delazify getAllColumnOffsets 5: JSScript 13: LazyScript without SourceObject => delazify getAllOffsets 1: LazyScript without SourceObject => delazify getLineOffsets 303: JSScript 154: LazyScript without SourceObject => delazify getOffsetLocation 588: JSScript 1274: LazyScript without SourceObject => delazify isInCatchScope 2: JSScript 6: LazyScript without SourceObject => delazify setBreakpoint 104: JSScript 115: LazyScript without SourceObject => delazify Debugger::findScripts call count 570: findScript does not delazification 20: findScript needs delazification (hasLine, urlCString.ptr()) 245: findScript needs delazification (hasLine, hasSource) Debugger::wrapVariantReferent call count 48973: wrap JSScript 14318: wrap LazyScript retrieved from JSScript 17767: wrap LazyScript with SourceObject 4308: wrap LazyScript without SourceObject and the statistics for opening Inspector on GMail: Script property/method access count (get source) 4500: JSScript 29960: LazyScript with JSScript 97774: LazyScript without JSScript Debugger::findScripts call count 1: findScript does not delazification Debugger::wrapVariantReferent call count 2491: wrap JSScript 16182: wrap LazyScript retrieved from JSScript 42563: wrap LazyScript with SourceObject 6247: wrap LazyScript without SourceObject and the statistics for opening Debugger on GMail: Script property/method access count (get source) 4399: JSScript 97459: LazyScript without JSScript 29512: LazyScript with JSScript Debugger::findScripts call count 1: findScript does not delazification Debugger::wrapVariantReferent call count 2440: wrap JSScript 15945: wrap LazyScript retrieved from JSScript 42407: wrap LazyScript with SourceObject 6246: wrap LazyScript without SourceObject

(some debug code is not yet removed) * Added IterateLazyScripts, which behaves just like IterateScripts for LazyScript * Supported LazyScript for several GC-related classes/templates that accepts JSScript * Changed Debugger::ScriptQuery::findScripts to delazify all scripts only if necessary, that is, if the query requires JSScript-specific information * Changed Debugger::ScriptQuery::findScripts to collect LazyScript as well * Added LazyScript variant to DebuggerScriptReferent * Changed Debugger::wrapVariantReferent to wrap LazyScript of the given JSScript, if there is. This is because we cannot have 2 object instances for JSScript and LazyScript which points the same script, because otherwise `obj1 == obj2` becomes false when obj1 has JSScript reference and obj2 has LazyScript reference for same script. So, I chose to use LazyScript whenever possible, because LazyScript doesn't have shorter lifetime than corresponding JSScript (is this correct?) * Changed Debugger.Script accessor/methods to support LazyScript * If the property is available in LazyScript, just return it * If the property is not available in LazyScript, get or delazify JSScript and use it (delazification is not always necessary, because the LazyScript can already have corresponding JSScript) * Allow lazily parse in Debugger's eval, because now we can handle LazyScript in debugger Then, I have some issues around GC. When supporting LazyScript in debugger, I copied the things done for JSScript, and the issue is that LazyScript is private, where JSScript is public, and some GC-related functions are not defined for LazyScript. what I added LazyScript support so far are: * ExposeLazyScriptToActiveJS * MovableCellHasher * TraceManuallyBarrieredCrossCompartmentEdge * WeakMap::getDelegate * WeakMap::keyNeedsMark * CrossCompartmentKey I'm not sure what I've implemented are correct, and also I cannot implement EdgeNeedsSweepUnbarrieredSlow because of some complicated template issue and public/private APIs difference. jonco, can I have some feedback for those GC part?

Comment on attachment 8974599 [details] [diff] [review] Support LazyScript in Debugger.Script. Review of attachment 8974599 [details] [diff] [review]: ----------------------------------------------------------------- It's unfortunate that this adds complexity, but the GC parts are good. ::: js/public/HeapAPI.h @@ +630,5 @@ > + // FIXMEarai > + // MOZ_ASSERT(!js::gc::EdgeNeedsSweepUnbarrieredSlow(&lazyScript)); > + > + gc::ExposeGCThingToActiveJS(JS::GCCellPtr(lazyScript)); > +} This is unnecessary (see below). ::: js/src/vm/Debugger.cpp @@ +4459,5 @@ > + for (LazyScript** i = lazyScriptVector.begin(); i != lazyScriptVector.end(); ++i) { > + ExposeLazyScriptToActiveJS(*i); > + if ((*i)->maybeScript()) > + JS::ExposeScriptToActiveJS((*i)->maybeScript()); > + } I didn't realise the debugger still called ExposeScriptToActiveJS. This is unnecessary since we added a read barrier (equivalent to this) to IterateScripts a while ago. So you can remove this block and the one for JSScripts above.

Thank you for your feedback :D here are times taken by Debugger::findScripts call, when opening devtools after opening GMail Inbox page before GMail Inspector: ~949 ms (985 806 891 1115) GMail Debugger: ~898 ms (905 880 904 904) after GMail Inspector: ~112 ms (121 103 90 137) GMail Debugger: ~96 ms (88 91 106 99) I'll fix the GC part and post the patch after another try.

green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c19902597fe5aa27b251778d0161493f6fc77ac

(In reply to Tooru Fujisawa [:arai] from comment #14) > here are times taken by Debugger::findScripts call, when opening devtools > after opening GMail Inbox page > > before > GMail Inspector: ~949 ms (985 806 891 1115) > GMail Debugger: ~898 ms (905 880 904 904) > after > GMail Inspector: ~112 ms (121 103 90 137) > GMail Debugger: ~96 ms (88 91 106 99) \o/ Thanks a lot for looking into that! Is this with the inspector or debugger opened?

(In reply to Alexandre Poirot [:ochameau] from comment #16) > (In reply to Tooru Fujisawa [:arai] from comment #14) > > here are times taken by Debugger::findScripts call, when opening devtools > > after opening GMail Inbox page > > > > before > > GMail Inspector: ~949 ms (985 806 891 1115) > > GMail Debugger: ~898 ms (905 880 904 904) > > after > > GMail Inspector: ~112 ms (121 103 90 137) > > GMail Debugger: ~96 ms (88 91 106 99) > > \o/ Thanks a lot for looking into that! > > Is this with the inspector or debugger opened? I've used Gecko Profiler to profile, when opening Inspector and Debugger for each, from [Web Developer] menu. the time there are time taken by Debugger::findScripts call, that is called once while opening devtools pane.

Oh right, I read it too fast, that looks very promising. I'm looking forward the impact of this patch against BHR: https://arewesmoothyet.com/?mode=track&trackedStat=Devtools%20Hangs Most of the hangs in "Content" process are related to findScript, so it should have a sensible impact on this graph.

* Added IterateLazyScripts, which behaves just like IterateScripts for LazyScript * Supported LazyScript for several GC-related classes/templates that accepts JSScript * MovableCellHasher * TraceManuallyBarrieredCrossCompartmentEdge * WeakMap::getDelegate * WeakMap::keyNeedsMark * LazyScriptVector (corresponds to ScriptVector) * Changed Debugger::ScriptQuery::findScripts to delazify all scripts only if necessary, that is, if the query requires JSScript-specific information * 'line' property LazyScript doesn't have information for the function's last line * 'innermost' property I haven't implemented the 'innermost' algorithm for LazyScript (I have to first figure out what it does) * Changed Debugger::ScriptQuery::findScripts to collect LazyScript as well * LazyScripts are collected only when it doesn't have corresponding JSScript * this operation isn't performed if we delazified all scripts (for above queries) * Added LazyScript variant to DebuggerScriptReferent * Changed Debugger::wrapVariantReferent to wrap LazyScript of the given JSScript, if there is. This is because we cannot have 2 object instances for JSScript and LazyScript which points the same script, because otherwise `obj1 == obj2` becomes false when obj1 has JSScript reference and obj2 has LazyScript reference for same script. So, I chose to use LazyScript whenever possible, because LazyScript doesn't have shorter lifetime than corresponding JSScript (is this correct?) * Changed Debugger.Script accessor/methods to support LazyScript * If the property is available in LazyScript, just return it * If the property is not available in LazyScript, get or delazify JSScript and use it (delazification is not always necessary, because the LazyScript can already have corresponding JSScript) * Added EnclosingLazyScriptFinder to delazify enclosing script recursively (since currently there's no direct reference from inner script to enclosing script, and we don't have scope object if enclosing script is also lazy) * Allow lazily parse in Debugger's eval, because now we can handle LazyScript in debugger * Fixed a test that expects delazification, that is no more performed * Debugger-findScripts-23.js * Added LazyScript::compartment method that returns JSFunction's compartment, just for simplicity in the debugger logic (to make LazyScript behave like JSScript) * Removed unnecessary JS::ExposeScriptToActiveJS call, per comment #13 * (removed debug-prints from the last patch)

(In reply to Tooru Fujisawa [:arai] from comment #19) > * Changed Debugger::wrapVariantReferent to wrap LazyScript of the given > JSScript, if there is. > > This is because we cannot have 2 object instances for JSScript and > LazyScript which points the same script, because otherwise `obj1 == obj2` > becomes false when obj1 has JSScript reference and obj2 has LazyScript > reference for same script. So, I chose to use LazyScript whenever possible, > because LazyScript doesn't have shorter lifetime than corresponding > JSScript (is this correct?) I think that is correct. Yes, this approach to preserving Debugger.Object identity makes sense.

>* Changed Debugger::ScriptQuery::findScripts to delazify all scripts only if necessary, > that is, if the query requires JSScript-specific information > * 'line' property > LazyScript doesn't have information for the function's last line > * 'innermost' property > I haven't implemented the 'innermost' algorithm for LazyScript > (I have to first figure out what it does) The 'innermost' query property is documented here: https://searchfox.org/mozilla-central/rev/2b9779c59390ecc47be7a70d99753653d8eb5afc/js/src/doc/Debugger/Debugger.md#397-400 If it turns out that queries with these properties are common, we could certainly store them on the LazyScript when it is created. But that should be a follow-up bug.

(In reply to Tooru Fujisawa [:arai] from comment #19) > Created attachment 8975689 [details] [diff] [review] > Support LazyScript in Debugger.Script. I'm very excited to see this work, but this patch is an awful lot to digest in one go. Would it be possible to divide it up into a series of patches that can be reviewed in order? The series should be landable: although earlier patches may contain features that aren't used until later in the series, each intermediate step should compile and pass tests. This would allow me to review the patch more quickly, by making the dependencies between the changes easier to see. And I think it reduces opportunities for mistakes. From what I understand, perhaps the following breakdown would work: - IterateLazyScripts - Support for using LazyScript as a weakmap key (I think this is the MovableCellHasher and the WeakMap changes) - TraceManuallyBarrieredCrossCompartmentEdge - LazyScript::compartment - Let Debugger.Script refer to LazyScripts. This will still be a big patch, but it all needs to land together. - add LazyScript variant - adapt Debugger.Script methods to handle LazyScripts, where possible - LazyScriptVector (if this is trivial, then it can be folded into the next) - findScripts changes: - delazify only if necessary - collect LazyScripts, if any exist - fix tests that expect delazification - Allow lazy parses in Debugger's eval - remove unnecessary JS::ExposeScriptToActiveJS call

Thank you for reviewing :) then, actually the try wasn't green. https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c19902597fe5aa27b251778d0161493f6fc77ac&selectedJob=178288107 the leak seems to be caused by wrapping LazyScript. (some trace or something isn't handled properly, I think?) I'll investigate the issue.

Here's separated patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=def2292592b7a26005897fdc47ca578af1683b01&selectedJob=178682343 and the leak happens from Part 8, which wraps the corresponding LazyScript for JSScript: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ec48b5fd3ea35ac1cef382ad72b3951835b98427&selectedJob=178780456 the failing test seems to be dom/base/test/jsmodules/test_importIntroType.html https://searchfox.org/mozilla-central/source/dom/base/test/jsmodules/test_importIntroType.html which uses Debugger inside iframe: https://searchfox.org/mozilla-central/source/dom/base/test/jsmodules/iframe_extractIntroType.html if anyone have hit similar issue before and have any hint where to look into, let me know :)

So, the LazyScript keeps returning false for IsAboutToBeFinalizedUnbarriered [1], that results in keeping the compartment (which CrossCompartmentKey with Debugger+LazyScript is put into) alive [2]. for the LazyScript, IsAboutToBeFinalizedUnbarriered is calling IsAboutToBeFinalizedDuringSweep [3], and that returns false. jonco, can I have some hint where to look into next, to figure out why the LazyScript is marked? [1] https://searchfox.org/mozilla-central/rev/8affe6e83188787eb61fe0528eeb6eef6081ba06/js/src/gc/Marking.cpp#3394 [2] https://searchfox.org/mozilla-central/rev/8affe6e83188787eb61fe0528eeb6eef6081ba06/js/src/vm/JSCompartment.h#432 [3] https://searchfox.org/mozilla-central/rev/8affe6e83188787eb61fe0528eeb6eef6081ba06/js/src/gc/Marking.cpp#3385

when I run the mochitest with --keep-open, and wait a little before shutting down, the leak issue disappears, (the LazyScript returns true for IsAboutToBeFinalizedUnbarriered) also, when I compare the GC/CC graph before closing the testcase HTML, there's no notable difference. (except that Debugger.Script holds LazyScript instead of JSScript) and when I close the testcase tab and then run GC/CC, the LazyScript disappears from the GC/CC log. so I guess it's more like timing issue (instead of actual leak cause by something holds LazyScript too long), maybe? or perhaps we have some special path for JSScript, to avoid such issue?

err, maybe I found the issue. while I was trying to log GC/CC, it crashed because LazyScript wasn't listed here https://searchfox.org/mozilla-central/rev/8affe6e83188787eb61fe0528eeb6eef6081ba06/xpcom/base/CycleCollectedJSRuntime.h#413-420 > // Returns true if the JS::TraceKind is one the cycle collector cares about. > inline bool AddToCCKind(JS::TraceKind aKind) > { > return aKind == JS::TraceKind::Object || > aKind == JS::TraceKind::Script || > aKind == JS::TraceKind::Scope || > aKind == JS::TraceKind::RegExpShared; > } and I added LazyScript there (only intended quick workaround for the crash) and started logging. and from then, the issue seems to be disappeared. I'll retrigger try run and ask review if it passes.

(In reply to Tooru Fujisawa [:arai] from comment #27) > > // Returns true if the JS::TraceKind is one the cycle collector cares about. > > inline bool AddToCCKind(JS::TraceKind aKind) Do you understand why LazyScript needs to be included in this list? I had never seen AddToCCKind before, and I don't really understand what it does.

(In reply to Jim Blandy :jimb from comment #28) > (In reply to Tooru Fujisawa [:arai] from comment #27) > > > // Returns true if the JS::TraceKind is one the cycle collector cares about. > > > inline bool AddToCCKind(JS::TraceKind aKind) > > Do you understand why LazyScript needs to be included in this list? I had > never seen AddToCCKind before, and I don't really understand what it does. I haven't yet investigated. but I believe there should be some hint why the leak was happening and why the leak disappeared by the change. I'll look into this today, before asking review. at least, try run looks almost green (no leak) https://treeherder.mozilla.org/#/jobs?repo=try&revision=46ed89b4b0140b34462fc8f74dc8e9909983023f&group_state=expanded

Corresponds to JSScript::{compartment,realm}, which is called in some place around debugger etc. I used JSFunction's {compartment,realm} as return value, since LazyScript doesn't have them.

Corresponds to IterateScripts. The implementation is merged into template, since it's same for JSScript and LazyScript. (given that now LazyScript has components() method)

Corresponds to Debugger::LazyScriptWeakMap and Debugger.scripts. The change in CycleCollectedJSRuntime.h is necessary to handle LazyScript as WeakMap key (`LazyScriptWeakMap lazyScripts` added in Part 8). The following assertion fails if I don't change and I hit "Save verbose" button in about:memory, after running dom/base/test/jsmodules/test_importIntroType.html with `mach mochitest --keep-open`. https://searchfox.org/mozilla-central/rev/2aa42f2cab4a110edf21dd7281ac23a1ea8901f9/xpcom/base/CycleCollectedJSRuntime.cpp#198-202 > void > NoteWeakMapsTracer::trace(JSObject* aMap, JS::GCCellPtr aKey, > JS::GCCellPtr aValue) > { > ... > // The cycle collector can only properly reason about weak maps if it can > // reason about the liveness of their keys, which in turn requires that > // the key can be represented in the cycle collector graph. All existing > // uses of weak maps use either objects or scripts as keys, which are okay. > MOZ_ASSERT(AddToCCKind(aKey.kind()));

This is also called in Part 8.

Corresponds to CrossCompartmentKey with the pair of Debugger and JSScript, which is used in Debugger::wrapVariantReferent.

Currently, delazification happens only if the enclosing script is not lazy, but when we support LazyScript in debugger, we have to delazify LazyScript (to get JSScript-specific data) even if the enclosing script is also lazy. This requires delazification of enclosing script recursively. EnclosingLazyScriptFinder is a class to find enclosing LazyScript, which iterates over all LazyScript instances, and look for the LazyScript which has the given LazyScript as inner function's script. It does so because currently we don't have direct reference from inner LazyScript to enclosing LazyScript, but only the reference from enclosing LazyScript to inner functions, and function to corresponding LazyScript. (If this operation is found to be slow, we should consider adding direct reference from LazyScript to enclosing LazyScript, or similar thing around JSFunction) Then, DelazifyScript delazifies the given LazyScript, using EnclosingLazyScriptFinder. If the enclosing script is also lazy, it recursively delazifies the enclosing script.

* Added LazyScript to DebuggerScriptReferent * Added LazyScript in Debugger.Script accessors/methods * If the property is available in LazyScript, returns it * If the property is not available in LazyScript, delazify it and perform it on JSScript * uncomment "static" for DelazifyScript since now it's called

* Support LazyScript in Debugger::wrapVariantReferent * as mentioned above, it wraps LazyScript of JSScript if there is * Added Debugger::LazyScriptWeakMap which corresponds to Debugger::ScriptWeakMap and Debugger.lazyScripts which corresponds to Debugger.scripts. which holds the reference to wrapped lazyScripts * Added Debugger::wrapLazyScript which corresponds to Debugger::wrapScript.

Just as a preparation to add LazyScript variant, changed the field name `vector` to `scriptVector` for clarification.

Debugger::ScriptQuery::findScripts now: * doesn't always delazify scripts, but only if the query requires JSScript-specific data * iterates over LazyScripts, and returns wrapped LazyScript.

Now Debugger supports LazyScript, that means we can lazily parse the eval code for debugger.

Removed unnecessary JS::ExposeScriptToActiveJS call, per comment #13

While debugging the leak around WeakMap key, it was important to log LazyScript's filename+line number in GC/CC log, so I'd like to add it in-tree.

Comment on attachment 8979449 [details] [diff] [review] Part 3: Support LazyScript in WeakMap. Review of attachment 8979449 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/base/CycleCollectedJSRuntime.h @@ +414,5 @@ > +// Everything used as WeakMap key should be listed here, to represent the key > +// in cycle collector's graph, otherwise the key is considered to be pointed > +// from somewhere unknown, and results in leaking the subgraph which contains > +// the key. > +// See the comments in NoteWeakMapsTracer::trace for more details. This is great. My version of this comment is in bug 1463343. (Your patches should have priority for landing, and I'm hoping mccr8 will come up with the final wording of the comment here.)

Comment on attachment 8979453 [details] [diff] [review] Part 6: Add DelazifyScript to delazify a certain LazyScript and its all ancestor scripts recursively. Review of attachment 8979453 [details] [diff] [review]: ----------------------------------------------------------------- The code looks fine, but there's something about this patch that worries me. Is it ever possible that a LazyScript's parent could be GC'd, while the LazyScript itself lives on? With Debugger.prototype.findScripts returning Debugger.Script objects that refer to LazyScripts, this certainly seems possible, unless a LazyScript somehow retains its parent. If a LazyScript being live does cause its parent to be retained, then it should be possible to implement DelazifyScript without scanning arenas, simply by following whatever links the LazyScript uses to point to its parent. On the other hand, if a LazyScript being live does not cause its parent to be retained, then the assertion `MOZ_ASSERT(enclosingLazyScript_);` could fail, if a LazyScript's parent simply doesn't exist any more. ::: js/src/vm/Debugger.cpp @@ +5222,5 @@ > + MOZ_ASSERT(enclosingLazyScript_); > + return enclosingLazyScript_; > + } > + > + static void considerLazyScript(JSRuntime* rt, void* data, LazyScript* lazyScript, This could be private, right?

Comment on attachment 8979455 [details] [diff] [review] Part 7: Support LazyScript variant in DebuggerScriptReferent, and support LazyScript in Debugger.Script accessors and methods. Review of attachment 8979455 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, just some minor changes to request. ::: js/src/vm/Debugger.cpp @@ +5398,2 @@ > static JSObject* > DebuggerScript_checkThis(JSContext* cx, const CallArgs& args, const char* fnname, There is only one use of DebuggerScript_checkThis, so let's make it an ordinary non-template function instead. @@ +5450,5 @@ > + args.rval().setBoolean(script->isGenerator()); > + } else { > + LazyScript* lazyScript = GetScriptReferent(obj).as<LazyScript*>(); > + args.rval().setBoolean(lazyScript->isGenerator()); > + } Do you think using a helper function like this would make these functions more legible, overall? template<typename Result> Result CallScriptMethod(HandleObject obj, Result (JSScript::*ifJSScript)(), Result (LazyScript::*ifLazyScript)()) { if (GetScriptReferent(obj).is<JSScript*>()) { JSScript* script = GetScriptReferent(obj).as<JSScript*>(); return script->*ifJSScript(); } LazyScript* lazyScript = GetScriptReferent(obj).as<LazyScript*>(); return lazyScript->*ifLazyScript(); } For example: static bool DebuggerScript_getIsGeneratorFunction(JSContext* cx, unsigned argc, Value* vp) { THIS_DEBUGSCRIPT_SCRIPT_MAYBE_LAZY(cx, argc, vp, "(get isGeneratorFunction)", args, obj); args.rval().setBoolean(CallScriptMethod(obj, &JSScript::isGenerator, &LazyScript::isGenerator); return true; } I toyed with the idea of just passing in two closures, but in the end it doesn't really seem shorter or more legible.

Jan, do you know the answer to the question I raised in comment 44?

Continuing to think about the GC'd lazy parent problem: The current CreateLazyScriptsForCompartment code sidesteps this problem in a subtle way. It only builds a list of "live *root* lazy functions": https://searchfox.org/mozilla-central/rev/eb51d734a8c06ac5973291be9bcc74ed4e999bca/js/src/vm/JSCompartment.cpp#1107 In other words, it starts with LazyScripts for which isEnclosingScriptLazy is false, and then delazifies them and their children. So if there were ever a LazyScript whose parent had been lazy and was GC'd, the current code will never delazify it, so it will never be visible to the present Debugger.

(In reply to Jim Blandy :jimb from comment #46) > Jan, do you know the answer to the question I raised in comment 44? I noticed there was some IRC discussion yesterday so I assume you got the answer? FWIW, we never *relazify* non-leaf functions (functions with inner functions), but I think the scenario you mentioned is probably possible when there are multiple layers of lazy scripts and you have a pointer to one of the lazy "grandkids".

I'll think about storing back-pointer from inner LazyScript to outer LazyScript or JSFunction or something, so that we don't have to worry about GC'd parent. at first I was thinking we can replace enclosingScope_ pointer with LazyScript or JSFunction, but it's not possible when the enclosing scope is global, so I should come up with something different. (or, perhaps we can store LazyScript and enclosing global scope in the same pointer and distinguish between them with TraceKind?)

I forgot to handle one more case, that is hasUncompletedEnclosingScript case. in that case LazyScript.enclosingScope_ is non-null, but we cannot compile the LazyScript because the enclosing script failed to compile and the data is broken. So, we should compile enclosing script in the following 2 cases: * if enclosing script is lazy * if enclosing script's compilation failed and current code doesn't check the latter case. I'll fix the case after bug 1463979

I think if a LazyScript has an uncompleted enclosing script, findScripts should not report it at all. Delazification happens at times that the Debugger.Script user can't predict, so it needs to be infallible (except for OOM).

I think, we shouldn't iterate all LazyScript in arenas, but iterate JSScript and filter out uncompleted ones, and then traverse the tree from JSScript to find out all live LazyScripts. so that all enclosing scripts are known to be either completed JSScript, or LazyScript which syntax parse is finished. I'll rewrite that part (IterateScripts and IterateLazyScripts)

jimb, do you think we should filter out JSScript which ancestor script is uncompleted? so far I don't see any problem due to touching such script from debugger, but basically they have been thrown away while compiling, and are just waiting for GC, so there's no point of exposing them.

looks like I completely misunderstood the relation between the lifetime of enclosing script and inner script. the enclosing JSScript can be GCed while keeping the inner LazyScript alive which is pointed by something else. (I thought the enclosing JSScript is somehow kept alive) That means LazyScript is delazify-able as long as the scope is there, even if the enclosing script isn't there. is it correct? in that case, the way I described in comment #52 doesn't work, and what we should do is, collect LazyScripts which has enclosing scope pointer (what CreateLazyScriptsForRealm does), and then traverse the innerFunctions tree from those LazyScripts.

updated patches based on bug 1463979. https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d03313dc12ec5b1ab70c9a6fc8f7e0e263e2c73&group_state=expanded

clearing review requests for now. will post them again once bug 1463979 is fixed.

Are these ready to review, now that bug 1463979 has landed?

I'll post rebased patches (14 files)

Changed IterateLazyScripts to do the following: * iterate over LazyScripts in the arenas, and * call the callback for the LazyScript is it has enclosing scope * traverse the innerFunctions tree and call the callback for corresponding LazyScripts Also moved the `script->isUncompiled()` check into IterateScripts's side, since the function comment says "in-use script".

Now that LazyScript has back-pointer to the enclosing LazyScript if the enclosing script have never been compiled. So we can just use the pointer to get enclosing LazyScript and recursively delazify them.

Changed accessors to use CallScriptMethod. And also added {JSScript,LazyScript}::sourceLength to simplify sourceLength accessor.

* Support LazyScript in Debugger::wrapVariantReferent * as mentioned above, it wraps LazyScript of JSScript if there is * Added Debugger::LazyScriptWeakMap which corresponds to Debugger::ScriptWeakMap and Debugger.lazyScripts which corresponds to Debugger.scripts. which holds the reference to wrapped lazyScripts * Added Debugger::wrapLazyScript which corresponds to Debugger::wrapScript.

Just as a preparation to add LazyScript variant, changed the field name `vector` to `scriptVector` for clarification.

Debugger::ScriptQuery::findScripts now: * doesn't always delazify scripts, but only if the query requires JSScript-specific data * iterates over LazyScripts, and returns wrapped LazyScript.

Now Debugger supports LazyScript, that means we can lazily parse the eval code for debugger.

Removed unnecessary JS::ExposeScriptToActiveJS call, per comment #13

While debugging the leak around WeakMap key, it was important to log LazyScript's filename+line number in GC/CC log, so I'd like to add it in-tree.

Added tests that checks the uncompleted scripts and its inner functions are not exposed to debugger. The test is using "too many switch cases" error, which is thrown while emitting bytecode, that results in JSScript's code being null. Then, since scripts created while parsing and partial-compiling can be GCed (because nothing keeps them alive), those tests only check that there's no unexpected scripts exposed.

Comment on attachment 8993153 [details] [diff] [review] Part 14: Add testcases. Review of attachment 8993153 [details] [diff] [review]: ----------------------------------------------------------------- Great stuff. I love the ${"case 1:".repeat(2**16+1)} trick.

Comment on attachment 8993139 [details] [diff] [review] Part 3: Support LazyScript in WeakMap. r=jimb,sfink Review of attachment 8993139 [details] [diff] [review]: ----------------------------------------------------------------- Just a random thought as I remind myself of what's going on in this patch series: ::: xpcom/base/CycleCollectedJSRuntime.h @@ +410,5 @@ > > void TraceScriptHolder(nsISupports* aHolder, JSTracer* aTracer); > > // Returns true if the JS::TraceKind is one the cycle collector cares about. > +// Everything used as WeakMap key should be listed here, to represent the key It would be nice if we could make it a compile-time error if someone uses a type as a WeakMap key without adding it here. I can't quite figure out how one would do that, though.

Comment on attachment 8993143 [details] [diff] [review] Part 6: Add DelazifyScript to delazify a certain LazyScript and its all ancestor scripts recursively. Review of attachment 8993143 [details] [diff] [review]: ----------------------------------------------------------------- Looks nice. I wish AutoRealm accepted LazyScript directly.

Comment on attachment 8993143 [details] [diff] [review] Part 6: Add DelazifyScript to delazify a certain LazyScript and its all ancestor scripts recursively. Review of attachment 8993143 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/Debugger.cpp @@ +5180,5 @@ > } > } > } > > +/* static */ JSScript* This is fixed in the next patch, but this shouldn't be commented out.

(In reply to Jim Blandy :jimb from comment #75) > Comment on attachment 8993143 [details] [diff] [review] > Part 6: Add DelazifyScript to delazify a certain LazyScript and its all > ancestor scripts recursively. > > Review of attachment 8993143 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: js/src/vm/Debugger.cpp > @@ +5180,5 @@ > > } > > } > > } > > > > +/* static */ JSScript* > > This is fixed in the next patch, but this shouldn't be commented out. I've commented it out in order to avoid warning for the unused function, and make it compilable. the function is not used at that point, and the consumer is added later.

Comment on attachment 8993145 [details] [diff] [review] Part 7: Support LazyScript variant in DebuggerScriptReferent, and support LazyScript in Debugger.Script accessors and methods. r=jimb Review of attachment 8993145 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/Debugger.cpp @@ +5182,5 @@ > } > } > } > > +static JSScript* Oops, missed this when I reviewed the prior patch @@ +5372,5 @@ > +template <typename Result> > +Result > +CallScriptMethod(HandleObject obj, > + Result (JSScript::*ifJSScript)() const, > + Result (LazyScript::*ifLazyScript)() const) I feel like our dynamic dispatch techniques are multiplying out of control. But the uses of this look nice and neat. @@ +5500,5 @@ > + ReturnType match(Handle<LazyScript*> lazyScript) { > + RootedScript script(cx_, DelazifyScript(cx_, lazyScript)); > + if (!script) > + return false; > + return match(script); lol, this tiny recursion is nice. Since we do it in so many places, would it be possible to hoist it out into a common base class for matchers? You'd probably need to use CRTP. @@ +5540,5 @@ > &UncheckedUnwrap(script->sourceObject())->as<ScriptSourceObject>()); > return dbg_->wrapSource(cx_, source); > } > + ReturnType match(Handle<LazyScript*> lazyScript) { > + RootedScriptSourceObject source(cx_, &lazyScript->sourceObject()); Why do we not need to call UncheckedUnwrap here? Needs a comment, at least.

Comment on attachment 8993147 [details] [diff] [review] Part 8: Support wrapping LazyScript in DebuggerScriptReferent. Review of attachment 8993147 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/Debugger.cpp @@ +5290,5 @@ > + if (untaggedReferent->maybeLazyScript()) { > + // If the JSScript has corresponding LazyScript, wrap the LazyScript > + // instead. > + // > + // This is necessary for Debugger.Object identity. If we use both "Debugger.Script identity", right? @@ +5297,5 @@ > + // actually identical. > + // > + // If a script has corresponding LazyScript and JSScript, the > + // lifetime of the LazyScript is always longer than the JSScript. > + // So we can use the LazyScript like as a proxy for the JSScript. s/like as/as/

Comment on attachment 8993148 [details] [diff] [review] Part 9: Rename Debugger::ScriptQuery::vector to Debugger::ScriptQuery::scriptVector. Review of attachment 8993148 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for splitting out these mechanical patches.

Comment on attachment 8993151 [details] [diff] [review] Part 12: Remove JS::ExposeScriptToActiveJS call on scripts returned by IterateScripts. Review of attachment 8993151 [details] [diff] [review]: ----------------------------------------------------------------- If jonco says it, that's good enough for me.

(In reply to Tooru Fujisawa [:arai] from comment #76) > I've commented it out in order to avoid warning for the unused function, and > make it compilable. > the function is not used at that point, and the consumer is added later. Oh --- that makes sense. I wasn't expecting that.

(In reply to Tooru Fujisawa [:arai] from comment #53) > jimb, do you think we should filter out JSScript which ancestor script is > uncompleted? > so far I don't see any problem due to touching such script from debugger, > but basically they have been thrown away while compiling, and are just > waiting for GC, > so there's no point of exposing them. Yes, we should definitely filter out such scripts. If I understand the way we're iterating over lazy scripts at the moment, the patch series as it stands will not report them. If that is not right, please let me know.

Thank you for reviewing :D (In reply to Jim Blandy :jimb from comment #82) > (In reply to Tooru Fujisawa [:arai] from comment #53) > > jimb, do you think we should filter out JSScript which ancestor script is > > uncompleted? > > so far I don't see any problem due to touching such script from debugger, > > but basically they have been thrown away while compiling, and are just > > waiting for GC, > > so there's no point of exposing them. > > Yes, we should definitely filter out such scripts. If I understand the way > we're iterating over lazy scripts at the moment, the patch series as it > stands will not report them. If that is not right, please let me know. it reports LazyScript where its nearest ancestor JSScript is completed, regardless of the more ancestor's state, so there can be an ancestor JSScript which is uncompleted. for example, suppose that the following script fails compiling because of the too-many-cases error, while compiling nonLazy1. (function nonLazy1() { (function nonLazy2() { (function nonLazy3() { function lazy1() { function lazy2() { } } })(); })(); switch (x) { case 1: .... } })(); nonLazy2 and nonLazy3 are already compiled before hitting the switch-case, and its inner lazy1 and lazy2 are also instantiated as LazyScript. when iterating over JSScript, it look for JSScript where itself is completed, so it finds nonLazy2 and nonLazy3, and reports them. when iterating over LazyScript, it first look for LazyScript where its parent is completed JSScript, so it finds lazy1 and report it, and then traverses its children, so it finds lazy2 and report it.

On IRC, Arai said that, as far as they know, the behavior described in comment 83 can also occur now on Mozilla Central, with only JSScripts. If it's a pre-existing bug, then we should file a bug for it, but there's no need for it to block this work. D.p.findScripts has a bad record of exposing partially-initialized values, and should be replaced with something more well-behaved.

(In reply to Jim Blandy :jimb from comment #77) > @@ +5540,5 @@ > > &UncheckedUnwrap(script->sourceObject())->as<ScriptSourceObject>()); > > return dbg_->wrapSource(cx_, source); > > } > > + ReturnType match(Handle<LazyScript*> lazyScript) { > > + RootedScriptSourceObject source(cx_, &lazyScript->sourceObject()); > > Why do we not need to call UncheckedUnwrap here? Needs a comment, at least. because JSScript can be cloned, and in that case we create wrapper for the original one: https://searchfox.org/mozilla-central/rev/943a6cf31a96eb439db8f241ab4df25c14003bb8/js/src/vm/JSScript.cpp#3632-3665 and that kind of thing doesn't happen for LazyScript and we hold the reference to raw ScriptSourceObject. I'll add comment about that to both match methods.

https://hg.mozilla.org/integration/mozilla-inbound/rev/1a5bdbbfc37535b866b79015bbe849bf8f6e42bd Bug 1434305 - Part 1: Add LazyScript::{compartment,realm} which returns corresponding JSFunction's {compartment,realm}. r=jimb https://hg.mozilla.org/integration/mozilla-inbound/rev/dea04c3e53869104c0e79e6b5ecc4cd9df2f0d17 Bug 1434305 - Part 2: Add IterateLazyScripts. r=jimb https://hg.mozilla.org/integration/mozilla-inbound/rev/49f82b7a2cb1991eec2db836efca5762d1e50a06 Bug 1434305 - Part 3: Support LazyScript in WeakMap. r=jimb,sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/8cdbd389b55de9123fc24167279c9d335e729336 Bug 1434305 - Part 4: Instantiate TraceManuallyBarrieredCrossCompartmentEdge template for LazyScript. r=jimb https://hg.mozilla.org/integration/mozilla-inbound/rev/9ef73c62f110d41e9ee4b805ebfb225fac88fdc9 Bug 1434305 - Part 5: Support the pair of Debugger and LazyScript in CrossCompartmentKey. r=jimb https://hg.mozilla.org/integration/mozilla-inbound/rev/cac8918606978b862db740fb865f4e155e442125 Bug 1434305 - Part 6: Add DelazifyScript to delazify a certain LazyScript and its all ancestor scripts recursively. r=jimb https://hg.mozilla.org/integration/mozilla-inbound/rev/ec8c69da8281a0798132b68b82accf0ab241df9d Bug 1434305 - Part 7: Support LazyScript variant in DebuggerScriptReferent, and support LazyScript in Debugger.Script accessors and methods. r=jimb https://hg.mozilla.org/integration/mozilla-inbound/rev/b67ea788a6f351a7c3c5347bc91c7c2a8741be77 Bug 1434305 - Part 8: Support wrapping LazyScript in DebuggerScriptReferent. r=jimb https://hg.mozilla.org/integration/mozilla-inbound/rev/7f0c5ef819a50e3ad5fbca73f169cae78e34f337 Bug 1434305 - Part 9: Rename Debugger::ScriptQuery::vector to Debugger::ScriptQuery::scriptVector. r=jimb https://hg.mozilla.org/integration/mozilla-inbound/rev/351618a4e6a7c542d8b7d70d7e2bc9d95cb6510b Bug 1434305 - Part 10: Support LazyScript in Debugger::ScriptQuery::findScripts. r=jimb https://hg.mozilla.org/integration/mozilla-inbound/rev/103ff157601000c175c987abdf43d42a83650c41 Bug 1434305 - Part 11: Support Lazy Parsing in Debugger's eval environment. r=jimb https://hg.mozilla.org/integration/mozilla-inbound/rev/32dd0504e5972bef289c5b25cbc19a6351a9e59b Bug 1434305 - Part 12: Remove JS::ExposeScriptToActiveJS call on scripts returned by IterateScripts. r=jimb,f=jonco https://hg.mozilla.org/integration/mozilla-inbound/rev/960a1f20a4117ed597bd89238c0cc53d4be4b5c3 Bug 1434305 - Part 13: Print LazyScript filename and line number in GC log. r=jimb https://hg.mozilla.org/integration/mozilla-inbound/rev/697902c363da639e5171e9af352b43aeab324e87 Bug 1434305 - Part 14: Add testcases. r=jimb

https://hg.mozilla.org/mozilla-central/rev/1a5bdbbfc375 https://hg.mozilla.org/mozilla-central/rev/dea04c3e5386 https://hg.mozilla.org/mozilla-central/rev/49f82b7a2cb1 https://hg.mozilla.org/mozilla-central/rev/8cdbd389b55d https://hg.mozilla.org/mozilla-central/rev/9ef73c62f110 https://hg.mozilla.org/mozilla-central/rev/cac891860697 https://hg.mozilla.org/mozilla-central/rev/ec8c69da8281 https://hg.mozilla.org/mozilla-central/rev/b67ea788a6f3 https://hg.mozilla.org/mozilla-central/rev/7f0c5ef819a5 https://hg.mozilla.org/mozilla-central/rev/351618a4e6a7 https://hg.mozilla.org/mozilla-central/rev/103ff1576010 https://hg.mozilla.org/mozilla-central/rev/32dd0504e597 https://hg.mozilla.org/mozilla-central/rev/960a1f20a411 https://hg.mozilla.org/mozilla-central/rev/697902c363da