78300 - Make imglib process data on a separate thread

Status: RESOLVED DUPLICATE of bug 716140

(Core :: Graphics: ImageLib, enhancement)

Description

[Cathleen]

Comment 1

[Stuart Parmenter]

accepting.. posting patch

Comment 2

[Stuart Parmenter]

Attachment: make imglib work on a seperate thread

Comment 3

[Stuart Parmenter]

there are a few "issues" with this patch:
1. it sends async events way too often when it should be sending sync events.
2. we should probably just do an async proxy of GetInputStream on chrome:// and 
file:// urls and read the whole buffer at once.
3. http isn't going to be threadsafe for 0.9.1, so we can't call AsyncOpen on a 
thread other than the UI thread (at least not for http:// urls).  We need to 
call AsyncOpen on the UI thread and proxy onstart/onstop/ondataavailable calls 
from http on the UI thread to the imglib thread.
4. we really want the image container created on the UI thread... but I'd like 
to do this without the decoder needing to know anything about what thread it is 
running on.. this should be possible, since decoders are quite isolated, but 
will require a little bit of thought.

Comment 4

[Stuart Parmenter]

so what does it do you might ask?  it loads all the chrome images... http 
images will load after about 30 asserts.

Comment 5

[Stuart Parmenter]

On unix, this will require me to remove the use of gtk_timeout_* in the code and
fire plevents on the current thread's event queue.  This should be fairly
straight forward.

don't know about mac or windows yet... I expect I will have to do something
similar.

Comment 6

[Stuart Parmenter]

er, previous comments were meant for bug 78611

Comment 7

[Stuart Parmenter]

Attachment: newer patch

Comment 8

[Stuart Parmenter]

the last patch is missing some changes..  new one coming

Comment 9

[Stuart Parmenter]

Attachment: newest patch

Comment 10

[Stuart Parmenter]

moving to 0.9.2.

Comment 11

[Stuart Parmenter]

Attachment: latest patch

Comment 12

[Stuart Parmenter]

shipping off to mozilla 1.0.  this is kinda pointless to land until http is 
threadsafe.

Comment 13

[basic]

Stuart: so this depends on bug 59434 being fixed?

Comment 14

[Stuart Parmenter]

yes, very much depends on 59434

Comment 15

[Marek Z. Jeziorek]

batch: adding topembed per Gecko2 document
http://rocknroll.mcom.com/users/marek/publish/Gecko/Gecko2Tasks.html

Comment 16

[Jaime Rodriguez, Jr.]

Topembed- as this is not currently needed by a major embedding customer.

Comment 18

[Eric Butler [:ebutler]]

After testing the impact image decoding has on page load times, I believe that such a patch would not result in any significant gains.

I ran some performance tests to try to determine the possible speed-ups that could be gained from implementing thread decoding. I ran the Talos tp tests that Tinderbox uses on Windows XP on two release builds of the trunk, one normal and one with all the image decoders disabled. Of course this kind of test isn't very accurate, but it is close enough for a rough estimate.

The time difference between the two was, on each test, anywhere from 1 to 4 percent. I'm not sure that such a small gain justifies the complexity of a patch, which would probably have a comparable (or more likely smaller) time savings. Besides, the overhead that comes from threading would probably nullify most of the gains and perhaps make it slower than before.

I would suggest marking this WONTFIX.

Comment 19

[Benoit Jacob [:bjacob] (mostly away)]

No code activity here since 2002, last reply in 2008 (comment 18) suggests WONTFIX, no reply since.

  ==> actually marking WONTFIX now.

Comment 20

[Bobby Holley (:bholley)]

Actually, jeff is talking about doing this. CCing

Comment 21

[Bobby Holley (:bholley)]

Doing this over in bug 716140 now.