Closed Bug 411579 Opened 12 years ago Closed 12 years ago

Optimize read file buffer sizes for faster startup times

Categories

(Core :: General, defect, P2)

x86
All
defect

Tracking

()

RESOLVED FIXED
mozilla1.9

People

(Reporter: jimm, Assigned: jimm)

References

Details

(Keywords: perf)

Attachments

(1 file, 4 obsolete files)

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.
Assignee: jmathies → nobody
Component: File Handling → General
Product: Firefox → Core
QA Contact: file.handling → general
Blocks: 397947
Flags: blocking1.9+
Priority: -- → P2
Attached patch nsPrefService patch v.1 (obsolete) — Splinter Review
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.
Attachment #296364 - Flags: review?(benjamin)
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.

 
Attached patch read buffer tuning patch v.1 (obsolete) — Splinter Review
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
Attachment #296364 - Attachment is obsolete: true
Attachment #297032 - Flags: review?(benjamin)
Attachment #296364 - Flags: review?(benjamin)
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.
Attachment #297032 - Flags: review?(sayrer)
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.
Attachment #297032 - Flags: review?(sayrer) → review+
Assignee: nobody → jmathies
(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
> 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.
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..
Attached patch read buffer tuning patch v.2 (obsolete) — Splinter Review
code commenting updated.
Attachment #297103 - Attachment is patch: true
(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?
Attachment #297032 - Flags: review?(benjamin)
Attachment #297103 - Flags: superreview?(benjamin)
Attachment #297103 - Flags: superreview?(benjamin) → superreview+
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
Status: NEW → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9 M11
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.
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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 
> 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.
(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?
yes I'd like to, I need to get with reed when I have a chance and chat with him about it.
(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.
Keywords: checkin-needed
Attached patch read buffer tuning patch v.3 (obsolete) — Splinter Review
- removed nsInt64(bufSize)
Attachment #297032 - Attachment is obsolete: true
Attachment #297103 - Attachment is obsolete: true
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
Keywords: checkin-needed
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.
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Depends on: 414925
Depends on: 414905
Backed out due to regressions. Try again for b4?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla1.9beta3 → mozilla1.9beta4
(In reply to comment #22)
> Backed out due to regressions. Try again for b4?
> 

What regressions?
Bug #414905 for sure, and Mats' comments in bug #414925 for a potential problem.
Moving bugs that aren't beta 4 blockers to target final release.
Target Milestone: mozilla1.9beta4 → mozilla1.9
Flags: tracking1.9+ → wanted-next+
Updated. I ended up using nsAutoArrayPtr in the prefs read loop for the best performance.
Attachment #298835 - Attachment is obsolete: true
Attachment #310634 - Flags: superreview?(benjamin)
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!
Attachment #310634 - Flags: superreview?(benjamin) → superreview+
Attachment #310634 - Flags: approval1.9b5?
Attachment #310634 - Flags: approval1.9?
Comment on attachment 310634 [details] [diff] [review]
 read buffer tuning patch v.4

Tests?
Attachment #310634 - Flags: approval1.9b5? → approval1.9b5-
Comment on attachment 310634 [details] [diff] [review]
 read buffer tuning patch v.4

Tests?
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..
(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?
> 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 on attachment 310634 [details] [diff] [review]
 read buffer tuning patch v.4

a=beltzner
Attachment #310634 - Flags: approval1.9? → approval1.9+
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
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.