[Webkit-unassigned] [Bug 80615] CopiedSpace::tryAllocateOversize assumes system page size

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=80615





--- Comment #11 from Filip Pizlo <fpizlo at apple.com>  2012-03-12 18:02:09 PST ---
(In reply to comment #7)
> (From update of attachment 131466 [details])
> 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().

This function should not be hot and even if it was, we'd have already lost.  So I think it should be fine to use WTF::pageSize() and WTF::pageMask() without making them inline.  That simplifies this patch, and has the nice property that we're not unnecessarily making things inline even though we're not using them in hot paths.

Geoff, do you agree?

> 
> > 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.

I concur with Geoff's other two comments.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.