Closed Bug 1409324 Opened 2 years ago Closed 2 years ago

Update the resize telemetry/profiling for GCMinor

Categories

(Core :: JavaScript: GC, enhancement, P3)

55 Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox-esr52 --- wontfix
firefox56 --- wontfix
firefox57 --- wontfix
firefox59 --- fixed

People

(Reporter: pbone, Assigned: pbone)

References

Details

Attachments

(2 files, 1 obsolete file)

Bug 1298018 will mean that the resize phase of a minor GC is going to be exceptionally quick, since it no-longer does any allocation itself and therefore there is no point in measuring it for telemetry or profiling.  Instead we may wish to measure any (tiny) pauses as the nursery grows lazilly.

Investigate what we currently do for the tenured heap and see if we should replicate that.
Depends on: 1298018
Priority: -- → P3
Assignee: nobody → pbone
Status: NEW → ASSIGNED
Attachment #8928111 - Flags: review?(sphink) → review+
Comment on attachment 8928112 [details] [diff] [review]
Bug 1409324 (part 2) - Add a new timer for nursery chunk allocation

Review of attachment 8928112 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/gc/Nursery.cpp
@@ +553,5 @@
>      json.property("cur_capacity", previousGC.nurseryCapacity);
>      json.property("new_capacity", spaceToEnd());
>      json.property("lazy_capacity", allocatedChunkCount() * ChunkSize);
> +    if (!timeInChunkAlloc_.IsZero())
> +        json.property("time_in_chunk_alloc", timeInChunkAlloc_, json.MICROSECONDS);

Personally, I'd call it "chunk_alloc_us", but it's up to you.
Attachment #8928112 - Flags: review?(sphink) → review+
(In reply to Steve Fink [:sfink] [:s:] from comment #3)
> Comment on attachment 8928112 [details] [diff] [review]
> Bug 1409324 (part 2) - Add a new timer for nursery chunk allocation
> 
> Review of attachment 8928112 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/gc/Nursery.cpp
> @@ +553,5 @@
> >      json.property("cur_capacity", previousGC.nurseryCapacity);
> >      json.property("new_capacity", spaceToEnd());
> >      json.property("lazy_capacity", allocatedChunkCount() * ChunkSize);
> > +    if (!timeInChunkAlloc_.IsZero())
> > +        json.property("time_in_chunk_alloc", timeInChunkAlloc_, json.MICROSECONDS);
> 
> Personally, I'd call it "chunk_alloc_us", but it's up to you.

I think I'll rename it. I wanted a name that helped make it clear it wasn't actually part of GC now, since it's now in mutator time (but can still create pauses).  Neither name does that well.

Thanks.
Updated from review feedback, r+ carried forward.
Attachment #8928112 - Attachment is obsolete: true
Attachment #8928406 - Flags: review+
Keywords: checkin-needed
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/05c80a2c4462
(part 1) - Remove old resize profiling from GCMinor. r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/cfa3672ccb30
(part 2) - Add a new timer for nursery chunk allocation r=sfink
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/05c80a2c4462
https://hg.mozilla.org/mozilla-central/rev/cfa3672ccb30
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.