Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1581593003:
Skip PaintPhaseFloat if no floats in a layer (Closed)
|
Created:
4 years, 11 months ago by Xianzhu Modified:
4 years, 11 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@OutlinePhase Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSkip PaintPhaseFloat if no floats in a layer
BUG=574938
TEST=PaintLayerPaintTest.PaintPhaseFloat
Committed: https://crrev.com/6b616c49773e7b878799342251519195f3b32067
Cr-Commit-Position: refs/heads/master@{#371972}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #
Total comments: 4
Patch Set 6 : #
Total comments: 2
Patch Set 7 : #
Total comments: 2
Patch Set 8 : #Patch Set 9 : Rebase #Patch Set 10 : Not for commit. What caused the memory leak? #Patch Set 11 : #Patch Set 12 : Disable the test and land #Patch Set 13 : Remove test (instead of #if 0) #
Dependent Patchsets: Messages
Total messages: 41 (19 generated)
wangxianzhu@chromium.org changed reviewers: + chrishtr@chromium.org
This CL is also ready for review. Ptal.
https://codereview.chromium.org/1581593003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayer.h (right): https://codereview.chromium.org/1581593003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayer.h:767: unsigned m_needsPaintPhaseFloat : 1; Add a test like this one? https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... https://codereview.chromium.org/1581593003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp (right): https://codereview.chromium.org/1581593003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:677: if (displayItemListSizeBefore == context.paintController().newDisplayItemList().size()) This looks like a fragile way to try to clear the bit when floats leave this layer.
https://codereview.chromium.org/1581593003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayer.h (right): https://codereview.chromium.org/1581593003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayer.h:767: unsigned m_needsPaintPhaseFloat : 1; On 2016/01/21 22:09:08, chrishtr wrote: > Add a test like this one? > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... Done (https://codereview.chromium.org/1616193002/). https://codereview.chromium.org/1581593003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp (right): https://codereview.chromium.org/1581593003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:677: if (displayItemListSizeBefore == context.paintController().newDisplayItemList().size()) On 2016/01/21 22:09:08, chrishtr wrote: > This looks like a fragile way to try to clear the bit when floats leave this > layer. I think at first we can leave the flag never cleared, so the optimization applies for layers never have floats. This may not affect how much this CL will improve performance of Cluster Telemetry. Not sure how much this affects dynamic change of pages because we lack such benchmarks.
https://codereview.chromium.org/1581593003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PaintLayer.h (right): https://codereview.chromium.org/1581593003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayer.h:629: bool needsPaintPhaseFloat() const { return m_needsPaintPhaseFloat; } Add a comment that once-float, always float.
Patchset #7 (id:120001) has been deleted
https://codereview.chromium.org/1581593003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PaintLayer.h (right): https://codereview.chromium.org/1581593003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayer.h:629: bool needsPaintPhaseFloat() const { return m_needsPaintPhaseFloat; } On 2016/01/22 00:14:44, chrishtr wrote: > Add a comment that once-float, always float. Added full comments in https://codereview.chromium.org/1584493002/ for descendant outlines above, and add comment for this method referencing above.
lgtm https://codereview.chromium.org/1581593003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PaintLayerPainterTest.cpp (right): https://codereview.chromium.org/1581593003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayerPainterTest.cpp:331: EXPECT_FALSE(selfPaintingLayer.needsPaintPhaseFloat()); Remove this part of the unittest.
https://codereview.chromium.org/1581593003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PaintLayerPainterTest.cpp (right): https://codereview.chromium.org/1581593003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayerPainterTest.cpp:331: EXPECT_FALSE(selfPaintingLayer.needsPaintPhaseFloat()); On 2016/01/22 01:51:46, chrishtr wrote: > Remove this part of the unittest. Done.
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/1581593003/#ps180001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1581593003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1581593003/180001
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/1581593003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1581593003/180001
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 to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1581593003/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1581593003/200001
The CQ bit was checked by wangxianzhu@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/1581593003/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1581593003/220001
Patchset #11 (id:220001) has been deleted
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/1581593003/#ps240001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1581593003/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1581593003/240001
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/1581593003/#ps260001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1581593003/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1581593003/260001
The CQ bit was checked by wangxianzhu@chromium.org to run a CQ dry run
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1581593003/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1581593003/280001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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/1581593003/#ps280001 (title: "Remove test (instead of #if 0)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1581593003/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1581593003/280001
Message was sent while issue was closed.
Committed patchset #13 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== Skip PaintPhaseFloat if no floats in a layer BUG=574938 TEST=PaintLayerPaintTest.PaintPhaseFloat ========== to ========== Skip PaintPhaseFloat if no floats in a layer BUG=574938 TEST=PaintLayerPaintTest.PaintPhaseFloat Committed: https://crrev.com/6b616c49773e7b878799342251519195f3b32067 Cr-Commit-Position: refs/heads/master@{#371972} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/6b616c49773e7b878799342251519195f3b32067 Cr-Commit-Position: refs/heads/master@{#371972} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
