[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 #13 from Filip Pizlo <fpizlo at apple.com>  2012-03-12 19:37:08 PST ---
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.

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