[jsdbg2] Trace breakpoints directly
Categories
(Core :: JavaScript Engine, task, P2)
Tracking
()
Tracking | Status | |
---|---|---|
firefox72 | --- | fixed |
People
(Reporter: jimb, Assigned: jimb)
References
Details
Attachments
(10 files, 2 obsolete files)
623 bytes,
patch
|
Details | Diff | Splinter Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review |
The algorithm the garbage collector uses to retain Debugger
s due to live breakpoint handlers is more complicated than necessary, and should be replaced with a more direct approach.
In the current code, each time through the iterative weak map marking cycle, we search the JSRuntime
for marked global objects, see if they are debuggees, and if so, see if their Debugger
s have any breakpoints set in marked JSScript
s or WasmInstanceObject
s. If so, then we mark the Debugger
, which marks its breakpoints.
It would be equivalent and simpler to have tracing a JSScript
or WasmInstanceObject
trace their breakpoint sites, which then traces breakpoints, which then traces Debugger
s.
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 1•5 years ago
|
||
The Debugger.prototype.enabled accessor was removed in bug 1564168, but there
are still a few stray references to 'enabled' or 'disabled' debuggers in the
docs and the code. These should be cleaned up.
Assignee | ||
Comment 2•5 years ago
|
||
Between breakpoint operations, a BreakpointSite's enabledCount is greater than
zero if and only if its list of breakpoints is empty. Since checking whether the
list is empty is cheap, there's no reason to maintain a separate count.
Depends on D48465
Updated•5 years ago
|
Assignee | ||
Comment 3•5 years ago
|
||
Yury, I would expect this patch to introduce a bug, because it skips the call to BreakpointSite::recompile
for Wasm breakpoints. The test file js/src/jit-test/tests/debug/wasm-breakpoint.js
does cover this case, but it seems like the problem gets fixed up by accident someplace else.
Why this patch? I introduced an equivalent bug in the midst of another patch series, and noticed it by eye, and was worried that the tests hadn't caught it. I tried to come up with a test case that would expose the bug, but I haven't been able to.
Can you see any way to come up with a test case that detects the bug this patch introduces?
Updated•5 years ago
|
Assignee | ||
Comment 4•5 years ago
|
||
Depends on D48465
Assignee | ||
Comment 5•5 years ago
|
||
Depends on D49850
Assignee | ||
Comment 6•5 years ago
|
||
Depends on D49851
Assignee | ||
Comment 7•5 years ago
|
||
Depends on D49852
Assignee | ||
Comment 8•5 years ago
|
||
Depends on D49853
Assignee | ||
Comment 9•5 years ago
|
||
Depends on D49854
Assignee | ||
Comment 10•5 years ago
|
||
Depends on D49855
Assignee | ||
Comment 11•5 years ago
|
||
Depends on D49856
Assignee | ||
Comment 12•5 years ago
|
||
Depends on D49857
Assignee | ||
Comment 13•5 years ago
|
||
That last is a work in progress. Current failures:
debug/Debugger-onNativeCall-01.js
debug/Script-clearBreakpoint-01.js
debug/Script-clearBreakpoint-02.js
debug/Script-clearBreakpoint-03.js
debug/Script-getBreakpoints-01.js
debug/Script-getBreakpoints-02.js
debug/breakpoint-multi-02.js
debug/breakpoint-multi-04.js
debug/wasm-breakpoint.js
Comment 14•5 years ago
|
||
Comment on attachment 9099434 [details]
Bug 1586452: Remove stray references to 'enabled' debuggers.
Revision D48465 was moved to bug 1590589. Setting attachment 9099434 [details] to obsolete.
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Assignee | ||
Comment 15•5 years ago
|
||
Okay, that's the updated patch queue.
JS-centric try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=29e6a9f0db9bc1f21b9133a805c5bc6d86af9ff8
Assignee | ||
Comment 16•5 years ago
|
||
Looks green! I'm astonished.
Comment 17•5 years ago
|
||
Pushed by jblandy@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/1acb660e5fc4 Use tighter type JSBreakpointSite where possible. r=jonco https://hg.mozilla.org/integration/autoland/rev/0a1e6f5aa6c3 Make BreakpointSite constructor and destructor protected. r=jonco https://hg.mozilla.org/integration/autoland/rev/d0d79b88fd49 Let WasmBreakpointSite hold a WasmInstanceObject*, not a wasm::Instance*. r=jonco https://hg.mozilla.org/integration/autoland/rev/4d3fe0b93397 Remove class js::WasmBreakpoint; use plain js::Breakpoint instead. r=jonco https://hg.mozilla.org/integration/autoland/rev/7fc4169017bc Make `js::BreakpointSite::remove` the pure virtual method, and hoist `removeIfEmpty' into the base class. r=jonco https://hg.mozilla.org/integration/autoland/rev/b49320ce9db5 Manage JIT traps when we create and remove breakpoint sites. r=jonco https://hg.mozilla.org/integration/autoland/rev/df4696b00c37 SetBreakpointMatcher: success path should be main line. r=jonco https://hg.mozilla.org/integration/autoland/rev/dca8e34d8b27 Separate removal and destruction of Breakpoints into two methods. r=jonco https://hg.mozilla.org/integration/autoland/rev/843d64235cfa Let JSScripts and wasm::Instances own their BreakpointSites and Breakpoints. r=jonco
Comment 18•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1acb660e5fc4
https://hg.mozilla.org/mozilla-central/rev/0a1e6f5aa6c3
https://hg.mozilla.org/mozilla-central/rev/d0d79b88fd49
https://hg.mozilla.org/mozilla-central/rev/4d3fe0b93397
https://hg.mozilla.org/mozilla-central/rev/7fc4169017bc
https://hg.mozilla.org/mozilla-central/rev/b49320ce9db5
https://hg.mozilla.org/mozilla-central/rev/df4696b00c37
https://hg.mozilla.org/mozilla-central/rev/dca8e34d8b27
https://hg.mozilla.org/mozilla-central/rev/843d64235cfa
Updated•3 years ago
|
Description
•