Bug 175041 – On memory-constrained iOS devices, reduce the rate at which the JS heap grows before a GC to try to keep more memory available for the system

• Status: REOPENED
• Component: JavaScriptCore

[Saam Barati]

patch forthcoming

[Radar WebKit Bug Importer]

<rdar://problem/33659370>

[Saam Barati]

Created attachment 316883 [details]
patch

[Filip Pizlo]

Comment on attachment 316883 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=316883&action=review

> Source/JavaScriptCore/heap/Heap.cpp:146
> +        double memoryUsed = bmalloc::api::percentAvailableMemoryInUse();
> +        double result = ((1 - memoryUsed) / 3.5) + 1;
> +        return heapSize * std::max(std::min(result, 2.0), 1.06);

Can we put these constants in Options?

[Filip Pizlo]

Comment on attachment 316883 [details]
patch

r=me with Options

[Saam Barati]

Created attachment 316886 [details]
patch for landing

[Filip Pizlo]

Comment on attachment 316886 [details]
patch for landing

seems good to me

[Saam Barati]

I'm holding off on landing this because I think I'm going to make it just apply to all devices with <= 1GB RAM on iOS.

[Saam Barati]

Created attachment 316937 [details]
patch for landing

[Saam Barati]

Created attachment 316943 [details]
patch for landing

fixed changelog to reflect the new heuristic.

[Simon Fraser (smfr)]

Can we retitle this something like "On memory-contained iOS devices, reduce the rate at which the JS heap grows to keep more memory available for the system" or something? Please re-word to make it accurate.

[Saam Barati]

(In reply to Simon Fraser (smfr) from comment #10)
> Can we retitle this something like "On memory-contained iOS devices, reduce
> the rate at which the JS heap grows to keep more memory available for the
> system" or something? Please re-word to make it accurate.

This wording SGTM. I’ll change and land once I’m in front of a computer.

[Mark Lam]

Comment on attachment 316943 [details]
patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=316943&action=review

> Source/JavaScriptCore/heap/Heap.cpp:121
> +static bool useAggressiveHeapTrigger()

If I understand the code correctly, the use of proportionalHeapSize() is for determining max heap size, and therefore, it determines when to trigger a GC.  I suggest renaming this function to useAggressiveGCTrigger.

> Source/JavaScriptCore/runtime/Options.h:216
> +    v(bool, forceAggressiveHeapTrigger, false, Normal, "If true, on iOS, we will use a different formula for proportionalHeapSize().") \
> +    v(double, aggressiveHeapTriggerMinumumMultiplier, 1.07, Normal, "This is the minimum we must grow by for proportionalHeapSize() when doing aggressive triggering.") \
> +    v(double, aggressiveHeapTriggerMaximumMultiplier, 2.0, Normal,  "This is the maximum we can grow by for proportionalHeapSize() when doing aggressive triggering.") \
> +    v(double, aggressiveHeapTriggerScalingValue, 3.5, Normal, "This scales the above formula. A larger number is more aggressive in limiting heap growth. A smaller number is more permissive in allowing heap growth.") \

I suggest renaming these options as follows:
forceAggressiveHeapTrigger => forceAggressiveGCTrigger
aggressiveHeapTriggerMinumumMultiplier => aggressiveGCTriggerMinMultiplier
aggressiveHeapTriggerMaximumMultiplier => aggressiveGCTriggerMaxMultiplier
aggressiveHeapTriggerScalingValue => aggressiveGCTriggerScalingValue

Min and max are so commonly use that anyone reading this code should understand them.  It'd be nice to have shorted options names.

[Mark Lam]

(In reply to Mark Lam from comment #12)
> It'd be nice to have shorted options names.

*shorter*.  Can't type today.

[Saam Barati]

landed in:
https://trac.webkit.org/changeset/220144/webkit

[WebKit Commit Bot]

Re-opened since this is blocked by bug 175276

[Saam Barati]

wont fix this, it didn't actually help perf.

[Saam Barati]

It actually looks like this might be a good idea. It turned out it was a memory improvement and a JetStream speedup.