Bug 234921 – Remove IPC::SharedBufferDataReference and use IPC::SharedBufferCopy instead

• Component: WebKit2
• Status: RESOLVED FIXED

[Jean-Yves Avenard [:jya]]

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.

[Radar WebKit Bug Importer]

<rdar://problem/87196287>

[Jean-Yves Avenard [:jya]]

Created attachment 448557 [details]
Patch

[Jean-Yves Avenard [:jya]]

Created attachment 448566 [details]
Patch

Fix compilation on GTK and Windows

[Jean-Yves Avenard [:jya]]

Created attachment 448570 [details]
Patch

[Jean-Yves Avenard [:jya]]

Created attachment 448571 [details]
Patch

[Jean-Yves Avenard [:jya]]

Created attachment 448572 [details]
Patch

[Jean-Yves Avenard [:jya]]

Created attachment 448574 [details]
Patch

[youenn fablet]

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?

[Jean-Yves Avenard [:jya]]

Created attachment 448580 [details]
Patch

apply comments

[Jean-Yves Avenard [:jya]]

Created attachment 448581 [details]
Patch

remove stray changes

[youenn fablet]

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;

[Jean-Yves Avenard [:jya]]

(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(); }

[youenn fablet]

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.

[youenn fablet]

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.

[Jean-Yves Avenard [:jya]]

Created attachment 448585 [details]
Patch

apply comments: make constructor explicit, add comments

[Jean-Yves Avenard [:jya]]

Created attachment 448587 [details]
Patch

[Jean-Yves Avenard [:jya]]

Created attachment 448591 [details]
Patch

GTK fixes

[Jean-Yves Avenard [:jya]]

Created attachment 448641 [details]
Patch

GTK/WPE compilation fixes; fix crash when loading icons

[Jean-Yves Avenard [:jya]]

Created attachment 448646 [details]
Patch

[Jean-Yves Avenard [:jya]]

Created attachment 448663 [details]
Patch

[EWS]

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