Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2565223002:
Add more specific metrics for main thread scrolling reasons (Closed)
|
Created:
4 years ago by yigu Modified:
3 years, 11 months ago CC:
asvitkine+watch_chromium.org, blink-layers+watch_chromium.org, blink-reviews, blink-reviews-layout_chromium.org, blink-reviews-paint_chromium.org, cc-bugs_chromium.org, chromium-reviews, dshwang, eae+blinkwatch, jchaffraix+rendering, kenneth.christiansen, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, szager+layoutwatch_chromium.org, zoltan1 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionWe should replace the broad main thread reasons of non layer viewport constrained objects and non fast scrollable reason with the more specific reasons which are preventing us from compositing them so that we can prioritize.
BUG=660907
TEST=ScrollingCoordinatorTest.MainThreadScrollingReasonDueToLayoutObject
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Committed: https://crrev.com/e92b679fee2d010570bb1ea1896b443d01eb5a84
Cr-Commit-Position: refs/heads/master@{#439944}
Patch Set 1 #Patch Set 2 : Add unit test && refactoring #
Total comments: 24
Patch Set 3 : Relocate the logic of updating mainThreadScrollingFromStyle #
Total comments: 14
Patch Set 4 : Move logic regarding clearing up ReasonsCounter upon layer dispose to ScrollingCoordinator #
Total comments: 2
Patch Set 5 : Add preferCompositingToLCDTextEnable check #
Total comments: 4
Patch Set 6 : Move updating main thread scrolling reasons from style to PLSA #
Total comments: 8
Patch Set 7 : Refactoring #
Total comments: 18
Patch Set 8 : Refactoring && merging master #
Total comments: 4
Patch Set 9 : nit #
Total comments: 4
Patch Set 10 : Test that there should be no reason set if setPrefercompositingToCDLTextEnabled(true) #
Total comments: 4
Messages
Total messages: 59 (26 generated)
Description was changed from ========== We should replace the broad main thread reasons of non layer viewport constrained objects and non fast scrollable reason with the more specific reasons which are preventing us from compositing them so that we can prioritize. BUG=660907 ========== to ========== We should replace the broad main thread reasons of non layer viewport constrained objects and non fast scrollable reason with the more specific reasons which are preventing us from compositing them so that we can prioritize. BUG=660907 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Description was changed from ========== We should replace the broad main thread reasons of non layer viewport constrained objects and non fast scrollable reason with the more specific reasons which are preventing us from compositing them so that we can prioritize. BUG=660907 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== We should replace the broad main thread reasons of non layer viewport constrained objects and non fast scrollable reason with the more specific reasons which are preventing us from compositing them so that we can prioritize. BUG=660907 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Description was changed from ========== We should replace the broad main thread reasons of non layer viewport constrained objects and non fast scrollable reason with the more specific reasons which are preventing us from compositing them so that we can prioritize. BUG=660907 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== We should replace the broad main thread reasons of non layer viewport constrained objects and non fast scrollable reason with the more specific reasons which are preventing us from compositing them so that we can prioritize. BUG=660907 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Description was changed from ========== We should replace the broad main thread reasons of non layer viewport constrained objects and non fast scrollable reason with the more specific reasons which are preventing us from compositing them so that we can prioritize. BUG=660907 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== We should replace the broad main thread reasons of non layer viewport constrained objects and non fast scrollable reason with the more specific reasons which are preventing us from compositing them so that we can prioritize. BUG=660907 TEST=ScrollingCoordinatorTest.MainThreadScrollingReasonDueToLayoutObject CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
yigu@chromium.org changed reviewers: + bokan@chromium.org
Hi David, This patch is about adding the main thread scrolling reason "layout has opacity". PTAL. Thanks! Yi
https://codereview.chromium.org/2565223002/diff/20001/cc/input/main_thread_sc... File cc/input/main_thread_scrolling_reason.h (right): https://codereview.chromium.org/2565223002/diff/20001/cc/input/main_thread_sc... cc/input/main_thread_scrolling_reason.h:34: kHasOpacity = 1 << 16, kHasOpacityAndScrollDependent? A regular scroller won't be main threaded because it's transparent, right? https://codereview.chromium.org/2565223002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2565223002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.cpp:4683: m_mainThreadScrollingReasonLayoutCounter[reasonIndex(reason)] += This should never be negative, right? If so, please DCHECK that it isn't. https://codereview.chromium.org/2565223002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameView.h (right): https://codereview.chromium.org/2565223002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.h:770: inline int reasonIndex(uint32_t reason) const { inline unneeded, this is inline by virtue of being defined in the class declaration. https://codereview.chromium.org/2565223002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.h:770: inline int reasonIndex(uint32_t reason) const { This should move as a helper into MainThreadScrollingReason.h https://codereview.chromium.org/2565223002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.h:781: void adjustMainThreadReasonsDueToLayoutObject(uint32_t reason, bool increase); IMO, this should be called adjustMainThreadReasons or adjustMainThreadReasonsFromStyle https://codereview.chromium.org/2565223002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.h:783: MainThreadScrollingReasons checkMainThreadScrollingReasonDueToLayoutObject(); check->get, Reason->Reasons https://codereview.chromium.org/2565223002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.h:1155: Vector<int> m_mainThreadScrollingReasonLayoutCounter; My read is that this vector is tracking scrolling reasons for all scrollers in a FrameView (other than the FrameView itself) and they all get combined together for a page-wide bitmask. In which case this doesn't belong here (we're trying to remove scrolling from FrameView anyway, so it wouldn't belong here in any case). Perhaps this should just be in ScrollingCoordinator itself, globally, for the whole page. https://codereview.chromium.org/2565223002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/compositing/CompositingReasonFinder.cpp (right): https://codereview.chromium.org/2565223002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/compositing/CompositingReasonFinder.cpp:186: // is not ideal. When a layout becomes to non-scrollable nit: "layout becomes to" -> "layer becomes" https://codereview.chromium.org/2565223002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/compositing/CompositingReasonFinder.cpp:186: // is not ideal. When a layout becomes to non-scrollable Also, perhaps counterintuitively, I believe all layers created for a LayoutBox will have a PLSA -- even if they're not scrollable. Perhaps you just need to make the PLSA decrement any counts it has in dispose() https://codereview.chromium.org/2565223002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/compositing/CompositingReasonFinder.cpp:212: updateMainThreadScrollingReasonDueToLayoutObject(layer); This is only meant for Fixed/Sticky layers? The name should make that clear. https://codereview.chromium.org/2565223002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/data/two_transparent_scrollable_area.html (right): https://codereview.chromium.org/2565223002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/data/two_transparent_scrollable_area.html:9: position: fixed; Please add another scroller that's transparent but not fixed. And maybe make one of these sticky?
Hi David, Thanks for the feedback! Just a quick thought. I am not sure about "A regular scroller won't be main threaded because it's transparent". Without this patch, with http://output.jsbin.com/buziheyila: - The main scroller (s1) has mainThreadScrollingReason: Has non-layer viewport-constrained objects. But when checking the box "scrolling performance issues" in dev_tools, s1 seems NOT to repaint on scroll. - The other scroller (s2) has the same reason but need to repaint on scroll. If we remove the position:fixed property: - S1 doesn't have any mainThreadScrollingReasons and it doesn't repaint on scroll - S2 has the reason: Non fast scrollable region and need to repaint on scroll. If we further remove the content div: - S1 scrolls on impl - S2 doesn't exist It look like s2 will scroll on main as long as it has opacity. The reasons for that vary due to other factors. Maybe I misunderstood something here. Please correct me. BTW, I track the reasons by adding "LOG(ERROR) << "slowScrollingReasons: " << cc::MainThreadScrollingReason::mainThreadScrollingReasonsAsText(reasons);" here <https://cs.chromium.org/chromium/src/ui/events/blink/input_handler_proxy.cc?q...> . Thanks! Yi On Tue, Dec 13, 2016 at 9:38 AM, <bokan@chromium.org> wrote: > > https://codereview.chromium.org/2565223002/diff/20001/cc/ > input/main_thread_scrolling_reason.h > File cc/input/main_thread_scrolling_reason.h (right): > > https://codereview.chromium.org/2565223002/diff/20001/cc/ > input/main_thread_scrolling_reason.h#newcode34 > cc/input/main_thread_scrolling_reason.h:34: kHasOpacity = 1 << 16, > kHasOpacityAndScrollDependent? A regular scroller won't be main threaded > because it's transparent, right? > > https://codereview.chromium.org/2565223002/diff/20001/ > third_party/WebKit/Source/core/frame/FrameView.cpp > File third_party/WebKit/Source/core/frame/FrameView.cpp (right): > > https://codereview.chromium.org/2565223002/diff/20001/ > third_party/WebKit/Source/core/frame/FrameView.cpp#newcode4683 > third_party/WebKit/Source/core/frame/FrameView.cpp:4683: > m_mainThreadScrollingReasonLayoutCounter[reasonIndex(reason)] += > This should never be negative, right? If so, please DCHECK that it > isn't. > > https://codereview.chromium.org/2565223002/diff/20001/ > third_party/WebKit/Source/core/frame/FrameView.h > File third_party/WebKit/Source/core/frame/FrameView.h (right): > > https://codereview.chromium.org/2565223002/diff/20001/ > third_party/WebKit/Source/core/frame/FrameView.h#newcode770 > third_party/WebKit/Source/core/frame/FrameView.h:770: inline int > reasonIndex(uint32_t reason) const { > inline unneeded, this is inline by virtue of being defined in the class > declaration. > > https://codereview.chromium.org/2565223002/diff/20001/ > third_party/WebKit/Source/core/frame/FrameView.h#newcode770 > third_party/WebKit/Source/core/frame/FrameView.h:770: inline int > reasonIndex(uint32_t reason) const { > This should move as a helper into MainThreadScrollingReason.h > > https://codereview.chromium.org/2565223002/diff/20001/ > third_party/WebKit/Source/core/frame/FrameView.h#newcode781 > third_party/WebKit/Source/core/frame/FrameView.h:781: void > adjustMainThreadReasonsDueToLayoutObject(uint32_t reason, bool > increase); > IMO, this should be called adjustMainThreadReasons or > adjustMainThreadReasonsFromStyle > > https://codereview.chromium.org/2565223002/diff/20001/ > third_party/WebKit/Source/core/frame/FrameView.h#newcode783 > third_party/WebKit/Source/core/frame/FrameView.h:783: > MainThreadScrollingReasons > checkMainThreadScrollingReasonDueToLayoutObject(); > check->get, Reason->Reasons > > https://codereview.chromium.org/2565223002/diff/20001/ > third_party/WebKit/Source/core/frame/FrameView.h#newcode1155 > third_party/WebKit/Source/core/frame/FrameView.h:1155: Vector<int> > m_mainThreadScrollingReasonLayoutCounter; > My read is that this vector is tracking scrolling reasons for all > scrollers in a FrameView (other than the FrameView itself) and they all > get combined together for a page-wide bitmask. In which case this > doesn't belong here (we're trying to remove scrolling from FrameView > anyway, so it wouldn't belong here in any case). Perhaps this should > just be in ScrollingCoordinator itself, globally, for the whole page. > > https://codereview.chromium.org/2565223002/diff/20001/ > third_party/WebKit/Source/core/layout/compositing/ > CompositingReasonFinder.cpp > File > third_party/WebKit/Source/core/layout/compositing/ > CompositingReasonFinder.cpp > (right): > > https://codereview.chromium.org/2565223002/diff/20001/ > third_party/WebKit/Source/core/layout/compositing/ > CompositingReasonFinder.cpp#newcode186 > third_party/WebKit/Source/core/layout/compositing/ > CompositingReasonFinder.cpp:186: > // is not ideal. When a layout becomes to non-scrollable > nit: "layout becomes to" -> "layer becomes" > > https://codereview.chromium.org/2565223002/diff/20001/ > third_party/WebKit/Source/core/layout/compositing/ > CompositingReasonFinder.cpp#newcode186 > third_party/WebKit/Source/core/layout/compositing/ > CompositingReasonFinder.cpp:186: > // is not ideal. When a layout becomes to non-scrollable > Also, perhaps counterintuitively, I believe all layers created for a > LayoutBox will have a PLSA -- even if they're not scrollable. Perhaps > you just need to make the PLSA decrement any counts it has in dispose() > > https://codereview.chromium.org/2565223002/diff/20001/ > third_party/WebKit/Source/core/layout/compositing/ > CompositingReasonFinder.cpp#newcode212 > third_party/WebKit/Source/core/layout/compositing/ > CompositingReasonFinder.cpp:212: > updateMainThreadScrollingReasonDueToLayoutObject(layer); > This is only meant for Fixed/Sticky layers? The name should make that > clear. > > https://codereview.chromium.org/2565223002/diff/20001/ > third_party/WebKit/Source/web/tests/data/two_transparent_ > scrollable_area.html > File > third_party/WebKit/Source/web/tests/data/two_transparent_ > scrollable_area.html > (right): > > https://codereview.chromium.org/2565223002/diff/20001/ > third_party/WebKit/Source/web/tests/data/two_transparent_ > scrollable_area.html#newcode9 > third_party/WebKit/Source/web/tests/data/two_transparent_ > scrollable_area.html:9: > position: fixed; > Please add another scroller that's transparent but not fixed. And maybe > make one of these sticky? > > https://codereview.chromium.org/2565223002/ > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
Hi David, Thanks for the feedback! Just a quick thought. I am not sure about "A regular scroller won't be main threaded because it's transparent". Without this patch, with http://output.jsbin.com/buziheyila: - The main scroller (s1) has mainThreadScrollingReason: Has non-layer viewport-constrained objects. But when checking the box "scrolling performance issues" in dev_tools, s1 seems NOT to repaint on scroll. - The other scroller (s2) has the same reason but need to repaint on scroll. If we remove the position:fixed property: - S1 doesn't have any mainThreadScrollingReasons and it doesn't repaint on scroll - S2 has the reason: Non fast scrollable region and need to repaint on scroll. If we further remove the content div: - S1 scrolls on impl - S2 doesn't exist It look like s2 will scroll on main as long as it has opacity. The reasons for that vary due to other factors. Maybe I misunderstood something here. Please correct me. BTW, I track the reasons by adding "LOG(ERROR) << "slowScrollingReasons: " << cc::MainThreadScrollingReason::mainThreadScrollingReasonsAsText(reasons);" here <https://cs.chromium.org/chromium/src/ui/events/blink/input_handler_proxy.cc?q...> . Thanks! Yi On Tue, Dec 13, 2016 at 9:38 AM, <bokan@chromium.org> wrote: > > https://codereview.chromium.org/2565223002/diff/20001/cc/ > input/main_thread_scrolling_reason.h > File cc/input/main_thread_scrolling_reason.h (right): > > https://codereview.chromium.org/2565223002/diff/20001/cc/ > input/main_thread_scrolling_reason.h#newcode34 > cc/input/main_thread_scrolling_reason.h:34: kHasOpacity = 1 << 16, > kHasOpacityAndScrollDependent? A regular scroller won't be main threaded > because it's transparent, right? > > https://codereview.chromium.org/2565223002/diff/20001/ > third_party/WebKit/Source/core/frame/FrameView.cpp > File third_party/WebKit/Source/core/frame/FrameView.cpp (right): > > https://codereview.chromium.org/2565223002/diff/20001/ > third_party/WebKit/Source/core/frame/FrameView.cpp#newcode4683 > third_party/WebKit/Source/core/frame/FrameView.cpp:4683: > m_mainThreadScrollingReasonLayoutCounter[reasonIndex(reason)] += > This should never be negative, right? If so, please DCHECK that it > isn't. > > https://codereview.chromium.org/2565223002/diff/20001/ > third_party/WebKit/Source/core/frame/FrameView.h > File third_party/WebKit/Source/core/frame/FrameView.h (right): > > https://codereview.chromium.org/2565223002/diff/20001/ > third_party/WebKit/Source/core/frame/FrameView.h#newcode770 > third_party/WebKit/Source/core/frame/FrameView.h:770: inline int > reasonIndex(uint32_t reason) const { > inline unneeded, this is inline by virtue of being defined in the class > declaration. > > https://codereview.chromium.org/2565223002/diff/20001/ > third_party/WebKit/Source/core/frame/FrameView.h#newcode770 > third_party/WebKit/Source/core/frame/FrameView.h:770: inline int > reasonIndex(uint32_t reason) const { > This should move as a helper into MainThreadScrollingReason.h > > https://codereview.chromium.org/2565223002/diff/20001/ > third_party/WebKit/Source/core/frame/FrameView.h#newcode781 > third_party/WebKit/Source/core/frame/FrameView.h:781: void > adjustMainThreadReasonsDueToLayoutObject(uint32_t reason, bool > increase); > IMO, this should be called adjustMainThreadReasons or > adjustMainThreadReasonsFromStyle > > https://codereview.chromium.org/2565223002/diff/20001/ > third_party/WebKit/Source/core/frame/FrameView.h#newcode783 > third_party/WebKit/Source/core/frame/FrameView.h:783: > MainThreadScrollingReasons > checkMainThreadScrollingReasonDueToLayoutObject(); > check->get, Reason->Reasons > > https://codereview.chromium.org/2565223002/diff/20001/ > third_party/WebKit/Source/core/frame/FrameView.h#newcode1155 > third_party/WebKit/Source/core/frame/FrameView.h:1155: Vector<int> > m_mainThreadScrollingReasonLayoutCounter; > My read is that this vector is tracking scrolling reasons for all > scrollers in a FrameView (other than the FrameView itself) and they all > get combined together for a page-wide bitmask. In which case this > doesn't belong here (we're trying to remove scrolling from FrameView > anyway, so it wouldn't belong here in any case). Perhaps this should > just be in ScrollingCoordinator itself, globally, for the whole page. > > https://codereview.chromium.org/2565223002/diff/20001/ > third_party/WebKit/Source/core/layout/compositing/ > CompositingReasonFinder.cpp > File > third_party/WebKit/Source/core/layout/compositing/ > CompositingReasonFinder.cpp > (right): > > https://codereview.chromium.org/2565223002/diff/20001/ > third_party/WebKit/Source/core/layout/compositing/ > CompositingReasonFinder.cpp#newcode186 > third_party/WebKit/Source/core/layout/compositing/ > CompositingReasonFinder.cpp:186: > // is not ideal. When a layout becomes to non-scrollable > nit: "layout becomes to" -> "layer becomes" > > https://codereview.chromium.org/2565223002/diff/20001/ > third_party/WebKit/Source/core/layout/compositing/ > CompositingReasonFinder.cpp#newcode186 > third_party/WebKit/Source/core/layout/compositing/ > CompositingReasonFinder.cpp:186: > // is not ideal. When a layout becomes to non-scrollable > Also, perhaps counterintuitively, I believe all layers created for a > LayoutBox will have a PLSA -- even if they're not scrollable. Perhaps > you just need to make the PLSA decrement any counts it has in dispose() > > https://codereview.chromium.org/2565223002/diff/20001/ > third_party/WebKit/Source/core/layout/compositing/ > CompositingReasonFinder.cpp#newcode212 > third_party/WebKit/Source/core/layout/compositing/ > CompositingReasonFinder.cpp:212: > updateMainThreadScrollingReasonDueToLayoutObject(layer); > This is only meant for Fixed/Sticky layers? The name should make that > clear. > > https://codereview.chromium.org/2565223002/diff/20001/ > third_party/WebKit/Source/web/tests/data/two_transparent_ > scrollable_area.html > File > third_party/WebKit/Source/web/tests/data/two_transparent_ > scrollable_area.html > (right): > > https://codereview.chromium.org/2565223002/diff/20001/ > third_party/WebKit/Source/web/tests/data/two_transparent_ > scrollable_area.html#newcode9 > third_party/WebKit/Source/web/tests/data/two_transparent_ > scrollable_area.html:9: > position: fixed; > Please add another scroller that's transparent but not fixed. And maybe > make one of these sticky? > > https://codereview.chromium.org/2565223002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/12/13 15:35:11, chromium-reviews wrote: > Hi David, > > Thanks for the feedback! Just a quick thought. I am not sure about "A > regular scroller won't be main threaded > because it's transparent". > Without this patch, with http://output.jsbin.com/buziheyila: S1 is the containing frame and S2 is the <div> scroller, right? > - The main scroller (s1) has mainThreadScrollingReason: Has non-layer > viewport-constrained objects. But when checking the box "scrolling > performance issues" in dev_tools, s1 seems NOT to repaint on scroll. > - The other scroller (s2) has the same reason but need to repaint on > scroll. Presumably they have the same reason because we don't keep reasons per-scroller but, rather, per-frame. > If we remove the position:fixed property: > - S1 doesn't have any mainThreadScrollingReasons and it doesn't repaint on > scroll > - S2 has the reason: Non fast scrollable region and need to repaint on > scroll. > If we further remove the content div: > - S1 scrolls on impl > - S2 doesn't exist > It look like s2 will scroll on main as long as it has opacity. The reasons > for that vary due to other factors. > Maybe I misunderstood something here. Please correct me. > BTW, I track the reasons by adding "LOG(ERROR) << "slowScrollingReasons: " > << > cc::MainThreadScrollingReason::mainThreadScrollingReasonsAsText(reasons);" > here > <https://cs.chromium.org/chromium/src/ui/events/blink/input_handler_proxy.cc?q...> In that case, updateMainThreadScrollingReasonDueToLayoutObject needs to be called from somewhere other than CompositingReasonFinder::requiresCompositingForScrollDependentPosition since that'll early out if its not fixed/sticky. In that case, kOpacity is basically a refinement on the more general kNonFastScrollableRegion? Do you know where that gets set? BTW, are you changing the style via dev tools? I'm not sure I totally trust our tracking of these reasons as PaintLayers get removed. I'd be more certain with changing the bin and reloading the page.
On 2016/12/13 15:56:56, bokan wrote: > On 2016/12/13 15:35:11, chromium-reviews wrote: > > Hi David, > > > > Thanks for the feedback! Just a quick thought. I am not sure about "A > > regular scroller won't be main threaded > > because it's transparent". > > Without this patch, with http://output.jsbin.com/buziheyila: > > S1 is the containing frame and S2 is the <div> scroller, right? Yes. > > - The main scroller (s1) has mainThreadScrollingReason: Has non-layer > > viewport-constrained objects. But when checking the box "scrolling > > performance issues" in dev_tools, s1 seems NOT to repaint on scroll. > > - The other scroller (s2) has the same reason but need to repaint on > > scroll. > > Presumably they have the same reason because we don't keep reasons per-scroller > but, rather, per-frame. > > > If we remove the position:fixed property: > > - S1 doesn't have any mainThreadScrollingReasons and it doesn't repaint on > > scroll > > - S2 has the reason: Non fast scrollable region and need to repaint on > > scroll. > > If we further remove the content div: > > - S1 scrolls on impl > > - S2 doesn't exist > > It look like s2 will scroll on main as long as it has opacity. The reasons > > for that vary due to other factors. > > Maybe I misunderstood something here. Please correct me. > > BTW, I track the reasons by adding "LOG(ERROR) << "slowScrollingReasons: " > > << > > cc::MainThreadScrollingReason::mainThreadScrollingReasonsAsText(reasons);" > > here > > > <https://cs.chromium.org/chromium/src/ui/events/blink/input_handler_proxy.cc?q...> > > In that case, updateMainThreadScrollingReasonDueToLayoutObject needs to be > called from somewhere other than > CompositingReasonFinder::requiresCompositingForScrollDependentPosition since > that'll early out if its not fixed/sticky. In that case, kOpacity is basically a > refinement on the more general kNonFastScrollableRegion? Do you know where that > gets set? Yes. We plan to get detailed reasons of main thread scrolling instead of the broad ones. Currently the broad reasons won't get replaced. > BTW, are you changing the style via dev tools? I'm not sure I totally trust our > tracking of these reasons as PaintLayers get removed. I'd be more certain with > changing the bin and reloading the page. I only use the dev tools to see layers and check "repaint on scroll". I changed the style using the way you suggest. Will work on other of your feedback. Thanks :)
Hi David, I've moved part of the logic out of compositingReasonFinder as you suggested. The vector that tracks reasons is now located in ScrollingCoordinator instead of FrameView. PTAL. Thanks! Yi https://codereview.chromium.org/2565223002/diff/20001/cc/input/main_thread_sc... File cc/input/main_thread_scrolling_reason.h (right): https://codereview.chromium.org/2565223002/diff/20001/cc/input/main_thread_sc... cc/input/main_thread_scrolling_reason.h:34: kHasOpacity = 1 << 16, On 2016/12/13 14:38:56, bokan wrote: > kHasOpacityAndScrollDependent? A regular scroller won't be main threaded because > it's transparent, right? It should scroll on main thread due to blending I guess. https://codereview.chromium.org/2565223002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2565223002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.cpp:4683: m_mainThreadScrollingReasonLayoutCounter[reasonIndex(reason)] += On 2016/12/13 14:38:56, bokan wrote: > This should never be negative, right? If so, please DCHECK that it isn't. Done. https://codereview.chromium.org/2565223002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameView.h (right): https://codereview.chromium.org/2565223002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.h:770: inline int reasonIndex(uint32_t reason) const { On 2016/12/13 14:38:57, bokan wrote: > This should move as a helper into MainThreadScrollingReason.h Done. https://codereview.chromium.org/2565223002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.h:770: inline int reasonIndex(uint32_t reason) const { On 2016/12/13 14:38:57, bokan wrote: > inline unneeded, this is inline by virtue of being defined in the class > declaration. Done. https://codereview.chromium.org/2565223002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.h:781: void adjustMainThreadReasonsDueToLayoutObject(uint32_t reason, bool increase); On 2016/12/13 14:38:56, bokan wrote: > IMO, this should be called adjustMainThreadReasons or > adjustMainThreadReasonsFromStyle Done. https://codereview.chromium.org/2565223002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.h:783: MainThreadScrollingReasons checkMainThreadScrollingReasonDueToLayoutObject(); On 2016/12/13 14:38:56, bokan wrote: > check->get, Reason->Reasons Done. https://codereview.chromium.org/2565223002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.h:1155: Vector<int> m_mainThreadScrollingReasonLayoutCounter; On 2016/12/13 14:38:57, bokan wrote: > My read is that this vector is tracking scrolling reasons for all scrollers in a > FrameView (other than the FrameView itself) and they all get combined together > for a page-wide bitmask. In which case this doesn't belong here (we're trying to > remove scrolling from FrameView anyway, so it wouldn't belong here in any case). > Perhaps this should just be in ScrollingCoordinator itself, globally, for the > whole page. Done. https://codereview.chromium.org/2565223002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/compositing/CompositingReasonFinder.cpp (right): https://codereview.chromium.org/2565223002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/compositing/CompositingReasonFinder.cpp:186: // is not ideal. When a layout becomes to non-scrollable On 2016/12/13 14:38:57, bokan wrote: > nit: "layout becomes to" -> "layer becomes" Done. https://codereview.chromium.org/2565223002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/compositing/CompositingReasonFinder.cpp:186: // is not ideal. When a layout becomes to non-scrollable On 2016/12/13 14:38:57, bokan wrote: > Also, perhaps counterintuitively, I believe all layers created for a LayoutBox > will have a PLSA -- even if they're not scrollable. Perhaps you just need to > make the PLSA decrement any counts it has in dispose() Done. https://codereview.chromium.org/2565223002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/compositing/CompositingReasonFinder.cpp:212: updateMainThreadScrollingReasonDueToLayoutObject(layer); On 2016/12/13 14:38:57, bokan wrote: > This is only meant for Fixed/Sticky layers? The name should make that clear. No. It should be moved somewhere else. https://codereview.chromium.org/2565223002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/data/two_transparent_scrollable_area.html (right): https://codereview.chromium.org/2565223002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/data/two_transparent_scrollable_area.html:9: position: fixed; On 2016/12/13 14:38:57, bokan wrote: > Please add another scroller that's transparent but not fixed. And maybe make one > of these sticky? position:fixed is no longer relevant to this patch. Remove it accordingly.
Thanks, I think I better understand what's going on now. https://codereview.chromium.org/2565223002/diff/20001/cc/input/main_thread_sc... File cc/input/main_thread_scrolling_reason.h (right): https://codereview.chromium.org/2565223002/diff/20001/cc/input/main_thread_sc... cc/input/main_thread_scrolling_reason.h:34: kHasOpacity = 1 << 16, On 2016/12/13 20:54:10, yigu wrote: > On 2016/12/13 14:38:56, bokan wrote: > > kHasOpacityAndScrollDependent? A regular scroller won't be main threaded > because > > it's transparent, right? > > It should scroll on main thread due to blending I guess. Ah, of course, if it's transparent and low-DPI, we won't promote the scroller because we'd lose LCD text. This means though you need to add a check when setting the kHasOpacity that the scroller actually isn't composited. If you try the current patch on a HiDPI monitor it'll scroll on impl but you'll set this flag. You can do that by checking PaintLayerCompositor::preferCompositingToLCDText, if that's false, a transparent layer will lose composited scrolling. https://codereview.chromium.org/2565223002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp (right): https://codereview.chromium.org/2565223002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:256: updateMainThreadScrollingReasonsFromStyle(layer, scrollingCoordinator); No need to pass in ScrollingCoordinator, just get it inside the method. https://codereview.chromium.org/2565223002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.h (right): https://codereview.chromium.org/2565223002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.h:30: #include "core/page/scrolling/ScrollingCoordinator.h" Forward declare rather than include. https://codereview.chromium.org/2565223002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp (right): https://codereview.chromium.org/2565223002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:1240: DCHECK_GE(m_mainThreadScrollingReasonLayoutCounter[index], 0); This check should be below the following line. https://codereview.chromium.org/2565223002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.h (right): https://codereview.chromium.org/2565223002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.h:193: Vector<int> m_mainThreadScrollingReasonLayoutCounter; ReasonLayoutCounter -> ReasonsCounter https://codereview.chromium.org/2565223002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/2565223002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:161: scrollingCoordinator->willDestroyScrollableArea(this); Actually, since we already have this willDestroyScrollableArea in ScrollingCoordinator, it makes more sense for the above code to go in there since its specific to ScrollingCoordinator. https://codereview.chromium.org/2565223002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h (right): https://codereview.chromium.org/2565223002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h:587: // MainThreadScrollingReason due to the property of each layout object "property of each layout object" -> "properties of the LayoutObject" https://codereview.chromium.org/2565223002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/ScrollingCoordinatorTest.cpp (right): https://codereview.chromium.org/2565223002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/ScrollingCoordinatorTest.cpp:862: // Remove opacity from one of the layouts. layouts->scrollers
Hi David, I have moved the logic of resetting ReasonCounter to ScrollingCoordinator. PTAL. Thanks! https://codereview.chromium.org/2565223002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp (right): https://codereview.chromium.org/2565223002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:256: updateMainThreadScrollingReasonsFromStyle(layer, scrollingCoordinator); On 2016/12/14 13:28:58, bokan wrote: > No need to pass in ScrollingCoordinator, just get it inside the method. Done. https://codereview.chromium.org/2565223002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.h (right): https://codereview.chromium.org/2565223002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.h:30: #include "core/page/scrolling/ScrollingCoordinator.h" On 2016/12/14 13:28:58, bokan wrote: > Forward declare rather than include. Done. https://codereview.chromium.org/2565223002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp (right): https://codereview.chromium.org/2565223002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:1240: DCHECK_GE(m_mainThreadScrollingReasonLayoutCounter[index], 0); On 2016/12/14 13:28:59, bokan wrote: > This check should be below the following line. Done. https://codereview.chromium.org/2565223002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.h (right): https://codereview.chromium.org/2565223002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.h:193: Vector<int> m_mainThreadScrollingReasonLayoutCounter; On 2016/12/14 13:28:59, bokan wrote: > ReasonLayoutCounter -> ReasonsCounter Done. https://codereview.chromium.org/2565223002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/2565223002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:161: scrollingCoordinator->willDestroyScrollableArea(this); On 2016/12/14 13:28:59, bokan wrote: > Actually, since we already have this willDestroyScrollableArea in > ScrollingCoordinator, it makes more sense for the above code to go in there > since its specific to ScrollingCoordinator. Done. https://codereview.chromium.org/2565223002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h (right): https://codereview.chromium.org/2565223002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h:587: // MainThreadScrollingReason due to the property of each layout object On 2016/12/14 13:28:59, bokan wrote: > "property of each layout object" -> "properties of the LayoutObject" Done. https://codereview.chromium.org/2565223002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/ScrollingCoordinatorTest.cpp (right): https://codereview.chromium.org/2565223002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/ScrollingCoordinatorTest.cpp:862: // Remove opacity from one of the layouts. On 2016/12/14 13:28:59, bokan wrote: > layouts->scrollers Done.
https://codereview.chromium.org/2565223002/diff/20001/cc/input/main_thread_sc... File cc/input/main_thread_scrolling_reason.h (right): https://codereview.chromium.org/2565223002/diff/20001/cc/input/main_thread_sc... cc/input/main_thread_scrolling_reason.h:34: kHasOpacity = 1 << 16, On 2016/12/14 13:28:58, bokan wrote: > On 2016/12/13 20:54:10, yigu wrote: > > On 2016/12/13 14:38:56, bokan wrote: > > > kHasOpacityAndScrollDependent? A regular scroller won't be main threaded > > because > > > it's transparent, right? > > > > It should scroll on main thread due to blending I guess. > > Ah, of course, if it's transparent and low-DPI, we won't promote the scroller > because we'd lose LCD text. This means though you need to add a check when > setting the kHasOpacity that the scroller actually isn't composited. If you try > the current patch on a HiDPI monitor it'll scroll on impl but you'll set this > flag. You can do that by checking > PaintLayerCompositor::preferCompositingToLCDText, if that's false, a transparent > layer will lose composited scrolling. Hey Yi, I think this still hasn't been addressed. I think a scroller with opacity will scroll on impl thread if you're on a high-dpi monitor or pass --prefer-compositing-to-lcd-text. Could you confirm that's true, and if so, you'll need to adjust the condition under which we set this MainThreadScrollingReason https://codereview.chromium.org/2565223002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp (right): https://codereview.chromium.org/2565223002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:565: if (ScrollingCoordinator* scrollingCoordinator = Should we early out above if there's no scrolling coordinator? Seems like we shouldn't set the scrolling reason on the PaintLayer unless we do it on the ScrollingCoordinator too (otherwise if we add one after they'll be out of sync)
Hi David, I have added a check about preferCompositingToLCDText. PTAL. Thanks! https://codereview.chromium.org/2565223002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp (right): https://codereview.chromium.org/2565223002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:565: if (ScrollingCoordinator* scrollingCoordinator = On 2016/12/15 20:47:45, bokan wrote: > Should we early out above if there's no scrolling coordinator? Seems like we > shouldn't set the scrolling reason on the PaintLayer unless we do it on the > ScrollingCoordinator too (otherwise if we add one after they'll be out of sync) Done.
Thanks, almost there :) https://codereview.chromium.org/2565223002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp (right): https://codereview.chromium.org/2565223002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:256: updateMainThreadScrollingReasonsFromStyle(layer); Sorry, one last thing. I think this is the wrong place for this. CompositingRequirementsUpdater is collecting reasons we *should* composite. This looks like it rather belongs in PaintLayerScrollableArea's layerNeedsCompositedScrolling, here particularily, where we return false: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/paint/Pai... https://codereview.chromium.org/2565223002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:561: if (ScrollingCoordinator* scrollingCoordinator = nit: we generally prefer the above pattern of early-return on null to prevent excessive indenting.
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Hi David, I've moved the updater to PLSA. I am not checking LCD this time since it has been checked in layerNeedsCompositedScrolling, right? There are some layout tests failure on mac_chromium_rel_ng. But it seems the master branch would have those failures as well. They are all webgl related. PTAL. Thanks! Yi https://codereview.chromium.org/2565223002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp (right): https://codereview.chromium.org/2565223002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:256: updateMainThreadScrollingReasonsFromStyle(layer); On 2016/12/16 23:06:01, bokan wrote: > Sorry, one last thing. I think this is the wrong place for this. > CompositingRequirementsUpdater is collecting reasons we *should* composite. This > looks like it rather belongs in PaintLayerScrollableArea's > layerNeedsCompositedScrolling, here particularily, where we return false: > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/paint/Pai... Done. https://codereview.chromium.org/2565223002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:561: if (ScrollingCoordinator* scrollingCoordinator = On 2016/12/16 23:06:01, bokan wrote: > nit: we generally prefer the above pattern of early-return on null to prevent > excessive indenting. Done.
https://codereview.chromium.org/2565223002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/2565223002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1743: return false; I'd prefer we call updateMainThreadScrollingReasonsFromStyle here. The reason being, I'm a little worried about having it far away from the place where we make the "real" decision about main thread scrolling - if someone updates the conditions here, they'll likely forget to make a corresponding change below. If the call is made here just before we return false, we're guaranteed to only set the main thread reason if we're actually main thread scrolling. You can merge this static method directly into PLSA::updateNeedsCompositedScrolling. It'd also be better if you passed the MainThreadScrollingReason from here into updateMainThreadScrollingReasonsFromStyle rather than recomputing it there. e.g. if (...) { if (layer->compositesWithOpacity()) { updateMainThreadScrollingReasonsFromStyle(layer, MTSR::kHasOpacity, true); } return false; } else { updateMainThreadScrollingReasonsFromStyle(layer, MTSR::kHasOpacity, false); } https://codereview.chromium.org/2565223002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1756: void PaintLayerScrollableArea::updateMainThreadScrollingReasonsFromStyle( Sorry to waffle on the name again, but after seeing it a bunch of times I think updateStyleRelatedMainThreadScrollingReasons feels less awkward. https://codereview.chromium.org/2565223002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1760: if (!layer || !layer->getScrollableArea()) Now that we're in PLSA you don't have to check for layer or scrollable area (you can DCHECK(layer())) https://codereview.chromium.org/2565223002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1768: layer->getScrollableArea()->hasMainThreadScrollingReason( layer->getScrollableArea() isn't needed anymore.
Hi David, New patch regarding the refactoring is on. PTAL. Thanks! Yi https://codereview.chromium.org/2565223002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/2565223002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1743: return false; On 2016/12/19 14:54:25, bokan wrote: > I'd prefer we call updateMainThreadScrollingReasonsFromStyle here. The reason > being, I'm a little worried about having it far away from the place where we > make the "real" decision about main thread scrolling - if someone updates the > conditions here, they'll likely forget to make a corresponding change below. If > the call is made here just before we return false, we're guaranteed to only set > the main thread reason if we're actually main thread scrolling. > > You can merge this static method directly into > PLSA::updateNeedsCompositedScrolling. It'd also be better if you passed the > MainThreadScrollingReason from here into > updateMainThreadScrollingReasonsFromStyle rather than recomputing it there. e.g. > > if (...) { > if (layer->compositesWithOpacity()) { > updateMainThreadScrollingReasonsFromStyle(layer, MTSR::kHasOpacity, > true); > } > return false; > } else { > updateMainThreadScrollingReasonsFromStyle(layer, MTSR::kHasOpacity, > false); > } > Since we clear all reasons for a layer at the beginning of this function, there is no need to update with "false". https://codereview.chromium.org/2565223002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1756: void PaintLayerScrollableArea::updateMainThreadScrollingReasonsFromStyle( On 2016/12/19 14:54:25, bokan wrote: > Sorry to waffle on the name again, but after seeing it a bunch of times I think > updateStyleRelatedMainThreadScrollingReasons feels less awkward. Done. https://codereview.chromium.org/2565223002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1760: if (!layer || !layer->getScrollableArea()) On 2016/12/19 14:54:25, bokan wrote: > Now that we're in PLSA you don't have to check for layer or scrollable area (you > can DCHECK(layer())) Done. https://codereview.chromium.org/2565223002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1768: layer->getScrollableArea()->hasMainThreadScrollingReason( On 2016/12/19 14:54:25, bokan wrote: > layer->getScrollableArea() isn't needed anymore. Done.
https://codereview.chromium.org/2565223002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp (right): https://codereview.chromium.org/2565223002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:1234: // Decrese the number of layer that has any main thread nit: layer->layers, has->have https://codereview.chromium.org/2565223002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:1241: if (paintLayerScrollableArea->hasMainThreadScrollingReason(1 << i)) { Nit: might be clearer to store and reuse 1 << i https://codereview.chromium.org/2565223002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:1242: paintLayerScrollableArea->flipMainThreadScrollingReason(1 << i); You have adding a reason go through PLSA and call into ScrollingCoordinator but removing go through ScrollingCoordinator and adjust the PLSA. They should work the same way. IMO, this method should be on PLSA and decrement the ScrollingCoordinator counts from there. https://codereview.chromium.org/2565223002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.h (right): https://codereview.chromium.org/2565223002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.h:102: void removeStyleRelatedMainThreadScrollingReasonsForLayer(ScrollableArea*); "ForLayer" is redundant, just leave it off. https://codereview.chromium.org/2565223002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/2565223002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1731: getScrollingCoordinator() Doing this in here means that calling what looks like a simple getter that should be "const" actually mutates the object. That's unexpected. Simplest solution would be to just inline this function directly into updateNeedsCompositedScrolling where mutation is expected based on the function name. https://codereview.chromium.org/2565223002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1773: // Check if current layer has layout object that contains properties that cause This comment is no longer accurate. https://codereview.chromium.org/2565223002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1777: // If current layer has opacity and has not been recorded, add the layer into This comment can be removed, IMO. https://codereview.chromium.org/2565223002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1784: flipMainThreadScrollingReason(reason); Rather than "flip", this should now be "set" and take a bool. Right now, calling add twice will remove it the second time (but add it twice in ScrollingCoordinator).
Hi David, I have some feedback. Please take a look. Thanks! Yi https://codereview.chromium.org/2565223002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp (right): https://codereview.chromium.org/2565223002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:1234: // Decrese the number of layer that has any main thread On 2016/12/19 21:25:14, bokan wrote: > nit: layer->layers, has->have Done. https://codereview.chromium.org/2565223002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:1241: if (paintLayerScrollableArea->hasMainThreadScrollingReason(1 << i)) { On 2016/12/19 21:25:14, bokan wrote: > Nit: might be clearer to store and reuse 1 << i Done. https://codereview.chromium.org/2565223002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:1242: paintLayerScrollableArea->flipMainThreadScrollingReason(1 << i); On 2016/12/19 21:25:14, bokan wrote: > You have adding a reason go through PLSA and call into ScrollingCoordinator but > removing go through ScrollingCoordinator and adjust the PLSA. They should work > the same way. IMO, this method should be on PLSA and decrement the > ScrollingCoordinator counts from there. Done. https://codereview.chromium.org/2565223002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.h (right): https://codereview.chromium.org/2565223002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.h:102: void removeStyleRelatedMainThreadScrollingReasonsForLayer(ScrollableArea*); On 2016/12/19 21:25:14, bokan wrote: > "ForLayer" is redundant, just leave it off. Done. https://codereview.chromium.org/2565223002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/2565223002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1731: getScrollingCoordinator() On 2016/12/19 21:25:15, bokan wrote: > Doing this in here means that calling what looks like a simple getter that > should be "const" actually mutates the object. That's unexpected. Simplest > solution would be to just inline this function directly into > updateNeedsCompositedScrolling where mutation is expected based on the function > name. Agreed! Is it OK to use ScrollingCoordinator* scrollingCoordinator = getScrollingCoordinator(); then scrollingCoordinator->adjust* ? I saw something similar in dispose(). https://codereview.chromium.org/2565223002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1773: // Check if current layer has layout object that contains properties that cause On 2016/12/19 21:25:14, bokan wrote: > This comment is no longer accurate. Done. https://codereview.chromium.org/2565223002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1777: // If current layer has opacity and has not been recorded, add the layer into On 2016/12/19 21:25:14, bokan wrote: > This comment can be removed, IMO. Done. https://codereview.chromium.org/2565223002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1784: flipMainThreadScrollingReason(reason); On 2016/12/19 21:25:15, bokan wrote: > Rather than "flip", this should now be "set" and take a bool. Right now, calling > add twice will remove it the second time (but add it twice in > ScrollingCoordinator). I am not sure about this. Since we remove all existed reasons before each addStyleRelatedMainThreadScrollingReasons(..), flip in this function should never remove it. Right? Same for the one used in "remove". To make sure that removing works, we only "flip" it if we "has" it.
https://codereview.chromium.org/2565223002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/2565223002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1731: getScrollingCoordinator() On 2016/12/20 01:41:20, yigu wrote: > On 2016/12/19 21:25:15, bokan wrote: > > Doing this in here means that calling what looks like a simple getter that > > should be "const" actually mutates the object. That's unexpected. Simplest > > solution would be to just inline this function directly into > > updateNeedsCompositedScrolling where mutation is expected based on the > function > > name. > > Agreed! Is it OK to use ScrollingCoordinator* scrollingCoordinator = > getScrollingCoordinator(); then scrollingCoordinator->adjust* ? > I saw something similar in dispose(). Yah, unless I'm misunderstanding your question. My comment was about folding this into the updateNeedsCompositedScrolling method so someone in the future isn't surprised that calling a method like this makes changes locally or in ScrollingCoordinator. I don't see a problem with this call on ScrollingCoordinator as is (just that a caller wouldn't expect it from a method named like this). https://codereview.chromium.org/2565223002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1784: flipMainThreadScrollingReason(reason); On 2016/12/20 01:41:20, yigu wrote: > On 2016/12/19 21:25:15, bokan wrote: > > Rather than "flip", this should now be "set" and take a bool. Right now, > calling > > add twice will remove it the second time (but add it twice in > > ScrollingCoordinator). > > I am not sure about this. Since we remove all existed reasons before each > addStyleRelatedMainThreadScrollingReasons(..), flip in this function should > never remove it. Right? Same for the one used in "remove". To make sure that > removing works, we only "flip" it if we "has" it. That might be true today, but code often evolves in ways we don't predict :). Someone might want to later add a reason from another place in code for which this may not be true (and may miss this fact by reading the code, after all, the method says "add"). You could just return if the reason already exists and that would make it correct. IMO, there's no reason to prefer the "flip" approach though since all you're ever doing is setting and removing, you might as well make the underlying method support that directly.
Hi David. I landed another CL which makes mainThreadScrollingReason frame based rather than page based. The logic has been moved from ScrollingCoordinator to FrameView. I made some relevant changes to current CL. PTAL
https://codereview.chromium.org/2565223002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/2565223002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1754: addStyleRelatedMainThreadScrollingReasons( This still has the issue I brought up in the last patchset. There's only one place this method is called from - updateNeedsCompositedScrolling which is only a few lines long. Just inline this entire method in there. https://codereview.chromium.org/2565223002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h (right): https://codereview.chromium.org/2565223002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h:484: void setMainThreadScrollingReason(uint32_t reason) { m_reasons |= reason; } These aren't called from outside the class and only called in one place inside. Remove the set and reset method and just inline the `|=` and `&=` inside addStyle../removeStyle... where they're used.
FYI, for future reviews, if you have multiple patches that depend on each other, you can set set one as the upstream e.g. git checkout -b Patch1 ...do some work... git checkout -b Patch2 //continues work in Patch1 git branch --set-upstream-to=Patch1 Now when you git cl upload from branch Patch2, it'll upload just the diff between Patch1 and Patch2 and let you can run trybots as well. It requires a bit of work to rebase changes as you work on the two but your reviewers can see what the final state will look like and should speed things up.
Hi David, Made the changes as you suggested. Thanks! Yi https://codereview.chromium.org/2565223002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/2565223002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1754: addStyleRelatedMainThreadScrollingReasons( On 2016/12/20 19:35:53, bokan wrote: > This still has the issue I brought up in the last patchset. There's only one > place this method is called from - updateNeedsCompositedScrolling which is only > a few lines long. Just inline this entire method in there. Renamed the method to computeNeedsCompositedScrolling to be clear. https://codereview.chromium.org/2565223002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h (right): https://codereview.chromium.org/2565223002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h:484: void setMainThreadScrollingReason(uint32_t reason) { m_reasons |= reason; } On 2016/12/20 19:35:53, bokan wrote: > These aren't called from outside the class and only called in one place inside. > Remove the set and reset method and just inline the `|=` and `&=` inside > addStyle../removeStyle... where they're used. Done.
Please add another test but otherwise lgtm! https://codereview.chromium.org/2565223002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/tests/ScrollingCoordinatorTest.cpp (right): https://codereview.chromium.org/2565223002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/ScrollingCoordinatorTest.cpp:1035: webViewImpl()->settings()->setPreferCompositingToLCDTextEnabled(false); Please add another test where setPreferCompositingToLCDTextEnabled is true and make sure that we don't set a main thread scrolling reason. https://codereview.chromium.org/2565223002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/ScrollingCoordinatorTest.cpp:1055: // Remove opacity from the other layout would lead to nit: layout->scroller
Thanks David :) https://codereview.chromium.org/2565223002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/tests/ScrollingCoordinatorTest.cpp (right): https://codereview.chromium.org/2565223002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/ScrollingCoordinatorTest.cpp:1035: webViewImpl()->settings()->setPreferCompositingToLCDTextEnabled(false); On 2016/12/20 20:09:07, bokan wrote: > Please add another test where setPreferCompositingToLCDTextEnabled is true and > make sure that we don't set a main thread scrolling reason. Done. https://codereview.chromium.org/2565223002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/ScrollingCoordinatorTest.cpp:1055: // Remove opacity from the other layout would lead to On 2016/12/20 20:09:07, bokan wrote: > nit: layout->scroller Done.
yigu@chromium.org changed reviewers: + weiliangc@chromium.org
cc/ LGTM
yigu@chromium.org changed reviewers: + jwd@chromium.org
lgtm
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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 Link to the patchset: https://codereview.chromium.org/2565223002/#ps180001 (title: "Test that there should be no reason set if setPrefercompositingToCDLTextEnabled(true)")
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": 180001, "attempt_start_ts": 1482278588135680,
"parent_rev": "88efa9ce9bcd4ccfd61f098435cebf5fe95017e1", "commit_rev":
"d7ed3651b793314c5613122f0e15545185315c73"}
Message was sent while issue was closed.
Description was changed from ========== We should replace the broad main thread reasons of non layer viewport constrained objects and non fast scrollable reason with the more specific reasons which are preventing us from compositing them so that we can prioritize. BUG=660907 TEST=ScrollingCoordinatorTest.MainThreadScrollingReasonDueToLayoutObject CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== We should replace the broad main thread reasons of non layer viewport constrained objects and non fast scrollable reason with the more specific reasons which are preventing us from compositing them so that we can prioritize. BUG=660907 TEST=ScrollingCoordinatorTest.MainThreadScrollingReasonDueToLayoutObject CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2565223002 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== We should replace the broad main thread reasons of non layer viewport constrained objects and non fast scrollable reason with the more specific reasons which are preventing us from compositing them so that we can prioritize. BUG=660907 TEST=ScrollingCoordinatorTest.MainThreadScrollingReasonDueToLayoutObject CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2565223002 ========== to ========== We should replace the broad main thread reasons of non layer viewport constrained objects and non fast scrollable reason with the more specific reasons which are preventing us from compositing them so that we can prioritize. BUG=660907 TEST=ScrollingCoordinatorTest.MainThreadScrollingReasonDueToLayoutObject CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Committed: https://crrev.com/e92b679fee2d010570bb1ea1896b443d01eb5a84 Cr-Commit-Position: refs/heads/master@{#439944} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/e92b679fee2d010570bb1ea1896b443d01eb5a84 Cr-Commit-Position: refs/heads/master@{#439944}
Message was sent while issue was closed.
pdr@chromium.org changed reviewers: + pdr@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2565223002/diff/180001/cc/input/main_thread_s... File cc/input/main_thread_scrolling_reason.h (right): https://codereview.chromium.org/2565223002/diff/180001/cc/input/main_thread_s... cc/input/main_thread_scrolling_reason.h:34: kHasOpacity = 1 << 16, Why does opacity (and later patches: transform) prevent composited scrolling? Is it due to how opacity and lcd text interact?
Message was sent while issue was closed.
https://codereview.chromium.org/2565223002/diff/180001/cc/input/main_thread_s... File cc/input/main_thread_scrolling_reason.h (right): https://codereview.chromium.org/2565223002/diff/180001/cc/input/main_thread_s... cc/input/main_thread_scrolling_reason.h:34: kHasOpacity = 1 << 16, On 2017/01/07 22:25:25, pdr. wrote: > Why does opacity (and later patches: transform) prevent composited scrolling? Is > it due to how opacity and lcd text interact? Right, I think it's because in order to do LCD text antialiasing, you need to know what's behind the text. Opacity means other layers in CC might change what's behind the text. Transforms I'm less sure, I'm guessing some transforms might "ruin" the antialiasing (e.g. perspective might distort the subpixel adjustments across multiple pixels)? Just to clarify, these patches are just adding metrics to existing compositing outs rather than actually changing what's getting composited.
Message was sent while issue was closed.
https://codereview.chromium.org/2565223002/diff/180001/cc/input/main_thread_s... File cc/input/main_thread_scrolling_reason.h (right): https://codereview.chromium.org/2565223002/diff/180001/cc/input/main_thread_s... cc/input/main_thread_scrolling_reason.h:34: kHasOpacity = 1 << 16, On 2017/01/09 at 13:29:08, bokan wrote: > On 2017/01/07 22:25:25, pdr. wrote: > > Why does opacity (and later patches: transform) prevent composited scrolling? Is > > it due to how opacity and lcd text interact? > > Right, I think it's because in order to do LCD text antialiasing, you need to know what's behind the text. Opacity means other layers in CC might change what's behind the text. Transforms I'm less sure, I'm guessing some transforms might "ruin" the antialiasing (e.g. perspective might distort the subpixel adjustments across multiple pixels)? Just to clarify, these patches are just adding metrics to existing compositing outs rather than actually changing what's getting composited. Thanks for the explanation. Would you be okay with changing kHasOpacity to kHasOpacityAndLCDText? I can put up a patch to do that. Also, aren't we using these flags for real performance changes by forcing the main thread? If not, why don't we put them on a different structure instead of re-using MainThreadScrollingReason?
Message was sent while issue was closed.
bokan@chromium.org changed reviewers: + flackr@chromium.org
Message was sent while issue was closed.
+flackr@ who's been OOO but should have more context here and is back now. https://codereview.chromium.org/2565223002/diff/180001/cc/input/main_thread_s... File cc/input/main_thread_scrolling_reason.h (right): https://codereview.chromium.org/2565223002/diff/180001/cc/input/main_thread_s... cc/input/main_thread_scrolling_reason.h:34: kHasOpacity = 1 << 16, On 2017/01/09 17:22:12, pdr. wrote: > On 2017/01/09 at 13:29:08, bokan wrote: > > On 2017/01/07 22:25:25, pdr. wrote: > > > Why does opacity (and later patches: transform) prevent composited > scrolling? Is > > > it due to how opacity and lcd text interact? > > > > Right, I think it's because in order to do LCD text antialiasing, you need to > know what's behind the text. Opacity means other layers in CC might change > what's behind the text. Transforms I'm less sure, I'm guessing some transforms > might "ruin" the antialiasing (e.g. perspective might distort the subpixel > adjustments across multiple pixels)? Just to clarify, these patches are just > adding metrics to existing compositing outs rather than actually changing what's > getting composited. > > Thanks for the explanation. Would you be okay with changing kHasOpacity to > kHasOpacityAndLCDText? I can put up a patch to do that. Yeah, that sounds clearer. sgtm > Also, aren't we using these flags for real performance changes by forcing the > main thread? If not, why don't we put them on a different structure instead of > re-using MainThreadScrollingReason? For these newly added flags, my understanding is that we wouldn't have created a cc::Layer in these cases at all so there was nothing to put the reason onto. We're now storing these on the ScrollableArea as well as the cc::Layer but (on main thread) it's being used to track stats rather than making compositing decisions. |
