Closed
Bug 1340067
Opened 7 years ago
Closed 7 years ago
Change DPBegin() to async
Categories
(Core :: Graphics: WebRender, defect)
Core
Graphics: WebRender
Tracking
()
RESOLVED
FIXED
mozilla54
People
(Reporter: sotaro, Assigned: sotaro)
Details
Attachments
(2 files)
4.60 KB,
patch
|
kats
:
review+
|
Details | Diff | Splinter Review |
820 bytes,
patch
|
dvander
:
review+
|
Details | Diff | Splinter Review |
It seems OK to change DPBegin() to async.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → sotaro.ikeda.g
Assignee | ||
Comment 1•7 years ago
|
||
Assignee | ||
Comment 2•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a2dddc4b5a0231bd16197cd02721e345aa62889&selectedJob=77863942
Assignee | ||
Updated•7 years ago
|
Attachment #8837951 -
Flags: review?(bugmail)
Comment 3•7 years ago
|
||
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?
Attachment #8837951 -
Flags: review?(bugmail)
Assignee | ||
Comment 4•7 years ago
|
||
> 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.
Assignee | ||
Comment 5•7 years ago
|
||
Comment on attachment 8837951 [details] [diff] [review] patch - Change DPBegin() to async :kats, can you review it again based on comment 4?
Attachment #8837951 -
Flags: review?(bugmail)
Comment 6•7 years ago
|
||
Comment on attachment 8837951 [details] [diff] [review] patch - Change DPBegin() to async Review of attachment 8837951 [details] [diff] [review]: ----------------------------------------------------------------- OK, that makes sense, thanks!
Attachment #8837951 -
Flags: review?(bugmail) → review+
Pushed by sikeda@mozilla.com: https://hg.mozilla.org/projects/graphics/rev/ae5c9d27cc3c Change DPBegin() to async r=kats
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Comment 8•7 years ago
|
||
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•7 years ago
|
||
Backed out: https://hg.mozilla.org/projects/graphics/rev/f6a20aef8d1066dee2a351c069a6bf84355d8d72
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 10•7 years ago
|
||
It seems to hit RenderThread's existing problem.
Assignee | ||
Comment 11•7 years ago
|
||
tryserver result becomes good now. Bug 1337885 seemed to affect to the failures. https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a94c56dc8e782afa0c735b7ddc8610171652608
Comment 12•7 years ago
|
||
Pushed by sikeda@mozilla.com: https://hg.mozilla.org/projects/graphics/rev/31f1deaef697 Change DPBegin() to async r=kats
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
Resolution: --- → FIXED
Comment 13•7 years ago
|
||
(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•7 years ago
|
||
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.
Attachment #8839471 -
Flags: review?(dvander)
Comment 15•7 years ago
|
||
Pushed by kgupta@mozilla.com: https://hg.mozilla.org/projects/graphics/rev/e4e9dcee9683 Follow-up to delist a formerly sync message that is now async. r=dvander?
Attachment #8839471 -
Flags: review?(dvander) → review+
Updated•7 years ago
|
Target Milestone: --- → mozilla54
Comment 16•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/31f1deaef697 https://hg.mozilla.org/mozilla-central/rev/e4e9dcee9683
You need to log in
before you can comment on or make changes to this bug.
Description
•