Closed Bug 810151 Opened 8 years ago Closed 8 years ago

Use readahead for ordered jar files such as omni.ja. Should be ~10% startup speedup

Categories

(Core :: Networking: JAR, defect)

x86_64
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: taras.mozilla, Assigned: aklotz)

References

(Blocks 1 open bug)

Details

(Whiteboard: c=startup_io u= p=)

Attachments

(1 file, 4 obsolete files)

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.
Blocks: start-faster
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.
(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)
Depends on: 810454
Depends on: 845907
No longer depends on: 810454
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
Assignee: nobody → aklotz
Attached patch Proposed patch (obsolete) — Splinter Review
Attachment #721529 - Flags: review?(taras.mozilla)
huh, why not use bug 845907 ?
(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 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.
Attachment #721529 - Flags: review?(taras.mozilla) → review+
(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.
(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.
Attached patch Patch v2 (obsolete) — Splinter Review
Fixed nits, carrying forward r+
Attachment #721529 - Attachment is obsolete: true
Attachment #721803 - Flags: review+
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).
Whiteboard: c=startup_io u= p=
Attached patch Patch v3 (obsolete) — Splinter Review
xpcshell errors were caused by sharing violations due to the fd being kept open on Windows.
Attachment #721803 - Attachment is obsolete: true
Attachment #724015 - Flags: review?(taras.mozilla)
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.
Attachment #724015 - Flags: review?(taras.mozilla) → review-
Attached patch Patch v4 (obsolete) — Splinter Review
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.
Attachment #724015 - Attachment is obsolete: true
Attachment #725463 - Flags: review?(taras.mozilla)
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
Attachment #725463 - Flags: review?(taras.mozilla) → review-
Attached patch Patch v5Splinter Review
This version removes the instance variable and passes the fd via parameter as discussed.
Attachment #725463 - Attachment is obsolete: true
Attachment #727255 - Flags: review?(taras.mozilla)
Attachment #727255 - Flags: review?(taras.mozilla) → review+
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)
https://hg.mozilla.org/mozilla-central/rev/f8e71f41c30f
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Depends on: 873640
Duplicate of this bug: 613124
You need to log in before you can comment on or make changes to this bug.