Bug 92252

Summary:

Grid Demo spends 1.5% of total time allocating Path objects in RenderBoxModelObject::paintBorderSides

Product:

WebKit

Reporter:

Eric Seidel (no email) <eric>

Component:

Layout and Rendering

Assignee:

Eric Seidel (no email) <eric>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bdakin, dglazkov, fishd, jchaffraix, loki, ossy, reed, senorblanco, simon.fraser, webkit.review.bot, xan.lopez, zherczeg, zoltan

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

92752

Bug Blocks:

92258

Attachments:

Description	Flags
grid demo	none
wip	none
another wip	none
another wip	none
another wip	none
fix chromium crashers?	none
Archive of layout-test-results from gce-cr-linux-08	none
Archive of layout-test-results from gce-cr-linux-05	none
include missing skia changes?	none
Archive of layout-test-results from gce-cr-linux-02	none
fix chromium pixel tests	none
Patch	none
Patch for landing	none

Eric Seidel (no email)

Reported 2012-07-25 07:42:32 PDT

Grid Demo spends 1.5% of total time allocating Path objects in RenderBoxModelObject::paintBorderSides The problem is that Path::Path() actually does a malloc() (at least on Mac), because it holds a reference to a CGPath. void RenderBoxModelObject::paintBorderSides(...) { bool renderRadii = outerBorder.isRounded(); Path roundedPath; if (renderRadii) roundedPath.addRoundedRect(outerBorder); always performs this malloc, even if roundedPath is not needed. I suspect any painting benchmark which includes borders is hitting this.

Attachments
grid demo (2.86 KB, text/html) 2012-07-25 08:33 PDT, Eric Seidel (no email)	no flags	Details
wip (11.29 KB, patch) 2012-07-25 17:58 PDT, Eric Seidel (no email)	no flags	Details Formatted Diff Diff
another wip (39.92 KB, patch) 2012-07-25 22:32 PDT, Eric Seidel (no email)	no flags	Details Formatted Diff Diff
another wip (39.89 KB, patch) 2012-07-25 23:14 PDT, Eric Seidel (no email)	no flags	Details Formatted Diff Diff
another wip (40.15 KB, patch) 2012-07-26 01:15 PDT, Eric Seidel (no email)	no flags	Details Formatted Diff Diff
fix chromium crashers? (40.60 KB, patch) 2012-07-27 01:09 PDT, Eric Seidel (no email)	no flags	Details Formatted Diff Diff
Archive of layout-test-results from gce-cr-linux-08 (357.99 KB, application/zip) 2012-07-27 07:32 PDT, WebKit Review Bot	no flags	Details
Archive of layout-test-results from gce-cr-linux-05 (442.33 KB, application/zip) 2012-07-27 08:19 PDT, WebKit Review Bot	no flags	Details
include missing skia changes? (42.67 KB, patch) 2012-07-27 11:50 PDT, Eric Seidel (no email)	no flags	Details Formatted Diff Diff
Archive of layout-test-results from gce-cr-linux-02 (1.17 MB, application/zip) 2012-07-27 15:46 PDT, WebKit Review Bot	no flags	Details
fix chromium pixel tests (42.98 KB, patch) 2012-07-30 10:43 PDT, Eric Seidel (no email)	no flags	Details Formatted Diff Diff
Patch (45.22 KB, patch) 2012-07-30 13:22 PDT, Eric Seidel (no email)	no flags	Details Formatted Diff Diff
Patch for landing (44.93 KB, patch) 2012-07-30 15:10 PDT, Eric Seidel (no email)	no flags	Details Formatted Diff Diff
Show Obsolete (8) View All Add attachment proposed patch, testcase, etc.

Eric Seidel (no email)

Comment 1 2012-07-25 07:43:50 PDT

One solution would be to use OwnPtr<Path>, but that would cause a double-malloc in the rounded borders case. We might also consider having Path() create a null path which has no CGPath backer. That would add several null-checks into the Path code, but might be a more general win.

Eric Seidel (no email)

Comment 2 2012-07-25 08:33:03 PDT

Created attachment 154366 [details] grid demo

Julien Chaffraix

Comment 3 2012-07-25 08:54:20 PDT

Mike Reed mentioned that for some cases, the platform may know how to do proper borders in a more optimized way than what paintBorderSides does (it is very low-level from that perspective). This optimization couldn't be applied to all paintBorderSides's code path but it would likely help.

Simon Fraser (smfr)

Comment 4 2012-07-25 09:17:18 PDT

How many of the allocated paths are actually unused? We could certainly make Path not allocate a CGPath until it really needs one.

Eric Seidel (no email)

Comment 5 2012-07-25 12:13:39 PDT

I suspect the proper fix here is to make Path not allocate a CGPath until it needs one. I don't know how often that happens, but it clearly happens in this method, and the malloc shows up on profiles, since the grid contains many (simple) borders.

Eric Seidel (no email)

Comment 6 2012-07-25 14:48:18 PDT

Some brief grepping shows that platformPath() is used in 10 or so places (most of them port-specific) and always assumed to be non-null. I think this is doable. It will just require a bit of work. I suspect it's a (small) win on the PLT as well.

Eric Seidel (no email)

Comment 7 2012-07-25 14:49:37 PDT

For example, lines like this: void drawOutlinedQuadWithClip(GraphicsContext& context, const FloatQuad& quad, const FloatQuad& clipQuad, const Color& fillColor) { context.save(); Path clipQuadPath = quadToPath(clipQuad); Currently do 2 mallocs, when they really only need to do 1.

Eric Seidel (no email)

Comment 8 2012-07-25 17:58:21 PDT

Created attachment 154504 [details] wip

Eric Seidel (no email)

Comment 9 2012-07-25 18:04:20 PDT

I'm curious as to other's impressions of this approach. I've not looked into this optimization for other ports yet.

Simon Fraser (smfr)

Comment 10 2012-07-25 18:06:33 PDT

Comment on attachment 154504 [details] wip Seems OK, but I'd like to see some data on how many empty paths we make. Should be trivial with some instrumentation.

Eric Seidel (no email)

Comment 11 2012-07-25 18:08:55 PDT

You'd like to know how many Path objects we make rendering a specific page? Or this demo? Are you interested in how many Paths we destroy while empty? Are you concerned about the performance of the null checks, or about the complexity introduced? I'm just curious what questions I can best answer for you here.

Simon Fraser (smfr)

Comment 12 2012-07-25 18:17:04 PDT

I want to know how much of a win this will be. So I'd like data from the grid page, and maybe when browsing a few "modern" web sites.

Eric Seidel (no email)

Comment 13 2012-07-25 19:03:20 PDT

grid demo: ~1800 Paths created per draw, 100% destroyed with a null m_path. I loaded a few popular front pages: apple.com: 147 Paths created, 9 (6%) destroyed null. google.com: 58 Paths created, 26 (44%) destroyed null (google seems creating 5 paths every second after load? 1 of which is destroyed empty.) amazon.com: 130 Paths created, 130 (100%) destroyed null.

Eric Seidel (no email)

Comment 14 2012-07-25 19:06:14 PDT

Loading an empty spreadsheet from drive.google.com: 5237 created, 4861 destroyed null (92%!) :) Are those numbers helpful Simon?

Simon Fraser (smfr)

Comment 15 2012-07-25 20:05:55 PDT

Nice data! Yes, so I think your approach here is worth pursuing.

Eric Seidel (no email)

Comment 16 2012-07-25 22:32:18 PDT

Created attachment 154541 [details] another wip

Eric Seidel (no email)

Comment 17 2012-07-25 22:33:01 PDT

In writing this, I found that all the various Path implementations have little incompatible quirks. We really need a way to unittest Path.

Gyuyoung Kim

Comment 18 2012-07-25 22:45:17 PDT

Comment on attachment 154541 [details] another wip Attachment 154541 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13351509

WebKit Review Bot

Comment 19 2012-07-25 22:56:11 PDT

Comment on attachment 154541 [details] another wip Attachment 154541 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13343605

Eric Seidel (no email)

Comment 20 2012-07-25 23:14:32 PDT

Created attachment 154551 [details] another wip

WebKit Review Bot

Comment 21 2012-07-25 23:42:51 PDT

Comment on attachment 154551 [details] another wip Attachment 154551 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13341572

Gyuyoung Kim

Comment 22 2012-07-26 00:18:58 PDT

Comment on attachment 154551 [details] another wip Attachment 154551 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13350580

Eric Seidel (no email)

Comment 23 2012-07-26 01:15:03 PDT

Created attachment 154573 [details] another wip

Adam Barth

Comment 24 2012-07-26 11:56:45 PDT

Comment on attachment 154573 [details] another wip Removing from queue

Eric Seidel (no email)

Comment 25 2012-07-27 01:09:42 PDT

Created attachment 154872 [details] fix chromium crashers?

Eric Seidel (no email)

Comment 26 2012-07-27 03:03:56 PDT

My local run showed a bunch of failures, but none look related to my patch. With luck, this patch should be good to go. I'll upload a copy with a pretty ChangeLog tomorrow if the EWSes are green.

WebKit Review Bot

Comment 27 2012-07-27 07:32:07 PDT

Comment on attachment 154872 [details] fix chromium crashers? Attachment 154872 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13376379 New failing tests: fast/borders/borderRadiusDouble07.html canvas/philip/tests/2d.path.clip.empty.html fast/css/nested-rounded-corners.html fast/borders/borderRadiusDouble06.html platform/chromium/virtual/gpu/canvas/philip/tests/2d.path.clip.empty.html

WebKit Review Bot

Comment 28 2012-07-27 07:32:14 PDT

Created attachment 154944 [details] Archive of layout-test-results from gce-cr-linux-08 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-08 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

WebKit Review Bot

Comment 29 2012-07-27 08:19:25 PDT

Comment on attachment 154872 [details] fix chromium crashers? Attachment 154872 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13369489 New failing tests: fast/borders/borderRadiusDouble07.html platform/chromium/virtual/gpu/canvas/philip/tests/2d.path.clip.empty.html fast/css/nested-rounded-corners.html fast/borders/borderRadiusDouble06.html canvas/philip/tests/2d.path.clip.empty.html

WebKit Review Bot

Comment 30 2012-07-27 08:19:31 PDT

Created attachment 154955 [details] Archive of layout-test-results from gce-cr-linux-05 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-05 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Eric Seidel (no email)

Comment 31 2012-07-27 11:50:06 PDT

Created attachment 155002 [details] include missing skia changes?

WebKit Review Bot

Comment 32 2012-07-27 15:46:28 PDT

Comment on attachment 155002 [details] include missing skia changes? Attachment 155002 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13378663 New failing tests: canvas/philip/tests/2d.path.clip.empty.html platform/chromium/virtual/gpu/canvas/philip/tests/2d.path.clip.empty.html

WebKit Review Bot

Comment 33 2012-07-27 15:46:33 PDT

Created attachment 155070 [details] Archive of layout-test-results from gce-cr-linux-02 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Eric Seidel (no email)

Comment 34 2012-07-30 10:43:12 PDT

Created attachment 155318 [details] fix chromium pixel tests

Eric Seidel (no email)

Comment 35 2012-07-30 13:22:58 PDT

Created attachment 155348 [details] Patch

Eric Seidel (no email)

Comment 36 2012-07-30 13:23:52 PDT

The final patch is identical to attachment 155318 [details], except has an updated ChangeLog. I expect all the EWS bots to roll green again.

Eric Seidel (no email)

Comment 37 2012-07-30 13:28:03 PDT

Qt folks might consider moving to a pointer-based Path implementation to match other ports. Or perhaps Skia should consider moving to a value-based Path implementation like Qt, if that's found to be faster. CoreGraphics (and I think Cairo) are stuck using a pointer-based implementation regardless. In any case, Qt will not benefit from this performance enhancement, but it may already be faster than this code by being value-based.

Simon Fraser (smfr)

Comment 38 2012-07-30 14:51:05 PDT

Comment on attachment 155348 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155348&action=review > Source/WebCore/ChangeLog:31 > + I expect that platforms which do not run the pixel tests on build.webkit.org may later discover pixel-only regressions > + from this change, due to possible mistakes on my part while adding null-path support. I'm happy > + to work with those platforms to correct any pixel changes they may find. I don't think you need to say this in the changelog. > Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:1091 > + // Why does clipping to an empty path do nothing? > + // Why is this different from GraphicsContext::clip(const Path&). > if (path.isEmpty()) > return; Does svn history tell you anything? > Source/WebCore/platform/graphics/cg/PathCG.cpp:91 > + if (m_path) > + CGPathRelease(m_path); > +} > + > +PlatformPathPtr Path::ensurePlatformPath() > +{ > + if (!m_path) > + m_path = CGPathCreateMutable(); > + return m_path; Shame we can't use a RetainPtr in the header.

Eric Seidel (no email)

Comment 39 2012-07-30 15:10:16 PDT

Created attachment 155377 [details] Patch for landing

Eric Seidel (no email)

Comment 40 2012-07-30 15:13:45 PDT

(In reply to comment #38) > (From update of attachment 155348 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=155348&action=review > > > Source/WebCore/ChangeLog:31 > > + I expect that platforms which do not run the pixel tests on build.webkit.org may later discover pixel-only regressions > > + from this change, due to possible mistakes on my part while adding null-path support. I'm happy > > + to work with those platforms to correct any pixel changes they may find. > > I don't think you need to say this in the changelog. Removed. > > Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:1091 > > + // Why does clipping to an empty path do nothing? > > + // Why is this different from GraphicsContext::clip(const Path&). > > if (path.isEmpty()) > > return; > > Does svn history tell you anything? The different platforms seem to handle this method differently. I'm not sure why the split, but I expect this is just poorly tested (like Path::currentPoint!) > > Source/WebCore/platform/graphics/cg/PathCG.cpp:91 > > + if (m_path) > > + CGPathRelease(m_path); > > +} > > + > > +PlatformPathPtr Path::ensurePlatformPath() > > +{ > > + if (!m_path) > > + m_path = CGPathCreateMutable(); > > + return m_path; > > Shame we can't use a RetainPtr in the header. Yeah. I think we could, we'd just need to rejigger the #defines a little. It seemed interesting, but out of scope for this patch.

WebKit Review Bot

Comment 41 2012-07-30 18:14:36 PDT

Comment on attachment 155377 [details] Patch for landing Clearing flags on attachment: 155377 Committed r124135: <http://trac.webkit.org/changeset/124135>

WebKit Review Bot

Comment 42 2012-07-30 18:14:44 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.