109428 - PresShell::ContentAppended calls GPFF on every frame that it added

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Description

[Chris Waterson]

This defeats optimizations in the frame ctor which don't add frames that are
likely not to be searched for; e.g., <br>'s. Thing is, this code is here to
restore a frame's state, much of which ought to go away with jkeiser and jst's
work to move frame state into the content model.

Filing this bug, dependent on that work: when it's done, we ought to nuke this code.

Comment 1

[Johnny Stenback (:jst)]

Yeah, we should be able to get rid of that, the only frames (AFAIK) that still
need to save/restore state is scroll frames, but I don't see why the frames
themselves couldn't do that when they're created/destroyed.

Jkeiser, could this be done already, or do we still have some odd formcontrol
frames that still rely on this?

Comment 2

[Chris Waterson]

(waah! jst, you're clobbering fields!)

Comment 3

[John Keiser (jkeiser)]

I think <IFRAME> saves / restores, but I'm not sure about that.  And all form
controls are currently saving / restoring from the frame.  But I don't see any
roadblocks to moving save / restore calls into the frame itself.

Comment 4

[Johnny Stenback (:jst)]

John, isn't the idea to actually move the save/restore into the elements so that
the frames wouldn't need to care at all about this?

Comment 5

[John Keiser (jkeiser)]

Yes, that's the plan.  And that will happen for all form controls.  IFRAME may
or may not need to be stateful, but I hear that's a whole can of worms so that
might not get tackled just yet.

I had thought you were talking about a proposal to have the frames that need to
call RestoreState/SaveState do so in their Init and Destroy methods (or some
such) so that the overhead isn't taken on every single element, just stateful ones.

Comment 6

[Johnny Stenback (:jst)]

Yeah, that's the general approach, but if we're taking out the restore/save
functionality from form control frames anyway we might as well do that *before*
we do this so that we don't need to spend time here moving that code around.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Another possibility for this bug is that we put all frames into the map when we
create them, fixing a number of serious performance problems with GPFF.  We
could then make GPFF be a pldhash.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

... or all non-leaf frames.

Comment 9

[Chris Waterson]

IIRC, the reason that it was originally done this way was because (most of the
time, when just browsing web pages) you don't need to be able to get from a
content node to a frame. Perhaps this was a premature optimization: we ought to
collect some data to get a feel for how many primary frames we end up with given
a set of ``typical'' documents.

(Thinking out loud.) A dhash would cost (on average) ~4.5 words per primary
frame (which is somewhere between 3 words per PF for a fully loaded table and 6
words per PF for a 50% loaded table).

   #PF  words    bytes
    10     45      180
   100    450     1800 
  1000   4500    18000
 10000  45000   180000
100000 450000  1800000

If we had 10000 frames, say at an average of 60 bytes per frame (rough estimate
from looking at viewer's Debug -> Dump Frame Sizes menu), that would account for
600000 bytes (not including lineboxes). In that light, saving some portion (say,
half) of the 180000 bytes from the PF map represents a non-trivial savings on
the overall footprint of the formatting objects.

Anyway, I'd lobby for collecting some real numbers before doing away with this
optimization.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The pldhash could be done as 2 words per hash entry, since we can override
GetKey to avoid storing the content node in the entry itself (since the frame
has a pointer to it).  How much smaller is that than the current DST structure?
 Also, what would happen if we didn't put in any frame that was the only child
of its parent (which would cover many text frames), or maybe no text frames at all?

Comment 11

[Chris Waterson]

It sounds like there are two things we're discussing:

1. Using dhash instead of dst as the map's datastructure. Each dst entry is
   three words IIRC, so your way of using dhash (clever!) would win on a
   well-loaded table. Probably a no brainer to just go do it, and this'd drop
   my numbers from 180,000 out of 600,000 (30%) to 120,000 out of 600,000
   (20%).

2. Eagerly adding all (or more) frames to the map. It's not clear to
   me that lazy addition was a bad idea (assuming that we can
   un-do the ContentAppended GPFF damage, of course). On large, static pages,
   it seems like it ought to save a noticable amount of memory, if you
   believe my back-o'-the-napkin math above. We ought to do a bit more
   analysis here, IMO.

Comment 12

[Chris Waterson]

Okay, did some quick and dirty analysis on this: #if 0'ing out the frame state
restoration code in ContentAppended drops page load on jrgm's test from 1095msec
to 1086msec (in my laptop loopback configuration). That's about a 0.8% win, but
is pretty close to the harness noise level. Worth getting to this some day. I'll
mark 0.9.8 and hope that 108309 gets fixed soon.

Comment 13

[Randell Jesup [:jesup] (needinfo me)]

As per comments in bug 54542: this is a minor win for the pageload test
(dbaron's jprof shows 0.5% in GPFF).  However, in a number of large pages like
the c't page (from bug 110251) it's as high as 58% of (much too high) loadtime.
 I have others that show it over 30%.

Comment 14

[Boris Zbarsky [:bzbarsky]]

This absolutely kills us on long tables, by the way.  Since table-internal
elements are not in the hash it makes table loading O(n^2) in the number of
rows.... bug 109885 in an extreme example

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The interesting thing is that there's already code to restore the frame state at
frame construction time -- we just only set the layout history state in rare
cases.  I have a patch that uses that existing code more and seems to work.

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch that seems to work

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: cleaner patch

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

*** Bug 77114 has been marked as a duplicate of this bug. ***

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Taking.

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The above patch breaks session history restoration for comboboxes.  I have a
change that I think should fix that problem, but I'm waiting for my build to
finish.  (The change is moving the InitAndRestorFrame call for the combobox
frame after the SetDropdown call.)

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: current state of my patch

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'm pretty much stuck here on trying to get session history restoration to work
again, at least for comboboxes.  I haven't even debugged checkboxes and radio
buttons yet, although they could be simpler...

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: working patch (diff -u, for testing)

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: working patch (diff -uw, for review)

Comment 25

[Chris Waterson]

Comment on attachment 63976 [details] [diff] [review]
working patch (diff -uw, for review)

>Index: base/src/nsFrameManager.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/layout/html/base/src/nsFrameManager.cpp,v
>retrieving revision 1.100
>diff -u -d -w -r1.100 nsFrameManager.cpp
>--- base/src/nsFrameManager.cpp	5 Jan 2002 15:22:52 -0000	1.100
>+++ base/src/nsFrameManager.cpp	8 Jan 2002 17:11:13 -0000
>@@ -649,7 +649,14 @@
>         rv = parent->IndexOf(aContent, index);
>         if (NS_SUCCEEDED(rv) && index>0)  // no use looking if it's the first child
>         {
>-          rv = parent->ChildAt(index-1, *getter_AddRefs(prevSibling));
>+          nsCOMPtr<nsIAtom> tag;
>+          do {
>+            rv = parent->ChildAt(--index, *getter_AddRefs(prevSibling));
>+            prevSibling->GetTag(*getter_AddRefs(tag));
>+          } while (index &&
>+                   (tag == nsLayoutAtoms::textTagName ||
>+                    tag == nsLayoutAtoms::commentTagName ||
>+                    tag == nsLayoutAtoms::processingInstructionTagName));
>           if (NS_SUCCEEDED(rv) && prevSibling)
>           {
>             entry = NS_STATIC_CAST(PrimaryFrameMapEntry*,

You could probably just do away with |rv| here. You're going to crash
if |parent->ChildAt()| fails, anyway.

The rest looks fine (although rods should probably go through it).

sr=waterson

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Rod, could you review this patch?  I'm not sure whether you got my email.

Comment 27

[rods (gone)]

The combobox changes look good. r=rods

Comment 28

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Fix checked in 2002-01-11 12:10 PST.