WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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-
Details
Formatted Diff
Diff
WIP
(234.68 KB, patch)
2021-12-22 05:06 PST
,
Jean-Yves Avenard [:jya]
no flags
Details
Formatted Diff
Diff
WIP
(239.09 KB, patch)
2021-12-23 03:46 PST
,
Jean-Yves Avenard [:jya]
no flags
Details
Formatted Diff
Diff
WIP
(261.24 KB, patch)
2021-12-23 05:52 PST
,
Jean-Yves Avenard [:jya]
ews-feeder
: commit-queue-
Details
Formatted Diff
Diff
WIP
(261.23 KB, patch)
2021-12-23 06:09 PST
,
Jean-Yves Avenard [:jya]
no flags
Details
Formatted Diff
Diff
WIP
(281.09 KB, patch)
2021-12-27 06:12 PST
,
Jean-Yves Avenard [:jya]
no flags
Details
Formatted Diff
Diff
Patch for EWS test (combined)
(280.36 KB, patch)
2021-12-27 17:39 PST
,
Jean-Yves Avenard [:jya]
no flags
Details
Formatted Diff
Diff
Patch For Review
(255.67 KB, patch)
2021-12-27 18:37 PST
,
Jean-Yves Avenard [:jya]
ews-feeder
: commit-queue-
Details
Formatted Diff
Diff
Patch For Review
(255.63 KB, patch)
2021-12-27 19:17 PST
,
Jean-Yves Avenard [:jya]
no flags
Details
Formatted Diff
Diff
Patch For EWS (combined patch)
(270.06 KB, patch)
2021-12-27 19:18 PST
,
Jean-Yves Avenard [:jya]
no flags
Details
Formatted Diff
Diff
Patch for review
(260.42 KB, patch)
2021-12-28 19:02 PST
,
Jean-Yves Avenard [:jya]
no flags
Details
Formatted Diff
Diff
Patch for EWS testing (combined patch)
(276.92 KB, patch)
2021-12-28 19:03 PST
,
Jean-Yves Avenard [:jya]
no flags
Details
Formatted Diff
Diff
Patch
(260.43 KB, patch)
2022-01-05 20:04 PST
,
Jean-Yves Avenard [:jya]
no flags
Details
Formatted Diff
Diff
rebase
(259.77 KB, patch)
2022-01-06 05:25 PST
,
Jean-Yves Avenard [:jya]
no flags
Details
Formatted Diff
Diff
Show Obsolete
(13)
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2021-10-27 23:40:20 PDT
<
rdar://problem/84744687
>
Jean-Yves Avenard [:jya]
Comment 2
2021-12-22 04:45:59 PST
Created
attachment 447793
[details]
WIP
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
Created
attachment 447868
[details]
WIP
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
Created
attachment 447876
[details]
WIP
Jean-Yves Avenard [:jya]
Comment 8
2021-12-27 06:12:31 PST
Created
attachment 447999
[details]
WIP
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
Created
attachment 448488
[details]
rebase
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.
Top of Page
Format For Printing
XML
Clone This Bug