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)
Core
XBL
Tracking
()
RESOLVED
FIXED
Future
People
(Reporter: DomIncollingo, Assigned: john)
References
(Blocks 2 open bugs, )
Details
(Keywords: memory-footprint, perf, testcase, Whiteboard: [MemShrink])
Attachments
(4 files, 1 obsolete file)
(deleted),
image/jpeg
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
patch
|
john
:
review+
bryner
:
superreview+
asa
:
approval1.4+
|
Details | Diff | Splinter Review |
(deleted),
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.
Reporter | ||
Comment 1•22 years ago
|
||
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
Comment 3•22 years ago
|
||
-> Layout for triage
Comment 4•22 years ago
|
||
2nd try
Assignee: Matti → attinasi
Component: Browser-General → Layout
QA Contact: imajes-qa → petersen
Comment 5•22 years ago
|
||
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.
Comment 6•22 years ago
|
||
original URL uses 89MB of memory for me. testcase of ~5000 <INPUT> uses 72MB.
Comment 7•22 years ago
|
||
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.
Updated•22 years ago
|
QA Contact: petersen → amar
Updated•22 years ago
|
Updated•22 years ago
|
Priority: -- → P2
Updated•22 years ago
|
Target Milestone: --- → Future
Comment 9•22 years ago
|
||
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.
Comment 10•22 years ago
|
||
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.
Comment 11•22 years ago
|
||
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.....
Comment 12•22 years ago
|
||
Each input element gets 43 "keypress" listeners (comment 7)
Comment 13•22 years ago
|
||
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
Comment 14•22 years ago
|
||
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
Updated•22 years ago
|
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
Updated•22 years ago
|
Keywords: mozilla1.3
Comment 15•22 years ago
|
||
==> form controls
Assignee: attinasi → form
Component: Layout → Layout: Form Controls
Keywords: testcase
QA Contact: amar → tpreston
Assignee | ||
Comment 16•22 years ago
|
||
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
Comment 18•22 years ago
|
||
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).
Comment 20•21 years ago
|
||
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
Comment 21•21 years ago
|
||
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 22•21 years ago
|
||
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)
Comment 23•21 years ago
|
||
Did you look at the possiblilty to have one listener for several keys instead of
one listener per key?
Comment 24•21 years ago
|
||
> 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 ....).
Comment 25•21 years ago
|
||
I'm looking at sharing listeners. I have a patch that seems to be working but I
haven't done any performance tests yet.
Assignee | ||
Comment 26•21 years ago
|
||
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 27•21 years ago
|
||
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.
Comment 28•21 years ago
|
||
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
Comment 29•21 years ago
|
||
Updated•21 years ago
|
Attachment #123692 -
Flags: superreview?(bryner)
Attachment #123692 -
Flags: review?(jkeiser)
Assignee | ||
Comment 30•21 years ago
|
||
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 31•21 years ago
|
||
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+
Comment 32•21 years ago
|
||
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 33•21 years ago
|
||
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 34•21 years ago
|
||
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+
Updated•21 years ago
|
Attachment #123619 -
Flags: superreview?(bryner)
Comment 35•21 years ago
|
||
Comment on attachment 123692 [details] [diff] [review]
same thing with peterv's change
checked in
Comment 36•21 years ago
|
||
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?
Comment 37•21 years ago
|
||
peterv points out that once (!) attachto is implemented all those event
listeners that use it have to be unique...
Comment 38•21 years ago
|
||
Neil: all of them, or just one for each "attachto" type?
Comment 39•21 years ago
|
||
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).
Comment 40•21 years ago
|
||
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 :-(.
Comment 41•21 years ago
|
||
Could anyone redo some tests now that the fix for bug 206321 is in?
Comment 42•21 years ago
|
||
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
Comment 43•21 years ago
|
||
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.
Comment 44•21 years ago
|
||
Pray tell me what features you would suggest removing from the current textfield
implementation in the process?
Comment 45•21 years ago
|
||
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.
Comment 46•21 years ago
|
||
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
Comment 47•21 years ago
|
||
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.
Comment 48•21 years ago
|
||
I created a new bug 221150 for "text edit fields should default to white-space:
nowrap".
Ivan
Comment 49•21 years ago
|
||
Are you suggesting that the default should be "nowrap" because that's correct,
or because it's faster?
Comment 50•21 years ago
|
||
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 }
Comment 51•21 years ago
|
||
I added that bug as a dependant... lets take this up there.
Depends on: 221150
Comment 52•20 years ago
|
||
*** Bug 152385 has been marked as a duplicate of this bug. ***
Comment 53•18 years ago
|
||
Bug 277123 comment 1 also as a testcase, attachment 170353 [details]
Updated•15 years ago
|
QA Contact: ian → xbl
Comment 54•15 years ago
|
||
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.
Comment 55•13 years ago
|
||
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]
Comment 57•13 years ago
|
||
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.
Description
•