Bug 86340

Summary: [CG] CGImageCreateWithImageInRect is too slow, but for now we still need to use it
Product: WebKit Reporter: Darin Adler <darin>
Component: PlatformAssignee: Darin Adler <darin>
Status: RESOLVED FIXED    
Severity: Normal CC: ddkilzer, ggaren, mitz, sam, simon.fraser, thorton, webkit-bug-importer, webkit.review.bot, yongjun_zhang
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Macintosh   
OS: All   
Attachments:
Description Flags
First cut; needs performance testing
none
Patch none

Description Darin Adler 2012-05-13 23:53:30 PDT
[CG] CGImageCreateImageWithRect is too slow, but for now we still need to use it
Comment 1 Darin Adler 2012-05-13 23:57:12 PDT
Created attachment 141653 [details]
First cut; needs performance testing
Comment 2 Darin Adler 2012-05-13 23:58:32 PDT
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.
Comment 3 Tim Horton 2012-05-14 00:03:50 PDT
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?
Comment 4 Darin Adler 2012-05-14 00:08:33 PDT
(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.
Comment 5 Tim Horton 2012-05-14 00:09:48 PDT
(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?
Comment 6 mitz 2012-05-14 00:14:12 PDT
Did you mean CGImageCreateWithImageInRect?
Comment 7 Darin Adler 2012-05-14 10:08:52 PDT
(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.
Comment 8 Geoffrey Garen 2012-05-14 12:32:43 PDT
(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.
Comment 9 Radar WebKit Bug Importer 2012-05-14 17:45:05 PDT
<rdar://problem/11451243>
Comment 10 Darin Adler 2012-05-14 20:20:56 PDT
Created attachment 141846 [details]
Patch
Comment 11 Darin Adler 2012-05-14 20:21:41 PDT
We measured a significant speedup on the IE fish tank test; I think this is ready to land.
Comment 12 Geoffrey Garen 2012-05-15 00:31:10 PDT
Comment on attachment 141846 [details]
Patch

r=me
Comment 13 WebKit Review Bot 2012-05-15 09:22:10 PDT
Comment on attachment 141846 [details]
Patch

Clearing flags on attachment: 141846

Committed r117083: <http://trac.webkit.org/changeset/117083>
Comment 14 WebKit Review Bot 2012-05-15 09:22:15 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 Darin Adler 2012-05-30 09:41:43 PDT
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.
Comment 16 Simon Fraser (smfr) 2013-02-28 11:34:25 PST
I see subimage caching being expensive on http://www.thewildernessdowntown.com
Comment 17 Darin Adler 2013-03-01 10:02:48 PST
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.