Bug 11904 – SVGPaintServerPatternCg should cache CGPatternRef (m_pattern) for speed

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

This is a continuation from bug 10817.

[Nikolas Zimmermann]

Created attachment 12058 [details]
Initial patch

No new regressions -> CGPatternRef's are properly cached now.

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?

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

Created attachment 12062 [details]
Updated patch

Mitz is correct, fixed the issues he mentioned.

[Nikolas Zimmermann]

Created attachment 12063 [details]
Final patch

Seperating the alpha handling changes, as they don't belong in this patchset.

[Nikolas Zimmermann]

Created attachment 12064 [details]
Final patch (II)

Eric mentioned some more optimizations. Fixed.

[Eric Seidel (no email)]

Comment on attachment 12064 [details]
Final patch (II)

We discussed a couple further optimizations.  Wow what a pretty patch. :)

[Nikolas Zimmermann]

Landed in r18433.

[Nikolas Zimmermann]

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.