1405824 - ShmSegmentsWriter::Write allocates during AddBlobImage

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, enhancement, P2)

Description

[Jeff Muizelaar [:jrmuizel]]

These allocations are showing in profiles.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

i.e. 40% of our BlobImage recording time is spent allocating shared memory

Comment 2

[Nicolas Silva [:nical]]

I think that recycling shmem segments would fix most of the issue. alternatively we could decide to spend time implementing the data pipe thing. I am fine with going in either direction. The data pipe approach will certainly be better in the long run but also require more work to get right.

Comment 3

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Hi nical,
Could you tell more about the data pipe thing? Do you already have the discussion log or document for this?

Comment 4

[Jeff Muizelaar [:jrmuizel]]

(In reply to Jerry Shih[:jerry] (UTC+8) from comment #3)
> Hi nical,
> Could you tell more about the data pipe thing? Do you already have the
> discussion log or document for this?

https://bugzilla.mozilla.org/show_bug.cgi?id=1346446#c1

Comment 5

[Jeff Muizelaar [:jrmuizel]]

We may be able to work around this by fixing bug 1422040

Comment 6

[Nicolas Silva [:nical]]

Attachment: Part 1 - Use PWebRenderBridge insead of IShmemAllocator in IpcResourceUpdateQueue.

It will make sense in another patch.

Comment 7

[Nicolas Silva [:nical]]

Attachment: Part 2 - store an atomic ref count at the begining of small allocs

A later patch takes advantage of this.

Comment 8

[Nicolas Silva [:nical]]

Attachment: Part 3 - Keep a shmem in WebRenderBridgeChild that IpcResourceUpdateQueue can reuse.

Comment 9

[Nicolas Silva [:nical]]

Attachment: Part 4 - Properly handle deallocating the shmem on either side.

And accidentally found out that WebRenderBridgeParent::IPCOpen() always returns true which is not what it is meant to do, so the fix for that got included here.

Comment 10

[Nicolas Silva [:nical]]

This patch set gets read of the majority of shmem allocations. It's not great but it's simple. Eventually I'd like to replace that either by a proper data pipe as we have talked about a few times, or by a proper reference counted shmem type that would handle the cross process atomic refcount in a nicer way (storing the ref count along with the metadata in the front guard page instead of the visible part of the buffer among other things).
In the mean time this should do.

Comment 11

[Nicolas Silva [:nical]]

My plan in the very short term is to add an atomic counter in the shared memory header that indicates how many processes have ipc::Shmem instances referring to the shared memory segment. This will make it possible to remove the ugly/dangerous hack of storing that counter at the beginning of the buffer with the data for some shmems and not others.

Comment 12

[Nicolas Silva [:nical]]

Attachment: Add a RefCountedShmem type

This type will be used in later patches to implement recycling optimizations. The idea is to have a Shmem type separate from Shmem to avoid dangerous bugs, that stores an atomic reference count. This reference count is updated manually for now.
I learned (the unpleasant way) that while it is possible to store a Shmem in a C++ type and serialize it with with the ParamTraits mechanism, what you get on the other side is a Shmem that points to no shared memory at all. As a result there is an IPDL type RefCountedShmem and a set of static methods implemented in a fake class RefCountedShm (pretty confusing, suggestions to make this better within the constraints of IPDL very welcome).

Comment 13

[Nicolas Silva [:nical]]

Attachment: Part 3 - Use the RefCountedShmem type instead of Shmem for small ResourceUpdates alloc.

Use the RefCountedShmem type instead of Shmem for small allocs, but don't use the ref counting at all. This should exactly the same thing as without the patch, the goal being to move the uninteresting boilerplate out of the next patch which deserves a more careful review.

Comment 14

[Nicolas Silva [:nical]]

Attachment: Part 4 - Take advantage of the ref counting to recycle a shmem.

This does the manual reference counting and keeps a single ref counted shmem in WebRenderBridgeChild to recycle it. We could build upon this to recycle any number of shmems, but recycling one gets rid of the vast majority of allocations with all the sites that I tested this on.

Comment 15

[Nicolas Silva [:nical]]

Attachment: Take the header into account in the default chunk alloc size.

Adjust the size so that the total is exactly 64k, otherwise we'll end up allocating an extra page each time.

Comment 16

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8941110 [details] [diff] [review]
Add a RefCountedShmem type

Review of attachment 8941110 [details] [diff] [review]:
-----------------------------------------------------------------

Can anything bad happen if the client is maliciously manipulating the refcount?

Comment 17

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8941115 [details] [diff] [review]
Part 4 - Take advantage of the ref counting to recycle a shmem.

Review of attachment 8941115 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/wr/WebRenderBridgeChild.cpp
@@ +643,5 @@
>  }
>  
> +bool
> +WebRenderBridgeChild::AllocResourceShmem(size_t aSize, RefCountedShmem& aShm)
> +{

It's probably worth adding some more comments to this function. It took me some time to reason through what was going on.

Comment 18

[Nicolas Silva [:nical]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #16)
> Comment on attachment 8941110 [details] [diff] [review]
> Add a RefCountedShmem type
> 
> Review of attachment 8941110 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can anything bad happen if the client is maliciously manipulating the
> refcount?

As far as I can tell it shouldn't be worse than what clients can already do with regular shmems (deallocate them too early or leak them). My understanding is that if the client deallocates the shmem too early and the host tries to read from it, an assert will blow up on the host side in ipdl code as we try to look a shmem up that isn't in the table anymore.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/35b2ebc973dc
Add a RefCountedShmem type that supports manual cross-process reference counting. r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/62401d22f92a
Use WebRenderBridgeChild instead of IShmemAllocator in IpcResourceUpdateQueue. r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c8d66d7b464
Recycle a Shmem for IpcResourceUpdateQueue to avoid allocation overhead. r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d1e7b9d2caa
Adjust the default resource update alloc chunk size. r=jrmuizel

Comment 20

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/35b2ebc973dc
https://hg.mozilla.org/mozilla-central/rev/62401d22f92a
https://hg.mozilla.org/mozilla-central/rev/8c8d66d7b464
https://hg.mozilla.org/mozilla-central/rev/0d1e7b9d2caa