Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2822653003:
[SPInvalidation] Micro-optimize PaintLayerClipper::calculateRects (Closed)
Description[SPInvalidation] Micro-optimize PaintLayerClipper::calculateRects
According to this try job:
https://console.developers.google.com/m/cloudstorage/b/chromium-telemetry/o/html-results/results-2017-04-14_14-14-12
we observe a 5% improvement in caching-disabled performance.
BUG=699198
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Review-Url: https://codereview.chromium.org/2822653003
Cr-Commit-Position: refs/heads/master@{#465005}
Committed: https://chromium.googlesource.com/chromium/src/+/44b96aec9c8b09356c59a7153c6bd7cbb4f9d69f
Patch Set 1 #
Total comments: 7
Patch Set 2 : none #Patch Set 3 : none #Patch Set 4 : none #Patch Set 5 : none #Patch Set 6 : none #
Messages
Total messages: 28 (20 generated)
Description was changed from ========== none BUG= ========== to ========== none BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Description was changed from ========== none BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== For a perf tryjob. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Description was changed from ========== For a perf tryjob. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== For a perf tryjob. BUG=699198 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Description was changed from ========== For a perf tryjob. BUG=699198 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== [SPInvalidation] Micro-optimize PaintLayerClipper::calculateRects According to this try job: https://console.developers.google.com/m/cloudstorage/b/chromium-telemetry/o/h... we observe a 5% improvement in caching-disabled performance. BUG=699198 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
chrishtr@chromium.org changed reviewers: + trchen@chromium.org
The CQ bit was checked by chrishtr@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/2822653003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/paint/PaintLayerClipper.cpp (right): https://codereview.chromium.org/2822653003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintLayerClipper.cpp:291: .OverflowClipRect(offset, context.overlay_scrollbar_clip_behavior); Should it be (offset + context.sub_pixel_accumulation)? https://codereview.chromium.org/2822653003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintLayerClipper.cpp:297: if (layout_object.HasClip()) { nit: When constructed from bottom-up, apply CSS clip before overflow clip. https://codereview.chromium.org/2822653003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintLayerClipper.cpp:298: LayoutRect new_pos_clip = ToLayoutBox(layout_object).ClipRect(offset); Ditto. (offset + context.sub_pixel_accumulation)?
https://codereview.chromium.org/2822653003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/paint/PaintLayerClipper.cpp (right): https://codereview.chromium.org/2822653003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintLayerClipper.cpp:291: .OverflowClipRect(offset, context.overlay_scrollbar_clip_behavior); On 2017/04/14 at 23:35:35, trchen wrote: > Should it be (offset + context.sub_pixel_accumulation)? Do you mean I should adjust OverflowClipRect() by an additional sub_pixel_accumulation? It seems not if one is trying to copy non-GM behavior in CalculateRects(), which does not do that I believe. Maybe that behavior is incorrect? https://codereview.chromium.org/2822653003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintLayerClipper.cpp:297: if (layout_object.HasClip()) { On 2017/04/14 at 23:35:35, trchen wrote: > nit: When constructed from bottom-up, apply CSS clip before overflow clip. Done, except now it technically disagrees with non-GM. :) https://codereview.chromium.org/2822653003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintLayerClipper.cpp:298: LayoutRect new_pos_clip = ToLayoutBox(layout_object).ClipRect(offset); On 2017/04/14 at 23:35:35, trchen wrote: > Ditto. (offset + context.sub_pixel_accumulation)? Same answer as above.
The CQ bit was checked by chrishtr@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/2822653003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/paint/PaintLayerClipper.cpp (right): https://codereview.chromium.org/2822653003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintLayerClipper.cpp:291: .OverflowClipRect(offset, context.overlay_scrollbar_clip_behavior); On 2017/04/14 23:51:56, chrishtr wrote: > On 2017/04/14 at 23:35:35, trchen wrote: > > Should it be (offset + context.sub_pixel_accumulation)? > > Do you mean I should adjust OverflowClipRect() by an additional > sub_pixel_accumulation? It seems not if one is trying to copy > non-GM behavior in CalculateRects(), which does not do that I > believe. Maybe that behavior is incorrect? Hmmmmm. That looks very weird indeed. The non-GM version seemed to me that every time we inherit the parent layer's foreground rect, we shift it by context.sub_pixel_accumulation. (Feels wrong, and doesn't match this CL's version either.) If I understand it correctly, context.sub_pixel_accumulation is the difference between root layer origin and backing origin (i.e. paint offset for root layer). I feel it is cleaner to just change #269 to: LayoutPoint offset = context.sub_pixel_accumulation; Then the question becomes whether *offset_from_root already included context.sub_pixel_accumulation. :/
On 2017/04/15 at 00:09:51, trchen wrote: > https://codereview.chromium.org/2822653003/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/paint/PaintLayerClipper.cpp (right): > > https://codereview.chromium.org/2822653003/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/paint/PaintLayerClipper.cpp:291: .OverflowClipRect(offset, context.overlay_scrollbar_clip_behavior); > On 2017/04/14 23:51:56, chrishtr wrote: > > On 2017/04/14 at 23:35:35, trchen wrote: > > > Should it be (offset + context.sub_pixel_accumulation)? > > > > Do you mean I should adjust OverflowClipRect() by an additional > > sub_pixel_accumulation? It seems not if one is trying to copy > > non-GM behavior in CalculateRects(), which does not do that I > > believe. Maybe that behavior is incorrect? > > Hmmmmm. That looks very weird indeed. > > The non-GM version seemed to me that every time we inherit the parent layer's foreground rect, we shift it by context.sub_pixel_accumulation. (Feels wrong, and doesn't match this CL's version either.) > > If I understand it correctly, context.sub_pixel_accumulation is the difference between root layer origin and backing origin (i.e. paint offset for root layer). I feel it is cleaner to just change #269 to: > LayoutPoint offset = context.sub_pixel_accumulation; > Done. I did it for the GM and non-GM version. > Then the question becomes whether *offset_from_root already included context.sub_pixel_accumulation. :/ I checked all callsites and the paint ones either pass nullptr or a LayoutRect that includes subpixel. Therefore the old code was wrong! Your thorough review found not just this bug but also a bug in LayoutSVGRoot. Left a comment and will follow up on that in another patch.
The CQ bit was checked by chrishtr@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_chromium_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 chrishtr@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.
This lgtm. Thank you for making everything right!
The CQ bit was checked by chrishtr@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1492455448253770,
"parent_rev": "ef07b23fe53c8640375d2d6421c22fc50b94cf9e", "commit_rev":
"44b96aec9c8b09356c59a7153c6bd7cbb4f9d69f"}
Message was sent while issue was closed.
Description was changed from ========== [SPInvalidation] Micro-optimize PaintLayerClipper::calculateRects According to this try job: https://console.developers.google.com/m/cloudstorage/b/chromium-telemetry/o/h... we observe a 5% improvement in caching-disabled performance. BUG=699198 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== [SPInvalidation] Micro-optimize PaintLayerClipper::calculateRects According to this try job: https://console.developers.google.com/m/cloudstorage/b/chromium-telemetry/o/h... we observe a 5% improvement in caching-disabled performance. BUG=699198 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2822653003 Cr-Commit-Position: refs/heads/master@{#465005} Committed: https://chromium.googlesource.com/chromium/src/+/44b96aec9c8b09356c59a7153c6b... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/44b96aec9c8b09356c59a7153c6b... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
