1436697 - Heap growth factor limits are too low

Status: RESOLVED FIXED

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

Description

[Jon Coppeard (:jonco)]

Bug 995165 added lower limits for the heap growth factor parameters to prevent the GC trigger threshold decreasing over time given a steady allocation rate.  This would lead to us doing more and more frequent GCs over time.  However due to a bug the limits are too low.

The limit is not 1, since we can GC before we hit the threshold. ZoneHeapThreshold::allocTrigger() which returns either 0.85 or 0.9 of the current threshold.  So the limit should be e.g. 1 / 0.85 as described in the original bug.

We should also make constants for all these things.

Comment 1

[Jon Coppeard (:jonco)]

The problem is that asserts like:

    MOZ_ASSERT(rt->gcHighFrequencyHeapGrowthMax / 0.85 > 1.0);

Should really have been:

    MOZ_ASSERT(rt->gcHighFrequencyHeapGrowthMax > 1.0 / 0.85);

Comment 2

[Jon Coppeard (:jonco)]

Attachment: bug1436697-growth-factor-limits

This patch introduces a minimum for the JSGC_ALLOCATION_THRESHOLD_FACTOR and JSGC_ALLOCATION_THRESHOLD_FACTOR_AVOID_INTERRUPT parameters and calculates minimum permissable heap growth factors based on that and on the eager alloc trigger factors.  Then we check against those.

I made the eager alloc trigger factors into constants and renamed ZoneHeapThreshold::allocTrigger() to eagerAllocTrigger() for make it clearer what it's used for.

I had to change some floats to doubles because I found comparisions between floats and doubles were causing some assertions to fail since e.g. (float)0.9 < (double)0.9.

Comment 3

[Paul Bone [:pbone]]

Comment on attachment 8949720 [details] [diff] [review]
bug1436697-growth-factor-limits

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

I'd have prefered to stick with floats since we just don't need the precision of doubles and can save on some memory, but since most the code base seems to default to doubles I agree that it's often just easier to keep using those.

If my understanding is correct on jsgc.cpp:1350 then r+, otherwise I'd like to understand this before I can pass it.

::: js/src/jsgc.cpp
@@ +284,5 @@
>      /* JSGC_MAX_MALLOC_BYTES */
>      static const size_t MaxMallocBytes = 128 * 1024 * 1024;
>  
>      /* JSGC_ALLOCATION_THRESHOLD_FACTOR */
> +    static const double AllocThresholdFactor = 0.9f;

Remove the "f" literal suffix.  (and below)

@@ +1346,5 @@
>          break;
>        }
>        case JSGC_HIGH_FREQUENCY_HEAP_GROWTH_MAX: {
>          double newGrowth = value / 100.0;
> +        if (newGrowth < MinHighFrequencyHeapGrowthFactor || newGrowth > MaxHeapGrowthFactor)

Above MinHighFrequencyHeapGrowthFactor is the recipricol of HighFrequencyEagerAllocTriggerFactor (because it is the minimum, at 0.85)  Which makes MinHighFrequencyHeapGrowthFactor 1.17-ish.  Previously we compared newGrowth against 0.85, not it's recipricol.  It looks like this new code is correct and the old code was a bug.  Is that right?

Comment 4

[Jon Coppeard (:jonco)]

(In reply to Paul Bone [:pbone] from comment #3)
> I'd have prefered to stick with floats since we just don't need the
> precision of doubles and can save on some memory, but since most the code
> base seems to default to doubles I agree that it's often just easier to keep
> using those.

As you say we use doubles everywhere else, and the memory savings are negligible here.

> Remove the "f" literal suffix.  (and below)

Cheers, removed.

> Above MinHighFrequencyHeapGrowthFactor is the recipricol of
> HighFrequencyEagerAllocTriggerFactor (because it is the minimum, at 0.85) 
> Which makes MinHighFrequencyHeapGrowthFactor 1.17-ish.  Previously we
> compared newGrowth against 0.85, not it's recipricol.  It looks like this
> new code is correct and the old code was a bug.  Is that right?

Yes, see the previous comments.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/37c5d7afbe4b
Fix GC heap growth factor limits r=pbone

Comment 6

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/37c5d7afbe4b