Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2633383004:
Avoid unnecessary FloatRect<->LayoutRect conversions in PaintInvalidator (Closed)
DescriptionAvoid unnecessary FloatRect<->LayoutRect conversions in PaintInvalidator
Make the static functions templates, to avoid unnecessary conversions
between FloatRects and LayoutRects;
Code size will not increase much because the functions are called only
from 1 or 2 places.
BUG=678597
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Review-Url: https://codereview.chromium.org/2633383004
Cr-Commit-Position: refs/heads/master@{#444459}
Committed: https://chromium.googlesource.com/chromium/src/+/9b27859765e32d5de88395f802d9a861e2feac96
Patch Set 1 #
Total comments: 5
Patch Set 2 : Inline PaintInvalidation functions and avoid unnecessary conversions #
Total comments: 4
Patch Set 3 : Seperate inline change #
Dependent Patchsets: Messages
Total messages: 23 (16 generated)
Description was changed from ========== Separate LayoutRect and FloatRect paths in PaintInvalidator This avoids conversions back and forth between LayoutRects and FloatRects which contributed about 2% of the total PrePaint time. BUG=678597 ========== to ========== Separate LayoutRect and FloatRect paths in PaintInvalidator This avoids conversions back and forth between LayoutRects and FloatRects which contributed about 2% of the total PrePaint time. BUG=678597 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...
wangxianzhu@chromium.org changed reviewers: + pdr@chromium.org
https://codereview.chromium.org/2633383004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/paint/PaintInvalidator.cpp (right): https://codereview.chromium.org/2633383004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintInvalidator.cpp:34: LayoutRect result(rect); (Just curious, not needed for this patch) Do these local result values cost anything? For example, for all of these helpers, could we pass in a "LayoutRect&" and modify it in-place (like you did in computeVisualRectInBacking)? https://codereview.chromium.org/2633383004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintInvalidator.cpp:44: static LayoutRect mapLocalRectToPaintInvalidationContainerUsingPaintProperties( Nit: does "UsingPaintProperties" add information? I wonder if we could just use "mapLocalRectToPaintInvalidationContainer". https://codereview.chromium.org/2633383004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintInvalidator.cpp:146: static LayoutRect mapSVGLocalRectToPaintInvalidationBacking( I like the perf benefit here, but I don't like how this copies the SVG-specific parts of mapLocalRectToPaintInvalidationBacking so we have to maintain the parallel SVG codepath in mapLocalRectToPaintInvalidationBacking. Would it be possible to templatize mapLocalRectToPaintInvalidationBacking like you did for mapLocalRectToPaintInvalidationContainerUsingPaintProperties? If not, does the SVG selection rect code need to be in LayoutRects?
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_TIMED_OUT, no build URL)
Description was changed from ========== Separate LayoutRect and FloatRect paths in PaintInvalidator This avoids conversions back and forth between LayoutRects and FloatRects which contributed about 2% of the total PrePaint time. BUG=678597 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Inline PaintInvalidation functions and avoid unnecessary conversions - Make the static functions templates, to avoid unnecessary conversions between FloatRects and LayoutRects; - Inline the private methods of PaintInvalidator to avoid cost of function entry/exit. Code size will not increase much because the functions are called only from 1 or 2 places. BUG=678597 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...
https://codereview.chromium.org/2633383004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/paint/PaintInvalidator.cpp (right): https://codereview.chromium.org/2633383004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintInvalidator.cpp:34: LayoutRect result(rect); On 2017/01/18 02:31:19, pdr. wrote: > (Just curious, not needed for this patch) > Do these local result values cost anything? For example, for all of these > helpers, could we pass in a "LayoutRect&" and modify it in-place (like you did > in computeVisualRectInBacking)? Tested and found that "LayoutRect&" is better than "const LayoutRect&" and LayoutRect result in a standalone function, but has no difference in an inline function. https://codereview.chromium.org/2633383004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintInvalidator.cpp:146: static LayoutRect mapSVGLocalRectToPaintInvalidationBacking( On 2017/01/18 02:31:19, pdr. wrote: > I like the perf benefit here, but I don't like how this copies the SVG-specific > parts of mapLocalRectToPaintInvalidationBacking so we have to maintain the > parallel SVG codepath in mapLocalRectToPaintInvalidationBacking. > > Would it be possible to templatize mapLocalRectToPaintInvalidationBacking like > you did for mapLocalRectToPaintInvalidationContainerUsingPaintProperties? If > not, does the SVG selection rect code need to be in LayoutRects? Good idea. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM https://codereview.chromium.org/2633383004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintInvalidator.cpp (right): https://codereview.chromium.org/2633383004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintInvalidator.cpp:47: template <typename Rect, typename Point> Can you add the following comment here: // This function is templatized to avoid FloatRect<->LayoutRect conversions which affect performance. https://codereview.chromium.org/2633383004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintInvalidator.cpp:142: inline LayoutRect PaintInvalidator::computeVisualRectInBacking( Could you split the inline changes into a separate patch? I believe they help but would like to be able to separate out the effect on the perf bots.
https://codereview.chromium.org/2633383004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintInvalidator.cpp (right): https://codereview.chromium.org/2633383004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintInvalidator.cpp:47: template <typename Rect, typename Point> On 2017/01/18 17:57:00, pdr. wrote: > Can you add the following comment here: > // This function is templatized to avoid FloatRect<->LayoutRect conversions > which affect performance. Done. https://codereview.chromium.org/2633383004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintInvalidator.cpp:142: inline LayoutRect PaintInvalidator::computeVisualRectInBacking( On 2017/01/18 17:57:00, pdr. wrote: > Could you split the inline changes into a separate patch? I believe they help > but would like to be able to separate out the effect on the perf bots. Done.
Description was changed from ========== Inline PaintInvalidation functions and avoid unnecessary conversions - Make the static functions templates, to avoid unnecessary conversions between FloatRects and LayoutRects; - Inline the private methods of PaintInvalidator to avoid cost of function entry/exit. Code size will not increase much because the functions are called only from 1 or 2 places. BUG=678597 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Avoid unnecessary FloatRect<->LayoutRect conversions in PaintInvalidator Make the static functions templates, to avoid unnecessary conversions between FloatRects and LayoutRects; Code size will not increase much because the functions are called only from 1 or 2 places. BUG=678597 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
The CQ bit was checked by wangxianzhu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org Link to the patchset: https://codereview.chromium.org/2633383004/#ps40001 (title: "Seperate inline change")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1484763764774250,
"parent_rev": "79bac8b36367042a620658e5307cfb27dde8985a", "commit_rev":
"9b27859765e32d5de88395f802d9a861e2feac96"}
Message was sent while issue was closed.
Description was changed from ========== Avoid unnecessary FloatRect<->LayoutRect conversions in PaintInvalidator Make the static functions templates, to avoid unnecessary conversions between FloatRects and LayoutRects; Code size will not increase much because the functions are called only from 1 or 2 places. BUG=678597 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Avoid unnecessary FloatRect<->LayoutRect conversions in PaintInvalidator Make the static functions templates, to avoid unnecessary conversions between FloatRects and LayoutRects; Code size will not increase much because the functions are called only from 1 or 2 places. BUG=678597 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2633383004 Cr-Commit-Position: refs/heads/master@{#444459} Committed: https://chromium.googlesource.com/chromium/src/+/9b27859765e32d5de88395f802d9... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/9b27859765e32d5de88395f802d9... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
