Closed Bug 1293262 Opened 3 years ago Closed 3 years ago

Always pre-tenure objects after minor GC

Categories

(Core :: JavaScript: GC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: jonco, Assigned: jonco)

References

Details

Attachments

(2 files)

Since the recent changes to merge JSContext and JSRuntime we can pre-tenure objects after minor GC even if the caller didn't pass in a JSContext and get rid of the second version of minorGC().
Attachment #8778897 - Flags: review?(terrence)
Comment on attachment 8778897 [details] [diff] [review]
always-pre-tenure

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

Yay! This is the number one thing I was looking forward to with the merge cx/rt.

::: js/src/jsgc.cpp
@@ +6479,5 @@
>      minorGCTriggerReason = JS::gcreason::NO_REASON;
>      TraceLoggerThread* logger = TraceLoggerForMainThread(rt);
>      AutoTraceLog logMinorGC(logger, TraceLogger_MinorGC);
> +    Nursery::ObjectGroupList pretenureGroups;
> +    nursery.collect(rt, reason, &pretenureGroups);

We can actually go further here. The only reason that all of this code is not in Nursery::collect (and accounted for in our timing info) is the rt/cx split. I think we should be able to move this all into the collection proper and report it in our timing stats now.

I'd also like to collect telemetry on this to see how much stuff is getting pre-tenured in the wild as well.
Attachment #8778897 - Flags: review?(terrence) → review+
Keywords: leave-open
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5df7daac885a
Always pre-tenure objects after minor GC r=terrence
Good idea, that's much better.
Attachment #8779281 - Flags: review?(terrence)
Comment on attachment 8779281 [details] [diff] [review]
bug1293262-refactor-pretenuring

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

Yay!

::: js/src/gc/Nursery.cpp
@@ +648,5 @@
>      // pointers to nursery things, which will force a collection well before
>      // the nursery is full, look for object groups that are getting promoted
>      // excessively and try to pretenure them.
>      maybeStartProfile(ProfileKey::Pretenure);
> +    bool pretenureObjects = false;

I was actually thinking we'd want an integer count for this.

::: toolkit/components/telemetry/Histograms.json
@@ +566,5 @@
>    },
> +  "GC_PRETENURE_OBJECTS": {
> +    "alert_emails": ["dev-telemetry-gc-alerts@mozilla.org"],
> +    "expires_in_version": "never",
> +    "kind": "boolean",

We even know the top end of the range: 16. If there is a hump at 16 we'll know we need to embiggen the table.
Attachment #8779281 - Flags: review?(terrence) → review+
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c39bb9b5a8c8
Refactor pretenuring after minor GC and add telemetry r=terrence
Depends on: 1295027
Status: NEW → RESOLVED
Closed: 3 years ago
Keywords: leave-open
Resolution: --- → FIXED
Depends on: 1296272
You need to log in before you can comment on or make changes to this bug.