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.
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.
Created attachment 154366 [details] grid demo
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.
How many of the allocated paths are actually unused? We could certainly make Path not allocate a CGPath until it really needs one.
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.
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.
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.
Created attachment 154504 [details] wip
I'm curious as to other's impressions of this approach. I've not looked into this optimization for other ports yet.
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.
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.
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.
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.
Loading an empty spreadsheet from drive.google.com: 5237 created, 4861 destroyed null (92%!) :) Are those numbers helpful Simon?
Nice data! Yes, so I think your approach here is worth pursuing.
Created attachment 154541 [details] another wip
In writing this, I found that all the various Path implementations have little incompatible quirks. We really need a way to unittest Path.
Comment on attachment 154541 [details] another wip Attachment 154541 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13351509
Comment on attachment 154541 [details] another wip Attachment 154541 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13343605
Created attachment 154551 [details] another wip
Comment on attachment 154551 [details] another wip Attachment 154551 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13341572
Comment on attachment 154551 [details] another wip Attachment 154551 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13350580
Created attachment 154573 [details] another wip
Comment on attachment 154573 [details] another wip Removing from queue
Created attachment 154872 [details] fix chromium crashers?
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.
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
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
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
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
Created attachment 155002 [details] include missing skia changes?
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
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
Created attachment 155318 [details] fix chromium pixel tests
Created attachment 155348 [details] Patch
The final patch is identical to attachment 155318 [details], except has an updated ChangeLog. I expect all the EWS bots to roll green again.
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.
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.
Created attachment 155377 [details] Patch for landing
(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.
Comment on attachment 155377 [details] Patch for landing Clearing flags on attachment: 155377 Committed r124135: <http://trac.webkit.org/changeset/124135>
All reviewed patches have been landed. Closing bug.