Closed Bug 1564174 Opened 5 years ago Closed 5 years ago

[jsdbg2] Simplify Debugger/GC interactions

Categories

(Core :: JavaScript Engine, task, P3)

task

Tracking

()

RESOLVED FIXED

People

(Reporter: jimb, Unassigned)

References

(Blocks 1 open bug)

Details

(Whiteboard: [debugger-mvp])

Information about what behaviors a Debugger is observing is scattered about in various data structures, making it complicated to determine when a Debugger may be GC'd, and introducing strange custom GC logic in Debugger.cpp. A more direct representation of the Debugger's interests could make this complexity and custom logic unnecessary.

The fundamental principle of garbage collection is that GC is an invisible optimization: an object's memory is reclaimed only when doing so would have no observable impact on the program's execution. The Debugger API attempts to follow this principle: even if a Debugger object is not directly reachable from JavaScript, if it has hooks that might still be called, those could certainly have observable effects, and so the Debugger must be held alive.

This principle also justifies the characteristic optimization of weak maps: since JavaScript cannot enumerate a weak map's entries, if an entry's key becomes unreachable other than through that entry's value, then removing the entry altogether would be unobservable. Debugger implements a similar optimization: if a Debugger is unreachable from JavaScript and has no hooks that might still be called, detaching the Debugger from its debuggees altogether and reclaiming its memory can have no visible side effects.

These two behaviors are presently implemented via complex custom marking functions in Debugger.cpp, and checks scattered throughout SpiderMonkey for the presence of debuggers with particular sorts of hooks. (For example, js::jit::HandleExceptionIo checks if the global has any Debugger with an onExceptionUnwind hook.)

There is a more direct implementation approach that should be clearer for the execution engines and the JITs. The execution engines should not concern themselves with Debuggers directly at all. Rather, there should be a family of abstract base classes, which we will call 'auditors', with a class for each sort of behavior the debugger can observe: a stack frame entry auditor, a single step auditor, a new script auditor, and so on. Auditors serve as intermediaries between execution engines and the Debugger API.

Each JS::Realm should carry a pointer to a single new script auditor, nullable when no observation is needed. Each stack frame should carry a nullable pointer to one single-step auditor (probably stored in a side table keyed by AbstractFramePtr). Execution engines should concern themselves solely with reporting debuggee activity faithfully to whatever auditors are present (if any).

In turn, the concrete implementations of the auditor classes report activity to the interested Debuggers. If multiple Debuggers are interested, the auditor must take care of iterating over them. The Debugger implementation must install an auditor when the user first sets a hook, and remove the auditor when the last hook is unset.

For garbage collection, realms and stack frames must trace their auditors. Auditors, in turn, must trace only those Debuggers whose interests they exist to serve.

Thus, when a Debugger is unreachable by JavaScript but has a onNewScript hook set, there must be a new script auditor on each of its debuggee realms, and those auditors will hold the Debugger alive for as long as the realms are alive. Similarly, if an otherwise unreachable Debugger.Frame has a onPop hook set, there must be a pop auditor stored on the stack frame, and that auditor will hold the Debugger.Frame alive until the frame has been popped.

When a Debugger has no hooks set, then regardless of how many debuggee realms it has, there should be no auditors installed on its behalf anywhere, and if it not otherwise reachable from JavaScript, the Debugger can be reclaimed by the GC naturally.

The isDebuggee flags present now can continue to serve checks on fast paths: a frame's isDebuggee flag, say, would be set if and only if the frame has any auditors set. This clarifies the meaning of the flag, as well as the arrangements necessary to set it (adding instrumentation to a script, to notify its auditors).

The various hooks in the Debugger API would be supported as follows:

  • auditor on JS::Realm: onEnterFrame, onDebuggerStatement, onExceptionUnwind, onNewScript, onNewPromise, onPromiseSettled
  • auditor on stack frame (actually, side table): onPop, onStep
  • auditor on JSScript (actually, DebugScript): breakpoint
  • auditor on JSRuntime: onNewGlobalObject

(Note that the present behavior of the onNewGlobalObject hook actually violates the fundamental principle cited above: even if a Debugger has an onNewGlobalObject hook set, it may be GC'd. I believe this is a bug, but there is some disagreement on whether it should be fixed or made official. Either choice can be accomodated, by making the new global auditor behave as either a strong or weak reference to its Debugger.)

Priority: -- → P3

One wrinkle in this plan, that will go away soon:

If you look into the implementation, Debugger::markIteratively calls Debugger::hasAnyLiveHooks, which checks the Debugger's enabled flag. If a Debugger is disabled and unreachable from JS, then its hooks will never fire, and so the Debugger is considered unreachable and collectable.

In an auditor-based world, setting a Debugger's enabled property to false would need to remove any auditors that were present solely for its benefit. The current implementation does something like this, but only for some sorts of hooks.

However, with bug 1564168, we will remove Debugger.prototype.enabled and its flag altogether, so this complexity will go away. If a hook is set, we can assume it will actually be honored.

This scheme makes sense to me. We discussed it some on IRC, and one way to describe what's going on is that currently the data structures do not correspond to the traceability rules -- eg, currently each global object has a vector of Debuggers, and when something of interest happens, we iterate over the vector and decide what to do for each Debugger. During marking, the analogous thing happens: we iterate over all live Debuggers and check for hooks/breakpoints (the logic that auditors would use to decide which Debuggers to notify). That forces the marking to be iterative, as we may discover new live Debuggers in the process. With auditors, the hope is that any hookable thing (eg a Realm) can directly trace its auditor, if any, and its auditor will use the same logic to enumerate Debuggers for either tracing or notifying. This would remove the need to iterate, and would hopefully localize some hairy logic.

Depends on: 1565621
Blocks: dbg-71
Whiteboard: [debugger-mvp]
Depends on: 1586452
Depends on: 1586450
Depends on: 1592116
Depends on: 1592155
Depends on: 1592158

All blockers have landed, so we can close this now.

Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.