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

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Paul Bone [:pbone]]

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()

Comment 1

[Paul Bone [:pbone]]

Attachment: Bug 1420333 - Add a new non-incremental reason for buffered grey roots

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.

Comment 2

[Jon Coppeard (:jonco)]

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.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f690e5794097
Add a new non-incremental reason for buffered grey roots r=jonco

Comment 4

[Paul Bone [:pbone]]

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.

Comment 5

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/f690e5794097

Comment 6

[Jon Coppeard (:jonco)]

(In reply to Paul Bone [:pbone] from comment #4)
There's no need to re-upload the patch for doing minor review feedback.  Cheers.