Dynamic heap growth is not enabled in the shell

RESOLVED WORKSFORME

Status

()

Core
JavaScript: GC
RESOLVED WORKSFORME
4 years ago
3 years ago

People

(Reporter: jonco, Assigned: jonco)

Tracking

(Blocks: 1 bug, {leave-open})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 2 obsolete attachments)

(Assignee)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
Created attachment 8445761 [details] [diff] [review]
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.
Attachment #8445761 - Flags: review?(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.
Attachment #8445761 - Flags: review?(terrence)
(Assignee)

Comment 3

4 years ago
Created attachment 8448613 [details] [diff] [review]
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.
Attachment #8445761 - Attachment is obsolete: true
Attachment #8448613 - Flags: review?(terrence)
(Assignee)

Comment 4

4 years ago
Created attachment 8448616 [details] [diff] [review]
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.
Attachment #8448616 - Flags: review?(terrence)
(Assignee)

Comment 5

4 years ago
Created attachment 8448620 [details] [diff] [review]
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.
Attachment #8448620 - Flags: review?(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
Attachment #8448613 - Flags: review?(terrence) → review+
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.
Attachment #8448616 - Flags: review?(terrence) → review+
Comment on attachment 8448620 [details] [diff] [review]
3 - Enable dynamic heap growth

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

\o/
Attachment #8448620 - Flags: review?(terrence) → review+
(Assignee)

Comment 9

4 years ago
(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.
(Assignee)

Updated

4 years ago
Keywords: leave-open
(Assignee)

Comment 11

4 years ago
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
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
(Assignee)

Comment 13

4 years ago
The test failures seem to have been triggered by the fix for bug 1032750 which was pushed together with these patches.
(Assignee)

Comment 14

4 years ago
Created attachment 8449558 [details] [diff] [review]
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.
Attachment #8449558 - Flags: review?(terrence)
(Assignee)

Updated

4 years ago
Attachment #8448620 - Attachment is obsolete: true
Comment on attachment 8449558 [details] [diff] [review]
3 - Enable dynamic heap growth v2

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

Yup. Fine for now.
Attachment #8449558 - Flags: review?(terrence) → review+

Comment 18

4 years ago
It seems this patch regressed a few Sunspider tests on the Firefox OS AWFY machine.
(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
Flags: needinfo?(jcoppeard)
(Assignee)

Comment 20

4 years ago
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.
Flags: needinfo?(jcoppeard)
(Assignee)

Comment 25

4 years ago
The previous backout has fixed the Sunspider regression on Flame devices.
Blocks: 1008333
Blocks: 958492
Created attachment 8460376 [details] [diff] [review]
fix_a_typo-v0.diff

Perhaps this typo is what's throwing a wrench in the works?
Attachment #8460376 - Flags: review?(jcoppeard)
(Assignee)

Comment 27

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

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

Ouch!  Nice find.
Attachment #8460376 - Flags: review?(jcoppeard) → review+
(Assignee)

Comment 28

4 years ago
I've tested the patch in comment 26, as well as removing the new use of atomics, and neither fix the problem.
(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
Attachment #8448613 - Flags: checkin+
Attachment #8449558 - Flags: checkin+
Attachment #8460376 - Flags: checkin+
Anything left here, Jon? It'll be nice to have the shell act more like the browser in some ways.
Flags: needinfo?(jcoppeard)
(Assignee)

Comment 32

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

Sadly this got backed out.
Attachment #8449558 - Flags: checkin+
(Assignee)

Comment 33

3 years ago
Dynamic heap grown got enabled in the fix for bug 958492 so closing this.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Flags: needinfo?(jcoppeard)
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.