113163 - Performance problem at downloading

Status: VERIFIED FIXED

(Core Graveyard :: File Handling, defect, P3)

Description

[Andi Kleen]

When downloading big binary files (50+MB) at DSL speed (40-80kb/s) mozilla becomes
very slow and eats a lot of CPU time (upto 90% on a 900mhz athlon)
Some profiling showed that nsStorageTransport::AppendSegment eat a lot 
of CPU time walking segment lists.
The attached patch fixes the segment list to have a tail pointer, which makes
mozilla use a lot less (but still too much) CPU time during download. It 
is now at  about 30% of CPU during downloading a random big file from the network.
I attached also a profile log generated by oprofile after the
nsStorageTransport::AppendSegment O(1) patch was added, with 30% CPU usage.

It shows that a lot of time is spent in memcpy() and 
NSPR_GetCurrentThread() and accessing of thread local pthread data. 
(the search_by_key showing up in the kernel is in reiserfs and does not seem to
be directly caused by mozilla) A little bit behind comes nsCachedStyleData(),
GetStyleData(). I suspect the are caused by updating the progress window,
because the downloaded data should not require any style processing. This
updating probably could be optimized too. For more details see the attached
profile log.

Comment 1

[Andi Kleen]

Attachment: patch to fix nsStorageTransport performance problem

Comment 2

[Andi Kleen]

Attachment: oprofile log from mozilla downloading files with previous patch

Here is the oprofile log. It shows some other candidates for speeding
up downloading (e.g. thread handling and the updating of the progress
window)

Comment 3

[Bradley Baetz (:bbaetz)]

darin, law: This yours?

We have other bugs on the download window updating too often, and I know I saw a
bug in the last few days about downloads being effectivly throtteld, but I cna't
find it now.

Comment 4

[Daniel Bratell]

I don't know enough of this to do a review but two comments:

     if (!mSegments) {
         mSegments = aSegment;
         mSegmentsLast = aSegment;
     } else {
         mSegmentsLast->next = aSegment;
         mSegmentsLast = aSegment; 
     }

Move mSegmentsLast out of the if?

     nsSegment      *mSegments;
+    nsSegment 	   *mSegmentsLast; 

     nsSegment      *mWriteSegment;

Align this like the other?

Comment 5

[Darin Fisher]

the real problem here is that we shouldn't be storing such large files in the
memory cache.  something needs to be regulating this.

that being said, i do like the sound of the nsStorageTransport patch :)

-> me

Comment 6

[[not reading bugmail]]

looks like a dupe of bug 91232

Comment 7

[Darin Fisher]

gordon: we have other bugs about not putting such large stuff in the memory
cache, right?

Comment 8

[gordon]

How are downloaded files getting in the memory cache?

Comment 9

[Andi Kleen]

Attachment: Optimize java script of download progress windw

With this patch and the O(1) append patch in the same bug I have
mozilla eating around 6-10% of my CPU during downloading.

Bas
ically it tries to do everything related to window updating as
lazily as possible. 

It is a kind of an hack; it would be better to add some way to whoever calls 
onProgress() to call it a bit less often. 

After that patch the far most prominent offenders in the profile log are
PR_GetCurrentThread/NS_CurrentThread(). One way to fix them at least
on Linux would be to use user stacks in pthreads (requires new glibc),
allocate the stacks privately and put the nspr thread data onto the bottom
of an aligned stack. Then PR_GetCurrentThread could be a simple and with
the stack pointer and shrink to ~three instructions. 

Second offender in a profile of a download (in order of prominence) 
libgkcontent.so RETIRED_INSNS nsCachedStyleData::GetStyleData(nsStyleStructID
co
Not sure how to fix that.

malloc() 

NS_CheckThreadSafe()  (may be because I'm using a debugging build.
At least it should probably cache the environment variable)

js_Interpret() - probably needs a more efficient onProgress() handler to fix

convertTrueColor_generic() from libwraster() 
No idea why it needs that for a download

PR_Unlock

index() from libc. Something doing too many string operations. Could 
be the getenv() from NS_CheckThreadSafe()

Comment 10

[Andi Kleen]

Attachment: Optimize java script of download progress windw

With this patch and the O(1) append patch in the same bug I have
mozilla eating around 6-10% of my CPU during downloading.

Bas
ically it tries to do everything related to window updating as
lazily as possible. 

It is a kind of an hack; it would be better to add some way to whoever calls 
onProgress() to call it a bit less often. 

After that patch the far most prominent offenders in the profile log are
PR_GetCurrentThread/NS_CurrentThread(). One way to fix them at least
on Linux would be to use user stacks in pthreads (requires new glibc),
allocate the stacks privately and put the nspr thread data onto the bottom
of an aligned stack. Then PR_GetCurrentThread could be a simple and with
the stack pointer and shrink to ~three instructions. 

Second offender in a profile of a download (in order of prominence) 
libgkcontent.so RETIRED_INSNS nsCachedStyleData::GetStyleData(nsStyleStructID
co
Not sure how to fix that.

malloc() 

NS_CheckThreadSafe()  (may be because I'm using a debugging build.
At least it should probably cache the environment variable)

js_Interpret() - probably needs a more efficient onProgress() handler to fix

convertTrueColor_generic() from libwraster() 
No idea why it needs that for a download

PR_Unlock

index() from libc. Something doing too many string operations. Could 
be the getenv() from NS_CheckThreadSafe()

Comment 11

[Darin Fisher]

gordon: i would assume that data is going into the memory cache because the user
has set their disk cache to zero size.

Comment 12

[gordon]

I didn't see where the report said the user had set their disk cache to zero...

Comment 13

[Darin Fisher]

reporter: what URL are you loading that demonstrates this problem?  what are
your cache preferences?

Comment 14

[Andi Kleen]

Indeed my disk cache size is set to 0.
(doubt that even a 20MB disk cache would have helped with a 400MB download though)

I downloaded from several different URLs (mostly private) They all showed the
same CPU time eating.

Comment 15

[[not reading bugmail]]

will this patch/change be at all useful after the download manager suppose to land?

Comment 16

[Andi Kleen]

Unless the download manager contains a new cache implementation the patch
to fix the memory cache list insertion should still apply.

The javascript optimization may not anymore.

Comment 17

[Bill Law]

The .js file in the patch isn't used.  The code you want is in
mozilla/embedding/components/ui/progressDlg/nsProgressDlg.js, I think.

Comment 18

[Mike Shaver (:shaver emeritus)]

Darin? Netwerk buddies? This looks like a slam-dunk here, and while there's been
a lot of commentary about the cache settings, nobody but Daniel has said word
one about the patch itself.  Are there changes needed, other than the ones
Daniel has suggested?

(Don't like to see free performance patches rotting on the vine for weeks at a
time, but I'm funny that way.)

Comment 19

[Darin Fisher]

Comment on attachment 60097 [details] [diff] [review]
patch to fix nsStorageTransport performance problem

>Index: nsStorageTransport.h

>     nsSegment      *mSegments;
>+    nsSegment 	   *mSegmentsLast; 

patch looks good.  indentation problem here, otherwise sr=darin.

Comment 20

[Darin Fisher]

i can take care of landing the nsStorageTransport changes.  for the JS changes,
i think i'm going to reassign this bug to xpapps.

Comment 21

[gordon]

Comment on attachment 60097 [details] [diff] [review]
patch to fix nsStorageTransport performance problem

r=gordon if the changes from comment #4 are factored in.

Comment 22

[Darin Fisher]

gordon: agreed... thx for the code review.  i'll check this in once the tree opens.

Comment 23

[Darin Fisher]

checked in attachment 60097 [details] [diff] [review].

reassigning to xpapps for the JS optimizations.

Comment 24

[Peter Trudelle]

->file handling

Comment 25

[Bill Law]

The JS patches are to files that are no longer used.

There are differences between the patched file and the progress dialog that is
currently in use:
http://lxr.mozilla.org/seamonkey/source/embedding/components/ui/progressDlg/nsProgressDlg.js#80

I don't think that the changes in the patch apply to the current code.

I'm marking this fixed, to account for the back-end changes having gone in.

Comment 26

[Mike Shaver (:shaver emeritus)]

So are you saying that the problem that the JS changes addressed is no longer
present, or are you just closing this bug because the patch doesn't apply? 
(IOW: are window updates throttled?)

(Neither the reporter nor the Keeper of the Back-End thought it was finished
when the back-end changes went in, which is an interesting datapoint.)

Comment 27

[Andi Kleen]

(as the original patch creator) It's fine for me to drop the javascript
change. It was only a **** attempt at my part to make some of
the top profile log contenders faster, but I don't think it made 
that much difference anyways and I posted it just for completeness. 

Comment 28

[Bill Law]

Sorry, Mike.  My use of "apply" was ambiguous.  I meant that the changes
wouldn't appear to have any performance impact in context of the current
implementation.  I'm not sure how much difference they made in the old
implementation, as Andi seems to confirm.

Comment 29

[sairuh (rarely reading bugmail)]

rs vrfy. more recent issues should be filed as new bugs...