1497873 - Investigate improving the nursery resizing heuristic

Status: RESOLVED FIXED

(Core :: JavaScript: GC, enhancement, P1)

Description

[Jon Coppeard (:jonco)]

Currently we resize the nursery depending on the promotion rate and the size of the nursery used, and according to some thresholds we can double the size of the nursery or shrink it by one chunk.

The problem with this is that growth is very aggressive while shrinking can take a while to have any effect and that the nursery size can end up growing and shrinking a lot even for a uniform workload.

It might make more sense instead to estimate an optimal nursery size based on the data we have, possibly using a moving average, and then resize to that.

Comment 1

[Paul Bone [:pbone]]

Depends on Bug 1433007 because that bug complicates the GC size calculation and is already underway.

Blocks Bug 1449600 because this may make the nursery size more stable and therefore be "nicer" to the chunk allocator.

Related to Bug 1492720 because both bugs propose to change the nursery size calculation.

Comment 2

[Paul Bone [:pbone]]

I'd like to work on this, I think the nursery size stability it should provide will make some of my other work have more tangible results.

Comment 3

[Paul Bone [:pbone]]

Attachment: Bug 1497873 - Add a kind-of goal seeking herustic for nursery resizing

This seems to improve some stability in nursery sizes.  Also the nursery is now very quick to react to changing workloads, maybe too quick.  For example in the some benchmarks as it switches between tasks the "switch" of tasks has different memory allocation patterns, the nursery reacts to this and then starts the next benchmark larger than necessary.  We could prerhaps add back in the condition that the current and previous promotion rates both exceed a threshold before we resize the nursery.

Comment 4

[Paul Bone [:pbone]]

Here's the first set of differences with this patch:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=e92a6d2427ba&framework=1&showOnlyComparable=1&selectedTimeRange=172800

There are some sagnificant size improvments but also some perf regressions.

Comment 5

[Paul Bone [:pbone]]

Attachment: Bug 1497873 - Resizing also depends on previous promotion rate

It seemed that the Nursery was adjusting it's size too quickly.  So I added this patch that restores the behaviour where it needs two collections with a high or low promotion rate before adjusting its size.

Comment 6

[Paul Bone [:pbone]]

And here are the results including the second patch, still needs some investigation:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=3f00d2abd8cf&framework=1&showOnlyComparable=1&selectedTimeRange=172800

Comment 7

[Paul Bone [:pbone]]

In the ares6 test and possibly others we were getting unlucky and hit a high promotion rate while the nursery was small. causing the wrong objects to get pretenured, then those cause other objects to be tenured and the generational GC stops being an optimisation.

Comment 8

[Jon Coppeard (:jonco)]

(In reply to Paul Bone [:pbone] from comment #7)
These patches seem reasonable but as you note there are still some regressions.

In order to compare with the current setup you post some numbers illustrating how the nursery size changes over time for benchmarks that make particular use of the nursery?

Comment 9

[Paul Bone [:pbone]]

Attachment: Tweaking the pretenure condition

Here is another patch I'm testing with, it may commit on this bug or maybe by itself.

Comment 10

[Paul Bone [:pbone]]

As I noted (comment 7) the ares6 test got unlucky with 3 specific object groups getting pretenured, causing a lot more nursery objects surviving the nursery and being evicted, which also meant more regular GC activity.

I've added another patch which refused to make a pretenuring decision if the nursery is less than 4MB.  My thought that if the nursery is small and we promoted a lot, we should probably grow the nursery before we make a pretenuring decision.  This works out and the test is improved.  However there's still some regressions.  One of the interesting comparisions is:

pretenure tweak vs pretenure tweak + nursery sizing change:

All the tests:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=c985a960915e&framework=1&showOnlyComparable=1&selectedTimeRange=172800

Ares6:
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=c985a960915e&newProject=try&newRevision=eefa2a8c1919&originalSignature=0bae7e5fb769123dc4504aea0458898f63bee0dd&newSignature=0bae7e5fb769123dc4504aea0458898f63bee0dd&framework=11

So there's still something sub-optimal about this new nursery sizing change.  The pretenuring tweak on its own doesn't change much (there's some slight changes but it's hard to be sure), as it is it's only valuable if we also commit the sizing changes.

Comment 11

[Paul Bone [:pbone]]

(In reply to Jon Coppeard (:jonco) from comment #8)
> (In reply to Paul Bone [:pbone] from comment #7)
> These patches seem reasonable but as you note there are still some
> regressions.
> 
> In order to compare with the current setup you post some numbers
> illustrating how the nursery size changes over time for benchmarks that make
> particular use of the nursery?

I'll see what I can do.

Comment 12

[Paul Bone [:pbone]]

Attachment: Test results showing changes in nursery size for Ares6 Air test.

Comment 13

[Paul Bone [:pbone]]

Attachment: Test results showing changes in nursery size for Ares6 Babylon test.

Here the Nightly version starts with a 10MB nursery, while the test version starts with a 1MB Nursery.  That's because with the patched code the previous test (Basic) drops its nursery size quickly, while the nightly version does not - and sees a higher tenure rate.

Comment 14

[Paul Bone [:pbone]]

Okay, based on that data I've tweaked it a little further (patches soon).  What I've got now is it only consideres the current tenure rate (not the previous one) and it will at most double or halve the nursery.  We needed the halving because sometimes you'd see a really low tenure rate, the nursery size would drop to 1MB, then take 4 collections to restore itself to 16MB.

Here's the tests, comparing a specific version of central, against my patches:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=98c53f6ef2ba&newProject=try&newRevision=8fa0a2c613d9&framework=1&showOnlyConfident=1

There might be some regressions with about_preferences and tp6_facebook,  Neither do much GC work (I'm looking at the profiles generated from those tests) so I'm not sure why there's a difference.  I'll tidy up the patches and share them later tonight.
?

Comment 15

[Paul Bone [:pbone]]

Attachment: Bug 1497873 - Add a kind-of goal seeking herustic for nursery resizing

The tests look good for this patch, but it must not land without Bug 1510560 without which it will regress.

Comment 16

[Jon Coppeard (:jonco)]

Attachment: charts.zip

To help evaluate changes to the nursery size heuristics, I wrote some scripts to generate charts of the size and tenure rate against time for various benchmarks (octane plus some of talos ones).  Here's the output, with and without the patch in this bug.  (I can post the scripts too if interested).

The results show some improvements (e.g. for EarleyBoyer the nursery size is more stable) and some regressions (e.g. for Splay it varies a lot more).

Comment 17

[Jon Coppeard (:jonco)]

I put the scripts to generate these charts on github: https://github.com/jonco3/nurserySizeCharts

Comment 18

[Jon Coppeard (:jonco)]

Comment on attachment 9028253 [details] [diff] [review]
Bug 1497873 - Add a kind-of goal seeking herustic for nursery resizing

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

Cancelling review request for now.

Comment 19

[Paul Bone [:pbone]]

So in Jon's results, and replicated in mine, Splay running in the shell sees a lot of floundering WRT the nursery size.  While most of the other benchmarks show fairly stable behavour for stable tenure rates, and react quickly to changing tenure rates.  I also get the sense (but no hard data) that considering the previous tenure rate, and tweaking the pretenure condition result in better behavour in these tests (the latter definitly improved ares when tested on try due to some unlucky pretenuring).

I've exprimented with a few things to remove the floundering in Splay, including a larger deadzone for the tenure rate (didn't work).  So far what has worked, and seems a bit funny, is the "correct" calculation for the tenure rate (considering nursery used size not capacity).  I'm going to start some try/talos jobs now with this change and see what it's like.

One other change that I'm on the fence about is how it should behave WRT the previous tenure rate.  Should it observe it both for growing and shrinking the nursery, or only growing (as before)?  It seems like shrinking quickly might be desirable, particularly if there's not going to be another minor GC for a little while as the app hits a steady state (when tenure rate drops anyway).

¯\_(ツ)_/¯

Comment 20

[Paul Bone [:pbone]]

My current patches with incorrect tenure rate calculation:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=6e55e0f8e303&newProject=try&newRevision=8fac29e891b3&framework=1&showOnlyConfident=1

This still has some regressions for ares6, but other tests seem good.  Including some size improvements.

With correct tenure rate calculation:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=6e55e0f8e303&newProject=try&newRevision=1c7c752cfa2e&framework=4&showOnlyConfident=1

Regresses awsy tests.

reminder to myself: Try to work out why ares6 regresses in the first comparison.  And for the 2nd, if we can have the correct tenure rate (which fixes the flip-floppy-ness of Splay) without regressing awsy.

Comment 21

[Paul Bone [:pbone]]

Attachment: Bug 1497873 - Tweaking pretenure condition

Comment 22

[Paul Bone [:pbone]]

So the problem with ares6 is in the babylon subtest (as always) there are more non-imcremental GCs with these patches due to GCBytesTrigger.

Comment 23

[Paul Bone [:pbone]]

Attachment: Bug 1497873 - Tweak the pretenure condition r=jonco!

Comment 24

[Paul Bone [:pbone]]

Attachment: Bug 1497873 - Add a kind-of goal seeking herustic for nursery resizing r=jonco!

Also allow the nursery to shrink up to half it's current size, previously
it'd be one chunk at a time. Growing is still capped at twice the current
size. These limits tend to prevent it changing too dramantically for small
changes in allocation patterns.

Depends on D17593

Comment 25

[Paul Bone [:pbone]]

Hi Jon,

I'd like to land this next week. The current talos/raptor/js-bench results look good. The big red marks on some graphics test is because someone recently landed something that improved those benchmarks but the changes I tested were based of an older central version.

https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=5a33f9b4e7466ecb0de6ba63cfae2699a7ab942d&framework=1&showOnlyComparable=1&showOnlyConfident=1&selectedTimeRange=86400

Note the raptor tests, some of those tests look very good.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/adcc2b05c708
Tweak the pretenure condition r=jonco
https://hg.mozilla.org/integration/autoland/rev/a3e1dfc5aae9
Add a kind-of goal seeking herustic for nursery resizing r=jonco

Comment 27

[Paul Bone [:pbone]]

NI perfherder people. This may change a few of our benchmarks, see https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=5a33f9b4e7466ecb0de6ba63cfae2699a7ab942d&framework=10&showOnlyComparable=1&showOnlyConfident=1&selectedTimeRange=86400 (unless it was reddit that changed their code).

I still want to know about any changes you happen to notice, but if you're trying to find a patch that cased a perf change, look here.

Comment 28

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/adcc2b05c708
https://hg.mozilla.org/mozilla-central/rev/a3e1dfc5aae9

Comment 29

[Joel Maher ( :jmaher ) (UTC -8)]

thanks :pbone!

:igoldan, :bebe, heads up- you should be seeing alerts by now

Comment 30

[Robert Wood [:rwood]]

Thanks Paul.

Comment 31

[Paul Bone [:pbone]]

Hrm, maybe the one I saw on try really was that reddit changed their code. Which is good because I didn't think this change would make that much difference.

Comment 32

[Paul Bone [:pbone]]

This has slightly reduced the nursery bytes telemetry probe:

http://alerts.telemetry.mozilla.org/index.html#/detectors/1/metrics/1931/alerts/?from=2019-01-29&to=2019-01-29