Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2186643002:
Fold compositing display items into contained drawings if the drawing is a singleton. (Closed)
|
Created:
4 years, 4 months ago by chrishtr Modified:
4 years, 4 months ago CC:
ajuma+watch_chromium.org, blink-reviews, blink-reviews-platform-graphics_chromium.org, Rik, chromium-reviews, danakj+watch_chromium.org, dshwang, drott+blinkwatch_chromium.org, krit, f(malita), jbroman, Justin Novosad, pdr+graphicswatchlist_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. |
DescriptionFold compositing display items into contained drawings if the drawing is a singleton.
BUG=628831
Committed: https://crrev.com/09f3b178cfb677eaaf91a282df0d898a041378cd
Cr-Commit-Position: refs/heads/master@{#408725}
Patch Set 1 #Patch Set 2 : none #
Total comments: 9
Patch Set 3 : none #
Total comments: 1
Patch Set 4 : none #Patch Set 5 : none #
Total comments: 2
Patch Set 6 : none #Patch Set 7 : none #
Messages
Total messages: 42 (23 generated)
Description was changed from ========== none none none BUG= ========== to ========== Fold compositing display items into contained drawings if the drawing is a singleton. BUG=628831 ==========
chrishtr@chromium.org changed reviewers: + fmalita@chromium.org, wkorman@chromium.org
I have verified that the Skia optimization still works properly on http://www.wholecloud.net/. Have not verified that it works with the rtree patch applied, but see no reason why it would not.
The CQ bit was checked by chrishtr@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...
lgtm https://codereview.chromium.org/2186643002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/paint/CompositingRecorder.cpp (right): https://codereview.chromium.org/2186643002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/paint/CompositingRecorder.cpp:39: PaintController& paintController = graphicsContext.getPaintController(); Need to turn off caching. https://codereview.chromium.org/2186643002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/paint/CompositingRecorder.cpp:41: const DisplayItem* secondToLastDisplayItem = paintController.lastDisplayItem(1); fun: opportunity to use 'penultimate' vs 'secondToLast', if you find such a word entertaining https://codereview.chromium.org/2186643002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/paint/PaintController.h (right): https://codereview.chromium.org/2186643002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/paint/PaintController.h:111: const DisplayItem* lastDisplayItem(unsigned offset); Perhaps worth documenting the lifespan of the item, add DCHECK/docs.
https://codereview.chromium.org/2186643002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/paint/CompositingRecorder.cpp (right): https://codereview.chromium.org/2186643002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/paint/CompositingRecorder.cpp:39: PaintController& paintController = graphicsContext.getPaintController(); On 2016/07/26 18:41:08, wkorman wrote: > Need to turn off caching. Done. https://codereview.chromium.org/2186643002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/paint/CompositingRecorder.cpp:41: const DisplayItem* secondToLastDisplayItem = paintController.lastDisplayItem(1); On 2016/07/26 18:41:08, wkorman wrote: > fun: opportunity to use 'penultimate' vs 'secondToLast', if you find such a word > entertaining Acknowledged.
The CQ bit was unchecked by chrishtr@chromium.org
The CQ bit was checked by chrishtr@chromium.org
The CQ bit was unchecked by chrishtr@chromium.org
The CQ bit was checked by chrishtr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wkorman@chromium.org Link to the patchset: https://codereview.chromium.org/2186643002/#ps30006 (title: "none")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM w/ a question. https://codereview.chromium.org/2186643002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/paint/CompositingDisplayItem.h (right): https://codereview.chromium.org/2186643002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/paint/CompositingDisplayItem.h:33: bool isBeginCompositingDisplayItem() const final { return true; } Nit: 'final' is redundant (class is final). https://codereview.chromium.org/2186643002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/paint/CompositingRecorder.cpp (right): https://codereview.chromium.org/2186643002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/paint/CompositingRecorder.cpp:50: DrawingRecorder newRecorder(pictureBuilder.context(), displayItemClient, displayItemType, cullRect); Why is this inner DrawingRecording needed? Wouldn't pictureBuilder achieve the same on its own?
https://codereview.chromium.org/2186643002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/paint/CompositingDisplayItem.h (right): https://codereview.chromium.org/2186643002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/paint/CompositingDisplayItem.h:33: bool isBeginCompositingDisplayItem() const final { return true; } On 2016/07/26 19:03:15, f(malita) wrote: > Nit: 'final' is redundant (class is final). Acknowledged. https://codereview.chromium.org/2186643002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/paint/CompositingRecorder.cpp (right): https://codereview.chromium.org/2186643002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/paint/CompositingRecorder.cpp:50: DrawingRecorder newRecorder(pictureBuilder.context(), displayItemClient, displayItemType, cullRect); On 2016/07/26 19:03:15, f(malita) wrote: > Why is this inner DrawingRecording needed? Wouldn't pictureBuilder achieve the > same on its own? SkPictureBuilder doesn't start a recording of a specific DrawingDisplayItem, it instead sets up a nested DisplayItemList etc that can be drawn into. To draw into a DisplayItemList you need a recorder.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
wangxianzhu@chromium.org changed reviewers: + wangxianzhu@chromium.org
Discussed with chrishtr, and found this CL exposes a problem of the current DisplayItemClient cache generation mechanism: we don't support skipping cache of some of display items of a DisplayItemClient. As display item cache flags are marked on DisplayItemClients, we should either (1) disallow partial cache skipping, or (2) treat the DisplayItemClient as invalidated if any of its display items skipped cache. 2 seems a safer choice. https://codereview.chromium.org/2186643002/diff/30006/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/paint/DisplayItem.h (right): https://codereview.chromium.org/2186643002/diff/30006/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/paint/DisplayItem.h:319: virtual bool isBeginCompositingDisplayItem() const { return false; } You can avoid this virtual function by using "displayItem.type() == BeginCompositing" instead at the call site.
https://codereview.chromium.org/2186643002/diff/70001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp (right): https://codereview.chromium.org/2186643002/diff/70001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp:433: item.client().setDisplayItemsUncached(); @xianzhu: added this. WDYT?
The CQ bit was checked by chrishtr@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/2186643002/diff/70001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp (right): https://codereview.chromium.org/2186643002/diff/70001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp:433: item.client().setDisplayItemsUncached(); On 2016/07/27 17:47:48, chrishtr wrote: > @xianzhu: added this. WDYT? This has a problem that if any cacheable display item is after skipped-cache display item for the client, the uncached status will be overriden. You also need to check item.skippedCache() because non-drawing and non-subsequence display items are also not cacheable.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Just found a problem affecting this CL in under-invalidation checking of cached subsequences that it failed to handle deletion of display items during painting. Working on fixing it.
The CQ bit was checked by chrishtr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fmalita@chromium.org, wkorman@chromium.org Link to the patchset: https://codereview.chromium.org/2186643002/#ps90001 (title: "none")
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
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
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...
The CQ bit was unchecked by chrishtr@chromium.org
Fixed one unittest, and added another one to exercise the folding code.
The CQ bit was checked by chrishtr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fmalita@chromium.org, wkorman@chromium.org Link to the patchset: https://codereview.chromium.org/2186643002/#ps110001 (title: "none")
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.
Committed patchset #7 (id:110001)
Message was sent while issue was closed.
Description was changed from ========== Fold compositing display items into contained drawings if the drawing is a singleton. BUG=628831 ========== to ========== Fold compositing display items into contained drawings if the drawing is a singleton. BUG=628831 Committed: https://crrev.com/09f3b178cfb677eaaf91a282df0d898a041378cd Cr-Commit-Position: refs/heads/master@{#408725} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/09f3b178cfb677eaaf91a282df0d898a041378cd Cr-Commit-Position: refs/heads/master@{#408725}
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:110001) has been created in https://codereview.chromium.org/2197903002/ by tzik@chromium.org. The reason for reverting is: This CL seems to break layout tests on bot. An example of failure is: https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Linux__db... STDERR: [4:4:0731/203155:3802475433:FATAL:PaintController.cpp(554)] Check failed: false. STDERR: #0 0x7f109c38b99e base::debug::StackTrace::StackTrace() STDERR: #1 0x7f109c3f2fef logging::LogMessage::~LogMessage() STDERR: #2 0x7f1097594655 blink::PaintController::checkUnderInvalidation() STDERR: #3 0x7f1097592e64 blink::PaintController::processNewItem() STDERR: #4 0x7f1097583d52 _ZN5blink15PaintController15createAndAppendINS_18DrawingDisplayItemEJRKNS_17DisplayItemClientERKNS_11DisplayItem4TypeEN3WTF10PassRefPtrI9SkPictureEERbEEEvDpOT0_ STDERR: #5 0x7f1097583af1 blink::DrawingRecorder::~DrawingRecorder() STDERR: #6 0x7f109422f925 base::internal::OptionalStorage<>::~OptionalStorage() STDERR: #7 0x7f109422f8c5 base::Optional<>::~Optional() STDERR: #8 0x7f109422ed83 blink::LayoutObjectDrawingRecorder::~LayoutObjectDrawingRecorder() STDERR: #9 0x7f109423e187 blink::BoxPainter::paintBoxDecorationBackgroundWithRect() STDERR: #10 0x7f109423dcde blink::BoxPainter::paintBoxDecorationBackground() STDERR: #11 0x7f10943f34f5 blink::LayoutBox::paintBoxDecorationBackground() STDERR: #12 0x7f109422dbcd blink::BlockPainter::paintObject() STDERR: #13 0x7f10943b1bd5 blink::LayoutBlock::paintObject() STDERR: #14 0x7f109422d25c blink::BlockPainter::paint() STDERR: #15 0x7f10943b1b55 blink::LayoutBlock::paint(). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
