138900 - pageload times went up ~1% with this checkin

Status: RESOLVED FIXED

(Core :: Layout: Images, Video, and HTML Frames, defect)

Description

[Chris McAfee]

pageload times went up ~1% with this checkin:

http://bonsai.mozilla.org/cvsquery.cgi?module=MozillaTinderboxAll&branch=HEAD&cvsroot=/cvsroot&date=explicit&mindate=1019330340&maxdate=1019330580&who=jkeiser%25netscape.com

Comment 1

[Chris McAfee]

sorry, bugzilla + return submits?!  Who made that a feature.

Anyways, just a reminder bug to check this out,
most of the linux Tp tests show a jump of about 1%
around noon today.

Comment 2

[John Keiser (jkeiser)]

Methinks we must be calling load twice, though I am not sure why.

Comment 3

[Markus Hübner]

Any new findings on that?

Comment 4

[John Keiser (jkeiser)]

Attachment: Patch

This patch ought to help things (it will cause us to call LoadFrame() less,
which we were doing on unload and when the iframe was inside a form).

Comment 5

[John Keiser (jkeiser)]

Attachment: Patch v1.0.1

Oops, that last patch was an earlier version--this is the real one.

Comment 6

[Johnny Stenback (:jst)]

Comment on attachment 80744 [details] [diff] [review]
Patch v1.0.1

sr=jst

Do we still have a problem with iframe's loading their document twice if we end
up demoting a container that contains an iframe element?

Either way, we can open a new bug on that if that's still a problem.

Comment 7

[John Keiser (jkeiser)]

We shouldn't--that's why I added the code in SetParent().

Comment 8

[Johnny Stenback (:jst)]

What happens when we're done with DemoteContainer() and we set the parent back
to a non-null value? Don't we then end up requesting the same document again?

Comment 9

[John Keiser (jkeiser)]

Attachment: Patch v1.1

Fix DemoteForm() problems by only loading frame when the frame loader is not
there, which it will not be in the case that the iframe has actually been
removed from the document and reinserted.

Comment 10

[Peter Van der Beken [:peterv]]

r=peterv on the right patch (the one with |if (!aParent || mFrameLoader)|
instead of |if (!aParent)|). I don't like the name *Ensure*FrameLoader since
you're now actually destroying the loader in that function sometimes. r stands
regardless of a better name, but as you suggested I prefer Validate to Ensure.

Comment 11

[John Keiser (jkeiser)]

Comment on attachment 80905 [details] [diff] [review]
Patch v1.1

From IRC:
r=peterv, assuming you upload the actual patch with the change you talked about
(ED: change if (!aParent) to if (!aParent || mFrameLoader))

Also consider changing EnsureFrameLoader to ValidateFrameLoader.

Comment 12

[Johnny Stenback (:jst)]

Comment on attachment 80905 [details] [diff] [review]
Patch v1.1

sr=jst

Comment 13

[John Keiser (jkeiser)]

Fix checked in, but does not resolve the pageload regression.

Setting as blocker of bug 52334 (the springboard of all this) so that we don't 
forget patches here.

Comment 14

[Johnny Stenback (:jst)]

Attachment: Untested perf regression fix.

This should eliminate double loading (or double initialization of loads) of
iframes which should have a positive impact on our pageload perf numbers. I'm
building right now to get some numbers, I'll report back once I have those
numbers...

Comment 15

[John Keiser (jkeiser)]

Comment on attachment 85512 [details] [diff] [review]
Untested perf regression fix.

r=jkeiser

Comment 16

[Johnny Stenback (:jst)]

Comment on attachment 85512 [details] [diff] [review]
Untested perf regression fix.

jag says sr=jag

Comment 17

[Johnny Stenback (:jst)]

Fix checked in, lets see if we get any Tp improvments from this now...

Comment 18

[Markus Hübner]

Any stats about the improvements?

Comment 19

[Johnny Stenback (:jst)]

There was an improvment in the Tp numbers after the checkin, but we're talking
small numbers here, so I'm not sure it was all of 1%... Cathleen, could you look
into this, how much did we loose and how much did we gain back? Can we close
this bug? I'm not sure there's much more we can do here...

Comment 20

[Michael Gabriel]

so check it in ?

Comment 21

[Markus Hübner]

Is there still anything that need's to be done about that?

Comment 22

[Chris McAfee]

What is the status of the two attached patches?  Let's find that out and then
make a decision about what to do with this bug.

Comment 23

[Daniel Bratell]

They were checked in according to comments, so there's no work here not in the
tree already.

Comment 24

[Chris McAfee]

jst, we probably should mark this as worksforme or fixed.  I'll give jst a
chance to do so.

Comment 25

[John Keiser (jkeiser)]

Much of the 1% was already recovered by the patches in this bug.