Bug 11904 - SVGPaintServerPatternCg should cache CGPatternRef (m_pattern) for speed
Summary: SVGPaintServerPatternCg should cache CGPatternRef (m_pattern) for speed
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Nikolas Zimmermann
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2006-12-21 04:03 PST by Eric Seidel (no email)
Modified: 2006-12-27 12:38 PST (History)
1 user (show)

See Also:


Attachments
Initial patch (6.90 KB, patch)
2006-12-27 06:23 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Updated patch (6.80 KB, patch)
2006-12-27 09:13 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Final patch (5.96 KB, patch)
2006-12-27 09:34 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Final patch (II) (5.84 KB, patch)
2006-12-27 09:51 PST, Nikolas Zimmermann
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2006-12-21 04:03:54 PST
SVGPaintServerPatternCg should cache CGPatternRef (m_pattern) for speed

Right now m_pattern is destroyed in teardown.  Causing the first CGFillPath call which uses the pattern to rebuild the pattern caches on every draw (since every draw is the first draw with that pattern).

This accounts for over 50% of the time spent drawing plan.svg (a 30MB SVG file).

I did a quick hack to test this.  I only created m_pattern if it had not been created before, and I destroyed m_pattern only when destroying SVGPaintServerPatternCg, or when the tile was set.  That didn't seem to fix the problem though.  So either the tile is being set more often than I thought, the paintserver itself is being regenerated a bunch, or something else is wrong.  Either way, caching the CGPattern instance longer is the right direction to move in.

It might be advisable to talk with one of the CG guys however.
Comment 1 Eric Seidel (no email) 2006-12-21 04:04:17 PST
This is a continuation from bug 10817.
Comment 2 Nikolas Zimmermann 2006-12-27 06:23:59 PST
Created attachment 12058 [details]
Initial patch

No new regressions -> CGPatternRef's are properly cached now.
Comment 3 mitz 2006-12-27 06:47:23 PST
Comment on attachment 12058 [details]
Initial patch

Looks like it's leaking the last pattern and space used when the SVGPaintServerPattern is destroyed. Also, isn't it possible for an ImageBuffer to be shared by two SVGPaintServerPatterns?
Comment 4 mitz 2006-12-27 07:05:37 PST
(In reply to comment #3)
> (From update of attachment 12058 [details] [edit])
> Also, isn't it possible for an ImageBuffer
> to be shared by two SVGPaintServerPatterns?
> 

Looking at the current code, it does indeed seem impossible, so keeping the flag on the ImageBuffer should work. However, having the "contents changed" flag on the ImageBuffer still doesn't feel right for a general ImageBuffer object. As far as I can see, the current code always creates the ImageBuffer, renders into it and then calls setTile, and never draws to the ImageBuffer afterwards, let alone does that without calling setTile again. In this case, it is sufficient (as far as image buffer contents are concerned; see below) for the paint server to invalidate its cache only in setTile().

I noticed that the cached CGPattern depends on the bbox and the tile's size, but you don't invalidate the cache in setBBox(). Is that (also) guaranteed not to be called after setTile()? 
Comment 5 Nikolas Zimmermann 2006-12-27 09:13:55 PST
Created attachment 12062 [details]
Updated patch

Mitz is correct, fixed the issues he mentioned.
Comment 6 Nikolas Zimmermann 2006-12-27 09:34:56 PST
Created attachment 12063 [details]
Final patch

Seperating the alpha handling changes, as they don't belong in this patchset.
Comment 7 Nikolas Zimmermann 2006-12-27 09:51:58 PST
Created attachment 12064 [details]
Final patch (II)

Eric mentioned some more optimizations. Fixed.
Comment 8 Eric Seidel (no email) 2006-12-27 09:56:43 PST
Comment on attachment 12064 [details]
Final patch (II)

We discussed a couple further optimizations.  Wow what a pretty patch. :)
Comment 9 Nikolas Zimmermann 2006-12-27 10:00:01 PST
Landed in r18433.
Comment 10 Nikolas Zimmermann 2006-12-27 12:38:40 PST
Will revert this patch again, as it broke pattern zooming. Either we need a more clever logic for the caching or just don't cache. Needs to be sharked to actually decide it.