RESOLVED FIXED 232424
Have CachedRawResourceClient and related networking actors use SharedBuffer.
https://bugs.webkit.org/show_bug.cgi?id=232424
Summary Have CachedRawResourceClient and related networking actors use SharedBuffer.
Jean-Yves Avenard [:jya]
Reported 2021-10-27 23:39:56 PDT
As seen in bug 232422. By using SharedBuffer with CachedRawResourceClient we could eliminate some unnecessary memory allocation and copies.
Attachments
WIP (229.54 KB, patch)
2021-12-22 04:45 PST, Jean-Yves Avenard [:jya]
ews-feeder: commit-queue-
WIP (234.68 KB, patch)
2021-12-22 05:06 PST, Jean-Yves Avenard [:jya]
no flags
WIP (239.09 KB, patch)
2021-12-23 03:46 PST, Jean-Yves Avenard [:jya]
no flags
WIP (261.24 KB, patch)
2021-12-23 05:52 PST, Jean-Yves Avenard [:jya]
ews-feeder: commit-queue-
WIP (261.23 KB, patch)
2021-12-23 06:09 PST, Jean-Yves Avenard [:jya]
no flags
WIP (281.09 KB, patch)
2021-12-27 06:12 PST, Jean-Yves Avenard [:jya]
no flags
Patch for EWS test (combined) (280.36 KB, patch)
2021-12-27 17:39 PST, Jean-Yves Avenard [:jya]
no flags
Patch For Review (255.67 KB, patch)
2021-12-27 18:37 PST, Jean-Yves Avenard [:jya]
ews-feeder: commit-queue-
Patch For Review (255.63 KB, patch)
2021-12-27 19:17 PST, Jean-Yves Avenard [:jya]
no flags
Patch For EWS (combined patch) (270.06 KB, patch)
2021-12-27 19:18 PST, Jean-Yves Avenard [:jya]
no flags
Patch for review (260.42 KB, patch)
2021-12-28 19:02 PST, Jean-Yves Avenard [:jya]
no flags
Patch for EWS testing (combined patch) (276.92 KB, patch)
2021-12-28 19:03 PST, Jean-Yves Avenard [:jya]
no flags
Patch (260.43 KB, patch)
2022-01-05 20:04 PST, Jean-Yves Avenard [:jya]
no flags
rebase (259.77 KB, patch)
2022-01-06 05:25 PST, Jean-Yves Avenard [:jya]
no flags
Radar WebKit Bug Importer
Comment 1 2021-10-27 23:40:20 PDT
Jean-Yves Avenard [:jya]
Comment 2 2021-12-22 04:45:59 PST
Jean-Yves Avenard [:jya]
Comment 3 2021-12-22 05:06:40 PST
Created attachment 447795 [details] WIP fix gtk build
Jean-Yves Avenard [:jya]
Comment 4 2021-12-22 16:24:25 PST
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.
Jean-Yves Avenard [:jya]
Comment 5 2021-12-23 03:46:26 PST
Jean-Yves Avenard [:jya]
Comment 6 2021-12-23 05:52:22 PST
Created attachment 447875 [details] WIP rebase
Jean-Yves Avenard [:jya]
Comment 7 2021-12-23 06:09:22 PST
Jean-Yves Avenard [:jya]
Comment 8 2021-12-27 06:12:31 PST
Jean-Yves Avenard [:jya]
Comment 9 2021-12-27 17:39:11 PST
Created attachment 448015 [details] Patch for EWS test (combined)
Jean-Yves Avenard [:jya]
Comment 10 2021-12-27 18:37:36 PST
Created attachment 448018 [details] Patch For Review
Jean-Yves Avenard [:jya]
Comment 11 2021-12-27 19:17:05 PST
Created attachment 448022 [details] Patch For Review
Jean-Yves Avenard [:jya]
Comment 12 2021-12-27 19:18:07 PST
Created attachment 448023 [details] Patch For EWS (combined patch)
Jean-Yves Avenard [:jya]
Comment 13 2021-12-28 19:02:09 PST
Created attachment 448060 [details] Patch for review
Jean-Yves Avenard [:jya]
Comment 14 2021-12-28 19:03:27 PST
Created attachment 448061 [details] Patch for EWS testing (combined patch)
Jean-Yves Avenard [:jya]
Comment 15 2022-01-05 20:04:09 PST
Created attachment 448464 [details] Patch rebase
youenn fablet
Comment 16 2022-01-06 01:59:38 PST
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?
Jean-Yves Avenard [:jya]
Comment 17 2022-01-06 05:25:12 PST
EWS
Comment 18 2022-01-06 07:32:55 PST
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].
Chris Dumez
Comment 19 2022-01-25 14:19:00 PST
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.
Jean-Yves Avenard [:jya]
Comment 20 2022-01-25 17:08:42 PST
(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.
Note You need to log in before you can comment on or make changes to this bug.