[webkit-reviews] review denied: [Bug 80615] CopiedSpace::tryAllocateOversize assumes system page size : [Attachment 131466] Patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Myles C. Maxfield
<mmaxfield at google.com>'s request for review:
Bug 80615: CopiedSpace::tryAllocateOversize assumes system page size
https://bugs.webkit.org/show_bug.cgi?id=80615

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

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=131466&action=review


> Source/JavaScriptCore/heap/CopiedSpaceInlineMethods.h:174
> +    return reinterpret_cast<CopiedBlock*>(reinterpret_cast<size_t>(ptr) &
WTF::pageMask());

This function is hot, so I'd prefer not to turn a constant into two out-of-line
function calls.

I'd suggest making WTF::pageSize() and WTF::pageMask() inline function calls
that read globals inside WTF. You can initialize these globals inside
WTF::initializeThreading().

> Source/JavaScriptCore/wtf/StdLibExtras.h:76
> +#define ROUND_UP_TO_MULTIPLE_OF(divisor, x) \
> +    size_t remainderMask = divisor - 1; \
> +    return (x + remainderMask) & ~remainderMask;

I'd prefer to see this as an inline function. In fact, you've already written
that inline function. So now, all you need is to make you function template
call your other function.

> Source/JavaScriptCore/wtf/StdLibExtras.h:166
> +inline size_t roundUpToMultipleOf(size_t divisor, size_t x)

This function should ASSERT that "divisor" is a power of two.