1029648 - Dynamic heap growth is not enabled in the shell

Status: RESOLVED WORKSFORME

(Core :: JavaScript: GC, defect)

Description

[Jon Coppeard (:jonco)]

Dynamic heap growth, implemented in bug 765435, is not turned on in the shell.

In order to make the shell behave as much as possible like the browser we should enable this.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: turn-on-dynamic-heap-growth

Patch to change this, and also dynamic mark slice to match browser setting (the latter is unsused unless anyone manually triggers an incremental GC).

Annoyingly this is a ~10% regression for Splay and Typescript as tested in the shell:

         Richards:       25805.8 (10  7.3%)       25353.8 (10  8.7%)  -1.8%
        DeltaBlue:       35696.3 (10  0.6%)       35579.5 (10  0.6%)  -0.3%
           Crypto:       27956.4 (10  0.2%)       27930.5 (10  0.2%)  -0.1%
         RayTrace:       84240.6 (10  2.3%)       83804.0 (10  1.5%)  -0.5%
      EarleyBoyer:       32772.0 (10  1.1%)       32776.2 (10  0.6%)   0.0%
           RegExp:        3181.2 (10  0.7%)        3160.2 (10  1.4%)  -0.7%
            Splay:       17186.0 (10  2.6%)       15816.4 (10  2.1%)  -8.0%
     SplayLatency:       11166.9 (10  2.5%)        9987.5 (10  4.6%) -10.6%
     NavierStokes:       32857.6 (10  0.2%)       32862.5 (10  0.1%)   0.0%
            PdfJS:       19889.8 (10  1.3%)       19879.8 (10  1.6%)  -0.1%
         Mandreel:       31954.1 (10  1.5%)       31824.2 (10  1.8%)  -0.4%
  MandreelLatency:       39198.6 (10  3.3%)       37948.1 (10  1.6%)  -3.2%
          Gameboy:       54262.8 (10  0.5%)       54002.2 (10  0.4%)  -0.5%
         CodeLoad:       19278.4 (10  1.1%)       19297.5 (10  1.0%)   0.1%
            Box2D:       37283.2 (10  1.5%)       36613.0 (10  3.2%)  -1.8%
             zlib:       84747.1 (10  0.3%)       84794.3 (10  0.3%)   0.1%
       Typescript:       28699.6 (10  2.0%)       26181.4 (10  2.3%)  -8.8%
Score (version 9):       27713.9 (10  0.6%)       27101.6 (10  0.7%)  -2.2%

This is probably because the heap is growing slower now for tests that use > 150MB of memory.  Of course, we could adjust the settings (in both shell and browser) to accommodate larger workloads.

Comment 2

[Terrence Cole [:terrence]]

Comment on attachment 8445761 [details] [diff] [review]
turn-on-dynamic-heap-growth

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

Well, we obviously don't want to take a 2% regression: let's try an take a 2% win in the browser instead.

Comment 3

[Jon Coppeard (:jonco)]

Attachment: 1 - Factor out calculation of highFrequencyGC flag

Currently we calculate the highFrequencyGC flag repeatedly for every zone.  This patch factors it out so we only set it once.

Comment 4

[Jon Coppeard (:jonco)]

Attachment: 2 - Update growth factor and triggers once after background sweep

Currently we calculate the heap growth factor for a zone once at the end of GC, and calculate the trigger based on it as:

  trigger = heap_used * factor

Unfortunately we don't actually know this the amount of heap used after GC yet as background sweeping has not run, so we adjust the trigger every time we free an arena during background sweep.

The problem with this is that the factor is also dependent on heap_used, and this does not get recalculated.

For workloads that generate a lot of garbage that is swept in the background, this means that dynamic heap growth may not be used as the initial heap used value falls outside the applicable range, even though the final value is inside that range.

Instead, we should calculate the factor and the triggers twice: once at the end of GC, and again after background sweeping has finished when we know the final data.

Comment 5

[Jon Coppeard (:jonco)]

Attachment: 3 - Enable dynamic heap growth

With the previous two patches, we can enable dynamic heap growth in the shell and get parity on the octane benchmark.

Comment 6

[Terrence Cole [:terrence]]

Comment on attachment 8448613 [details] [diff] [review]
1 - Factor out calculation of highFrequencyGC flag

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

Nice! r=me

Comment 7

[Terrence Cole [:terrence]]

Comment on attachment 8448616 [details] [diff] [review]
2 - Update growth factor and triggers once after background sweep

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

r=me

::: js/src/jsgc.cpp
@@ +2551,5 @@
>                  ArenaHeader *arenas = zone->allocator.arenas.arenaListsToSweep[kind];
>                  if (arenas)
>                      ArenaLists::backgroundFinalize(&fop, arenas, onBackgroundThread);
>              }
> +

Why the extra space here?

@@ +4477,5 @@
>      highFrequencyGC = dynamicHeapGrowth && lastGCTime &&
>          lastGCTime + highFrequencyTimeThreshold * PRMJ_USEC_PER_MSEC > currentTime;
>  
>      for (ZonesIter zone(rt, WithAtoms); !zone.done(); zone.next()) {
> +        zone->setGCLastBytes(zone->gcBytes, lastKind);

Shouldn't we still be using gckind on the foreground thread and only use lastKind on the background thread? It looks like |lastKkind| won't even be initialized on the first GC.

Comment 8

[Terrence Cole [:terrence]]

Comment on attachment 8448620 [details] [diff] [review]
3 - Enable dynamic heap growth

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

\o/

Comment 9

[Jon Coppeard (:jonco)]

(In reply to Terrence Cole [:terrence] from comment #7)
> Shouldn't we still be using gckind on the foreground thread and only use
> lastKind on the background thread? It looks like |lastKkind| won't even be
> initialized on the first GC.

I set this in incrementalCollectSlice() so it should will be set at this point.

Comment 10

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/020c3ec13ba6
https://hg.mozilla.org/integration/mozilla-inbound/rev/4cc5a9749735
https://hg.mozilla.org/integration/mozilla-inbound/rev/c609e4ddcff6

Comment 11

[Jon Coppeard (:jonco)]

Backed out the patch which turns this on because of assertion failures on XPCShell tests on B2G emulator.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3081a82b0ae3

Comment 12

[Carsten Book [:Tomcat]]

sorry had to backout this changes for test failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=42936365&tree=Mozilla-Inbound starting with this patch included

Comment 13

[Jon Coppeard (:jonco)]

The test failures seem to have been triggered by the fix for bug 1032750 which was pushed together with these patches.

Comment 14

[Jon Coppeard (:jonco)]

Attachment: 3 - Enable dynamic heap growth v2

I had to update this to fix an assertion failure.  We assert that highFrequencyLowLimitBytes < highFrequencyHighLimitBytes, but we don't check this anywhere and allow callers to set anything they like.  Not only that, but since these are set individually then the invariant can be temporarily violated between two calls to JS_SetGCParameter().  If these is done from JS, then we can GC in this time.

Ideally the API would take both parameters at the same time and return and error, but we don't have that.

Instead, we can adjust the other parameter whenever the invariant would be violated.  This isn't great but will work fine in practice.

Comment 15

[Terrence Cole [:terrence]]

Comment on attachment 8449558 [details] [diff] [review]
3 - Enable dynamic heap growth v2

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

Yup. Fine for now.

Comment 16

[Jon Coppeard (:jonco)]

Re-landed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/c76b804566e9
https://hg.mozilla.org/integration/mozilla-inbound/rev/41da9bcec12e
https://hg.mozilla.org/integration/mozilla-inbound/rev/cad7de31731e

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c76b804566e9
https://hg.mozilla.org/mozilla-central/rev/41da9bcec12e
https://hg.mozilla.org/mozilla-central/rev/cad7de31731e

Comment 18

[Guilherme Lima]

It seems this patch regressed a few Sunspider tests on the Firefox OS AWFY machine.

Comment 19

[Nicolas B. Pierron [:nbp]]

(In reply to Guilherme Lima from comment #18)
> It seems this patch regressed a few Sunspider tests on the Firefox OS AWFY
> machine.

To be more precise, this only regressed on the FxOS Flame devices[0] and not on Unagis nor on Firefox for Android (Flame).

And this correlate well with the first landing[1], backout[2] and re-landing[3].

Jon, should we backout, or you can find a fix within the next two days?
Do you have everything for testing on a flame?

[0] http://arewefastyet.com/?a=b&view=regress#machine=26&view=breakdown&suite=ss
[1] http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=3e013301d93abf7a9a657d7ba7b0377a9c1c560d&tochange=60133a85f8aedb4b1bf05f3fbe4ff28c62e1bf02
[2] http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=60133a85f8aedb4b1bf05f3fbe4ff28c62e1bf02&tochange=4f5e337cfa8361aee02a43f95616f432c072fb4a
[3] http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=b9006622fcecb6fc505b3c710a31ea13a19ed207&tochange=a133199d09e2c9cc834ebbc51c92c07ff399bcbe

Comment 20

[Jon Coppeard (:jonco)]

The flame benchmarking is done in a browser running on the devices, so I don't know why this has affected it.  As per nbp's suggestion I'm going to back this out one patch at a time so we will see exactly what caused this.

Comment 21

[Jon Coppeard (:jonco)]

Backout of patch 3:

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

Comment 22

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

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

Comment 23

[Jon Coppeard (:jonco)]

Backout of patch 2:

https://hg.mozilla.org/integration/mozilla-inbound/rev/4bb01d74359a

Comment 24

[Ryan VanderMeulen [:RyanVM]]

(In reply to Jon Coppeard (:jonco) from comment #23)
> Backout of patch 2:
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/4bb01d74359a

Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/4bb01d74359a

Comment 25

[Jon Coppeard (:jonco)]

The previous backout has fixed the Sunspider regression on Flame devices.

Comment 26

[Terrence Cole [:terrence]]

Attachment: fix_a_typo-v0.diff

Perhaps this typo is what's throwing a wrench in the works?

Comment 27

[Jon Coppeard (:jonco)]

Comment on attachment 8460376 [details] [diff] [review]
fix_a_typo-v0.diff

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

Ouch!  Nice find.

Comment 28

[Jon Coppeard (:jonco)]

I've tested the patch in comment 26, as well as removing the new use of atomics, and neither fix the problem.

Comment 29

[Terrence Cole [:terrence]]

(In reply to Jon Coppeard (:jonco) from comment #28)
> I've tested the patch in comment 26, as well as removing the new use of
> atomics, and neither fix the problem.

Oh well, worth a shot.

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

Comment 30

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

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

Comment 31

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Anything left here, Jon? It'll be nice to have the shell act more like the browser in some ways.

Comment 32

[Jon Coppeard (:jonco)]

Comment on attachment 8449558 [details] [diff] [review]
3 - Enable dynamic heap growth v2

Sadly this got backed out.

Comment 33

[Jon Coppeard (:jonco)]

Dynamic heap grown got enabled in the fix for bug 958492 so closing this.

Comment 34

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.