ShmSegmentsWriter::Write allocates during AddBlobImage

RESOLVED FIXED in Firefox 59

Status

()

P2
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: jrmuizel, Assigned: nical)

Tracking

(Depends on: 1 bug)

unspecified
mozilla59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 unaffected, firefox58 unaffected, firefox59 fixed)

Details

(Whiteboard: [wr-mvp])

Attachments

(5 attachments, 3 obsolete attachments)

(Reporter)

Description

2 years ago
These allocations are showing in profiles.
(Reporter)

Updated

2 years ago
Flags: needinfo?(nical.bugzilla)
(Reporter)

Comment 1

2 years ago
i.e. 40% of our BlobImage recording time is spent allocating shared memory
(Assignee)

Comment 2

2 years ago
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.
Flags: needinfo?(nical.bugzilla)
Whiteboard: [wr-mvp] [triage]
Blocks: 1386665
status-firefox57: --- → unaffected
status-firefox58: --- → unaffected
Priority: -- → P2
Whiteboard: [wr-mvp] [triage] → [wr-mvp]
Hi nical,
Could you tell more about the data pipe thing? Do you already have the discussion log or document for this?
Flags: needinfo?(nical.bugzilla)
(Reporter)

Comment 4

a year ago
(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
(Reporter)

Updated

a year ago
Depends on: 1346446
(Assignee)

Updated

a year ago
Flags: needinfo?(nical.bugzilla)
(Reporter)

Comment 5

a year ago
We may be able to work around this by fixing bug 1422040
Priority: P2 → P3
Whiteboard: [wr-mvp] → [wr-reserve]
(Assignee)

Comment 6

a year ago
It will make sense in another patch.
Assignee: nobody → nical.bugzilla
Attachment #8935409 - Flags: review?(jmuizelaar)
(Assignee)

Comment 7

a year ago
A later patch takes advantage of this.
Attachment #8935411 - Flags: review?(jmuizelaar)
(Assignee)

Comment 9

a year ago
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.
Attachment #8935415 - Flags: review?(jmuizelaar)
Status: NEW → ASSIGNED
Priority: P3 → P1
Whiteboard: [wr-reserve] → [wr-mvp]
(Assignee)

Comment 10

a year ago
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.
(Assignee)

Comment 11

a year ago
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.
(Assignee)

Comment 12

a year ago
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).
Attachment #8935411 - Attachment is obsolete: true
Attachment #8935413 - Attachment is obsolete: true
Attachment #8935415 - Attachment is obsolete: true
Attachment #8935411 - Flags: review?(jmuizelaar)
Attachment #8935413 - Flags: review?(jmuizelaar)
Attachment #8935415 - Flags: review?(jmuizelaar)
Attachment #8941110 - Flags: review?(jmuizelaar)
(Assignee)

Comment 13

a year ago
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.
Attachment #8941113 - Flags: review?(jmuizelaar)
(Assignee)

Comment 14

a year ago
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.
Attachment #8941115 - Flags: review?(jmuizelaar)
(Assignee)

Comment 15

a year ago
Adjust the size so that the total is exactly 64k, otherwise we'll end up allocating an extra page each time.
Attachment #8941120 - Flags: review?(jmuizelaar)
(Reporter)

Updated

a year ago
Attachment #8935409 - Flags: review?(jmuizelaar) → review+
(Reporter)

Updated

a year ago
Attachment #8941110 - Flags: review?(jmuizelaar) → review+
(Reporter)

Comment 16

a year ago
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?
(Reporter)

Updated

a year ago
Attachment #8941113 - Flags: review?(jmuizelaar) → review+
(Reporter)

Comment 17

a year ago
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.
Attachment #8941115 - Flags: review?(jmuizelaar) → review+
(Reporter)

Updated

a year ago
Attachment #8941120 - Flags: review?(jmuizelaar) → review+
(Assignee)

Comment 18

a year ago
(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

a year ago
Pushed by nsilva@mozilla.com:
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

a year ago
bugherder
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
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.