1) Don't optimize image until needed (first draw) 2) Destroy DDB after a specified period of inactivity (60 seconds sounds good) This does wonders on Win9x. I'd also say that it will reduce pageload time on pages with images not in view, but that reduction probably won't be noticable. Related to Bug 284978, and makes that bug even harder to reproduce, but does not solve it completely. One can still reproduce Bug 284978 with this bug's patch by loading a page with a huge amount of big images, and scrolling _fast_ (holding down the PGDN).

In addition to shortening DDB's life, I cleaned up ConvertDDBtoDIB again.. we weren't deleting mImageBits on error, and re-setting mSizeImage was not needed (no code no longer compares mSizeImage to 0 to determine the state of mImageBits)

Target Milestone -> 1.8b3. No need to rush to get it into b2.

Comment on attachment 181941 [details] [diff] [review] Shorten a DDB's life I couldn't apply this patch to my trunk although it should be up to date. I wonder if it's a good idea to have a timer for every single optimized image. Depends on how many we'll usually end up with. Any test results?

(In reply to comment #3) > (From update of attachment 181941 [details] [diff] [review] [edit]) > I couldn't apply this patch to my trunk although it should be up to date. I updated my trunk and re-made the patch, but it was exactly the same as the one attached here. What's the error you get when applying? > I wonder if it's a good idea to have a timer for every single optimized image. > Depends on how many we'll usually end up with. Any test results? I talked to Stuart about timers, and he says a large amount of timers should not stress out Gecko. (The following is my interpretation) Timers get processed on another thread, and are all managed through TimerThread::Run(), so adding another one, only adds another object to the array. So, there's really only one timer, which is the shortest time of all the timers in the array. Having 2, 200 or 2000 timers will not have a drastic effect. Also note that this patch reduces the number of optimized images dramatically. Images are only optimized when they are first drawn. So, even a image heavy page like a Fark photoshop thread, will only have 7 images displayed initially. Scrolling will obviously increase the number, but when you are viewing a photoshop thread or an image gallery, you usually take a few seconds to look at each image. Even if you look at 4 images a second, you are only going to peak at 240 timers. For the power users who open up every image gallery link in a new tab, the images on those tabs won't get timers until the user views that tab anyway (and again, only for the visible images) One concern that was brought up was if 200 timers fired off all at once. Each one of those timers would be put in the queue, in front of all the Gecko events. So, those 200 images would get unoptimized all in a row, possibly causing a stall if the images are large. However, the likelyhood of 200 large images drawing all at once is slim, considering it's very hard to fit 200 large images on the screen at once. Javascript might be able to do it, but you are going to have other issues more pressing than timers if a Javascript is showing over 200 images in less than a second. 200 small images could definitely fit on one screen and get drawn at once, but the time to convert those small optimized images back to imagebits is lightning fast. Ask yourself, how many images do you think an average person views in 60 seconds? That's how many timers there will be. :) If it's a memory concern, IMO, having the size of timer object in each optimized image is a fair trade-off for not being a GDI resource hog.

Comment on attachment 181941 [details] [diff] [review] Shorten a DDB's life Weird, nothing changed and now it applied correctly. I did some testing and found that just surfing around quickly kept the number of timers around 200, which, I think, is perfectly acceptable. One question: does SetDelay reinitialize the timer? When it's a one-shot timer, does calling SetDelay in the callback make it fire again later? If so, good. Could you change this comment: + // Save resource, optimize later to "Save resources, optimize later". Maybe it's just me, but I stumbled upon it wondering what particulat resource we're saving and where :) r=me if SetDelay reinits the timer.

1) Modified comment 2) Good catch. SetTimer does reinit the timer, except when being called during a callback. Fixed it by creating a new timer. Hoping change #2 doesn't effec the review, I've transfered the r+, and sr?'d tor.

Comment on attachment 182364 [details] [diff] [review] Shorten a DDB's life v2 Requesting 1.8b2 approval. The earlier we get this in, the more testing it will get. This bug has a drastic effect on cutting down the instances of Bug 284978, and partially blocks the patch I have in that bug. If you think it's too late in the b2 cycle, I'll request a b3 approval (or you can save a step and mark it + or - for b3 as well)

Comment on attachment 182364 [details] [diff] [review] Shorten a DDB's life v2 Agreed on the value of more testing. a=shaver+brendan for 1.8b2.

Checking in gfx/src/windows/nsImageWin.cpp; /cvsroot/mozilla/gfx/src/windows/nsImageWin.cpp,v <-- nsImageWin.cpp new revision: 3.148; previous revision: 3.147 done Checking in gfx/src/windows/nsImageWin.h; /cvsroot/mozilla/gfx/src/windows/nsImageWin.h,v <-- nsImageWin.h new revision: 3.65; previous revision: 3.64 done

warning: drive-by review... > nsImageWin :: ~nsImageWin() > { >+ if (mTimer) >+ mTimer->Cancel(); >+ Given that the timer owns a reference to the nsImageWin, how can this code ever make sense? The dtor for nsImageWin could only run if all references to it have been cleared. This leads me to ask, are you sure you aren't leaking memory? Timers are the source of a lot of memory leaks due to the fact that the timer code does not break cycles like this for you.

> Given that the timer owns a reference to the nsImageWin, how can > this code ever make sense? The dtor for nsImageWin could only > run if all references to it have been cleared. This leads me to > ask, are you sure you aren't leaking memory? Timers are the > source of a lot of memory leaks due to the fact that the timer > code does not break cycles like this for you. InitWithCallback doesn't addref, so the dtor is called (I tested with a |if (mTimer) printf("!!!Destroying mTimer in dtor\n");| in dtor just to be sure). You might be thinking of InitWithCallback, which does addref.

Sorry, I meant to say, _InitWithFuncCallback_ doesn't addref

> You might be thinking of InitWithCallback, which does addref. Yeah, my mistake. Sorry for the noise :-/

this patch broke icons which are disabled in Thunderbird's toolbar. They no longer get drawn correctly when they are disabled. Backing out this pathc fixes the problem.

I filed Bug #293665 to track the regression. Would be great if you could jump on that Arron. Thanks!

(In reply to comment #14) > this patch broke icons which are disabled in Thunderbird's toolbar. > > They no longer get drawn correctly when they are disabled. > > Backing out this pathc fixes the problem. See Bug 293586 (Navigation icons change their shade of grey after time); ArronM is on the case and developing a fix.