Reduce copies and allocations in SharedBuffer::append
Created attachment 307398 [details] Patch
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
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 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
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 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
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 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
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 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
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
Created attachment 307596 [details] Patch
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 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.
(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 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
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 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
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 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
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
Created attachment 307666 [details] Patch
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
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 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
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 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 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.
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.
Geoff's other comments are being implemented.
Created attachment 307703 [details] Patch
http://trac.webkit.org/r215608
> > 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 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.
> > 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()"?
> > 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.
Reverted r215608 for reason: Hundreds of test failures on GTK bot Committed r215661: <http://trac.webkit.org/changeset/215661>
This caused a huge amount of layout test failures and timeouts for GTK port. :(
This needs to go in. Could someone debug this on linux?
(In reply to Alex Christensen from comment #39) > This needs to go in. Could someone debug this on linux? I will
Comment on attachment 307703 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=307703&action=review > Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:163 > - const char* segment; > - while (unsigned segmentLength = data.getSomeData(segment, m_readOffset)) { > - m_readOffset += segmentLength; > + for (const auto& segment : data) { > + m_readOffset += segment->size(); > m_currentBufferSize = m_readOffset; > - png_process_data(m_png, m_info, reinterpret_cast<png_bytep>(const_cast<char*>(segment)), segmentLength); > + png_process_data(m_png, m_info, reinterpret_cast<png_bytep>(const_cast<char*>(segment->data())), segment->size()); Here is the problem. We are always reading the first segment, we need to skip bytes up to m_readOffset. We should probably document this pattern, or even better try to add API to SharedBuffer, since this is duplicated in 3 places in this patch now. I'll submit a new patch with the fix now.
Created attachment 307963 [details] Patch for landing I've fixed the png encoder and removed the debug thing that landed in the original patch, but I think it would be better to address Darin's comments before landing this again.
Comment on attachment 307963 [details] Patch for landing Attachment 307963 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3593761 New failing tests: webrtc/datachannel/basic.html
Created attachment 307966 [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
(In reply to Carlos Garcia Campos from comment #41) > Comment on attachment 307703 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=307703&action=review > > > Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:163 > > - const char* segment; > > - while (unsigned segmentLength = data.getSomeData(segment, m_readOffset)) { > > - m_readOffset += segmentLength; > > + for (const auto& segment : data) { > > + m_readOffset += segment->size(); > > m_currentBufferSize = m_readOffset; > > - png_process_data(m_png, m_info, reinterpret_cast<png_bytep>(const_cast<char*>(segment)), segmentLength); > > + png_process_data(m_png, m_info, reinterpret_cast<png_bytep>(const_cast<char*>(segment->data())), segment->size()); > > Here is the problem. We are always reading the first segment, we need to > skip bytes up to m_readOffset. We should probably document this pattern, or > even better try to add API to SharedBuffer, since this is duplicated in 3 > places in this patch now. I'll submit a new patch with the fix now. Thanks, Carlos! In a followup patch, I think I'll add a function to access a const Vector<Ref<DataSegment>> so we can store the index of the next segment instead of the number of bytes we've read. That'll take a O(n) operation of skipping bytes down to a O(1) operation of skipping to a known index in the segment vector. Storing the read offset in bytes was a bad design designed around the getSomeData function, which didn't really reflect the way in which we receive data. The media loader still needs to have an option of skipping bytes right now in case a byte range request is responded to with a different range of bytes, which we might actually decide we don't want to support. I'm planning to address Darin's comments and upload another patch here.
Created attachment 307986 [details] Patch
http://trac.webkit.org/r215686
rdar://problem/29249686 rdar://problem/31485485
Re-opened since this is blocked by bug 171347
This caused: Bug 171602: REGRESSION(r215686): Incremental reads from SharedBuffer are wrong after r215686
Comment on attachment 307986 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=307986&action=review > Source/WebCore/platform/graphics/avfoundation/objc/WebCoreAVFResourceLoader.mm:202 > + if (!remainingLength) This looks odd. Since you set remainingLength to 0 on the previous line, we'll also break here..
Comment on attachment 307986 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=307986&action=review > Source/WebCore/platform/graphics/avfoundation/objc/WebCoreAVFResourceLoader.mm:-183 > - // Check to see if there is any data available in the buffer to fulfill the data request. > - if (data->size() <= [dataRequest currentOffset] - responseOffset) > - return; I'm also not completely sure why this was there. It didn't seem to need it conceptually, but this might be the source of the regression. >> Source/WebCore/platform/graphics/avfoundation/objc/WebCoreAVFResourceLoader.mm:202 >> + if (!remainingLength) > > This looks odd. Since you set remainingLength to 0 on the previous line, we'll also break here.. That is indeed odd, probably wrong, and maybe the cause of rdar://problem/32003717 Maybe we get away with it because most of the SharedBuffers we see here only have one segment. We should fix this.
Comment on attachment 307986 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=307986&action=review > Source/WebCore/platform/SharedBuffer.cpp:152 > + clone->m_segments.uncheckedAppend(segment.copyRef()); This makes copy to no longer copy the data, it's a new SharedBuffer with new segments pointing to the same data. Is this on purpose? If it is, we should probably document it, I've just found a bug caused by a misuse of this copy function.
(In reply to Carlos Garcia Campos from comment #53) > Comment on attachment 307986 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=307986&action=review > > > Source/WebCore/platform/SharedBuffer.cpp:152 > > + clone->m_segments.uncheckedAppend(segment.copyRef()); > > This makes copy to no longer copy the data, it's a new SharedBuffer with new > segments pointing to the same data. Is this on purpose? If it is, we should > probably document it, I've just found a bug caused by a misuse of this copy > function. See https://bugs.webkit.org/show_bug.cgi?id=173464