Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2883153002:
Don't bother checking for paint properties for classes that never need them. (Closed)
DescriptionDon't bother checking for paint properties for classes that never need them.
In local testing,
PerformanceTests/Paint/complex-content-slow-scroll.html improves by about
10%.
BUG=718045
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Review-Url: https://codereview.chromium.org/2883153002
Cr-Commit-Position: refs/heads/master@{#471952}
Committed: https://chromium.googlesource.com/chromium/src/+/b717e22b72275498d43183016392b2ae0a011b3e
Patch Set 1 #Patch Set 2 : none #
Total comments: 6
Patch Set 3 : none #Messages
Total messages: 17 (9 generated)
Description was changed from ========== none none BUG= ========== to ========== none none BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Description was changed from ========== none none BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Don't bother checking for paint properties for classes that never need them. BUG=718045 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
chrishtr@chromium.org changed reviewers: + pdr@chromium.org
This shows a significant performance improvement in local testing.
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/2883153002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right): https://codereview.chromium.org/2883153002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:1211: return object.IsBoxModelObject() || object.IsSVG(); Why is IsSVG() needed? https://codereview.chromium.org/2883153002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:1225: bool might_need_paint_properties = MightNeedPaintProperties(object); It's important that this does not change between updates. In other words, a layoutobject cannot change from not might-need-paint-properties to needing paint properties. I think it would be useful if this variable and function reflected that. Something like "ObjectTypeMightNeedPaintProperties"? https://codereview.chromium.org/2883153002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:1227: if (might_need_paint_properties) Is the perf win just reducing the calls to this fn by one?
On 2017/05/15 at 21:21:50, chrishtr wrote: > This shows a significant performance improvement in local testing. Can you quantify this and include the command line in the change description?
On 2017/05/15 at 21:33:04, pdr wrote: > On 2017/05/15 at 21:21:50, chrishtr wrote: > > This shows a significant performance improvement in local testing. > > Can you quantify this and include the command line in the change description? The values varied a lot from run to run, but it seemed to be 10% or so. I looked at the values output when loading Paint/complex-content-slow-scroll.html in a browser.
https://codereview.chromium.org/2883153002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right): https://codereview.chromium.org/2883153002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:1211: return object.IsBoxModelObject() || object.IsSVG(); On 2017/05/15 at 21:32:45, pdr. wrote: > Why is IsSVG() needed? NeedsSVGLocalToBorderBoxTransform, NeedsTransformForNonRootSVG and NeedsEffect all trigger on SVG roots or children. https://codereview.chromium.org/2883153002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:1225: bool might_need_paint_properties = MightNeedPaintProperties(object); On 2017/05/15 at 21:32:45, pdr. wrote: > It's important that this does not change between updates. In other words, a layoutobject cannot change from not might-need-paint-properties to needing paint properties. I think it would be useful if this variable and function reflected that. Something like "ObjectTypeMightNeedPaintProperties"? Indeed. Changed. https://codereview.chromium.org/2883153002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:1227: if (might_need_paint_properties) On 2017/05/15 at 21:32:45, pdr. wrote: > Is the perf win just reducing the calls to this fn by one? No. the perf win is not calling UpdatePaintProperties, which will call a whole bunch of other functions, some of which call others and have a bit of non-trivial logic, just to verify that text nodes etc have no paint properties.
LGTM
Description was changed from ========== Don't bother checking for paint properties for classes that never need them. BUG=718045 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Don't bother checking for paint properties for classes that never need them. In local testing, PerformanceTests/Paint/complex-content-slow-scroll.html improves by about 10%. BUG=718045 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
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": 40001, "attempt_start_ts": 1494884629175290,
"parent_rev": "efe1452454a46b9926b8298e318f83042c11ff19", "commit_rev":
"b717e22b72275498d43183016392b2ae0a011b3e"}
Message was sent while issue was closed.
Description was changed from ========== Don't bother checking for paint properties for classes that never need them. In local testing, PerformanceTests/Paint/complex-content-slow-scroll.html improves by about 10%. BUG=718045 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Don't bother checking for paint properties for classes that never need them. In local testing, PerformanceTests/Paint/complex-content-slow-scroll.html improves by about 10%. BUG=718045 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2883153002 Cr-Commit-Position: refs/heads/master@{#471952} Committed: https://chromium.googlesource.com/chromium/src/+/b717e22b72275498d43183016392... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/b717e22b72275498d43183016392... |
