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]
:
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 (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 (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 (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 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 Aaron Klotz [:aklotz] 2013-03-05 16:24:09 PST
Created attachment 721529 [details] [diff] [review]
Proposed patch
Comment 5 Mike Hommey [:glandium] 2013-03-05 16:30:04 PST
huh, why not use bug 845907 ?
Comment 6 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 (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 (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 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 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 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 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 (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 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 (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 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 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 Ryan VanderMeulen [:RyanVM] 2013-03-21 14:11:10 PDT
https://hg.mozilla.org/mozilla-central/rev/f8e71f41c30f
Comment 21 (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.