1293262 - Always pre-tenure objects after minor GC

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Jon Coppeard (:jonco)]

Attachment: always-pre-tenure

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

Comment 1

[Terrence Cole [: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.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5df7daac885a
Always pre-tenure objects after minor GC r=terrence

Comment 3

[Jon Coppeard (:jonco)]

Attachment: bug1293262-refactor-pretenuring

Good idea, that's much better.

Comment 4

[Terrence Cole [: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": ["[e-mail]"],
> +    "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.

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5df7daac885a

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c39bb9b5a8c8
Refactor pretenuring after minor GC and add telemetry r=terrence

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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