Image library causing unnecessary timers. Many unnecessary timers.

VERIFIED FIXED in mozilla0.9.1

Status

()

Core
ImageLib
P3
normal
VERIFIED FIXED
18 years ago
17 years ago

People

(Reporter: mkaply, Assigned: Stuart Parmenter)

Tracking

({perf})

Trunk
mozilla0.9.1
x86
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [rtm-])

Attachments

(1 attachment)

(Reporter)

Description

18 years ago
Check out :

http://lxr.mozilla.org/seamonkey/source/modules/libimg/gifcom/gif.cpp#1517

The problem is that 0 length delays are always being set to MINIMUM_DELAY_TIME 
(10). They should actually be going down the GETN(1, gif_image_start); path.

This is causing tons of unecessary timers to be created.

Here is the correct code:

                if (gs->delay_time){
                  if(gs->delay_time < MINIMUM_DELAY_TIME )
                      gs->delay_time = MINIMUM_DELAY_TIME;
                    if(ic->imgdcb){
                          gs->delay_timeout = (void *)
                                
ic->imgdcb->ImgDCBSetTimeout(gif_delay_time_callback, gs->ic, gs->delay_time);
                     }
                    /* Essentially, tell the decoder state machine to wait
                       forever.  The timeout callback routine will wake up the
                       state machine and force it to decode the next image. */
                    GETN(1L<<30, gif_image_start);
                    gs->state = gif_delay;
                } else {
                    GETN(1, gif_image_start);
                }


The if check for MINIMUM_DELAY_TIME should be in the second if.

Updated

18 years ago
Keywords: perf
(Reporter)

Comment 1

18 years ago
I'm putting an RTM+ in here because someone in Netscape land needs to 
look at this before shipping Netscape 6.

This is a genuine performance bug.
Whiteboard: RTM+

Comment 2

18 years ago
Created attachment 17032 [details] [diff] [review]
patch version of mkaply's change

Comment 3

18 years ago
Removing {rtm+] from status - that should only be added once you have r/sr.
Looks right, and hasn't caused problems for the couple days I've had it in
my tree.

pnunn, could you review this?
Keywords: rtm
Whiteboard: RTM+

Comment 4

18 years ago
At one time there was a Good Reason for the delay time
to always be set at the minimum value. But that was the old
code base. Time(rs) have changed. I'll need to pound on this
one alittle to verify its goodness.
-p
Status: NEW → ASSIGNED
mkaply mailed me enquiring about when this might get r=/sr= (I think he composed
his mail to me before pnunn's update of today).  I don't know, but I thought I'd
ask in the bug.

/be

Comment 6

18 years ago
How bad is the performance problem?  Can we go ahead with reviews?  (If this
doesn't happen ASAP, it's going to be minus by default because the RTM will have
shipped.)
Whiteboard: [need info]

Comment 7

18 years ago
PDT marking [rtm-]. Looks like this patch has been languishing.
Whiteboard: [need info] → [rtm-]

Updated

18 years ago
Keywords: approval, patch, review
Yes, the patch is languishing.  Pam, can we check it into the trunk so that 
everyone can pound on it?

/be

Comment 9

18 years ago
I've been testing the patch in my local tree and
it works fine (linux & NT). There was a particular
reason why a minimun delay time was _always_ set before
the timer code was changed. It appears a minimum of
zero is now ok.

This patch also appears to help fix or alleviate #50786,
where the clean up after end of stream wins (or loses ) a race with the
clean up after a bad image and crashes.

r:pnunn for trunk
who gives an sr: ?
I reviewed this earlier, and while reviewing bug 53597, and even whined about 
the non-four-space indentation at the deepest nesting level shown in the patch. 
Anyway, sr=brendan@mozilla.org.

/be

Comment 11

18 years ago
*** Bug 58071 has been marked as a duplicate of this bug. ***

Comment 12

18 years ago
Make that bug#58071 (not 50786).

(so thats what that loud, constant background noise is...)

Comment 13

18 years ago
My trunk build isn't up to date.
Anyone have a fresh tree want to check this in?
-p
(Reporter)

Comment 14

18 years ago
Since it was originally mine, I'll check it in.

Comment 15

18 years ago
much thanks.
(Reporter)

Comment 16

18 years ago
Fix checked in.

And I fixed a couple of the bad indentations that brendan was talking about.
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED

Comment 17

18 years ago
Coding issue, marking verified
Status: RESOLVED → VERIFIED

Comment 18

18 years ago
 
MK:
would you back out this patch for the minimum timer delay?
See bug#59494.
-pn

Status: VERIFIED → REOPENED
Resolution: FIXED → ---

Updated

18 years ago
Blocks: 61527
(Assignee)

Updated

18 years ago
Depends on: 70938

Updated

18 years ago
Target Milestone: --- → mozilla0.9.1

Comment 19

18 years ago
All pnunn bugs reassigned to Pav, who is taking over
the imglib.
Assignee: pnunn → pavlov
Status: REOPENED → NEW
(Assignee)

Comment 20

18 years ago
New imagelib doesn't use timers for anything except animations.
old code no longer used.  marking fixed.
Status: NEW → RESOLVED
Last Resolved: 18 years ago18 years ago
Resolution: --- → FIXED

Comment 21

17 years ago
Verified
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.