Closed Bug 109428 Opened 19 years ago Closed 18 years ago

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

Categories

(Core :: Layout, defect, P3)

x86
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla0.9.8

People

(Reporter: waterson, Assigned: dbaron)

References

(Blocks 1 open bug)

Details

(Keywords: memory-footprint, perf)

Attachments

(2 files, 3 obsolete files)

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.
Status: NEW → ASSIGNED
Depends on: 108309
Keywords: footprint, perf
Depends on: 71668
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?
No longer depends on: 71668, 108309
Keywords: footprint, perf
(waah! jst, you're clobbering fields!)
Depends on: 71668, 108309
Keywords: footprint, perf
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.
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?
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.
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.
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.
... or all non-leaf frames.
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.
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?
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.
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.
Priority: -- → P3
Target Milestone: --- → mozilla0.9.8
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%.
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
No longer blocks: 109885
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.
Attached patch cleaner patch (obsolete) — Splinter Review
Attachment #59012 - Attachment is obsolete: true
*** Bug 77114 has been marked as a duplicate of this bug. ***
Taking.
Assignee: waterson → dbaron
Status: ASSIGNED → NEW
Status: NEW → ASSIGNED
Blocks: 71874
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.)
No longer depends on: 71668
Blocks: 71668
Attached patch current state of my patch (obsolete) — Splinter Review
Attachment #59070 - Attachment is obsolete: true
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 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
Attachment #63976 - Flags: superreview+
Rod, could you review this patch?  I'm not sure whether you got my email.
The combobox changes look good. r=rods
Fix checked in 2002-01-11 12:10 PST.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.