Bug 30055 - Bad DOM performance in large SVG files
Summary: Bad DOM performance in large SVG files
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows XP
: P2 Normal
Assignee: Nobody
URL: http://paroga.com/webkit/rect.php?c=1...
Keywords:
Depends on: 32815 33012
Blocks:
  Show dependency treegraph
 
Reported: 2009-10-04 03:08 PDT by Patrick R. Gansterer
Modified: 2010-02-17 11:54 PST (History)
12 users (show)

See Also:


Attachments
Paint RenderPath only when necessary (1.34 KB, patch)
2009-10-18 09:10 PDT, Patrick R. Gansterer
eric: review-
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Paint RenderPath only when necessary (1.51 KB, patch)
2009-12-30 05:47 PST, Patrick R. Gansterer
no flags Details | Formatted Diff | Diff
Bad DOM performance in large SVG files (3.79 KB, patch)
2010-01-10 15:18 PST, Oliver Hunt
no flags Details | Formatted Diff | Diff
Bad DOM performance in large SVG files (5.53 KB, patch)
2010-01-10 15:19 PST, Oliver Hunt
no flags Details | Formatted Diff | Diff
Patch (2.98 KB, patch)
2010-01-15 03:49 PST, Oliver Hunt
no flags Details | Formatted Diff | Diff
Patch (3.58 KB, patch)
2010-01-15 03:51 PST, Oliver Hunt
no flags Details | Formatted Diff | Diff
Patch (4.21 KB, patch)
2010-01-15 12:48 PST, Oliver Hunt
no flags Details | Formatted Diff | Diff
Patch (1.96 KB, patch)
2010-01-15 16:04 PST, Oliver Hunt
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Patrick R. Gansterer 2009-10-04 03:08:50 PDT
When you click on the 1st rect all rects get yellow, on the 2nd they get blue.
When you click on the 3rd the first rect gets lime, on the 4th it gets red.

Setting only one rect is as slow as setting all rects.
In Firefox 3 you see a big difference between setting all or only one rect.

With the c-parameter you can change the count of the rects, with the s-parameter the size.

Setting only one rect is much faster in http://paroga.com/webkit/rect.php?c=4 than in http://paroga.com/webkit/rect.php?c=10000.
In Firefox there is no difference.
Comment 1 Patrick R. Gansterer 2009-10-04 13:01:58 PDT
Most of the time ist spent in:
RenderLayer::paintLayer
RenderSVGRoot::paint

It seems that the complete SVG will be rendered every time.

I tested it with the Qt port where the dirtyRect is always the size of the whole frame.
Comment 2 Patrick R. Gansterer 2009-10-06 06:28:27 PDT
The same page as html (HTML:div insted of SVG:rect):
http://paroga.com/webkit/div.php?c=4
http://paroga.com/webkit/div.php?c=10000

When only setting a single rect (click on rect 3 or 4) the HTML version is much faster. When changing the color of all rects the SVG version is faster.
Comment 3 Eric Seidel (no email) 2009-10-07 12:47:18 PDT
I'm not sure I see the difference.  WebKit only seems to be drawing the SVG rects as their added.  Using Quartz Debug, I do not see it repainting the whole SVG every time.

Bug 14015 is sorta related (drawing too much).

I think we need to see a better example to see the performance difference, if there is one.  Your example does not convince me that we're drawing too much.  However having written the code, I believe you that we're drawing too much. :)
Comment 4 Patrick R. Gansterer 2009-10-07 22:37:11 PDT
(In reply to comment #3)
> I'm not sure I see the difference.  WebKit only seems to be drawing the SVG
> rects as their added.  Using Quartz Debug, I do not see it repainting the whole
> SVG every time.
Have you tried it wit rect.php?c=10000&s=3? Maybay it there to many rects out of the frame, you can't see a difference. It is not the inital rendering, you can see the difference only when click on rect 1, 2, 3 or 4.

I think that the dirtyRegion is calculatet correctly, but nothing cares about it (at least in the Qt port)
Comment 5 Eric Seidel (no email) 2009-10-08 13:23:54 PDT
OK, so clicking is important, yes.  I didn't realize I had to click on them.

I tried these just now in Safari 4.0.3 and SVG was correctly redrawing only one svg rect when toggling the color on one rect.  If Qt is re-drawing the whole page, that seems like a qt bug.  I did not try on tip of tree, but I kinda doubt this regressed... but maybe it did.
Comment 6 Patrick R. Gansterer 2009-10-08 13:44:40 PDT
(In reply to comment #5)
1) open http://paroga.com/webkit/rect.php?c=4&s=3
2) click on the 1st rect and stop the time until the first rect changes its color
3) click on the 3rd rect and stop the time until the first rect changes its color
4) open http://paroga.com/webkit/rect.php?c=10000&s=3
5) click on the 1st rect and stop the time until the first rect changes its color
6) click on the 3rd rect and stop the time until the first rect changes its color
7) time in step 6 is much bigger than in step 3

Both times should be the same, because only one rect changes it's color. When time in step 2 is much bigger than in step 5 this is ok, because in the second page are much more items to be redrawn.

You can see the expected result when you open the page with Firefox.
Comment 7 Patrick R. Gansterer 2009-10-18 09:10:27 PDT
Created attachment 41379 [details]
Paint RenderPath only when necessary

With this path clicking on the 3rd or 4th rect in http://paroga.com/webkit/rect.php?c=10000&s=3 is about 4 times faster on my computer. (time from click event until the first rect changes it's color)
I'm not sure if this is the right place for my changes.
Comment 8 Eric Seidel (no email) 2009-10-20 23:17:48 PDT
This looks great!  I'd like to test the patch myself first before I r+ this though.
Comment 9 Oliver Hunt 2009-10-28 12:52:31 PDT
Comment on attachment 41379 [details]
Paint RenderPath only when necessary

Why are you checking for intersection prior to applying the local transform to the paintInfo region?
Comment 10 Patrick R. Gansterer 2009-10-28 13:40:14 PDT
(In reply to comment #9)
> Why are you checking for intersection prior to applying the local transform to
> the paintInfo region?
I only tried to find an easy way to demonstate the performance problem. The paintInfo.context->save(); stuff takes a significant amount of time on my machine. In my optinion there must be a better way to handle "DOM repaints" in general. Always iterateing over _all_ items isn't very cool.
Maybe you can confirm this issue and tell me the prefered solution.

If you take a look at http://paroga.com/webkit/livedemo.php you can see a (not so good) example what people do with SVG.
Comment 11 Eric Seidel (no email) 2009-10-28 14:49:24 PDT
This patch is exactly the right idea.  I just need to confirm that it's implemented correctly.  I need to make space in my schedule to test your patch locally and check to see if other if statements like this one should be added to SVG renderers.
Comment 12 Eric Seidel (no email) 2009-10-29 15:56:25 PDT
Ok, I'm not able to reproduce the slowness you mention about clicking on the 3rd or 4th rect in http://paroga.com/webkit/rect.php?c=10000&s=3.

That example is actually really poor, because it requires someone to know that they're supposed to click on a special rect to demonstrate the bug.  Much better would be a self-documenting page with buttons one could click on which say what they do.

I agree we should probably make a change like this.  I just don't think we've justified it very well up until now.

The pathalogical case that you're trying to demonstrate is a single group containing a large number of path objects (<rect>, etc) where only one of them changes.  In current SVG code, all of them will be asked to paint and then the results will be clipped out. It appears that the clipping is rather cheap in CoreGraphics, but whatever platform you are using is slower.
Comment 13 Patrick R. Gansterer 2009-10-30 01:00:14 PDT
(In reply to comment #12)
> That example is actually really poor, because it requires someone to know that
> they're supposed to click on a special rect to demonstrate the bug.  Much
> better would be a self-documenting page with buttons one could click on which
> say what they do.
I've improved the example.
Comment 14 Patrick R. Gansterer 2009-10-30 12:45:26 PDT
(In reply to comment #12)
> Ok, I'm not able to reproduce the slowness you mention about clicking on the
> 3rd or 4th rect in http://paroga.com/webkit/rect.php?c=10000&s=3.
I can't believe you! ;-) I now compiled r50334 with the Qt-port (4.6 beta) on windows and tested the following patch (with g_checkIntersect true and false):
>  if (paintInfo.context->paintingDisabled() || style()->visibility() ...)
>          return;
> 
> +    FloatRect boundingBox = repaintRectInLocalCoordinates();
> +    if (g_checkIntersect && !boundingBox.intersects(paintInfo.rect))
> +        return;
> +
>      paintInfo.context->save();
>      paintInfo.context->concatCTM(localToParentTransform());
>  
>      SVGResourceFilter* filter = 0;
>  
> -    FloatRect boundingBox = repaintRectInLocalCoordinates();
>      if (paintInfo.phase == PaintPhaseForeground) {
>          PaintInfo savedInfo(paintInfo);
When I open http://paroga.com/webkit/rect.php?c=80000&s=3 with g_checkIntersect = true it takes about 2 seconds to change the color of the first element. With g_checkIntersect = false the same action takes about 10 seconds on my machine.

Maybe the rendering/clipping isn't as time-consuming with your port? Is it possible that you try it with the Qt-port? When i apply this patch to WebCore it can't be a Qt-specific rendering problem?!?
Comment 15 Oliver Hunt 2009-11-09 14:19:36 PST
> Maybe the rendering/clipping isn't as time-consuming with your port? Is it
> possible that you try it with the Qt-port? When i apply this patch to WebCore
> it can't be a Qt-specific rendering problem?!?

WebCore uses the platform's graphics library, if that graphics library has bad perf in some cases it can easily cause all sorts of badness.  I'm unsure whether the solution should be to get Qt to fix this issue (presumably it's a problem in their graphics library) or fix it in WebCore itself (which may negatively impact performance of other platforms)

Maybe this logic should be in GrpahicsContextQt?
Comment 16 Patrick R. Gansterer 2009-11-10 01:01:12 PST
(In reply to comment #15)
> WebCore uses the platform's graphics library, if that graphics library has bad
> perf in some cases it can easily cause all sorts of badness.  I'm unsure
> whether the solution should be to get Qt to fix this issue (presumably it's a
> problem in their graphics library) or fix it in WebCore itself (which may
> negatively impact performance of other platforms)
> 
> Maybe this logic should be in GrpahicsContextQt?
Do you think rendering anything ist better than checking if it is necessary? I think WebCore should render as least as possible. In GrpahicsContextQt there won't be any information to check if painting is required. Rely on a "perfect performing" graphics library isn't the right way in opinion. There might be some other ports with slow graphics librarys too.
Comment 17 Oliver Hunt 2009-11-15 15:01:06 PST
(In reply to comment #16)
> (In reply to comment #15)
> > WebCore uses the platform's graphics library, if that graphics library has bad
> > perf in some cases it can easily cause all sorts of badness.  I'm unsure
> > whether the solution should be to get Qt to fix this issue (presumably it's a
> > problem in their graphics library) or fix it in WebCore itself (which may
> > negatively impact performance of other platforms)
> > 
> > Maybe this logic should be in GrpahicsContextQt?
> Do you think rendering anything ist better than checking if it is necessary? I
> think WebCore should render as least as possible. In GrpahicsContextQt there
> won't be any information to check if painting is required. Rely on a "perfect
> performing" graphics library isn't the right way in opinion. There might be
> some other ports with slow graphics librarys too.

The problem is that if the underlying graphics library is already doing the tests it may have a more efficient way of doing so, add to that the fact that the underlying graphics libraries that do perform sensible clipping optimisations will still be doing their own clip tests this seems like a patch that has bad perf impact on good libraries to deal with poor performance in others.

I suspect if we really do want webcore to work around what is arguably a perf bug in Qts graphics subsystem the fix should be placed in GraphicsContextQt directly
Comment 18 Eric Seidel (no email) 2009-11-15 18:32:46 PST
I think we can use both approaches, and I think that this approach has some usefulness.  It needs to be well tested though.  I also agree with Oliver, that if Qt is not properly ignoring drawing commands which are issued outside of the clip region, than that seems like a major bug in Qt.
Comment 19 Patrick R. Gansterer 2009-11-15 18:48:26 PST
(In reply to comment #17)
> The problem is that if the underlying graphics library is already doing the
> tests it may have a more efficient way of doing so, add to that the fact that
> the underlying graphics libraries that do perform sensible clipping
> optimisations will still be doing their own clip tests this seems like a patch
> that has bad perf impact on good libraries to deal with poor performance in
> others.
So this patch has to move to a higher level where "more intelligent" test can
be done and the clipping should be fixed in GraphicsContextQt directly.

However the following test is much slower(!) in Safari/Chrome than in FF:
Click on "Set first lime/red" at http://paroga.com/webkit/rect.php?c=80000&s=2
On my machine: FF ~150ms, Opera ~300ms, Chrome ~2s, Safari ~10s

How to get the same performance of FF/Opera in WebCore? Any suggestions?
Comment 20 Adam Barth 2009-11-30 12:17:58 PST
Attachment 41379 [details] passed the style-queue
Comment 21 Oliver Hunt 2009-12-10 10:22:12 PST
(In reply to comment #19)
> (In reply to comment #17)
> > The problem is that if the underlying graphics library is already doing the
> > tests it may have a more efficient way of doing so, add to that the fact that
> > the underlying graphics libraries that do perform sensible clipping
> > optimisations will still be doing their own clip tests this seems like a patch
> > that has bad perf impact on good libraries to deal with poor performance in
> > others.
> So this patch has to move to a higher level where "more intelligent" test can
> be done and the clipping should be fixed in GraphicsContextQt directly.
> 
> However the following test is much slower(!) in Safari/Chrome than in FF:
> Click on "Set first lime/red" at http://paroga.com/webkit/rect.php?c=80000&s=2
> On my machine: FF ~150ms, Opera ~300ms, Chrome ~2s, Safari ~10s
> 
> How to get the same performance of FF/Opera in WebCore? Any suggestions?

I don't see this 10s delay you're referring to what platform are you testing on?
Comment 22 Patrick R. Gansterer 2009-12-10 11:12:14 PST
(In reply to comment #21)
> I don't see this 10s delay you're referring to what platform are you testing
> on?
Safari 4.0.4, Windows XP SP3, IBM T60 2GHz, 3GB
Comment 23 Oliver Hunt 2009-12-12 23:22:35 PST
Comment on attachment 41379 [details]
Paint RenderPath only when necessary

I found another testcase where we get pathologically bad perf due to this issue which would imply that there's just too much cost in not clipping prior to calling the graphics library -- my test case just created many many many (~50000 odd) nodes mostly off the screen and this patch represents a huge improvement in that case.

Still not sure why i don't see the perf problems you do in your example, but with a testcase that is too slow this shows up as a clear problem in shark.

r=me.

The patch applies cleanly lets see how the commit bot deals with the changelog.
Comment 24 WebKit Commit Bot 2009-12-13 01:14:20 PST
Comment on attachment 41379 [details]
Paint RenderPath only when necessary

Rejecting patch 41379 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--quiet']" exit_code: 1
Running build-dumprendertree
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 11771 test cases.
svg/batik/filters/filterRegions.svg -> failed

Exiting early after 1 failures. 10019 tests run.
313.92s total testing time

10018 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
6 test cases (<1%) had stderr output
Comment 25 Dirk Schulze 2009-12-13 01:23:31 PST
(In reply to comment #23)
> (From update of attachment 41379 [details])
> I found another testcase where we get pathologically bad perf due to this issue
> which would imply that there's just too much cost in not clipping prior to
> calling the graphics library -- my test case just created many many many
> (~50000 odd) nodes mostly off the screen and this patch represents a huge
> improvement in that case.

offscreen:
It depends. If the SVG size is also bigger than the screensize, we shouldn't clip to the currently visible area. smfr remind me, that we can include a SVG into a HTML-file and make some 3D transformations (via CSS3) with it. That might not work correctly, if the SVG is clipped.
Comment 26 Oliver Hunt 2009-12-13 12:59:51 PST
(In reply to comment #25)
> (In reply to comment #23)
> > (From update of attachment 41379 [details] [details])
> > I found another testcase where we get pathologically bad perf due to this issue
> > which would imply that there's just too much cost in not clipping prior to
> > calling the graphics library -- my test case just created many many many
> > (~50000 odd) nodes mostly off the screen and this patch represents a huge
> > improvement in that case.
> 
> offscreen:
> It depends. If the SVG size is also bigger than the screensize, we shouldn't
> clip to the currently visible area. smfr remind me, that we can include a SVG
> into a HTML-file and make some 3D transformations (via CSS3) with it. That
> might not work correctly, if the SVG is clipped.

We should simply be clipping to the region that is going to actually be drawn to, assuming everything is working correctly that is correct -- if i can apply a 3d transform and that will result in incorrect clipping area then thats a serious bug in the 3d transform paths
Comment 27 Eric Seidel (no email) 2009-12-20 19:07:05 PST
Curious what the status of this patch is?  Should this be r-'d now that we know that this patch will at least cause one layout test to fail?
Comment 28 Eric Seidel (no email) 2009-12-28 22:16:01 PST
Comment on attachment 41379 [details]
Paint RenderPath only when necessary

Marking r- since this causes a test to fail.
Comment 29 Eric Seidel (no email) 2009-12-28 22:16:39 PST
Also the second OOPS in the ChangeLog would have caused the commit to fail.  ChangeLogs should mention tests affected by a change.
Comment 30 Patrick R. Gansterer 2009-12-29 08:26:04 PST
(In reply to comment #28)
> (From update of attachment 41379 [details])
> Marking r- since this causes a test to fail.
The problem is that an other area of Webkit does something wrong. This patch is ok, but i depends on an "not created bug". Can we create a new bug, make the patch depend on in r+ it again?  Who is responsible for the code tested with filterRegions.svg? Maybe he can make a change there to get this patch in?
Comment 31 Eric Seidel (no email) 2009-12-29 09:19:56 PST
Yes, if we're waiting to commit this because a problem exists that we've never actually created a bug for, then definitely we should create a bug. :)

Getting an r+ is relatively easy.  Especially since this patch already had one.  But if we can't land it due to another bug, we should file the other bug and mark this one blocked.
Comment 32 Dirk Schulze 2009-12-29 23:52:58 PST
Can we move this fix to SVGRenderBase::prepareToRenderSVGContent??

This would stop rendering earlier and can improve the rendering time on other parts like images too.
Comment 33 Patrick R. Gansterer 2009-12-30 02:19:59 PST
(In reply to comment #31)
> Yes, if we're waiting to commit this because a problem exists that we've never
> actually created a bug for, then definitely we should create a bug. :)
Can we r+ and cq+ it again since bug 32815 is fixed now?

(In reply to comment #32)
> Can we move this fix to SVGRenderBase::prepareToRenderSVGContent??
> 
> This would stop rendering earlier and can improve the rendering time on other
> parts like images too.
I think we should commit this change and maybe another too. This patch is not in the SVG part so it might improve also some other parts.
Comment 34 Dirk Schulze 2009-12-30 03:12:06 PST
> (In reply to comment #32)
> (In reply to comment #32)
> > Can we move this fix to SVGRenderBase::prepareToRenderSVGContent??
> > 
> > This would stop rendering earlier and can improve the rendering time on other
> > parts like images too.
> I think we should commit this change and maybe another too. This patch is not
> in the SVG part so it might improve also some other parts.

RenderPath is SVG only.
Comment 35 Patrick R. Gansterer 2009-12-30 03:57:10 PST
(In reply to comment #34)
> RenderPath is SVG only.
I meant that maybe another componet will use RenderPath in the future, but thats not a real reason.

Moving it to prepareToRenderSVGContent won't prevent _unnecessary_ paintInfo.context->save(), concatCTM(...), restore() and so on.
It should be done as early as possible! Iterating over _all_ items, even if only one pixel changed, is already not very performant. (checking which items need a repaint might be even more cpu consuming)
Comment 36 Dirk Schulze 2009-12-30 04:22:13 PST
(In reply to comment #35)
> (In reply to comment #34)
> > RenderPath is SVG only.
> I meant that maybe another componet will use RenderPath in the future, but
> thats not a real reason.
> 
> Moving it to prepareToRenderSVGContent won't prevent _unnecessary_
> paintInfo.context->save(), concatCTM(...), restore() and so on.
> It should be done as early as possible! Iterating over _all_ items, even if
> only one pixel changed, is already not very performant. (checking which items
> need a repaint might be even more cpu consuming)

Can you please upload a new patch with a link to the bugreport Changelog and the title of the bug? There shouldn't be Oops'es in a Changelog. Please write why it is not possible to test the speed improvement and I think we can commit it.
Comment 37 Patrick R. Gansterer 2009-12-30 05:47:41 PST
Created attachment 45658 [details]
Paint RenderPath only when necessary
Comment 38 WebKit Review Bot 2009-12-30 05:49:17 PST
style-queue ran check-webkit-style on attachment 45658 [details] without any errors.
Comment 39 Dirk Schulze 2009-12-30 08:01:52 PST
Comment on attachment 45658 [details]
Paint RenderPath only when necessary

LGTM. Thanks for the patch.
Comment 40 WebKit Commit Bot 2009-12-30 08:10:48 PST
Comment on attachment 45658 [details]
Paint RenderPath only when necessary

Rejecting patch 45658 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--quiet']" exit_code: 1
Running build-dumprendertree
Running tests from /Users/eseidel/Projects/CommitQueueSVN/LayoutTests
Testing 11854 test cases.
svg/custom/marker-overflow-clip.svg -> failed

Exiting early after 1 failures. 10306 tests run.
313.90s total testing time

10305 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
7 test cases (<1%) had stderr output

Full output: http://webkit-commit-queue.appspot.com/results/152351
Comment 41 Dirk Schulze 2009-12-30 08:56:48 PST
(In reply to comment #40)
> (From update of attachment 45658 [details])
> Rejecting patch 45658 from commit-queue.
> 
> Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari',
> '--exit-after-n-failures=1', '--quiet']" exit_code: 1
> Running build-dumprendertree
> Running tests from /Users/eseidel/Projects/CommitQueueSVN/LayoutTests
> Testing 11854 test cases.
> svg/custom/marker-overflow-clip.svg -> failed
> 
> Exiting early after 1 failures. 10306 tests run.
> 313.90s total testing time
> 
> 10305 test cases (99%) succeeded
> 1 test case (<1%) had incorrect layout
> 7 test cases (<1%) had stderr output
> 
> Full output: http://webkit-commit-queue.appspot.com/results/152351

Ahhh sorry. There is another bug about markers and boundingBoxes: bug 33012 . Without knowing the actual result with your patch, I guess that this fixes the problem.
Comment 42 Eric Seidel (no email) 2010-01-03 18:46:18 PST
Comment on attachment 45658 [details]
Paint RenderPath only when necessary

Marking r- since this seems to cause a test to fail?  Correct me if I'm wrong Dirk.
Comment 43 Dirk Schulze 2010-01-03 23:58:23 PST
(In reply to comment #42)
> (From update of attachment 45658 [details])
> Marking r- since this seems to cause a test to fail?  Correct me if I'm wrong
> Dirk.
No, the patch is ok. It just depends on anotherone. I'll commot-queue+ the patch again, once Niko fixed the marker bug.
Comment 44 Dirk Schulze 2010-01-06 15:33:07 PST
Comment on attachment 45658 [details]
Paint RenderPath only when necessary

Markers and repaintRects are fixed. Lets try it again.
Comment 45 WebKit Commit Bot 2010-01-06 21:30:37 PST
Comment on attachment 45658 [details]
Paint RenderPath only when necessary

Clearing flags on attachment: 45658

Committed r52900: <http://trac.webkit.org/changeset/52900>
Comment 46 WebKit Commit Bot 2010-01-06 21:30:50 PST
All reviewed patches have been landed.  Closing bug.
Comment 47 Eric Seidel (no email) 2010-01-06 21:34:53 PST
Comment on attachment 45658 [details]
Paint RenderPath only when necessary

This type of fix would be more interesting to apply to the hit-testing code path.  But this fix should be fine.  Thanks for the patch Patrick and the review Dirk!
Comment 48 Dimitri Glazkov (Google) 2010-01-07 09:53:32 PST
This change broke a bunch of SVG tests, all pixel results. For illustration, visit this at ToT: http://trac.webkit.org/export/52929/trunk/LayoutTests/fast/backgrounds/size/contain-and-cover.html

I think we should roll this change out.
Comment 49 Dimitri Glazkov (Google) 2010-01-07 10:09:13 PST
Reverted as http://trac.webkit.org/changeset/52930.
Comment 50 Dirk Schulze 2010-01-07 10:23:32 PST
(In reply to comment #48)
> This change broke a bunch of SVG tests, all pixel results. For illustration,
> visit this at ToT:
> http://trac.webkit.org/export/52929/trunk/LayoutTests/fast/backgrounds/size/contain-and-cover.html
> 
> I think we should roll this change out.

A problem of transparent and half transparent parts of some objects?
Comment 51 Oliver Hunt 2010-01-10 15:18:39 PST
Created attachment 46243 [details]
Bad DOM performance in large SVG files
Comment 52 Oliver Hunt 2010-01-10 15:19:10 PST
Created attachment 46244 [details]
Bad DOM performance in large SVG files
Comment 53 Nikolas Zimmermann 2010-01-10 17:02:31 PST
Comment on attachment 46243 [details]
Bad DOM performance in large SVG files

LGTM, I assume there are no pixel test regressions, r+.
Comment 54 Nikolas Zimmermann 2010-01-10 17:03:27 PST
Comment on attachment 46244 [details]
Bad DOM performance in large SVG files

Interessting that CGPathIsEmpty is that slow... I assume you see a good benefit, so r=me.
Comment 55 Oliver Hunt 2010-01-10 19:05:16 PST
First patch landed in r53057.

I'm actually slightly uneasy about landing the isempty caching so i'm going to see if there's something that can be done to get a bigger win.  That said i think that this patch should resolve all of the perf issues in the main testcase attached to this patch.
Comment 56 Dimitri Glazkov (Google) 2010-01-10 21:03:50 PST
Sorry guys, still no luck on pixel tests. Running "run-webkit-tests --pixel-tests svg" produces: 

Enabling pixel tests with a tolerance of 0.1%
WARNING: Temporarily changing the main display color profile:
	The colors on your screen will change for the duration of the testing.
	This allows the pixel tests to have consistent color values across all machines.
Testing 945 test cases.
svg/W3C-SVG-1.1 .......................................................................
svg/W3C-SVG-1.1/coords-units-01-b.svg -> pixel test failed
.........
svg/W3C-SVG-1.1/filters-composite-02-b.svg -> pixel test failed
..........................................
svg/W3C-SVG-1.1/masking-intro-01-f.svg -> pixel test failed
.
svg/W3C-SVG-1.1/masking-mask-01-b.svg -> pixel test failed
......................................
svg/W3C-SVG-1.1/pservers-grad-03-b.svg -> pixel test failed
...
svg/W3C-SVG-1.1/pservers-grad-06-b.svg -> pixel test failed
.......
svg/W3C-SVG-1.1/pservers-grad-13-b.svg -> pixel hash failed (but pixel test still passes)
....
svg/W3C-SVG-1.1/pservers-grad-17-b.svg -> pixel hash failed (but pixel test still passes)
...
svg/W3C-SVG-1.1/pservers-pattern-01-b.svg -> pixel test failed
.......................................................................
svg/W3C-SVG-1.1/text-deco-01-b.svg -> pixel hash failed (but pixel test still passes)
....
svg/W3C-SVG-1.1/text-path-01-b.svg -> pixel hash failed (but pixel test still passes)
...
svg/W3C-SVG-1.1/text-text-03-b.svg -> pixel hash failed (but pixel test still passes)
....
svg/W3C-SVG-1.1/text-text-07-t.svg -> pixel hash failed (but pixel test still passes)
........
svg/animations .
svg/batik/filters ..
svg/batik/masking .
svg/batik/masking/maskRegions.svg -> pixel test failed
svg/batik/paints ..
svg/batik/paints/patternPreserveAspectRatioA.svg -> pixel test failed
.
svg/batik/paints/patternRegionA.svg -> pixel test failed
.
svg/batik/paints/patternRegions.svg -> pixel test failed
svg/batik/text .....
svg/batik/text/textDecoration2.svg -> pixel hash failed (but pixel test still passes)
..
svg/batik/text/textEffect3.svg -> pixel hash failed (but pixel test still passes)
..
svg/batik/text/textFeatures.svg -> pixel hash failed (but pixel test still passes)
.
svg/batik/text/textGlyphOrientationHorizontal.svg -> pixel hash failed (but pixel test still passes)
.......
svg/batik/text/textOnPathSpaces.svg -> pixel hash failed (but pixel test still passes)
.....
svg/batik/text/textProperties.svg -> pixel hash failed (but pixel test still passes)
.
svg/batik/text/textStyles.svg -> pixel hash failed (but pixel test still passes)
.
svg/batik/text/verticalText.svg -> pixel hash failed (but pixel test still passes)
.
svg/batik/text/verticalTextOnPath.svg -> pixel hash failed (but pixel test still passes)
.
svg/carto.net ..........
svg/css .
svg/css/arrow-with-shadow.svg -> pixel hash failed (but pixel test still passes)
.
svg/css/circle-in-mask-with-shadow.svg -> pixel test failed
..
svg/css/composite-shadow-example.html -> pixel hash failed (but pixel test still passes)
.
svg/css/composite-shadow-with-opacity.html -> pixel hash failed (but pixel test still passes)
....
svg/css/group-with-shadow.svg -> pixel hash failed (but pixel test still passes)
.
svg/css/mask-with-shadow.svg -> pixel test failed
.
svg/css/path-with-shadow.svg -> pixel hash failed (but pixel test still passes)
.
svg/css/stars-with-shadow.html -> pixel hash failed (but pixel test still passes)
svg/custom .......................................
svg/custom/deep-dynamic-updates.svg -> pixel test failed
...................................................
svg/custom/grayscale-gradient-mask.svg -> pixel test failed
............
svg/custom/image-with-transform-clip-filter.svg -> pixel hash failed (but pixel test still passes)
.................
svg/custom/js-late-mask-and-object-creation.svg -> pixel test failed
.
svg/custom/js-late-mask-creation.svg -> pixel test failed
.
svg/custom/js-late-pattern-and-object-creation.svg -> pixel test failed
.
svg/custom/js-late-pattern-creation.svg -> pixel test failed
............
svg/custom/js-update-pattern-child.svg -> pixel test failed
.
svg/custom/js-update-pattern.svg -> pixel test failed
.......................
svg/custom/marker-changes.svg -> pixel hash failed (but pixel test still passes)
.
svg/custom/marker-child-changes.svg -> pixel hash failed (but pixel test still passes)
......
svg/custom/marker-strokeWidth-changes.svg -> pixel hash failed (but pixel test still passes)
..
svg/custom/mask-changes.svg -> pixel test failed
.
svg/custom/mask-child-changes.svg -> pixel test failed
.
svg/custom/mask-excessive-malloc.svg -> pixel test failed
.
svg/custom/mask-inside-defs.svg -> pixel test failed
.
svg/custom/mask-on-multiple-objects.svg -> pixel test failed
.
svg/custom/mask-with-all-units.svg -> pixel test failed
..............
svg/custom/pattern-cycle-detection.svg -> pixel test failed
.
svg/custom/pattern-deep-referencing.svg -> pixel test failed
.
svg/custom/pattern-in-defs.svg -> pixel test failed
.
svg/custom/pattern-rotate.svg -> pixel test failed
.
svg/custom/pattern-with-transformation.svg -> pixel test failed
.
svg/custom/pattern-y-offset.svg -> pixel test failed
...............
svg/custom/radial-gradient-with-outstanding-focalPoint.svg -> pixel test failed
....
svg/custom/resource-invalidate-on-target-update.svg -> pixel test failed
................
svg/custom/stroked-pattern.svg -> pixel test failed
...........................................................................
svg/custom/use-on-symbol-inside-pattern.svg -> pixel test failed
.........................
svg/custom/visibility-override-mask.svg -> pixel test failed
....
svg/dom .........................
svg/dom/SVGRectElement .
svg/dom/SVGScriptElement ....
svg/dom/SVGStyleElement .
svg/dynamic-updates ................................................................................................................................
svg/filters ..........
svg/filters/filter-clip.svg -> pixel test failed
........
svg/hittest ..
svg/hixie/cascade ..
svg/hixie/data-types ..
svg/hixie/dynamic ......
svg/hixie/error ..................
svg/hixie/links ...
svg/hixie/mixed .........
svg/hixie/painting .
svg/hixie/perf .....
svg/hixie/processing-model ...
svg/hixie/rendering-model .....
svg/hixie/shapes/path .
svg/hixie/text .....
svg/hixie/transform .
svg/hixie/use ...
svg/hixie/viewbox ....
svg/hixie/viewbox/preserveAspectRatio ..
svg/text ....
svg/text/selection-background-color.xhtml -> pixel hash failed (but pixel test still passes)
....
svg/text/text-align-04-b.svg -> pixel hash failed (but pixel test still passes)
....
svg/text/text-deco-01-b.svg -> pixel hash failed (but pixel test still passes)
.......
svg/text/text-path-01-b.svg -> pixel hash failed (but pixel test still passes)
....
svg/text/text-text-01-b.svg -> pixel hash failed (but pixel test still passes)
.
svg/text/text-text-03-b.svg -> pixel hash failed (but pixel test still passes)
.
svg/text/text-text-04-t.svg -> pixel hash failed (but pixel test still passes)
...
svg/text/text-text-07-t.svg -> pixel hash failed (but pixel test still passes)
.
svg/text/text-text-08-b.svg -> pixel hash failed (but pixel test still passes)
.......
svg/transforms ..
svg/transforms/text-with-pattern-inside-transformed-html.xhtml -> pixel test failed
.
svg/transforms/text-with-pattern-with-svg-transform.svg -> pixel test failed
svg/webarchive ...
72.68s total testing time

904 test cases (95%) succeeded
41 test cases (4%) had incorrect layout
1 test case (<1%) had stderr output
Comment 58 Dimitri Glazkov (Google) 2010-01-10 21:22:09 PST
Just to make sure, with the patch rolled out locally, same command produces:

944 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
1 test case (<1%) had stderr output
Comment 59 Nikolas Zimmermann 2010-01-11 02:42:42 PST
Comment on attachment 46243 [details]
Bad DOM performance in large SVG files

Clearing review flag, this patch needs to be revisited. Anyone who works on this has to be aware that regressions can only be spotted when running _PIXEL TESTS_.
Comment 60 Nikolas Zimmermann 2010-01-11 02:43:00 PST
Comment on attachment 46244 [details]
Bad DOM performance in large SVG files

Taking back review, as discussed with Oliver.
Comment 61 Oliver Hunt 2010-01-15 03:49:24 PST
Created attachment 46667 [details]
Patch
Comment 62 Oliver Hunt 2010-01-15 03:51:56 PST
Created attachment 46668 [details]
Patch
Comment 63 Nikolas Zimmermann 2010-01-15 04:19:41 PST
Comment on attachment 46668 [details]
Patch

Okay, let's hope it fixes it this time... Can you file a follow-up bug regarding PaintInfo fixes is renderSubtreeToImage?
Comment 64 Oliver Hunt 2010-01-15 04:25:40 PST
Committed r53331: <http://trac.webkit.org/changeset/53331>
Comment 65 Dimitri Glazkov (Google) 2010-01-15 09:21:33 PST
Darnit! One test still broke: http://trac.webkit.org/browser/trunk/LayoutTests/fast/borders/svg-as-border-image-2.html

Can you take a look and see what's happening there?
Comment 66 Dimitri Glazkov (Google) 2010-01-15 09:48:27 PST
Rolled out Oliver's patch in http://trac.webkit.org/changeset/53334, per IRC discussion w/Nikolas.

But you're really, really close now! Don't give up!
Comment 67 Oliver Hunt 2010-01-15 12:48:23 PST
Created attachment 46701 [details]
Patch
Comment 68 Oliver Hunt 2010-01-15 12:54:47 PST
Committed r53343: <http://trac.webkit.org/changeset/53343>
Comment 69 Oliver Hunt 2010-01-15 16:04:23 PST
Created attachment 46715 [details]
Patch
Comment 70 Oliver Hunt 2010-01-15 16:05:44 PST
Just opening for a improvement to culling
Comment 71 Sam Weinig 2010-01-15 16:14:56 PST
Comment on attachment 46715 [details]
Patch

Please add more complete explanation to the ChangeLog.  r=me
Comment 72 Oliver Hunt 2010-01-15 16:16:35 PST
Committed r53349: <http://trac.webkit.org/changeset/53349>