Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2476813002:
Move LayoutObject::m_previousPosition... into a global map in ObjectPaintInvalidator (Closed)
|
Created:
4 years, 1 month ago by Xianzhu Modified:
4 years, 1 month ago 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, szager+layoutwatch_chromium.org, zoltan1 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMove LayoutObject::m_previousPosition... into a global map in ObjectPaintInvalidator
Save the field in a global map in ObjectPaintInvalidator only when:
- the object is not a LayoutText (because checking change of visual
rect suffices to detect layout caused invalidation);
- the location is different from visual rect location (if they are
same, we can just get previous location from previous visual rect)
Also renamed the field to previousLocationInBacking.
Stat shows that only 4.6% of all LayoutObjects need to save the previous
location (https://ct.skia.org/chromium_perf_runs/ run 1436).
This makes space for storing previous paint offset for all objects,
which is needed by incremental paint property tree update, and paint
offset change detection.
BUG=660195
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Committed: https://crrev.com/31922c6cd1181c77ae039adf22243cae6ca526f3
Cr-Commit-Position: refs/heads/master@{#430095}
Patch Set 1 #Patch Set 2 : - #Patch Set 3 : - #
Total comments: 8
Patch Set 4 : - #
Total comments: 2
Patch Set 5 : doc #Messages
Total messages: 34 (25 generated)
Description was changed from ========== Remove LayoutObject::m_previousPositionFromPaintInvalidationBacking Now we save it in a global map in ObjectPaintInvalidator only when: - the object is a LayoutBoxModelObject; - the location is different from visual rect location (if they are same, we can just get previous location from previous visual rect] location) Stat shows that only n% LayoutBoxModelObjects need to save the previous location. This is to make space for storing previous paint offset for all objects, which is needed by incremental paint property tree update, and paint offset change detection. BUG=660195 ========== to ========== Remove LayoutObject::m_previousPositionFromPaintInvalidationBacking Now we save it in a global map in ObjectPaintInvalidator only when: - the object is a LayoutBoxModelObject; - the location is different from visual rect location (if they are same, we can just get previous location from previous visual rect] location) Stat shows that only n% LayoutBoxModelObjects need to save the previous location. This is to make space for storing previous paint offset for all objects, which is needed by incremental paint property tree update, and paint offset change detection. BUG=660195 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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Description was changed from ========== Remove LayoutObject::m_previousPositionFromPaintInvalidationBacking Now we save it in a global map in ObjectPaintInvalidator only when: - the object is a LayoutBoxModelObject; - the location is different from visual rect location (if they are same, we can just get previous location from previous visual rect] location) Stat shows that only n% LayoutBoxModelObjects need to save the previous location. This is to make space for storing previous paint offset for all objects, which is needed by incremental paint property tree update, and paint offset change detection. BUG=660195 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Move LayoutObject::m_previousPositionFrom... into a global map in ObjectPaintInvalidator Now we save the field in a global map in ObjectPaintInvalidator only when: - the object is a LayoutBoxModelObject; - the location is different from visual rect location (if they are same, we can just get previous location from previous visual rect] location) Stat shows that only 3% LayoutObjects need to save the previous location. This is to make space for storing previous paint offset for all objects, which is needed by incremental paint property tree update, and paint offset change detection. BUG=660195 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Description was changed from ========== Move LayoutObject::m_previousPositionFrom... into a global map in ObjectPaintInvalidator Now we save the field in a global map in ObjectPaintInvalidator only when: - the object is a LayoutBoxModelObject; - the location is different from visual rect location (if they are same, we can just get previous location from previous visual rect] location) Stat shows that only 3% LayoutObjects need to save the previous location. This is to make space for storing previous paint offset for all objects, which is needed by incremental paint property tree update, and paint offset change detection. BUG=660195 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Move LayoutObject::m_previousPositionFrom... into a global map in ObjectPaintInvalidator Save the field in a global map in ObjectPaintInvalidator only when: - the object is a LayoutBoxModelObject and LayoutSVGContainer; - the location is different from visual rect location (if they are same, we can just get previous location from previous visual rect] location) Stat shows that only 4.6% of all LayoutObjects need to save the previous location. With the first criteria, now we don't save previous locations for LayoutTexts and non-container LayoutSVGModelObjects, because checking change of visual rect suffices to detect layout caused paint invalidation for them. This makes space for storing previous paint offset for all objects, which is needed by incremental paint property tree update, and paint offset change detection. BUG=660195 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Description was changed from ========== Move LayoutObject::m_previousPositionFrom... into a global map in ObjectPaintInvalidator Save the field in a global map in ObjectPaintInvalidator only when: - the object is a LayoutBoxModelObject and LayoutSVGContainer; - the location is different from visual rect location (if they are same, we can just get previous location from previous visual rect] location) Stat shows that only 4.6% of all LayoutObjects need to save the previous location. With the first criteria, now we don't save previous locations for LayoutTexts and non-container LayoutSVGModelObjects, because checking change of visual rect suffices to detect layout caused paint invalidation for them. This makes space for storing previous paint offset for all objects, which is needed by incremental paint property tree update, and paint offset change detection. BUG=660195 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Move LayoutObject::m_previousPosition... into a global map in ObjectPaintInvalidator Save the field in a global map in ObjectPaintInvalidator only when: - the object is a LayoutBoxModelObject and LayoutSVGContainer; - the location is different from visual rect location (if they are same, we can just get previous location from previous visual rect] location) Also renamed the field to previousLocationInBacking. Stat shows that only 4.6% of all LayoutObjects need to save the previous location (https://ct.skia.org/chromium_perf_runs/ run 1436). With the first criteria, now we don't save previous locations for LayoutTexts and non-container LayoutSVGModelObjects, because checking change of visual rect suffices to detect layout caused paint invalidation for them. This makes space for storing previous paint offset for all objects, which is needed by incremental paint property tree update, and paint offset change detection. BUG=660195 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
wangxianzhu@chromium.org changed reviewers: + chrishtr@chromium.org, pdr@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...
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.
Description was changed from ========== Move LayoutObject::m_previousPosition... into a global map in ObjectPaintInvalidator Save the field in a global map in ObjectPaintInvalidator only when: - the object is a LayoutBoxModelObject and LayoutSVGContainer; - the location is different from visual rect location (if they are same, we can just get previous location from previous visual rect] location) Also renamed the field to previousLocationInBacking. Stat shows that only 4.6% of all LayoutObjects need to save the previous location (https://ct.skia.org/chromium_perf_runs/ run 1436). With the first criteria, now we don't save previous locations for LayoutTexts and non-container LayoutSVGModelObjects, because checking change of visual rect suffices to detect layout caused paint invalidation for them. This makes space for storing previous paint offset for all objects, which is needed by incremental paint property tree update, and paint offset change detection. BUG=660195 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Move LayoutObject::m_previousPosition... into a global map in ObjectPaintInvalidator Save the field in a global map in ObjectPaintInvalidator only when: - the object is a LayoutBoxModelObject or LayoutSVGContainer; - the location is different from visual rect location (if they are same, we can just get previous location from previous visual rect] location) Also renamed the field to previousLocationInBacking. Stat shows that only 4.6% of all LayoutObjects need to save the previous location (https://ct.skia.org/chromium_perf_runs/ run 1436). With the first criteria, now we don't save previous locations for LayoutTexts and non-container LayoutSVGModelObjects, because checking change of visual rect suffices to detect layout caused paint invalidation for them. This makes space for storing previous paint offset for all objects, which is needed by incremental paint property tree update, and paint offset change detection. BUG=660195 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
https://codereview.chromium.org/2476813002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/2476813002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutObject.cpp:1212: context.newLocation = !isBoxModelObject() && !isSVGContainer() You don't need a conditional here I think. paintInvalidationState.computeLocationInBacking already does the conditional for you. https://codereview.chromium.org/2476813002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/ObjectPaintInvalidator.cpp (right): https://codereview.chromium.org/2476813002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/ObjectPaintInvalidator.cpp:383: auto it = locationInBackingMap().find(&m_object); This lookup will be slow. How slow? I think you should address the TODO in this patch to avoid making things slower. https://codereview.chromium.org/2476813002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintInvalidator.cpp (right): https://codereview.chromium.org/2476813002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintInvalidator.cpp:159: if (!object.isBoxModelObject() && !object.isSVGContainer()) You say it suffices to use newVisualRect.location() for this objects. I presume if you just let the logic below execute, a lot more objects would get a value that is different than context.newVisualRect.location()? How many? https://codereview.chromium.org/2476813002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintInvalidator.h (right): https://codereview.chromium.org/2476813002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintInvalidator.h:79: // shifts that forces a full invalidation. nit: s/forces/force/
https://codereview.chromium.org/2476813002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/2476813002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutObject.cpp:1212: context.newLocation = !isBoxModelObject() && !isSVGContainer() On 2016/11/04 18:20:06, chrishtr wrote: > You don't need a conditional here I think. > paintInvalidationState.computeLocationInBacking already does > the conditional for you. They are in different paths (PaintInvalidator path vs PaintInvalidationState path). Now put the check into PaintInvalidationState::computePreviousLocationInBacking(). https://codereview.chromium.org/2476813002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/ObjectPaintInvalidator.cpp (right): https://codereview.chromium.org/2476813002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/ObjectPaintInvalidator.cpp:383: auto it = locationInBackingMap().find(&m_object); On 2016/11/04 18:20:07, chrishtr wrote: > This lookup will be slow. How slow? > > I think you should address the TODO in this patch to avoid making things slower. Done. https://codereview.chromium.org/2476813002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintInvalidator.cpp (right): https://codereview.chromium.org/2476813002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintInvalidator.cpp:159: if (!object.isBoxModelObject() && !object.isSVGContainer()) On 2016/11/04 18:20:07, chrishtr wrote: > You say it suffices to use newVisualRect.location() for this objects. I presume > if you just let > the logic below execute, a lot more objects would get a value that is different > than > context.newVisualRect.location()? How many? In the stat result from cluster telemetry: Total number of LayoutObjects: 17,960,850 Objects that location != visualRect.location(): 3,255,651 { LayoutText: 2,438,061 LayoutBox: 138,208 LayoutInline: 427,867 SVG: 264,137 (this may overlap with the above 3 making the total > 3,255,651) } LayoutText seems to use the location of its containing block, making almost all LayoutTexts having location != visualRect.location(). I didn't measure the details of SVG. As the number is small, for simplicity we can just check isText() here. Done. https://codereview.chromium.org/2476813002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintInvalidator.h (right): https://codereview.chromium.org/2476813002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintInvalidator.h:79: // shifts that forces a full invalidation. On 2016/11/04 18:20:07, chrishtr wrote: > nit: s/forces/force/ Done.
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 ========== Move LayoutObject::m_previousPosition... into a global map in ObjectPaintInvalidator Save the field in a global map in ObjectPaintInvalidator only when: - the object is a LayoutBoxModelObject or LayoutSVGContainer; - the location is different from visual rect location (if they are same, we can just get previous location from previous visual rect] location) Also renamed the field to previousLocationInBacking. Stat shows that only 4.6% of all LayoutObjects need to save the previous location (https://ct.skia.org/chromium_perf_runs/ run 1436). With the first criteria, now we don't save previous locations for LayoutTexts and non-container LayoutSVGModelObjects, because checking change of visual rect suffices to detect layout caused paint invalidation for them. This makes space for storing previous paint offset for all objects, which is needed by incremental paint property tree update, and paint offset change detection. BUG=660195 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Move LayoutObject::m_previousPosition... into a global map in ObjectPaintInvalidator Save the field in a global map in ObjectPaintInvalidator only when: - the object is not a LayoutText (because checking change of visual rect suffices to detect layout caused invalidation); - the location is different from visual rect location (if they are same, we can just get previous location from previous visual rect] location) Also renamed the field to previousLocationInBacking. Stat shows that only 4.6% of all LayoutObjects need to save the previous location (https://ct.skia.org/chromium_perf_runs/ run 1436). This makes space for storing previous paint offset for all objects, which is needed by incremental paint property tree update, and paint offset change detection. BUG=660195 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Description was changed from ========== Move LayoutObject::m_previousPosition... into a global map in ObjectPaintInvalidator Save the field in a global map in ObjectPaintInvalidator only when: - the object is not a LayoutText (because checking change of visual rect suffices to detect layout caused invalidation); - the location is different from visual rect location (if they are same, we can just get previous location from previous visual rect] location) Also renamed the field to previousLocationInBacking. Stat shows that only 4.6% of all LayoutObjects need to save the previous location (https://ct.skia.org/chromium_perf_runs/ run 1436). This makes space for storing previous paint offset for all objects, which is needed by incremental paint property tree update, and paint offset change detection. BUG=660195 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Move LayoutObject::m_previousPosition... into a global map in ObjectPaintInvalidator Save the field in a global map in ObjectPaintInvalidator only when: - the object is not a LayoutText (because checking change of visual rect suffices to detect layout caused invalidation); - the location is different from visual rect location (if they are same, we can just get previous location from previous visual rect) Also renamed the field to previousLocationInBacking. Stat shows that only 4.6% of all LayoutObjects need to save the previous location (https://ct.skia.org/chromium_perf_runs/ run 1436). This makes space for storing previous paint offset for all objects, which is needed by incremental paint property tree update, and paint offset change detection. BUG=660195 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2476813002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutObject.h (right): https://codereview.chromium.org/2476813002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutObject.h:1745: bool hasPreviousLocationInBacking() const { Document why this bit is here and what it does.
https://codereview.chromium.org/2476813002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutObject.h (right): https://codereview.chromium.org/2476813002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutObject.h:1745: bool hasPreviousLocationInBacking() const { On 2016/11/04 21:48:07, chrishtr wrote: > Document why this bit is here and what it does. Done.
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...
Message was sent while issue was closed.
Description was changed from ========== Move LayoutObject::m_previousPosition... into a global map in ObjectPaintInvalidator Save the field in a global map in ObjectPaintInvalidator only when: - the object is not a LayoutText (because checking change of visual rect suffices to detect layout caused invalidation); - the location is different from visual rect location (if they are same, we can just get previous location from previous visual rect) Also renamed the field to previousLocationInBacking. Stat shows that only 4.6% of all LayoutObjects need to save the previous location (https://ct.skia.org/chromium_perf_runs/ run 1436). This makes space for storing previous paint offset for all objects, which is needed by incremental paint property tree update, and paint offset change detection. BUG=660195 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Move LayoutObject::m_previousPosition... into a global map in ObjectPaintInvalidator Save the field in a global map in ObjectPaintInvalidator only when: - the object is not a LayoutText (because checking change of visual rect suffices to detect layout caused invalidation); - the location is different from visual rect location (if they are same, we can just get previous location from previous visual rect) Also renamed the field to previousLocationInBacking. Stat shows that only 4.6% of all LayoutObjects need to save the previous location (https://ct.skia.org/chromium_perf_runs/ run 1436). This makes space for storing previous paint offset for all objects, which is needed by incremental paint property tree update, and paint offset change detection. BUG=660195 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Move LayoutObject::m_previousPosition... into a global map in ObjectPaintInvalidator Save the field in a global map in ObjectPaintInvalidator only when: - the object is not a LayoutText (because checking change of visual rect suffices to detect layout caused invalidation); - the location is different from visual rect location (if they are same, we can just get previous location from previous visual rect) Also renamed the field to previousLocationInBacking. Stat shows that only 4.6% of all LayoutObjects need to save the previous location (https://ct.skia.org/chromium_perf_runs/ run 1436). This makes space for storing previous paint offset for all objects, which is needed by incremental paint property tree update, and paint offset change detection. BUG=660195 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Move LayoutObject::m_previousPosition... into a global map in ObjectPaintInvalidator Save the field in a global map in ObjectPaintInvalidator only when: - the object is not a LayoutText (because checking change of visual rect suffices to detect layout caused invalidation); - the location is different from visual rect location (if they are same, we can just get previous location from previous visual rect) Also renamed the field to previousLocationInBacking. Stat shows that only 4.6% of all LayoutObjects need to save the previous location (https://ct.skia.org/chromium_perf_runs/ run 1436). This makes space for storing previous paint offset for all objects, which is needed by incremental paint property tree update, and paint offset change detection. BUG=660195 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Committed: https://crrev.com/31922c6cd1181c77ae039adf22243cae6ca526f3 Cr-Commit-Position: refs/heads/master@{#430095} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/31922c6cd1181c77ae039adf22243cae6ca526f3 Cr-Commit-Position: refs/heads/master@{#430095} |
