[chromium] Add WebDiscardableMemory to platform
Created attachment 183289 [details] Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment on attachment 183289 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=183289&action=review > Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:45 > + dst->lockPixels(); allocateDisablecardMemory() and lockPixels() are separated. Is it possible that right after allocation the memory is discarded? Should allocateAndLock be one operation?
Comment on attachment 183289 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=183289&action=review I think we will benefit from massaging the WebDiscardableMemory API a bit. Now that I've had a bit of time to think about this API it'll mean some extra kernel trips on every allocation, which probably isn't great. What's the minimum practical allocation size for ashmem? Probably at least a page, right? Do we want to avoid it for small allocations? Do we have any idea of what the allocation size distribution looks like for this path? Sounds like a job for histograms to me. > Source/Platform/chromium/public/Platform.h:237 > + // Allocate discardable memory for caching decoded images. the "caching decoding images" part isn't really part of the API, it's one potential use case. Better would be describing what this call does (attempt to allocate blah blah blah). also document the initial state of the WebDiscardableMemory chunk - is it pinned, or unpinned? > Source/Platform/chromium/public/Platform.h:240 > // Message Ports ------------------------------------------------------- keep 2 blank lines between sections > Source/Platform/chromium/public/WebDiscardableMemory.h:8 > + * * Redistributions of source code must retain the above copyright 2-clause for new files (like you have in the .cpp) > Source/Platform/chromium/public/WebDiscardableMemory.h:36 > +// A memory object that can be automatically purge by system when it is typo "purge" -> "purged" include some sample code here of how to lock, use, and unlock memory correctly > Source/Platform/chromium/public/WebDiscardableMemory.h:42 > + // Lock the memory so it will not be released. Returns 0 on failure. can you expand on what "failure" means? i think with pretty much every underlying implementation evictable memory will be initially resident and then later marked as evictable. this doesn't match up all that well with this API since the only way to know where block is is to call lock(), which means we're going to force every caller to do an extra unpin/pin on initial allocation :(. also lock/unlock are a little bit too close to synchronization, which isn't really what this is about. maybe we could rejigger it to have a void* getter, bool pin() and unpin()? > Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:39 > + DiscardablePixelRef* pixelRef = new DiscardablePixelRef(ctable, adoptPtr(new SkMutex())); do we really need atomic inc/dec of the refcount for this object? if not, instead of allocating a mutex per object can we just call setPreLocked()? also can we patch skia to use atomic inc/dec instead of a Mutex for atomic refcounting? seriously!
(In reply to comment #3) > (From update of attachment 183289 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=183289&action=review > > > Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:45 > > + dst->lockPixels(); > > allocateDisablecardMemory() and lockPixels() are separated. Is it possible that right after allocation the memory is discarded? Should allocateAndLock be one operation? allocateDiscardableMemory will guarantee that the memory is pinned if successful, so the memory will not be discarded.
(In reply to comment #4) > (From update of attachment 183289 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=183289&action=review > > I think we will benefit from massaging the WebDiscardableMemory API a bit. Now that I've had a bit of time to think about this API it'll mean some extra kernel trips on every allocation, which probably isn't great. > > What's the minimum practical allocation size for ashmem? Probably at least a page, right? Do we want to avoid it for small allocations? Do we have any idea of what the allocation size distribution looks like for this path? Sounds like a job for histograms to me. > > > Source/Platform/chromium/public/Platform.h:237 > > + // Allocate discardable memory for caching decoded images. > > the "caching decoding images" part isn't really part of the API, it's one potential use case. Better would be describing what this call does (attempt to allocate blah blah blah). > > also document the initial state of the WebDiscardableMemory chunk - is it pinned, or unpinned? > > > Source/Platform/chromium/public/Platform.h:240 > > // Message Ports ------------------------------------------------------- > > keep 2 blank lines between sections > > > Source/Platform/chromium/public/WebDiscardableMemory.h:8 > > + * * Redistributions of source code must retain the above copyright > > 2-clause for new files (like you have in the .cpp) > > > Source/Platform/chromium/public/WebDiscardableMemory.h:36 > > +// A memory object that can be automatically purge by system when it is > > typo "purge" -> "purged" > > include some sample code here of how to lock, use, and unlock memory correctly > > > Source/Platform/chromium/public/WebDiscardableMemory.h:42 > > + // Lock the memory so it will not be released. Returns 0 on failure. > > can you expand on what "failure" means? > > i think with pretty much every underlying implementation evictable memory will be initially resident and then later marked as evictable. this doesn't match up all that well with this API since the only way to know where block is is to call lock(), which means we're going to force every caller to do an extra unpin/pin on initial allocation :(. also lock/unlock are a little bit too close to synchronization, which isn't really what this is about. maybe we could rejigger it to have a void* getter, bool pin() and unpin()? The initial allocation will pin the memory, so there is no need to call pin after it. > > > Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:39 > > + DiscardablePixelRef* pixelRef = new DiscardablePixelRef(ctable, adoptPtr(new SkMutex())); > > do we really need atomic inc/dec of the refcount for this object? if not, instead of allocating a mutex per object can we just call setPreLocked()? We cannot use setPrelocked（）. SetPrelocked() will ignire all the onLockedPixels() and onUnlockPixels(). > > also can we patch skia to use atomic inc/dec instead of a Mutex for atomic refcounting? seriously!
(In reply to comment #5) > (In reply to comment #3) > > (From update of attachment 183289 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=183289&action=review > > > > > Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:45 > > > + dst->lockPixels(); > > > > allocateDisablecardMemory() and lockPixels() are separated. Is it possible that right after allocation the memory is discarded? Should allocateAndLock be one operation? > > allocateDiscardableMemory will guarantee that the memory is pinned if successful, so the memory will not be discarded. How do you unpin it then? There must be a balancing pin and unpin.
(In reply to comment #7) > (In reply to comment #5) > > (In reply to comment #3) > > > (From update of attachment 183289 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=183289&action=review > > > > > > > Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:45 > > > > + dst->lockPixels(); > > > > > > allocateDisablecardMemory() and lockPixels() are separated. Is it possible that right after allocation the memory is discarded? Should allocateAndLock be one operation? > > > > allocateDiscardableMemory will guarantee that the memory is pinned if successful, so the memory will not be discarded. > > How do you unpin it then? There must be a balancing pin and unpin. When u delete DiscardableMemory, it will be unpinned. Or otherwise, call Unpin（） specifically. Calling Pin() when memory is pinned is a no-op
(In reply to comment #8) > (In reply to comment #7) > > (In reply to comment #5) > > > (In reply to comment #3) > > > > (From update of attachment 183289 [details] [details] [details] [details]) > > > > View in context: https://bugs.webkit.org/attachment.cgi?id=183289&action=review > > > > > > > > > Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:45 > > > > > + dst->lockPixels(); > > > > > > > > allocateDisablecardMemory() and lockPixels() are separated. Is it possible that right after allocation the memory is discarded? Should allocateAndLock be one operation? > > > > > > allocateDiscardableMemory will guarantee that the memory is pinned if successful, so the memory will not be discarded. > > > > How do you unpin it then? There must be a balancing pin and unpin. > > When u delete DiscardableMemory, it will be unpinned. Or otherwise, call Unpin（） specifically. Calling Pin() when memory is pinned is a no-op If you call unpin only when you delete DiscardableMemory then how does it different than regular memory? I thought the idea is to pin it only when we're using the bitmap for drawing / decoding.
(In reply to comment #9) > (In reply to comment #8) > > (In reply to comment #7) > > > (In reply to comment #5) > > > > (In reply to comment #3) > > > > > (From update of attachment 183289 [details] [details] [details] [details] [details]) > > > > > View in context: https://bugs.webkit.org/attachment.cgi?id=183289&action=review > > > > > > > > > > > Source/WebCore/platform/graphics/chromium/DiscardablePixelRef.cpp:45 > > > > > > + dst->lockPixels(); > > > > > > > > > > allocateDisablecardMemory() and lockPixels() are separated. Is it possible that right after allocation the memory is discarded? Should allocateAndLock be one operation? > > > > > > > > allocateDiscardableMemory will guarantee that the memory is pinned if successful, so the memory will not be discarded. > > > > > > How do you unpin it then? There must be a balancing pin and unpin. > > > > When u delete DiscardableMemory, it will be unpinned. Or otherwise, call Unpin（） specifically. Calling Pin() when memory is pinned is a no-op > > If you call unpin only when you delete DiscardableMemory then how does it different than regular memory? I thought the idea is to pin it only when we're using the bitmap for drawing / decoding. When we call skpixelRef::OnLockPixels(), it will pin the DiscardableMemory, when OnUnlockPixels() gets called, it will unpin the memory. After the intial allocation, the first pin is a noop. So the first unpin will unpin the memory
I'm sorry, the WebCore changes were included in error. I want to first get on the same page about WebDiscardable. New changes uploaded to try to accomodate everyone's concerns.
Created attachment 183306 [details] Patch
Comment on attachment 183306 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=183306&action=review > Source/Platform/chromium/public/Platform.h:-160 > - nit: Don't remove empty lines.
Comment on attachment 183306 [details] Patch The interface looks good to me.
Whitespace deletion fixed.
Ping
Comment on attachment 183306 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=183306&action=review I think a few of these APIs aren't needed, but Platform::allocateAndLock...() and Platform/..../WebDiscardableMemory.h look good except for some typos and formatting issues. > Source/Platform/chromium/public/Platform.h:238 > + // Determine if the underlying platform supports discardable memory > + virtual bool discardableMemorySupported() const { return false; } I don't think this adds much value. If the platform doesn't support discardable memory, allocateAndLock...() will return 0 and I can't think of a case where the caller would want to distinguish between the platform not supporting discardable memory and the specific call failing. > Source/Platform/chromium/public/Platform.h:244 > + // Discardable memory is often based on an underlying kernel paging system. > + // Thus, allocations below a certain size are often counterproductive. This > + // returns the suggested allocation size in bytes for discardable memory > + // allocations. > + virtual size_t suggestedMinimumDiscardableMemoryAllocationSizeInBytes() const { return 0; } I'm not sure how this would be implemented in a way that's intelligent, since the implementation of this function has no idea what the caller wants to do. If we want to set a hard floor the implementation can just return 0 from allocateAndLock...() for certain sizes. > Source/Platform/chromium/public/WebDiscardableMemory.h:37 > + // Locks the memory, prevent it from being discarded. Once locked. you may i think conventionally these sort of block comments are placed before the class declaration > Source/Platform/chromium/public/WebDiscardableMemory.h:40 > + // Lock() may return false, indicating that the underlying memory was s/Lock()/lock()/ > Source/Platform/chromium/public/WebDiscardableMemory.h:53 > + // delete mem; // Make sure to destroy it. Its never going to come back. s/Its/It's/ > Source/Platform/chromium/public/WebDiscardableMemory.h:67 > + // Unlock the memory so that it can be purged by the system. Must be called > + // after every successful lock call. is deleting the WebDiscardableMemory while locked valid? If so, we should document as such somewhere.
(In reply to comment #17) > (From update of attachment 183306 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=183306&action=review > > I think a few of these APIs aren't needed, but Platform::allocateAndLock...() and Platform/..../WebDiscardableMemory.h look good except for some typos and formatting issues. > > > Source/Platform/chromium/public/Platform.h:238 > > + // Determine if the underlying platform supports discardable memory > > + virtual bool discardableMemorySupported() const { return false; } > > I don't think this adds much value. If the platform doesn't support discardable memory, allocateAndLock...() will return 0 and I can't think of a case where the caller would want to distinguish between the platform not supporting discardable memory and the specific call failing. Do you mean we need to call allocate twice each time we allocate memory for an image? First call to allocate DiscardableMemory will fail, and the 2nd time we will fallback to SkMallocPixels. > > > Source/Platform/chromium/public/Platform.h:244 > > + // Discardable memory is often based on an underlying kernel paging system. > > + // Thus, allocations below a certain size are often counterproductive. This > > + // returns the suggested allocation size in bytes for discardable memory > > + // allocations. > > + virtual size_t suggestedMinimumDiscardableMemoryAllocationSizeInBytes() const { return 0; } > > I'm not sure how this would be implemented in a way that's intelligent, since the implementation of this function has no idea what the caller wants to do. If we want to set a hard floor the implementation can just return 0 from allocateAndLock...() for certain sizes. > > > Source/Platform/chromium/public/WebDiscardableMemory.h:37 > > + // Locks the memory, prevent it from being discarded. Once locked. you may > > i think conventionally these sort of block comments are placed before the class declaration > > > Source/Platform/chromium/public/WebDiscardableMemory.h:40 > > + // Lock() may return false, indicating that the underlying memory was > > s/Lock()/lock()/ > > > Source/Platform/chromium/public/WebDiscardableMemory.h:53 > > + // delete mem; // Make sure to destroy it. Its never going to come back. > > s/Its/It's/ > > > Source/Platform/chromium/public/WebDiscardableMemory.h:67 > > + // Unlock the memory so that it can be purged by the system. Must be called > > + // after every successful lock call. > > is deleting the WebDiscardableMemory while locked valid? If so, we should document as such somewhere.
(In reply to comment #18) > (In reply to comment #17) > > (From update of attachment 183306 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=183306&action=review > > > > I think a few of these APIs aren't needed, but Platform::allocateAndLock...() and Platform/..../WebDiscardableMemory.h look good except for some typos and formatting issues. > > > > > Source/Platform/chromium/public/Platform.h:238 > > > + // Determine if the underlying platform supports discardable memory > > > + virtual bool discardableMemorySupported() const { return false; } > > > > I don't think this adds much value. If the platform doesn't support discardable memory, allocateAndLock...() will return 0 and I can't think of a case where the caller would want to distinguish between the platform not supporting discardable memory and the specific call failing. > > Do you mean we need to call allocate twice each time we allocate memory for an image? First call to allocate DiscardableMemory will fail, and the 2nd time we will fallback to SkMallocPixels. What's the alternative? I would think it'd be something like: if (Platform::current()->discardableMemorySupported()) { Platform::current()->allocateAndLock...(); } else { malloc(...) } which is the same number of virtual calls as if (WebDiscardableMemory* memory = Platform::current()->allocateAndLock...()) { // use memory } else { malloc(...) }
Even if you could check for platform support without the virtual call I think you'd still need the fallback path for cases where the ashmem call fails but we aren't actually OOM.
Created attachment 184105 [details] Patch for landing
Min, to use this, stop thinking of having a mode for discadable memory. Always use discardable memory, and if the alloc fails, then create an skbitmap with a regular malloc. E.g.: WebDiscardable* disc = Platform::current()->allocateAndLock(...); if(!disc) { bitmap->setPixelelRef(pixelRef that wraps and owns void*) } else { bitmap->setPixelelRef(pixelRef that wraps webdiscardable) } And, update the memory managment policy to just track bytes allocated against each type. E.g. bytes_of_discardable , bytes_of_non_discardable.
Comment on attachment 184105 [details] Patch for landing Clearing flags on attachment: 184105 Committed r140499: <http://trac.webkit.org/changeset/140499>
All reviewed patches have been landed. Closing bug.
(In reply to comment #20) > Even if you could check for platform support without the virtual call I think you'd still need the fallback path for cases where the ashmem call fails but we aren't actually OOM. One problem is that if there are no DiscardableMemory on the platform, the image cache will use 32MB physical memory. However, with discardableMemory, the cache size limit should be gone, but we should have a file descriptor limit. Removing this flag will make us unaware which rule we should use when doing the cache pruning
I still think we can do it. The alloc discardable should fail, or we should have some reaosonable limit on discardable-backed cache entries. We should then either back with malloc memory, or evict old discardables when we're at the limit.
(In reply to comment #25) > (In reply to comment #20) > > Even if you could check for platform support without the virtual call I think you'd still need the fallback path for cases where the ashmem call fails but we aren't actually OOM. > > > One problem is that if there are no DiscardableMemory on the platform, the image cache will use 32MB physical memory. > However, with discardableMemory, the cache size limit should be gone, but we should have a file descriptor limit. > Removing this flag will make us unaware which rule we should use when doing the cache pruning Just keep track of how many active discardable memory instances you have and limit that.
Out of curiosity, how does this relate to WebCore/platform/PurgeableBuffer.h ? This wraps it?
(In reply to comment #28) > Out of curiosity, how does this relate to WebCore/platform/PurgeableBuffer.h ? This wraps it? No, this wraps base::DiscardableMemory. WebCore/platform/PurgeableBuffer appears to only be implemented on mac. In chromium, we could implement WebCore/platform/PurgeableBuffer on top of this interface if that made sense. I'm not sure how things like PurgePriority would map in that context.