Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2867793002:
Add a cache of LayerImpl's viewport layer type (Closed)
DescriptionAdd a cache of LayerImpl's viewport layer type
This patch caches the viewport layer type of LayerImpl to fix a performance
regression in LayerImpl::ViewportBoundsDelta due to layer lookups.
Big changes in this patch:
* A viewport_layer_type_ member has been added to LayerImpl along with
getters and setters.
* The viewport layer type is updated when LayerTreeImpl's viewport layer
id changes, or when LayerImpl's scroll_clip_layer_id changes. Both of
these values affect the viewport layer type.
* DCHECKs have been added that the viewport layer type does not change.
Making this a constructor parameter to LayerImpl would be ideal but is
very difficult.
* DCHECKs have been added in LTI::IsViewportLayerId that verify
LayerTreeImpl's viewport layer ids are always in sync with LayerImpl's
viewport layer types.
BUG=715956
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2867793002
Cr-Commit-Position: refs/heads/master@{#470743}
Committed: https://chromium.googlesource.com/chromium/src/+/6280cc1d5a1dccfbe23ca438eed862e245f18614
Patch Set 1 #Patch Set 2 : Use typed enum because by default VC uses signed enums #Patch Set 3 : sacrificial offering to appease the compiler gods #Patch Set 4 : Back to static cats and unsigned enums #Patch Set 5 : Fix compile on windows again #
Total comments: 12
Patch Set 6 : Address reviewer comments #
Total comments: 2
Patch Set 7 : ADD_A_LAST_ENUM #Patch Set 8 : Rebase from space #
Messages
Total messages: 41 (27 generated)
Description was changed from ========== Add a cache of LayerImpl's viewport layer type This patch caches the viewport layer type of LayerImpl to fix a performance regression in LayerImpl::ViewportBoundsDelta due to layer lookups. Big changes in this patch: * A viewport_layer_type_ member has been added to LayerImpl along with getters and setters. * The viewport layer type is updated when LayerTreeImpl's viewport layer id changes, or when LayerImpl's scroll_clip_layer_id changes. Both of these values affect the viewport layer type. * DCHECKs have been added that the viewport layer type does not change. Making this a constructor parameter to LayerImpl would be ideal but is very difficult. * DCHECKs have been added in LTI::IsViewportLayerId that verify LayerTreeImpl's viewport layer ids are always in sync with LayerImpl's viewport layer types. BUG=715956 ========== to ========== Add a cache of LayerImpl's viewport layer type This patch caches the viewport layer type of LayerImpl to fix a performance regression in LayerImpl::ViewportBoundsDelta due to layer lookups. Big changes in this patch: * A viewport_layer_type_ member has been added to LayerImpl along with getters and setters. * The viewport layer type is updated when LayerTreeImpl's viewport layer id changes, or when LayerImpl's scroll_clip_layer_id changes. Both of these values affect the viewport layer type. * DCHECKs have been added that the viewport layer type does not change. Making this a constructor parameter to LayerImpl would be ideal but is very difficult. * DCHECKs have been added in LTI::IsViewportLayerId that verify LayerTreeImpl's viewport layer ids are always in sync with LayerImpl's viewport layer types. BUG=715956 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
pdr@chromium.org changed reviewers: + enne@chromium.org, jaydasika@chromium.org
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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
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: 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 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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) 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 to run a CQ dry run
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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Have you measured if the cc_perftest, CalcDrawPropsTest, shows an improvement with this patch ? https://codereview.chromium.org/2867793002/diff/80001/cc/trees/layer_tree_imp... File cc/trees/layer_tree_impl.cc (left): https://codereview.chromium.org/2867793002/diff/80001/cc/trees/layer_tree_imp... cc/trees/layer_tree_impl.cc:144: return true; Since we want to avoid calling LayerById whenever possible (as its costly), can we keep this ?
https://codereview.chromium.org/2867793002/diff/80001/cc/layers/layer_impl.cc File cc/layers/layer_impl.cc (right): https://codereview.chromium.org/2867793002/diff/80001/cc/layers/layer_impl.cc... cc/layers/layer_impl.cc:543: default: style nit: can you not use default/NOTREACHED in the switch statement so that anybody who adds additional enum values will have the compiler yell? Instead just list the other two values here as a fallthrough to NOTREACHED? https://codereview.chromium.org/2867793002/diff/80001/cc/layers/layer_impl.h File cc/layers/layer_impl.h (right): https://codereview.chromium.org/2867793002/diff/80001/cc/layers/layer_impl.h#... cc/layers/layer_impl.h:79: // If values are added or removed, ensure LayerImpl::viewport_layer_type_ is Can you static assert this instead of leaving a comment? Maybe down below saying static_assert(2<<bitfieldsize > LAST_VIEWPORT_LAYER_TYPE)? https://codereview.chromium.org/2867793002/diff/80001/cc/layers/layer_impl.h#... cc/layers/layer_impl.h:284: // Once set as a viewport layer type, the viewport type should not change. Can you leave a "why" here as well? Or a quick pointer at which code assumes this? https://codereview.chromium.org/2867793002/diff/80001/cc/layers/layer_impl.h#... cc/layers/layer_impl.h:526: unsigned viewport_layer_type_ : 3; // ViewportLayerType s/unsigned/uint8_t/
On 2017/05/09 at 05:54:22, jaydasika wrote: > Have you measured if the cc_perftest, CalcDrawPropsTest, shows an improvement with this patch ? Yeah, we are back to the original performance with this patch.
PTAL, also a question for Enne on Line 284. https://codereview.chromium.org/2867793002/diff/80001/cc/layers/layer_impl.cc File cc/layers/layer_impl.cc (right): https://codereview.chromium.org/2867793002/diff/80001/cc/layers/layer_impl.cc... cc/layers/layer_impl.cc:543: default: On 2017/05/09 at 17:08:29, enne wrote: > style nit: can you not use default/NOTREACHED in the switch statement so that anybody who adds additional enum values will have the compiler yell? Instead just list the other two values here as a fallthrough to NOTREACHED? Done. https://codereview.chromium.org/2867793002/diff/80001/cc/layers/layer_impl.h File cc/layers/layer_impl.h (right): https://codereview.chromium.org/2867793002/diff/80001/cc/layers/layer_impl.h#... cc/layers/layer_impl.h:79: // If values are added or removed, ensure LayerImpl::viewport_layer_type_ is On 2017/05/09 at 17:08:29, enne wrote: > Can you static assert this instead of leaving a comment? Maybe down below saying static_assert(2<<bitfieldsize > LAST_VIEWPORT_LAYER_TYPE)? Done, I went with: static_assert(OUTER_VIEWPORT_SCROLL< (1u << 3), "enough bits for ViewportLayerType (viewport_layer_type_)"); https://codereview.chromium.org/2867793002/diff/80001/cc/layers/layer_impl.h#... cc/layers/layer_impl.h:284: // Once set as a viewport layer type, the viewport type should not change. On 2017/05/09 at 17:08:29, enne wrote: > Can you leave a "why" here as well? Or a quick pointer at which code assumes this? This appears to be how the system works (we don't shuffle around viewport layers) but I haven't seen that stated anywhere. The new code in this patch doesn't unset the viewport layer types on the old layers if a different layer becomes the viewport layer, so that would break if this assumption changes. Do you think it would be useful to include that info here as a comment? Or did you mean to include something else here? https://codereview.chromium.org/2867793002/diff/80001/cc/layers/layer_impl.h#... cc/layers/layer_impl.h:526: unsigned viewport_layer_type_ : 3; // ViewportLayerType On 2017/05/09 at 17:08:29, enne wrote: > s/unsigned/uint8_t/ Done https://codereview.chromium.org/2867793002/diff/80001/cc/trees/layer_tree_imp... File cc/trees/layer_tree_impl.cc (left): https://codereview.chromium.org/2867793002/diff/80001/cc/trees/layer_tree_imp... cc/trees/layer_tree_impl.cc:144: return true; On 2017/05/09 at 05:54:22, jaydasika wrote: > Since we want to avoid calling LayerById whenever possible (as its costly), can we keep this ? I'm a little torn on this because I don't know the most common way this function is called. For example, if inner_viewport_scroll_layer_id_ or outer_viewport_scroll_layer_id_ equal the id, your suggestion will be fast. On the other hand, the old InnerViewportContainerLayer() and OuterViewportContainerLayer() conditionals actually had two LayerById() calls in them, so the new code would be faster. WDYT of committing this and seeing the effect on the perf graphs?
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...
https://codereview.chromium.org/2867793002/diff/80001/cc/layers/layer_impl.h File cc/layers/layer_impl.h (right): https://codereview.chromium.org/2867793002/diff/80001/cc/layers/layer_impl.h#... cc/layers/layer_impl.h:284: // Once set as a viewport layer type, the viewport type should not change. On 2017/05/09 at 22:20:07, pdr. wrote: > On 2017/05/09 at 17:08:29, enne wrote: > > Can you leave a "why" here as well? Or a quick pointer at which code assumes this? > > This appears to be how the system works (we don't shuffle around viewport layers) but I haven't seen that stated anywhere. > > The new code in this patch doesn't unset the viewport layer types on the old layers if a different layer becomes the viewport layer, so that would break if this assumption changes. Do you think it would be useful to include that info here as a comment? Or did you mean to include something else here? Hmm ok ok. This is fine as-is. https://codereview.chromium.org/2867793002/diff/100001/cc/layers/layer_impl.h File cc/layers/layer_impl.h (right): https://codereview.chromium.org/2867793002/diff/100001/cc/layers/layer_impl.h... cc/layers/layer_impl.h:524: static_assert(OUTER_VIEWPORT_SCROLL < (1u << 3), Sorry to nit, but are you opposed to using LAST here? My concern is that you could update the enum up above, adding more values, and this would still pass. If there's a LAST enum, then that's pretty clear that you need to update that.
https://codereview.chromium.org/2867793002/diff/100001/cc/layers/layer_impl.h File cc/layers/layer_impl.h (right): https://codereview.chromium.org/2867793002/diff/100001/cc/layers/layer_impl.h... cc/layers/layer_impl.h:524: static_assert(OUTER_VIEWPORT_SCROLL < (1u << 3), On 2017/05/09 at 22:31:52, enne wrote: > Sorry to nit, but are you opposed to using LAST here? My concern is that you could update the enum up above, adding more values, and this would still pass. If there's a LAST enum, then that's pretty clear that you need to update that. This is no nit, you are right. Fixed.
Thanks! lgtm
Jaydasika, could you PTAL as well? Lets commit if you are happy.
lgtm https://codereview.chromium.org/2867793002/diff/80001/cc/trees/layer_tree_imp... File cc/trees/layer_tree_impl.cc (left): https://codereview.chromium.org/2867793002/diff/80001/cc/trees/layer_tree_imp... cc/trees/layer_tree_impl.cc:144: return true; On 2017/05/09 22:20:07, pdr. wrote: > On 2017/05/09 at 05:54:22, jaydasika wrote: > > Since we want to avoid calling LayerById whenever possible (as its costly), > can we keep this ? > > I'm a little torn on this because I don't know the most common way this function > is called. For example, if inner_viewport_scroll_layer_id_ or > outer_viewport_scroll_layer_id_ equal the id, your suggestion will be fast. On > the other hand, the old InnerViewportContainerLayer() and > OuterViewportContainerLayer() conditionals actually had two LayerById() calls in > them, so the new code would be faster. WDYT of committing this and seeing the > effect on the perf graphs? Sounds good. lets commit this and see the perf graphs.
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: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?))
The CQ bit was checked by pdr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jaydasika@chromium.org, enne@chromium.org Link to the patchset: https://codereview.chromium.org/2867793002/#ps140001 (title: "Rebase from space")
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": 1494445437473200,
"parent_rev": "887dee671bfb3f1862912eae6d92a127e67f6325", "commit_rev":
"6280cc1d5a1dccfbe23ca438eed862e245f18614"}
Message was sent while issue was closed.
Description was changed from ========== Add a cache of LayerImpl's viewport layer type This patch caches the viewport layer type of LayerImpl to fix a performance regression in LayerImpl::ViewportBoundsDelta due to layer lookups. Big changes in this patch: * A viewport_layer_type_ member has been added to LayerImpl along with getters and setters. * The viewport layer type is updated when LayerTreeImpl's viewport layer id changes, or when LayerImpl's scroll_clip_layer_id changes. Both of these values affect the viewport layer type. * DCHECKs have been added that the viewport layer type does not change. Making this a constructor parameter to LayerImpl would be ideal but is very difficult. * DCHECKs have been added in LTI::IsViewportLayerId that verify LayerTreeImpl's viewport layer ids are always in sync with LayerImpl's viewport layer types. BUG=715956 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Add a cache of LayerImpl's viewport layer type This patch caches the viewport layer type of LayerImpl to fix a performance regression in LayerImpl::ViewportBoundsDelta due to layer lookups. Big changes in this patch: * A viewport_layer_type_ member has been added to LayerImpl along with getters and setters. * The viewport layer type is updated when LayerTreeImpl's viewport layer id changes, or when LayerImpl's scroll_clip_layer_id changes. Both of these values affect the viewport layer type. * DCHECKs have been added that the viewport layer type does not change. Making this a constructor parameter to LayerImpl would be ideal but is very difficult. * DCHECKs have been added in LTI::IsViewportLayerId that verify LayerTreeImpl's viewport layer ids are always in sync with LayerImpl's viewport layer types. BUG=715956 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2867793002 Cr-Commit-Position: refs/heads/master@{#470743} Committed: https://chromium.googlesource.com/chromium/src/+/6280cc1d5a1dccfbe23ca438eed8... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/6280cc1d5a1dccfbe23ca438eed8... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
