Closed Bug 1380770 Opened 2 years ago Closed 2 years ago

Add minor GC reason to MinorGC marker

Categories

(Core :: JavaScript: GC, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: sfink, Assigned: sfink)

References

(Blocks 1 open bug)

Details

(Whiteboard: [qf])

Attachments

(1 file)

And nursery size, and promotion rate.
I haven't actually tried this out yet, but it seems worth getting feedback on. I appended '-kb' to specify units, which doesn't really match anything else and is somewhat arbitrary. I also am reporting the parts used to calculate the promotion rate, not just the rate itself, because the additional info seems potentially useful -- but normally the rate is the most interesting thing. I could add it in too, but it seems redundant. Maybe the rate plus one of the others, since you can infer the third but at least you'd have the rate? Ugh.
Attachment #8886472 - Flags: review?(jcoppeard)
Assignee: nobody → sphink
Status: NEW → ASSIGNED
Comment on attachment 8886472 [details] [diff] [review]
Add more info to GCMinor marker JSON

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

The additional info seems useful, but I'd say report the promotion rate too.  It's clearer and that's more important than a little redundancy in my view.

::: js/src/gc/Nursery.cpp
@@ +481,5 @@
> +
> +    json.property("reason", JS::gcreason::ExplainReason(minorGCTriggerReason_));
> +    json.property("nursery-used-kb", nurseryUsedBytes_ / 1024);
> +    json.property("tenured-kb", tenuredBytes_ / 1024);
> +    json.property("nursery-kb", numChunks() * (ChunkSize >> 10));

I'd just use bytes and let the whoever gets this data format it how they like.

::: js/src/gc/Nursery.h
@@ +352,5 @@
>      ProfileDurations profileDurations_;
>      ProfileDurations totalDurations_;
>      uint64_t minorGcCount_;
> +    int nurseryUsedBytes_;
> +    int tenuredBytes_;

These should be size_t.  If they relate to the last collection they should be named to indicate that - maybe lastUsedByes or something.
Attachment #8886472 - Flags: review?(jcoppeard) → review+
Whiteboard: [qf]
Pushed by sfink@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b05837ec165
Add more info to GCMinor marker JSON, r=jonco
https://hg.mozilla.org/mozilla-central/rev/6b05837ec165
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Depends on: 1381568
You need to log in before you can comment on or make changes to this bug.