mozilla is much slower then IE6 ! and uses 90MB

RESOLVED FIXED

Status

()

Core
Networking
RESOLVED FIXED
16 years ago
5 years ago

People

(Reporter: DK, Unassigned)

Tracking

(Depends on: 3 bugs, {perf, testcase})

Trunk
perf, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink], URL)

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

16 years ago
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

16 years ago
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.
(Reporter)

Comment 2

16 years ago
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

15 years ago
Created attachment 79794 [details]
Testcase
Attachment #79794 - Attachment mime type: application/octet-stream → application/zip
Created attachment 79805 [details]
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.
Not sure whom this should go to...  To networking as a first guess, but this
could be imagelib or docshell...
Assignee: asa → new-network-bugs
Status: UNCONFIRMED → NEW
Component: Browser-General → Networking
Ever confirmed: true
Keywords: perf, testcase
OS: Windows 2000 → All
QA Contact: doron → benc
Hardware: PC → All

Comment 6

15 years ago
(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

15 years ago
Are we still doing so badly on this one?
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

15 years ago
+helpwanted, cc'd neeti.

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

Comment 10

15 years ago
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.
Assignee: new-network-bugs → dp

Comment 11

15 years ago
My Observation: 
I noticed IE progressively downloads and Gecko doesnt. 2002050807, Win2k
Created attachment 89661 [details] [diff] [review]
This could help here... Make nsLoadGroup use a pldhash in stead of a linear array

Updated

15 years ago
Keywords: helpwanted → mozilla1.1, patch
Created attachment 89760 [details] [diff] [review]
Also make nsDocLoader use a pldhash for the request info
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 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
Attachment #89760 - Flags: review+

Comment 16

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

sr=rpotts@netscape.com
Attachment #89760 - Flags: superreview+

Comment 17

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

sr=rpotts@netscape.com
Attachment #89661 - Flags: superreview+
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 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.
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...
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 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.
Attachment #89661 - Flags: review+
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?
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.
> 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?
33 additional MB of course for both.
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....
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.
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).
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?
about:cache will list the cache usage stats...
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?
Ok, sounds like we could improve some more.  Thanks for the data.

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

Comment 34

14 years ago
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
Attachment #89760 - Attachment is obsolete: true

Updated

14 years ago
Attachment #89661 - Attachment is obsolete: true
Assignee: dp → nobody
QA Contact: benc → networking
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.
Whiteboard: [MemShrink]
Hm..  I see a fair amount of heap-unclassified on that testcase...
The cpu spike will be fixed in bug 642551, by the way.
I see 39.4% heap-unclassified, which is moderately high.
Depends on: 676724
Created attachment 556710 [details]
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.
nsAttrAndChildArray is a DOM thing, and it looks like a mem reporter esque thing was implemented in Bug 667883.
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.
Depends on: 682437, 671297, 682432, 682431
Depends on: 683079
Depends on: 683080
Filed bug 683079 on item #4 and #10 above and bug 683080 on item #7.
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.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.