414685 - image downscaling slower than fx2

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: scaling benchmark

Spin-off bug from bug 412396.  Attached is a benchmark for image scaling; needs the JPEG subdir as from the other bug.

One problem is that the benchmark doesn't seem to work in Fx2 -- the scrollTo()'s are just ignored, so I can't get useful numbers.

Looking at the profile from Fx3, we're spending all our time deep inside Quartz, which is where things should be; I'm not sure what wins we can do here, but I need to see the Fx2 numbers first.  Working on figuring out how to get that.

Comment 1

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: patch

welp.  I thought CGBitmapContextCreateImage was fast (just returning a reference to an internal image), guess not!  Let's cache it for wins.

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: patch, part 2

Part 2 of the patch, to make it play nice with the image loading optimization in the other bug.  Without this, we end up caching an early version of the image data, and bad things happen.

I may have a third patch here that might help with memory usage as well.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 300205 [details] [diff] [review]
patch

Looks good but please document the ownership rules here, namely that contextImage has an associated ref to the image. Also document that it's imperative for performance that we drop all references to the contextImage before we draw to the surface.

Sometime you should explain to me why this is important for scaled image performance but not normal images.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Actually, explaining that in some comments in the code would be good in case anyone wonders why you're doing this.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

For performance, shouldn't ImageUpdated be ImageWillUpdate so we can wipe out the image before we change the surface?

Comment 6

[Stuart Parmenter]

Comment on attachment 300206 [details] [diff] [review]
patch, part 2

Using ImageUpdated is going to be fragile.  I'm pretty sure there are already places that need this called.  It would be a lot nicer to have a read only surface that was copy on write or something...

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The Quartz image is copy-on-write (or something like that, says the documentation). The problem is, we don't want the copy to happen because a) that's expensive and b) it would be incorrect for us to use the stale copy. So we have to do some kind of ImageUpdated/ImageWillUpdate anyway.

Comment 8

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: door #2

No review requests yet, need to run benchmarks tomorrow first.

Comment 9

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 300322 [details] [diff] [review]
door #2

Approve this for big wins.

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 300322 [details] [diff] [review]
door #2

a=beltzner for 1.9, but not for beta, sorry - gotta keep some of our perf wins for later! :)

Comment 11

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Checked in.  Sadly, no massive Tp drop!

Comment 12

[Mike Schroepfer]

Confirming a 20-50x speedup from yesterday's trunk to today on the scale benchmark.

Comment 13

[Mike Schroepfer]

This didn't register on Tgfx either:

http://graphs.mozilla.org/#spst=range&spstart=1200616813&spend=1200679929&bpst=cursor&bpstart=1200616813&bpend=1200679929&m1tid=103010&m1bl=0&m1avg=0&m2tid=103075&m2bl=0&m2avg=0&m3tid=103125&m3bl=0&m3avg=0

Perhaps we can add your test to that suite?