1492720 - Incorrect tenure rate calculation used for nursery size

Status: RESOLVED WONTFIX

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

Description

[Paul Bone [:pbone]]

The tenure rate calculations in maybeResizeNursery are based on nursery size and not used size.

Comment 1

[Jon Coppeard (:jonco)]

Just wanted to document that we do this because it works better even if it's not 'correct':

https://searchfox.org/mozilla-central/source/js/src/gc/Nursery.cpp#1217

Comment 2

[Paul Bone [:pbone]]

Yeah, I want to investigate if it makes sense to do it correctly and get the same or better performance.   Good call to place that info here in the bug.  Here's the comment in case the above URL moves:

    /*
     * This incorrect promotion rate results in better nursery sizing
     * decisions, however we should to better tuning based on the real
     * promotion rate in the future.
     */
    const float promotionRate =
        float(previousGC.tenuredBytes) / float(previousGC.nurseryCapacity);

Comment 3

[Paul Bone [:pbone]]

Attachment: Bug 1492720 (Part 1) - Use the nursery size for the tenure rate calculation

Also adjust the tenure rate thresholds to 20% for growth and 5% for
shrinking.  These seem to perform just as well as the previous settings with
the broken calculation.

Comment 4

[Paul Bone [:pbone]]

Attachment: Bug 1492720 (Part 2) - Only grow the nursery almost all chunks were used

Comment 5

[Paul Bone [:pbone]]

These patches perform as well or almost as well as the central:

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

And it seems to improve memory usage, at least on windows, which I wasn't expecting.

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

Comment 6

[Jon Coppeard (:jonco)]

Comment on attachment 9014266 [details] [diff] [review]
Bug 1492720 (Part 1) - Use the nursery size for the tenure rate calculation

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

::: js/src/gc/Nursery.cpp
@@ +1195,5 @@
>  void
>  js::Nursery::maybeResizeNursery(JS::gcreason::Reason reason)
>  {
> +    static const float GrowThreshold   = 0.20;
> +    static const float ShrinkThreshold = 0.05;

This is a really drastic change to these thresholds.  The new shrink threshold is now higher than the old grow threshold.  Do you have evidence that this will work better in practice?  I suspect that this will just result in us keeping the nursery at the smallest size.

Comment 7

[Jon Coppeard (:jonco)]

Comment on attachment 9014267 [details] [diff] [review]
Bug 1492720 (Part 2) - Only grow the nursery almost all chunks were used

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

This has a similar effect to the original 'incorrect' calculation: don't grown the nursery unless we've used most of it.

As a first step I think we should test this change with correct promotion rate calculation and the existing thresholds.  If that doesn't cause problems we can look at adjusting the thresholds if necessary.

::: js/src/gc/Nursery.cpp
@@ +1231,5 @@
>  
> +    // Only grow the nursery if we allocated more than half way into the final
> +    // chunk.
> +    const bool almostUsedAllChunks =
> +        (previousGC.nurseryUsedBytes + unsigned(ChunkSize*0.5)) >> ChunkShift == maxChunkCount();

Please make this a more natural comparison, e.g.:

  usedBytes >= nurserySize - (ChunkSize / 2)

Comment 8

[Jon Coppeard (:jonco)]

The second (In reply to Paul Bone [:pbone] from comment #5)
> These patches perform as well or almost as well as the central:

You only seem to have tested speedometer and stylebench.  Please run all talos and awfy tests to check the impact of changes like this.

Comment 9

[Paul Bone [:pbone]]

(In reply to Jon Coppeard (:jonco) (PTO until 9th October) from comment #8)
> The second (In reply to Paul Bone [:pbone] from comment #5)
> > These patches perform as well or almost as well as the central:
> 
> You only seem to have tested speedometer and stylebench.  Please run all
> talos and awfy tests to check the impact of changes like this.

I ran more tests, some have quite a bit worse results like tp6_amazon, I looked at the profile but nothing was obvious.  Sure the nursery size behaves differently but there's not enough GC activity to really account for this (kinda gut-feeling-y)  The perfherder history tracking shows some other benchmarks on central that have similar scores, so I'm going to run more tests and see.

(In reply to Jon Coppeard (:jonco) (PTO until 9th October) from comment #7)
> Comment on attachment 9014267 [details] [diff] [review]
> Bug 1492720 (Part 2) - Only grow the nursery almost all chunks were used
> 
> Review of attachment 9014267 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This has a similar effect to the original 'incorrect' calculation: don't
> grown the nursery unless we've used most of it.

I disagree.  The original code perfromed this calculation on every minor collection and used the nursery capacity.  Therefore on a full slots buffer or interslice GC it would perform the calculation, even if there were only a few KB in the nursery used and they were all tenured, could potentially calculate a very small tenure rate compared with the large capacity.

The new code refuses to grow the nursery if if at least half the last chunk wasn't used.  Meaning that if there's an interslice GC where only a few KB are used, no tenure rate will be calculated and the nursery will not grow.

> As a first step I think we should test this change with correct promotion
> rate calculation and the existing thresholds.  If that doesn't cause
> problems we can look at adjusting the thresholds if necessary.

I think they make sense together, I'll reply below.  The thresholds are adjusted (and I tuned them to speedometer as best I could) with the new promotion rate calculation.

If the nursery is 10MB big, 1MB is used, and 256K is tenured.  Then the old tenure rate would be calculated as 2.5%.  And a low-ish threshold makes sense (but only when the nursery is not full).  With the new calculation, the tenure rate is correctly calculated at 25%, where the higher threshold makes sense, and also makes sense when the nursery is more full.

In the case when the nursery was full.  Let's say we tenure 1MB out of a 10MB nursery, both calculations would say 10%.  And the old one would grow the nursery because that's way bigger than the old threshold (3%), and the new thresholds would keep the size the same.  Differences in performance are most likely to occur when the nursery is full and the new calculation would be less likely to grow the nursery.

I initially wanted to make each of these nursery tuning changes on different bugzilla bugs, so their effects could be tracked seperately.  However now I'm concerned that this will result in "hill climbing" and we'd miss out on a bigger picture kind-of performance improvment because we'd be exploring each change seperately.

Comment 10

[Paul Bone [:pbone]]

P2 for now, could be P3 but that I've already started working on it and it'd be nice to complete it.  It may depend on further work such as improving tenureing performance (no bug).

Comment 11

[Jon Coppeard (:jonco)]

(In reply to Paul Bone [:pbone] from comment #9)
> I disagree.  The original code perfromed this calculation on every minor
> collection and used the nursery capacity.  Therefore on a full slots buffer
> or interslice GC it would perform the calculation, even if there were only a
> few KB in the nursery used and they were all tenured, could potentially
> calculate a very small tenure rate compared with the large capacity.
> 
> The new code refuses to grow the nursery if if at least half the last chunk
> wasn't used.  Meaning that if there's an interslice GC where only a few KB
> are used, no tenure rate will be calculated and the nursery will not grow.

Right, so in the original case an early collection will calculate a very small tenure rate (because capacity is larger than used size) and therefore not grow the nursery, and with this patch we would see that the used size was small and therefore not grow the nursery.  Hence the effect is similar.  Or is there something else happening here?

Comment 12

[Paul Bone [:pbone]]

(In reply to Jon Coppeard (:jonco) from comment #11)
> (In reply to Paul Bone [:pbone] from comment #9)
> > I disagree.  The original code perfromed this calculation on every minor
> > collection and used the nursery capacity.  Therefore on a full slots buffer
> > or interslice GC it would perform the calculation, even if there were only a
> > few KB in the nursery used and they were all tenured, could potentially
> > calculate a very small tenure rate compared with the large capacity.
> > 
> > The new code refuses to grow the nursery if if at least half the last chunk
> > wasn't used.  Meaning that if there's an interslice GC where only a few KB
> > are used, no tenure rate will be calculated and the nursery will not grow.
> 
> Right, so in the original case an early collection will calculate a very
> small tenure rate (because capacity is larger than used size) and therefore
> not grow the nursery, and with this patch we would see that the used size
> was small and therefore not grow the nursery.  Hence the effect is similar. 
> Or is there something else happening here?

I think that's a fairly good summary.  My intuition is that if you're not going to use all the nursery capacity, particularly on a regular basis, then there's no point in growing the nursery.