Closed Bug 1420333 Opened 3 years ago Closed 3 years ago
buffered grey roots cause non-incremental collection, but this is not recorded in perf or telemetry
See GCRuntime::incrementalCollectSlice() a check for buffered grey roots causes collection to switch to non-incremental, but it doesn't record this in perf of telemetry properly like in the code in GCRuntime::budgetIncrementalGC()
Hi Jon, Can you check the naming of this new constant. The logic in the collector looks strange to me (it switches to non-incremental if there are _no_ buffered grey roots). I don't know one way or the other so I'd like to check this in particular. Thanks.
Assignee: nobody → pbone
Status: NEW → ASSIGNED
Attachment #8931615 - Flags: review?(jcoppeard)
Comment on attachment 8931615 [details] [diff] [review] Bug 1420333 - Add a new non-incremental reason for buffered grey roots Review of attachment 8931615 [details] [diff] [review]: ----------------------------------------------------------------- Well spotted. We are indeed missing a non-incremental reason here. However hasBufferedGrayRoots() is badly named. It means 'did we successfully copy the gray roots to our buffer (without hitting OOM)', not 'are there any gray roots'. So the new constant should be something like GrayRootBufferingFailed.
Attachment #8931615 - Flags: review?(jcoppeard) → review+
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/f690e5794097 Add a new non-incremental reason for buffered grey roots r=jonco
Thanks jonco. I think that's a good name so I changed my patch. Now that I have commit access I have committed this directly to inbound, should I still be re-uploading the updated patch to bugzilla or is the link to the commit sufficient to see what was actually committed? Thanks.
(In reply to Paul Bone [:pbone] from comment #4) There's no need to re-upload the patch for doing minor review feedback. Cheers.
You need to log in before you can comment on or make changes to this bug.