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

RESOLVED FIXED in mozilla22

Status

()

Core
Networking: JAR
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: (dormant account), Assigned: aklotz)

Tracking

(Blocks: 1 bug)

unspecified
mozilla22
x86_64
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: c=startup_io u= p=)

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

5 years ago
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.
(Reporter)

Updated

5 years ago
Blocks: 810156
(Reporter)

Comment 1

4 years ago
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.
(Reporter)

Comment 2

4 years ago
(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
Created attachment 721529 [details] [diff] [review]
Proposed patch
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.
(Reporter)

Comment 7

4 years ago
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+
(Reporter)

Comment 8

4 years ago
(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.
Created attachment 721803 [details] [diff] [review]
Patch v2

Fixed nits, carrying forward r+
Attachment #721529 - Attachment is obsolete: true
Attachment #721803 - Flags: review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f6c38239328
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=
Created attachment 724015 [details] [diff] [review]
Patch v3

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)
(Reporter)

Comment 14

4 years ago
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-
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.
Attachment #724015 - Attachment is obsolete: true
Attachment #725463 - Flags: review?(taras.mozilla)
(Reporter)

Comment 16

4 years ago
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-
Created attachment 727255 [details] [diff] [review]
Patch v5

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)
(Reporter)

Updated

4 years ago
Attachment #727255 - Flags: review?(taras.mozilla) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/f8e71f41c30f

Try:
https://tbpl.mozilla.org/?tree=Try&rev=674516f57c40
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
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Depends on: 873640
(Reporter)

Updated

4 years ago
Duplicate of this bug: 613124
You need to log in before you can comment on or make changes to this bug.