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.

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

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

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.

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

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 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 on attachment 8448620 [details] [diff] [review] 3 - Enable dynamic heap growth Review of attachment 8448620 [details] [diff] [review]: ----------------------------------------------------------------- \o/

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

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

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

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

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 on attachment 8449558 [details] [diff] [review] 3 - Enable dynamic heap growth v2 Review of attachment 8449558 [details] [diff] [review]: ----------------------------------------------------------------- Yup. Fine for now.

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

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

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

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.

Backout of patch 3: https://hg.mozilla.org/integration/mozilla-inbound/rev/c6ca2340a718

Backout of patch 2: https://hg.mozilla.org/integration/mozilla-inbound/rev/4bb01d74359a

(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

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

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

Comment on attachment 8460376 [details] [diff] [review] fix_a_typo-v0.diff Review of attachment 8460376 [details] [diff] [review]: ----------------------------------------------------------------- Ouch! Nice find.

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

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

Comment on attachment 8449558 [details] [diff] [review] 3 - Enable dynamic heap growth v2 Sadly this got backed out.

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

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