126212 - mozilla is much slower then IE6 ! and uses 90MB

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[DK]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:0.9.8) Gecko/20020204
BuildID:    2002020406

http://213.149.32.135/test/mozilla.zip
you can download this zip which is ~80kb

or direct download
http://213.149.32.135/test/category.asp2.html
this is ~1.8MB HTML

Time needed for rendering page is longer then IE6 and memory consuption by
Mozilla is at peak 100MB !!!

Since this document is generated by ASP, after few random selection my mozilla
used about 200MB !!!!

Reproducible: Sometimes
Steps to Reproduce:
connect to that URL or at:
http://www.operation-flashpoint.de/squadindex.php


Actual Results:  watch for slow mozilla rendering (compared at MS IE 6)

Expected Results:  faster page rendering, less memory usage

this is also on page:
http://www.operation-flashpoint.de/squadindex.php


This isn't actual bug, just observation, 'cos sometimes I use MS IE6 ...
Hope this help in better design for Gecko :)

Comment 1

[Michael Klose]

I only have a 64MB IBM Thinkpad running Win98SE, but I have noticed with build
2002021708 that it might have a memory leak. While it is one of the most stable
builds in a while, after using it for quite a while, especially with pages that
have rescaled images and afterward just browsing normally, it is terribly slow.
Closing all instances of Mozilla and restarting speeds up Mozilla on the same
pages dramatically.

So I guess there is a memory leak somewhere. I don't know if this is related to
your bug report, might be though which is why I am mentioning it.

Comment 2

[DK]

True my mozilla after some time, builds memory usage even "Enable Mem cache" is
disabled and after pressing "Flush memory", memory usage stays high.

Comment 3

[Sitsofe Wheeler]

Attachment: Testcase

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Profile of that pageload

We spend about 30% of the pageload under imgRequestProxy::OnStopRequest (from
the about 13000 spacer images on the page!).  Of this,

24.3% of pageload is in  nsDocLoaderImpl::GetRequestInfo (called from
nsDocLoaderImpl::OnStopRequest)

3.1% of pageload is in nsSupportsArray::IndexOfStartingAt, called from
nsSupportsArray::RemoveElement, called from nsLoadGroup::RemoveRequest.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Not sure whom this should go to...  To networking as a first guess, but this
could be imagelib or docshell...

Comment 6

[Sitsofe Wheeler]

(gah had a collision!)
Linux tests:
Konqueror (2.2.2) started out using about 26Mb takes 41 seconds and when it
finished rendering the page had used 54Mb (28Mb increase). Opera (6 Beta 2)
starts out at 17Mb and was using 31Mb by the time the page finished (14Mb
increase) taking about 38 seconds. Moz (2002041707) started off using 37Mb and
grew to 71Mb (34Mb increase) and took about 52 seconds.

It is not clear how well formed the html of that page is though. Confirming and
adding perf keyword.

Comment 7

[Markus Hübner]

Are we still doing so badly on this one?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Yes.  See my 2-days-old comment.  We spend forever loading this page, freezing
the whole browser in the process.  I don't care how IE does, that's
unacceptable.

Comment 9

[benc]

+helpwanted, cc'd neeti.

If this is the right component, this needs to be assigned.

Comment 10

[Darin Fisher]

seems like GetRequestInfo needs to be optimized.  perhaps the linear searched
array needs to be replaced with a hash table.  or perhaps we can do away with
the lookup somehow.

-> dp

dp: feel free to bounce this one over to me... i just won't have time to
investigate for a while.

Comment 11

[Gavin Jackson]

My Observation: 
I noticed IE progressively downloads and Gecko doesnt. 2002050807, Win2k

Comment 12

[Johnny Stenback (:jst)]

Attachment: This could help here... Make nsLoadGroup use a pldhash in stead of a linear array

Comment 13

[Johnny Stenback (:jst)]

Attachment: Also make nsDocLoader use a pldhash for the request info

Comment 14

[Johnny Stenback (:jst)]

Comment on attachment 89661 [details] [diff] [review]
This could help here... Make nsLoadGroup use a pldhash in stead of a linear array

Trivial testing shows that this gives a ~15% speedup when loading this testcase
from the cache. Yay!

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 89760 [details] [diff] [review]
Also make nsDocLoader use a pldhash for the request info

>+  // Inititlize the entry with placement new

Fix "Initialize" and r=bzbarsky

Comment 16

[rpotts (gone)]

Comment on attachment 89760 [details] [diff] [review]
Also make nsDocLoader use a pldhash for the request info

sr=[e-mail]

Comment 17

[rpotts (gone)]

Comment on attachment 89661 [details] [diff] [review]
This could help here... Make nsLoadGroup use a pldhash in stead of a linear array

sr=[e-mail]

Comment 18

[Johnny Stenback (:jst)]

Comment on attachment 89760 [details] [diff] [review]
Also make nsDocLoader use a pldhash for the request info

+  nsRequestInfo(const void *key) : mKey(key)
   {
+    // No need to initialize mCurrentProgress or mMaxProgress here
+    // since the PLDHashTable code guarantees that they're zeroed out
+    // when we get here.
   }

For everyone's information, the above comment is bogus (I told the reviewers
about this already), we do need to initialize all members since the pldhash
code only guarantees that the memory used for an entry is zeroed out the
*first* time an entry is initialized, not if an entry is reused.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 89661 [details] [diff] [review]
This could help here... Make nsLoadGroup use a pldhash in stead of a linear array

>+PR_STATIC_CALLBACK(PLDHashNumber)
>+RequestHashHashKey(PLDHashTable *table, const void *key)
>+{
>+    return NS_PTR_TO_INT32(key);
>+}

Any reason not to use the stub here?

>+    // Inititlize the entry with placement new

Spelling of "Initialize" again.

> nsresult nsLoadGroup::Init()
> {
>-    return NS_NewISupportsArray(&mRequests);
>+    return mRequests.ops ? NS_OK : NS_ERROR_OUT_OF_MEMORY;
> }

Wouldn't it make sense to PL_DHashTableInit here instead of in the constructor?

>+AppendRequestsToVoidArray(PLDHashTable *table, PLDHashEntryHdr *hdr,

Add an assertion in here that |request| is not null, please?  (since that
NS_ADDREF will crash in any case.... but it's good to have assertions up
front).

>+    //
>+    // Operate the elements from back to front so that if items get
>+    // get removed from the list it won't affect our iteration
>+    //

This comment is no longer relevant, is it?  You're iterating over the array and
removing from the hashtable, unlike the old code which iterated over the array
and removed from the array...

It feels like nsLoadGroup::Cancel should just enumerate the hashtable instead
of constructing this temporary array and enumerating that....  Not sure whether
PLDHash will deal well with an enumeration function that might ADD or REMOVE
things....  The way you're doing may be the cleanest after all.

Same for nsLoadGroup::Suspend and nsLoadGroup::Resume.

Comment 20

[Johnny Stenback (:jst)]

Can't just enumerate the hash in ::Cancel() and friends since you're right, the
pldhash can't deal with the hash being alterd from within an enumeration
callback... The other changes are made, checking in...

Comment 21

[Johnny Stenback (:jst)]

Oh, um, so I thought I had r=bzbarsky on both these patches and I checked in, I
guess I jumped the gun a little on the nsLoadGroup one :-) Let me know if we
need to change something in that diff and we'll fix it in the tree...

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 89661 [details] [diff] [review]
This could help here... Make nsLoadGroup use a pldhash in stead of a linear array

after-the-fact-r=bzbarsky with the changes you made.

Comment 23

[Boris Zbarsky [:bzbarsky]]

I just reprofiled this, using the attached zipped testcase.  Mozilla is taking
40MB of RAM with the page fully loaded.  Pageload takes less than 10 seconds on
my machine, but I have no IE to compare to.

Looking at the profile, of the 995 total jprof hits, 338 are under
nsImageLoadingContent::ImageURIChanged; of these 163 are in
imgLoader::LoadImage, 139 are in converting the image URI to a URI object (there
are about 9000 images on this page, mind you) and 11 are asking the content
policy whether to load the image.

There seem to be no other clear bottlenecks, just hits all over as you'd expect
with a 45000-line HTML file...

Reporter, could you retest how we compare to IE again?

Comment 24

[Zibi Braniecki [:zbraniecki][:gandalf]]

Mozilla 20030423: 33MB (after closing window it's reduced by 7 mb only!)
IE 6: 17 MB (after closing window it's reduced by 17 MB)

I tested on zipped testcase. So we're still using almost 2 times more memory
here, and (it's another bug) we're leaking much of this after closing window.

Comment 25

[Boris Zbarsky [:bzbarsky]]

> Mozilla 20030423: 33MB (after closing window it's reduced by 7 mb only!)

Um... 33MB of which 15-20MB is just the size of the Mozilla code that has to be
loaded into memory for Mozilla to run?  Or 33MB more than Mozilla takes up when
just started?

Comment 26

[Zibi Braniecki [:zbraniecki][:gandalf]]

33 additional MB of course for both.

Comment 27

[Boris Zbarsky [:bzbarsky]]

OK.  And how much of that (in Mozilla) is image (memory) cache?

Frankly, if we're using a reasonable amount of memory now (and it sounds like we
are), I'd consider this bug, as filed, fixed....

Comment 28

[Zibi Braniecki [:zbraniecki][:gandalf]]

1) Mozilla runned with 1 window (abount:blank) - 18 124 K
2) Mozilla runned with 1 window (testcase page from zip) - 50 305 K
--------
Diff: 32181


1) IE runned with 1 window (about:blank) - 18 872 K
2) IE runned with 1 window (testcase page from zip) - 37 608 K
--------
Diff: 18736

time diff between running query (enter on adress bar) and ready page

1) Mozilla	- 11 sec
2) IE		- 3 sec

There is also memory leak in this testcase. I would not close this bug.

Comment 29

[Boris Zbarsky [:bzbarsky]]

You didn't answer my question....  And what's leaking?  Or is it just that we
allocate a larger frame arena and the C library refuses to properly deallocate
it (see the bug on that).

Comment 30

[Zibi Braniecki [:zbraniecki][:gandalf]]

About leaking: you're probably right. I wrote in comment #24 that Mozilla doesnt
deallocate memory from this page after closing it.

About you're question. Meaby i didnt get it.
'And how much of that (in Mozilla) is image (memory) cache?' - how can i check it? 
I just compared blank window memory use with this page memory use. Is there any
diff in image cache between those two sites?

Comment 31

[Boris Zbarsky [:bzbarsky]]

about:cache will list the cache usage stats...

Comment 32

[Zibi Braniecki [:zbraniecki][:gandalf]]

Agh. Sorry. Here are my results

Clear window:
Number of entries: 	32
Maximum storage size: 	21504 k
Storage in use: 	155 k
Inactive Storage:	0 k


After loading this page:
Number of entries: 	63
Maximum storage size: 	21504 k
Storage in use: 	441 k
Inactive Storage:	94 k

My question is if we can call this bug worksforme (following Your comment #27)
and say that we're using reasonable amount of RAM if Mozilla still use ~2x more
memory on this page, and loads it 2-3 times slower?

Comment 33

[Boris Zbarsky [:bzbarsky]]

Ok, sounds like we could improve some more.  Thanks for the data.

A trace-malloc run would be most helpful here....

Comment 34

[timeless]

Comment on attachment 89760 [details] [diff] [review]
Also make nsDocLoader use a pldhash for the request info

mozilla/uriloader/base/nsDocLoader.cpp	3.257	
mozilla/uriloader/base/nsDocLoader.h	1.24

Comment 35

[Marco Castelluccio [:marco]]

Testcase starts using more or less 100 MB of memory and 100% of CPU and after a bit frees memory and goes to 40 MB of usage.
Adding to MemShrink.

Comment 36

[Boris Zbarsky [:bzbarsky]]

Hm..  I see a fair amount of heap-unclassified on that testcase...

Comment 37

[Boris Zbarsky [:bzbarsky]]

The cpu spike will be fixed in bug 642551, by the way.

Comment 38

[Nicholas Nethercote [inactive]]

I see 39.4% heap-unclassified, which is moderately high.

Comment 39

[Nicholas Nethercote [inactive]]

Attachment: DMD output

Here's the top 20 records produced by DMD.  Some of these may be covered by reporters that DMD doesn't yet know about.  bz, you have a better idea than I do at this point which DOM/CSS allocations have bugs filed to add memory reporters.

Comment 40

[Andrew McCreight [:mccr8]]

nsAttrAndChildArray is a DOM thing, and it looks like a mem reporter esque thing was implemented in Bug 667883.

Comment 41

[Boris Zbarsky [:bzbarsky]]

Looking at the list from attachment 556710 [details] in order:

1) nsTextNode should be reported already: see fixed bug 670965 
2) nsHTMLImageElement is covered by bug 682432
3) For stringbuffer I need more stack frames to see what's allocating it
4) We probably need a reporter for imgRequestProxy/imgRequest
5) nsAttrAndChildArray is reported per comment 40.
6) URIs are covered by bug 682431
7) We probably need a reporter for the pipe recycling allocator, as well as for the
   segments of live pipes!
8) Not sure why the startup cache is allocating memory when called from the JS component
   loader.  Someone familiar with the startup cache needs to look at that.
9) More startup cache stuff.
10) Similar to #4, some sort of imglib bookkeeping hashtable
11) Anchor elements; see #2.
12) nsAttrAndChildArray; see #5
13) Table row element; see #2.
14) Looks like the nsImageListener; may be worth adding a reporter for that too.
15) Table cell element; see #2.
16) String buffer; see #3.
17) Some sort of constant-overhead XPTI startup stuff.
18) String allocation; need more stack frames.
19) Textruns.  See bug 671297.
20) History link hashtable.  See bug 682437.

Comment 42

[Boris Zbarsky [:bzbarsky]]

Filed bug 683079 on item #4 and #10 above and bug 683080 on item #7.

Comment 43

[Nicholas Nethercote [inactive]]

I'm going to close this.  There are bugs filed for the unclassified stuff, and otherwise it doesn't seem like our behaviour is unreasonable.