Currently we may flatten the SharedBuffer segments into the internal buffer of the SharedBuffer before creating a PurgeableBuffer. As a result we may need to resize the internal buffer of the SharedBuffer to fit the contents of its segments. We can avoid this resize operation by allocating a PurgeableBuffer and copying the SharedBuffer's internal buffer and segments directly into it.
Created attachment 213708 [details] Patch
Comment on attachment 213708 [details] Patch Attachment 213708 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/3711268
Comment on attachment 213708 [details] Patch Attachment 213708 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/3758028
Comment on attachment 213708 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=213708&action=review I like the direction this patch is going, but there are some details I think you should fix. > Source/WebCore/platform/PurgeableBuffer.h:38 > + static PassOwnPtr<PurgeableBuffer> create(size_t); I think we should follow the pattern for String::createUninitialized here. Use an out argument for the pointer and use the word “uninitialized” in the name of the function. I also suggest including the makePurgeable(false) in that function instead of doing it at the call site. > Source/WebCore/platform/PurgeableBuffer.h:44 > + char* data(); I think it would be better to pass the non-const char* out of the create function rather than always exposing a non-const data pointer after the fact on all purgeable buffers. So please change this back. > Source/WebCore/platform/SharedBuffer.cpp:383 > +void SharedBuffer::copySegmentsOrDataArrayAndClear(char* destination, unsigned bytesToCopy) const Is there any caller of copyDataArrayAndClear that does not want this behavior? Why do we need a separate function for this?
(In reply to comment #4) > [...] > > Source/WebCore/platform/PurgeableBuffer.h:38 > > + static PassOwnPtr<PurgeableBuffer> create(size_t); > > I think we should follow the pattern for String::createUninitialized here. Use an out argument for the pointer and use the word “uninitialized” in the name of the function. I like this idiom. Will modify patch. > I also suggest including the makePurgeable(false) in that function instead of doing it at the call site. By adopting the createUninitialized idiom, it seems sufficient to omit this check as we create the PurgeableBuffer in a non-volatile state by default. For some reason, I thought that adding this check at the call site would make the usage of PurgeableBuffer::create(size_t) and the code that writes directly into its buffer less error prone or at least consistent consistent with the document usage for PurgeableBuffer::data(). The new name PurgeableBuffer::createUninitialized(...) seems to obviate the need for such a check since the name better conveys to the caller the state of the PurgeableBuffer. That is, it is uninitialized and hence there is an assumption that it's non-volatile. Do you see a benefit in explicitly checking/asserting makePurgeable(false) is true in PurgeableBuffer::createUninitialized()? > > > Source/WebCore/platform/PurgeableBuffer.h:44 > > + char* data(); > > I think it would be better to pass the non-const char* out of the create function rather than always exposing a non-const data pointer after the fact on all purgeable buffers. So please change this back. > Will revert this change. > > Source/WebCore/platform/SharedBuffer.cpp:383 > > +void SharedBuffer::copySegmentsOrDataArrayAndClear(char* destination, unsigned bytesToCopy) const > > Is there any caller of copyDataArrayAndClear that does not want this behavior? Why do we need a separate function for this? Will remove function copySegmentsOrDataArrayAndClear. Instead, will move the !USE(NETWORK_CFDATA_ARRAY_CALLBACK)-guarded code in copySegmentsOrDataArrayAndClear() into a !USE(NETWORK_CFDATA_ARRAY_CALLBACK)-guarded version of copyDataArrayAndClear() such that SharedBuffer.h will declare a platform-independent function called copyDataArrayAndClear, SharedBuffer.cpp will provide the platform-independent implementation of this function and SharedBufferCF.cp will provide a CF-specific implementation of this function.
Created attachment 213725 [details] Patch
Comment on attachment 213725 [details] Patch Attachment 213725 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/3709348
Comment on attachment 213725 [details] Patch Attachment 213725 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/3714305
Created attachment 213727 [details] Patch
Created attachment 213728 [details] Patch
Comment on attachment 213728 [details] Patch Attachment 213728 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/3758068
Comment on attachment 213728 [details] Patch Attachment 213728 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/3760083
Comment on attachment 213728 [details] Patch Attachment 213728 [details] did not pass gtk-ews (gtk): Output: http://webkit-queues.appspot.com/results/3752161
Created attachment 213729 [details] Patch
Comment on attachment 213729 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=213729&action=review > Source/WebCore/platform/SharedBuffer.cpp:233 > + char* destination = nullptr; I don’t think this needs to be initialized. > Source/WebCore/platform/mac/PurgeableBufferMac.cpp:86 > + char* buffer = allocatePurgeableBuffer(size); > + if (!buffer) > + return nullptr; > + memcpy(buffer, data, size); > + return adoptPtr(new PurgeableBuffer(buffer, size)); Seems like this could just call createUninitialized and then do the memcpy. No need for the allocatePurgeableBuffer function at all.
(In reply to comment #15) > (From update of attachment 213729 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=213729&action=review > > > Source/WebCore/platform/SharedBuffer.cpp:233 > > + char* destination = nullptr; > > I don’t think this needs to be initialized. Will fix. > > > Source/WebCore/platform/mac/PurgeableBufferMac.cpp:86 > > + char* buffer = allocatePurgeableBuffer(size); > > + if (!buffer) > > + return nullptr; > > + memcpy(buffer, data, size); > > + return adoptPtr(new PurgeableBuffer(buffer, size)); > > Seems like this could just call createUninitialized and then do the memcpy. No need for the allocatePurgeableBuffer function at all. Will remove function allocatePurgeableBuffer() and implement PurgeableBuffer::create() in terms of PurgeableBuffer::createUninitialized().
Committed r157235: <http://trac.webkit.org/changeset/157235>