Summary: | Remove IPC::SharedBufferDataReference and use IPC::SharedBufferCopy instead | ||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jean-Yves Avenard [:jya] <jean-yves.avenard> | ||||||||||||||||||||||||||||||
Component: | WebKit2 | Assignee: | Jean-Yves Avenard [:jya] <jean-yves.avenard> | ||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | calvaris, cgarcia, eric.carlson, ews-watchlist, glenn, gustavo, jer.noble, kkinnunen, menard, philipj, pnormand, sergio, vjaquez, webkit-bug-importer, youennf | ||||||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||||
Version: | Other | ||||||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Jean-Yves Avenard [:jya]
2022-01-06 07:06:30 PST
Created attachment 448557 [details]
Patch
Created attachment 448566 [details]
Patch
Fix compilation on GTK and Windows
Created attachment 448570 [details]
Patch
Created attachment 448571 [details]
Patch
Created attachment 448572 [details]
Patch
Created attachment 448574 [details]
Patch
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? Created attachment 448580 [details]
Patch
apply comments
Created attachment 448581 [details]
Patch
remove stray changes
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; (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 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 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.
Created attachment 448585 [details]
Patch
apply comments: make constructor explicit, add comments
Created attachment 448587 [details]
Patch
Created attachment 448591 [details]
Patch
GTK fixes
Created attachment 448641 [details]
Patch
GTK/WPE compilation fixes; fix crash when loading icons
Created attachment 448646 [details]
Patch
Created attachment 448663 [details]
Patch
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]. |