Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1739893006:
Skip PaintLayer empty paint phases if it previously painted nothing (Closed)
|
Created:
4 years, 10 months ago by Xianzhu Modified:
4 years, 9 months ago Reviewers:
chrishtr CC:
blink-reviews, blink-reviews-layout_chromium.org, blink-reviews-paint_chromium.org, chromium-reviews, dshwang, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, slimming-paint-reviews_chromium.org, szager+layoutwatch_chromium.org, zoltan1 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSkip PaintLayer empty paint phases if it previously painted nothing
This avoids later painting of empty phases if the layer is marked to need
a paint phase but the phase painted nothing in the following cases:
- the object marking the layer needing the paint phase is not visible
(e.g. because it is clipped)
- the layer was marked needing the paint phase by some object that
has been deleted or moved to another layer.
Instead of directly clearing the needsPaintPhase flags, this CL adds
a new set of flags to indicate if the previous paint of the phases painted
nothing. This is because invalidations of the needsPaintPhase flags and the
new flags need different conditions. The new flags are cleared when scroll
offset, interest rect or clipping changes.
Cluster telemetry runs show that this will improve non-cached painting
by 1.7% and fully-cached painting (with subsequence-caching disabled) by
45%.
BUG=574938
Committed: https://crrev.com/8ad6e62af5301a7e8dacd12fc1ae7914258b9e7a
Cr-Commit-Position: refs/heads/master@{#380841}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : Revert LayoutBox #Patch Set 4 : Keep LayoutBox change #Patch Set 5 : #Patch Set 6 : Rebase #
Total comments: 3
Patch Set 7 : Add documentation #
Total comments: 2
Patch Set 8 : Doc about PaintLayer::needsRepaint #Patch Set 9 : PaintLayer now has more than 32 bit fields #Patch Set 10 : Fix 32bit build #Patch Set 11 : Fix empty phase flag issue about non-subsequenced layers #Patch Set 12 : Rebase #
Messages
Total messages: 46 (24 generated)
Description was changed from ========== Clear PaintLayer needsPaintPhaseXXX if the phase painted nothing BUG=574938 ========== to ========== Clear PaintLayer needsPaintPhase flag if the phase painted nothing BUG=574938 ==========
Description was changed from ========== Clear PaintLayer needsPaintPhase flag if the phase painted nothing BUG=574938 ========== to ========== Clear PaintLayer needsPaintPhase flag if the phase painted nothing This avoids later painting of empty phases if the layer is marked to need a paint phase but the phase painted nothing in the following cases: - the object marking the layer needing the paint phase is not visible (e.g. because it is clipped) - the layer has been marked needing the paint phase by some object that has been deleted or moved to another layer. Cluster telemetry runs show that this will improve non-cached painting by 1.7% and fully-cached painting (with subsequence-caching disabled) by 45%. BUG=574938 ==========
Description was changed from ========== Clear PaintLayer needsPaintPhase flag if the phase painted nothing This avoids later painting of empty phases if the layer is marked to need a paint phase but the phase painted nothing in the following cases: - the object marking the layer needing the paint phase is not visible (e.g. because it is clipped) - the layer has been marked needing the paint phase by some object that has been deleted or moved to another layer. Cluster telemetry runs show that this will improve non-cached painting by 1.7% and fully-cached painting (with subsequence-caching disabled) by 45%. BUG=574938 ========== to ========== Skip PaintLayer empty paint phases if it previously painted nothing This avoids later painting of empty phases if the layer is marked to need a paint phase but the phase painted nothing in the following cases: - the object marking the layer needing the paint phase is not visible (e.g. because it is clipped) - the layer has been marked needing the paint phase by some object that has been deleted or moved to another layer. Instead of directly clearing the needsPaintPhase flags, this CL adds a new set of flags to indicate if the previous paint of the phases painted nothing. This is because invalidations of the needsPaintPhase flags and the new flags need different conditions. The new flags are cleared when scroll offset, interest rect or clipping changes. Cluster telemetry runs show that this will improve non-cached painting by 1.7% and fully-cached painting (with subsequence-caching disabled) by 45%. BUG=574938 ==========
wangxianzhu@chromium.org changed reviewers: + chrishtr@chromium.org
https://codereview.chromium.org/1739893006/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/1739893006/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutBox.cpp:1488: enclosingSelfPaintingLayer.setNeedsPaintPhaseFloat(); This change is because a self-painting floating layer just paints its contents normally, not needing PaintPhaseFloat by itself.
The skip-paint-phases optimization needs more documentation. Could you add Markdown for it? https://codereview.chromium.org/1739893006/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/1739893006/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutBox.cpp:1493: enclosingSelfPaintingLayer.setNeedsPaintPhaseDescendantBlockBackgrounds(); A self-painting layer also doesn't need PaintPhaseDescendantBlockBackgrounds?
On 2016/03/09 at 03:40:47, chrishtr wrote: > The skip-paint-phases optimization needs more documentation. (in general, not just this CL) > > https://codereview.chromium.org/1739893006/diff/100001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): > > https://codereview.chromium.org/1739893006/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/layout/LayoutBox.cpp:1493: enclosingSelfPaintingLayer.setNeedsPaintPhaseDescendantBlockBackgrounds(); > A self-painting layer also doesn't need PaintPhaseDescendantBlockBackgrounds?
On 2016/03/09 03:41:08, chrishtr wrote: > On 2016/03/09 at 03:40:47, chrishtr wrote: > > The skip-paint-phases optimization needs more documentation. > > (in general, not just this CL) > Done. https://codereview.chromium.org/1739893006/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/1739893006/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutBox.cpp:1493: enclosingSelfPaintingLayer.setNeedsPaintPhaseDescendantBlockBackgrounds(); On 2016/03/09 03:40:47, chrishtr wrote: > A self-painting layer also doesn't need PaintPhaseDescendantBlockBackgrounds? Right. The layer paints its own background in PaintPhaseSelfBlockBackgroundOnly.
https://codereview.chromium.org/1739893006/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/README.md (right): https://codereview.chromium.org/1739893006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/README.md:112: we paint with different clipping, scroll offset or interest rect from the previous paint. ..Or the PaintLayer is invalidated?
https://codereview.chromium.org/1739893006/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/README.md (right): https://codereview.chromium.org/1739893006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/README.md:112: we paint with different clipping, scroll offset or interest rect from the previous paint. On 2016/03/10 17:05:44, chrishtr wrote: > ..Or the PaintLayer is invalidated? Added a paragraph here and comments in code to explain how we clear the flags when the PaintLayer is invalidated.
The CQ bit was checked by chrishtr@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1739893006/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1739893006/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
The CQ bit was checked by wangxianzhu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chrishtr@chromium.org Link to the patchset: https://codereview.chromium.org/1739893006/#ps160001 (title: "PaintLayer now has more than 32 bit fields")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1739893006/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1739893006/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by wangxianzhu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chrishtr@chromium.org Link to the patchset: https://codereview.chromium.org/1739893006/#ps180001 (title: "Fix 32bit build")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1739893006/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1739893006/180001
Description was changed from ========== Skip PaintLayer empty paint phases if it previously painted nothing This avoids later painting of empty phases if the layer is marked to need a paint phase but the phase painted nothing in the following cases: - the object marking the layer needing the paint phase is not visible (e.g. because it is clipped) - the layer has been marked needing the paint phase by some object that has been deleted or moved to another layer. Instead of directly clearing the needsPaintPhase flags, this CL adds a new set of flags to indicate if the previous paint of the phases painted nothing. This is because invalidations of the needsPaintPhase flags and the new flags need different conditions. The new flags are cleared when scroll offset, interest rect or clipping changes. Cluster telemetry runs show that this will improve non-cached painting by 1.7% and fully-cached painting (with subsequence-caching disabled) by 45%. BUG=574938 ========== to ========== Skip PaintLayer empty paint phases if it previously painted nothing This avoids later painting of empty phases if the layer is marked to need a paint phase but the phase painted nothing in the following cases: - the object marking the layer needing the paint phase is not visible (e.g. because it is clipped) - the layer was marked needing the paint phase by some object that has been deleted or moved to another layer. Instead of directly clearing the needsPaintPhase flags, this CL adds a new set of flags to indicate if the previous paint of the phases painted nothing. This is because invalidations of the needsPaintPhase flags and the new flags need different conditions. The new flags are cleared when scroll offset, interest rect or clipping changes. Cluster telemetry runs show that this will improve non-cached painting by 1.7% and fully-cached painting (with subsequence-caching disabled) by 45%. BUG=574938 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by wangxianzhu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chrishtr@chromium.org Link to the patchset: https://codereview.chromium.org/1739893006/#ps200001 (title: "Fix empty phase flag issue about non-subsequenced layers")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1739893006/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1739893006/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by wangxianzhu@chromium.org
The CQ bit was unchecked by wangxianzhu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1739893006/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1739893006/200001
The CQ bit was checked by wangxianzhu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chrishtr@chromium.org Link to the patchset: https://codereview.chromium.org/1739893006/#ps220001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1739893006/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1739893006/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by wangxianzhu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1739893006/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1739893006/220001
Message was sent while issue was closed.
Description was changed from ========== Skip PaintLayer empty paint phases if it previously painted nothing This avoids later painting of empty phases if the layer is marked to need a paint phase but the phase painted nothing in the following cases: - the object marking the layer needing the paint phase is not visible (e.g. because it is clipped) - the layer was marked needing the paint phase by some object that has been deleted or moved to another layer. Instead of directly clearing the needsPaintPhase flags, this CL adds a new set of flags to indicate if the previous paint of the phases painted nothing. This is because invalidations of the needsPaintPhase flags and the new flags need different conditions. The new flags are cleared when scroll offset, interest rect or clipping changes. Cluster telemetry runs show that this will improve non-cached painting by 1.7% and fully-cached painting (with subsequence-caching disabled) by 45%. BUG=574938 ========== to ========== Skip PaintLayer empty paint phases if it previously painted nothing This avoids later painting of empty phases if the layer is marked to need a paint phase but the phase painted nothing in the following cases: - the object marking the layer needing the paint phase is not visible (e.g. because it is clipped) - the layer was marked needing the paint phase by some object that has been deleted or moved to another layer. Instead of directly clearing the needsPaintPhase flags, this CL adds a new set of flags to indicate if the previous paint of the phases painted nothing. This is because invalidations of the needsPaintPhase flags and the new flags need different conditions. The new flags are cleared when scroll offset, interest rect or clipping changes. Cluster telemetry runs show that this will improve non-cached painting by 1.7% and fully-cached painting (with subsequence-caching disabled) by 45%. BUG=574938 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Skip PaintLayer empty paint phases if it previously painted nothing This avoids later painting of empty phases if the layer is marked to need a paint phase but the phase painted nothing in the following cases: - the object marking the layer needing the paint phase is not visible (e.g. because it is clipped) - the layer was marked needing the paint phase by some object that has been deleted or moved to another layer. Instead of directly clearing the needsPaintPhase flags, this CL adds a new set of flags to indicate if the previous paint of the phases painted nothing. This is because invalidations of the needsPaintPhase flags and the new flags need different conditions. The new flags are cleared when scroll offset, interest rect or clipping changes. Cluster telemetry runs show that this will improve non-cached painting by 1.7% and fully-cached painting (with subsequence-caching disabled) by 45%. BUG=574938 ========== to ========== Skip PaintLayer empty paint phases if it previously painted nothing This avoids later painting of empty phases if the layer is marked to need a paint phase but the phase painted nothing in the following cases: - the object marking the layer needing the paint phase is not visible (e.g. because it is clipped) - the layer was marked needing the paint phase by some object that has been deleted or moved to another layer. Instead of directly clearing the needsPaintPhase flags, this CL adds a new set of flags to indicate if the previous paint of the phases painted nothing. This is because invalidations of the needsPaintPhase flags and the new flags need different conditions. The new flags are cleared when scroll offset, interest rect or clipping changes. Cluster telemetry runs show that this will improve non-cached painting by 1.7% and fully-cached painting (with subsequence-caching disabled) by 45%. BUG=574938 Committed: https://crrev.com/8ad6e62af5301a7e8dacd12fc1ae7914258b9e7a Cr-Commit-Position: refs/heads/master@{#380841} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/8ad6e62af5301a7e8dacd12fc1ae7914258b9e7a Cr-Commit-Position: refs/heads/master@{#380841}
Message was sent while issue was closed.
A revert of this CL (patchset #12 id:220001) has been created in https://codereview.chromium.org/1815293002/ by wangxianzhu@chromium.org. The reason for reverting is: BUG=595839. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
