Closed Bug 114914 Opened 22 years ago Closed 18 years ago
addition of sidebar close button slowed startup (tbox 12/11/01)
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!!
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.
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 , 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.  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.
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.
Any possible way to get a speedup would be just great.
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.