1166985 - Use two image decoding threads on dual core devices

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Seth Fowler [:seth] [:s2h]]

Right now we use one image decoding thread for single core devices, and (numCores - 1) image decoding threads for devices with more cores. That means that dual core devices also use 1 image decoding thread, 3 cores results in 2 threads, and 4 cores results in 3 threads.

The result is that dual core devices have *half* the image decoding bandwidth of devices with three cores.

I don't think this tradeoff makes sense. On single core devices we oversubscribe (so we multiplex the main thread and the image decoding thread on the same core), and I think we should do this on dual core devices as well by giving dual core devices two image decoding threads. It's true that this won't necessarily double image decoding performance, because one of the two cores may be occupied by the main thread (or indeed, by one of the many other threads we have!) but I still expect the performance gain to be significant.

There are significant benefits to keeping a core free for things other than image decoding, as well, so I don't think we should do this for devices with 3 or more cores. Adding an additional decoding thread to a device which already has two yields a 1.5x gain in image decoding performance in the best case, and the case gets even weaker for devices with four or more cores. I think dual core is really a special case where the current heuristics fall down.

I should mention that in bug 1152147, I suggested that we give this extra thread a lower priority, but on reflection I do not think that's a good idea. We should just trust the OS scheduler. Giving one of the two threads a lower priority could unfairly penalize image decoding.

Comment 1

[Seth Fowler [:seth] [:s2h]]

Attachment: Use two image decoding threads on dual core devices

Here's the patch.

Comment 2

[Seth Fowler [:seth] [:s2h]]

Thanks for the quick review!

Comment 3

[Seth Fowler [:seth] [:s2h]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/edddd285c20c

Comment 4

[Dave Garrett]

If this causes no regressions, then I'd like to request considering it for uplifting to Aurora. It's simple and can have a noticeable impact with low risk.

Comment 5

[Seth Fowler [:seth] [:s2h]]

(In reply to Dave Garrett from comment #4)
> If this causes no regressions, then I'd like to request considering it for
> uplifting to Aurora. It's simple and can have a noticeable impact with low
> risk.

Yeah, I agree. Let's give it a week to make sure nobody reports any performance regressions, and if everything looks good I'll request uplift.

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/edddd285c20c

Comment 7

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8608404 [details] [diff] [review]
Use two image decoding threads on dual core devices

Approval Request Comment
[Feature/regressing bug #]: None. This is a pure performance improvement.
[User impact if declined]: Image decoding performance on dual core machines will be half of what it would be with this patch. That means that it will be much easier to scroll faster than we can decode images, resulting in lots of "pop-in".
[Describe test coverage new/current, TreeHerder]: On m-c for a month with no issues.
[Risks and why]: This is very low risk. It just changes the number of image decoding threads we create on dual core machines.
[String/UUID change made/needed]: None.

Comment 8

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8608404 [details] [diff] [review]
Use two image decoding threads on dual core devices

Approving for uplift to Aurora. This patch has been in nightly for several weeks now, it seems safe to land it in Aurora now.

Comment 9

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Setting the qe-verify flag to request QE team to test this fix on a dual-core and multi-core machine to ensure all is well.

Comment 10

[Bogdan Maris, Desktop Test Engineering]

(In reply to Ritu Kothari (:ritu) from comment #9)
> Setting the qe-verify flag to request QE team to test this fix on a
> dual-core and multi-core machine to ensure all is well.

I don`t see any differences between Firefox 40 which is affected and Firefox 41.0 RC build 3 on a machine with octa-core. Unfortunately we don`t have a machine with fewer cores in order to see any differences. I will go ahead and remove qe-verify but leave it fixed, maybe someone with a dual-core machine can check it out.