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.

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)

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

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.

(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.

Fixed nits, carrying forward r+

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).

xpcshell errors were caused by sharing violations due to the fd being kept open on Windows.

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.

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

This version removes the instance variable and passes the fd via parameter as discussed.

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)