Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2841603003:
Record non-stacking-context as main thread scrolling reasons (Closed)
DescriptionRecord non-stacking-context as main thread scrolling reasons
There is another common main thread scrolling reason that needs
to be added. A scrollable area with an opaque background
is not a stacking context therefore we don't composite it for
now.
BUG=660907
TEST=ScrollingCoordinatorTest.StackingContextTest
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/2841603003
Cr-Commit-Position: refs/heads/master@{#468122}
Committed: https://chromium.googlesource.com/chromium/src/+/2605f2b320fe9d3d9fbe4d00964fa784a1b6ab16
Patch Set 1 #Patch Set 2 : Test update #
Total comments: 4
Patch Set 3 : Update reason name #
Total comments: 8
Patch Set 4 : Only keep the NonStackingContext reason #
Total comments: 2
Patch Set 5 : nit #
Messages
Total messages: 36 (15 generated)
Description was changed from ========== Record more main thread scrolling reasons BUG=660907 TEST=ScrollingCoordinatorTest.StackingContextTest ========== to ========== Record more main thread scrolling reasons BUG=660907 TEST=ScrollingCoordinatorTest.StackingContextTest CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Description was changed from ========== Record more main thread scrolling reasons BUG=660907 TEST=ScrollingCoordinatorTest.StackingContextTest CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Record more main thread scrolling reasons BUG=660907 TEST=ScrollingCoordinatorTest.StackingContextTest CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Description was changed from ========== Record more main thread scrolling reasons BUG=660907 TEST=ScrollingCoordinatorTest.StackingContextTest CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Record more main thread scrolling reasons The two main thread scrolling reasons being added to record are relatively common. A scrollable area with a pure background is not a stacking context therefore we don't composite it for now. Also breaking down one of the previous reasons "Background is not opaque in rect": If a scrollable area has background with an alpha channel, it's not composited due to "HasOverflowClip". This is a common case and is now recorded with "Background is not opaque in rect". It should be recorded separately. BUG=660907 TEST=ScrollingCoordinatorTest.StackingContextTest CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
yigu@chromium.org changed reviewers: + ajuma@chromium.org, bokan@chromium.org, flackr@chromium.org, jwd@chromium.org
With some further investigation on main thread scrolling reasons, I've found that there are at least two reasons should be added/separately recorded. PTAL. Thanks!
I'm not sure I understand the commit message on the first reason, what is a pure background? https://codereview.chromium.org/2841603003/diff/20001/cc/input/main_thread_sc... File cc/input/main_thread_scrolling_reason.h (right): https://codereview.chromium.org/2841603003/diff/20001/cc/input/main_thread_sc... cc/input/main_thread_scrolling_reason.h:48: kHasOverflowClipAndLCDText = 1 << 23, I think this name should be more specific - having an overflow clip isn't enough to have this reason set. https://codereview.chromium.org/2841603003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayer.h (right): https://codereview.chromium.org/2841603003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayer.h:592: uint32_t* reasons = nullptr) const; Document this parameter in the comments. A more descriptive name would also be helpful.
On 2017/04/25 18:04:15, bokan wrote: > I'm not sure I understand the commit message on the first reason, what is a pure > background? > > https://codereview.chromium.org/2841603003/diff/20001/cc/input/main_thread_sc... > File cc/input/main_thread_scrolling_reason.h (right): > > https://codereview.chromium.org/2841603003/diff/20001/cc/input/main_thread_sc... > cc/input/main_thread_scrolling_reason.h:48: kHasOverflowClipAndLCDText = 1 << > 23, > I think this name should be more specific - having an overflow clip isn't enough > to have this reason set. > > https://codereview.chromium.org/2841603003/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/paint/PaintLayer.h (right): > > https://codereview.chromium.org/2841603003/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/paint/PaintLayer.h:592: uint32_t* reasons = > nullptr) const; > Document this parameter in the comments. A more descriptive name would also be > helpful. By "pure background" I meant: <div style="overflow:scroll; width:200px; height:200px; background:green;"> <div style="height:1000px;"></div> </div>. Currently we handle this situation on main thread.
On 2017/04/25 19:16:56, yigu wrote: > By "pure background" I meant: > <div style="overflow:scroll; width:200px; height:200px; background:green;"> > <div style="height:1000px;"></div> > </div>. > Currently we handle this situation on main thread. Ah, please call this an "opaque background"
Description was changed from ========== Record more main thread scrolling reasons The two main thread scrolling reasons being added to record are relatively common. A scrollable area with a pure background is not a stacking context therefore we don't composite it for now. Also breaking down one of the previous reasons "Background is not opaque in rect": If a scrollable area has background with an alpha channel, it's not composited due to "HasOverflowClip". This is a common case and is now recorded with "Background is not opaque in rect". It should be recorded separately. BUG=660907 TEST=ScrollingCoordinatorTest.StackingContextTest CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Record more main thread scrolling reasons The two main thread scrolling reasons being added to record are relatively common. A scrollable area with an opaque background is not a stacking context therefore we don't composite it for now. Also breaking down one of the previous reasons "Background is not opaque in rect": If a scrollable area has background with an alpha channel, it's not composited due to "HasOverflowClip". This is a common case and is now recorded with "Background is not opaque in rect". It should be recorded separately. BUG=660907 TEST=ScrollingCoordinatorTest.StackingContextTest CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
PTAL. Thanks! https://codereview.chromium.org/2841603003/diff/20001/cc/input/main_thread_sc... File cc/input/main_thread_scrolling_reason.h (right): https://codereview.chromium.org/2841603003/diff/20001/cc/input/main_thread_sc... cc/input/main_thread_scrolling_reason.h:48: kHasOverflowClipAndLCDText = 1 << 23, On 2017/04/25 18:04:15, bokan wrote: > I think this name should be more specific - having an overflow clip isn't enough > to have this reason set. Updated. It also requires that the layout object's background is not opaque, e.g. it has alpha channel. https://codereview.chromium.org/2841603003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayer.h (right): https://codereview.chromium.org/2841603003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayer.h:592: uint32_t* reasons = nullptr) const; On 2017/04/25 18:04:15, bokan wrote: > Document this parameter in the comments. A more descriptive name would also be > helpful. Done.
code is lgtm % fixes to comments but please have flackr@ take a look as I'm not as familiar with whether these reasons are being correctly reported. https://codereview.chromium.org/2841603003/diff/40001/cc/input/main_thread_sc... File cc/input/main_thread_scrolling_reason.h (right): https://codereview.chromium.org/2841603003/diff/40001/cc/input/main_thread_sc... cc/input/main_thread_scrolling_reason.h:48: kHasOverflowClipAndLayoutObjectBackgroundNotOpaqueAndLCDText = 1 << 23, I'd just call it kHasOverflowClipNotOpaqueAndLCDText, LayoutObject is implied. https://codereview.chromium.org/2841603003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayer.h (right): https://codereview.chromium.org/2841603003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayer.h:592: // why it returns false. Under which circumstance? It's in order to record UMA stats about why we didn't composite.
metrics LGTM
https://codereview.chromium.org/2841603003/diff/40001/cc/input/main_thread_sc... File cc/input/main_thread_scrolling_reason.h (right): https://codereview.chromium.org/2841603003/diff/40001/cc/input/main_thread_sc... cc/input/main_thread_scrolling_reason.h:48: kHasOverflowClipAndLayoutObjectBackgroundNotOpaqueAndLCDText = 1 << 23, On 2017/04/26 15:52:15, bokan wrote: > I'd just call it kHasOverflowClipNotOpaqueAndLCDText, LayoutObject is implied. We have another BackgroundNotOpaque used for PaintLayer (see reason 18 above). That's why I had this lame name. https://codereview.chromium.org/2841603003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayer.h (right): https://codereview.chromium.org/2841603003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayer.h:592: // why it returns false. On 2017/04/26 15:52:15, bokan wrote: > Under which circumstance? It's in order to record UMA stats about why we didn't > composite. Done.
https://codereview.chromium.org/2841603003/diff/40001/cc/input/main_thread_sc... File cc/input/main_thread_scrolling_reason.h (right): https://codereview.chromium.org/2841603003/diff/40001/cc/input/main_thread_sc... cc/input/main_thread_scrolling_reason.h:48: kHasOverflowClipAndLayoutObjectBackgroundNotOpaqueAndLCDText = 1 << 23, On 2017/04/27 15:28:27, yigu wrote: > On 2017/04/26 15:52:15, bokan wrote: > > I'd just call it kHasOverflowClipNotOpaqueAndLCDText, LayoutObject is implied. > > We have another BackgroundNotOpaque used for PaintLayer (see reason 18 above). > That's why I had this lame name. But the difference between the two is that this reason also adds HasOverflowClip, right? For my understanding: this new reason is basically a specialisation of BackgroundNotOpaque, right? So if this reason is set, BackgroundNotOpaque will also be set. Is that right?
On 2017/04/27 15:40:31, bokan wrote: > https://codereview.chromium.org/2841603003/diff/40001/cc/input/main_thread_sc... > File cc/input/main_thread_scrolling_reason.h (right): > > https://codereview.chromium.org/2841603003/diff/40001/cc/input/main_thread_sc... > cc/input/main_thread_scrolling_reason.h:48: > kHasOverflowClipAndLayoutObjectBackgroundNotOpaqueAndLCDText = 1 << 23, > On 2017/04/27 15:28:27, yigu wrote: > > On 2017/04/26 15:52:15, bokan wrote: > > > I'd just call it kHasOverflowClipNotOpaqueAndLCDText, LayoutObject is > implied. > > > > We have another BackgroundNotOpaque used for PaintLayer (see reason 18 above). > > That's why I had this lame name. > > But the difference between the two is that this reason also adds > HasOverflowClip, right? For my understanding: this new reason is basically a > specialisation of BackgroundNotOpaque, right? So if this reason is set, > BackgroundNotOpaque will also be set. Is that right? Yes. If this reason is set, reason 18 is also set. To be more specific, if a paint layer's immediate layout object has non-opaque background and the paint layer has overflow clip, we set the paint layer's background as non-opaque.
On 2017/04/27 15:44:14, yigu wrote: > On 2017/04/27 15:40:31, bokan wrote: > > > https://codereview.chromium.org/2841603003/diff/40001/cc/input/main_thread_sc... > > File cc/input/main_thread_scrolling_reason.h (right): > > > > > https://codereview.chromium.org/2841603003/diff/40001/cc/input/main_thread_sc... > > cc/input/main_thread_scrolling_reason.h:48: > > kHasOverflowClipAndLayoutObjectBackgroundNotOpaqueAndLCDText = 1 << 23, > > On 2017/04/27 15:28:27, yigu wrote: > > > On 2017/04/26 15:52:15, bokan wrote: > > > > I'd just call it kHasOverflowClipNotOpaqueAndLCDText, LayoutObject is > > implied. > > > > > > We have another BackgroundNotOpaque used for PaintLayer (see reason 18 > above). > > > That's why I had this lame name. > > > > But the difference between the two is that this reason also adds > > HasOverflowClip, right? For my understanding: this new reason is basically a > > specialisation of BackgroundNotOpaque, right? So if this reason is set, > > BackgroundNotOpaque will also be set. Is that right? > > Yes. If this reason is set, reason 18 is also set. To be more specific, if a > paint layer's immediate layout object has non-opaque background and the paint > layer has overflow clip, we set the paint layer's background as non-opaque. I see. I'll let flackr@ judge if the long name is better since he's more well-versed here. The only thing I'd add is that I think LayoutObject is implicit since all the properties of a PaintLayer come from LayoutObjects. If I understand correctly, it doesn't really make sense to talk about a PaintLayer's background as separate from the LayoutObject's background so I think we could avoid the verbose name. But I'll leave that to flackr@ :)
...and lgtm (assuming you haven't uploaded a new patch with the updated comment).
https://codereview.chromium.org/2841603003/diff/40001/cc/input/main_thread_sc... File cc/input/main_thread_scrolling_reason.h (right): https://codereview.chromium.org/2841603003/diff/40001/cc/input/main_thread_sc... cc/input/main_thread_scrolling_reason.h:48: kHasOverflowClipAndLayoutObjectBackgroundNotOpaqueAndLCDText = 1 << 23, On 2017/04/27 15:40:31, bokan wrote: > On 2017/04/27 15:28:27, yigu wrote: > > On 2017/04/26 15:52:15, bokan wrote: > > > I'd just call it kHasOverflowClipNotOpaqueAndLCDText, LayoutObject is > implied. > > > > We have another BackgroundNotOpaque used for PaintLayer (see reason 18 above). > > That's why I had this lame name. > > But the difference between the two is that this reason also adds > HasOverflowClip, right? For my understanding: this new reason is basically a > specialisation of BackgroundNotOpaque, right? So if this reason is set, > BackgroundNotOpaque will also be set. Is that right? I thought all of these were reasons not to promote overflow scrollers? The function seems to be called from PaintLayerScrollableArea::ComputeNeedsCompositedScrolling which has an early out if it's not a scroller. So BackgroundNotOpaque also has overflow, in which case I'm not sure I understand the difference here. When is BackgroundNotOpaque true but this false, or vice versa? Looking at where you added it, I think the condition you mean to add is just Clip, not specifically overflow clip, but because we're only recording this metric for overflow scrollers which we are considering reporting it will always have a clip related property so all we can say is that we don't know if the background will be opaque (i.e. BackgroundNotOpaque). https://codereview.chromium.org/2841603003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayer.h (right): https://codereview.chromium.org/2841603003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayer.h:592: // why it returns false. On 2017/04/27 15:28:27, yigu wrote: > On 2017/04/26 15:52:15, bokan wrote: > > Under which circumstance? It's in order to record UMA stats about why we > didn't > > composite. > > Done. New patchset?
After chatting with flackr@, we decide that only record the NonStackingContext reason in this patch. PTAL. Thanks! https://codereview.chromium.org/2841603003/diff/40001/cc/input/main_thread_sc... File cc/input/main_thread_scrolling_reason.h (right): https://codereview.chromium.org/2841603003/diff/40001/cc/input/main_thread_sc... cc/input/main_thread_scrolling_reason.h:48: kHasOverflowClipAndLayoutObjectBackgroundNotOpaqueAndLCDText = 1 << 23, On 2017/04/27 18:42:35, flackr wrote: > On 2017/04/27 15:40:31, bokan wrote: > > On 2017/04/27 15:28:27, yigu wrote: > > > On 2017/04/26 15:52:15, bokan wrote: > > > > I'd just call it kHasOverflowClipNotOpaqueAndLCDText, LayoutObject is > > implied. > > > > > > We have another BackgroundNotOpaque used for PaintLayer (see reason 18 > above). > > > That's why I had this lame name. > > > > But the difference between the two is that this reason also adds > > HasOverflowClip, right? For my understanding: this new reason is basically a > > specialisation of BackgroundNotOpaque, right? So if this reason is set, > > BackgroundNotOpaque will also be set. Is that right? > > I thought all of these were reasons not to promote overflow scrollers? The > function seems to be called from > PaintLayerScrollableArea::ComputeNeedsCompositedScrolling which has an early out > if it's not a scroller. So BackgroundNotOpaque also has overflow, in which case > I'm not sure I understand the difference here. When is BackgroundNotOpaque true > but this false, or vice versa? > > Looking at where you added it, I think the condition you mean to add is just > Clip, not specifically overflow clip, but because we're only recording this > metric for overflow scrollers which we are considering reporting it will always > have a clip related property so all we can say is that we don't know if the > background will be opaque (i.e. BackgroundNotOpaque). This reason has been removed from this patch.
Description was changed from ========== Record more main thread scrolling reasons The two main thread scrolling reasons being added to record are relatively common. A scrollable area with an opaque background is not a stacking context therefore we don't composite it for now. Also breaking down one of the previous reasons "Background is not opaque in rect": If a scrollable area has background with an alpha channel, it's not composited due to "HasOverflowClip". This is a common case and is now recorded with "Background is not opaque in rect". It should be recorded separately. BUG=660907 TEST=ScrollingCoordinatorTest.StackingContextTest CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Record non-stacking-context as main thread scrolling reasons There is another common main thread scrolling reason that needs to be added. A scrollable area with an opaque background is not a stacking context therefore we don't composite it for now. BUG=660907 TEST=ScrollingCoordinatorTest.StackingContextTest CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Description was changed from ========== Record non-stacking-context as main thread scrolling reasons There is another common main thread scrolling reason that needs to be added. A scrollable area with an opaque background is not a stacking context therefore we don't composite it for now. BUG=660907 TEST=ScrollingCoordinatorTest.StackingContextTest CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Record non-stacking-context as main thread scrolling reasons There is another common main thread scrolling reason that needs to be added. A scrollable area with an opaque background is not a stacking context therefore we don't composite it for now. BUG=660907 TEST=ScrollingCoordinatorTest.StackingContextTest CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
lgtm
https://codereview.chromium.org/2841603003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/ScrollMetricsTest.cpp (right): https://codereview.chromium.org/2841603003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/ScrollMetricsTest.cpp:352: nit: Remove extra line
The CQ bit was checked by yigu@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/2841603003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/ScrollMetricsTest.cpp (right): https://codereview.chromium.org/2841603003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/ScrollMetricsTest.cpp:352: On 2017/04/28 17:36:58, flackr wrote: > nit: Remove extra line Done.
cc lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by yigu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org, jwd@chromium.org, flackr@chromium.org Link to the patchset: https://codereview.chromium.org/2841603003/#ps80001 (title: "nit")
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": 80001, "attempt_start_ts": 1493411959601920,
"parent_rev": "c76599f4da775f3f5a992083a841ea2207831470", "commit_rev":
"2605f2b320fe9d3d9fbe4d00964fa784a1b6ab16"}
Message was sent while issue was closed.
Description was changed from ========== Record non-stacking-context as main thread scrolling reasons There is another common main thread scrolling reason that needs to be added. A scrollable area with an opaque background is not a stacking context therefore we don't composite it for now. BUG=660907 TEST=ScrollingCoordinatorTest.StackingContextTest CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Record non-stacking-context as main thread scrolling reasons There is another common main thread scrolling reason that needs to be added. A scrollable area with an opaque background is not a stacking context therefore we don't composite it for now. BUG=660907 TEST=ScrollingCoordinatorTest.StackingContextTest 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/2841603003 Cr-Commit-Position: refs/heads/master@{#468122} Committed: https://chromium.googlesource.com/chromium/src/+/2605f2b320fe9d3d9fbe4d00964f... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/2605f2b320fe9d3d9fbe4d00964f... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
