1592155 - [jsdbg2] Directly mark Debugger.Frames for generators

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P2)

Description

[Jim Blandy :jimb]

SpiderMonkey's algorithm for deciding whether a Debugger.Frame for an async or generator call can be garbage collected is needlessly complex, and should be simplified.

If there is a Debugger.Frames referring to a suspended async or generator call, and it has an onStep or onPop handler set, and the generator object for the call is reachable, then garbage-collecting the Debugger.Frame would be incorrect, even if it is not otherwise reachable. GC should not have observable side effects, and removing the Debugger.Frame would cause its hooks never to fire, even though the generator can be resumed.

At present, we implement this behavior by incorporating Debugger-specific code into the garbage collector's weak marking loop:

• js::gc::GCRuntime::markWeakReferences repeatedly calls js::DebugAPI::markIteratively.

• markIteratively scans all debuggee realms in the runtime, looking for marked globals.

  • If a marked global is a debuggee, then we iterate over its Debuggers.

    • If a Debugger is not marked, but has any Debugger.Frames referring to generator objects that are marked, and those Debugger.Frames have onPop or onStep hooks set, then we mark the Debugger.

    • Marking a Debugger marks its generatorFrames weak map, which maps generator objects to the Debugger.Frames that represent them.

    • The normal weak map marking process then marks those Debugger.Frames whose generator objects are marked.

• If the above process caused any new Debuggers to be marked, then we make another iteration around the loop, to see if anything interesting has become reachable.

All this is equivalent to having a strong cross-compartment edge from a generator object to any Debugger.Frames representing it that have hooks set. We can implement this as follows:

• When a generator object is traced, we should call a new DebugAPI method to trace Debugger.Frames that refer to it and that have hooks set.

• This DebugAPI method should first check whether the generator's realm is a debuggee, and whether its script has a non-zero generatorObserverCount. If either is not the case, the generator must have no Debugger.Frames referring to it, and we can return immediately. (Since this affects the way every generator object is traced, we may want to split this into a fast path and slow path. The fast path could return immediately if the realm is not a debuggee, since that check is quick.)

• Otherwise, we should search the generator's realm's debugger vector for a Debugger with a Debugger.Frame for the generator. (There will generally only be one Debugger, and the search for the frame is a hash lookup, so this shouldn't be a performance problem in practice.) We should not trace the Debugger or the Debugger.Frame yet.

• If there is a Debugger.Frame, and it has hooks set, we should trace it.

• Tracing a Debugger.Frame object traces its owning Debugger via its OWNER_SLOT reserved slot, so the Debugger will get marked, just as before.

Note that AbstractGeneratorObject has multiple subclasses, which may have their own JSClassOps trace hooks, so there may be more than one function that must be updated to call the new DebugAPI method.

Comment 1

[Jim Blandy :jimb]

Attachment: Bug 1592155: Trace Debugger.Frames for suspended generator/async calls directly. r?jonco

For collection purposes, there must be an owning edge from an
AbstractGeneratorObject to each Debugger.Frame with hooks set that refers to
it. If a Debugger.Frame refers to a suspended generator or async call, and the
Debugger.Frame has onStep or onPop hooks set, and the call's
AbstractGeneratorObject is reachable, the garbage collector must retain the
Debugger.Frame: collecting it would cause the hooks not to fire, which would
be visible to JavaScript.

At the moment, we obtain the effect of that owning edge in a needlessly
circuitous way. The iterative marking loop in GCRuntime::markWeakReferences
calls DebugAPI::markIteratively to scan all Debugger objects with live
debuggee globals to see if they have any Debugger.Frames for marked
AbstractGeneratorObjects. If they do, then we mark the Debugger, which marks
the Debugger.Frames and causes markIteratively to return true, so we go
around the markWeakReferences loop again.

With this patch, tracing an AbstractGeneratorObject in a debuggee realm checks
the Debuggers weak maps for Debugger.Frames, and marks them. The edge is
still only inferred (via the Realm's isDebuggee flag; its list of Debuggers; and
the Debuggers' tables of generator frames) rather than being represented
directly as a pointer, but overall, this is much closer to ordinary tracing
behavior: tracing one object causes more objects to become markable.

Hg: Enter commit message. Lines beginning with 'HG:' are removed.

Comment 2

[Jim Blandy :jimb]

Attachment: Bug 1592155: Make TraceCrossCompartmentEdge handle JSObject subclasses.

Give TraceCrossCompartmentEdge an inline definition in Tracer.h that uses
gc::ConvertToBase and unsafeUnbarrieredForTracing (already in use by other
definitions in that file) to bring things to the point that
TraceManuallyBarrieredCrossCompartmentEdge can finish the job.

This gives us a definition that handles WriteBarriered<T> for pointers to
subclasses of JSObject, so we can use tighter types in data structures.

Remove the non-inline definition and explicit template instantiations in
Marking.cpp.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/59c3668969ef
Make TraceCrossCompartmentEdge handle JSObject subclasses. r=jonco
https://hg.mozilla.org/integration/autoland/rev/9f5acf34610f
Trace Debugger.Frames for suspended generator/async calls directly. r=jonco

Comment 4

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/59c3668969ef
https://hg.mozilla.org/mozilla-central/rev/9f5acf34610f