Closed Bug 133428 Opened 22 years ago Closed 22 years ago

Page layout just went up....

Categories

(Core :: Layout, defect)

x86
Windows 2000
defect
Not set
blocker

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.
this will keep the tree closed.
Keywords: perf, smoketest
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. 
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. 
No, password manager shouldn't be involved at all.  If it is responsible, I 
can't for the life of me see how.
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.
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.
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.

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
I just backed out the document viewer checkin to see if it effects page load.
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
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.
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.
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.
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().
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. 
Success.  Btek finished and the numbers improved.  Closing this out as fixed.
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
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.