[jsdbg2] Directly mark Debugger objects with hooks set
Categories
(Core :: JavaScript Engine, enhancement, P2)
Tracking
()
Tracking | Status | |
---|---|---|
firefox73 | --- | fixed |
People
(Reporter: jimb, Assigned: loganfsmyth)
References
Details
Attachments
(1 file, 1 obsolete file)
SpiderMonkey's algorithm for deciding whether a Debugger
can be garbage collected is needlessly complex, and should be simplified.
If a Debugger has an onDebuggerStatement
, onExceptionUnwind
, onNewScript
, or onEnterFrame
hook set, and it has live debuggees, then it would be incorrect to collect the Debugger
even if it is not otherwise reachable. GC should not have observable side effects, and removing the Debugger
would cause the hooks not to fire.
At present, we implement this behavior by incorporating Debugger
-specific behavior into the garbage collector's weak marking loop:
-
js::gc::GCRuntime::markWeakReferences
repeatedly callsjs::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
Debugger
s.- If a
Debugger
is not marked, but has any of the hooks mentioned above set (according toDebugger::hasAnyLiveHooks
), then we mark it.
- If a
-
-
If the above process caused any new
Debugger
s to be marked, then we make another iteration around the loop.
The effect of all this is equivalent to having a strong cross-compartment reference from each debuggee realm to each of its Debugger
s that has any hooks set. We can implement this as follows:
-
Let
Realm::traceGlobal
iterate over the theRealm
'sDebuggerVector
:- If a
Debugger
has any of the hooks mentioned above set, then it should be traced. (Debugger
s without hooks set should not be traced!)
- If a
-
Since all interactions between the debugger and the rest of SpiderMonkey should go through
DebugAPI
, we'll need a newDebugAPI
method fortraceGlobal
to use that takes aDebugger*
and returns true if it has any of those hooks set. -
Debugger::hasAnyLiveHooks
should no longer check for those hooks.
At present, Debugger::hasAnyLiveHooks
also takes care of looking for suspended generator frames with hooks set, which also must cause their owning Debugger
s to be retained, so we can't delete that function entirely. However, this change leaves that function with a misleading name; perhaps Debugger::hasSuspendedGeneratorHooks
would be better.
Reporter | ||
Updated•5 years ago
|
Reporter | ||
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Updated•5 years ago
|
Assignee | ||
Comment 1•5 years ago
|
||
Assignee | ||
Comment 2•5 years ago
|
||
Hey Jim, I'd love to chat up about this tomorrow. Before I dive into questions though, I want to make sure I followed roughly what you were saying. Is this what you had in mind? https://phabricator.services.mozilla.com/D51097
I'm running into two issues and I want your thoughts:
-
If
traceGlobal
in the Realm traces the edge intoDebugger
, it fails some assertions because it isn't in the cross-compartment map. Is that something that I should be explicitly adding to the map now, or should we be editingDebugAPI::edgeIsInDebuggerWeakmap
(and probably renaming it), so that these cross-compartment links work? The patch above has that logic, but not sure it's the right way to do it. -
There seems to be an issue with the order of marking and sweeping and I'm at a bit of a loss. For generator frames, we only want the to hold a strong link to the Debugger.Frame as long as there is as the AbstractGeneratorObject is alive. What should be responsible for that? It looks like, previously, this was done as a side-effect of
forEachWeakMap([trc](auto& weakMap) { weakMap.trace(trc); });
because by the time that executed, the traversal will have already marked the generator, so marking the WeakMap will also mark the DebuggerFrame.
Because we've moved the call to Debugger::trace
call from markIteratively
to traceGlobal
, it seems that the AbstractGeneratorObject
has not yet been marked, meaning that nothing ever marks the Debugger.Frame. That is causing js/src/jit-test/tests/debug/Frame-onStep-async-gc-01.js
to fail with my current patch.
Reporter | ||
Comment 3•5 years ago
|
||
(In reply to Logan Smyth [:loganfsmyth] from comment #2)
- If
traceGlobal
in the Realm traces the edge intoDebugger
, it fails some assertions because it isn't in the cross-compartment map. Is that something that I should be explicitly adding to the map now, or should we be editingDebugAPI::edgeIsInDebuggerWeakmap
(and probably renaming it), so that these cross-compartment links work? The patch above has that logic, but not sure it's the right way to do it.
Yes, we are replacing a situation where one thing's liveness has been indirectly arranged to ensure another thing's liveness with a simple owning edge, so that's going to trip tests that are concerned with edges, hmm.
Changing edgeIsInDebuggerWeakmap
is certainly one approach. (Agree about the need to rename.)
Another would be for Realm::debuggees_
to hold cross-compartment wrappers to the Debugger
s' JSObjects. Then all uses of that array would need to unwrap. But I think unwrapping is forbidden during GC.
Another issue I noticed: I think the type of Realm::DebuggerVector
may need to change. WeakHeapPtr<Debugger*>
ensures that any read of that pointer calls Debugger::readBarrier
to mark the referent (if you don't know why this behavior is usually needed for weak pointers, see the "GC Barriers" SMDOC comment, or ask me) and that is obviously not the right behavior for this use case: we're trying to decide whether to mark it at all.
It would be smart to talk to jonco about how best to handle this. He's on London time.
- There seems to be an issue with the order of marking and sweeping and I'm at a bit of a loss. For generator frames, we only want the to hold a strong link to the Debugger.Frame as long as there is as the AbstractGeneratorObject is alive. What should be responsible for that? It looks like, previously, this was done as a side-effect of
forEachWeakMap([trc](auto& weakMap) { weakMap.trace(trc); });
because by the time that executed, the traversal will have already marked the generator, so marking the WeakMap will also mark the DebuggerFrame.
WeakMap marking is very special. Let's talk about this live at some point.
Because we've moved the call to
Debugger::trace
call frommarkIteratively
totraceGlobal
, it seems that theAbstractGeneratorObject
has not yet been marked, meaning that nothing ever marks the Debugger.Frame. That is causingjs/src/jit-test/tests/debug/Frame-onStep-async-gc-01.js
to fail with my current patch.
Yes, this is supposed to magically work out.
Updated•5 years ago
|
Assignee | ||
Comment 4•5 years ago
|
||
Comment 5•5 years ago
|
||
(In reply to Jim Blandy :jimb from comment #3)
Another would be for
Realm::debuggees_
to hold cross-compartment wrappers to theDebugger
s' JSObjects. Then all uses of that array would need to unwrap. But I think unwrapping is forbidden during GC.
It's not forbidden but you need to use js::UncheckedUnwrapWithoutExpose (which doesn't have a gray-unmarking read barrier). I'd say that using CCWs for this is a good way forward.
Reporter | ||
Comment 6•5 years ago
|
||
(In reply to Jon Coppeard (:jonco) from comment #5)
It's not forbidden but you need to use js::UncheckedUnwrapWithoutExpose (which doesn't have a gray-unmarking read barrier). I'd say that using CCWs for this is a good way forward.
Oh. I should probably use that in Breakpoint
, instead of including a separate Debugger*
field.
Reporter | ||
Comment 7•5 years ago
|
||
Okay, I managed to catch jonco this morning. Here's how we manage the realm's DebuggerVector
:
DebuggerVector
should be a simple js::Vector<js::WeakHeapPtr<JSObject*>>
, holding CCWs of Debugger
JS objects.
When a Realm
is traced, we traverse its DebuggerVector
(not tracing, just dereferences), call js::UncheckedUnwrapWithoutExpose
to cross the CCW, call Debugger::fromJSObject
to get the actual Debugger*
, and check whether it has hooks set. If it does, then we call TraceEdge
on that DebuggerVector
element. This marks the Debugger
.
When a Realm
is swept, we must call IsAboutToBeFinalized
on each element of the DebuggerVector
, and remove those elements for which it returns true from the vector. The elements that we did not call TraceEdge
on earlier, but that are nonetheless not being finalized (presumably because they are otherwise reachable), get their pointers fixed up by IsAboutToBeFinalized
.
Reporter | ||
Comment 8•5 years ago
|
||
Jonco adds:
A good place to do the sweeping would be from
GCRuntime::sweepDebuggerOnMainThread
, where it already does per-realm sweeping by callingsweepDebugEnvironments
on realms.
Comment 9•5 years ago
|
||
(In reply to Jim Blandy :jimb from comment #7)
DebuggerVector
should be a simplejs::Vector<js::WeakHeapPtr<JSObject*>>
, holding CCWs ofDebugger
JS objects.
I know I said this would work but I've spotted a problem. CCWs are not kept alive while the target is alive but only if we trace pointers to them. If we don't trace one of these it may die (and be removed from the list) even if the target debugger is still present, which is not what we want to happen here AIUI.
Comment 10•5 years ago
|
||
How is this looking now that bug 1597925 has landed?
Assignee | ||
Comment 11•5 years ago
|
||
I think this should be all set to land now. I actually missed that Jim had okayed it with his last review, so was thinking he needed to do another pass. Given how close we are to release now though, I'll probably just aim to land this once the release is cut, since it's not missing-critical to anything.
Comment 12•5 years ago
|
||
Comment 13•5 years ago
|
||
bugherder |
Description
•