Bug 86340 – [CG] CGImageCreateWithImageInRect is too slow, but for now we still need to use it

• Component: Platform
• Status: RESOLVED FIXED

[Darin Adler]

[CG] CGImageCreateImageWithRect is too slow, but for now we still need to use it

[Darin Adler]

Created attachment 141653 [details]
First cut; needs performance testing

[Darin Adler]

I suspect that calling TimerBase::start every time is too slow. Some other technique for clearing the cache when idle would probably be more efficient.

[Tim Horton]

Hmm, is this using a cache size based on the number of subimages? I could see a pathological case where someone uses subimages which are nearly the entirety of a large image (or even comparatively small portions of a very large image), and suddenly their memory usage explodes. Can we base it on the total size of the cached subimages instead?

[Darin Adler]

(In reply to comment #3)
> Hmm, is this using a cache size based on the number of subimages? I could see a pathological case where someone uses subimages which are nearly the entirety of a large image (or even comparatively small portions of a very large image), and suddenly their memory usage explodes. Can we base it on the total size of the cached subimages instead?

Subimages don’t copy the bits, so the sizes of the subimages are not relevant.

The sizes of the base images might be relevant.

[Tim Horton]

(In reply to comment #4)
> (In reply to comment #3)
> > Hmm, is this using a cache size based on the number of subimages? I could see a pathological case where someone uses subimages which are nearly the entirety of a large image (or even comparatively small portions of a very large image), and suddenly their memory usage explodes. Can we base it on the total size of the cached subimages instead?
> 
> Subimages don’t copy the bits, so the sizes of the subimages are not relevant.
> 
> The sizes of the base images might be relevant.

Oh, that's comforting.

I wonder why they're so slow to compute, then?

Did you mean CGImageCreateWithImageInRect?

[Darin Adler]

(In reply to comment #5)
> I wonder why they're so slow to compute, then?

I believe the issue is that they are made of multiple memory blocks and memory allocation and deallocation is costly even when the amounts of memory are small. But there may be other factors as well. Allocating and deallocating may be causing trouble with caching inside Core Graphics, for example.

[Geoffrey Garen]

(In reply to comment #2)
> I suspect that calling TimerBase::start every time is too slow. Some other technique for clearing the cache when idle would probably be more efficient.

How about this:
If timer is idle, start it for 1s, else:
    Set a flag saying "the next timer fire should just restart the timer instead of clearing the cache".

This ensures only 1 timer start per second, which solves the cost of TimerBase::start. It has the negative side-effect of making the timer "sloppy". In the pathological case, if you create 1 subimage per second, you'll never clear the cache. This seems acceptable, since the pathological case is, in point of fact, getting value out of the cache, and the cache has an absolute bound at maxSubimageCacheSize.

[Radar WebKit Bug Importer]

<rdar://problem/11451243>

[Darin Adler]

Created attachment 141846 [details]
Patch

[Darin Adler]

We measured a significant speedup on the IE fish tank test; I think this is ready to land.

[Geoffrey Garen]

Comment on attachment 141846 [details]
Patch

r=me

[WebKit Review Bot]

Comment on attachment 141846 [details]
Patch

Clearing flags on attachment: 141846

Committed r117083: <http://trac.webkit.org/changeset/117083>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Later testing showed different results. I am no longer sure this is a speed-up and we may want to either turn it off or roll it out.

[Simon Fraser (smfr)]

I see subimage caching being expensive on http://www.thewildernessdowntown.com

[Darin Adler]

Feel free to turn this off or roll this out as per my 2012-05-30 comment. Especially if we have test data indicating that’s the right thing to do.