Closed Bug 148636 Opened 22 years ago Closed 13 years ago

Enormous memory usage rendering with lots of form elements

Categories

(Core :: XBL, defect, P2)

Product:
Core
Component:
XBL
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Future

People

(Reporter: DomIncollingo, Assigned: john)

References

(Blocks 2 open bugs,
URL
)

Details

(Keywords: memory-footprint, perf, testcase, Whiteboard: [MemShrink])

Attachments

(4 files, 1 obsolete file)

Memory usage at http://dialaccess.rr.com/servlet/dr/AccessNumber
56.63 KB, image/jpeg
Details
testcase
133.37 KB, text/html
Details
same thing with peterv's change
15.28 KB, patch
john
: review+
bryner
: superreview+
asa
: approval1.4+
Details | Diff | Splinter Review
diff -wu version of above
3.83 KB, patch
Details | Diff | Splinter Review 
From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.0.0+)
Gecko/20020601
BuildID:    2002060104

Mozilla memory usgage jumps to 92 MB trying to render the Time Warner Road
Runner site http://dialaccess.rr.com/servlet/dr/AccessNumber.

Using 2002060104 on Win 2K with 512 MB of RAM.

Reproducible: Always
Steps to Reproduce:
1.Launch Mozilla. 
2.Launch Windows Task Manager and verify that Mozilla memory usage is about 21
MB.  (May vary slightly depending on your home page.)
3.Naviate to http://dialaccess.rr.com/servlet/dr/AccessNumber.
4.Watch Mozilla memory usage in Windows Task Manger.  Memory usage climbs
steadily as Mozilla tries to render this page.  Memory usage continues to climb
even after the page is rendered. Memory usage tops off at about 92 MB. 

Actual Results:  Memory usage climbs to 92 MB.

Expected Results:  Memory usage should not climb this much.  If this page is a
memory-intensive page, it might be normal for memory to climb a bit, but a
memory increase of almost 450% seems excessive.

This page also renders very slowly on Mozilla.  But it renders very fast on IE
5.5.  I don't know if this is a problem with Mozilla, or if this page uses
non-standard HTML that is "optimized" for IE.  If this page has been "optimized"
for IE, then I will file an Evangilism bug.  A Time Warner site should not be
optimized for a competitor's product.
The image at the top shows memory usage before navigating to the above site. 
The image on the bottom shows memory usage a few seconds after the above site
was rendered.  (After memory usage stopped climbing.)
Confirmed Win2k 2002060204
CPU pegs to 100% for about 30 seconds, memory usage jumped to over 100MB

Probably a dupe, though...
Status: UNCONFIRMED → NEW
Ever confirmed: true
-> Layout for triage
2nd try
Assignee: Matti → attinasi
Component: Browser-General → Layout
QA Contact: imajes-qa → petersen
The page at this URL is horrible in so many ways.  The most telling thing is:
<META NAME="GENERATOR" Content="Microsoft Visual Studio 6.0">

The document's structure is also busted:
<HTML>
 <HEAD>...</HEAD>
 <BODY>
  ...
  <HTML>
    <HEAD>...</HEAD>
    <BODY>...</BODY>
  </HTML>
 </BODY>
</HTML>

The basic layout of the page is a table with 1,686 rows and 3 columns per row 
(5058 total).  Each TD element has a WIDTH=xx attribute and a style="WIDTH: 
xxpx" attribute.  Inside each TD element is a readonly INPUT element with the 
actual data.

Horrible, horrible, horrible.  It apparently takes Mozilla a while to create 
over 5000 textboxes...

That said, IE6 renders it here in under 9 seconds while Moz 2002060208/WinNT 
takes almost three minutes.  While Moz is churning away, its UI is completely 
unresponsive.  (I also can't seem to start new processes while Moz is trying to 
render the page.  Weird.)

Mozilla's performance isn't on-par with IE, but whoever made that page needs to 
be hit in the head a couple times with an aluminum baseball bat.
Attached file testcase — 
original URL uses 89MB of memory for me.  testcase of ~5000 <INPUT> uses 72MB.
I ran this under gdb and noticed it spending a lot of time in
nsXBLBinding::InstallEventHandlers.

http://lxr.mozilla.org/mozilla/source/content/xbl/src/nsXBLBinding.cpp#829

Further invesetigation showed that for every <INPUT> element in the page,
nsXBLBinding::InstallEventHandlers is called twice.  Between the two calls, it
does "receiver->AddEventListener" 43 times, all of "type" keypress.

43*5055=217365 event listeners.  To account for the ~40-50MB of memory the
<INPUT> tags seem to use, each EventListener would need to take up ~200 bytes. 
I don't know if that is close or not.

if EventListener is eating the memory, this isn't Layout.
QA Contact: petersen → amar
Keywords: footprint
OS: Windows 2000 → All
Hardware: PC → All
Priority: -- → P2
adding myself to CC and nominating for mozila1.0.1
Keywords: mozilla1.0.1
Target Milestone: --- → Future
Live Bloat (http://www.mozilla.org/projects/footprint/live-bloat.html) says:
TOTAL                 1299068 50564544 100.00   0.00
FrameArena               2467 10151705  20.08  20.08
nsEventListenerManager 310389  5707260  11.29  31.36
nsXBLKeyHandler        217447  5218728  10.32  41.68
void*                   70904  4168556   8.24  49.93
nsCSSDeclaration        91011  3954464   7.82  57.75
nsSupportsArray         71489  3717428   7.35  65.10
nsDOMEventRTTearoff    217412  2608944   5.16  70.26
nsTypedSelection        30360  2064480   4.08  74.34
nsTransactionManager    10112  1233664   2.44  76.78
nsPlaintextEditor        5056  1051648   2.08  78.86
nsSelection              5060   991760   1.96  80.82
nsHTMLAttributes        25302   728544   1.44  82.27
nsScrollPortView         5059   688024   1.36  83.63
PRMonitor               10156   629672   1.25  84.87
nsGenericElement        20304   608616   1.20  86.08
nsCSSRule                7275   579852   1.15  87.22
nsView                   5090   529360   1.05  88.27
xpti-unclassified         376   403344   0.80  89.07
nsXBLBinding            10550   380400   0.75  89.82
OTHER                  183249  5148095  10.18 100.00

this is also pointing at EventListener/XBL as a major memory grabber.
looks like I left off the heading. it was something like:

                       count   bytes     %     cumulative %
TOTAL                 1299068 50564544 100.00
FrameArena               2467 10151705  20.08  20.08
...

that was with the testcase rather than the original page.
So... why are there 310389 nsEventListenerManager objects?  And 217412
nsDOMEventRTTearoff objects?  We only have 20304 nsGenericElement objects, and
it seems like the number of EventListenerManagers should not be much bigger than
that.....
Each input element gets 43 "keypress" listeners (comment 7)
From #mozilla:

<caillon> bz: each <command> or <key> generates an nsXBLPrototypeHandler which is
 inherited by an nsXBLFooHandler
> caillon: so... each <key> generates a separate event listener?
<caillon> bz: yes.
<shaver> we should consider fixing that


Another question. Why does each <input> get fresh nsXBLKeyHandlers?  Shouldn't
we just create them per-binding and attach the same handler to multiple objects
or something?  There seems to be no good reason to create thousands of objects
which just implement a single <key> but are attached to different <input>s
Keywords: mozilla1.0.1
Summary: Enormous memory usage rendering http://dialaccess.rr.com/servlet/dr/AccessNumber → Enormous memory usage rendering with lots of form elements
Keywords: mozilla1.3
==> form controls
Assignee: attinasi → form
Component: Layout → Layout: Form Controls
Keywords: testcase
QA Contact: amar → tpreston
The fix here is in XBL.  If this is the case still, I'd expect a pretty big
footprint win as the chrome uses this stuff all over the place.
Component: Layout: Form Controls → XBL
reassign
Assignee: form → hyatt
QA Contact: tpreston → ian
Blocks: 146468
A nice example is also
http://www.lysator.liu.se/~alla/moz/rules.html
from bug 39573. Using trunk build 2003020308 on winxp pro sp1 ... memory usage 
is skyrocketing (about 1MB/sec).

-> someone who works here
Assignee: hyatt → jkeiser
Keywords: perf
I was poking around this a while back ...

So, as bz said, the primary problem here is that XBL end just allocates way too
many event handlers. Looks like that is non-trivial to fix.... haven't really
looked at it. 

The secondary problem is that these handlers hold on to content objects through
nsIDOMEventReceiver interfaces. nsGenericElement implements this in a tearoff,
so we end up holding on to tons of little tearoff objects for no good reason. In
this case, if, as mentioned above, there are 43 key listeners per <input>, we
allocate and hold on to 43 |nsDOMEventRTTearoff|'s for approximately as long as
the lifetime of the input element itself. Which is clearly lame, and it also
overwhelms the little 4 element tearoff cache in nsGenericElement. 

I'm about to attach rather simple patch that will get us down to holding on to 
1 tearoff per element. As expected it reduces the memory usage for testcase in
this bug by a decent bit. (Still nowhere close to IE of course.)

However, in general, I dont see the point of doing tearoffs if other long-lived
objects are holding strong refs through those interfaces. Even if there were
fewer key handlers as bz outlined above, the tearoff overall still hurts
footprint as long as there is even a single handler holding on to it.

One of these needs to happen:

1) We teach XBL to not QI to tearoff interfaces till it absolutely needs to, and
then release the ref immediately. This may actually slow down dispatch of the
events since you now have to do a rather slow QI at that time. I have no idea if
this will be significant/measurable though. 

2) We decide the tearoff is just not worth it for some elements which we know
are going to have lots of handlers. Special case certain elements (like input)
to not use the nsGenericElement tearoff for them. Basically, make these elements
inherit directly from the interface and implement it. And write a special QI
that does the right thing. This will cause some code bloat and increase the size
of the particular element object itself, but saves us quite a bit at runtime.
Keywords: mozilla1.3
Attached patch Do the QI's outside the loop (obsolete) — — Splinter Review 
I dont have an opt build handy right now and am unable to quote useful
footprint numbers. In my windows debug build from about 15 days back, this
patch shaved off ~10 MB from the memory needed for loading attachment 86005 [details]
above. Clearly, the real win may be less than that. It would be nice if someone
can measure it.
Comment on attachment 123619 [details] [diff] [review]
Do the QI's outside the loop

John, can you please look at this simple patch.
Attachment #123619 - Flags: review?(jkeiser)
Did you look at the possiblilty to have one listener for several keys instead of
one listener per key?

> Did you look at the possiblilty to have one listener for several keys instead
> of one listener per key?

Not yet. As I said at the top of comment 20, that is indeed the primary problem,
and something I have not looked at yet. (Someone more familiar with the xbl code
will probably have to do that.) The patch above, and the rest of my comment
addressed a secondary issue, that I believe needs to be fixed nonetheless. 

In case I wasnt clear, the patch is only the first of several things that could
be done here. It simply happens to be the most obvious and most straightforward
of them. It just fixes what I believe was a oversight. (Infact, its simple
enough that I would not hesitate to ask for 1.4 approval even at this stage, if
I happen to get reviews in time.) 

Even if we had just one key listener, the patch would still be worth doing,
since we would share the tearoff object even between instances of different
*kinds* of handlers (mouse/key and so on ....).
I'm looking at sharing listeners. I have a patch that seems to be working but I
haven't done any performance tests yet.
Comment on attachment 123619 [details] [diff] [review]
Do the QI's outside the loop

It looks good to me, bryner needs to sr though.  This is a real problem with
the way we do these tearoffs, and you're right, we should try not to have
long-lived references to tearoffs.
Attachment #123619 - Flags: superreview?(bryner)
Attachment #123619 - Flags: review?(jkeiser)
Attachment #123619 - Flags: review+
Comment on attachment 123619 [details] [diff] [review]
Do the QI's outside the loop

Since you moved the receiver out of the loop you could also get the system
eventgroup once. Something like this:

>Index: nsXBLBinding.cpp
>===================================================================

>+    nsCOMPtr<nsIDOMEventReceiver> receiver = do_QueryInterface(mBoundElement);
>+    nsCOMPtr<nsIDOM3EventTarget> target = do_QueryInterface(receiver);

    nsCOMPtr<nsIDOMEventGroup> systemEventGroup;

>     for (nsXBLPrototypeHandler* curr = handlerChain; curr;
>          curr = curr->GetNextHandler()) {
...
>       // If this is a command, add it in the system event group, otherwise 
>       // add it to the standard event group.
> 
>-      nsCOMPtr<nsIDOM3EventTarget> target = do_QueryInterface(receiver);

      nsIDOMEventGroup* eventGroup = nsnull;
      if (curr->GetType() & NS_HANDLER_TYPE_XBL_COMMAND) {
	if (!systemEventGroup)
	  receiver->GetSystemEventGroup(getter_AddRefs(systemEventGroup));
	eventGroup = systemEventGroup;
      }

> 
>       if (found) { 
...

Not sure if getting the system eventgroup is expensive though.
Well, I wouldnt think that getting the system event group is expensive, since
the eventlistener manager is returning a cached ref to it. But it doesnt hurt,
so here is a patch that does that.

I also realized that the previous patch made us do extra work in the case when
there are no event handlers, so I put the whole loop thing inside a null check.


This patch is much smaller than it looks. diff -w is coming up in a minute for
easier reviewing.
Attachment #123619 - Attachment is obsolete: true
Attachment #123692 - Flags: superreview?(bryner)
Attachment #123692 - Flags: review?(jkeiser)
Depends on: 206321
Comment on attachment 123692 [details] [diff] [review]
same thing with peterv's change 

You need to at least add a comment next to eventGroup saying it is a weak
reference.  I think it's fine to make it so since you're guaranteed
systemEventGroup will be around, just comment it.

Also, I'm seeing weird whitespace even in this (not -w) version of the patch:

+	   }
+	   else if (!attachType.IsEmpty() &&
!attachType.Equals(NS_LITERAL_STRING("_element"))) {
	   nsCOMPtr<nsIDocument> boundDoc;
	   mBoundElement->GetDocument(*getter_AddRefs(boundDoc));
	   nsCOMPtr<nsIDOMDocument> domDoc(do_QueryInterface(boundDoc));
	   nsCOMPtr<nsIDOMElement> otherElement;
	   domDoc->GetElementById(attachType, getter_AddRefs(otherElement));
	   receiver = do_QueryInterface(otherElement);

Fix that and r=me.  If the whitespace is an error don't worry about posting
another patch.

This will be worth getting approval for, I think.
Attachment #123692 - Flags: review?(jkeiser) → review+
Comment on attachment 123692 [details] [diff] [review]
same thing with peterv's change 

This is a great fix. sr=me (though if you could come up with a way to avoid
adding another level of indentation that would be nice, and make the diff
smaller)
Attachment #123692 - Flags: superreview?(bryner) → superreview+
I added the comment about the weak ref. Have also fixed up the indentation
glitch. (That bit of code was commented out, emacs got confused about indenting
the commented out code.)

Avoiding the extra indentation looks like it will some more reorganization or
more code. I've elected to leave as it is for now, hope thats ok.

Comment on attachment 123692 [details] [diff] [review]
same thing with peterv's change 

This is a small change that reduces the footprint costs of xbl event handlers.
The real changes are much smaller than the diff looks. I believe its quite
safe. 
It saves us a decent bit of memory on some large pages. Would be nice to have
this this in 1.4.
Attachment #123692 - Flags: approval1.4?
Comment on attachment 123692 [details] [diff] [review]
same thing with peterv's change 

a=asa (on behalf of drivers) for checkin to 1.4
Attachment #123692 - Flags: approval1.4? → approval1.4+
Attachment #123619 - Flags: superreview?(bryner)
Comment on attachment 123692 [details] [diff] [review]
same thing with peterv's change 

checked in
So, are we really "cloning" all 43 event listeners for each input/password/text
area/other xbl binding? Why can't we make the xbl prototype handler the event
listener, surely it can get the event target from the event itself?
peterv points out that once (!) attachto is implemented all those event
listeners that use it have to be unique...
Neil: all of them, or just one for each "attachto" type?
Every attachto event handler needs to know the original bound element so that
the "this" variable can be set correctly when the handler executes (currently
this is simulated by setting globals or using JS objects as event handlers).
This fix seems to have improved things a bit. For a large record set in our
system  (3885 rows at 3 hidden fields, a drop down list and 2 simple text fields
per row) the change in memory usage between after and before displaying the page
are:

          delta M (kB)    delta peak memory (kB)   delta VM size (kB)
1.4beta        154,008                  160,696               152,916
2003052108     149,200                  148,852               148,184

So it saves ~1.3kB per row at the end of processing. I forgot to note the time
taken, but it certainly refreshed the screen more with the later builds. (All
measurements from clean start-up on a Win2K system).

Unfortunately IE still takes less than half the time and only consumes an extra
79MB when displaying this page :-(.
Blocks: 203448
Could anyone redo some tests now that the fix for bug 206321 is in?
The patch for bug 206321 seems to have improved things a bit.

* Before patch (Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.5b)
Gecko/20030910)
On launch with http://www.mozilla.org/start/
RPRVT  RSIZE  VSIZE
12.9M  28.6M   289M

After loading http://bugzilla.mozilla.org/attachment.cgi?id=86005&action=view
RPRVT  RSIZE  VSIZE
55.7M+ 71.6M+  332M+

Delta: ~43M

* After patch (Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.5b)
Gecko/20030914)
On launch with http://www.mozilla.org/start/
RPRVT  RSIZE  VSIZE
12.8M- 28.6M+  290M-

After loading http://bugzilla.mozilla.org/attachment.cgi?id=86005&action=view
RPRVT  RSIZE  VSIZE
49.1M+ 65.0M+  326M+

Delta: ~36M

In my opinion, the sorta fundamental problem is this, what 'looks like' 
a simple little input text field, is actually implemented in Mozilla as 
a rather full-function text editor with a small window. What's seems a 
huge footprint for 5000 input fields, isn't really so large for 5000 
concurrent text editors. The only way to get significant improvement 
for such cases is a fundamental change in the implementation of single 
line input text fields. These need to be implemented much more simply, 
probably not using libeditor.
Pray tell me what features you would suggest removing from the current textfield
implementation in the process?
Boris, the current textfield implementation in Mozilla is 13X-15X 
slower, and many times larger footprint, compared to the same function 
in IE. There are few other things in Mozilla that compare so badly to 
IE, performance-wise. The point that I was trying to make, perhaps 
badly, was just 'why this is so'. IE's implements a simple inputfield 
as a simple inputfield. Mozilla implements a simple inputfield as a 
full-function text editor having special attributes to make it 'look 
like' a simple input field. Not saying that any features need to be 
removed. The design needs re-worked, and the features re-implemented 
much more simple design. Sure, the current design can be tweaked, and 
perhaps that can make in only 10X slower, for whatever that is worth.  
I found a line of code that is costing over 20% of the time in bringing up forms
with lots of input fields.  I commented out the call to set the text editor
created for each input field to nowrap ("white-space: pre").  The code I am
talking about  is in layout/html/forms/src/nsTextControlFrame.cpp.  I marked it
with //ivan

 // Set up wrapping
1844     if (IsTextArea()) {
1845       // wrap=off means -1 for wrap width no matter what cols is
1846       nsFormControlHelper::nsHTMLTextWrap wrapProp;
1847       nsFormControlHelper::GetWrapPropertyEnum(mContent, wrapProp);
1848       if (wrapProp == nsFormControlHelper::eHTMLTextWrap_Off) {
1849         // do not wrap when wrap=off
1850   //ivan      textEditor->SetWrapWidth(-1);
1851       } else {
1852         // Set wrapping normally otherwise
1853         textEditor->SetWrapWidth(GetCols());
1854       }
1855     } else {
1856       // Never wrap non-textareas
1857  //ivan    textEditor->SetWrapWidth(-1);
1858     }

The SetWrapWidth(-1) turns off wrapping ("white-space: pre").  I believe the
default for wrapping is inherit.  I assume that should be nowrap.  I tested
after commenting out the 2 lines above and my tests ran 20% faster.

Ivan 
Ivan: that's great! can you file a seperate bug? something along the lines of
"text edit fields should default to white-space: nowrap"?

this is more of a meta-bug so actual issues belong in their own bug... CC me on
it and I'll help review. 
I created a new bug 221150 for "text edit fields should default to white-space:
nowrap".

Ivan
Are you suggesting that the default should be "nowrap" because that's correct,
or because it's faster?
I believe nowrap is both correct and faster for editing an input field.
I don't think leaving out the nowrap setting will hurt anything. If an
input field fails the check to text area than it can't wrap so the setting is
meaningless.  I believe it is a single line text control.
  I made a mistake in my first try for a fix.  Only the second
"textEditor->SetWrapWidth(-1);" should have been commented out (see below).

 // Set up wrapping
1844     if (IsTextArea()) {
1845       // wrap=off means -1 for wrap width no matter what cols is
1846       nsFormControlHelper::nsHTMLTextWrap wrapProp;
1847       nsFormControlHelper::GetWrapPropertyEnum(mContent, wrapProp);
1848       if (wrapProp == nsFormControlHelper::eHTMLTextWrap_Off) {
1849         // do not wrap when wrap=off
1850         textEditor->SetWrapWidth(-1);
1851       } else {
1852         // Set wrapping normally otherwise
1853         textEditor->SetWrapWidth(GetCols());
1854       }
1855     } else {
1856       // Never wrap non-textareas
1857  //ivan    textEditor->SetWrapWidth(-1);
1858     } 
I added that bug as a dependant... lets take this up there.
Depends on: 221150
*** Bug 152385 has been marked as a duplicate of this bug. ***
Blocks: 277123
Blocks: 335984
Depends on: 221820
QA Contact: ian → xbl
So, I tested this test case with a build before bug 221820 and another one after it.  Before bug 221820, it took me about 32MB of memory to load that page, after that, it took me about 20MB.
I don't see huge memory usage with Firefox 8.0a2. Adding to MemShrink to see if this can be marked as WORKSFORME.
Whiteboard: [MemShrink]
Based on comment 54 and comment 55, and the fact that all the dependent bugs are fixed, I'll resolve this.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: