Closed
Bug 1380025
Opened 7 years ago
Closed 7 years ago
GC 'poke' trigger doesn't have its own reason code
Categories
(Core :: JavaScript: GC, enhancement)
Tracking
()
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: jonco, Assigned: jonco)
References
Details
Attachments
(1 file)
19.06 KB,
patch
|
sfink
:
review+
|
Details | Diff | Splinter Review |
As mentioned in bug 1377466, we don't know how often pokes are causing GCs because they don't have their own reason code.
Assignee | ||
Comment 1•7 years ago
|
||
This patch does a few things: - 'poke' is renamed to 'notifyRootsRemoved' to better explain its purpose - GCs thus triggered get their own reason code - the logic in GCRuntime::collect is updated to ensure we wait until the end of GC to trigger a new one
Attachment #8885327 -
Flags: review?(sphink)
Comment 2•7 years ago
|
||
Comment on attachment 8885327 [details] [diff] [review] bug1380025-poke-gc Review of attachment 8885327 [details] [diff] [review]: ----------------------------------------------------------------- Wow, I had no idea what any of this did. Thanks for clearing it up! ::: js/src/jsgc.cpp @@ +6841,5 @@ > + /* Need to re-schedule all zones for GC. */ > + JS::PrepareForFullGC(rt->activeContextFromOwnThread()); > + repeat = true; > + reason = JS::gcreason::ROOTS_REMOVED; > + } else if (shouldRepeatForDeadZone(reason) && !wasReset) { The !wasReset here is redundant. That case was already handled. ::: js/src/vm/NativeObject.cpp @@ -2864,5 @@ > // as the result parameter. This always succeeds when there is no hook. > return CallJSDeletePropertyOp(cx, obj->getClass()->getDelProperty(), obj, id, result); > } > > - cx->runtime()->gc.poke(); Why is this one no longer necessary?
Attachment #8885327 -
Flags: review?(sphink) → review+
Assignee | ||
Comment 3•7 years ago
|
||
(In reply to Steve Fink [:sfink] [:s:] from comment #2) > Why is this one no longer necessary? I found a description of what GC poke was originally for in bug 481922 comment 11. The second case is still valid, but any check we had to skip a GC if we hadn't been poked must have been removed a long time ago. This poke was for the first case.
Assignee | ||
Comment 4•7 years ago
|
||
(In reply to Steve Fink [:sfink] [:s:] from comment #2) > The !wasReset here is redundant. That case was already handled. Cheers, fixed.
Assignee | ||
Comment 5•7 years ago
|
||
I accidentally landed this patch with bug 1377466 in the commit message.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
I'm seeing a spike in test failures that start with `Assertion failure: false (Ran out of memory while building cycle collector graph), at nsCycleCollector.cpp:930` in the last few days (bug 1381177). Any chance I can pin those on this patch?
status-firefox56:
--- → fixed
Target Milestone: --- → mozilla56
Depends on: 1381177
Comment 7•7 years ago
|
||
These landings were http://hg.mozilla.org/integration/mozilla-inbound/rev/ed043698eafa http://hg.mozilla.org/mozilla-central/rev/ed043698eafa
Updated•2 years ago
|
Performance Impact: --- → ?
Whiteboard: [qf]
You need to log in
before you can comment on or make changes to this bug.
Description
•