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

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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)]

Created attachment 154366 [details]
grid demo

[Julien Chaffraix]

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)]

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)]

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)]

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)]

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)]

Created attachment 154504 [details]
wip

[Eric Seidel (no email)]

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 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)]

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)]

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)]

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)]

Loading an empty spreadsheet from drive.google.com: 5237 created, 4861 destroyed null (92%!) :)

Are those numbers helpful Simon?

[Simon Fraser (smfr)]

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

[Eric Seidel (no email)]

Created attachment 154541 [details]
another wip

[Eric Seidel (no email)]

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 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 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)]

Created attachment 154551 [details]
another wip

[WebKit Review Bot]

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 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)]

Created attachment 154573 [details]
another wip

[Adam Barth]

Comment on attachment 154573 [details]
another wip

Removing from queue

[Eric Seidel (no email)]

Created attachment 154872 [details]
fix chromium crashers?

[Eric Seidel (no email)]

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 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]

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 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]

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)]

Created attachment 155002 [details]
include missing skia changes?

[WebKit Review Bot]

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]

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)]

Created attachment 155318 [details]
fix chromium pixel tests

[Eric Seidel (no email)]

Created attachment 155348 [details]
Patch

[Eric Seidel (no email)]

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)]

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 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)]

Created attachment 155377 [details]
Patch for landing

[Eric Seidel (no email)]

(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 on attachment 155377 [details]
Patch for landing

Clearing flags on attachment: 155377

Committed r124135: <http://trac.webkit.org/changeset/124135>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.