Bug 232424 – Have CachedRawResourceClient and related networking actors use SharedBuffer.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Jean-Yves Avenard [:jya]]

As seen in bug 232422.

By using SharedBuffer with CachedRawResourceClient we could eliminate some unnecessary memory allocation and copies.

[Radar WebKit Bug Importer]

<rdar://problem/84744687>

[Jean-Yves Avenard [:jya]]

Created attachment 447793 [details]
WIP

[Jean-Yves Avenard [:jya]]

Created attachment 447795 [details]
WIP

fix gtk build

[Jean-Yves Avenard [:jya]]

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]]

Created attachment 447868 [details]
WIP

[Jean-Yves Avenard [:jya]]

Created attachment 447875 [details]
WIP

rebase

[Jean-Yves Avenard [:jya]]

Created attachment 447876 [details]
WIP

[Jean-Yves Avenard [:jya]]

Created attachment 447999 [details]
WIP

[Jean-Yves Avenard [:jya]]

Created attachment 448015 [details]
Patch for EWS test (combined)

[Jean-Yves Avenard [:jya]]

Created attachment 448018 [details]
Patch For Review

[Jean-Yves Avenard [:jya]]

Created attachment 448022 [details]
Patch For Review

[Jean-Yves Avenard [:jya]]

Created attachment 448023 [details]
Patch For EWS (combined patch)

[Jean-Yves Avenard [:jya]]

Created attachment 448060 [details]
Patch for review

[Jean-Yves Avenard [:jya]]

Created attachment 448061 [details]
Patch for EWS testing (combined patch)

[Jean-Yves Avenard [:jya]]

Created attachment 448464 [details]
Patch

rebase

[youenn fablet]

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]]

Created attachment 448488 [details]
rebase

[EWS]

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 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]]

(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.