Closed Bug 114914 Opened 22 years ago Closed 18 years ago

addition of sidebar close button slowed startup (tbox 12/11/01)

Categories

(SeaMonkey :: General, defect, P1)

Product:
SeaMonkey
Component:
General
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED WORKSFORME
Milestone:
Future

People

(Reporter: cathleennscp, Assigned: samir_bugzilla)

References

Details

(Keywords: perf) 

tinderbox browser startup number went from average of 6.24 to average of 6.34.

there is a cycle time period, on 12/11 18:04pm to 19:35pm showed the biggest
jump from 6.27 to 6.31

Here is the checkin log 
http://tinderbox.mozilla.org/bonsai/cvsquery.cgi?module=MozillaTinderboxAll&date=explicit&mindate=1008119640&maxdate=1008122639

Most fishy check-in is sgehani's fix for bug 33420, adding a close button to
sidebar, so I'm assigning this bug to sgehani.

cc'ing everyone else who made checkins during that period.

Please reply and confirm that you did/didn't contributed to the jump.

We need to address this soon.

Thanks!!
Blocks: 7251
See bug 114797.  This is likely related to that reporting change as well.
yes, and new window time slowed down too.
Txul number  (for window open) went from 1639 to 1708ms  ( 4% slow down )
Summary: browser startup number went up on tinderbox → browser startup & window open went up on tinderbox 12/11/01
actually, txul (window open) increase was also due to a tweak in txul test, so
it is very possible to be invalid.

however, startup increase is a real increase.
Summary: browser startup & window open went up on tinderbox 12/11/01 → browser startup went up on tinderbox 12/11/01
Txul: yes, law checked in a change to report median values, hence the jump.
Here's the startup data for sleestack for this window.
Times are report times, in ms.

2001:12:11:14:29:14     6245
2001:12:11:15:20:03     6247
2001:12:11:16:15:13     6250
2001:12:11:17:09:59     6261  <--- checkin window start
2001:12:11:17:59:14     6266  <--- checkin window end
2001:12:11:19:39:47     6312  <--- increase shows up here here
2001:12:11:20:12:58     6311
2001:12:11:21:06:37     6301
2001:12:11:22:03:38     6312
2001:12:11:23:50:45     6328


Is there a way people can try backing out their changes on sleestack
without having to go thru the review/super-review/drivers-approval
process?

Cool, so I'm no longer on the hook.  For the checkin window mcafee sites this 
startup time increase, there are 3 folks who checked in:

(a) blizzard: gtk2 "Not part of the build" changes
(b)  sgehani: one line correcting an API usage from 3 params to 2 params in js
(c)   hewitt: some menu gif changes

Blizzard's code is irrelevant and I'm certain it can't be my change (go see: 
<http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/xpfe/components/prefwindow/resources/content&command=DIFF_FRAMESET&file=pref-themes.js&rev1=1.25&rev2=1.26&root=/cvsroot>
if you want affirmation) which leaves hewitt (sorry Joe).

Here's the revised bonsai link for the window sited by mcafee:
<http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=MozillaTinderboxAll&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=regexp&sortby=Date&hours=2&date=explicit&mindate=12%2F11%2F2001+17%3A59%3A14&maxdate=12%2F11%2F2001+19%3A39%3A47&cvsroot=%2Fcvsroot>
Assignee: sgehani → hewitt
I increased the size of the menu checkbox/radio images to 16x16.  These images
aren't even loaded until you open a menu, so this would have zero affect on startup.
isn't the time is 17:09 to 17:59 (during which time 10 people checked in)?

sgehani, neeti, naving, jdunn, peterlubczynski, mscott, naving, kmcclusk, 
cmanske, bstell

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=MozillaTinderboxAll
&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortb
y=Date&hours=2&date=explicit&mindate=12%2F11%2F2001+17%3A09&maxdate=12%2F11%2F20
01+17%3A59&cvsroot=%2Fcvsroot



I misunderstood mcafee's comment.  Please ignore my noise in comment 8.  Thanks
for straightening that out for me, bstell.
Assignee: hewitt → sgehani
I can suspend and hack on any unix tinderbox, so can other people.
Builds are usually located in /builds/tinderbox/SeaMonkey,
start/stop using this:
  http://www.mozilla.org/build/sheriff.html#kick
Ask a build person (me, leaf, etc.) for help with passwords, etc.
My change was related to anti-alias scaled bitmaps (aasb).
I tried disabling aasb on sleestack and the Txul time did not change.

samir, do you have data to confirm yet?
adding keywords and nominating for a quick fix. We can't let these slide, we're
being nibbled to death by minnows on performance so anything with a measurable
hit has to be quashed.
Keywords: mozilla0.9.8, perf
Priority: -- → P1
Target Milestone: --- → mozilla0.9.8
I am seeing no difference with or without the sidebar close button.  The
statistical mode with the close button on my perf, timeline-enabled, optimized
Win2K build is identical to the startup time without the close button
(toolbarbutton XUL eliminated and images removed from the jar).  There are no
wide deviations from the mode either.  
Worked with McAfee to try backing out my changes on one of the testerboxes. 
Indeed we did see an increase in time but not as much as initially found.  

Startup time went up from 7050ms to 7075ms: 0.00354%
Winopen time went up from 2035ms to 2045ms: 0.00491%

The change includes: addition of 3 images for the various close button states,
one toolbarbutton widget inclusion in XUL, and 12 lines in 3 CSS rules.  All in
all it's a pretty simple change.  If we want to reduce the startup and window
open time it looks like we could either:

(a) delay load the close button (dynamically add the node)
(b) get rid of the feature
If we think that the increase in time is not too little to hunt down a fix, I'll
hack in delay loading the button and report results.  Note that delay loading
may actually increase window open + page load (i.e., the test named xulwinopen)
based on what we have learned from sidebar performance testing.  In fact, user
perception may degrade as well.
We aren't getting rid of this feature because startup takes 1/40 second longer,
and window open takes 1/100 second longer.  It will save most user 10X that
every time they use it. I'm not sure it is even worth trying to improve such a
tiny delta, surely we can find bigger gains that make more sense to attack first.
This is the first feature that we've really measured at
checkin-time.  All features are going to have some time
and space cost, and we have to evaluate the trade-off
probably on a case-by-case basis.  How many feature units
is 25ms worth?  It depends on the feature, and how big
your pool of "25ms finds" is.
I agree with peter.

Suggestion: It would be great if the perf impact of features is tested *before*
checkin and go-nogo decision taken ahead of time. Plus if we are alerted of any
compromise taken, it would save us time in not chasing after those.
Good suggestion.  Another: Perhaps we could adopt the 'pollution credits' model,
requiring those who checkin small/acceptable degradations like this to counter
with an improvement that more than balances the scales?  If so, then Samir would
owe us a modest improvment.
I'm a bit leary of the pollution credit model because we aren't at
a stage yet where start up is anywhere near competive...  This
sets us up for still another release where we are flat line on
start up time when measured against previous releases.

we made some progress on starup in past releases and they
were eaten away by the minnows that dveditz mentioned
in the form of lots of extra images and small features.

If we did go to the model we need to kick this feature out until
the improvement is found, because we all know the ability to 
find startup improvements is a tough road.

When I read http://bugzilla.mozilla.org/show_bug.cgi?id=33420
I don't see the significant and clear cut improvement that
warrents _any_ degredation.  There is a lot of controversy 
in that bug about benefit and implementation.
There is a lot of controversy in almost any bug that changes the UI, because
everyone involved has a personal preference.  However the sum total of their
opinions adds up to an insignificant portion of our total user base.  This is
one of the most important usability improvements we have for MachV, well worth
the imperceptible difference in startup and window open times.  This one is a
big net win, one that can't be appreciated by measuring startup time alone.
i think that close button for sidebar is useful, but if the end game is to let
users to easily close/disable the sidebar, maybe we should also consider making
default pref not showing sidebar??  that would definitely put back the time we
lost. :-)
Um, has anyone tried using a gif for the close button instead of a png?  That's
the only thing I can think of, except for awful style rules, which doesn't seem
to be the case here.
Since my patch landed, hewitt replaced the 3 images with a single gif and
simpler style rules (reused the toolbarbutton-image binding).  The gif is used
by both modern and classic I believe.  I haven't done any measurements but I
suspect the 0.00354% increase to startup time and the 0.00491% increase to
window open time has reduced even further.  

(And for the record the pngs were only in the classic skin as an
engineering-created stopgap measure while we were waiting for icons from UE. 
The modern skin always had official UE-supplied gifs.)
btw, increases are 0.35% (0.00354) on startup and 0.49% (0.00491) on new window :-) 

since hewitt simplified the skin, maybe we can double check the impact of this
fix; it's possible that it has decreased too.

chofmann, I agree with your principle of no degradation, but we should allow
ourselves to apply judgement.  I doubt that there's an alternative
implementation of the feature that can meet a zero tolerance objective since
displaying new images is involved.  It's reasonable to expect that this small
increase should either be accepted on a cost/benefit basis or balanced by a
decrease in the infrastructure it depends upon.

The precedent I'd like to see this establish is that we *examine* every such
checkin and *knowingly* decide whether or not to accept it.  If we allow
degradations too often, subsequent checkins will have a tougher time getting
accepted.  (And the possibility of backing some of them out remains as a backup
strategy.)
can we double check the impact now that hewitt has completed skin simplification?
With hewitt's change [1], startup time is affected very slightly less and window
open time appears to have gone down!

Startup time increase *before* hewitt's change: 0.35%
Startup time increase *after* hewitt's change:  0.26%

Window open time increase *before* hewitt's change: 0.49%
Window open time increase *after* hewitt's change: -8.83% i.e., winopen improved!

One may be disinclined to trust the window open numbers but these reports are
from the sensitive MozillaTest tree.  Will chat with mcafee to see why the
window open numbers went up.  I seem to recall he mentioned such behavior was
observed with other features as well.

[1] Reduce number of images from 3 -> 1 and reduce complexity of close button image
->future. We should still keep on the outlook for ways to improve this, but for
moz1.0/MachV it is an overall vast improvement, and not worth spending
significant time on  when there are much worse problems, and limited time and
resources.
Target Milestone: mozilla0.9.8 → Future
If we're keeping this around for the future then we need a better summary. Feel
free to refine if I've gotten it wrong.
Summary: browser startup went up on tinderbox 12/11/01 → addition of sidebar close button slowed startup (tbox 12/11/01)
I did a lot of work to get numbers on this, we should
make the call now or at least summarize what was found.
If you don't want to fix this, then mark it won't fix.
The time to fix perf bugs like this is "soon", futuring it
is basically a won't fix.

I don't think that's true, we do want to reduce the cost to zero, but it doesn't
make the cut for this release. This isn't a perf bug, it just notes the cost of
adding this feature, which is acceptable for MachV.  There are possibly
long-term ways to address this, and I'd like to keep it open in the meantime so
we don't lose the issue.
Keywords: mozilla0.9.8
Any possible way to get a speedup would be just great.
Product: Browser → Seamonkey
long-since WFM or fixed-by-hewitt's change (comment 27)
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.