411579 - Optimize read file buffer sizes for faster startup times

Status: RESOLVED FIXED

(Core :: General, defect, P2)

Description

[Jim Mathies [:jimm]]

Based on the comments in bug 285157, we should look at optimizing read file buffer sizes used during startup. Specifically MFL_CHECKSUM_BUFSIZE in nsFastLoadFile and PREF_READ_BUFFER_SIZE in nsPrefService, both of which make repeated calls with smallish buffer sizes on large files.

Comment 1

[Jim Mathies [:jimm]]

Attachment: nsPrefService patch v.1

Pretty good savings on this one, reduced the number of calls by about 80% on new profiles. Even better results on established profiles with bigger pref files.

Comment 2

[Jim Mathies [:jimm]]

I'm working with AQtime and the timelining stuff on this now. Using timelining as a rough indicator the prefs patch actually saves us a nice little chunk of time, about 1/3 of a second on my system and fresh profile.

 

Comment 3

[Jim Mathies [:jimm]]

Attachment: read buffer tuning patch v.1

Release build w/timeline enabled gain summary - 

trunk:
00001.886 (01a63388):  Navigator Window visible now
w/patch:
00001.799 (01953388):  Navigator Window visible now

Not much, but every little bit helps I guess. 

Reviews requested  

nsPrefService / xpcom changes -> benjamin
mozJSComponentLoader changes -> bradley

Comment 4

[Jim Mathies [:jimm]]

Comment on attachment 297032 [details] [diff] [review]
read buffer tuning patch v.1

David's email in module owners was invalid, switching to sayrer for the js changes.

Comment 5

[Robert Sayre]

Comment on attachment 297032 [details] [diff] [review]
read buffer tuning patch v.1

Somewhere between 80 and 300ms is pretty good. We're looking for about 350ms to get even with the branch. 

r=me. I'm assuming you're sure that each of these changes is an improvement. Please change the XXXtuneme comments in each location to reference this bug.

Comment 6

[Brendan Eich [:brendan]]

(In reply to comment #5)
> Please change the XXXtuneme comments in each location to reference this bug.

My "XXX tuneme" fastload comment proves that XXX comments are not as effective as "FIXME: bug # (tune this macro!)" comments with a bug on file, ideally owned.

/be

Comment 7

[Jim Mathies [:jimm]]

> I'm assuming you're sure that each of these changes is an improvement.

The prefs and local buffers (as a group) were tested individually. I'll do a quick check on each of the buffer changes to confirm, and also update the comments.

Comment 8

[Jim Mathies [:jimm]]

The changes to nsFastLoadFile and mozJSComponentLoader, each save maybe 10 ms or so in a few runs using timelining as a guage. The big one is the prefs reading, that's saving us the most. Updated patch forthcoming..

Comment 9

[Jim Mathies [:jimm]]

Attachment: read buffer tuning patch v.2

code commenting updated.

Comment 10

[Mike Schroepfer]

(In reply to comment #9)
> Created an attachment (id=297103) [details]
> read buffer tuning patch v.2
> 
> code commenting updated.
> 

Are you doing another patch or you need review on this?

Comment 11

[Reed Loden [:reed]]

Checking in js/src/xpconnect/loader/mozJSComponentLoader.cpp;
/cvsroot/mozilla/js/src/xpconnect/loader/mozJSComponentLoader.cpp,v  <--  mozJSComponentLoader.cpp
new revision: 1.152; previous revision: 1.151
done
Checking in modules/libpref/src/nsPrefService.cpp;
/cvsroot/mozilla/modules/libpref/src/nsPrefService.cpp,v  <--  nsPrefService.cpp
new revision: 1.99; previous revision: 1.98
done
Checking in xpcom/io/nsFastLoadFile.cpp;
/cvsroot/mozilla/xpcom/io/nsFastLoadFile.cpp,v  <--  nsFastLoadFile.cpp
new revision: 3.47; previous revision: 3.46
done

Comment 12

[Robert Sayre]

I changed the last line of this:

> PRInt64 fileSize;
> ...
> char *fileBuffer = nsnull;
> fileBuffer = new char[nsInt64(fileSize)];

to

> fileBuffer = new char[fileSize];

to let this compile on GCC. But I confess I'm not sure why the old way didn't work.

Comment 13

[Reed Loden [:reed]]

Initial Ts numbers did not look promising, so sayrer and I decided to back this out, as kaie wanted to land the new NSS branch.

Comment 14

[Jim Mathies [:jimm]]

Since it wasn't in for long, it's hard to say.

XP:

out:
Tp:458ms
Tp2:356.95ms
Tdhtml:1129ms
Txul:397ms
Ts:1703ms 

out:
Tp:447ms
Tp2:354.2625ms
Tdhtml:1124ms
Txul:386ms
Ts:1672ms 

in
Tp:457ms
Tp2:358.625ms
Tdhtml:1119ms
Txul:387ms
Ts:1687ms 

in
Tp:471ms
Tp2:370.1125ms
Tdhtml:1128ms
Txul:392ms
Ts:1687ms 

before:
Tp:466ms
Tp2:374.15ms
Tdhtml:1131ms
Txul:387ms
Ts:1703ms 

Vista: didn't cycle fast enough

Linux:

after:
Tp:248ms
Tp2:181.375ms
Tdhtml:636ms
Txul:203ms
Ts:879ms 

in:
Tp:249ms
Tp2:180.625ms
Tdhtml:633ms
Txul:205ms
Ts:879ms 

before:
Tp:248ms
Tp2:182.075ms
Tdhtml:637ms
Txul:203ms
Ts:875ms 

MAC:

after:
Z:14.63MB
Tp:139ms
Tp2:87.85ms
Tdhtml:433ms
Txul:121ms
Ts:808ms 

in:
Z:14.70MB
Tp:135ms
Tp2:81.2375ms
Tdhtml:394ms
Txul:102ms
Ts:807ms 

before:
Z:14.70MB
Tp:140ms
Tp2:91.075ms
Tdhtml:433ms
Txul:121ms
Ts:804ms 

Comment 15

[Jim Mathies [:jimm]]

> fileBuffer = new char[nsInt64(fileSize)];

Odd, I plucked that code from another place in the tree. Regardless, it works with the changes so obvioulsy it's needed here.

Comment 16

[Mike Schroepfer]

(In reply to comment #14)
> Since it wasn't in for long, it's hard to say.
> 
> XP:
> 
> out:
> Tp:458ms
> Tp2:356.95ms
> Tdhtml:1129ms
> Txul:397ms
> Ts:1703ms 
> 
> out:
> Tp:447ms
> Tp2:354.2625ms
> Tdhtml:1124ms
> Txul:386ms
> Ts:1672ms 
> 
> in
> Tp:457ms
> Tp2:358.625ms
> Tdhtml:1119ms
> Txul:387ms
> Ts:1687ms 
> 
> in
> Tp:471ms
> Tp2:370.1125ms
> Tdhtml:1128ms
> Txul:392ms
> Ts:1687ms 
> 
> before:
> Tp:466ms
> Tp2:374.15ms
> Tdhtml:1131ms
> Txul:387ms
> Ts:1703ms 
> 
> Vista: didn't cycle fast enough
> 
> Linux:
> 
> after:
> Tp:248ms
> Tp2:181.375ms
> Tdhtml:636ms
> Txul:203ms
> Ts:879ms 
> 
> in:
> Tp:249ms
> Tp2:180.625ms
> Tdhtml:633ms
> Txul:205ms
> Ts:879ms 
> 
> before:
> Tp:248ms
> Tp2:182.075ms
> Tdhtml:637ms
> Txul:203ms
> Ts:875ms 
> 
> MAC:
> 
> after:
> Z:14.63MB
> Tp:139ms
> Tp2:87.85ms
> Tdhtml:433ms
> Txul:121ms
> Ts:808ms 
> 
> in:
> Z:14.70MB
> Tp:135ms
> Tp2:81.2375ms
> Tdhtml:394ms
> Txul:102ms
> Ts:807ms 
> 
> before:
> Z:14.70MB
> Tp:140ms
> Tp2:91.075ms
> Tdhtml:433ms
> Txul:121ms
> Ts:804ms 
> 

Do we want to try again for a longer run?

Comment 17

[Jim Mathies [:jimm]]

yes I'd like to, I need to get with reed when I have a chance and chat with him about it.

Comment 18

[Reed Loden [:reed]]

(In reply to comment #17)
> yes I'd like to, I need to get with reed when I have a chance and chat with him
> about it.

Just ping me on IRC when you have a chance.

Comment 19

[Jim Mathies [:jimm]]

Attachment: read buffer tuning patch v.3

- removed nsInt64(bufSize)

Comment 20

[Reed Loden [:reed]]

Relanded.

Checking in js/src/xpconnect/loader/mozJSComponentLoader.cpp;
/cvsroot/mozilla/js/src/xpconnect/loader/mozJSComponentLoader.cpp,v  <--  mozJSComponentLoader.cpp
new revision: 1.156; previous revision: 1.155
done
Checking in modules/libpref/src/nsPrefService.cpp;
/cvsroot/mozilla/modules/libpref/src/nsPrefService.cpp,v  <--  nsPrefService.cpp
new revision: 1.102; previous revision: 1.101
done
Checking in xpcom/io/nsFastLoadFile.cpp;
/cvsroot/mozilla/xpcom/io/nsFastLoadFile.cpp,v  <--  nsFastLoadFile.cpp
new revision: 3.49; previous revision: 3.48
done

Comment 21

[Jim Mathies [:jimm]]

Looking at graphs - vista and mac dropped a bit, linux was up on something else and was dropping so it's hard to say if this had a hand in bringing things down, and xp doesn't appear to have benefited.

Comment 22

[Reed Loden [:reed]]

Backed out due to regressions. Try again for b4?

Comment 23

[Mike Schroepfer]

(In reply to comment #22)
> Backed out due to regressions. Try again for b4?
> 

What regressions?

Comment 24

[Matthew Gregan [:kinetik]]

Bug #414905 for sure, and Mats' comments in bug #414925 for a potential problem.

Comment 25

[Mike Beltzner [:beltzner, not reading bugmail]]

Moving bugs that aren't beta 4 blockers to target final release.

Comment 26

[Jim Mathies [:jimm]]

Attachment: read buffer tuning patch v.4

Updated. I ended up using nsAutoArrayPtr in the prefs read loop for the best performance.

Comment 27

[Benjamin Smedberg]

Comment on attachment 310634 [details] [diff] [review]
 read buffer tuning patch v.4

Note bug 421561 which asked for a 15-second fastload delay, but 10 works for now!

Comment 28

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 310634 [details] [diff] [review]
 read buffer tuning patch v.4

Tests?

Comment 29

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 310634 [details] [diff] [review]
 read buffer tuning patch v.4

Tests?

Comment 30

[Jim Mathies [:jimm]]

I've been looking at this trying to figure out what would constitute a good test. The main change here relates to loading preference files so my guess is if this was broken, all kinds of things would break in our existing tests. I'm not sure what type of additional test I should add. Curious if anybody tuned in to the bug has any suggestions..

Comment 31

[Mike Shaver (:shaver emeritus)]

(I believe what beltzner meant was: "Tests?")

Maybe testing with a preference file where the length is BUFSZ, BUFSZ-1, BUFSZ+1, and where the buffer boundary is in the middle of a preference string?

Comment 32

[Jim Mathies [:jimm]]

> Maybe testing with a preference file where the length is BUFSZ, BUFSZ-1,
> BUFSZ+1, and where the buffer boundary is in the middle of a preference string?

I don't think so, the point of the patch was to allocate a buffer the exact size of the file and the read / parse the the whole thing in one shot. (BUFSZ doesn't exist anymore.) Looking at the prefs service - 

http://www.xulplanet.com/references/xpcomref/comps/c_preferencesservice1.html

All I really have to work with here is void readUserPrefs ( nsIFile file ). Am I missing something?

Comment 33

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 310634 [details] [diff] [review]
 read buffer tuning patch v.4

a=beltzner

Comment 34

[Reed Loden [:reed]]

Checking in js/src/xpconnect/loader/mozJSComponentLoader.cpp;
/cvsroot/mozilla/js/src/xpconnect/loader/mozJSComponentLoader.cpp,v  <--  mozJSComponentLoader.cpp
new revision: 1.160; previous revision: 1.159
done
Checking in modules/libpref/src/nsPrefService.cpp;
/cvsroot/mozilla/modules/libpref/src/nsPrefService.cpp,v  <--  nsPrefService.cpp
new revision: 1.104; previous revision: 1.103
done
Checking in xpcom/io/nsFastLoadFile.cpp;
/cvsroot/mozilla/xpcom/io/nsFastLoadFile.cpp,v  <--  nsFastLoadFile.cpp
new revision: 3.52; previous revision: 3.51
done