Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1862313002:
Update PaintLayer::needsPaintPhaseXXX flags when add/remove layer on style change (Closed)
|
Created:
4 years, 8 months ago by Xianzhu Modified:
4 years, 8 months ago CC:
blink-reviews, blink-reviews-paint_chromium.org, chromium-reviews, dshwang, slimming-paint-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUpdate PaintLayer::needsPaintPhaseXXX flags when add/remove layer on style change
When addding/removing a layer on style change, we may not
do paint invalidation to update the needsPaintPhaseXXX flags, so
we need to update the flags manually.
In the future, the logic can be simplified by updating the flags
during pre-painting tree walk.
BUG=598978
Committed: https://crrev.com/eb3bc319810c236e1a030d0f0c04867e1cdb8862
Cr-Commit-Position: refs/heads/master@{#386264}
Patch Set 1 #Patch Set 2 : #
Total comments: 11
Patch Set 3 : #
Total comments: 9
Patch Set 4 : #
Messages
Total messages: 19 (7 generated)
wangxianzhu@chromium.org changed reviewers: + chrishtr@chromium.org, pdr@chromium.org
I'll leave this review to pdr@. Off on vacation now.
https://codereview.chromium.org/1862313002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right): https://codereview.chromium.org/1862313002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayer.cpp:1253: if (PaintLayer* enclosingSelfPaintingLayer = this->enclosingSelfPaintingLayer()) enclosingSelfPaintingLayer is O(n) so it looks like this code will introduce an O(n^2) when tearing down the paint layer tree (or the layout object tree through LayoutObject::willBeRemovedFromTree). I don't have a good intuition about the performance of these cases... do you think this is okay in terms of performance? https://codereview.chromium.org/1862313002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayer.cpp:1254: enclosingSelfPaintingLayer->mergeNeedsPaintPhaseFlagsFrom(*oldChild); I'm new to this code... does "removeChild" always remove the child layer and merge the child's contents into the parent layer? (If so, your codechange here makes sense.) https://codereview.chromium.org/1862313002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayer.cpp:2446: if (PaintLayer* enclosingSelfPaintingLayer = parent->enclosingSelfPaintingLayer()) { Are these codepaths tested? I removed these lines and no tests failed. https://codereview.chromium.org/1862313002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayer.cpp:2454: } Is the call to clearNeedsPaintPhaseFlags needed or is it okay to leave stale bits on a non-self-painting-layer? If clearNeedsPaintPhaseFlags is needed, do we need to also call clearNeedsPaintPhaseFlags in the cases where there isn't a layer parent, and in cases where there isn't an enclosing self painting layer? https://codereview.chromium.org/1862313002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/README.md (right): https://codereview.chromium.org/1862313002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/README.md:244: pre-painting tree walk to simplify the logics. Very well written comment.
https://codereview.chromium.org/1862313002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right): https://codereview.chromium.org/1862313002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayer.cpp:1253: if (PaintLayer* enclosingSelfPaintingLayer = this->enclosingSelfPaintingLayer()) On 2016/04/08 04:19:00, pdr wrote: > enclosingSelfPaintingLayer is O(n) so it looks like this code will introduce an > O(n^2) when tearing down the paint layer tree (or the layout object tree through > LayoutObject::willBeRemovedFromTree). I don't have a good intuition about the > performance of these cases... do you think this is okay in terms of performance? Because except for non-composited non-stacked overflow clipping object and non-composited non-stacked LayoutParts, all layers are self-painting, so normally we should find a self-painting layer fast, seldom hit the worst case O(n). https://codereview.chromium.org/1862313002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayer.cpp:1254: enclosingSelfPaintingLayer->mergeNeedsPaintPhaseFlagsFrom(*oldChild); On 2016/04/08 04:19:00, pdr wrote: > I'm new to this code... does "removeChild" always remove the child layer and > merge the child's contents into the parent layer? (If so, your codechange here > makes sense.) Good catch. We should avoid the operation when this method is called from LayoutObject::willBeRemovedFromTree(). Moved the logic into PaintLayer::removeOnlyThisLayer() which is to handle the case that an object changes style and remove the existing layer. https://codereview.chromium.org/1862313002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayer.cpp:2446: if (PaintLayer* enclosingSelfPaintingLayer = parent->enclosingSelfPaintingLayer()) { On 2016/04/08 04:19:00, pdr wrote: > Are these codepaths tested? I removed these lines and no tests failed. Added these code because I was afraid there were cases self-painting-layer change not to cause layout and paint invalidation, but failed to find a test case. Rethought about this, and it seems that all self-painting-layer status change will cause at least paint invalidation. For example, on compositing-status change of a scrolling layer, we'll invalidate all objects on the layer. Remove these code. https://codereview.chromium.org/1862313002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayer.cpp:2454: } On 2016/04/08 04:19:00, pdr wrote: > Is the call to clearNeedsPaintPhaseFlags needed or is it okay to leave stale > bits on a non-self-painting-layer? > If clearNeedsPaintPhaseFlags is needed, do we need to also call > clearNeedsPaintPhaseFlags in the cases where there isn't a layer parent, and in > cases where there isn't an enclosing self painting layer? Good questions. I think this affects how accurately we keep the bits thus performance but not a big deal as we already don't clear the bits on self-painting layers once set. The stale bits on a non-self-painting-layer may affect the result of line 2448 when the layer becomes self-painting again. Would like to remove clearNeedsPaintPhaseFlags(). https://codereview.chromium.org/1862313002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (left): https://codereview.chromium.org/1862313002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:307: This is moved into PaintLayer::insertOnlyThisLayer() to make the logic symmetric with PaintLayer::removeOnlyThisLayer(). This method is the only caller of PaintLayer::insertOnlyThisLayer().
The CQ bit was checked by pdr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1862313002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1862313002/40001
Thanks for the detailed explanations. I understand this code better now and I think this approach is good overall. https://codereview.chromium.org/1862313002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right): https://codereview.chromium.org/1862313002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayer.cpp:1254: enclosingSelfPaintingLayer->mergeNeedsPaintPhaseFlagsFrom(*oldChild); On 2016/04/08 at 18:08:48, Xianzhu wrote: > On 2016/04/08 04:19:00, pdr wrote: > > I'm new to this code... does "removeChild" always remove the child layer and > > merge the child's contents into the parent layer? (If so, your codechange here > > makes sense.) > > Good catch. We should avoid the operation when this method is called from LayoutObject::willBeRemovedFromTree(). Moved the logic into PaintLayer::removeOnlyThisLayer() which is to handle the case that an object changes style and remove the existing layer. Because PaintLayer::{insert,remove}OnlyThisLayer() now differs from addChild by needing to update these flags, what do you think about renaming these functions to something like PaintLayer::{insert,remove}OnlyThisLayerAfterStyleChange()? Alternatively, we could just add a comment. https://codereview.chromium.org/1862313002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (left): https://codereview.chromium.org/1862313002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:307: On 2016/04/08 at 18:08:48, Xianzhu wrote: > This is moved into PaintLayer::insertOnlyThisLayer() to make the logic symmetric with PaintLayer::removeOnlyThisLayer(). This method is the only caller of PaintLayer::insertOnlyThisLayer(). Is this safe to move? The comment says that we must do this invalidation before m_layer is changed because the containerForPaintInvalidation may change. If it's necessary, we could preserve the original logic by passing the original containerForPaintInvalidation to insertOnlyThisLayer. https://codereview.chromium.org/1862313002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right): https://codereview.chromium.org/1862313002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayer.cpp:1274: if (!didSetPaintInvalidation && isSelfPaintingLayer()) { Could we just check "if (!needsRepaint() && isSelfPaintingLayer())" ? https://codereview.chromium.org/1862313002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayer.cpp:1305: void PaintLayer::insertOnlyThisLayer() LayoutObject::insertedIntoTree (which calls blink::addLayers) looks similar to PaintLayer::{insert,remove}OnlyThisLayer(). I think we don't need the merge flags logic because insertedIntoTree will always cause a paint invalidation... is that true? https://codereview.chromium.org/1862313002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayer.cpp:1330: if (PaintLayer* enclosingSelfPaintingLayer = m_parent->enclosingSelfPaintingLayer()) Similar to my comment in LayoutBoxModelObject::createLayer, will "m_parent->enclosingSelfPaintingLayer()" always be the original enclosing self painting layer of the layout object before it gained a layer?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
https://codereview.chromium.org/1862313002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right): https://codereview.chromium.org/1862313002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayer.cpp:1254: enclosingSelfPaintingLayer->mergeNeedsPaintPhaseFlagsFrom(*oldChild); On 2016/04/08 19:46:07, pdr wrote: > On 2016/04/08 at 18:08:48, Xianzhu wrote: > > On 2016/04/08 04:19:00, pdr wrote: > > > I'm new to this code... does "removeChild" always remove the child layer and > > > merge the child's contents into the parent layer? (If so, your codechange > here > > > makes sense.) > > > > Good catch. We should avoid the operation when this method is called from > LayoutObject::willBeRemovedFromTree(). Moved the logic into > PaintLayer::removeOnlyThisLayer() which is to handle the case that an object > changes style and remove the existing layer. > > Because PaintLayer::{insert,remove}OnlyThisLayer() now differs from addChild by > needing to update these flags, what do you think about renaming these functions > to something like PaintLayer::{insert,remove}OnlyThisLayerAfterStyleChange()? > Alternatively, we could just add a comment. Done renaming. https://codereview.chromium.org/1862313002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (left): https://codereview.chromium.org/1862313002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:307: On 2016/04/08 19:46:07, pdr wrote: > On 2016/04/08 at 18:08:48, Xianzhu wrote: > > This is moved into PaintLayer::insertOnlyThisLayer() to make the logic > symmetric with PaintLayer::removeOnlyThisLayer(). This method is the only caller > of PaintLayer::insertOnlyThisLayer(). > > Is this safe to move? The comment says that we must do this invalidation before > m_layer is changed because the containerForPaintInvalidation may change. > > If it's necessary, we could preserve the original logic by passing the original > containerForPaintInvalidation to insertOnlyThisLayer. You're right. Thanks for scheduling the dry-run. It does break layout tests. Modified containerForPaintInvalidation() to parent()->containerForPaintInvalidation() in PaintLayer::insertOnlyThisLayer() to get the original paint invalidation container before creating the layer. https://codereview.chromium.org/1862313002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right): https://codereview.chromium.org/1862313002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayer.cpp:1274: if (!didSetPaintInvalidation && isSelfPaintingLayer()) { On 2016/04/08 19:46:07, pdr wrote: > Could we just check "if (!needsRepaint() && isSelfPaintingLayer())" ? No. First, needsRepaint flag is set during paint invalidation, and it's always false before paint invalidation. Second, needsRepaint flag is set when anything in the layer needs repaint, which doesn't mean the needsPaintPhase flags will be updated. https://codereview.chromium.org/1862313002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayer.cpp:1305: void PaintLayer::insertOnlyThisLayer() On 2016/04/08 19:46:07, pdr wrote: > LayoutObject::insertedIntoTree (which calls blink::addLayers) looks similar to > PaintLayer::{insert,remove}OnlyThisLayer(). I think we don't need the merge > flags logic because insertedIntoTree will always cause a paint invalidation... > is that true? Yes. https://codereview.chromium.org/1862313002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayer.cpp:1330: if (PaintLayer* enclosingSelfPaintingLayer = m_parent->enclosingSelfPaintingLayer()) On 2016/04/08 19:46:07, pdr wrote: > Similar to my comment in LayoutBoxModelObject::createLayer, will > "m_parent->enclosingSelfPaintingLayer()" always be the original enclosing self > painting layer of the layout object before it gained a layer? Yes, because here it's using m_parent->.
LGTM > https://codereview.chromium.org/1862313002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/paint/PaintLayer.cpp:1330: if (PaintLayer* enclosingSelfPaintingLayer = m_parent->enclosingSelfPaintingLayer()) > On 2016/04/08 19:46:07, pdr wrote: > > Similar to my comment in LayoutBoxModelObject::createLayer, will > > "m_parent->enclosingSelfPaintingLayer()" always be the original enclosing self > > painting layer of the layout object before it gained a layer? > > Yes, because here it's using m_parent->. I added an assert locally to check if the parent's enclosingSelfPaintingLayer is always equal to the layout object's original enclosing self painting layer and it looks like it is true.
Description was changed from ========== Update PaintLayer::needsPaintPhaseXXX flags when self-painting relationship changes When self-painting relationship changes (e.g. when layer structure changes, or a layer's self-painting status changes), we may not do paint invalidation to update the needsPaintPhaseXXX flags, so we need to update the flags manually. For example, if a layer becomes self-painting, we copy the flags from its containing self-painting layer to this layer, assuming that this layer needs all paint phases that its self-painting layer needs. In the future, the logic can be simplified by updating the flags during pre-painting tree walk. BUG=598978 ========== to ========== Update PaintLayer::needsPaintPhaseXXX flags when add/remove layer on style change When addding/removing a layer on style change, we may not do paint invalidation to update the needsPaintPhaseXXX flags, so we need to update the flags manually. In the future, the logic can be simplified by updating the flags during pre-painting tree walk. BUG=598978 ==========
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/1862313002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1862313002/60001
Message was sent while issue was closed.
Description was changed from ========== Update PaintLayer::needsPaintPhaseXXX flags when add/remove layer on style change When addding/removing a layer on style change, we may not do paint invalidation to update the needsPaintPhaseXXX flags, so we need to update the flags manually. In the future, the logic can be simplified by updating the flags during pre-painting tree walk. BUG=598978 ========== to ========== Update PaintLayer::needsPaintPhaseXXX flags when add/remove layer on style change When addding/removing a layer on style change, we may not do paint invalidation to update the needsPaintPhaseXXX flags, so we need to update the flags manually. In the future, the logic can be simplified by updating the flags during pre-painting tree walk. BUG=598978 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Update PaintLayer::needsPaintPhaseXXX flags when add/remove layer on style change When addding/removing a layer on style change, we may not do paint invalidation to update the needsPaintPhaseXXX flags, so we need to update the flags manually. In the future, the logic can be simplified by updating the flags during pre-painting tree walk. BUG=598978 ========== to ========== Update PaintLayer::needsPaintPhaseXXX flags when add/remove layer on style change When addding/removing a layer on style change, we may not do paint invalidation to update the needsPaintPhaseXXX flags, so we need to update the flags manually. In the future, the logic can be simplified by updating the flags during pre-painting tree walk. BUG=598978 Committed: https://crrev.com/eb3bc319810c236e1a030d0f0c04867e1cdb8862 Cr-Commit-Position: refs/heads/master@{#386264} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/eb3bc319810c236e1a030d0f0c04867e1cdb8862 Cr-Commit-Position: refs/heads/master@{#386264} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
