[webkit-reviews] review granted: [Bug 87039] GC allocation trigger should be tuned to system RAM : [Attachment 143323] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Geoffrey Garen <ggaren at apple.com>'s
request for review:
Bug 87039: GC allocation trigger should be tuned to system RAM
https://bugs.webkit.org/show_bug.cgi?id=87039

Attachment 143323: Patch
https://bugs.webkit.org/attachment.cgi?id=143323&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=143323&action=review


> Source/JavaScriptCore/heap/Heap.cpp:49
> +static const size_t largeHeapSize = 32 * MB;
> +static const size_t smallHeapSize = 1 * MB;

Can you add “why” comments?

> Source/JavaScriptCore/heap/Heap.cpp:158
> +    if (heapSize < ramSize / 4)
> +	   return 2 * heapSize;
> +    if (heapSize < ramSize / 2)
> +	   return 1.5 * heapSize;
> +    return 1.25 * heapSize;

Can you add “why” comments?

> Source/JavaScriptCore/heap/Heap.cpp:237
> +    , m_ramSize(WTF::amountOfRAM())

The bug title calls this “system RAM” but you chose the term “amount of RAM”.
Also not clear why an explicit WTF prefix is needed or helpful.

> Source/WTF/wtf/AmountOfRAM.cpp:76
> +    static size_t amountOfRAM = computeAmountOfRAM();

Could be static const.

> Source/WTF/wtf/AmountOfRAM.h:31
> +WTF_EXPORT_PRIVATE size_t amountOfRAM();

This file is missing "using WTF::amountOfRAM".

> Source/WTF/wtf/StdLibExtras.h:110
>  static const size_t KB = 1024;

Should be lowercase K. Not for this patch, though.