Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2184363002:
Handle removed display items in under-invalidation checking in cached subsequences (Closed)
|
Created:
4 years, 4 months ago by Xianzhu Modified:
4 years, 4 months ago Reviewers:
chrishtr 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. |
DescriptionHandle removed display items in under-invalidation checking in cached subsequences
When we see a mismatched display item when checking under-invalidation
in a cached subsequence, if the item might be removed later, we
temporarily skip the display item and look forward.
- If the item is finally removed, the mismatch will be ignored;
- If the item is not removed, report the mismatch as under-invalidation.
Also added unit tests for under-invalidation.
BUG=619103
Committed: https://crrev.com/0e615407319a0aa91de9166e653ad341e7681033
Cr-Commit-Position: refs/heads/master@{#408569}
Patch Set 1 #Patch Set 2 : Rebase #Patch Set 3 : Some small tweaks #
Total comments: 6
Patch Set 4 : - #
Total comments: 10
Patch Set 5 : - #Patch Set 6 : Fix compilation error: 2 -> 2u #Patch Set 7 : Make test expectations suitable for both debug and release+dcheck #
Messages
Total messages: 33 (17 generated)
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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
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...
wangxianzhu@chromium.org changed reviewers: + chrishtr@chromium.org
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/2184363002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp (right): https://codereview.chromium.org/2184363002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp:555: if (!oldAndNewEqual) { Don't you need to reset m_skippedUnderInvalidationCount to zero as soon as the pattern of begin/end or compositebegin/drawing/end does *not* happen? https://codereview.chromium.org/2184363002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp:561: if (newItem.isDrawing() && m_skippedUnderInvalidationCount == 1 && m_newDisplayItemList[m_newDisplayItemList.size() - 2].getType() == DisplayItem::BeginCompositing) { Check if the display list has length at least 2.
https://codereview.chromium.org/2184363002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp (right): https://codereview.chromium.org/2184363002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp:555: if (!oldAndNewEqual) { On 2016/07/28 16:55:36, chrishtr wrote: > Don't you need to reset m_skippedUnderInvalidationCount to zero as soon as the > pattern of begin/end or compositebegin/drawing/end does *not* happen? No. If the pattern does not happen, non-zero m_skippedUnderInvalidationCount means real under-invalidation because the mismatching display items actually exist. Line 568-274 below will report under-invalidation in the case. https://codereview.chromium.org/2184363002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp:561: if (newItem.isDrawing() && m_skippedUnderInvalidationCount == 1 && m_newDisplayItemList[m_newDisplayItemList.size() - 2].getType() == DisplayItem::BeginCompositing) { On 2016/07/28 16:55:36, chrishtr wrote: > Check if the display list has length at least 2. Added a DCHECK. The condition should be always true because m_newDisplayItemList.size() > m_skippedUnderInvalidationCount.
https://codereview.chromium.org/2184363002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp (right): https://codereview.chromium.org/2184363002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp:555: if (!oldAndNewEqual) { On 2016/07/28 at 17:20:48, Xianzhu wrote: > On 2016/07/28 16:55:36, chrishtr wrote: > > Don't you need to reset m_skippedUnderInvalidationCount to zero as soon as the > > pattern of begin/end or compositebegin/drawing/end does *not* happen? > > No. If the pattern does not happen, non-zero m_skippedUnderInvalidationCount means real under-invalidation because the mismatching display items actually exist. Line 568-274 below will report under-invalidation in the case. Suppose the display list is this: BeginCompositing - Drawing - Drawing - EndCompositing After the first two are processed, won't m_skippedUnderInvalidationCount be 2? What resets it to 0 when the second Drawing is encountered?
https://codereview.chromium.org/2184363002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp (right): https://codereview.chromium.org/2184363002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp:555: if (!oldAndNewEqual) { On 2016/07/28 17:29:21, chrishtr wrote: > On 2016/07/28 at 17:20:48, Xianzhu wrote: > > On 2016/07/28 16:55:36, chrishtr wrote: > > > Don't you need to reset m_skippedUnderInvalidationCount to zero as soon as > the > > > pattern of begin/end or compositebegin/drawing/end does *not* happen? > > > > No. If the pattern does not happen, non-zero m_skippedUnderInvalidationCount > means real under-invalidation because the mismatching display items actually > exist. Line 568-274 below will report under-invalidation in the case. > > Suppose the display list is this: > > BeginCompositing - Drawing - Drawing - EndCompositing > > After the first two are processed, won't m_skippedUnderInvalidationCount be 2? > What resets it to 0 when the second Drawing is encountered? It's not reset to 0. It is non-zero only if we have seen mismatching display items in a subsequence that is supposed to use the cache. It's 2 only if the BeginCompositing and the first Drawing didn't match the cached display items. At the second Drawing, we know that the skipped probable under-invalidations are real under-invalidations and will report the mismatching BeginCompositing as under-invalidation. If all the new display items are matching cached ones, m_skippedUnderInvalidationCount will be always 0.
On 2016/07/28 at 17:38:40, wangxianzhu wrote: > https://codereview.chromium.org/2184363002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp (right): > > https://codereview.chromium.org/2184363002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp:555: if (!oldAndNewEqual) { > On 2016/07/28 17:29:21, chrishtr wrote: > > On 2016/07/28 at 17:20:48, Xianzhu wrote: > > > On 2016/07/28 16:55:36, chrishtr wrote: > > > > Don't you need to reset m_skippedUnderInvalidationCount to zero as soon as > > the > > > > pattern of begin/end or compositebegin/drawing/end does *not* happen? > > > > > > No. If the pattern does not happen, non-zero m_skippedUnderInvalidationCount > > means real under-invalidation because the mismatching display items actually > > exist. Line 568-274 below will report under-invalidation in the case. > > > > Suppose the display list is this: > > > > BeginCompositing - Drawing - Drawing - EndCompositing > > > > After the first two are processed, won't m_skippedUnderInvalidationCount be 2? > > What resets it to 0 when the second Drawing is encountered? > > It's not reset to 0. It is non-zero only if we have seen mismatching display items in a subsequence that is supposed to use the cache. It's 2 only if the BeginCompositing and the first Drawing didn't match the cached display items. At the second Drawing, we know that the skipped probable under-invalidations are real under-invalidations and will report the mismatching BeginCompositing as under-invalidation. > > If all the new display items are matching cached ones, m_skippedUnderInvalidationCount will be always 0. Ah ok, now it makes sense. Thanks for explaining.
https://codereview.chromium.org/2184363002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/paint/PaintControllerTest.cpp (left): https://codereview.chromium.org/2184363002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/paint/PaintControllerTest.cpp:908: second.setDisplayItemsUncached(); These few lines you removed were incorrect? https://codereview.chromium.org/2184363002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/paint/PaintControllerTest.cpp (right): https://codereview.chromium.org/2184363002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/paint/PaintControllerTest.cpp:623: EXPECT_FALSE(SubsequenceRecorder::useCachedSubsequenceIfPossible(context, container2)); Add a note that useCachedSubsequenceIfPossible is forced off if slimmingPaintUnderInvalidationCheckingEnabled. That's the reason for this special-case. https://codereview.chromium.org/2184363002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/paint/PaintControllerTest.cpp:764: EXPECT_FALSE(SubsequenceRecorder::useCachedSubsequenceIfPossible(context, content1)); Ditto. https://codereview.chromium.org/2184363002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/paint/PaintControllerTest.cpp:1158: #if DCHECK_IS_ON() && defined(GTEST_HAS_DEATH_TEST) && !OS(ANDROID) Why the if for android? Please add a commment. https://codereview.chromium.org/2184363002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/paint/PaintControllerTest.cpp:1360: // This should not die. Shouldn't less drawing for the same client ideally be under-invalidation?
https://codereview.chromium.org/2184363002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/paint/PaintControllerTest.cpp (left): https://codereview.chromium.org/2184363002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/paint/PaintControllerTest.cpp:908: second.setDisplayItemsUncached(); On 2016/07/28 18:14:42, chrishtr wrote: > These few lines you removed were incorrect? This invalidation is not needed. Removing it can expose more possible issues. https://codereview.chromium.org/2184363002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/paint/PaintControllerTest.cpp (right): https://codereview.chromium.org/2184363002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/paint/PaintControllerTest.cpp:623: EXPECT_FALSE(SubsequenceRecorder::useCachedSubsequenceIfPossible(context, container2)); On 2016/07/28 18:14:42, chrishtr wrote: > Add a note that useCachedSubsequenceIfPossible is forced off if > slimmingPaintUnderInvalidationCheckingEnabled. That's the reason for this > special-case. Done. https://codereview.chromium.org/2184363002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/paint/PaintControllerTest.cpp:764: EXPECT_FALSE(SubsequenceRecorder::useCachedSubsequenceIfPossible(context, content1)); On 2016/07/28 18:14:42, chrishtr wrote: > Ditto. Done. https://codereview.chromium.org/2184363002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/paint/PaintControllerTest.cpp:1158: #if DCHECK_IS_ON() && defined(GTEST_HAS_DEATH_TEST) && !OS(ANDROID) On 2016/07/28 18:14:42, chrishtr wrote: > Why the if for android? Please add a commment. Done. https://codereview.chromium.org/2184363002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/paint/PaintControllerTest.cpp:1360: // This should not die. On 2016/07/28 18:14:42, chrishtr wrote: > Shouldn't less drawing for the same client ideally be under-invalidation? Added comments: // We don't detect under-invalidation in this case, and PaintController can also handle the case gracefully. // However, less-drawing at a time often means more-drawing at another time so eventually we'll detect // such under-invalidations.
The CQ bit was checked by chrishtr@chromium.org
lgtm
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: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
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/2184363002/#ps100001 (title: "Fix compilation error: 2 -> 2u")
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: linux_chromium_rel_ng on master.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
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/2184363002/#ps120001 (title: "Make test expectations suitable for both debug and release+dcheck")
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:120001)
Message was sent while issue was closed.
Description was changed from ========== Handle removed display items in under-invalidation checking in cached subsequences When we see a mismatched display item when checking under-invalidation in a cached subsequence, if the item might be removed later, we temporarily skip the display item and look forward. - If the item is finally removed, the mismatch will be ignored; - If the item is not removed, report the mismatch as under-invalidation. Also added unit tests for under-invalidation. BUG=619103 ========== to ========== Handle removed display items in under-invalidation checking in cached subsequences When we see a mismatched display item when checking under-invalidation in a cached subsequence, if the item might be removed later, we temporarily skip the display item and look forward. - If the item is finally removed, the mismatch will be ignored; - If the item is not removed, report the mismatch as under-invalidation. Also added unit tests for under-invalidation. BUG=619103 Committed: https://crrev.com/0e615407319a0aa91de9166e653ad341e7681033 Cr-Commit-Position: refs/heads/master@{#408569} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/0e615407319a0aa91de9166e653ad341e7681033 Cr-Commit-Position: refs/heads/master@{#408569} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
