Shorten Image DDB's life

RESOLVED FIXED in mozilla1.8beta3

Status

--
enhancement
RESOLVED FIXED
14 years ago
10 years ago

People

(Reporter: paper, Assigned: paper)

Tracking

Trunk
mozilla1.8beta3
x86
Windows 2000
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

14 years ago
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).
(Assignee)

Comment 1

14 years ago
Created attachment 181941 [details] [diff] [review]
Shorten a DDB's life

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)
(Assignee)

Comment 2

14 years ago
Target Milestone -> 1.8b3.  No need to rush to get it into b2.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.8beta3

Comment 3

14 years ago
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-
(Assignee)

Comment 4

14 years ago
(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 5

14 years ago
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+
(Assignee)

Comment 6

14 years ago
Created attachment 182364 [details] [diff] [review]
Shorten a DDB's life v2

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+

Updated

14 years ago
Attachment #182364 - Flags: superreview?(tor) → superreview+
(Assignee)

Comment 7

14 years ago
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+
(Assignee)

Comment 9

14 years ago
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
Last Resolved: 14 years ago
Resolution: --- → FIXED

Comment 10

14 years ago
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.
(Assignee)

Comment 11

14 years ago
> 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.
(Assignee)

Comment 12

14 years ago
Sorry, I meant to say, _InitWithFuncCallback_ doesn't addref

Comment 13

14 years ago
> You might be thinking of InitWithCallback, which does addref.

Yeah, my mistake.  Sorry for the noise :-/
Blocks: 293586

Comment 14

14 years ago
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. 

Updated

14 years ago
Depends on: 293665

Comment 15

14 years ago
I filed Bug #293665 to track the regression. Would be great if you could jump on
that Arron. Thanks!
No longer depends on: 293665

Comment 16

14 years ago
(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.