| Summary: | Remove unnecessary flattening of SharedBuffer when sending them over IPC | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Jean-Yves Avenard [:jya] <jean-yves.avenard> | ||||||
| Component: | Page Loading | Assignee: | Jean-Yves Avenard [:jya] <jean-yves.avenard> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | beidson, sam, webkit-bug-importer | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | Other | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| See Also: |
https://bugs.webkit.org/show_bug.cgi?id=233030 https://bugs.webkit.org/show_bug.cgi?id=233401 |
||||||||
| Attachments: |
|
||||||||
Created attachment 444814 [details]
Patch
Created attachment 444854 [details]
Patch
Ref<T> doesn't support indirection operator
Comment on attachment 444854 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=444854&action=review > Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:2668 > auto buffer = SharedBuffer::create(data); > + auto sharedMemory = SharedMemory::copyBuffer(buffer.get()); > + if (!sharedMemory) > + continue; Would be nice to avoid the SharedBuffer allocation here also at some point by adding something like a SharedMemory::copyBuffer() that took a Span. Comment on attachment 444854 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=444854&action=review >> Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:2668 >> + continue; > > Would be nice to avoid the SharedBuffer allocation here also at some point by adding something like a SharedMemory::copyBuffer() that took a Span. data is a NSData, the SharedBuffer here creates a DataSegment that keeps a ref to the NSData; so there's no copy ; it works in practice like a span. in bug 230329; we added the ability to append to a SharedBuffer a DataSegment::Provider which is a struct providing two methods: size() and data() Committed r286097 (244484@main): <https://commits.webkit.org/244484@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 444854 [details]. (In reply to Jean-Yves Avenard [:jya] from comment #5) > Comment on attachment 444854 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=444854&action=review > > >> Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:2668 > >> + continue; > > > > Would be nice to avoid the SharedBuffer allocation here also at some point by adding something like a SharedMemory::copyBuffer() that took a Span. > > data is a NSData, the SharedBuffer here creates a DataSegment that keeps a > ref to the NSData; so there's no copy ; it works in practice like a span. > > in bug 230329; we added the ability to append to a SharedBuffer a > DataSegment::Provider which is a struct providing two methods: size() and > data() I wasn't worried about a copy, just trying to avoid an extra allocation of the SharedBuffer itself. |
In a few places in the code, a pattern to send a SharedBuffer across IPC doing like so: ``` RefPtr<SharedBuffer> buffer = cachedImage->resourceBuffer(); if (!buffer) return; uint64_t bufferSize = buffer->size(); RefPtr<SharedMemory> sharedMemoryBuffer = SharedMemory::allocate(bufferSize); memcpy(sharedMemoryBuffer->data(), buffer->data(), bufferSize); SharedMemory::Handle handle; sharedMemoryBuffer->createHandle(handle, SharedMemory::Protection::ReadOnly); send(Messages::WebPageProxy::SaveImageToLibrary(SharedMemory::IPCHandle { WTFMove(handle), bufferSize })); ``` SharedBuffer::data() will silently flatten the Sharedbuffer if made of multiple segments, which includes a new memory allocation and a copy. When the above could be done like so: ``` RefPtr<SharedBuffer> buffer = cachedImage->resourceBuffer(); if (!buffer) return; auto sharedMemoryBuffer = SharedMemory::copyBuffer(*buffer); SharedMemory::Handle handle; sharedMemoryBuffer->createHandle(handle, SharedMemory::Protection::ReadOnly); send(Messages::WebPageProxy::SaveImageToLibrary(SharedMemory::IPCHandle { WTFMove(handle), buffer->size() })); ``` the call SharedMemory::copyBuffer will achieve the same as above, but without the preliminary flattening of the SharedBuffer, eliminating a memory allocation and a copy.