As seen in bug 232422. By using SharedBuffer with CachedRawResourceClient we could eliminate some unnecessary memory allocation and copies.
<rdar://problem/84744687>
Created attachment 447793 [details] WIP
Created attachment 447795 [details] WIP fix gtk build
I think the crash seeing here are similar to those seen in bug 230121. The underlying vector gets modified. I suspect a call to SharedBuffer::take() in the way.
Created attachment 447868 [details] WIP
Created attachment 447875 [details] WIP rebase
Created attachment 447876 [details] WIP
Created attachment 447999 [details] WIP
Created attachment 448015 [details] Patch for EWS test (combined)
Created attachment 448018 [details] Patch For Review
Created attachment 448022 [details] Patch For Review
Created attachment 448023 [details] Patch For EWS (combined patch)
Created attachment 448060 [details] Patch for review
Created attachment 448061 [details] Patch for EWS testing (combined patch)
Created attachment 448464 [details] Patch rebase
Comment on attachment 448464 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=448464&action=review > Source/WebCore/Modules/fetch/FetchLoader.cpp:151 > +void FetchLoader::didReceiveData(const SharedBuffer& buffer, bool) This bool is mysterious. Ideally, we would remove it, or use a bool enum. > Source/WebCore/loader/DocumentThreadableLoader.cpp:454 > + m_client->didReceiveData(buffer, false); It does not seem that we have a case where we call didReceiveData(buffer, true). Maybe we can just not pass this bool, especially since we probably no longer have -1 as the length in buffer. > Source/WebCore/workers/WorkerScriptLoader.cpp:218 > + size_t len = useStrLen ? strlen(reinterpret_cast<const char*>(buffer.data())) : buffer.size(); Can we try removing this -1 code path? > Source/WebCore/xml/XMLHttpRequest.cpp:1058 > + size_t len = useStrLen ? strlen(reinterpret_cast<const char*>(buffer.data())) : buffer.size(); Can we try removing this -1 code path? > Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerSoftUpdateLoader.cpp:189 > + if (!buffer.isEmpty()) { Should we move that check in forEachSegment? Or are we expecting that other forEachSegment call sites will not benefit of this?
Created attachment 448488 [details] rebase
Committed r287684 (245780@main): <https://commits.webkit.org/245780@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 448488 [details].
Comment on attachment 448488 [details] rebase Aren't we creating an extra copy of the data whenever we construct the SharedBuffer now? note that the long term place for these didReceiveData() functions was to pass a Span<const uint8_t> (which is nicer to deal with and yet doesn't create a copy). Note that this regressed memory (rdar://87830583). It is unclear to me if the issue is due to this extra copying or if there is some other bug. Either way, I do think it is a bad idea to create a copy of the data unconditionally and unnecessarily in many cases.
(In reply to Chris Dumez from comment #19) > Comment on attachment 448488 [details] > rebase > > Aren't we creating an extra copy of the data whenever we construct the > SharedBuffer now? note that the long term place for these didReceiveData() > functions was to pass a Span<const uint8_t> (which is nicer to deal with and > yet doesn't create a copy). No, the SharedBuffer is wrapping the SharedMemory as it comes from the network process. There's no new memory allocation compare to what was there before, only less. The data is never copied once in the content process, it's refcounted instead. > > Note that this regressed memory (rdar://87830583). It is unclear to me if > the issue is due to this extra copying or if there is some other bug. Either > way, I do think it is a bad idea to create a copy of the data > unconditionally and unnecessarily in many cases. we do not create a copy.