Closed Bug 130524 Opened 23 years ago Closed 23 years ago

Txul times up 3%

Categories

(SeaMonkey :: General, defect)

Product:
SeaMonkey
Component:
General
Platform:
x86
Linux
Type:
defect
Priority:
Not set
Severity:
blocker

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: mcafee, Assigned: dougt)

Details

(Keywords: perf, regression)

Attachments

(1 file, 1 obsolete file)

same as above but unified and tab'ed correctly
3.34 KB, patch
darin.moz
: superreview+
asa
: approval+
Details | Diff | Splinter Review
facedown, sleestack, comet, and mecca all show increases of around 3% in the Txul times. The data is a little choppy, but it looks like it happened sometime after lunch.
Keywords: perf, regression
3 jumps in a roll to window open yesterday.
Severity: normal → critical
upping the severity to blocker.
Severity: normal → blocker
3rd jump, (largest of all 3 jumps) seems to be dougt's check in. --> dougt Need help on figuring out the other jumps.
Assignee: cathleen → dougt
adding smoketest keyword so that it's on the blockers list radar. is this regression on win32 only ?
Keywords: smoketest
automated performance tests only run on linux tinderbox right now.
this shouldn't have anything to do w/ dougt's checkin... if anything, his checkin should have improved performance (albeit very very little).
The only thing in my checkin that might cause us to slow down is the change in ua.css (only changed parts shown): -parsererror { +@namespace parsererror url(http://www.mozilla.org/newlayout/xml/parsererror.xml); + +parsererror|parsererror { -sourcetext { +parsererror|sourcetext { I *thought* this could only improve page load time because the selector became more specific with its own namespace. I don't know how to test Txul to see if this made any difference. If someone could point me to instructions I could try. Looking at bryner's change I would give it a bigger probability for causing the regression. It includes more changes in CSS files, and we are doing more work, like in nsOutlinerContentView::ContentStatesChanged(). Dunno how much of the new code is used during Txul test, though.
i'd be very surpriced if my checkin caused any regressions since it only touches code that isn't run during the automated tests. AFAIK it's not even loaded
bryner is looking on it now
My checkin might have impacted this slightly, but I'm not sure there's much that can be done about it. We need this pseudoclass support in order to implement XBL form controls without altering the HTML DOM. Talked w/ hyatt and we weren't able to come up with any ideas for speeding this up.
More importantly, this was always something that was being done wrong by XUL menus, so this was a good fix.
>My checkin might have impacted this slightly how slightly? could it be up to 3%,well it's probably difficult to say :( I'm going to downgrade this blocker, and open the tree. Any objections?
Has anyone diffed jprof or quantify profiles, or done A/B testing of any other kind, to narrow down what the cause is? I don't think we should open the tree while we have a 3% Txul loss just floating around. That could take weeks of work to make up for.
Comment #8 is in error. Looking at the following runs, the Txul numbers taper down to ~1% of my precheck.
http://mozilla.org/performance/tinderbox-tests.html Txul docs, how about them apples.
Hmm. Of my two checkins in that range, one just change |.hidden = foo| to .setAttribute("hidden", foo). The other was in code that is _mostly_ not called at startup. The exception to all that was a change to computed -moz-binding which could have caused a slew of extra calls into the XBL binding manager. This was backed out at 03/12/2002 18:33: http://bonsai.mozilla.org/cvsquery.cgi?module=MozillaTinderboxAll&date=explicit&mindate=1015986000&maxdate=1015987000
OK. Running 3 tests each (thank you, mcafee) on "before" (a pull from before all these checkins), "during" (between my first checkin for bug 128428 and the backout I mention in comment 19, I get: Before: openingTimes=783,768,763,712,748,717,695,713,712 avgOpenTime:735 minOpenTime:695 maxOpenTime:783 medOpenTime:717 __xulWinOpenTime:717 ================== openingTimes=717,687,692,719,707,714,706,695,719 avgOpenTime:706 minOpenTime:687 maxOpenTime:719 medOpenTime:707 __xulWinOpenTime:707 ================== openingTimes=758,707,777,717,717,729,722,732,701 avgOpenTime:729 minOpenTime:701 maxOpenTime:777 medOpenTime:722 __xulWinOpenTime:722 During: openingTimes=819,706,777,753,763,759,707,706,737 avgOpenTime:747 minOpenTime:706 maxOpenTime:819 medOpenTime:753 __xulWinOpenTime:753 ================== openingTimes=757,717,765,747,706,717,722,742,721 avgOpenTime:733 minOpenTime:706 maxOpenTime:765 medOpenTime:722 __xulWinOpenTime:722 ================== openingTimes=753,701,793,722,758,706,707,721,732 avgOpenTime:733 minOpenTime:701 maxOpenTime:793 medOpenTime:722 __xulWinOpenTime:722 After: openingTimes=746,704,735,697,694,720,742,739,704 avgOpenTime:720 minOpenTime:694 maxOpenTime:746 medOpenTime:720 __xulWinOpenTime:720 ================== openingTimes=721,718,752,757,697,792,721,708,706 avgOpenTime:730 minOpenTime:697 maxOpenTime:792 medOpenTime:721 __xulWinOpenTime:721 ================== openingTimes=734,702,767,737,712,712,757,717,706 avgOpenTime:727 minOpenTime:702 maxOpenTime:767 medOpenTime:717 __xulWinOpenTime:717 I'm not quite sure what to make of the "707" and "753" times, but the rest seem to be pretty much all the same...
Ah, shoot. I forgot the and "after" -- with the backout patch from comment 19 applied.
I don't think my checkin would account for a 3% increase, but we're testing to find out.
I run Txul test on Windows (had to write the numbers down by hand as the whole app closed after the 10th window). Below 'old1' & 'old2' columns are with old ua.css, 'new1' & 'new2' are with ua.css that includes my changes: old1 old2 new1 new2 1 1773 1873 1773 1702 2 891 951 901 901 3 811 851 801 811 4 801 841 791 801 5 751 791 741 751 6 761 791 751 761 7 801 841 791 801 8 761 791 751 761 9 801 1031 791 801 10 811 791 751 761 avg 896.2 955.2 884.2 885.1 small 751 791 741 751 So I don't think my changes slowed us down (if anything, it looks like it was a small improvement). My changes beyond ua.css kick in only when we encounter an XML parser error. I even tested in the debugger and we don't hit the error cases during Txul test.
Whoah! I copied the table from Excel and it looked like a table before I submitted.
heikkiL thanks for testing. bryner says backing bz out buys us back some time, more to report in a bit.
bz backout looks flat on facedown, so I think bz is clear. Backing out dougt gets most of the Txul time back, but also costs us the startup time win. still looking. bryner backout next.
dougt's check-in is a trade off between speeding up startup and slowing down window open. txul 1747 -> 1802 +55 ms or 3.14% slow down ts 6620 -> 6441 -179ms or 2.70% speed up we've decided that window open is more important than startup, so he'll have a fix coming up soon.
proven bryner's change also caused slow down in txul, spin off this part to bug 130778
no fair! forking this bug to solve part of this regression will leave me in a world where I am trying to make up more than what I lost. Please note, that my fix will not improve total Txul by 3% if bryner checks in his stuff too.
I have a fix which will get us back to my base line.
Attached patch patch v.1 (obsolete) — — Splinter Review
Brings Txul down time to my precheckin levels.
Comment on attachment 74209 [details] [diff] [review] patch v.1 r=dp
Attachment #74209 - Flags: review+
Comment on attachment 74209 [details] [diff] [review] patch v.1 Weird -- how about a diff -u and what's up with the lack of indentation in this diff? Must be a diff glitch. /be
no diff glitch - user glich. new patch soon.
Attachment #74209 - Attachment is obsolete: true
Comment on attachment 74221 [details] [diff] [review] same as above but unified and tab'ed correctly sr=darin
Attachment #74221 - Flags: superreview+
Comment on attachment 74221 [details] [diff] [review] same as above but unified and tab'ed correctly a=asa (on behalf of drivers) for checkin to the 1.0 trunk
Attachment #74221 - Flags: approval+
antitux knows about this, off of the smoketest radar.
Keywords: smoketest
Checking in base/src/nsInputStreamChannel.cpp; /cvsroot/mozilla/netwerk/base/src/nsInputStreamChannel.cpp,v <-- nsInputStreamChannel.cpp new revision: 1.63; previous revision: 1.62 done Checking in protocol/jar/src/nsJARChannel.cpp; /cvsroot/mozilla/netwerk/protocol/jar/src/nsJARChannel.cpp,v <-- nsJARChannel.cpp new revision: 1.86; previous revision: 1.85 done Marking fixed. Of course, as i stated above, this does not drop us by 3% since bryner was responsible for some of this sum.
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: