Bug 11904

Summary: SVGPaintServerPatternCg should cache CGPatternRef (m_pattern) for speed
Product: WebKit Reporter: Eric Seidel (no email) <eric>
Component: SVGAssignee: Nikolas Zimmermann <zimmermann>
Status: RESOLVED FIXED    
Severity: Normal CC: mitz
Priority: P2    
Version: 420+   
Hardware: Mac   
OS: OS X 10.4   
Attachments:
Description Flags
Initial patch
none
Updated patch
none
Final patch
none
Final patch (II) eric: review+

Eric Seidel (no email)
Reported 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.
Attachments
Initial patch (6.90 KB, patch)
2006-12-27 06:23 PST, Nikolas Zimmermann
no flags
Updated patch (6.80 KB, patch)
2006-12-27 09:13 PST, Nikolas Zimmermann
no flags
Final patch (5.96 KB, patch)
2006-12-27 09:34 PST, Nikolas Zimmermann
no flags
Final patch (II) (5.84 KB, patch)
2006-12-27 09:51 PST, Nikolas Zimmermann
eric: review+
Eric Seidel (no email)
Comment 1 2006-12-21 04:04:17 PST
This is a continuation from bug 10817.
Nikolas Zimmermann
Comment 2 2006-12-27 06:23:59 PST
Created attachment 12058 [details] Initial patch No new regressions -> CGPatternRef's are properly cached now.
mitz
Comment 3 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?
mitz
Comment 4 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()?
Nikolas Zimmermann
Comment 5 2006-12-27 09:13:55 PST
Created attachment 12062 [details] Updated patch Mitz is correct, fixed the issues he mentioned.
Nikolas Zimmermann
Comment 6 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.
Nikolas Zimmermann
Comment 7 2006-12-27 09:51:58 PST
Created attachment 12064 [details] Final patch (II) Eric mentioned some more optimizations. Fixed.
Eric Seidel (no email)
Comment 8 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. :)
Nikolas Zimmermann
Comment 9 2006-12-27 10:00:01 PST
Landed in r18433.
Nikolas Zimmermann
Comment 10 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.
Note You need to log in before you can comment on or make changes to this bug.