Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1890603002:
Partly revert https://codereview.chromium.org/1860273003/ where paint follows (Closed)
|
Created:
4 years, 8 months ago by Xianzhu Modified:
4 years, 8 months ago CC:
blink-reviews, chromium-reviews, krit, f(malita), fs, gyuyoung2, kouhei+svg_chromium.org, pdr+svgwatchlist_chromium.org, rwlbuis, Stephen Chennney Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPartly revert https://codereview.chromium.org/1860273003/ where paint follows
Restored updateLifecycleToCompositingCleanPlusScrolling() to
updateAllLifecyclePhases() where paint follows, because some paint
code may depend on the result of lifecycle phases between
CompositingUpdateClean and InPaint. For example, we update
table collapsed borders and layer empty phase information during
paint invalidtion, which are needed by paint.
BUG=602961, 591468, 603230
Committed: https://crrev.com/f07d9b4a2ebb30cb77d3cca7d59e1f3c674e81eb
Cr-Commit-Position: refs/heads/master@{#387178}
Patch Set 1 #Patch Set 2 : #
Total comments: 2
Patch Set 3 : #
Messages
Total messages: 18 (5 generated)
Description was changed from ========== Partly revert https://codereview.chromium.org/1860273003/ where paint follows Restored updateLifecycleToCompositingCleanPlusScrolling() to updateAllLifecyclePhases() where paint follows, because some paint code may depend on the result of lifecycle phases between CompositingUpdateClean and InPaint. For example, we update table collapsed borders and layer empty phase information during paint invalidtion, which are needed by paint. BUG=602961,591468 ========== to ========== Partly revert https://codereview.chromium.org/1860273003/ where paint follows Restored updateLifecycleToCompositingCleanPlusScrolling() to updateAllLifecyclePhases() where paint follows, because some paint code may depend on the result of lifecycle phases between CompositingUpdateClean and InPaint. For example, we update table collapsed borders and layer empty phase information during paint invalidtion, which are needed by paint. BUG=602961,591468,603230 ==========
wangxianzhu@chromium.org changed reviewers: + pdr@chromium.org
https://codereview.chromium.org/1890603002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right): https://codereview.chromium.org/1890603002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/LocalFrame.cpp:622: // TODO(wangxianzhu): Don't need synchronized painting. Nit: Don't -> Doesn't. I'm learning about this code for the first time so forgive some silly questions. Is there any way we can enforce this comment with an assert instead of a comment? For example, in the stacktrace in 602961 it looks like we are painting but we've skipped the paint invalidation step. In FrameView::paintContents we have an assert that the lifecycle >= DocumentLifecycle::CompositingClean... should that assert lifecycle >= PaintInvalidationClean with a comment about why? A second issue is that the root cause seems to be that we are updating collapsed borders during paint invalidation. Would it be practical to just fix that issue (maybe by moving recalcCollapsedBordersIfNeeded into LayoutTable::collapsedBorders?). If not, WDYT about adding something like LayoutView::updateLifecycleToPaintInvalidationClean() with comments about why it's necessary? How important was Chris's original patch? It seems to have raised several issues (2 of the linked bugs are P1)... should we just fully roll it out? It's not clear to me why some of the calls to updateLifecycleToCompositingCleanPlusScrolling are still safe while the ones in this patch aren't.
https://codereview.chromium.org/1890603002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right): https://codereview.chromium.org/1890603002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/LocalFrame.cpp:622: // TODO(wangxianzhu): Don't need synchronized painting. On 2016/04/13 22:11:02, pdr wrote: > Nit: Don't -> Doesn't. > Done. > I'm learning about this code for the first time so forgive some silly questions. > > Is there any way we can enforce this comment with an assert instead of a > comment? For example, in the stacktrace in 602961 it looks like we are painting > but we've skipped the paint invalidation step. In FrameView::paintContents we > have an assert that the lifecycle >= DocumentLifecycle::CompositingClean... > should that assert lifecycle >= PaintInvalidationClean with a comment about why? > Good idea to update/add the asserts which ensures the least lifecycle requirement. The asserts won't replace this comment which reminds us the extra thing (synchronized painting) than needed. Will update/add the asserts in a separate CL because I'm afraid there might be something unexpected. > A second issue is that the root cause seems to be that we are updating collapsed > borders during paint invalidation. Would it be practical to just fix that issue > (maybe by moving recalcCollapsedBordersIfNeeded into > LayoutTable::collapsedBorders?). If not, WDYT about adding something like > LayoutView::updateLifecycleToPaintInvalidationClean() with comments about why > it's necessary? We must call recalcCollapsedBordersIfNeeded() during or before paint invalidation to detect changed cells and invalidate them. LayoutTable::collapsedBorders() is called from paint code which is too late (which was a paint invalidation during painting bug). Because we'll also need paint property update before painting for spv2, and will integrate paint invalidation into pre-painting tree walk, updating collapsed borders during paint invalidaiton (to be pre-painting tree walk) seems a non-issue. Though the bug is about collapsed borders, it's not the only reason that we require paint invalidation before painting. We also need paint invalidation to update some status/flags on LayoutObjects and PaintLayers in order to do correct painting. > How important was Chris's original patch? It seems to have raised several issues > (2 of the linked bugs are P1)... should we just fully roll it out? It's not > clear to me why some of the calls to > updateLifecycleToCompositingCleanPlusScrolling are still safe while the ones in > this patch aren't. Whether its safe depends what is needed to be ready by the following code. At this place, the following code does painting, so the document lifecycle should be ready for painting. They other places are safe because they only need compositing update ready.
On 2016/04/13 at 22:51:45, wangxianzhu wrote: > https://codereview.chromium.org/1890603002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right): > > https://codereview.chromium.org/1890603002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/frame/LocalFrame.cpp:622: // TODO(wangxianzhu): Don't need synchronized painting. > On 2016/04/13 22:11:02, pdr wrote: > > Nit: Don't -> Doesn't. > > > > Done. > > > I'm learning about this code for the first time so forgive some silly questions. > > > > Is there any way we can enforce this comment with an assert instead of a > > comment? For example, in the stacktrace in 602961 it looks like we are painting > > but we've skipped the paint invalidation step. In FrameView::paintContents we > > have an assert that the lifecycle >= DocumentLifecycle::CompositingClean... > > should that assert lifecycle >= PaintInvalidationClean with a comment about why? > > > > Good idea to update/add the asserts which ensures the least lifecycle requirement. The asserts won't replace this comment which reminds us the extra thing (synchronized painting) than needed. Will update/add the asserts in a separate CL because I'm afraid > there might be something unexpected. > > > A second issue is that the root cause seems to be that we are updating collapsed > > borders during paint invalidation. Would it be practical to just fix that issue > > (maybe by moving recalcCollapsedBordersIfNeeded into > > LayoutTable::collapsedBorders?). If not, WDYT about adding something like > > LayoutView::updateLifecycleToPaintInvalidationClean() with comments about why > > it's necessary? > > We must call recalcCollapsedBordersIfNeeded() during or before paint invalidation to detect changed cells and invalidate them. LayoutTable::collapsedBorders() is called from paint code which is too late (which was a paint invalidation during painting bug). > > Because we'll also need paint property update before painting for spv2, and will integrate paint invalidation into pre-painting tree walk, updating collapsed borders during paint invalidaiton (to be pre-painting tree walk) seems a non-issue. > > Though the bug is about collapsed borders, it's not the only reason that we require paint invalidation before painting. We also need paint invalidation to update some status/flags on LayoutObjects and PaintLayers in order to do correct painting. > > > How important was Chris's original patch? It seems to have raised several issues > > (2 of the linked bugs are P1)... should we just fully roll it out? It's not > > clear to me why some of the calls to > > updateLifecycleToCompositingCleanPlusScrolling are still safe while the ones in > > this patch aren't. > > Whether its safe depends what is needed to be ready by the following code. At this place, the following code does painting, so the document lifecycle should be ready for painting. They other places are safe because they only need compositing update ready. LGTM I like the future plan of having this be in the prepaint walk instead of the paint invalidation walk (mostly just a change in names of course, but it's easier to understand).
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/1890603002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1890603002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Partly revert https://codereview.chromium.org/1860273003/ where paint follows Restored updateLifecycleToCompositingCleanPlusScrolling() to updateAllLifecyclePhases() where paint follows, because some paint code may depend on the result of lifecycle phases between CompositingUpdateClean and InPaint. For example, we update table collapsed borders and layer empty phase information during paint invalidtion, which are needed by paint. BUG=602961,591468,603230 ========== to ========== Partly revert https://codereview.chromium.org/1860273003/ where paint follows Restored updateLifecycleToCompositingCleanPlusScrolling() to updateAllLifecyclePhases() where paint follows, because some paint code may depend on the result of lifecycle phases between CompositingUpdateClean and InPaint. For example, we update table collapsed borders and layer empty phase information during paint invalidtion, which are needed by paint. BUG=602961,591468,603230 Committed: https://crrev.com/f07d9b4a2ebb30cb77d3cca7d59e1f3c674e81eb Cr-Commit-Position: refs/heads/master@{#387178} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/f07d9b4a2ebb30cb77d3cca7d59e1f3c674e81eb Cr-Commit-Position: refs/heads/master@{#387178}
Message was sent while issue was closed.
ojan@chromium.org changed reviewers: + ojan@chromium.org
Message was sent while issue was closed.
Before synchronized paint, we didn't have to paint in this code path, right? What changed? Not criticizing. Just trying to understand.
Message was sent while issue was closed.
On 2016/04/14 02:23:35, ojan wrote: > Before synchronized paint, we didn't have to paint in this code path, right? > What changed? > > Not criticizing. Just trying to understand. In the cases changed in this CL, we do special painting after the document cycle update intentionally (not related to synchronized painting). Before synchronized painting, updateAllLifecyclePhases() stopped at PaintInvalidationClean, which met the requirement of the later special paintings. With synchronized painting, updateAllLifecyclePhases() also does painting which is unnecessary for the later special paintings. Update to PaintInvalidationClean (spv1) or UpdatePaintPropertiesClean (spv2) is enough for the cases.
Message was sent while issue was closed.
On 2016/04/14 at 02:33:50, wangxianzhu wrote: > On 2016/04/14 02:23:35, ojan wrote: > > Before synchronized paint, we didn't have to paint in this code path, right? > > What changed? > > > > Not criticizing. Just trying to understand. > > In the cases changed in this CL, we do special painting after the document cycle update intentionally (not related to synchronized painting). Before synchronized painting, updateAllLifecyclePhases() stopped at PaintInvalidationClean, which met the requirement of the later special paintings. With synchronized painting, updateAllLifecyclePhases() also does painting which is unnecessary for the later special paintings. Update to PaintInvalidationClean (spv1) or UpdatePaintPropertiesClean (spv2) is enough for the cases. So the plan is to do a followup patch that makes these cases go to UpdatePaintPropertiesClean?
Message was sent while issue was closed.
On 2016/04/14 03:24:12, ojan wrote: > On 2016/04/14 at 02:33:50, wangxianzhu wrote: > > On 2016/04/14 02:23:35, ojan wrote: > > > Before synchronized paint, we didn't have to paint in this code path, right? > > > What changed? > > > > > > Not criticizing. Just trying to understand. > > > > In the cases changed in this CL, we do special painting after the document > cycle update intentionally (not related to synchronized painting). Before > synchronized painting, updateAllLifecyclePhases() stopped at > PaintInvalidationClean, which met the requirement of the later special > paintings. With synchronized painting, updateAllLifecyclePhases() also does > painting which is unnecessary for the later special paintings. Update to > PaintInvalidationClean (spv1) or UpdatePaintPropertiesClean (spv2) is enough for > the cases. > > So the plan is to do a followup patch that makes these cases go to > UpdatePaintPropertiesClean? Perhaps, or if we prove the synchronized painting is harmless we can also keep them as-is. crbug.com/603230 is for the follow-up.
Message was sent while issue was closed.
On 2016/04/14 at 04:39:26, wangxianzhu wrote: > On 2016/04/14 03:24:12, ojan wrote: > > On 2016/04/14 at 02:33:50, wangxianzhu wrote: > > > On 2016/04/14 02:23:35, ojan wrote: > > > > Before synchronized paint, we didn't have to paint in this code path, right? > > > > What changed? > > > > > > > > Not criticizing. Just trying to understand. > > > > > > In the cases changed in this CL, we do special painting after the document > > cycle update intentionally (not related to synchronized painting). Before > > synchronized painting, updateAllLifecyclePhases() stopped at > > PaintInvalidationClean, which met the requirement of the later special > > paintings. With synchronized painting, updateAllLifecyclePhases() also does > > painting which is unnecessary for the later special paintings. Update to > > PaintInvalidationClean (spv1) or UpdatePaintPropertiesClean (spv2) is enough for > > the cases. > > > > So the plan is to do a followup patch that makes these cases go to > > UpdatePaintPropertiesClean? > > Perhaps, or if we prove the synchronized painting is harmless we can also keep them as-is. crbug.com/603230 is for the follow-up. Synchronized painting does recording, right? I don't see how that could possibly be harmless for performance unless recording is 10x faster than I last encountered.
Message was sent while issue was closed.
On 2016/04/14 05:18:57, ojan wrote: > On 2016/04/14 at 04:39:26, wangxianzhu wrote: > > On 2016/04/14 03:24:12, ojan wrote: > > > On 2016/04/14 at 02:33:50, wangxianzhu wrote: > > > > On 2016/04/14 02:23:35, ojan wrote: > > > > > Before synchronized paint, we didn't have to paint in this code path, > right? > > > > > What changed? > > > > > > > > > > Not criticizing. Just trying to understand. > > > > > > > > In the cases changed in this CL, we do special painting after the document > > > cycle update intentionally (not related to synchronized painting). Before > > > synchronized painting, updateAllLifecyclePhases() stopped at > > > PaintInvalidationClean, which met the requirement of the later special > > > paintings. With synchronized painting, updateAllLifecyclePhases() also does > > > painting which is unnecessary for the later special paintings. Update to > > > PaintInvalidationClean (spv1) or UpdatePaintPropertiesClean (spv2) is enough > for > > > the cases. > > > > > > So the plan is to do a followup patch that makes these cases go to > > > UpdatePaintPropertiesClean? > > > > Perhaps, or if we prove the synchronized painting is harmless we can also keep > them as-is. crbug.com/603230 is for the follow-up. > > Synchronized painting does recording, right? I don't see how that could possibly > be harmless for performance unless recording is 10x faster than I last > encountered. I meant "perhaps, in those cases, if proved". Comments on the bug are appreciated. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
