Closed Bug 1420333 Opened 2 years ago Closed 2 years ago

buffered grey roots cause non-incremental collection, but this is not recorded in perf or telemetry

Categories

(Core :: JavaScript: GC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: pbone, Assigned: pbone)

References

Details

Attachments

(1 file)

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 pbone@mozilla.com:
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.
Flags: needinfo?(jcoppeard)
https://hg.mozilla.org/mozilla-central/rev/f690e5794097
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
(In reply to Paul Bone [:pbone] from comment #4)
There's no need to re-upload the patch for doing minor review feedback.  Cheers.
Flags: needinfo?(jcoppeard)
Depends on: 1422264
You need to log in before you can comment on or make changes to this bug.