Bug 233363 – Remove unnecessary flattening of SharedBuffer when sending them over IPC

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Jean-Yves Avenard [:jya]]

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.

[Radar WebKit Bug Importer]

<rdar://problem/85600684>

[Jean-Yves Avenard [:jya]]

Created attachment 444814 [details]
Patch

[Jean-Yves Avenard [:jya]]

Created attachment 444854 [details]
Patch

Ref<T> doesn't support indirection operator

[Sam Weinig]

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.

[Jean-Yves Avenard [:jya]]

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

[EWS]

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

[Sam Weinig]

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