[webkit-reviews] review granted: [Bug 177586] Enable gigacage on iOS : [Attachment 322985] the patch

[bugzilla-daemon at webkit.org]

JF Bastien <jfbastien at apple.com> has granted Filip Pizlo <fpizlo at apple.com>'s
request for review:
Bug 177586: Enable gigacage on iOS
https://bugs.webkit.org/show_bug.cgi?id=177586

Attachment 322985: the patch

https://bugs.webkit.org/attachment.cgi?id=322985&action=review




--- Comment #21 from JF Bastien <jfbastien at apple.com> ---
Comment on attachment 322985
  --> https://bugs.webkit.org/attachment.cgi?id=322985
the patch

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

r=me with some comments.

> Source/JavaScriptCore/ChangeLog:8
> +	   The hardest part of enabling Gigacage on iOS is that it requires
loading global variables whil

"while"

> Source/bmalloc/bmalloc/Gigacage.cpp:108
> +} // anonymous namespce

I like that you moved the typo too.

> Source/bmalloc/bmalloc/Gigacage.h:41
> +#define GIGACAGE_ALLOCATION_CAN_FAIL 1

Can this be an option instead? Seems like we want to be able to disable it.

It would also be nice for the changelog to say what happens when it goes from
using gigacage to it failing. What happens to allocations then, is it
monotonic, etc.

> Source/bmalloc/bmalloc/Gigacage.h:72
> +BINLINE bool wasEnabled() { return g_wasEnabled; }

Can you just export the function, instead of exposing the bool?

> Source/bmalloc/bmalloc/HeapKind.h:97
> +    default:

I'd just make this Primary and have no default. That way the compiler will yell
if another kind is added.

> Source/bmalloc/bmalloc/HeapKind.h:113
> +    default:

I'd just make this Primary and have no default. That way the compiler will yell
if another kind is added.