Bug 234921 - Remove IPC::SharedBufferDataReference and use IPC::SharedBufferCopy instead
Summary: Remove IPC::SharedBufferDataReference and use IPC::SharedBufferCopy instead
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jean-Yves Avenard [:jya]
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-01-06 07:06 PST by Jean-Yves Avenard [:jya]
Modified: 2022-01-07 22:25 PST (History)
15 users (show)

See Also:


Attachments
Patch (94.93 KB, patch)
2022-01-06 20:44 PST, Jean-Yves Avenard [:jya]
no flags Details | Formatted Diff | Diff
Patch (97.62 KB, patch)
2022-01-07 00:39 PST, Jean-Yves Avenard [:jya]
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (97.62 KB, patch)
2022-01-07 01:14 PST, Jean-Yves Avenard [:jya]
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (97.64 KB, patch)
2022-01-07 01:21 PST, Jean-Yves Avenard [:jya]
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (98.51 KB, patch)
2022-01-07 01:46 PST, Jean-Yves Avenard [:jya]
no flags Details | Formatted Diff | Diff
Patch (96.85 KB, patch)
2022-01-07 02:11 PST, Jean-Yves Avenard [:jya]
no flags Details | Formatted Diff | Diff
Patch (95.39 KB, patch)
2022-01-07 04:52 PST, Jean-Yves Avenard [:jya]
no flags Details | Formatted Diff | Diff
Patch (94.22 KB, patch)
2022-01-07 04:57 PST, Jean-Yves Avenard [:jya]
no flags Details | Formatted Diff | Diff
Patch (108.17 KB, patch)
2022-01-07 07:09 PST, Jean-Yves Avenard [:jya]
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (108.21 KB, patch)
2022-01-07 07:21 PST, Jean-Yves Avenard [:jya]
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (111.22 KB, patch)
2022-01-07 07:59 PST, Jean-Yves Avenard [:jya]
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (111.50 KB, patch)
2022-01-07 15:57 PST, Jean-Yves Avenard [:jya]
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (111.50 KB, patch)
2022-01-07 16:31 PST, Jean-Yves Avenard [:jya]
no flags Details | Formatted Diff | Diff
Patch (112.20 KB, patch)
2022-01-07 19:50 PST, Jean-Yves Avenard [:jya]
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jean-Yves Avenard [:jya] 2022-01-06 07:06:30 PST
We have no need to distinguish a SharedBufferDataReference from a SharedBufferCopy.

A SharedBufferDataReference will be converted into a IPC::DataReference (a Span<uint8_t>) and from there the data will be copied again into a SharedBuffer. This is inefficient and unnecessary.

By using an IPC::SharedBufferCopy we can move the SharedBuffer around without ever having to copy the content.

IPC::SharedBufferCopy::decoder should be rewritten to avoid copying its content as done there: https://webkit-search.igalia.com/webkit/rev/105b58d2aab063d1ab6df6717b7b0d7816b674eb/Source/WebKit/Platform/IPC/SharedBufferCopy.cpp#55-57

IPC::SharedBufferCopy should be renamed IPC::SharedBuffer for simplicity sake.
Comment 1 Radar WebKit Bug Importer 2022-01-06 07:07:01 PST
<rdar://problem/87196287>
Comment 2 Jean-Yves Avenard [:jya] 2022-01-06 20:44:51 PST
Created attachment 448557 [details]
Patch
Comment 3 Jean-Yves Avenard [:jya] 2022-01-07 00:39:25 PST
Created attachment 448566 [details]
Patch

Fix compilation on GTK and Windows
Comment 4 Jean-Yves Avenard [:jya] 2022-01-07 01:14:59 PST
Created attachment 448570 [details]
Patch
Comment 5 Jean-Yves Avenard [:jya] 2022-01-07 01:21:46 PST
Created attachment 448571 [details]
Patch
Comment 6 Jean-Yves Avenard [:jya] 2022-01-07 01:46:25 PST
Created attachment 448572 [details]
Patch
Comment 7 Jean-Yves Avenard [:jya] 2022-01-07 02:11:43 PST
Created attachment 448574 [details]
Patch
Comment 8 youenn fablet 2022-01-07 02:54:58 PST
Comment on attachment 448574 [details]
Patch

I like the direction.
I wonder how we could remove those makeContiguous calls that are actually no-op on receive side.
Ideally we should be able on sender side to provide a Fragmented buffer and get a contiguous buffer on receiver side.
Any idea?

View in context: https://bugs.webkit.org/attachment.cgi?id=448574&action=review

> Source/WebCore/platform/graphics/win/ImageDecoderDirect2D.cpp:285
> +    Ref<SharedBuffer> buffer = data.makeContiguous();

auto

> Source/WebCore/platform/graphics/win/ImageDecoderDirect2D.cpp:286
> +    char* dataPtr = const_cast<char*>(buffer->data());

auto*
We should probably introduce a non const getter instead.

> Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerDownloadTask.cpp:185
> +    size_t bytesWritten = FileSystem::writeToFile(m_downloadFile, contiguousData->data(), contiguousData->size());

This looks inefficient even though this is probably efficient.
We do not need to call contiguousBuffer here given we want to write in a file and we could write pieces by pieces.
AIUI, data is coming straight from IPC so is probably contiguous here.

> Source/WebKit/Platform/IPC/SharedBufferCopy.h:55
> +    Ref<WebCore::FragmentedSharedBuffer> safeBuffer() const;

Do we need all these methods given I guess we only manipulate SharedBufferCopy (outside of IPC sending) that have a contiguous buffer?
Comment 9 Jean-Yves Avenard [:jya] 2022-01-07 04:52:07 PST
Created attachment 448580 [details]
Patch

apply comments
Comment 10 Jean-Yves Avenard [:jya] 2022-01-07 04:57:57 PST
Created attachment 448581 [details]
Patch

remove stray changes
Comment 11 youenn fablet 2022-01-07 05:01:54 PST
View in context: https://bugs.webkit.org/attachment.cgi?id=448580&action=review

> Source/WebCore/platform/graphics/win/ImageDecoderDirect2D.cpp:286
> +    char* dataPtr = const_cast<char*>(buffer->data());

auto/auto*

> Source/WebCore/platform/graphics/win/ImageDecoderDirect2D.cpp:287
>      hr = stream->InitializeFromMemory(reinterpret_cast<BYTE*>(dataPtr), data.size());

Do we need this reinterpret_cast given SharedBuffer can return a const uint8_t*

> Source/WebKit/Platform/IPC/SharedBufferCopy.cpp:54
> +    return downcast<SharedBuffer>(m_buffer.get())->data();

Do we need this getter?
I am a bit hesitant to do downcast.

> Source/WebKit/Platform/IPC/SharedBufferCopy.h:57
> +    RefPtr<WebCore::SharedBuffer> buffer() const { return downcast<WebCore::SharedBuffer>(m_buffer.get()); }

It might be safer to go with something like:
ASSERT(!m_buffer || m_buffer.isContiguous());
return m_buffer ? m_buffer->makeContiguous() : nullptr;
Comment 12 Jean-Yves Avenard [:jya] 2022-01-07 05:23:58 PST
(In reply to youenn fablet from comment #11)
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=448580&action=review
> 
> > Source/WebCore/platform/graphics/win/ImageDecoderDirect2D.cpp:286
> > +    char* dataPtr = const_cast<char*>(buffer->data());
> 
> auto/auto*
> 
> > Source/WebCore/platform/graphics/win/ImageDecoderDirect2D.cpp:287
> >      hr = stream->InitializeFromMemory(reinterpret_cast<BYTE*>(dataPtr), data.size());
> 
> Do we need this reinterpret_cast given SharedBuffer can return a const
> uint8_t*

no idea, I've never touched this code before; in fact I'm now 100% this isn't used at all because the code as it was it couldn't have compiled.
The argument is a FragmentedSharedBuffer() which doesn't have a data() method; I only found that code because I sourced a particular code pattern.


> 
> > Source/WebKit/Platform/IPC/SharedBufferCopy.cpp:54
> > +    return downcast<SharedBuffer>(m_buffer.get())->data();
> 
> Do we need this getter?
> I am a bit hesitant to do downcast.
> 
> > Source/WebKit/Platform/IPC/SharedBufferCopy.h:57
> > +    RefPtr<WebCore::SharedBuffer> buffer() const { return downcast<WebCore::SharedBuffer>(m_buffer.get()); }
> 
> It might be safer to go with something like:
> ASSERT(!m_buffer || m_buffer.isContiguous());
> return m_buffer ? m_buffer->makeContiguous() : nullptr;

this is 100% identical to using downcast. makeContiguous() will return the same object if isContiguous() is true.

downcast does:
ASSERT_WITH_SECURITY_IMPLICATION(!source || is<Target>(*source));
and is<Target> calls isType() which does:
bool isType(const WebCore::FragmentedSharedBuffer& buffer) { return buffer.isContiguous(); }
Comment 13 youenn fablet 2022-01-07 05:27:23 PST
Comment on attachment 448580 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=448580&action=review

> Source/WebKit/Platform/IPC/SharedBufferCopy.h:45
> +        : m_buffer(WTFMove(buffer)) { }

Let's mark them as explicit and mention they should only be used for IPC serialisation.
Comment 14 youenn fablet 2022-01-07 05:42:37 PST
Comment on attachment 448581 [details]
Patch

As discussed offline, it might be good to fix the perf regression.
In the end, we might want on receiver side to pass SharedBufferCopy&& from which we can extract Ref<SharedBuffer>.
Then we can remove SharedBufferCopy::data.
Comment 15 Jean-Yves Avenard [:jya] 2022-01-07 07:09:38 PST
Created attachment 448585 [details]
Patch

apply comments: make constructor explicit, add comments
Comment 16 Jean-Yves Avenard [:jya] 2022-01-07 07:21:44 PST
Created attachment 448587 [details]
Patch
Comment 17 Jean-Yves Avenard [:jya] 2022-01-07 07:59:48 PST
Created attachment 448591 [details]
Patch

GTK fixes
Comment 18 Jean-Yves Avenard [:jya] 2022-01-07 15:57:43 PST
Created attachment 448641 [details]
Patch

GTK/WPE compilation fixes; fix crash when loading icons
Comment 19 Jean-Yves Avenard [:jya] 2022-01-07 16:31:04 PST
Created attachment 448646 [details]
Patch
Comment 20 Jean-Yves Avenard [:jya] 2022-01-07 19:50:30 PST
Created attachment 448663 [details]
Patch
Comment 21 EWS 2022-01-07 22:24:58 PST
Committed r287808 (245860@main): <https://commits.webkit.org/245860@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 448663 [details].