Closed Bug 292051 Opened 19 years ago Closed 19 years ago

Shorten Image DDB's life

Categories

(Core Graveyard :: GFX: Win32, enhancement)

x86
Windows 2000
enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.8beta3

People

(Reporter: paper, Assigned: paper)

References

Details

Attachments

(1 file, 1 obsolete file)

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).
Attached patch Shorten a DDB's life (obsolete) — Splinter Review
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)
Attachment #181941 - Flags: review?(emaijala)
Target Milestone -> 1.8b3.  No need to rush to get it into b2.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.8beta3
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?
Attachment #181941 - Flags: review?(emaijala) → review-
(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.
Attachment #181941 - Flags: review- → review+
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.
Attachment #181941 - Attachment is obsolete: true
Attachment #182364 - Flags: superreview?(tor)
Attachment #182364 - Flags: review+
Attachment #182364 - Flags: superreview?(tor) → superreview+
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)
Attachment #182364 - Flags: approval1.8b2?
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.
Attachment #182364 - Flags: approval1.8b2? → approval1.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
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
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 :-/
Blocks: 293586
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. 
Depends on: 293665
I filed Bug #293665 to track the regression. Would be great if you could jump on
that Arron. Thanks!
No longer depends on: 293665
(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.
Depends on: 293665
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: