Bug 80615 – CopiedSpace::tryAllocateOversize assumes system page size

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Myles C. Maxfield]

Crash in CopiedSpace::tryAllocateOversize:

1. CopiedSpace::tryAllocateOversize is called with a number of bytes, and a pointer to set (See the end of this email for a stack trace).
2. In order to get the block size, this function calls WTF::roundUpToMultipleOf<s_pageSize>(sizeof(CopiedBlock) + bytes); which attempts to round its argument up to a multiple of s_pageSize. This s_pageSize refers to JSC::CopiedSpace::s_pageSize, declared as a static const in JavaScriptCore/heap/CopiedSpace.h as 4 * KB, or 4096.
3. CopiedSpace::tryAllocateOversize then passes this block size to WTF::PageAllocationAligned::allocate, which immediately checks that this parameter, size, is page-aligned via a call to WTF::isPageAligned(size)
4. WTF::isPageAligned queries the system page size with WTF::pageSize(), which returns WTF::s_pageSize, which has been initialized to the system page size with a call to getpagesize(). On our system, this returns 16384.
5. WTF::isPageAligned returns false because the value that is divisible by 4096 is not divisible by 16384.
6. The assert in WTF::PageAllocationAligned::allocate fails.

Eliminating CopiedSpace::s_pageSize in favor of using WTF::s_pageSize would probably be the preferable route.

Stack trace:
std::size_t                     WTF::pageSize()
bool                            WTF::isPageAligned(unsigned int)
void                            WTF::PageAllocationAligned::allocate(unsigned int,unsigned int,WTF::OSAllocator::Usage,bool,bool)
void                            JSC::CopiedSpace::tryAllocateOversize(unsigned int,void**)
void                            JSC::CopiedSpace::tryReallocateOversize(void**,unsigned int,unsigned int)
void                            JSC::CopiedSpace::tryReallocate(void**,unsigned int,unsigned int)
void                            JSC::Heap::tryReallocateStorage(void**,unsigned int,unsigned int)
bool                            JSC::JSArray::increaseVectorLength(JSC::JSGlobalData&,unsigned int)
void                            JSC::JSArray::putByIndexBeyondVectorLength(JSC::ExecState*,unsigned int,JSC::JSValue)
void                            JSC::JSArray::putByIndex(JSC::JSCell*,JSC::ExecState*,unsigned int,JSC::JSValue)
JSC::EncodedJSValue             JSC::arrayProtoFuncPush(JSC::ExecState*)
JSC::JSValue                    JSC::Interpreter::privateExecute(JSC::Interpreter::ExecutionFlag,JSC::RegisterFile*,JSC::ExecState*)

[Myles C. Maxfield]

I'd like to claim this bug.

[Myles C. Maxfield]

Created attachment 131073 [details]
Patch

[Mark Hahnenberg]

Comment on attachment 131073 [details]
Patch

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

I'm not an official reviewer, but I figured I'd give you some suggestions for improvement for when an official reviewer does get around to review this :-)

> Source/JavaScriptCore/wtf/BumpPointerAllocator.h:114
> +        size_t poolSize = std::max(static_cast<size_t>(MINIMUM_BUMP_POOL_SIZE), WTF::pageSize());

What does this change have to do with refactoring CopiedSpace to use WTF::pageSize()?

> Source/JavaScriptCore/wtf/StdLibExtras.h:74
> +#define __ROUND_UP_TO_MULTIPLE_OF__ \

Why not make this a function macro? It's kind of confusing to assume variables names from the surrounding scope where the macro is invoked and to have a return statement in the macro itself. 

Also, naming things with two initial underscores is generally frowned upon because they're typically assumed to be reserved by the compiler.

[Mark Hahnenberg]

Comment on attachment 131073 [details]
Patch

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

I'm not an official reviewer, but I figured I'd give you some suggestions for improvement for when an official reviewer does get around to review this :-)

> Source/JavaScriptCore/wtf/BumpPointerAllocator.h:114
> +        size_t poolSize = std::max(static_cast<size_t>(MINIMUM_BUMP_POOL_SIZE), WTF::pageSize());

What does this change have to do with refactoring CopiedSpace to use WTF::pageSize()?

> Source/JavaScriptCore/wtf/StdLibExtras.h:74
> +#define __ROUND_UP_TO_MULTIPLE_OF__ \

Why not make this a function macro? It's kind of confusing to assume variables names from the surrounding scope where the macro is invoked and to have a return statement in the macro itself. 

Also, naming things with two initial underscores is generally frowned upon because they're typically assumed to be reserved by the compiler.

[Geoffrey Garen]

Comment on attachment 131073 [details]
Patch

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

r- based on Mark's comments.

> Source/JavaScriptCore/heap/CopiedSpaceInlineMethods.h:174
> +    size_t pageMask = ~(WTF::pageSize() - 1);

I'd prefer to see a WTF::pageMask() function, rather than one-off code in lots of places.

[Myles C. Maxfield]

Created attachment 131466 [details]
Patch

[Geoffrey Garen]

Comment on attachment 131466 [details]
Patch

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.

[Filip Pizlo]

(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 is yucky since pageSize() is not known at compile-time.

I think we should be using our own fake notion of page size (64KB?) whenever possible and just asserting at run-time that it is both larger than pageSize() and is a (power-of-two) multiple of it.

[Myles C. Maxfield]

Comment on attachment 131466 [details]
Patch

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 is yucky since pageSize() is not known at compile-time.
> 
> I think we should be using our own fake notion of page size (64KB?) whenever possible and just asserting at run-time that it is both larger than pageSize() and is a (power-of-two) multiple of it.

pageSize() already reads a constant which gets set once: if (!s_pageSize) s_pageSize = systemPageSize(); ... return s_pageSize;

Should I continue this strategy and add another size_t to PageBlock.cpp? Or should I initialize the value in initializeThreading() like you said? Moving it to initializeThreading would make pageSize() slightly faster, as it wouldn't have to check to see if the constant is already initialized. Or, should I use a constant page size like Filip Pizlo said? I think using a constant page size doesn't make as much sense as using the system page size, initializing it once, and caching it.

I'm happy to do whatever you guys decide on; I'm just getting mixed messages.

[Filip Pizlo]

(In reply to comment #9)
> (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 is yucky since pageSize() is not known at compile-time.
> > 
> > I think we should be using our own fake notion of page size (64KB?) whenever possible and just asserting at run-time that it is both larger than pageSize() and is a (power-of-two) multiple of it.
> 
> pageSize() already reads a constant which gets set once: if (!s_pageSize) s_pageSize = systemPageSize(); ... return s_pageSize;
> 
> Should I continue this strategy and add another size_t to PageBlock.cpp? Or should I initialize the value in initializeThreading() like you said? Moving it to initializeThreading would make pageSize() slightly faster, as it wouldn't have to check to see if the constant is already initialized. Or, should I use a constant page size like Filip Pizlo said? I think using a constant page size doesn't make as much sense as using the system page size, initializing it once, and caching it.

Apologies, I hadn't read your patch closely enough.  This is actually not a hot path - it's the slow case when doing oversize allocation.

I think your use of pageSize(), even without making Geoff's suggested changes, is fine.

[Filip Pizlo]

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

[Myles C. Maxfield]

Comment on attachment 131466 [details]
Patch

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 is yucky since pageSize() is not known at compile-time.
>>>> 
>>>> I think we should be using our own fake notion of page size (64KB?) whenever possible and just asserting at run-time that it is both larger than pageSize() and is a (power-of-two) multiple of it.
>>> 
>>> pageSize() already reads a constant which gets set once: if (!s_pageSize) s_pageSize = systemPageSize(); ... return s_pageSize;
>>> 
>>> Should I continue this strategy and add another size_t to PageBlock.cpp? Or should I initialize the value in initializeThreading() like you said? Moving it to initializeThreading would make pageSize() slightly faster, as it wouldn't have to check to see if the constant is already initialized. Or, should I use a constant page size like Filip Pizlo said? I think using a constant page size doesn't make as much sense as using the system page size, initializing it once, and caching it.
>>> 
>>> I'm happy to do whatever you guys decide on; I'm just getting mixed messages.
>> 
>> Apologies, I hadn't read your patch closely enough.  This is actually not a hot path - it's the slow case when doing oversize allocation.
>> 
>> I think your use of pageSize(), even without making Geoff's suggested changes, is fine.
> 
> 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?

While inlining pageMask() and pageSize(), I'm coming across some pain-points:

- initializeThreading will be doing something that is completely unrelated to threading
- initializeThreading is system-specific, but all the systems need to set WTF::s_pageSize and WTF::s_pageMask. Doing this properly (having a generic initializeThreading routine that calls the system-specific one) seems like a very major change for this CL
- inlining pageSize and pageMask means I have to move their definitions to the header (PageBlock.h). Because I'd be moving systemPageSize out of PageBlock.cpp (to live next to initializeThreading), PageBlock.cpp won't have any functions defined in it. I'd then be deleting PageBlock.cpp because it would be empty.

Does this sounds reasonable? I wanted to know if I should proceed before changing how initializeThreading is called and deleting an entire file.

[Filip Pizlo]

As I mentioned in my previous two comments, inlining WTF::pageSize is unnecessary and probably unwise. 

(In reply to comment #12)
> (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 is yucky since pageSize() is not known at compile-time.
> >>>> 
> >>>> I think we should be using our own fake notion of page size (64KB?) whenever possible and just asserting at run-time that it is both larger than pageSize() and is a (power-of-two) multiple of it.
> >>> 
> >>> pageSize() already reads a constant which gets set once: if (!s_pageSize) s_pageSize = systemPageSize(); ... return s_pageSize;
> >>> 
> >>> Should I continue this strategy and add another size_t to PageBlock.cpp? Or should I initialize the value in initializeThreading() like you said? Moving it to initializeThreading would make pageSize() slightly faster, as it wouldn't have to check to see if the constant is already initialized. Or, should I use a constant page size like Filip Pizlo said? I think using a constant page size doesn't make as much sense as using the system page size, initializing it once, and caching it.
> >>> 
> >>> I'm happy to do whatever you guys decide on; I'm just getting mixed messages.
> >> 
> >> Apologies, I hadn't read your patch closely enough.  This is actually not a hot path - it's the slow case when doing oversize allocation.
> >> 
> >> I think your use of pageSize(), even without making Geoff's suggested changes, is fine.
> > 
> > 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?
> 
> While inlining pageMask() and pageSize(), I'm coming across some pain-points:
> 
> - initializeThreading will be doing something that is completely unrelated to threading
> - initializeThreading is system-specific, but all the systems need to set WTF::s_pageSize and WTF::s_pageMask. Doing this properly (having a generic initializeThreading routine that calls the system-specific one) seems like a very major change for this CL
> - inlining pageSize and pageMask means I have to move their definitions to the header (PageBlock.h). Because I'd be moving systemPageSize out of PageBlock.cpp (to live next to initializeThreading), PageBlock.cpp won't have any functions defined in it. I'd then be deleting PageBlock.cpp because it would be empty.
> 
> Does this sounds reasonable? I wanted to know if I should proceed before changing how initializeThreading is called and deleting an entire file.

[Myles C. Maxfield]

Created attachment 131667 [details]
Patch

[Geoffrey Garen]

> This function should not be hot and even if it was, we'd have already lost.

Agreed -- as of now. As such, I'll say "r+".

If we aim to use these functions throughout the system, in place of constants, we'll eventually run into some hot code paths, and we'll need to revisit the inlining question. I'm not convinced that there's any fundamental barrier to inlining here, but I am convinced that we can cross that bridge when we come to it.

[Geoffrey Garen]

Comment on attachment 131667 [details]
Patch

r=me

[WebKit Review Bot]

Comment on attachment 131667 [details]
Patch

Clearing flags on attachment: 131667

Committed r110902: <http://trac.webkit.org/changeset/110902>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.