| Summary: | CopiedSpace::tryAllocateOversize assumes system page size | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Myles C. Maxfield <litherum> | ||||||||
| Component: | JavaScriptCore | Assignee: | Nobody <webkit-unassigned> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | fpizlo, ggaren, mhahnenberg, webkit.review.bot | ||||||||
| Priority: | P2 | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Myles C. Maxfield
2012-03-08 11:13:56 PST
I'd like to claim this bug. Created attachment 131073 [details]
Patch
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. 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. 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. Created attachment 131466 [details]
Patch
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. (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. 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. (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. (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. 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. 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. Created attachment 131667 [details]
Patch
> 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.
Comment on attachment 131667 [details]
Patch
r=me
Comment on attachment 131667 [details] Patch Clearing flags on attachment: 131667 Committed r110902: <http://trac.webkit.org/changeset/110902> All reviewed patches have been landed. Closing bug. |