1340067 - Change DPBegin() to async

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect)

Description

[Sotaro Ikeda [:sotaro]]

It seems OK to change DPBegin() to async.

Comment 1

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Change DPBegin() to async

Comment 2

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a2dddc4b5a0231bd16197cd02721e345aa62889&selectedJob=77863942

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8837951 [details] [diff] [review]
patch - Change DPBegin() to async

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

What is the advantage of this patch? It's not clear to me why it's needed. And it seems like the success true/false return value is used right now, so I'm hesitant to just take it out without justification.

::: gfx/layers/wr/WebRenderBridgeParent.cpp
@@ -189,3 @@
>  {
>    if (mDestroyed) {
>      return IPC_OK();

Doesn't this codepath return success=false?

Comment 4

[Sotaro Ikeda [:sotaro]]

> What is the advantage of this patch?

The patch avoids sync ipc of DPBegin(). It could slow down client side's main thread.

> And it seems like the success true/false return value is used right now,
> so I'm hesitant to just take it out without justification.

mDestroyed check in DPBegin() actually do not work always, since there could be a race condition until HandleDPEnd(). And all WebRenderBridgeParent::Recv**() functions have mDestroyed check, then receiving ipc message since mDestroyed = true is not a problem now. Further, if mDestroyed = true, that WebRenderBridgeParent is going to be destroyed soon.

From the above, DPBegin() do not need to return success and could be async.

Comment 5

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8837951 [details] [diff] [review]
patch - Change DPBegin() to async

:kats, can you review it again based on comment 4?

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8837951 [details] [diff] [review]
patch - Change DPBegin() to async

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

OK, that makes sense, thanks!

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/projects/graphics/rev/ae5c9d27cc3c
Change DPBegin() to async r=kats

Comment 8

[Kartikaya Gupta (email:[e-mail])]

It looks like this caused a few intermittent-ish QR reftest failures: https://treeherder.mozilla.org/#/jobs?repo=graphics&fromchange=687b3bd601ba93cf746f2ef1cd3af2c60f7d2128&group_state=expanded&filter-searchStr=quantum%20reftest%20e10s%20opt&tochange=ae5c9d27cc3cb7e309b9b21d12a4c338a5de82a0

I'm going to back it out for now, sorry.

Comment 9

[Kartikaya Gupta (email:[e-mail])]

Backed out:

https://hg.mozilla.org/projects/graphics/rev/f6a20aef8d1066dee2a351c069a6bf84355d8d72

Comment 10

[Sotaro Ikeda [:sotaro]]

It seems to hit RenderThread's existing problem.

Comment 11

[Sotaro Ikeda [:sotaro]]

tryserver result becomes good now. Bug 1337885 seemed to affect to the failures.
 https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a94c56dc8e782afa0c735b7ddc8610171652608

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/projects/graphics/rev/31f1deaef697
Change DPBegin() to async r=kats

Comment 13

[Kartikaya Gupta (email:[e-mail])]

(In reply to Sotaro Ikeda [:sotaro] from comment #11)
> tryserver result becomes good now. Bug 1337885 seemed to affect to the
> failures.

That seems surprising since your patch originally landed before 1337885. But ok, it seems green now.

Comment 14

[Kartikaya Gupta (email:[e-mail])]

Attachment: Delist now async message from sync-messages

I'm doing another merge from m-c, which picked up the sync-messages changes and caused a build error. We need to delist this message from sync-messages and have it IPC-peer-reviewed.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/projects/graphics/rev/e4e9dcee9683
Follow-up to delist a formerly sync message that is now async. r=dvander?

Comment 16

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

https://hg.mozilla.org/mozilla-central/rev/31f1deaef697
https://hg.mozilla.org/mozilla-central/rev/e4e9dcee9683