Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2873313004:
Harmonize LayerTreeHost/LayerTreeHostImpl synchronization steps (Closed)
DescriptionHarmonize LayerTreeHost/LayerTreeHostImpl synchronization steps
Both LayerTreeHost::FinishCommitOnImpl and LayerTreeHostImpl::ActivateSyncTree
follow these rough steps to synchronize with the LayerTreeHostImpl:
1) Update property trees.
2) Update layer properties, possibly with side effects.
3) Push layer tree host properties (e.g., viewport types and page scale factor).
These steps have implicit dependencies: layer property updates may require
property trees, and the viewport layer type update may require layer properties.
Before this patch, LayerTreeHost and LayerTreeHostImpl followed these steps
in different orders. This patch refactors the synchronization code to use
the same ordering, and adds DCHECKs to enforce dependencies.
BUG=693740
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Review-Url: https://codereview.chromium.org/2873313004
Cr-Commit-Position: refs/heads/master@{#472004}
Committed: https://chromium.googlesource.com/chromium/src/+/80ecb98035a6654fff02e86877be39f51fcc3d54
Patch Set 1 #Patch Set 2 : More harmonious #
Total comments: 23
Patch Set 3 : harmonize with reviewers #Patch Set 4 : Improve comments #Patch Set 5 : Fix merge conflict with https://codereview.chromium.org/2877033002 #
Total comments: 1
Patch Set 6 : Improve reviewability #
Total comments: 3
Patch Set 7 : Remove redundant codes #Patch Set 8 : Ace of rebase #
Messages
Total messages: 51 (29 generated)
The CQ bit was checked by pdr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_trusty_blink_rel on master.tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...)
pdr@chromium.org changed reviewers: + chrishtr@chromium.org, enne@chromium.org, wkorman@chromium.org
pdr@chromium.org changed reviewers: + jaydasika@chromium.org
Excited about this overall, but have a bunch of cleanup thoughts: https://codereview.chromium.org/2873313004/diff/20001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.h (right): https://codereview.chromium.org/2873313004/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.h:141: NOT_SYNCING = 0, Don't need value initialization here. This is already the values they get. https://codereview.chromium.org/2873313004/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.h:621: #if DCHECK_IS_ON() There's an awful lot of #ifdefs here. I get that you're trying to be conservative and not have a perf impact from your change, but I think there's some overhead to #ifdefs in terms of development friction or landing things that break in one mode but not another (in both directions). Is there a way to minimize the amount of #ifdefs here or remove them entirely? What about just not #ifdefing anything, and maybe having SyncStateComplete exist but return true always in release? I'm ok with having sync state exist and advance in release as well. https://codereview.chromium.org/2873313004/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.h:625: // true if the state transition is allowed, and returns false otherwise. I think this would be cleaner if the function didn't return bool, and instead DCHECKed that state is the state + 1 or is NOT_SYNCING when LAST_SYNC_STATE. https://codereview.chromium.org/2873313004/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.h:912: SyncState sync_state_; Hmmmmmm. I'm super surprised to see this on LayerTreeHostImpl and not LayerTreeImpl. Sure, I realize that there's only one compositor thread and one pending tree and so you can't be committing and activating at the same time. But, if you put it on LayerTreeImpl, then you can DCHECK in the accessors themselves instead of making callers remember to do it. Also, initialize in headers instead of the ctor implementation if you can. https://codereview.chromium.org/2873313004/diff/20001/cc/trees/layer_tree_imp... File cc/trees/layer_tree_impl.cc (right): https://codereview.chromium.org/2873313004/diff/20001/cc/trees/layer_tree_imp... cc/trees/layer_tree_impl.cc:1008: // TODO(pdr): This DCHECK fails on existing tests but should be enabled. This is a pre-existing data inconsistency, I guess? https://codereview.chromium.org/2873313004/diff/20001/cc/trees/layer_tree_impl.h File cc/trees/layer_tree_impl.h (right): https://codereview.chromium.org/2873313004/diff/20001/cc/trees/layer_tree_imp... cc/trees/layer_tree_impl.h:140: return layer_tree_host_impl_->SyncStateComplete(SYNCED_PROPERTY_TREES); Is it possible to just have this hidden inside of the property_trees() LayerTreeImpl::property_trees accessor?
https://codereview.chromium.org/2873313004/diff/20001/cc/trees/layer_tree_imp... File cc/trees/layer_tree_impl.cc (right): https://codereview.chromium.org/2873313004/diff/20001/cc/trees/layer_tree_imp... cc/trees/layer_tree_impl.cc:206: // DCHECK(layer_tree_host_impl_->SyncStateComplete(SYNCED_LAYER_PROPERTIES)); Something related I just noticed : LayerImpl::SetBounds calls this function and bounds are set on a layer before the scroll_clip_layer_id is set in Layer/LayerImpl::PushPropertiesTo => not just the scroll_clip_layer properties, even the scroll_clip_layer_id is not up to date, so we might be fetching the wrong layer here.
On 2017/05/12 at 17:25:07, enne wrote: > Excited about this overall, but have a bunch of cleanup thoughts: Thanks, this review is super helpful. I'll address the code stuff shortly. > https://codereview.chromium.org/2873313004/diff/20001/cc/trees/layer_tree_hos... > cc/trees/layer_tree_host_impl.h:912: SyncState sync_state_; > Hmmmmmm. I'm super surprised to see this on LayerTreeHostImpl and not LayerTreeImpl. Sure, I realize that there's only one compositor thread and one pending tree and so you can't be committing and activating at the same time. But, if you put it on LayerTreeImpl, then you can DCHECK in the accessors themselves instead of making callers remember to do it. > > Also, initialize in headers instead of the ctor implementation if you can. +1, LayerTreeImpl makes a lot of sense. About putting these on accessors... Example: LayerTreeImpl { PropertyTrees* property_trees() { DCHECK(SyncStateComplete(SYNCED_PROPERTY_TREES)); return &property_trees_; } } I tried this but we access the property trees a lot during the property tree sync itself. I had added a RAII suppressor class to suppress these DCHECKs but the code started getting too crazy. For example, TreeSynchronizer::SynchronizeTrees legitimately accesses the trees during the sync. There are many examples that are not legitimate. I think it will be better to approach this from the other end, where we slowly add DCHECKs when we find violations. Definitely a worse design overall but it keeps this patch to a manageable size.
I'm fine with your original approach here, but I'd like to move towards putting them on accessors. I think when doing the property tree sync itself, maybe it doesn't have to be asked for from the tree, or maybe once set, then the state transition could happen. I still believe that it should end up in the accessor, but I can understand being pragmatic here. I think your patch is still a harmonious step forward.
https://codereview.chromium.org/2873313004/diff/20001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host.cc (right): https://codereview.chromium.org/2873313004/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host.cc:332: PushPropertiesTo(sync_tree); How about renaming this to PushLayerTreeHostPropertiesTo, and also making a PushLayerTreePropertiesTo for things on the tree_impl. (sync_tree). https://codereview.chromium.org/2873313004/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host.cc:337: host_impl->SetHasGpuRasterizationTrigger(has_gpu_rasterization_trigger_); Put this inside of PushPropertiesTo? https://codereview.chromium.org/2873313004/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host.cc:1147: void LayerTreeHost::PushPropertyTreesTo(LayerTreeImpl* tree_impl) { Inline this, or push change tracking down into this method? https://codereview.chromium.org/2873313004/diff/20001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.h (right): https://codereview.chromium.org/2873313004/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.h:628: if (state > sync_state_ || resetting) { Is it ever ok to skip one of the states in between two others? https://codereview.chromium.org/2873313004/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.h:636: bool SyncStateComplete(SyncState state) { Also, it woudl be great if this could go in a CompositorThreadLifecycle class modeled on the same design pattern as DocumentLifecycle, if the cc owners agree. https://codereview.chromium.org/2873313004/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.h:912: SyncState sync_state_; On 2017/05/12 at 17:25:06, enne wrote: > Hmmmmmm. I'm super surprised to see this on LayerTreeHostImpl and not LayerTreeImpl. Sure, I realize that there's only one compositor thread and one pending tree and so you can't be committing and activating at the same time. But, if you put it on LayerTreeImpl, then you can DCHECK in the accessors themselves instead of making callers remember to do it. > > Also, initialize in headers instead of the ctor implementation if you can. Perhaps there should be two of these, one for each thread. Each thread has its own lifecycle. https://codereview.chromium.org/2873313004/diff/20001/cc/trees/layer_tree_imp... File cc/trees/layer_tree_impl.cc (right): https://codereview.chromium.org/2873313004/diff/20001/cc/trees/layer_tree_imp... cc/trees/layer_tree_impl.cc:405: target_tree->SetCurrentlyScrollingNode(scrolling_node); "just noting" --wkorman It seems perhaps this field should go on the LayerTreeImpl, so that it doesn't get clobbered during the copy constructor.
The CQ bit was checked by pdr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
The CQ bit was checked by pdr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL! https://codereview.chromium.org/2873313004/diff/20001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host.cc (right): https://codereview.chromium.org/2873313004/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host.cc:332: PushPropertiesTo(sync_tree); On 2017/05/12 at 23:13:29, chrishtr wrote: > How about renaming this to PushLayerTreeHostPropertiesTo, and also > making a PushLayerTreePropertiesTo for things on the tree_impl. (sync_tree). Done, we now do: PushLayerTreePropertiesTo(sync_tree); PushLayerTreeHostPropertiesTo(host_impl); https://codereview.chromium.org/2873313004/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host.cc:337: host_impl->SetHasGpuRasterizationTrigger(has_gpu_rasterization_trigger_); On 2017/05/12 at 23:13:29, chrishtr wrote: > Put this inside of PushPropertiesTo? I put this in PushLayerTreeHostPropertiesTo. I think that's what you meant? https://codereview.chromium.org/2873313004/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host.cc:1147: void LayerTreeHost::PushPropertyTreesTo(LayerTreeImpl* tree_impl) { On 2017/05/12 at 23:13:29, chrishtr wrote: > Inline this, or push change tracking down into this method? Moved change tracking down here. For consistency, I did the same in for the LayerTreeImpl and put the damage tracking in LayerTreeImpl::PushPropertyTreesTo. https://codereview.chromium.org/2873313004/diff/20001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.h (right): https://codereview.chromium.org/2873313004/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.h:621: #if DCHECK_IS_ON() On 2017/05/12 at 17:25:06, enne wrote: > There's an awful lot of #ifdefs here. I get that you're trying to be conservative and not have a perf impact from your change, but I think there's some overhead to #ifdefs in terms of development friction or landing things that break in one mode but not another (in both directions). > > Is there a way to minimize the amount of #ifdefs here or remove them entirely? What about just not #ifdefing anything, and maybe having SyncStateComplete exist but return true always in release? > > I'm ok with having sync state exist and advance in release as well. I totes agree with this. Done. (In blink we've been more hard-line about not letting debug code in release, but I think that's wrong for cases like this one). https://codereview.chromium.org/2873313004/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.h:636: bool SyncStateComplete(SyncState state) { On 2017/05/12 at 23:13:29, chrishtr wrote: > Also, it woudl be great if this could go in a CompositorThreadLifecycle class > modeled on the same design pattern as DocumentLifecycle, if the cc > owners agree. I've rewritten this code as a LayerTreeLifecycle class on LayerTreeImpl. Enne and Chris, PTAL. https://codereview.chromium.org/2873313004/diff/20001/cc/trees/layer_tree_imp... File cc/trees/layer_tree_impl.cc (right): https://codereview.chromium.org/2873313004/diff/20001/cc/trees/layer_tree_imp... cc/trees/layer_tree_impl.cc:206: // DCHECK(layer_tree_host_impl_->SyncStateComplete(SYNCED_LAYER_PROPERTIES)); On 2017/05/12 at 18:19:16, jaydasika wrote: > Something related I just noticed : LayerImpl::SetBounds calls this function and bounds are set on a layer before the scroll_clip_layer_id is set in Layer/LayerImpl::PushPropertiesTo => > not just the scroll_clip_layer properties, even the scroll_clip_layer_id is not up to date, so we might be fetching the wrong layer here. +1, this bug is all over :( https://codereview.chromium.org/2873313004/diff/20001/cc/trees/layer_tree_imp... cc/trees/layer_tree_impl.cc:1008: // TODO(pdr): This DCHECK fails on existing tests but should be enabled. On 2017/05/12 at 17:25:06, enne wrote: > This is a pre-existing data inconsistency, I guess? This is due to how https://codereview.chromium.org/2867793002 updates the viewport layer types more than needed (during layer prop sync and host prop sync) because of the ordering bug that's fixed in this patch. I've gone ahead and enabled this DCHECK by removing the call to UpdateViewportLayerTypes during Layer tree property syncing. https://codereview.chromium.org/2873313004/diff/20001/cc/trees/layer_tree_impl.h File cc/trees/layer_tree_impl.h (right): https://codereview.chromium.org/2873313004/diff/20001/cc/trees/layer_tree_imp... cc/trees/layer_tree_impl.h:140: return layer_tree_host_impl_->SyncStateComplete(SYNCED_PROPERTY_TREES); On 2017/05/12 at 17:25:06, enne wrote: > Is it possible to just have this hidden inside of the property_trees() LayerTreeImpl::property_trees accessor? I've added a TODO comment here that we want to do this.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
https://codereview.chromium.org/2873313004/diff/80001/cc/layers/layer_impl.cc File cc/layers/layer_impl.cc (left): https://codereview.chromium.org/2873313004/diff/80001/cc/layers/layer_impl.cc... cc/layers/layer_impl.cc:284: // Ensure our viewport layer type is updated. Per offline, let's pull this out.
https://codereview.chromium.org/2873313004/diff/20001/cc/trees/layer_tree_imp... File cc/trees/layer_tree_impl.cc (right): https://codereview.chromium.org/2873313004/diff/20001/cc/trees/layer_tree_imp... cc/trees/layer_tree_impl.cc:1008: // TODO(pdr): This DCHECK fails on existing tests but should be enabled. On 2017/05/15 at 18:30:24, pdr. wrote: > On 2017/05/12 at 17:25:06, enne wrote: > > This is a pre-existing data inconsistency, I guess? > > This is due to how https://codereview.chromium.org/2867793002 updates the viewport layer types more than needed (during layer prop sync and host prop sync) because of the ordering bug that's fixed in this patch. I've gone ahead and enabled this DCHECK by removing the call to UpdateViewportLayerTypes during Layer tree property syncing. I've reverted this change and will do it in a followup. Patchset 5 shows the design and how we can enable this DCHECK.
Looking good to me. I will wait for Enne's review of the lifecycle pattern.
The CQ bit was checked by pdr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2873313004/diff/100001/cc/trees/layer_tree_im... File cc/trees/layer_tree_impl.cc (right): https://codereview.chromium.org/2873313004/diff/100001/cc/trees/layer_tree_im... cc/trees/layer_tree_impl.cc:64: state_ = next_state; Redundant? https://codereview.chromium.org/2873313004/diff/100001/cc/trees/layer_tree_im... cc/trees/layer_tree_impl.cc:410: if (target_tree->property_trees()->changed) { I like this cleanup to move this here into LayerTreeImpl too.
https://codereview.chromium.org/2873313004/diff/100001/cc/trees/layer_tree_im... File cc/trees/layer_tree_impl.cc (right): https://codereview.chromium.org/2873313004/diff/100001/cc/trees/layer_tree_im... cc/trees/layer_tree_impl.cc:64: state_ = next_state; On 2017/05/15 at 20:48:16, enne wrote: > Redundant? Can never be too safe.
The CQ bit was checked by pdr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from enne@chromium.org Link to the patchset: https://codereview.chromium.org/2873313004/#ps120001 (title: "Remove redundant codes")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by pdr@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by pdr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from enne@chromium.org Link to the patchset: https://codereview.chromium.org/2873313004/#ps140001 (title: "Ace of rebase")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by pdr@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by pdr@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1494901666921940,
"parent_rev": "d320fe5673b8ef1969d6b2e91a0116282143c836", "commit_rev":
"80ecb98035a6654fff02e86877be39f51fcc3d54"}
Message was sent while issue was closed.
Description was changed from ========== Harmonize LayerTreeHost/LayerTreeHostImpl synchronization steps Both LayerTreeHost::FinishCommitOnImpl and LayerTreeHostImpl::ActivateSyncTree follow these rough steps to synchronize with the LayerTreeHostImpl: 1) Update property trees. 2) Update layer properties, possibly with side effects. 3) Push layer tree host properties (e.g., viewport types and page scale factor). These steps have implicit dependencies: layer property updates may require property trees, and the viewport layer type update may require layer properties. Before this patch, LayerTreeHost and LayerTreeHostImpl followed these steps in different orders. This patch refactors the synchronization code to use the same ordering, and adds DCHECKs to enforce dependencies. BUG=693740 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Harmonize LayerTreeHost/LayerTreeHostImpl synchronization steps Both LayerTreeHost::FinishCommitOnImpl and LayerTreeHostImpl::ActivateSyncTree follow these rough steps to synchronize with the LayerTreeHostImpl: 1) Update property trees. 2) Update layer properties, possibly with side effects. 3) Push layer tree host properties (e.g., viewport types and page scale factor). These steps have implicit dependencies: layer property updates may require property trees, and the viewport layer type update may require layer properties. Before this patch, LayerTreeHost and LayerTreeHostImpl followed these steps in different orders. This patch refactors the synchronization code to use the same ordering, and adds DCHECKs to enforce dependencies. BUG=693740 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2873313004 Cr-Commit-Position: refs/heads/master@{#472004} Committed: https://chromium.googlesource.com/chromium/src/+/80ecb98035a6654fff02e86877be... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/80ecb98035a6654fff02e86877be... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
