1325227 - Use read locks instead of synchronous transactions for ContentClientRemoteBuffer

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Matt Woodrow (:mattwoodrow)]

Using synchronous transactions sucks, we should instead use our locks so that we only block when we actually want to use the buffer (next paint) instead of when we send it.

Comment 1

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 1: Remove EditReply

We only have a single reply type, and it doesn't add any information we didn't already know on the client side.

Let's get rid of entirely and pave the way for removing synchronous transactions.

Comment 2

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 2: Enable ReadLocks for ContentClient and don't force sync

Spinning waiting on the lock is pretty sad, but I can't think of a better way right now. Ideally we'd have 'real' cross-process locking/monitors.

Comment 3

[Matt Woodrow (:mattwoodrow)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a43851e641c256a9eced26a04413213b8ff1f731

Comment 4

[Nicolas Silva [:nical]]

Comment on attachment 8820921 [details] [diff] [review]
Part 1: Remove EditReply

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

Neat.

::: gfx/layers/composite/CompositableHost.h
@@ +89,5 @@
>  
>    /**
>     * Update the content host.
>     * aUpdated is the region which should be updated
>     * aUpdatedRegionBack is the region in aNewBackResult which has been updated

nit: update the comment since you remove aUdpatedRegionBack

Comment 5

[Nicolas Silva [:nical]]

Comment on attachment 8820924 [details] [diff] [review]
Part 2: Enable ReadLocks for ContentClient and don't force sync

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

r=me with the comments on the spin lock addressed.

::: gfx/layers/client/ContentClient.cpp
@@ +535,5 @@
>  void
>  ContentClientDoubleBuffered::FinalizeFrame(const nsIntRegion& aRegionToDraw)
>  {
>    if (mTextureClient) {
> +    while (mTextureClient->IsReadLocked()) {}

I'd feel better if this made sure the thread goes back to the scheduler every few hundreds of spins of the loop by calling sched_yield and equivalents.

It's worth making this a method of the ReadLock class.

::: gfx/layers/ipc/ShadowLayers.cpp
@@ +433,5 @@
>      MOZ_ASSERT(t.mTextureClient->GetIPDLActor());
>      MOZ_RELEASE_ASSERT(t.mTextureClient->GetIPDLActor()->GetIPCChannel() == mShadowManager->GetIPCChannel());
>      ReadLockDescriptor readLock;
>      t.mTextureClient->SerializeReadLock(readLock);
> +    

nit: trailing spaces

@@ +476,5 @@
>    ReadLockDescriptor readLockW;
>    ReadLockDescriptor readLockB;
>    aTextureOnBlack->SerializeReadLock(readLockB);
>    aTextureOnWhite->SerializeReadLock(readLockW);
> +  

nit: trailing spaces

Comment 6

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 2: Enable ReadLocks for ContentClient and don't force sync v2

Bill, do you have an intuition on how many iterations we should wait on the spin lock before yielding?

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8823941 [details] [diff] [review]
Part 2: Enable ReadLocks for ContentClient and don't force sync v2

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

It's hard to know without knowing how often you plan to take the lock, whether you expect contention, etc. Naively it seems like you could do this a little more nicely with a CrossProcessMutex. Is there a reason you don't want to do that?

Generally, I would guess that you should wait about N cycles if whatever code you're waiting for is expected to hold the lock for N cycles.

Comment 8

[Matt Woodrow (:mattwoodrow)]

(In reply to Bill McCloskey (:billm) from comment #7)
> Comment on attachment 8823941 [details] [diff] [review]
> Part 2: Enable ReadLocks for ContentClient and don't force sync v2
> 
> Review of attachment 8823941 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It's hard to know without knowing how often you plan to take the lock,
> whether you expect contention, etc. Naively it seems like you could do this
> a little more nicely with a CrossProcessMutex. Is there a reason you don't
> want to do that?

Lazyness, mainly. The existing usage of locking within Layers all depend on the non-blocking API provided by the current locks.

I'm feeling more motivated now, so I'll try abstract over the different so that we can optionally use a 'blocking' lock and implement it using CrossProcessMutex.

Comment 9

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 2: Add equality operator to FileDescriptor as it is required by IPDL generated structs.

Comment 10

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 3: Allow child process to share mutex handles with the parent/gpu processes.

Comment 11

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 4: Split out the parts of TextureReadLock that are specific to the 'non-blocking' usage needed by tiling/texture pools.

Comment 12

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 5: Hold the read lock while the TextureClient is locked.

Comment 13

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 6: Implement a blocking TextureReadLock using CrossProcessMutex.

Comment 14

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 7: Use blocking read locks instead of forcing a synchronous transaction when using ContentClientRemoteBuffer.

Carrying r=nical

Comment 15

[Bob Owen (:bobowen)]

Comment on attachment 8825185 [details] [diff] [review]
Part 3: Allow child process to share mutex handles with the parent/gpu processes.

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

This reminds me that the handle duplication machinery has been removed from later versions of the Chromium sandbox, but that is something I'll have to deal with later.

Comment 16

[Nicolas Silva [:nical]]

Comment on attachment 8825187 [details] [diff] [review]
Part 5: Hold the read lock while the TextureClient is locked.

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

I'm not super found of IsReadLocked() and mIsReadLocked meaning totally different things, but otherwise looks good. It's a neat feature.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d4827432cbc2
Part 1: Remove synchronous transaction replies since they don't return any data we didn't already have. r=nical
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a604f87a342
Part 2: Add equality operator to FileDescriptor as it is required by IPDL generated structs. r=billm
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6c6ef5ac42c
Part 3: Allow child process to share mutex handles with the parent/gpu processes. r=bobowen
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5d83d2b1632
Part 4: Split out the parts of TextureReadLock that are specific to the 'non-blocking' usage needed by tiling/texture pools. r=nical
https://hg.mozilla.org/integration/mozilla-inbound/rev/e21219e9638a
Part 5: Hold the read lock while the TextureClient is locked. r=nical
https://hg.mozilla.org/integration/mozilla-inbound/rev/70136ced844e
Part 6: Implement a blocking TextureReadLock using CrossProcessMutex. r=nical
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6e962b025e6
Part 7: Use blocking read locks instead of forcing a synchronous transaction when using ContentClientRemoteBuffer. r=nical

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Sorry, had to back this out to unblock the backout of bug 1323957. Feel free to rebase and re-land once inbound is open.
https://hg.mozilla.org/integration/mozilla-inbound/rev/46d3af166fcea535f960e2e3dde5728bea86f14b

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Actually, don't. It was crashing and asserting all over on Windows.
https://treeherder.mozilla.org/logviewer.html#?job_id=68274330&repo=mozilla-inbound

Comment 20

[Matt Woodrow (:mattwoodrow)]

This failed because we have the requirement that we can take the lock in the content process (when we finish drawing into the buffer), and then have the compositor release the lock when it's finished reading from it.

This isn't compatible with mutexes as they have strict ownership semantics, where the current lock is tied to the thread that took it. It managed to somewhat work as the mutex was recursive, and the content process was just continually taking the same lock over and over again.

I think the best way to solve this is to implement a CrossProcessSemaphore and use that (as a binary semaphore) to implement a mutex that doesn't have any ownership semantics.

I have patches for that and they show some petty decent talos wins. Unfortunately it appears that XPerf records time waiting on a semaphore as 'main thread IO', so we regress that talos test. That seems fine me though, as main-thread IO is just a proxy for performance, and we're winning on the actual graphics perf tests.

Comment 21

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 1: Remove synchronous transaction replies since they don't return any data we didn't already have

MozReview-Commit-ID: 2HIbk65L7zR

Comment 22

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 2: Add equality operator to FileDescriptor as it is required by IPDL generated structs

MozReview-Commit-ID: 9gD2FA1wGii

Comment 23

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 3: Allow child process to share semaphore handles with the parent/gpu processes

MozReview-Commit-ID: 3Q0Wfh6wn2a

Comment 24

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 4: Split out the parts of TextureReadLock that are specific to the 'non-blocking' usage needed by tiling/texture pools

MozReview-Commit-ID: FeuObCnZSyb

Comment 25

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 5: Hold the read lock while the TextureClient is locked

MozReview-Commit-ID: J8x20pWG1e1

Comment 26

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 6: Composite immediately when starting to listen to vsync

MozReview-Commit-ID: 8UUHKtTNFBt

Comment 27

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 7: Allow locking TextureClients in RotatedBuffer to be fallible

MozReview-Commit-ID: 1S1Kb9rnbxF

Comment 28

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 8: Add CrossProcessSemaphore

MozReview-Commit-ID: JPDvLShPHML

Comment 29

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 9: Implement a blocking TextureReadLock using CrossProcessSemaphore

MozReview-Commit-ID: ADCOddNwvUo

Comment 30

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 10: Use blocking read locks instead of forcing a synchronous transaction when using ContentClientRemoteBuffer

MozReview-Commit-ID: 6Y9MEkdc4KX

Comment 31

[Matt Woodrow (:mattwoodrow)]

CC'ing Joel for the expected Talos changes mentioned in comment 20.

https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&originalRevision=af8a2573d0f1&newProject=try&newRevision=930cbd170feb459791ee19b691633c8dc2da0a01&framework=1&showOnlyImportant=0

Comment 32

[Bob Owen (:bobowen)]

Comment on attachment 8836526 [details] [diff] [review]
Part 3: Allow child process to share semaphore handles with the parent/gpu processes

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

r+ with the comment update.

::: security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp
@@ +223,5 @@
>    MOZ_RELEASE_ASSERT(sandbox::SBOX_ALL_OK == result,
>                       "With these static arguments AddRule should never fail, what happened?");
> +
> +  // The content process needs to be able to duplicate mutex handles,
> +  // which are Mutant handles, to the broker process and other child processes.

Needs updating to Semaphore.

Comment 33

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8836531 [details] [diff] [review]
Part 8: Add CrossProcessSemaphore

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

::: ipc/glue/CrossProcessSemaphore.h
@@ +53,5 @@
> +   */
> +  explicit CrossProcessSemaphore(CrossProcessSemaphoreHandle aHandle);
> +
> +  /**
> +   * ~CrossProcessSemaphore

It looks like a lot of the weird comments and other style gunk got copied from CrossProcessMutex.h. Can you clean this up a little?

@@ +65,5 @@
> +   **/
> +  bool Wait(Maybe<TimeDuration> aWaitTime = Nothing());
> +
> +  /**
> +   * Signal

This is one method where it would actually help to have a comment saying that the value is incremented.

@@ +90,5 @@
> +  HANDLE mSemaphore;
> +#elif !defined(OS_NETBSD) && !defined(OS_OPENBSD) && !defined(OS_MACOSX)
> +  RefPtr<mozilla::ipc::SharedMemoryBasic> mSharedBuffer;
> +  sem_t* mSemaphore;
> +  mozilla::Atomic<int32_t>* mCount;

Can you call this mRefCount instead?

::: ipc/glue/CrossProcessSemaphore_posix.cpp
@@ +17,5 @@
> +
> +
> +struct SemaphoreData {
> +  sem_t mSemaphore;
> +  mozilla::Atomic<int32_t> mCount;

Same about mRefCount.

@@ +46,5 @@
> +  }
> +
> +  data->mInitialValue = aInitialValue;
> +  mSemaphore = &(data->mSemaphore);
> +  mCount = &(data->mCount);

No need for parens here.

@@ +81,5 @@
> +    MOZ_CRASH();
> +  }
> +
> +  mSemaphore = &(data->mSemaphore);
> +  mCount = &(data->mCount);

Same about the parens.

@@ +82,5 @@
> +  }
> +
> +  mSemaphore = &(data->mSemaphore);
> +  mCount = &(data->mCount);
> +  int32_t count = (*mCount)++;

Perhaps call this oldCount to emphasize that you're doing a post increment. I got a bit confused at first.

::: ipc/glue/CrossProcessSemaphore_windows.cpp
@@ +40,5 @@
> +}
> +
> +CrossProcessSemaphore::~CrossProcessSemaphore()
> +{
> +  NS_ASSERTION(mSemaphore, "Improper construction of semaphore or double free.");

Can this be MOZ_ASSERT instead?

@@ +48,5 @@
> +
> +bool
> +CrossProcessSemaphore::Wait(Maybe<TimeDuration> aWaitTime)
> +{
> +  NS_ASSERTION(mSemaphore, "Improper construction of semaphore.");

Same.

@@ +58,5 @@
> +
> +void
> +CrossProcessSemaphore::Signal()
> +{
> +  NS_ASSERTION(mSemaphore, "Improper construction of semaphore.");

Same.

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8255c2bb705a
Part 1: Remove synchronous transaction replies since they don't return any data we didn't already have. r=nical
https://hg.mozilla.org/integration/mozilla-inbound/rev/070b40fcd8d1
Part 2: Add equality operator to FileDescriptor as it is required by IPDL generated structs. r=billm
https://hg.mozilla.org/integration/mozilla-inbound/rev/383f86d1c803
Part 3: Allow child process to share semaphore handles with the parent/gpu processes. r=bobowen
https://hg.mozilla.org/integration/mozilla-inbound/rev/695b307d3df2
Part 4: Split out the parts of TextureReadLock that are specific to the 'non-blocking' usage needed by tiling/texture pools. r=nical
https://hg.mozilla.org/integration/mozilla-inbound/rev/97d9e6457494
Part 5: Hold the read lock while the TextureClient is locked. r=nical
https://hg.mozilla.org/integration/mozilla-inbound/rev/21a24c0a7b67
Part 6: Composite immediately when starting to listen to vsync. r=mchang
https://hg.mozilla.org/integration/mozilla-inbound/rev/7337fc1b714d
Part 7: Allow locking TextureClients in RotatedBuffer to be fallible. r=nical
https://hg.mozilla.org/integration/mozilla-inbound/rev/f01503971a81
Part 8: Add CrossProcessSemaphore. r=billm
https://hg.mozilla.org/integration/mozilla-inbound/rev/bcecaff2d5f4
Part 9: Implement a blocking TextureReadLock using CrossProcessSemaphore. r=nical
https://hg.mozilla.org/integration/mozilla-inbound/rev/a19ac5da6082
Part 10: Use blocking read locks instead of forcing a synchronous transaction when using ContentClientRemoteBuffer. r=nical

Comment 35

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8255c2bb705a
https://hg.mozilla.org/mozilla-central/rev/070b40fcd8d1
https://hg.mozilla.org/mozilla-central/rev/383f86d1c803
https://hg.mozilla.org/mozilla-central/rev/695b307d3df2
https://hg.mozilla.org/mozilla-central/rev/97d9e6457494
https://hg.mozilla.org/mozilla-central/rev/21a24c0a7b67
https://hg.mozilla.org/mozilla-central/rev/7337fc1b714d
https://hg.mozilla.org/mozilla-central/rev/f01503971a81
https://hg.mozilla.org/mozilla-central/rev/bcecaff2d5f4
https://hg.mozilla.org/mozilla-central/rev/a19ac5da6082