1052793 - Do per-zone GC for CC_WAITING triggers

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Terrence Cole [:terrence]]

Right now the CC_WAITING trigger is calling PrepareForFullGC, causing an all-zones GC. This means that 97% of GC's collect all zones, even zones that haven't been touched since the last GC. It should be possible to make AutoEnterCompartment touch something on the zone so that we can know what zones have seen activity and collect accordingly.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Well, AutoEnterCompartment isn't used when CC traces through js stuff, so
if CC releases tons of C++ objects, it would need some other way to tell to GC which
compartments/zones to gc.

Comment 2

[Andrew McCreight [:mccr8]]

We could get the compartment of every JS object, though then we'd lose the nice property that we never touch JS objects after traversal.

Comment 3

[Terrence Cole [:terrence]]

Could we track them during traversal by putting a flag on the zone in the TraceCallbacks, perhaps?

Comment 4

[Andrew McCreight [:mccr8]]

I have a patch that does this.  It doesn't really help much on the humble bundle demo.  The number of zones collected drops from 8 to 2, which is nice, but the max pause is still 40ms, in slice 1.  The main difference seems to be that the time in the final slice, which is mostly sweeping, goes from 20ms to 15ms.  But I'm sure with more than 1 tab open it would be more of an improvement. :)

Comment 5

[Andrew McCreight [:mccr8]]

Attachment: CC_WAITING GCs should only collect zones we CCed objects in. WIP

Comment 6

[Terrence Cole [:terrence]]

(In reply to Andrew McCreight [:mccr8] from comment #4)
> I have a patch that does this.  It doesn't really help much on the humble
> bundle demo.  The number of zones collected drops from 8 to 2, which is
> nice, but the max pause is still 40ms, in slice 1.  The main difference
> seems to be that the time in the final slice, which is mostly sweeping, goes
> from 20ms to 15ms.  But I'm sure with more than 1 tab open it would be more
> of an improvement. :)

Great! Shaving 25% off the last slice is a huge win. I'm sure there's probably something dumb going on with the second slice being 40ms. I'll look into it today.

Comment 7

[Andrew McCreight [:mccr8]]

(In reply to Terrence Cole [:terrence] from comment #6)
> Great! Shaving 25% off the last slice is a huge win. I'm sure there's
> probably something dumb going on with the second slice being 40ms. I'll look
> into it today.

Yeah, I think if the mark budget stuff is fixed, then sweep time will be the long pole (as expected!) so that will reduce our max GC pause by 25%.

Comment 8

[Andrew McCreight [:mccr8]]

Attachment: CC_WAITING GCs should only collect zones the CC thinks there is garbage in.

In the future, we could do a similar thing for other GC triggers, like page close or JS context destroy, but I think this is good enough for now.

One weird bit here is that we sometimes hit a case where we never hit a non-CC_WAITING trigger, but then we fire the timer and there are no zones. I think this happens when something outside of nsJSEnvironment triggers a GC, so we clear the list of waiting zones. In that case, we just don't do anything.

try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7054ffad7ffa

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8509029 [details] [diff] [review]
CC_WAITING GCs should only collect zones the CC thinks there is garbage in.

> static uint32_t sCCollectedWaitingForGC;
> static uint32_t sCCollectedZonesWaitingForGC;
>+static nsTHashtable<nsPtrHashKey<JS::Zone>> sZonesWaitingForGC;
Is this not causing static ctors?

Please add a comment why it is safe to keep pointers to JS::Zones.
This needs a review from a GC person too.


>-  JS::PrepareForFullGC(sRuntime);
>+  if (sNeedsFullGC || aReason != JS::gcreason::CC_WAITING) {
>+    sNeedsFullGC = false;
>+    JS::PrepareForFullGC(sRuntime);
>+  } else {
>+    if (sZonesWaitingForGC.Count() == 0) {
>+      // We already ran the GC, so there's nothing to collect.
>+      return;
>+    }
>+    sZonesWaitingForGC.EnumerateEntries(PrepareWaitingZoneForGC, nullptr);
>+  }
I would expect sZonesWaitingForGC.Clear() to happen somewhere here.
No reason to keep the hashtable entries alive longer than needed.

>+  aResults.mCollectedZones.EnumerateEntries(AddCollectedZones, nullptr);
This copying is a bit odd.
Could CC use nsAutoPtr to a hashtable, and then here we just do a swap and take the hashtable to JSEnvironment.

>@@ -92,26 +92,28 @@ struct CycleCollectorResults
>     mVisitedRefCounted = 0;
>     mVisitedGCed = 0;
>     mFreedRefCounted = 0;
>     mFreedGCed = 0;
>     mFreedJSZones = 0;
>     mNumSlices = 1;
>     // mNumSlices is initialized to one, because we call Init() after the
>     // per-slice increment of mNumSlices has already occurred.
>+    mCollectedZones.Clear();
>   }
> 
>   bool mForcedGC;
>   bool mMergedZones;
>   uint32_t mVisitedRefCounted;
>   uint32_t mVisitedGCed;
>   uint32_t mFreedRefCounted;
>   uint32_t mFreedGCed;
>   uint32_t mFreedJSZones;
>   uint32_t mNumSlices;
>+  nsTHashtable<nsPtrHashKey<JS::Zone>> mCollectedZones;
So this could be
nsAutoPtr<nsTHashtable<nsPtrHashKey<JS::Zone>>> mCollectedZones;
(And I don't care too much whether to use nsAutoPtr or some mfbt::* stuff. nsAutoPtr is easier to understand though.)


>         if (MOZ_UNLIKELY(pinfo->mParticipant == zoneParticipant)) {
>           ++numWhiteJSZones;
>+          zone = static_cast<JS::Zone*>(pinfo->mPointer);
>+        } else {
>+          zone = JS::GetTenuredGCThingZone(pinfo->mPointer);
Hmm, mPointer is guaranteed to live in tenured heap? I guess so. 
We do a minor GC before CC or something.

Comment 10

[Andrew McCreight [:mccr8]]

(In reply to Olli Pettay [:smaug] from comment #9)
> Please add a comment why it is safe to keep pointers to JS::Zones.
> This needs a review from a GC person too.
Ok.

> I would expect sZonesWaitingForGC.Clear() to happen somewhere here.
> No reason to keep the hashtable entries alive longer than needed.
Yeah, I can add it here, too. We still need it in the other place in case we get a GC triggered for other reasons.

> >+  aResults.mCollectedZones.EnumerateEntries(AddCollectedZones, nullptr);
> This copying is a bit odd.
> Could CC use nsAutoPtr to a hashtable, and then here we just do a swap and
> take the hashtable to JSEnvironment.
Well, potentially you can have multiple CCs in a row, so you have to accumulate into a single hash table. Or I could have an array of hashtables, I guess.

> Hmm, mPointer is guaranteed to live in tenured heap? I guess so. 
> We do a minor GC before CC or something.

Yeah, the CC already relies on not having any pointers into the nursery.  I think the "tenured" part just means we assert if it isn't tenured.

I'll figure out what to do about the possible static ctor.  I could have a hashtable pointer, maybe, or just use a list and not worry about duplicates, as it probably won't get too huge.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Well, couldn't CC still keep the hashtable alive long enough, and then
nsJSEnvironment just takes it when needed.

Comment 12

[Andrew McCreight [:mccr8]]

Ah, I could make nsJSEnvironment just pass in its hashtable to the CC via results, and accumulate things into it that way.

Comment 13

[Andrew McCreight [:mccr8]]

Attachment: Do per-zone GC for CC_WAITING triggers. WIP

This version tracks the set of collected zones entirely within the CC
itself, so there's no more passing around and unioning hash sets. I
think it is a lot less scary looking. I still need to do a try run.

Comment 14

[Andrew McCreight [:mccr8]]

Attachment: Do per-zone GC for CC_WAITING triggers. WIP

Here's a third version of the patch. This stores the zones to be
collected on the CycleCollectedJSRuntime. This gives us a more general
interface for adding zones of interest, which can be used for things
like bug 1110928.

Another difference with the previous version of the patch is that in
the case where there are no zones indicated to be collected, we do a
full GC, rather than do no GC at all. This doesn't seem too likely,
but it could maybe happen if we trigger a WAITING_CC, then do a GC,
then later somehow trigger the WAITING_CC. For this unlikely scenario,
it seems better to err on the side of more GCing.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f94bd85d97eb

Comment 15

[Andrew McCreight [:mccr8]]

Attachment: Do per-zone GC for CC_WAITING triggers. WIP

minor tweak

Comment 16

[Andrew McCreight [:mccr8]]

Attachment: Do per-zone GC for CC_WAITING triggers.

try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f94bd85d97eb

Comment 17

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c3c4c83e1efe

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

We should investigate doing per-zone GCs more often, since even with the patch I'm seeing most of the
GCs being for all the zones.

Comment 19

[Andrew McCreight [:mccr8]]

(In reply to Olli Pettay [:smaug] from comment #18)
> We should investigate doing per-zone GCs more often, since even with the
> patch I'm seeing most of the GCs being for all the zones.

In some light local testing, for me the reason this was happening was bug 1110928: we poke the GC for a bunch of different reasons (listed there) before a GC, and if any GC poke is non-CC_WAITING then we GC every zone.  As you can see in that bug, I think every non-CC_WAITING poke is associate with a particular document, so we should be able to just add a particular zone to the list rather than GC everything.

Comment 20

[Andrew McCreight [:mccr8]]

The last poke just happens to usually be CC_WAITING, which is why that shows up as the reason.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

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

Comment 22

[Andrew McCreight [:mccr8]]

It looks like GC_SWEEP_MS dropped by maybe 25% or so on 12/16, and GC_MAX_PAUSE_MS seems to have dropped by maybe 10% on the same day.

Comment 23

[Andrew McCreight [:mccr8]]

Vladan pointed out that this caused a change in GC_IS_COMPARTMENTAL.  We were collecting all zones about 92% of the time.  After this patch landed, we're only collecting all zones about every 47% of the time.

Comment 24

[Andrew McCreight [:mccr8]]

backed out for causing bug 1118397:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/204c0dfa5e0b

Comment 25

[Andrew McCreight [:mccr8]]

From what Terrence said, it sounds like the problem is that we only actually decommit arenas after full GCs, and even doing a compartmental GC with 4 seconds of the full GC will stop us from decommitting.  My patch makes us do compartmental GCs something like 4 or 5 times more often, so we don't end up cleaning up these arenas.  He's going to rework it so we don't depend on full GCs so much.  Traditionally, compartmental GCs have mostly only happened in the scenario of running some kind of heavy JS demo in a single tab, so this wasn't an issue.

Comment 26

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

https://hg.mozilla.org/mozilla-central/rev/204c0dfa5e0b

Comment 27

[Andrew McCreight [:mccr8]]

Terrence, has the problem where the GC relies on there being many full GCs in order to do shrinking been fixed yet?  I wasn't quite sure what happened there.

Comment 28

[Terrence Cole [:terrence]]

Sadly, no, the thing I wanted to do regressed Octane badly and had to get backed out. I'll give this some more thought this weekend and maybe we can chat about it on Monday.

Comment 29

[Andrew McCreight [:mccr8]]

Attachment: Do per-zone GC for CC_WAITING triggers.

Rebased.

Comment 30

[Andrew McCreight [:mccr8]]

Terrence pointed out that the chunk of code in DOMGCSliceCallback() that is near the comment "Avoid shrinking during heavy activity" should be moved outside of the "if (aDesc.isCompartment_) {" block because we're going to do compartmental GCs much more often now. My rebased patch does not include that change.

Comment 31

[Andrew McCreight [:mccr8]]

I did a try push which seems reasonable green:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=358d5dfa1c58

Comment 32

[Terrence Cole [:terrence]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c3c489a62dc

Comment 33

[Terrence Cole [:terrence]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1215b0d78079

Comment 34

[Andrew McCreight [:mccr8]]

Terrence's AWSY try push demonstrates that his suggested fix in comment 30 keeps us from regressing memory usage. I'll do a debug try push and upload a patch for review.

Comment 35

[Andrew McCreight [:mccr8]]

Attachment: part 2 - Shrink GC buffers during compartmental GCs.

With part 1, many more GCs are compartmental, so it is no longer a
good indication that there is heavy activity. If we don't regularly
try to shrink GC buffers, we don't decommit arenas enough, causing a
large regression in memory usage.

(Over IRC, smaug said he was okay with me landing part 1.)

Comment 36

[Terrence Cole [:terrence]]

Comment on attachment 8720943 [details] [diff] [review]
part 2 - Shrink GC buffers during compartmental GCs.

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

Heh, I guess we don't actually want the wrong comment anymore.

Comment 37

[Andrew McCreight [:mccr8]]

try looks green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b3d497309991

Comment 38

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b5fc3e849945
https://hg.mozilla.org/integration/mozilla-inbound/rev/8835594562be

Comment 39

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b5fc3e849945
https://hg.mozilla.org/mozilla-central/rev/8835594562be

Comment 40

[Jan de Mooij [:jandem]]

FWIW, some interesting telemetry numbers here:

https://telemetry.mozilla.org/new-pipeline/evo.html#!aggregates=median!mean!95th-percentile&cumulative=0&end_date=null&keys=&max_channel_version=nightly%252F47&measure=GC_MS&min_channel_version=nightly%252F46&product=Firefox&sanitize=1&sort_keys=submissions&start_date=null&trim=1&use_submission_date=0

Not many submissions so far for the Feb 20 build, but it will be interesting to check this again in a few days.

Comment 41

[Andrew McCreight [:mccr8]]

(In reply to Jan de Mooij [:jandem] from comment #40)
> FWIW, some interesting telemetry numbers here:

GC times are particularly prone to some kind of recency bias. I think that longer running sessions have worse GC numbers, and of course very new builds have shorter session times.

Last time I landed this (comment 22) we went from about 90% to 50% full GCs, and it looked like there were some improvements in GC pause times.

Comment 42

[Terrence Cole [:terrence]]

Median GC_MS seems to have gone from 185ms to 150ms. More importantly, the 95th percentile numbers have gone from 1s down to 0.85s.