Bug 122516 – Avoid resizing the internal buffer of SharedBuffer when creating a PurgeableBuffer

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Daniel Bates]

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.

[Daniel Bates]

Created attachment 213708 [details]
Patch

[EFL EWS Bot]

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

[EFL EWS Bot]

Comment on attachment 213708 [details]
Patch

Attachment 213708 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/3758028

[Darin Adler]

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?

[Daniel Bates]

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

[Daniel Bates]

Created attachment 213725 [details]
Patch

[EFL EWS Bot]

Comment on attachment 213725 [details]
Patch

Attachment 213725 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/3709348

[EFL EWS Bot]

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

[Daniel Bates]

Created attachment 213727 [details]
Patch

[Daniel Bates]

Created attachment 213728 [details]
Patch

[EFL EWS Bot]

Comment on attachment 213728 [details]
Patch

Attachment 213728 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/3758068

[EFL EWS Bot]

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

[kov's GTK+ EWS bot]

Comment on attachment 213728 [details]
Patch

Attachment 213728 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/3752161

[Daniel Bates]

Created attachment 213729 [details]
Patch

[Darin Adler]

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.

[Daniel Bates]

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

[Daniel Bates]

Committed r157235: <http://trac.webkit.org/changeset/157235>