Closed
Bug 133428
Opened 22 years ago
Closed 22 years ago
Page layout just went up....
Categories
(Core :: Layout, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: mscott, Assigned: morse)
Details
(Keywords: perf, smoketest)
During the recent bout of tree redness, page load performance went up. Page loading jumped from around 1168-1179ms to 1187-1194 ms on btek. As attrition for breaking the tree, Steve gets to be the lucky winner to help find the source of this bug =). cc'ing the rest of the folks who checked in during this time.
Reporter | ||
Comment 1•22 years ago
|
||
this will keep the tree closed.
Reporter | ||
Comment 2•22 years ago
|
||
steve is the password manager at all invovled in the page loading process? Looking through the bonsai logs that seems to be the only significant change that went into the tree when the perf numbers changed.
Reporter | ||
Comment 3•22 years ago
|
||
Steve, it would be great if you could jump in here and start looking at your checkin. I'd hate to have to start backing it out per the new performance policy: http://mozilla.org/hacking/regression-policy.html besides it looks like a complicated back out.
Assignee | ||
Comment 4•22 years ago
|
||
No, password manager shouldn't be involved at all. If it is responsible, I can't for the life of me see how.
Comment 5•22 years ago
|
||
My two checkins shouldn't be responsible for this. One is a typo fix in pageInfo.js (not called on page load) and the other removes two obsolete mail news DLLs which existed in older (~0.9.4) builds, but not in current builds.
Comment 6•22 years ago
|
||
I'm getting a test build pulled on Linux (should be finished soon) and I will try to confirm and/or isolate the cause. I'm also suspicious of the change to nsDocumentViewer.cpp, which adds one walk of the entire frame tree once per document load.
Assignee | ||
Comment 7•22 years ago
|
||
Here are the significant points of my change. None of which could possible by involved in increasing page layout. - Parameters to password manager's remove-user and add-user routines changed from char* to string classes. These routines are not called during page layout. They are called by ftpConnectionThread and by nsHttpChannel, both cases involving authentication. - FindPasswordEntry was significantly rewritten. It is called only by composer and mailnews, never during page layout.
Reporter | ||
Comment 8•22 years ago
|
||
Hmm well the only possible changes are: 1) password manager 2) null ptr check from attinasi 3) akkana had some changes to the document viewer which look possible. 4) some mail stuff which definetly isn't loaded during the page layout tests 5) a mac change from simon
Reporter | ||
Comment 9•22 years ago
|
||
I just backed out the document viewer checkin to see if it effects page load.
Assignee | ||
Comment 10•22 years ago
|
||
Going solely by the names of the files checked in, here are some likely candidates for the increase. - sfraser content/event/src/nsEventStateManager.cpp - akkana content/base/src/nsDocumentViewer.cpp - attinasi layout/html/base/src/nsInlineFrame.cpp
Reporter | ||
Comment 11•22 years ago
|
||
As I mentioned before STeve, Simon's was Mac only Attinasi's was a null ptr check. Akkana's change to nsDocumentViewer, I just tried backing out to see what happens. Let me know if you see anything else that looks supsicious.
Comment 12•22 years ago
|
||
It could be the document viewer change that I checked in. The change is in Stop(), but it turns out that Stop() is called at least once, and sometimes twice, at the beginning of every document load, with internal variables set so that we think the page has just finished loading. So it calls the pres context to try to stop image animations. I wouldn't think that would be time consuming, but perhaps it is (and it's not useful since nothing is loaded at that point). If that turns out to be it, by all means back it out and I'll talk to the author and we'll take a different approach.
Comment 13•22 years ago
|
||
Actually, SetImageAnimationMode does turn out to be expensive -- it walks the content tree looking for images to stop. So if it's doing this on the last page loaded just before throwing it away and loading another page, that could definitely explain the performance regression.
Comment 14•22 years ago
|
||
Actually, independent of whether this is part of the slowdown in Ts, perhaps DocumentViewerImpl::Stop() should take an argument, so that when called by nsDocShell::Stop (i.e., user action stop), it will propagate the 'stop' to the images, but when called from nsDocShell::SetupNewViewer (to shut down current activity), it avoids calling SetImageAnimationMode().
Reporter | ||
Comment 15•22 years ago
|
||
Looks like the btek build which just finished didn't grab my checkin. We'll have to wait another round to see if that's it. I think Akkana and John are right and the document viewer change is probably the cause. Hopefully the next tinderbox run will back that up.
Assignee | ||
Comment 16•22 years ago
|
||
Success. Btek finished and the numbers improved. Closing this out as fixed.
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Comment 17•22 years ago
|
||
btek's times are down again, so my checkin was the culprit. Leave it backed out, and I'll reopen the other bug and consult with the author about what he wants to do about it. I like jrgm's idea of adding an argument to Stop(). Who owns the document viewer, so we could ask about whether that would be acceptable?
You need to log in
before you can comment on or make changes to this bug.
Description
•