Bug 170956 - Reduce copies and allocations in SharedBuffer::append
Summary: Reduce copies and allocations in SharedBuffer::append
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-04-18 11:08 PDT by Alex Christensen
Modified: 2017-04-21 19:26 PDT (History)
7 users (show)

See Also:


Attachments
Patch (75.98 KB, patch)
2017-04-18 11:38 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-elcapitan (1.68 MB, application/zip)
2017-04-18 13:02 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews114 for mac-elcapitan (2.44 MB, application/zip)
2017-04-18 13:30 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-elcapitan-wk2 (1.90 MB, application/zip)
2017-04-18 13:30 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews124 for ios-simulator-wk2 (59.95 MB, application/zip)
2017-04-18 14:06 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews126 for ios-simulator-wk2 (71.17 MB, application/zip)
2017-04-18 16:07 PDT, Build Bot
no flags Details
Patch (84.34 KB, patch)
2017-04-20 09:57 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-elcapitan (960.42 KB, application/zip)
2017-04-20 11:12 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-elcapitan-wk2 (1.05 MB, application/zip)
2017-04-20 11:13 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews116 for mac-elcapitan (1.72 MB, application/zip)
2017-04-20 11:29 PDT, Build Bot
no flags Details
Patch (85.72 KB, patch)
2017-04-20 17:20 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-elcapitan (1.02 MB, application/zip)
2017-04-20 18:37 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews117 for mac-elcapitan (1.70 MB, application/zip)
2017-04-20 19:29 PDT, Build Bot
no flags Details
Patch (87.14 KB, patch)
2017-04-21 00:31 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2017-04-18 11:08:46 PDT
Reduce copies and allocations in SharedBuffer::append
Comment 1 Alex Christensen 2017-04-18 11:38:48 PDT
Created attachment 307398 [details]
Patch
Comment 2 Build Bot 2017-04-18 13:02:18 PDT
Comment on attachment 307398 [details]
Patch

Attachment 307398 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3558616

New failing tests:
http/tests/appcache/online-fallback-layering.html
http/tests/multipart/multipart-image.html
http/tests/appcache/offline-access.html
http/tests/appcache/simple.html
http/tests/media/track/track-webvtt-slow-loading-2.html
http/tests/multipart/invalid-image-data.html
http/tests/media/track/track-webvtt-slow-loading.html
http/tests/appcache/multi-fallback.html
fast/events/standalone-image-drag-to-editable.html
http/tests/appcache/foreign-fallback.html
http/tests/appcache/non-html.xhtml
editing/pasteboard/paste-RTFD.html
http/tests/multipart/invalid-image-data-standalone.html
webarchive/loading/test-loading-archive-subresource-null-mimetype.html
editing/pasteboard/copy-standalone-image.html
http/tests/appcache/fallback.html
webarchive/loading/cache-expired-subresource.html
http/tests/appcache/main-resource-fallback-for-network-error-crash.html
http/tests/multipart/stop-crash.html
Comment 3 Build Bot 2017-04-18 13:02:19 PDT
Created attachment 307404 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 4 Build Bot 2017-04-18 13:30:29 PDT
Comment on attachment 307398 [details]
Patch

Attachment 307398 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3558696

New failing tests:
http/tests/appcache/online-fallback-layering.html
http/tests/multipart/multipart-image.html
http/tests/appcache/offline-access.html
http/tests/appcache/simple.html
http/tests/media/track/track-webvtt-slow-loading-2.html
http/tests/multipart/invalid-image-data.html
http/tests/media/track/track-webvtt-slow-loading.html
http/tests/appcache/multi-fallback.html
fast/events/standalone-image-drag-to-editable.html
http/tests/appcache/foreign-fallback.html
http/tests/appcache/non-html.xhtml
http/tests/appcache/manifest-parsing.html
editing/pasteboard/paste-RTFD.html
http/tests/multipart/invalid-image-data-standalone.html
webarchive/loading/test-loading-archive-subresource-null-mimetype.html
editing/pasteboard/copy-standalone-image.html
http/tests/appcache/fallback.html
webarchive/loading/cache-expired-subresource.html
http/tests/appcache/main-resource-fallback-for-network-error-crash.html
http/tests/multipart/stop-crash.html
Comment 5 Build Bot 2017-04-18 13:30:31 PDT
Created attachment 307410 [details]
Archive of layout-test-results from ews114 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 6 Build Bot 2017-04-18 13:30:41 PDT
Comment on attachment 307398 [details]
Patch

Attachment 307398 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3558702

New failing tests:
http/tests/appcache/online-fallback-layering.html
http/tests/multipart/multipart-image.html
http/tests/appcache/offline-access.html
http/tests/appcache/simple.html
http/tests/media/track/track-webvtt-slow-loading-2.html
http/tests/multipart/invalid-image-data.html
http/tests/appcache/multi-fallback.html
http/tests/appcache/fallback.html
http/tests/appcache/foreign-fallback.html
http/tests/appcache/non-html.xhtml
http/tests/multipart/invalid-image-data-standalone.html
webarchive/loading/test-loading-archive-subresource-null-mimetype.html
editing/pasteboard/copy-standalone-image.html
http/tests/media/track/track-webvtt-slow-loading.html
webarchive/loading/cache-expired-subresource.html
http/tests/appcache/main-resource-fallback-for-network-error-crash.html
http/tests/multipart/stop-crash.html
Comment 7 Build Bot 2017-04-18 13:30:42 PDT
Created attachment 307411 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 8 Build Bot 2017-04-18 14:06:17 PDT
Comment on attachment 307398 [details]
Patch

Attachment 307398 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3558720

New failing tests:
http/tests/appcache/online-fallback-layering.html
http/tests/multipart/multipart-image.html
http/tests/appcache/offline-access.html
http/tests/appcache/simple.html
http/tests/multipart/invalid-image-data.html
http/tests/appcache/multi-fallback.html
http/tests/appcache/foreign-fallback.html
http/tests/appcache/non-html.xhtml
http/tests/multipart/invalid-image-data-standalone.html
webarchive/loading/test-loading-archive-subresource-null-mimetype.html
http/tests/appcache/fallback.html
webarchive/loading/cache-expired-subresource.html
http/tests/appcache/main-resource-fallback-for-network-error-crash.html
http/tests/multipart/stop-crash.html
Comment 9 Build Bot 2017-04-18 14:06:20 PDT
Created attachment 307416 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6
Comment 10 Build Bot 2017-04-18 16:07:28 PDT
Comment on attachment 307398 [details]
Patch

Attachment 307398 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3559241

New failing tests:
http/tests/appcache/online-fallback-layering.html
http/tests/multipart/multipart-image.html
http/tests/appcache/offline-access.html
http/tests/appcache/simple.html
http/tests/multipart/invalid-image-data.html
http/tests/appcache/multi-fallback.html
http/tests/appcache/foreign-fallback.html
http/tests/appcache/non-html.xhtml
http/tests/multipart/invalid-image-data-standalone.html
webarchive/loading/test-loading-archive-subresource-null-mimetype.html
http/tests/appcache/fallback.html
webarchive/loading/cache-expired-subresource.html
http/tests/appcache/main-resource-fallback-for-network-error-crash.html
http/tests/multipart/stop-crash.html
Comment 11 Build Bot 2017-04-18 16:07:31 PDT
Created attachment 307431 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6
Comment 12 Alex Christensen 2017-04-20 09:57:57 PDT
Created attachment 307596 [details]
Patch
Comment 13 Alex Christensen 2017-04-20 10:07:31 PDT
Comment on attachment 307596 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=307596&action=review

> Source/WebCore/platform/SharedBufferChunkReader.cpp:38
> +// It's only used on Linux, though, so I can continue my prototyping without making this change on Mac.

I plan to remove this line of the comment.  My prototyping is done.
Comment 14 Andreas Kling 2017-04-20 10:21:59 PDT
Comment on attachment 307596 [details]
Patch

r=me

We currently need the data() buffer flattening support for image decoders on mac and iOS, so we won't be able to get rid of that unless we get some new framework support.

Could we also do something with the Content-Length HTTP header if present? Pre-allocating the final buffer size right away might reduce allocations even further.
Comment 15 Alex Christensen 2017-04-20 10:47:06 PDT
(In reply to Andreas Kling from comment #14)
> Comment on attachment 307596 [details]
> Patch
> 
> r=me
> 
> We currently need the data() buffer flattening support for image decoders on
> mac and iOS, so we won't be able to get rid of that unless we get some new
> framework support.
Yep, we still need data() for a while, but I looked and many but not all of the calls to it could immediately be changed to iterating segments.
> 
> Could we also do something with the Content-Length HTTP header if present?
> Pre-allocating the final buffer size right away might reduce allocations
> even further.
CFNetwork would have to do that, and then we would lose incremental loading.
Comment 16 Build Bot 2017-04-20 11:12:16 PDT
Comment on attachment 307596 [details]
Patch

Attachment 307596 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3570816

New failing tests:
http/tests/media/track/track-webvtt-slow-loading-2.html
webarchive/archive-empty-frame-source.html
http/tests/media/track/track-webvtt-slow-loading.html
Comment 17 Build Bot 2017-04-20 11:12:17 PDT
Created attachment 307600 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 18 Build Bot 2017-04-20 11:13:07 PDT
Comment on attachment 307596 [details]
Patch

Attachment 307596 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3570814

New failing tests:
http/tests/media/track/track-webvtt-slow-loading-2.html
http/tests/media/track/track-webvtt-slow-loading.html
Comment 19 Build Bot 2017-04-20 11:13:08 PDT
Created attachment 307601 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 20 Build Bot 2017-04-20 11:29:06 PDT
Comment on attachment 307596 [details]
Patch

Attachment 307596 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3570831

New failing tests:
http/tests/media/track/track-webvtt-slow-loading-2.html
webarchive/archive-empty-frame-source.html
http/tests/media/track/track-webvtt-slow-loading.html
Comment 21 Build Bot 2017-04-20 11:29:08 PDT
Created attachment 307602 [details]
Archive of layout-test-results from ews116 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 22 Alex Christensen 2017-04-20 17:20:11 PDT
Created attachment 307666 [details]
Patch
Comment 23 Build Bot 2017-04-20 18:37:41 PDT
Comment on attachment 307666 [details]
Patch

Attachment 307666 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3573305

New failing tests:
webarchive/archive-empty-frame-source.html
Comment 24 Build Bot 2017-04-20 18:37:42 PDT
Created attachment 307673 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 25 Build Bot 2017-04-20 19:29:55 PDT
Comment on attachment 307666 [details]
Patch

Attachment 307666 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3573454

New failing tests:
webarchive/archive-empty-frame-source.html
Comment 26 Build Bot 2017-04-20 19:29:57 PDT
Created attachment 307684 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 27 Geoffrey Garen 2017-04-20 20:10:51 PDT
Comment on attachment 307666 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=307666&action=review

> Source/WebCore/loader/TextTrackLoader.cpp:102
> +    auto bytesToSkip = m_parseOffset;
> +    for (const auto& segment : *buffer) {
> +        if (bytesToSkip > segment->size()) {
> +            bytesToSkip -= segment->size();
> +            continue;
> +        }
> +        m_cueParser->parseBytes(segment->data() + bytesToSkip, segment->size() - bytesToSkip);
> +        bytesToSkip = 0;
> +        m_parseOffset += segment->size();

The ability to read data from a buffer starting at an offset seems like a generally useful utility, and not something each client of SharedBuffer should need to reinvent.

I'd suggest providing an inline function inside SharedBuffer.h that accepts a SharedBuffer*, an offset, and a lambda, and invokes the lambda with a char* and a size until it runs out of data.

Also note that this code may invoke parseBytes passing it zero bytes when bytesToSkip == segment->size(). It might be clearer to write the skipping as a separate loop from the copying.

> Source/WebCore/platform/SharedBuffer.cpp:82
> +    // This copies data and is bad for performance.

It's not clear what you're trying to convey here. Copying data is not always a measurable performance problem. By design, all vectors make amortized copies. Maybe you should add a comment to the header explaining when you should or should not call this function.

> Source/WebCore/platform/SharedBuffer.cpp:100
> +    if (m_segments.size() == 1)
> +        return m_segments[0]->data();

combineToOneSegment() contains this check so you can eliminate it.

> Source/WebCore/platform/SharedBuffer.cpp:115
> +        memcpy(reinterpret_cast<char*>(arrayBuffer->data()) + position, segment->data(), segment->size());

This should be static_cast. Technically, reinterpret_cast<char*> is a violation of strict aliasing rules, so it is a bit of a red flag. But static_cast will work just fine.

Note that this function copies data but you haven't called it out as being bad for performance. I wonder what the distinction is?

> Source/WebCore/platform/SharedBuffer.cpp:135
> +    Vector<char> vector(length);
> +    memcpy(vector.data(), data, length);

It's a little more idiomatic to make an empty vector and then call append(data, length). Also, you avoid filling the vector twice.

> Source/WebCore/platform/SharedBufferChunkReader.cpp:112
> -        m_segmentLength = m_buffer->getSomeData(m_segment, m_bufferPosition);
> +        // Let's pretend all the data is in one block.
> +        // It might be less efficient in some exotic cases, but does anyone really care?
> +        // This class should be removed anyways.
> +        m_segment = m_buffer->data() + m_bufferPosition;
> +        m_segmentLength = m_buffer->size() - m_bufferPosition;

I don't know if I care. Is this class still used right now? If so, I probably care. Using the shared helper function I suggested above would avoid copying here.

> Source/WebCore/platform/cocoa/SharedBufferCocoa.mm:99
> +    if (m_segments.size()) {

This would read better as an early return of nullptr if !m_segments.size().
Comment 28 Geoffrey Garen 2017-04-20 20:13:29 PDT
Comment on attachment 307666 [details]
Patch

Under what conditions does this patch remove previous copying? I think I found two places where it introduces copies, but I'm not sure where it removes them.
Comment 29 Alex Christensen 2017-04-20 22:53:28 PDT
Thanks for the additional review.  I'll improve my patch based on your suggestions.  Here are my replies.

(In reply to Geoffrey Garen from comment #27)
> The ability to read data from a buffer starting at an offset seems like a
> generally useful utility, and not something each client of SharedBuffer
> should need to reinvent.
I disagree.  I think it's what we've used because it's what we had.  We should continue to rewrite things to adopt this model instead of the getSomeData model, which required designing other code around the ability to get an unknown number of bytes starting at a given offset rather than iterating segments, which makes more sense and requires less overhead from how we get data from the network layer.
> > Source/WebCore/platform/SharedBuffer.cpp:82
> > +    // This copies data and is bad for performance.
> 
> It's not clear what you're trying to convey here. Copying data is not always
> a measurable performance problem. By design, all vectors make amortized
> copies. Maybe you should add a comment to the header explaining when you
> should or should not call this function.
I think in an ideal world we would not need to take segmented buffers, which is how we receive data from the network layer, and unnecessarily copy them into one buffer just to have them in sequential memory.  We would just make all parsers be able to finish a segment, save state, and move to the next segment somewhere else in memory.  SharedBuffer::data() returns the data possibly unnecessarily copied into such a single large buffer. I guess I need to make that more clear.
> > Source/WebCore/platform/SharedBuffer.cpp:115
> > +        memcpy(reinterpret_cast<char*>(arrayBuffer->data()) + position, segment->data(), segment->size());
> 
> This should be static_cast. Technically, reinterpret_cast<char*> is a
> violation of strict aliasing rules, so it is a bit of a red flag. But
> static_cast will work just fine.
> 
> Note that this function copies data but you haven't called it out as being
> bad for performance. I wonder what the distinction is?
Maybe we could make ArrayBuffers wrap SharedBuffers, but until then this one copy is necessary.  Note we copy from each segment to the ArrayBuffer without internally copying everything within the same SharedBuffer.
> > Source/WebCore/platform/SharedBufferChunkReader.cpp:112
> > -        m_segmentLength = m_buffer->getSomeData(m_segment, m_bufferPosition);
> > +        // Let's pretend all the data is in one block.
> > +        // It might be less efficient in some exotic cases, but does anyone really care?
> > +        // This class should be removed anyways.
> > +        m_segment = m_buffer->data() + m_bufferPosition;
> > +        m_segmentLength = m_buffer->size() - m_bufferPosition;
> 
> I don't know if I care. Is this class still used right now? If so, I
> probably care. Using the shared helper function I suggested above would
> avoid copying here.
This class is only used by the GTK port which has MHTML enabled.  It should be rewritten if it's important and I've indicated that with my comments.

(In reply to Geoffrey Garen from comment #28)
> Under what conditions does this patch remove previous copying?
This patch removes one copy of data in WebCoreAVFResourceLoader.mm which used to have to call [NSData alloc] initWithBytes] to copy data into a new NSData and now just uses SharedBuffer::createNSDataArray which does 0 copies and gives WebCoreSharedBufferDatas as NSDatas.
This patch removes another copy when buffering data in the NetworkProcess in NetworkResourceLoader::didReceiveBuffer for loads that are buffered, which is done with  synchronous XHR, fonts, and soon to be done with media loading.  It is already one of our top crashers because of failed allocations in SharedBuffer::duplicateDataBufferIfNecessary and this is the right way of removing those allocations and copies, all of which are unnecessary.
There are unfortunately still copies of the data but only when calling SharedBuffer::data, but in WebKit2 that is done in the WebProcess after a SharedBuffer has been serialized by segment in the NetworkProcess and deserialized in one segment in the WebProcess, so there is no actual copying in common cases.  There might be some cases like loading from blobs that still copy, but we can reduce the calls to SharedBuffer::data and do other future improvements, and this doesn't hurt those cases.
Comment 30 Alex Christensen 2017-04-21 00:13:58 PDT
Geoff's other comments are being implemented.
Comment 31 Alex Christensen 2017-04-21 00:31:24 PDT
Created attachment 307703 [details]
Patch
Comment 32 Alex Christensen 2017-04-21 07:22:04 PDT
http://trac.webkit.org/r215608
Comment 33 Geoffrey Garen 2017-04-21 08:33:34 PDT
> > Under what conditions does this patch remove previous copying?
> This patch removes another copy when buffering data in the NetworkProcess in
> NetworkResourceLoader::didReceiveBuffer for loads that are buffered, which
> is done with  synchronous XHR, fonts, and soon to be done with media
> loading.  It is already one of our top crashers because of failed
> allocations in SharedBuffer::duplicateDataBufferIfNecessary and this is the
> right way of removing those allocations and copies, all of which are
> unnecessary.

Nice!

> There are unfortunately still copies of the data but only when calling
> SharedBuffer::data, but in WebKit2 that is done in the WebProcess after a
> SharedBuffer has been serialized by segment in the NetworkProcess and
> deserialized in one segment in the WebProcess, so there is no actual copying
> in common cases.

Sounds good.
Comment 34 Darin Adler 2017-04-21 09:51:40 PDT
Comment on attachment 307703 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=307703&action=review

Looks great.

Since both you and the Andreas Kling who reviewed are experts on memory use I assume you have already considered some of the things I am pointing out here, but I spotted some things that maybe could be a little better.

> Source/WebCore/platform/SharedBuffer.cpp:89
> +    m_segments.clear();
> +    m_segments.append(DataSegment::create(WTFMove(combinedData)));
> +    ASSERT(m_segments.size() == 1);

I don’t think clear/append has optimal behavior for this case. This deallocates the buffer in m_segments then allocates a new one. And the new one is allocated with expandCapacity so I think it will have room for 16 segments. But I think we’d want m_segments to have room for just 1 segment since it’s unlikely there is more appending coming? Maybe the best approach to avoid memory allocation would be to do shrink(1) and then use assignment rather than doing a clear/append? Or maybe we should have an inline capacity of 1 for m_segments so we don’t have to worry? Or we could do reserveCapacity(1) to avoid allocating space for more than 16 segments?

> Source/WebCore/platform/SharedBuffer.cpp:123
> +    m_segments.reserveCapacity(m_segments.size() + data.m_segments.size());
> +    for (const auto& segment : data.m_segments)
> +        m_segments.uncheckedAppend(segment.copyRef());

Using reserveCapacity/uncheckedAppend like this would normally be an anti-pattern if we might be doing more appends after this one. It bypasses the algorithm in expandCapacity for growing capacity that allocates extra capacity. Is this really OK? If this was known to be the last append, then it would be OK, but if there might be more appends, then it’s probably not good.

> Source/WebCore/platform/SharedBuffer.cpp:130
> +    Vector<char> vector;
> +    vector.append(data, length);

This will use expandCapacity and allocate additional unused capacity that we will never use. We could use reserveInitialCapacity to prevent that or we could write this:

    Vector<char> vector { length };
    memcpy(vector.data(), data, length);

> Source/WebCore/platform/SharedBuffer.h:85
> +    RefPtr<ArrayBuffer> createArrayBuffer() const;

This should have the word "try" in its name since it returns nullptr when there is not enough memory, rather than doing a CRASH.

> Source/WebCore/platform/SharedBuffer.h:88
> +    // FIXME: This should return a size_t.
> +    unsigned size() const { return m_size; }

Yes, yes, a thousand times yes. What are we doing about this?

> Source/WebCore/platform/SharedBuffer.h:150
> +    SharedBuffer(Vector<char>&&);

explicit?

> Source/WebCore/platform/SharedBuffer.h:151
> +    SharedBuffer(MappedFileData&&);

explicit?

> Source/WebCore/platform/SharedBuffer.h:153
> +    SharedBuffer(CFDataRef);

explicit?

> Source/WebCore/platform/SharedBuffer.h:156
> +    SharedBuffer(SoupBuffer*);

explicit?

> Source/WebCore/platform/SharedBuffer.h:159
> +    void combineToOneSegment() const;

Would be nicer to have a more grammatical name for this. The phrase "combine to one segment" isn’t grammatically correct English. Maybe fix by using the word "into"?

> Source/WebCore/platform/SharedBuffer.h:164
> +    mutable Vector<Ref<const DataSegment>> m_segments;

Seems strange to use the type "const DataSegment" since there are no mutating functions on a DataSegment. It’s an immutable class so const doesn’t mean anything. Every use of StringImpl in WebKit could say const StringImpl but we don’t bother since it doesn’t mean anything.

Since the type of the vector is visible in the return type of begin/end, I think there should be a public:

    using DataSegmentVector = Vector<Ref<DataSegment>>;

> Source/WebCore/platform/URLParser.cpp:1153
> +    if (input == "file:///Users/alexchristensen/webkit/LayoutTests/webarchive/archive-empty-frame-source.html")
> +        WTFLogAlways("HIT");

Did you land this?

> Source/WebCore/platform/cf/SharedBufferCF.cpp:38
> +    append(data);

Seems unfortunate that we will allocate extra capacity in m_segments here since it’s highly likely we will not append to this buffer.

> Source/WebCore/platform/cf/SharedBufferCF.cpp:54
> +Ref<SharedBuffer> SharedBuffer::create(CFDataRef data)

We could consider adding a call to CFDataCreateCopy in this function. If someone passes a mutable data we really want to keep an immutable copy; if it’s already immutable the copy is just a CFRetain. On the other hand, if we are relying on code that passes mutable CFData and it somehow knows it will never be mutated after that point, then the CFDataCreateCopy call would be a major new expense. Still, seems unsafe now. (Probably no more unsafe than before.) The old name, "wrapCFData" seems more appropriate if the caller have to be careful about passing mutable and guarantee it won’t mutate.

> Source/WebCore/platform/cf/SharedBufferCF.cpp:69
>  void SharedBuffer::append(CFDataRef data)

Same CFDataCreateCopy issue.

> Source/WebCore/platform/cocoa/SharedBufferCocoa.mm:86
> +Ref<SharedBuffer> SharedBuffer::create(NSData *nsData)

We could consider adding a call to [NSData copy] in this function. If someone passes a mutable data we really want to keep an immutable copy; if it’s already immutable the copy is just a retain. On the other hand, if we are relying on code that passes mutable NSData and it somehow knows it will never be mutated after that point, then the [NSData copy] call would be a major new expense. Still, seems unsafe now. (Probably no more unsafe than before.) The old name, "wrapNSData" seems more appropriate if the caller have to be careful about passing mutable and guarantee it won’t mutate.

I noticed that call sites in EditorIOS.mm and WebResource.mm call copy/autorelease, and those are trying to resolve the same issue, but it’s messy to have that be caller responsibility.

> Source/WebCore/platform/cocoa/SharedBufferCocoa.mm:102
> +    combineToOneSegment();
> +    if (!m_segments.size())
> +        return adoptCF(CFDataCreate(nullptr, nullptr, 0));
> +    ASSERT(m_segments.size() == 1);
> +    return adoptCF((CFDataRef)adoptNS([[WebCoreSharedBufferData alloc] initWithSharedBufferDataSegment:m_segments[0]]).leakRef());

Why no special case for when the m_segments has only one object and it’s already a CFData? Like the one in SharedBufferCF.cpp for the non-Foundation case. Wouldn’t it be more efficient to use that instead of WebCoreSharedBufferData in that case?

> Source/WebCore/platform/cocoa/SharedBufferCocoa.mm:117
> +        [dataArray addObject:adoptNS([[WebCoreSharedBufferData alloc] initWithSharedBufferDataSegment:segment]).get()];

Same question here. Why not use existing CFDataRef if it exists?

> Source/WebCore/platform/graphics/avfoundation/objc/WebCoreAVFResourceLoader.mm:184
> +    for (size_t i = 0; i < [array count]; ++i) {
> +        NSData *segment = [array objectAtIndex:i];

Can we use a modern for loop?

    RetainPtr<NSArray> array = data->createNSDataArray();
    for (NSData *segment in array.get()) {

> Source/WebCore/platform/soup/SharedBufferSoup.cpp:34
>  SharedBuffer::SharedBuffer(SoupBuffer* soupBuffer)
> -    : m_buffer(*new DataBuffer)
> -    , m_soupBuffer(soupBuffer)
>  {
>      ASSERT(soupBuffer);
> +    m_size = soupBuffer->length;
> +    m_segments.append(DataSegment::create(GUniquePtr<SoupBuffer>(soupBuffer)));
>  }

Sadly optimizes for additional appends, but I think we expect only one segment here. Might be better to have a constructor that takes a DataSegment&& and have this constructor call that one. And use that pattern in many other places as well.
Comment 35 Geoffrey Garen 2017-04-21 10:19:59 PDT
> > Source/WebCore/platform/SharedBuffer.cpp:89
> > +    m_segments.clear();
> > +    m_segments.append(DataSegment::create(WTFMove(combinedData)));
> > +    ASSERT(m_segments.size() == 1);
> 
> I don’t think clear/append has optimal behavior for this case. This
> deallocates the buffer in m_segments then allocates a new one. And the new
> one is allocated with expandCapacity so I think it will have room for 16
> segments. But I think we’d want m_segments to have room for just 1 segment
> since it’s unlikely there is more appending coming? Maybe the best approach
> to avoid memory allocation would be to do shrink(1) and then use assignment
> rather than doing a clear/append? Or maybe we should have an inline capacity
> of 1 for m_segments so we don’t have to worry? Or we could do
> reserveCapacity(1) to avoid allocating space for more than 16 segments?

I like the option where we use inline capacity of 1 and shrink(1).

> > Source/WebCore/platform/SharedBuffer.cpp:123
> > +    m_segments.reserveCapacity(m_segments.size() + data.m_segments.size());
> > +    for (const auto& segment : data.m_segments)
> > +        m_segments.uncheckedAppend(segment.copyRef());
> 
> Using reserveCapacity/uncheckedAppend like this would normally be an
> anti-pattern if we might be doing more appends after this one. It bypasses
> the algorithm in expandCapacity for growing capacity that allocates extra
> capacity. Is this really OK? If this was known to be the last append, then
> it would be OK, but if there might be more appends, then it’s probably not
> good.

Yeah, grow() followed by indexed assignment is probably better. Alternatively, we could make Vector::expandCapacity() public, and use that.

Note that Vector::operator=() and Vector::fill() also use reserveCapacity() in this problematic way, and we should probably change them too.

> > Source/WebCore/platform/SharedBuffer.h:159
> > +    void combineToOneSegment() const;
> 
> Would be nicer to have a more grammatical name for this. The phrase "combine
> to one segment" isn’t grammatically correct English. Maybe fix by using the
> word "into"?

Maybe just "compact()"?
Comment 36 Alex Christensen 2017-04-21 15:15:14 PDT
> > Source/WebCore/platform/URLParser.cpp:1153
> > +    if (input == "file:///Users/alexchristensen/webkit/LayoutTests/webarchive/archive-empty-frame-source.html")
> > +        WTFLogAlways("HIT");
> 
> Did you land this?
This was SOOO useful for attaching the debugger.  I can't believe I uploaded and committed this :(

I'm going to follow up in https://bugs.webkit.org/show_bug.cgi?id=171144 but I'm not going to add calls to CFDataCreateCopy or [NSData copy] because we never mutate the data and if someone else does then they are misusing the class.  I don't see a need right now to add possible copies.  If a CFDataRef happens to actually copy with a call to CFDataCreateCopy it will hurt performance without changing behavior. We can add it later if we find it beneficial.
I'm also not convinced that I should add a shortcut for if there is only one segment that is a CFDataRef.  Either way, the point is the data buffer is not copied, and either way there's small overhead.
The other things are great suggestions, especially having an inline capacity of 1 in the Vector because that's the common case.
Comment 37 Michael Catanzaro 2017-04-21 19:24:01 PDT
Reverted r215608 for reason:

Hundreds of test failures on GTK bot

Committed r215661: <http://trac.webkit.org/changeset/215661>
Comment 38 Michael Catanzaro 2017-04-21 19:26:38 PDT
This caused a huge amount of layout test failures and timeouts for GTK port. :(