66263 - page loading times increased over the weekend

Status: VERIFIED FIXED

(Core :: Layout, defect, P1)

Description

[John Morrison]

So, the tests that are run weekdays showed a notable increase for Monday's 
tests on both Mac and Windows, for the time it takes to load a web page

(The test is http://jrgm.mcom.com/page-loader/loader.pl, and it simply 
ships out web pages that have been captured to that server, along with
a little bit of JS that is added to the page to call back to the server
when the onload event is fired for that page. The server then logs that
information and sends out the next page in the sequence. The results 
have been generally reliable to date, but lord knows, I'm a-fearing the
day when I make a bone-headed mistake.)

To confirm the results that I saw, I installed the builds for 19-9am, 
20-9am, 20-9pm, 21-9am and 22-6am on twalker's win98 machine (where the
tests are run weekdays), and using the same profile [but with a new 
cache created each time], I ran the same tests: 5 loads of each web 
page in a sequence of 40 pages (200 samples total per test).

These re-tests showed that there was a definite change in loading
times between the builds of Saturday, 20, 9am and Saturday 20, 9pm,
with an average increase of 21%. Looking through the checkin logs, the
only major checkin (with broad scope) was the checkin the split up
some of the style structs. So, I'll begin this with pierre, et al, 
(but maybe it's a one-line somewhere else, and is entirely unrelated
to this checkin.)

This is just to provide information on the impact since I realize that
this is a major restructuring, and there is a tradeoff between
performance and memory involved. Let me know if I can be of help in
further testing. I'll send you each a detailed chart of what the
deltas are, and I can provide more data as needed.

Comment 1

[Boris Zbarsky [:bzbarsky]]

fixing summary

Comment 2

[Håkan Waara]

Adding 'perf' keyword.

Comment 3

[Blake Ross]

If it is, indeed, the style changes, then this could also be causing the major 
slowdown in Mail that people have been reporting recently (66269).

Comment 4

[buster]

pierre is on vacation this week.  re-assigning to marc.  marc has a plan for
determining if the style code really is to blame.

we will treat this as top priority.  we won't let a major performance regression
slide.

Comment 5

[Marc Attinasi]

I have source before Pierre's checkins, so I'm running the Gecko performance
tests on these changes, then I'll run it against his changes so we can see where
the slowdown is (i.e. style, layout, parser, etc).

Comment 6

[Marc Attinasi]

OK - preliminary results show that Pierre's changes have cause a 50% slow down
in Style Resolution. I'm trying to verify these tentative results now, just to
be sure, but I think the test results will prove accurate.

I see three options: 1) I back the changes out - 63 files! 2) I try to figure
out why it slowed down - I figure it is worth some effort trying to find the
cause of the slowdown, and possibly fix it, since the tedium of backing out 63
files appeals to me as much has having dental work done. 3) leave the changes in
for Pierre to deal with when he gets back - I fear that this could be
disasterous due to overlapping changes, and the impact on those who are actively
trying to improve performance. Opinions?

Comment 7

[scottputterman]

I'd like to see mail become faster as soon as possible (we had performance
problems before this!)  I'm not against spending some time trying to figure it
out, but if that's not successful, I'd vote for doing #1.

Comment 8

[Chris McAfee]

Is there a bug re: pierre's checkin?  If we back this out,
what do we give up?

Comment 9

[Blake Ross]

Bug 43457.

Don't lose much.  It was just the first step of some memory footprint reduction

Comment 10

[Marc Attinasi]

What Blake said...

Comment 11

[Adam D. Moss]

Certainly I'd rather see the patch 'fixed' than
reverted -- style resolution has been quite a time-sink
and a memory hog at the best of times to date, and
balancing these two factors is obviously turning out
to be quite non-trivial!  I appreciate the brilliant work
put in by Marc, Pierre et al.

(Of course, on the side of the devil's advocate, however,
the weekend's slowdown was certainly very apparent and
painful, apparently having given rise to a bug [this one]
with a greater severity than the bug trying to be fixed --
a big neon sign saying 'revert me!')

Comment 12

[Jon Granrose]

back him out.  I've sent mail to attinasi, phil, and leaf with the bonsai URL
showing pierre's changes on 1/20 to be backed out which will give the CVS
commands to do the backout.  Assuming this is the action we're taking, I'll ask
leaf to do that today after smoketests and before opening the tree.

Comment 13

[Chris McAfee]

pierre has been backed out.  Please update & reevaluate.

Comment 14

[Axel Hecht]

This checkin has caused a regression in the XSLT code, which caused some worries
to both Peter and me over the last few days. The backing out made our life 
happy again.
To be specific, we couldn't style a xhtml body tag anymore.
If code like the one backed out should go back in, could you (pierre?) please
speak out load, so we can test in advance?

Thanx

Axel

Comment 15

[John Morrison]

I think you can close this bug, leaving bug 43457 for a further revision.

I currently have bug 66516 open for what appears to be another slowdown
that showed up in builds of the 24th, but as far as I can tell, the slowdown
for this bug is gone (and since it was just a back out ...).

Comment 16

[Marc Attinasi]

Marking fixed.

Comment 17

[Chris Petersen]

Marking verified per last comments.