20% Twinopen regression on Vista

RESOLVED INCOMPLETE

Status

()

Firefox
General
P2
normal
RESOLVED INCOMPLETE
10 years ago
6 years ago

People

(Reporter: jimm, Unassigned)

Tracking

({perf, regression})

Trunk
Firefox 3
x86
Windows Vista
perf, regression
Points:
---
Bug Flags:
blocking-firefox3 -
wanted1.9.0.x +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 1 obsolete attachment)

(Reporter)

Description

10 years ago
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

10 years ago
Created attachment 311273 [details]
graph

Updated

10 years ago
Flags: blocking-firefox3?
(Reporter)

Comment 2

10 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
Apparently this was caused by bug 422420.

Yeah, that's right. Bug 422420. Nobody knows why.
Assignee: nobody → mconnor
Flags: blocking-firefox3? → blocking-firefox3+
Priority: -- → P2
Target Milestone: --- → Firefox 3
(Reporter)

Comment 4

10 years ago
Created attachment 312798 [details]
simple twinopen-xul.dll.html profile results

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.
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

10 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

10 years ago
Created attachment 315585 [details]
Compare Runs (R1:without 422420, R2:with 422420)

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

10 years ago
Created attachment 315602 [details]
Compare Runs (R1:without 422420, R2:with 422420) 

2nd set of runs, same settings.
(Reporter)

Comment 9

10 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.
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

10 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

10 years ago
Created attachment 315775 [details]
100 windows R1:with R2: without
(Reporter)

Comment 13

10 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

10 years ago
Created attachment 315777 [details]
100 windows R1:with R2: without

accidentally snipped out some of the "good stuff"
Attachment #315775 - Attachment is obsolete: true
(Reporter)

Comment 15

10 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

10 years ago
This might be the cause - 

http://lxr.mozilla.org/mozilla/source/browser/base/content/browser.js#4663
(Reporter)

Comment 17

10 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

10 years ago
nope, most of that was removed. still looking...
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

9 years ago
Assignee: jmathies → nobody

Comment 20

6 years ago
Jim / Mike: Last update in 2008 - can this be closed as obsolete?
(Reporter)

Comment 21

6 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
Last Resolved: 6 years ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.