Summary: | Add SharedBufferBuilder class | ||
---|---|---|---|
Product: | WebKit | Reporter: | Jean-Yves Avenard [:jya] <jean-yves.avenard> |
Component: | WebCore Misc. | Assignee: | Jean-Yves Avenard [:jya] <jean-yves.avenard> |
Status: | RESOLVED FIXED | ||
Severity: | Normal | CC: | alecflett, beidson, benjamin, berto, calvaris, cdumez, cgarcia, changseok, eric.carlson, esprehn+autocc, ews-watchlist, galpeter, glenn, gustavo, gyuyoung.kim, hi, Hironori.Fujii, hta, japhet, jbedard, jer.noble, joepeck, jsbell, kangil.han, macpherson, menard, mifenton, mmaxfield, pangle, philipj, pnormand, sergio, tommyw, vjaquez, webkit-bug-importer, youennf |
Priority: | P2 | Keywords: | InRadar |
Version: | Other | ||
Hardware: | Unspecified | ||
OS: | Unspecified | ||
Bug Depends on: | 233030 | ||
Bug Blocks: | 233922, 234215, 233677 | ||
Attachments: |
Description
Jean-Yves Avenard [:jya]
2021-11-23 00:32:45 PST
Created attachment 445253 [details]
SharedBufferBuilder for EWS
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API Created attachment 445256 [details]
SharedBufferBuilder for EWS
Created attachment 445259 [details]
SharedBufferBuilder for EWS
fix gtk/wpe
Created attachment 445267 [details]
SharedBufferBuilder for EWS
fix windows/ios
Created attachment 445386 [details]
scripthash-tests-crash-log.txt
Created attachment 445465 [details]
Patch for EWS
fix intermittent crash, FetchBodyConsumer::takeData() must return null if no data was previously added.
Created attachment 445505 [details]
Patch for EWS
I can reproduce imported/w3c/web-platform-tests/fetch/stale-while-revalidate/stale-css.html intermittents on a clean ToT. Created attachment 445524 [details]
Patch for EWS
Fix API test and windows build
Created attachment 445549 [details]
Patch for EWS
Fix intermittent crash, make passing SharedBuffer const
Created attachment 445556 [details]
Patch for EWS
the api-mac failure is unrelated ; none of the code path modified is hit during that run Created attachment 445636 [details]
Patch
Created attachment 445705 [details]
Patch for EWS
Created attachment 445706 [details]
Patch
Created attachment 445707 [details]
Patch
Created attachment 445708 [details]
Patch for EWS
Created attachment 445709 [details]
Patch for EWS
Created attachment 445711 [details]
Patch
Created attachment 445826 [details]
Patch for EWS
Created attachment 445827 [details]
Patch
Created attachment 445832 [details]
Patch for EWS
Created attachment 445833 [details]
Patch
rebase
Created attachment 445836 [details]
Patch for EWS
Created attachment 445837 [details]
Patch
fix gtk build (again)
Created attachment 445851 [details]
Patch for EWS
Created attachment 445852 [details]
Patch for EWS with no windows symbols resolution
Created attachment 445922 [details]
Patch for EWS
Created attachment 445925 [details]
Patch
Created attachment 445926 [details]
Patch for EWS
Created attachment 445942 [details]
Patch
Created attachment 445949 [details]
Patch
rebase
Created attachment 445950 [details]
Patch for EWS
Created attachment 445996 [details]
Patch
rebase
Created attachment 445997 [details]
Patch for EWS
Created attachment 446021 [details]
Patch for EWS
fix api ios test
Created attachment 446022 [details]
Patch
Created attachment 446129 [details] Patch for EWS rebase following changes in bug 233030 Created attachment 446130 [details]
Patch
Comment on attachment 446130 [details] Patch Some nits below. Overall, this seems good. Two questions though: 1. In most part of the code we go from a Ref<SharedBuffer> (1 allocation) to a UniqueRef<SharedBufferBuilder> (2 allocations). Shouldn't we just use a SharedBufferBuilder with an initiated Ref<SharedBuffer> to keep to 1 allocation? 2. I understand the difference between empty and reset but I had to look at the code to understand it. Maybe we should just have empty (and call it reset). If it is really important to have m_data be back to a RefPtr, I guess we could call take() or use something like builder = { }. View in context: https://bugs.webkit.org/attachment.cgi?id=446130&action=review > Source/WebCore/Modules/fetch/FetchBodyConsumer.cpp:350 > + return m_buffer.take()->tryCreateArrayBuffer(); Looking at take, it creates a shared buffer if needed so we probably do not need the m_buffer check. Maybe though it would be better for take to return a RefPtr and the caller would then do a null check? Or have a takeAndCreateIfNeeded if both patterns are equally used? Or have a takeAsArrayBuffer if that is used in several places. > Source/WebCore/Modules/model-element/HTMLModelElement.cpp:152 > + m_data.empty(); What is the difference between reset and empty? > Source/WebCore/inspector/NetworkResourcesData.cpp:50 > + , m_dataBuffer(makeUniqueRef<SharedBufferBuilder>()) Why making it a unique ref contrary to other places? > Source/WebCore/inspector/NetworkResourcesData.h:125 > RefPtr<SharedBuffer> m_buffer; Is m_buffer needed? > Source/WebCore/loader/ResourceLoader.cpp:79 > + , m_resourceData { makeUniqueRef<SharedBufferBuilder>() } Why UniqueRef? Why not unique_ptr to lazily allocate it or just a direct member variable? > Source/WebCore/platform/SharedBuffer.h:301 > + explicit SharedBufferBuilder(RefPtr<SharedBuffer>&& buffer) Is this one actually needed? Ditto for the Ref version? Or do you envision in the future usage of these? I would tend to keep the Ref<> version if so, not sure the RefPtr version is useful. > Source/WebCore/platform/SharedBuffer.h:305 > + if (!buffer || (buffer->hasOneRef() && !buffer->isContiguous())) { I guess I would write it as if (!buffer) return; ... > Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:544 > + return m_data.take(); To be consistent with existing code, we should probably do: if (!m_data) return nullptr; return m_data.take(); Comment on attachment 446130 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=446130&action=review >> Source/WebCore/inspector/NetworkResourcesData.cpp:50 >> + , m_dataBuffer(makeUniqueRef<SharedBufferBuilder>()) > > Why making it a unique ref contrary to other places? I used a UniqueRef<SharedBufferBuilder> wherever SharedBuffer were only forward declared and the SharedBufferBuilder.h header was only included in the code. Seeing that there were a lot of efforts made to speed up compilation time, I thought it was appropriate to continue that trend. It does make the code more confusing I agree, I will remove that. >> Source/WebCore/inspector/NetworkResourcesData.h:125 >> RefPtr<SharedBuffer> m_buffer; > > Is m_buffer needed? I tried the keep the existing logic as much as possible. Like above, we have hasData() and hasBufferedData() doing the same thing, this smells very fishy to me. NetworkResourceData appears to have the ability to be assigned a SharedBuffer (via setBuffer) and has a getter (buffer()) for it. I didn't investigate to see how that was used. >> Source/WebCore/loader/ResourceLoader.cpp:79 >> + , m_resourceData { makeUniqueRef<SharedBufferBuilder>() } > > Why UniqueRef? Why not unique_ptr to lazily allocate it or just a direct member variable? as above, SharedBuffer was forward declared, so I made SharedBufferBuilder forward declared >> Source/WebCore/platform/SharedBuffer.h:301 >> + explicit SharedBufferBuilder(RefPtr<SharedBuffer>&& buffer) > > Is this one actually needed? Ditto for the Ref version? Or do you envision in the future usage of these? > I would tend to keep the Ref<> version if so, not sure the RefPtr version is useful. This is to replace code where we had: RefPtr<SharedBuffer> m_buffer and in the constructor: ScriptBuffer(RefPtr<SharedBuffer>&& buffer) : m_buffer(WTFMove<buffer)) This is used to initialise an `empty` but not `null` SharedBufferBuilder In various places of the code, there's a distinction between a RefPtr<SharedBufer> where both state (unset or empty (that is a size of 0) are handled differently to Ref<SharedBuffer> where. I will document that a SharedBufferBuilder is really a replacement for RefPtr<SharedBuffer> >> Source/WebCore/platform/SharedBuffer.h:305 >> + if (!buffer || (buffer->hasOneRef() && !buffer->isContiguous())) { > > I guess I would write it as if (!buffer) return; ... I agree. Comment on attachment 446130 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=446130&action=review >> Source/WebCore/Modules/model-element/HTMLModelElement.cpp:152 >> + m_data.empty(); > > What is the difference between reset and empty? reset() is equivalent to doing RefPtr<SharedBuffer> buffer; buffer = nullptr; empty() is Ref<SharedBuffer> buffer = SharedBuffer::create(); buffer->clear(); As SharedBufferBuilder is a replacement to a RefPtr<SharedBuffer> the null state has to be differentiated from the "empty" one. In future change I hope to be able to remove this distinction, but this was needed to make the change smaller and the logic easier with no chance of introducing regression. Comment on attachment 446130 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=446130&action=review >>> Source/WebCore/platform/SharedBuffer.h:301 >>> + explicit SharedBufferBuilder(RefPtr<SharedBuffer>&& buffer) >> >> Is this one actually needed? Ditto for the Ref version? Or do you envision in the future usage of these? >> I would tend to keep the Ref<> version if so, not sure the RefPtr version is useful. > > This is to replace code where we had: > RefPtr<SharedBuffer> m_buffer and in the constructor: > ScriptBuffer(RefPtr<SharedBuffer>&& buffer) : m_buffer(WTFMove<buffer)) > > > This is used to initialise an `empty` but not `null` SharedBufferBuilder > In various places of the code, there's a distinction between a RefPtr<SharedBufer> where both state (unset or empty (that is a size of 0) are handled differently to Ref<SharedBuffer> where. > > I will document that a SharedBufferBuilder is really a replacement for RefPtr<SharedBuffer> Sorry, the proper answer to this question was that I wanted to make the constructor explicit, so I needed an implementation for both RefPtr<SharedBuffer> and Ref<SharedBuffer> Created attachment 446521 [details]
Patch for EWS
Apply comments
Created attachment 446522 [details]
Patch
Created attachment 446526 [details]
Patch for EWS
rebase
Created attachment 446527 [details]
Patch for review
Created attachment 446532 [details]
Patch for EWS
rebase 2
Created attachment 446533 [details]
Patch for review
Created attachment 446540 [details]
Patch for EWS
Created attachment 446542 [details]
Patch for review
Comment on attachment 446542 [details] Patch for review LGTM. View in context: https://bugs.webkit.org/attachment.cgi?id=446542&action=review > Source/WebCore/Modules/fetch/FetchBodyConsumer.cpp:335 > + m_buffer.append(WTFMove(data)); m_buffer = WTFMove(data) might be better? > Source/WebCore/platform/SharedBuffer.cpp:544 > + ensureBuffer(); No need to call reset here. m_buffer = SharedBuffer::create might be enough. > Source/WebCore/platform/SharedBuffer.cpp:550 > + return m_buffer.releaseNonNull(); Could be rewritten to: return m_buffer ? m_buffer.releaseNonNull() : SharedBuffer::create(); > Source/WebCore/platform/SharedBuffer.h:314 > + : SharedBufferBuilder(RefPtr<SharedBuffer> { WTFMove(buffer) }) Here we are doing a if(!buffer) unnecessary check. If we want to share code, we could simply add a: initialize(Ref<SharedBuffer>&&) that we would call here and in the above constructor (initialize(buffer.releaseNonNull()). > Source/WebCore/platform/SharedBuffer.h:337 > + RefPtr<SharedBuffer> get() const { return m_buffer; } Looking a more places, I would be tempted to rename it as buffer(). Given the size of the patch, this could be done in a follow-up if you think that is valuable enough. > Source/WebCore/platform/cf/SharedBufferCF.cpp:38 > + ASSERT(!m_contiguous); Is it needed? I guess this is initialised in SharedBuffer m_contiguous declaration. > Source/WebCore/platform/graphics/gstreamer/eme/GStreamerEMEUtilities.h:61 > + if (auto parsedPayload = extractCencIfNeeded(payload->makeContiguous())) I am missing some other patches defining makeContiguous. InitData is taking a RefPtr&& which does no longer seem needed, except if doing WTFMove(payload)->makeContiguous() would make it more efficient. > Source/WebCore/workers/ScriptBuffer.h:51 > + const SharedBuffer* buffer() const { return m_buffer.get().get(); } .get().get() is a bit weird. I guess the first .get() could be renamed to .buffer(). But then we would have to rename m_buffer to m_bufferBuilder. Oh well, might be good enough for now. > Source/WebCore/workers/WorkerFontLoadRequest.cpp:85 > + m_data.append(*contiguousData); As I read it, we called takeAsContiguous so m_data is null. It seems like we could be able to optimise if we were able to call m_data constructor instead of append. > Source/WebKit/WebProcess/WebCoreSupport/ios/WebPreviewLoaderClient.cpp:70 > + webPage->didFinishLoadForQuickLookDocumentInMainFrame(m_buffer->take().get()); Probably s/->/./ > Source/WebKit/WebProcess/WebCoreSupport/ios/WebPreviewLoaderClient.cpp:75 > + m_buffer->reset(); s/->/./ > Source/WebKit/WebProcess/WebCoreSupport/ios/WebPreviewLoaderClient.h:33 > +#include <wtf/UniqueRef.h> No longer need for UniqueRef.h Created attachment 446693 [details]
Patch for EWS
Created attachment 446700 [details]
Patch for EWS
Created attachment 446701 [details]
Patch for landing
Created attachment 446871 [details]
Patch for EWS
Rebase
Created attachment 446872 [details]
Patch
Rebase
Created attachment 446880 [details]
Patch for EWS
Created attachment 446881 [details]
Patch for Landing
Created attachment 446958 [details]
Patch for EWS
Created attachment 446959 [details]
Patch for landing
Created attachment 446960 [details]
Patch for EWS
Created attachment 446967 [details]
Patch for EWS
rebase
Created attachment 446968 [details]
Patch for Landing
rebase
Created attachment 447004 [details]
Patch
fix GTK crash
Committed r286983 (?): <https://commits.webkit.org/r286983> All reviewed patches have been landed. Closing bug and clearing flags on attachment 447004 [details]. |