Last Comment Bug 810151 - Use readahead for ordered jar files such as omni.ja. Should be ~10% startup speedup
: Use readahead for ordered jar files such as omni.ja. Should be ~10% startup s...
Status: RESOLVED FIXED
c=startup_io u= p=
:
Product: Core
Classification: Components
Component: Networking: JAR (show other bugs)
: unspecified
: x86_64 Windows 7
: -- normal with 1 vote (vote)
: mozilla22
Assigned To: Aaron Klotz [:aklotz]
:
: Patrick McManus [:mcmanus]
Mentors:
: 613124 (view as bug list)
Depends on: 845907 873640
Blocks: start-faster
  Show dependency treegraph
 
Reported: 2012-11-08 17:11 PST by (dormant account)
Modified: 2013-10-28 09:36 PDT (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Proposed patch (4.14 KB, patch)
2013-03-05 16:24 PST, Aaron Klotz [:aklotz]
taras.mozilla: review+
Details | Diff | Splinter Review
Patch v2 (4.14 KB, patch)
2013-03-06 10:53 PST, Aaron Klotz [:aklotz]
aklotz: review+
Details | Diff | Splinter Review
Patch v3 (4.94 KB, patch)
2013-03-12 10:32 PDT, Aaron Klotz [:aklotz]
taras.mozilla: review-
Details | Diff | Splinter Review
Patch v4 (4.31 KB, patch)
2013-03-15 10:16 PDT, Aaron Klotz [:aklotz]
taras.mozilla: review-
Details | Diff | Splinter Review
Patch v5 (6.51 KB, patch)
2013-03-20 10:00 PDT, Aaron Klotz [:aklotz]
taras.mozilla: review+
Details | Diff | Splinter Review

Description User image (dormant account) 2012-11-08 17:11:14 PST
Need to do something similar to http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/standalone/nsGlueLinkingWin.cpp#42. I thought I already had a bug filed on this.
Comment 1 User image (dormant account) 2013-01-25 12:27:52 PST
If I remember correctly, the first 4bytes of optimized jars contain length of ordered data in jar.

So if one adds a clever call to readahead() to http://mxr.mozilla.org/mozilla-central/source/modules/libjar/nsZipArchive.cpp#526, things should get a lot better.
Comment 2 User image (dormant account) 2013-01-25 12:30:26 PST
(In reply to Taras Glek (:taras) from comment #1)
> So if one adds a clever call to readahead() to
> http://mxr.mozilla.org/mozilla-central/source/modules/libjar/nsZipArchive.
> cpp#526, things should get a lot better.

on linux one would use madvise(willneed)
Comment 3 User image Aaron Klotz [:aklotz] 2013-03-05 16:18:58 PST
xperf startup tests on Windows 7, cold startup, no SSD.

Average omni.ja read time, PGO: 228.4ms
Average omni.ja read time, PGO + ordered jar readahead: 91.0ms
Comment 4 User image Aaron Klotz [:aklotz] 2013-03-05 16:24:09 PST
Created attachment 721529 [details] [diff] [review]
Proposed patch
Comment 5 User image Mike Hommey [:glandium] 2013-03-05 16:30:04 PST
huh, why not use bug 845907 ?
Comment 6 User image Mike Hommey [:glandium] 2013-03-05 16:32:48 PST
(In reply to Mike Hommey [:glandium] from comment #5)
> huh, why not use bug 845907 ?

erf, looks like it's too late for me, it is being used... on windows only. I think you could use it on unix as well.
Comment 7 User image (dormant account) 2013-03-06 09:38:35 PST
Comment on attachment 721529 [details] [diff] [review]
Proposed patch

Sorry for lag. This came out slightly cleaner than I expected. Good work.

I like to reduce preprocessor ugly and rely on compiler when possible.

+#if defined(XP_WIN)
+#define OPEN_FLAGS PR_RDONLY | nsIFile::OS_READAHEAD
+#else
+#define OPEN_FLAGS PR_RDONLY
+#endif

do 
flags = bla
if (XP_WIN(or something)) {
flags |= nsIFile::OS_READAHEAD
}

I think in general #if defined() is preferred to #ifdef. It's not consistent already, so I don't care.
Comment 8 User image (dormant account) 2013-03-06 09:39:44 PST
(In reply to Mike Hommey [:glandium] from comment #6)
> (In reply to Mike Hommey [:glandium] from comment #5)
> > huh, why not use bug 845907 ?
> 
> erf, looks like it's too late for me, it is being used... on windows only. I
> think you could use it on unix as well.

On unix this patch avoids keeping an extra filehandle. If we ever decide to keep the filehandle around on all platforms. This might make sense.
Comment 9 User image Mike Hommey [:glandium] 2013-03-06 09:53:32 PST
(In reply to Taras Glek (:taras) from comment #8)
> (In reply to Mike Hommey [:glandium] from comment #6)
> > (In reply to Mike Hommey [:glandium] from comment #5)
> > > huh, why not use bug 845907 ?
> > 
> > erf, looks like it's too late for me, it is being used... on windows only. I
> > think you could use it on unix as well.
> 
> On unix this patch avoids keeping an extra filehandle. If we ever decide to
> keep the filehandle around on all platforms. This might make sense.

Ah, makes sense.
Comment 10 User image Aaron Klotz [:aklotz] 2013-03-06 10:53:44 PST
Created attachment 721803 [details] [diff] [review]
Patch v2

Fixed nits, carrying forward r+
Comment 12 User image Matt Brubeck (:mbrubeck) 2013-03-06 18:55:13 PST
Sorry, I backed this out on inbound because of Windows xpcshell test failures that look related to this patch.
https://hg.mozilla.org/integration/mozilla-inbound/rev/acec25f67366

Example failure:
https://tbpl.mozilla.org/php/getParsedLog.php?id=20399662&tree=Mozilla-Inbound

I will reland this patch if the backout does not solve the failures (or you can re-land it if you get to it before I do).
Comment 13 User image Aaron Klotz [:aklotz] 2013-03-12 10:32:31 PDT
Created attachment 724015 [details] [diff] [review]
Patch v3

xpcshell errors were caused by sharing violations due to the fd being kept open on Windows.
Comment 14 User image (dormant account) 2013-03-12 18:05:28 PDT
Comment on attachment 724015 [details] [diff] [review]
Patch v3

Good try.

Use AutoFDClose instead of a nspr handle. Then in BuildFileList have local autofdclose that forget()s the class one and assigns it back on success.

This avoids adding a :CloseFd and keeps it in sync better.
Comment 15 User image Aaron Klotz [:aklotz] 2013-03-15 10:16:57 PDT
Created attachment 725463 [details] [diff] [review]
Patch v4

I've switched over to AutoFDClose and call dispose() when nsZipArchive::BuildFileList is about to return success. To fix the test bustage we can't keep the fd open beyond that point.
Comment 16 User image (dormant account) 2013-03-18 09:21:46 PDT
Comment on attachment 725463 [details] [diff] [review]
Patch v4

+#if defined(XP_WIN)
+  , mFd(nullptr)
+#endif

This is redundant.

This patch is still not quite correct. This will hog a handle if BuildFileList fails.

You want to do mozilla::AutoFDClose fd = AutomFd->mFd.dispose() on top of buildfilelist then use the local handle for readahead
Comment 17 User image Aaron Klotz [:aklotz] 2013-03-20 10:00:21 PDT
Created attachment 727255 [details] [diff] [review]
Patch v5

This version removes the instance variable and passes the fd via parameter as discussed.
Comment 19 User image Landry Breuil (:gaston) 2013-03-21 02:34:36 PDT
Will keep an eye on this and testbuild m-i, but iirc madvise isn't supported/doesnt work on OpenBSD (see https://bugzilla.mozilla.org/show_bug.cgi?id=632404#c13)
Comment 20 User image Ryan VanderMeulen [:RyanVM] 2013-03-21 14:11:10 PDT
https://hg.mozilla.org/mozilla-central/rev/f8e71f41c30f
Comment 21 User image (dormant account) 2013-10-28 09:36:04 PDT
*** Bug 613124 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.