Closed
Bug 424675
Opened 16 years ago
Closed 12 years ago
20% Twinopen regression on Vista
Categories
(Firefox :: General, defect, P2)
Tracking
()
RESOLVED
INCOMPLETE
Firefox 3
People
(Reporter: jimm, Unassigned)
Details
(Keywords: perf, regression)
Attachments
(5 files, 1 obsolete file)
This might have been files someplace else, but all I found in bugzilla was bug 421072 from the 4th. The date of this was around the 17 -> 18th of March. Range: http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2008-03-17&maxdate=2008-03-18&cvsroot=%2Fcvsroot graphs screenshot forthcoming
Reporter | ||
Comment 1•16 years ago
|
||
Updated•16 years ago
|
Flags: blocking-firefox3?
Reporter | ||
Comment 2•16 years ago
|
||
tinderbox statsu: http://tinderbox.mozilla.org/showbuilds.cgi?tree=Firefox&maxdate=1205814053&legend=0&norules=1 most likely culprits according to tinderbox status: http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1205782800&maxdate=1205788979
Comment 3•16 years ago
|
||
Apparently this was caused by bug 422420. Yeah, that's right. Bug 422420. Nobody knows why.
Assignee: nobody → mconnor
Updated•16 years ago
|
Flags: blocking-firefox3? → blocking-firefox3+
Priority: -- → P2
Target Milestone: --- → Firefox 3
Reporter | ||
Comment 4•16 years ago
|
||
I'm going to try to use aqtime to track this down. While working on it I'll be posting any interesting stuff I find. Here's a simple run on xul.dll. GetShellFolderPath (ranks 7th) is interesting.
Comment 5•16 years ago
|
||
Jim, can you see if you can dig into this any better? I've hit a bit of a wall.
Assignee: mconnor → jmathies
Reporter | ||
Comment 6•16 years ago
|
||
Yep, plan to, I was right in the middle of this but got sidetracked on some other stuff that came up.
Reporter | ||
Comment 7•16 years ago
|
||
A comparison of the two. Paint / font related calls seem off quite a bit. This is run of twinopen (10 windows) and it's running under aqtime so the total times for methods / functions are going to be exaggerated.
Reporter | ||
Comment 8•16 years ago
|
||
2nd set of runs, same settings.
Reporter | ||
Comment 9•16 years ago
|
||
I did a bunch of profiling runs over the day looking for numbers that point to the patch for Bug 422420. In the end I've come to conclusion that patch may not be it.
Comment 10•16 years ago
|
||
Jim, backing out the patch undid the regression on the vista tinderboxes, so we're confident this was the cause. Are you seeing the difference outside of the profilere (just the raw time)?
Reporter | ||
Comment 11•16 years ago
|
||
I don't see a difference in time equal to that of tinderbox. I do see a much smaller difference (about ~15 msec) which is also in my profiling results which looks to be related to window event handling and painting. Honestly though with numbers this small it's often hard to reproduce them reliably. Attached is a profiling run with twinopen doing 100 windows with and without. The total accumulated differences are only 1-2 seconds in some paint routines, which is really small for a run of 100 windows. If we're sure this is the culprit I'll look for ways to minimize the small differences here by fiddling with the patch. That would hopefully fix the bigger problem on tinderbox.
Reporter | ||
Comment 12•16 years ago
|
||
Reporter | ||
Comment 13•16 years ago
|
||
note that's just a small snip of the data but it's where I see the small changes I'm referring to. Most everything else matches up, or the numbers are too small to tell.
Reporter | ||
Comment 14•16 years ago
|
||
accidentally snipped out some of the "good stuff"
Attachment #315775 -
Attachment is obsolete: true
Reporter | ||
Comment 15•16 years ago
|
||
I think we might be looking at this from the wrong angle. I don't think this is a performance hit we're taking by adding a button to the nav-bar, it's a performance gain we get when we add a home button to the PersonalToolbar. For example, without the patch applied, rotating through these test cases - PersonalToolbar defaultset has home-button - lower twinopen numbers PersonalToolbar defaultset has home-button / nav-bar defaultset has home-button - lower twinopen numbers PersonalToolbar defaultset does not have home-button / nav-bar defaultset has home-button - higher twinopen numbers PersonalToolbar defaultset does not have home-button / nav-bar defaultset does not have home-button - higher twinopen numbers PersonalToolbar defaultset has home-button / nav-bar defaultset does not have home-button - lower twinopen numbers
Reporter | ||
Comment 16•16 years ago
|
||
This might be the cause - http://lxr.mozilla.org/mozilla/source/browser/base/content/browser.js#4663
Reporter | ||
Comment 17•16 years ago
|
||
Actually there's a bunch of code from the original patch nsBrowserGlue I think can now be removed. Some of this looks like the likely culprit. https://bugzilla.mozilla.org/attachment.cgi?id=303388&action=diff
Reporter | ||
Comment 18•16 years ago
|
||
nope, most of that was removed. still looking...
Comment 19•16 years ago
|
||
We know what caused this, but the same regression didn't happen on XP, and its really unclear why this happened. The current tooling hasn't yielded anything of value, and its time to ship, so we're going to keep an eye on this, but not continue to block.
Flags: wanted1.9.0.x+
Flags: blocking-firefox3-
Flags: blocking-firefox3+
Reporter | ||
Updated•15 years ago
|
Assignee: jmathies → nobody
Comment 20•12 years ago
|
||
Jim / Mike: Last update in 2008 - can this be closed as obsolete?
Reporter | ||
Comment 21•12 years ago
|
||
(In reply to Andre Klapper from comment #20) > Jim / Mike: Last update in 2008 - can this be closed as obsolete? Yes, this is ancient.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → INCOMPLETE
You need to log in
before you can comment on or make changes to this bug.
Description
•