Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2035543003:
cc: Fix for synced property main thread updates with MFBA. (Closed)
Descriptioncc: Fix for synced property main thread updates with MFBA.
Main thread before activation allows sending BeginMainFrames while there
is a pending tree. In this mode pulling deltas from SyncedProperty for
BeginMainFrame should not be incorrect in between commit and activation
of a previous frame.
BUG=616086
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
Committed: https://crrev.com/d2580f5c4fd242f3b0bfe03fbad71e4db5b5fa08
Cr-Commit-Position: refs/heads/master@{#399079}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : test #
Total comments: 3
Patch Set 5 : aborted commit #Patch Set 6 : rebase #
Total comments: 4
Patch Set 7 : logging-- #
Total comments: 2
Patch Set 8 : aelias review #
Total comments: 2
Patch Set 9 : enne's review #Patch Set 10 : nits #
Messages
Total messages: 33 (11 generated)
Description was changed from ========== cc: Fix for synced property main thread updates with MFBA. Main thread before activation allows sending BeginMainFrames while there is a pending tree. In this mode pulling deltas from SyncedProperty for BeginMainFrame should not be incorrect in between commit and activation of a previous frame. R=enne@chromium.org BUG=616086 ========== to ========== cc: Fix for synced property main thread updates with MFBA. Main thread before activation allows sending BeginMainFrames while there is a pending tree. In this mode pulling deltas from SyncedProperty for BeginMainFrame should not be incorrect in between commit and activation of a previous frame. R=enne@chromium.org BUG=616086 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
PTAL I've verified that the test fails without my CL.
enne@chromium.org changed reviewers: + aelias@chromium.org
I need to digest this a little bit more, but I would feel more confident if you added two more tests for edge cases: * the second commit during MFBA is aborted * the second commit during MFBA clobbers scroll positions
https://codereview.chromium.org/2035543003/diff/60001/cc/base/synced_property.h File cc/base/synced_property.h (right): https://codereview.chromium.org/2035543003/diff/60001/cc/base/synced_property... cc/base/synced_property.h:111: T PendingDelta() const { It's really confusing that this method doesn't return the value of pending_delta_ -- it needs a different name. I see this is now the return value of PullDeltaForMainThread(), so how about calling this DeltaForMainThread()? https://codereview.chromium.org/2035543003/diff/60001/cc/base/synced_property... cc/base/synced_property.h:131: T sent_delta_; I'm convinced there is a way to express this behavior without tracking more than four T objects, and I think it's important not to add any more to keep this things simple to reason about. For brian's original patch, I had proposed a way to delete one, but that suggestion no longer really applies to how you're doing it now. Can we now delete sent_delta_? I *think* it's equivalent to simply call DeltaForMainThread() again in its place in both of the spots where it's used.
I think I have the aborted commit test almost nailed down. I had a question though: if a commit is aborted before activation of the previous commit, should we update active base and active delta? In other words should an aborted commit only show its effects after activation (provided activation is pending)? https://codereview.chromium.org/2035543003/diff/60001/cc/base/synced_property.h File cc/base/synced_property.h (right): https://codereview.chromium.org/2035543003/diff/60001/cc/base/synced_property... cc/base/synced_property.h:131: T sent_delta_; On 2016/06/07 23:52:26, aelias wrote: > I'm convinced there is a way to express this behavior without tracking more than > four T objects, and I think it's important not to add any more to keep this > things simple to reason about. For brian's original patch, I had proposed a way > to delete one, but that suggestion no longer really applies to how you're doing > it now. > > Can we now delete sent_delta_? I *think* it's equivalent to simply call > DeltaForMainThread() again in its place in both of the spots where it's used. I don't think we can remove sent_delta_. I think the fundamental problem is that we need to know what the last value sent to the main thread is (sent_delta_) and also what delta the main thread was sent for the pending tree. We can calculate the second value by either keeping track of it like in this patch or by accumulating the total delta the main thread has seen and subtracting sent_delta_ from that. I also think we need a has_pending_tree_ variable so that aborted commits affect pending_base_ if activation is pending and active_base_ otherwise.
I'm not 100% sure what to do about the aborted commit. An aborted commit usually just means "the main thread didn't have anything to update, so push any sent deltas into the base" but that makes less sense when there's an outstanding pending tree as well. There's no visible effects, other than just bookkeeping inside of synced property. The active tree has already scrolled and the main thread acked it, but that's it. So, I think you have to update the active base and delta, because if there's another main frame after the aborted commit, you don't want to re-send the same delta. (It'd be good to make sure the test has two aborted commits to verify this.) I think the current value on the pending tree also needs to reflect this value; that way it's as if the scroll happened before any updates from the main thread.
Added a test for aborted commits + MFBA with two aborted commits in succession. Also modified the existing scrolling test to make the second commit during MFBA to clobber scroll positions. I've verified that the two tests fail without my patch. PTAL
OK thanks, approach looks good. The rename made it much clearer to me what the new field is for and why it can't be avoided. https://codereview.chromium.org/2035543003/diff/100001/cc/base/synced_property.h File cc/base/synced_property.h (right): https://codereview.chromium.org/2035543003/diff/100001/cc/base/synced_propert... cc/base/synced_property.h:128: T sent_delta_; How about switching the names to: "reflected_delta_in_main_tree_" and "reflected_delta_in_pending_tree_"? I think this would better express that these are tracking duplicated quantities for the purpose of avoiding double-application. https://codereview.chromium.org/2035543003/diff/100001/cc/base/synced_propert... cc/base/synced_property.h:130: // pending tree. Please add comment: "This is always identity outside of the commit-to-activation interval."
https://codereview.chromium.org/2035543003/diff/120001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_unittest_scroll.cc (right): https://codereview.chromium.org/2035543003/diff/120001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_unittest_scroll.cc:1760: impl->BlockNotifyReadyToActivateForTesting(false); Is this racy? It seems like this means that the scroll commit activation and the aborted commit are potentially running against each other. Right now it looks like the activation occurs before the second commit aborts? If that's the case, then I think a better testing structure would be this: * initial commit + activation * scroll change commit + hold activation * aborted commit * aborted commit (+ unblock activation) * scroll change activation (check scroll positions, end tests after draw here) That way you always test that two aborted commits have the right scroll positions, and that the pending tree has the correct value after it finally activates as well.
PTAL https://codereview.chromium.org/2035543003/diff/100001/cc/base/synced_property.h File cc/base/synced_property.h (right): https://codereview.chromium.org/2035543003/diff/100001/cc/base/synced_propert... cc/base/synced_property.h:128: T sent_delta_; On 2016/06/09 00:46:13, aelias wrote: > How about switching the names to: "reflected_delta_in_main_tree_" and > "reflected_delta_in_pending_tree_"? I think this would better express that > these are tracking duplicated quantities for the purpose of avoiding > double-application. Done. https://codereview.chromium.org/2035543003/diff/100001/cc/base/synced_propert... cc/base/synced_property.h:130: // pending tree. On 2016/06/09 00:46:13, aelias wrote: > Please add comment: "This is always identity outside of the commit-to-activation > interval." Done. Also changed some of the other comments. https://codereview.chromium.org/2035543003/diff/120001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_unittest_scroll.cc (right): https://codereview.chromium.org/2035543003/diff/120001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_unittest_scroll.cc:1760: impl->BlockNotifyReadyToActivateForTesting(false); On 2016/06/09 00:57:44, enne wrote: > Is this racy? It seems like this means that the scroll commit activation and the > aborted commit are potentially running against each other. Right now it looks > like the activation occurs before the second commit aborts? > > If that's the case, then I think a better testing structure would be this: > * initial commit + activation > * scroll change commit + hold activation > * aborted commit > * aborted commit (+ unblock activation) > * scroll change activation (check scroll positions, end tests after draw here) > > That way you always test that two aborted commits have the right scroll > positions, and that the pending tree has the correct value after it finally > activates as well. The reason for blocking NotifyReadyToActivate is to ensure the second commit to be sent while the previous commit activation is pending. Activation is fundamentally racy with BeginMainFrame, right? I.e. if we activate in the same frame as commit, we'll do a non-MFBA BeginMainFrame but if activation gets delayed beyond the frame boundary then we'll do an MFBA BeginMainFrame. Blocking NotifyReadyToActivate is the only safe solution here. I like what I have here (i.e. first aborted commit unblocks activation) better because we're testing both when activation is pending (first aborted commit) and when it's not (second aborted commit).
Hmm. There's already tests for aborting commits when activation is not pending. Thinking about it, the check that the scroll delta is zero after the activation covers the case I was worried about. Sorry for the confusion. I think that's ok. Re: raciness. I don't think there has to fundmentally be anything racy in tests, because you can add locks and unblock things at particular times. I think your NOTREACHED would catch this case, but it might be better to not set needs commit when you unblock activation and then set needs commit after the activation, if that's the ordering that you want to enforce.
On 2016/06/09 17:28:52, enne wrote: > Hmm. There's already tests for aborting commits when activation is not pending. > Thinking about it, the check that the scroll delta is zero after the activation > covers the case I was worried about. Sorry for the confusion. I think that's > ok. > > Re: raciness. I don't think there has to fundmentally be anything racy in > tests, because you can add locks and unblock things at particular times. I > think your NOTREACHED would catch this case, but it might be better to not set > needs commit when you unblock activation and then set needs commit after the > activation, if that's the ordering that you want to enforce. PTAL I misunderstood your concern re: the race between draw and begin main frame. I have fixed the test now so that SetNeedsCommit happens in the draw in those cases and made a similar change in the simple scrolling MFBA test. I also removed the NOTREACHEDs because verifying the number of draws, commits, etc. should be sufficient.
lgtm, thanks!
Thanks! aelias@ Could you also take a final look at this please?
lgtm, thanks! https://codereview.chromium.org/2035543003/diff/140001/cc/base/synced_property.h File cc/base/synced_property.h (right): https://codereview.chromium.org/2035543003/diff/140001/cc/base/synced_propert... cc/base/synced_property.h:128: // pending tree. This is always identity outside of the BeginFrame to I think this comment line could more precisely say "commit", not "BeginFrame" right?
Description was changed from ========== cc: Fix for synced property main thread updates with MFBA. Main thread before activation allows sending BeginMainFrames while there is a pending tree. In this mode pulling deltas from SyncedProperty for BeginMainFrame should not be incorrect in between commit and activation of a previous frame. R=enne@chromium.org BUG=616086 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== cc: Fix for synced property main thread updates with MFBA. Main thread before activation allows sending BeginMainFrames while there is a pending tree. In this mode pulling deltas from SyncedProperty for BeginMainFrame should not be incorrect in between commit and activation of a previous frame. R=aelias@chromium.org,enne@chromium.org BUG=616086 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
Description was changed from ========== cc: Fix for synced property main thread updates with MFBA. Main thread before activation allows sending BeginMainFrames while there is a pending tree. In this mode pulling deltas from SyncedProperty for BeginMainFrame should not be incorrect in between commit and activation of a previous frame. R=aelias@chromium.org,enne@chromium.org BUG=616086 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== cc: Fix for synced property main thread updates with MFBA. Main thread before activation allows sending BeginMainFrames while there is a pending tree. In this mode pulling deltas from SyncedProperty for BeginMainFrame should not be incorrect in between commit and activation of a previous frame. BUG=616086 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
Thanks! https://codereview.chromium.org/2035543003/diff/140001/cc/base/synced_property.h File cc/base/synced_property.h (right): https://codereview.chromium.org/2035543003/diff/140001/cc/base/synced_propert... cc/base/synced_property.h:128: // pending tree. This is always identity outside of the BeginFrame to On 2016/06/09 22:40:46, aelias wrote: > I think this comment line could more precisely say "commit", not "BeginFrame" > right? Changed this to BeginMainFrame which is what we use elsewhere.
The CQ bit was checked by sunnyps@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org, enne@chromium.org Link to the patchset: https://codereview.chromium.org/2035543003/#ps180001 (title: "nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2035543003/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by sunnyps@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2035543003/180001
Message was sent while issue was closed.
Description was changed from ========== cc: Fix for synced property main thread updates with MFBA. Main thread before activation allows sending BeginMainFrames while there is a pending tree. In this mode pulling deltas from SyncedProperty for BeginMainFrame should not be incorrect in between commit and activation of a previous frame. BUG=616086 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== cc: Fix for synced property main thread updates with MFBA. Main thread before activation allows sending BeginMainFrames while there is a pending tree. In this mode pulling deltas from SyncedProperty for BeginMainFrame should not be incorrect in between commit and activation of a previous frame. BUG=616086 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
CQ bit was unchecked
Message was sent while issue was closed.
Description was changed from ========== cc: Fix for synced property main thread updates with MFBA. Main thread before activation allows sending BeginMainFrames while there is a pending tree. In this mode pulling deltas from SyncedProperty for BeginMainFrame should not be incorrect in between commit and activation of a previous frame. BUG=616086 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== cc: Fix for synced property main thread updates with MFBA. Main thread before activation allows sending BeginMainFrames while there is a pending tree. In this mode pulling deltas from SyncedProperty for BeginMainFrame should not be incorrect in between commit and activation of a previous frame. BUG=616086 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel Committed: https://crrev.com/d2580f5c4fd242f3b0bfe03fbad71e4db5b5fa08 Cr-Commit-Position: refs/heads/master@{#399079} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/d2580f5c4fd242f3b0bfe03fbad71e4db5b5fa08 Cr-Commit-Position: refs/heads/master@{#399079}
Message was sent while issue was closed.
emma.tierney.louise@gmail.com changed reviewers: + Emma.Tierney.Louise@gmail.com
Message was sent while issue was closed.
hi |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
