[chromium] Add WebDiscardableMemory to platform

• Component: New Bugs
• Status: RESOLVED FIXED

[Nat Duca]

[chromium] Add WebDiscardableMemory to platform

[Nat Duca]

Created attachment 183289 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Hin-Chung Lam]

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?

[James Robinson]

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!

[Min Qin]

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

[Min Qin]

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

[Hin-Chung Lam]

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

[Min Qin]

(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

[Hin-Chung Lam]

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

[Min Qin]

(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

[Nat Duca]

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.

[Nat Duca]

Created attachment 183306 [details]
Patch

[Hin-Chung Lam]

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.

[Hin-Chung Lam]

Comment on attachment 183306 [details]
Patch

The interface looks good to me.

[Nat Duca]

Whitespace deletion fixed.

[Nat Duca]

Ping

[James Robinson]

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.

[Min Qin]

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

[James Robinson]

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

[James Robinson]

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.

[Nat Duca]

Created attachment 184105 [details]
Patch for landing

[Nat Duca]

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.

[WebKit Review Bot]

Comment on attachment 184105 [details]
Patch for landing

Clearing flags on attachment: 184105

Committed r140499: <http://trac.webkit.org/changeset/140499>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Min Qin]

(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

[Nat Duca]

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.

[James Robinson]

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

[Avi Drissman]

Out of curiosity, how does this relate to WebCore/platform/PurgeableBuffer.h ? This wraps it?

[James Robinson]

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