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

RESOLVED FIXED in mozilla22

Status

()

defect
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: taras.mozilla, 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

7 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

7 years ago
Blocks: start-faster
Reporter

Comment 1

7 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

7 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
Assignee

Updated

6 years ago
Depends on: 845907
No longer depends on: 810454
Assignee

Comment 3

6 years ago
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
Assignee

Comment 4

6 years ago
Posted 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.
Reporter

Comment 7

6 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

6 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.
Assignee

Comment 10

6 years ago
Posted 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=
Assignee

Comment 13

6 years ago
Posted 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)
Reporter

Comment 14

6 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-
Assignee

Comment 15

6 years ago
Posted 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)
Reporter

Comment 16

6 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-
Assignee

Comment 17

6 years ago
Posted 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)
Reporter

Updated

6 years ago
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: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Depends on: 873640
Reporter

Updated

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