[webkit-reviews] review denied: [Bug 107183] [chromium] Add WebDiscardableMemory to platform : [Attachment 183289] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Nat Duca <nduca at chromium.org>'s
request for review:
Bug 107183: [chromium] Add WebDiscardableMemory to platform
https://bugs.webkit.org/show_bug.cgi?id=107183

Attachment 183289: Patch
https://bugs.webkit.org/attachment.cgi?id=183289&action=review

------- Additional Comments from James Robinson <jamesr at chromium.org>
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!