Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2277443003:
[SPv2] Rasterization invalidation (Closed)
|
Created:
4 years, 4 months ago by Xianzhu Modified:
4 years, 3 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. |
Description[SPv2] Rasterization invalidation
Design doc: https://docs.google.com/document/d/1M669yu7nsF9Wrkm7nQFi3Pp2r-QmCMqm4K7fPPo-doA/edit#heading=h.t80gaiwoyk1m
This will replace ObjectPaintInvalidator::invalidateUsingContainer()
for SPv2.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
BUG=510908
Committed: https://crrev.com/0a848fcd4d159ff31f30aa20f4c0d6fd7a6a4123
Cr-Commit-Position: refs/heads/master@{#414835}
Patch Set 1 #Patch Set 2 : - #Patch Set 3 : - #Patch Set 4 : - #Patch Set 5 : - #Patch Set 6 : - #
Total comments: 15
Patch Set 7 : - #Patch Set 8 : Optimize #Patch Set 9 : Fix break of debug build with dcheck off #
Total comments: 4
Patch Set 10 : - #Messages
Total messages: 56 (43 generated)
Description was changed from ========== [SPv2] Use space of containing transform node for paint invalidation ========== to ========== [SPv2] Use space of containing transform node for paint invalidation CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)
Description was changed from ========== [SPv2] Use space of containing transform node for paint invalidation CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== [SPv2] Rasterization invalidation CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 BUG=510908 ==========
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
Description was changed from ========== [SPv2] Rasterization invalidation CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 BUG=510908 ========== to ========== [SPv2] Rasterization invalidation CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 BUG=510908 ==========
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/v2/patch-status/codereview.chromium.or...
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/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [SPv2] Rasterization invalidation CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 BUG=510908 ========== to ========== [SPv2] Rasterization invalidation Design doc: https://docs.google.com/document/d/1M669yu7nsF9Wrkm7nQFi3Pp2r-QmCMqm4K7fPPo-d... This will replace ObjectPaintInvalidator::invalidateUsingContainer() for SPv2. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 BUG=510908 ==========
wangxianzhu@chromium.org changed reviewers: + chrishtr@chromium.org, pdr@chromium.org, trchen@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2277443003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/paint/PaintChunk.h (right): https://codereview.chromium.org/2277443003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/paint/PaintChunk.h:76: // The total bounds of this paint chunk's contents, in coordinate space of nit: "in the coordinate space..." Also, no need to reference SPv1 since PaintChunks don't exist there, right? https://codereview.chromium.org/2277443003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/paint/PaintChunk.h:83: // SPv2 only. Rectangles that need to be rerasterized in this chunk, in coordinate nit: "re-rasterized" "in the coordinate space..." https://codereview.chromium.org/2277443003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp (right): https://codereview.chromium.org/2277443003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp:543: if (!newChunk.id) { Why would there be no id? https://codereview.chromium.org/2277443003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp:587: // TODO(wangxianzhu): Support raster invalidation for recordered display items without invalidating Do you mean "reordered"? What is an example where we might be able to achieve that? https://codereview.chromium.org/2277443003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp:602: if (!newItem.client().isJustCreated() && newItem.client().getPaintInvalidationReason() == PaintInvalidationNone) This is to check for cached items? https://codereview.chromium.org/2277443003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/paint/PaintControllerTest.cpp (right): https://codereview.chromium.org/2277443003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/paint/PaintControllerTest.cpp:1102: FloatRect(100, 100, 100, 100), Old and new bounds right? If so, comment. https://codereview.chromium.org/2277443003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/paint/PaintControllerTest.cpp:1175: FloatRect(100, 100, 100, 100), ditto
https://codereview.chromium.org/2277443003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/paint/PaintChunk.h (right): https://codereview.chromium.org/2277443003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/paint/PaintChunk.h:76: // The total bounds of this paint chunk's contents, in coordinate space of On 2016/08/26 18:10:51, chrishtr wrote: > nit: "in the coordinate space..." > > Also, no need to reference SPv1 since PaintChunks don't exist there, right? Done. https://codereview.chromium.org/2277443003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/paint/PaintChunk.h:83: // SPv2 only. Rectangles that need to be rerasterized in this chunk, in coordinate On 2016/08/26 18:10:51, chrishtr wrote: > nit: "re-rasterized" "in the coordinate space..." Done. https://codereview.chromium.org/2277443003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp (right): https://codereview.chromium.org/2277443003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp:543: if (!newChunk.id) { On 2016/08/26 18:10:51, chrishtr wrote: > Why would there be no id? Please see https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graph.... https://codereview.chromium.org/2277443003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp:587: // TODO(wangxianzhu): Support raster invalidation for recordered display items without invalidating On 2016/08/26 18:10:51, chrishtr wrote: > Do you mean "reordered"? What is an example where we might be able to achieve > that? Yes :) An example is z-index change: actually we don't need to invalidate the display items, but only need to invalidate the painting layer and re-raster the affected chunks. Test cases are PaintControllerTest.UpdateSwapOrder, UpdateSwapOrderWithInvalidation, ComplexUpdateSwapOrder. https://codereview.chromium.org/2277443003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp:602: if (!newItem.client().isJustCreated() && newItem.client().getPaintInvalidationReason() == PaintInvalidationNone) On 2016/08/26 18:10:51, chrishtr wrote: > This is to check for cached items? Yes. Changed to clientCacheIsValid(newItem.client()). https://codereview.chromium.org/2277443003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/paint/PaintControllerTest.cpp (right): https://codereview.chromium.org/2277443003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/paint/PaintControllerTest.cpp:1102: FloatRect(100, 100, 100, 100), On 2016/08/26 18:10:51, chrishtr wrote: > Old and new bounds right? If so, comment. Done. https://codereview.chromium.org/2277443003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/paint/PaintControllerTest.cpp:1175: FloatRect(100, 100, 100, 100), On 2016/08/26 18:10:51, chrishtr wrote: > ditto Done. https://codereview.chromium.org/2277443003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/paint/PaintControllerTest.cpp:1208: FloatRect(100, 100, 100, 100))); Done.
The CQ bit was checked by chrishtr@chromium.org
lgtm
The CQ bit was unchecked by wangxianzhu@chromium.org
On 2016/08/26 18:59:47, chrishtr wrote: > lgtm Made some optimizations after the previous patch set. PTAL the change: https://codereview.chromium.org/2277443003/diff2/120001:140001/third_party/We... - Renamed generateChunkRerasterizationRects() to generateChunkRasterInvalidationRects() to align with PaintChunk::rasterInvalidationRects. - Reduced 4 loops to 2 in generateChunkRasterInvalidationRectsComparingOldChunk() (which requires a change in checkUnderInvalidation). BTW made a small change in displayItemListAsDebugString to allow showDebugData() of old display item list containing destroyed clients.
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
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/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2277443003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp (right): https://codereview.chromium.org/2277443003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp:678: m_newDisplayItemList.removeLast(); This part is new too, right? Why?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
https://codereview.chromium.org/2277443003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp (right): https://codereview.chromium.org/2277443003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp:678: m_newDisplayItemList.removeLast(); On 2016/08/26 20:01:49, chrishtr wrote: > This part is new too, right? Why? This is required by the new generateChunkRasterInvalidationRectsComparingOldChunk() in under-invalidation checking mode: any display items left in the old chunk will be invalidated for raster because cached display items have been moved into the new chunk, without the need to check if they are changed in the new chunk.
https://codereview.chromium.org/2277443003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp (right): https://codereview.chromium.org/2277443003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp:678: m_newDisplayItemList.removeLast(); On 2016/08/26 at 20:23:23, Xianzhu wrote: > On 2016/08/26 20:01:49, chrishtr wrote: > > This part is new too, right? Why? > > This is required by the new generateChunkRasterInvalidationRectsComparingOldChunk() in under-invalidation checking mode: any display items left in the old chunk will be invalidated for raster because cached display items have been moved into the new chunk, without the need to check if they are changed in the new chunk. I still don't get it. What exactly do these two lines achieve? Sorry to make you spell it out, still not grokking.
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/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2277443003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp (right): https://codereview.chromium.org/2277443003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp:678: m_newDisplayItemList.removeLast(); On 2016/08/26 20:32:22, chrishtr wrote: > On 2016/08/26 at 20:23:23, Xianzhu wrote: > > On 2016/08/26 20:01:49, chrishtr wrote: > > > This part is new too, right? Why? > > > > This is required by the new > generateChunkRasterInvalidationRectsComparingOldChunk() in under-invalidation > checking mode: any display items left in the old chunk will be invalidated for > raster because cached display items have been moved into the new chunk, without > the need to check if they are changed in the new chunk. > > I still don't get it. What exactly do these two lines achieve? Sorry to make you > spell it out, still not grokking. In normal mode, if a display item can use cache, we move the item from the old paint list into the new list using appendByMoving. The movement empties the slot in the old list. Then in generateChunkRasterInvalidationRectsComparingOldChunk(), all remaining display items in the old chunk will be for only disappeared or invalidated clients, so we can simply invalidate raster of all these clients, without needing to compare them to the items in the new list. In under-invalidation checking mode, we force repaint of all display items even if they can use cache to see if the newly painted display items really don't change. These two lines remove the forced repainted item and move the cached item from the old list into the new list, to meet the requirement of generateChunkRasterInvalidationRectsComparingOldChunk(). Updated comments: // Discard the forced repainted display item and move the cached item into m_newDisplayItemList. // This is to align with the non-under-invalidation-checking path to empty the original cached slot, // leaving only disappeared or invalidated display items in the old list after painting.
lgtm
The CQ bit was unchecked by wangxianzhu@chromium.org
The CQ bit was checked by wangxianzhu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [SPv2] Rasterization invalidation Design doc: https://docs.google.com/document/d/1M669yu7nsF9Wrkm7nQFi3Pp2r-QmCMqm4K7fPPo-d... This will replace ObjectPaintInvalidator::invalidateUsingContainer() for SPv2. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 BUG=510908 ========== to ========== [SPv2] Rasterization invalidation Design doc: https://docs.google.com/document/d/1M669yu7nsF9Wrkm7nQFi3Pp2r-QmCMqm4K7fPPo-d... This will replace ObjectPaintInvalidator::invalidateUsingContainer() for SPv2. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 BUG=510908 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== [SPv2] Rasterization invalidation Design doc: https://docs.google.com/document/d/1M669yu7nsF9Wrkm7nQFi3Pp2r-QmCMqm4K7fPPo-d... This will replace ObjectPaintInvalidator::invalidateUsingContainer() for SPv2. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 BUG=510908 ========== to ========== [SPv2] Rasterization invalidation Design doc: https://docs.google.com/document/d/1M669yu7nsF9Wrkm7nQFi3Pp2r-QmCMqm4K7fPPo-d... This will replace ObjectPaintInvalidator::invalidateUsingContainer() for SPv2. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 BUG=510908 Committed: https://crrev.com/0a848fcd4d159ff31f30aa20f4c0d6fd7a6a4123 Cr-Commit-Position: refs/heads/master@{#414835} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/0a848fcd4d159ff31f30aa20f4c0d6fd7a6a4123 Cr-Commit-Position: refs/heads/master@{#414835} |
