Currently jar io is super-inefficient. for every file within the jar file we stat the jar file, REopen the jar file and read it in. It's much more efficient to memory map the file once and memcpy afterwards.

This one does really cheesy readahead(mainly testing whether it's a win..it is)

What happens if there is no enough address space to map the entire file?

(In reply to comment #2) > What happens if there is no enough address space to map the entire file? then you are screwed. People should know better than to open multigb jar files on 32bit :) On a more serious note, I dunno, there a couple of things that can be done in that case(fallback to normal io, etc)

Who's a potential reviewer, here?

(In reply to comment #5) > Who's a potential reviewer, here? Benjamin. I'll submit a patch for review on monday.

I didn't change the ExtractFile stuff to use mmap io.

hg chewed up my previous patch

Comment on attachment 390969 [details] [diff] [review] mmap io >diff --git a/modules/libjar/nsZipArchive.cpp b/modules/libjar/nsZipArchive.cpp >+#include "private/pprio.h" >+#include "prerror.h" Why did you need pprio.h? I think we'd like to avoid needing private NSPR headers. >@@ -224,6 +234,19 @@ nsresult nsZipArchive::OpenArchive(PRFil > //-- Keep the filedescriptor for further reading... > mFd = fd; > >+ mLen = PR_Seek(fd, 0, PR_SEEK_END); >+ NS_ENSURE_TRUE(mLen > -1, NS_ERROR_FAILURE); I'm not a fan of the NS_ENSURE_ macros because they hide returns. Please use bare ifs in all new code. >+ int pos = PR_Seek(fd, 0, PR_SEEK_SET); >+ NS_ENSURE_TRUE(pos == 0, NS_ERROR_FAILURE); `int` is not right, and I think we should be a bit more defensive about really large files: how about PROffset64 mLen = PR_Seek64(...); if (mLen >= 0x8FFFFFFF) return NS_ERROR_FILE_TOO_BIG; I don't see anywhere in this patch where we call PR_MemUnmap or PR_CloseFileMap...

This removes excessive includes, gets rid of seeking in favour of PR_Available64 and makes sure that the size of the file is smaller than that of a signed 32bit int since a bunch of the code uses 32bit ints to seek around within the file. I also readded the unmmap stuff which seems to have gotten chewed away during my hg troubles :(. Thanks for spotting the bustage.

sorry for spam, here is the real patch

Comment on attachment 391386 [details] [diff] [review] mmap This looks good. I'm still a bit worried about how libjar has very few tests (and they are mostly zipwriter tests), so I've asked taras to come up with some ownership tests... in particular, if you hold a JAR stream after releasing the ziparchive, we need to make sure the stream stays valid.

Benjamin, I'm puzzled as to how to proceed. The above patch fixes the unsafety you spotted... However there is a bigger problem calling .close() on a zipreader currently doesn't not invalidate the inputstream, but with this patch it does. I'm not sure about a correct way to address that. a) Make nsZipArchive itself ref counted and make close() a no-op when refcount > 1 b) break out the read()+fd-owning part from nsZipArchive into a separate ZipReader class. This might also make it easier to provide a non-mmap fallback. I *think* b) is the correct way to go, but it bugs me that we already have nsJar: an annoying wrapper around nsZipArchive nsZipArchive: would now become a wrapper around the ZipReader class. I think the correctest solution would be to merge nsZipArchive/nsJAR and introduce a ZipReader of sorts, but that seems like way too much work. Thoughts?

Decided to bite the bullet and do refcounting on file handle stuff in order to keep behavior same as before. I think the abstractions I added should make it fairly easy to add non-mmap io. The only thing I'm not sure about the refcounting parts in nsZipHandle. It seems adding/removing fileinputstreams only happens on one thread, but reading happens on separate ones, so this should be ok as is. I'm also not sure what the heck is accomplished by setting refcount to 1 just before delete, there is lots of "stabilize" comments next to that sort of stuff elsehwere in moz, but no explanation as to what it accomplishes.

I believe the rationale is that it is extremely bad form to call methods on an object with a refcount of 0, so if the destructor calls any methods on |this| that stylistic error would occur (conceivably with bad results if the method actively requires a non-0 refcount, although such cases are probably buggy already even without stabilization). It may be provably not necessary on a case-by-case basis, but easier just to do the same thing everywhere than think too hard about it.

Pre-mortem finalization must stabilize the refcount or other "alive" mark. There is no bug in stabilizing, and I agree with Waldo that there's a bug (stylistic or real) in not stabilizing. /be

Comment on attachment 392301 [details] [diff] [review] fancy patch >diff --git a/modules/libjar/nsJARInputStream.cpp b/modules/libjar/nsJARInputStream.cpp >-nsJARInputStream::InitFile(nsZipArchive* aZip, nsZipItem *item, PRFileDesc *fd) >+nsJARInputStream::InitFile(nsZipHandle *aFd, nsZipItem *item) >+ PR_ASSERT(aFd); >+ PR_ASSERT(item); Use NS_ABORT_IF_FALSE >@@ -220,28 +215,24 @@ nsJARInputStream::Read(char* aBuffer, PR >- PR_Close(mFd); >- mFd = nsnull; >+ mFd.Close(); > return NS_ERROR_FILE_CORRUPTED; It took some reading of nsSeekableZipHandle to figure out that it was ok to .Close mFd twice (it will be closed again at nsJARInputStream::Close). A comment here might be good, as well as at nsSeekableZipHandle::Close >diff --git a/modules/libjar/nsZipArchive.cpp b/modules/libjar/nsZipArchive.cpp >+nsZipHandle::nsZipHandle():mFd(NULL), mFileData(NULL), mLen(0), mMap(NULL), mRefCnt(1) { style nit, use nsnull instead of NULL and fix the indentation thusly: nsZipHandle::nsZipHandle() : mFd(nsnull) , mFileData(nsnull) , etc... { } >+nsresult nsZipHandle::Init(PRFileDesc *fd, nsZipHandle **ret) { style nit here and elsewhere, the opening brace of a function goes on the following line > nsresult nsZipArchive::OpenArchive(PRFileDesc * fd) >+ if (!NS_SUCCEEDED(rv)) use NS_FAILED >diff --git a/modules/libjar/nsZipArchive.h b/modules/libjar/nsZipArchive.h >+ static nsresult Init(PRFileDesc *fd, nsZipHandle **ret); use NS_OUTPARAM >+ nsrefcnt AddRef(void) { >+ return ++mRefCnt; >+ } >+ >+ nsrefcnt Release(void) { >+ --mRefCnt; >+ if (mRefCnt == 0) { >+ mRefCnt = 1; // stabilize >+ delete this; >+ } >+ return mRefCnt; >+ } This class omits a lot of the debug refcounting magic, in particular the trace-refcount logging macros as well as the threadsafety enforcement. I'd like those both added. nit, trailing whitespace >+protected: >+ PRFileDesc *mFd; // OS file-descriptor >+ PRUint8 *mFileData; // pointer to mmaped file >+ PRUint32 mLen; // length of file and memory mapped area odd whitespace, either line them up or use single spaces >+/** nsSeekableZipHandle acts as a container for nsZipHandle, >+ emulates sequential file io */ >+class nsSeekableZipHandle { style nit, braces to start a class go on the following line r=me with nits picked: feel free to ask for a final review if you have questions

Comment on attachment 393021 [details] [diff] [review] fixup patch to checkin >diff --git a/modules/libjar/nsZipArchive.cpp b/modules/libjar/nsZipArchive.cpp >+nsZipHandle::nsZipHandle(): >+ mFd(nsnull), >+ mFileData(nsnull), >+ mLen(0), >+ mMap(nsnull), >+ mRefCnt(1) >+{ >+} That's not quite the requested formatting... >+ nsZipHandle *handle = new nsZipHandle(); >+ handle->mFd = fd; Missing an OOM check here, don't we still have a smidgen more time until the mythical time when OOM checks are removable? >+ handle->mFileData = (PRUint8*) PR_MemMap(map, 0, handle->mLen); I assume the same thing here. >diff --git a/modules/libjar/nsZipArchive.h b/modules/libjar/nsZipArchive.h >+class nsZipHandle { >+friend class nsZipArchive; >+friend class nsSeekableZipHandle; >+public: >+ static nsresult Init(PRFileDesc *fd, nsZipHandle **ret NS_OUTPARAM); >+ >+ /** >+ * Reads data at a certain point >+ * @param aPosition seek ofset >+ * @param aBuffer buffer >+ * @param aCount number of bytes to read */ >+ PRInt32 Read(PRUint32 aPosition, void *aBuffer, PRUint32 aCount); >+ >+ nsrefcnt AddRef(void); >+ nsrefcnt Release(void); NS_DECL_NSISUPPORTS, not just NS_IMPL_NSISUPPORTS or NS_INTERFACE_MAP_ENTRY and friends. >+ bool Open(nsZipHandle *aHandle, PRUint32 aOffset, PRUint32 aLength) { >+ NS_ABORT_IF_FALSE (aHandle, "Argument must not be NULL"); Spurious extra space here after the macro name.

No, not NS_DECL_ISUPPORTS, that declares virtual methods and QueryInterface, neither of which is appropriate in this situation.

Reed: thanks for review, cleaned the formatting in this one and added that oom check, this is good to go

Comment on attachment 393285 [details] [diff] [review] fixup patch [Backout: Comment 24] http://hg.mozilla.org/mozilla-central/rev/ff9eba3f8224

Comment on attachment 393285 [details] [diff] [review] fixup patch [Backout: Comment 24] >diff --git a/modules/libjar/nsJARInputStream.h b/modules/libjar/nsJARInputStream.h >@@ -83,14 +84,14 @@ class nsJARInputStream : public nsIInput >- >- PRPackedBool mDirectory; >- PRPackedBool mClosed; // Whether the stream is closed >+ PRPackedBool mDirectory; // is this a directory? Nit: mis-aligned. >+ PRPackedBool mClosed; // Whether the stream is closed >+ nsSeekableZipHandle mFd; // handle for reading

Comment on attachment 393285 [details] [diff] [review] fixup patch [Backout: Comment 24] http://hg.mozilla.org/mozilla-central/rev/630c859036af Backed out "changeset: ff9eba3f8224" due to { http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1249726330.1249727691.28491.gz WINNT 5.2 mozilla-central build on 2009/08/08 03:12:10 e:/builds/moz2_slave/mozilla-central-win32/build/modules/libjar/nsZipArchive.cpp(220) : error C2373: 'nsZipHandle::AddRef' : redefinition; different type modifiers e:\builds\moz2_slave\mozilla-central-win32\build\modules\libjar\nsZipArchive.h(242) : see declaration of 'nsZipHandle::AddRef' e:/builds/moz2_slave/mozilla-central-win32/build/modules/libjar/nsZipArchive.cpp(221) : error C2373: 'nsZipHandle::Release' : redefinition; different type modifiers e:\builds\moz2_slave\mozilla-central-win32\build\modules\libjar\nsZipArchive.h(243) : see declaration of 'nsZipHandle::Release' e:/builds/moz2_slave/mozilla-central-win32/build/modules/libjar/nsZipArchive.cpp(810) : warning C4804: '>' : unsafe use of type 'bool' in operation e:/builds/moz2_slave/mozilla-central-win32/build/modules/libjar/nsZipArchive.cpp(810) : warning C4018: '>' : signed/unsigned mismatch } I don't know if the warnings are yours too, but would be good to fix while there.

(In reply to comment #24) And give it a try run too, as it seems it may have broken xpcshell and mochitest-browser-chrome, on Linux and MacOSX.

There was already a bug on using mmap for JARS: Bug 201224 - Using mmap to memory map the JAR file, to reduce number of seeks in BuildFileList, and sequent entry copying.

This fixes ExtractFile(which caused failures in testsuite), has testcase check for that bug. I also fixed mem leak macros to and made addref/release compile on windows. This is good to check in.

Patch 393813 is ready to check in. It passes all of the try server tests, except for linux crashtest, but that seems to be broken without the patch too.

This was implicated as causing a Tp3 (private bytes) regression on OS X: Regression: Tp3 (Private Bytes) increase 2.37% on Leopard Firefox Previous results: 97050500.0 from build 20090812133821 of revision bf5b19bf6439 at 2009-08-12 13:38:00 on talos-rev2-leopard06 run # 0 New results: 99350900.0 from build 20090812135500 of revision ad1b7a04fbba at 2009-08-12 13:55:00 on talos-rev2-leopard08 run # 0 Suspected checkin range: from revision bf5b19bf6439 to revision ad1b7a04fbba

what does that mean?

(In reply to comment #32) > what does that mean? I should clarify my question. What are private bytes? is that fact that we are mmaping stuff changing some statistics or does this mean actual malloc()ed bytes?

Talos uses 'ps -o rss -p pid' to get "private bytes". It looks like the number that ps uses comes from the resident_size field of a task_basic_info structure which is documented here: http://web.mit.edu/darwin/src/modules/xnu/osfmk/man/task_basic_info.html and is described as "The number of resident pages for the task" So, I would assume that any mmapped pages that are resident would be included in the statistic.

> So, I would assume that any mmapped pages that are resident would be included > in the statistic. Seems like this isn't a regression, more of a new baseline.

(In reply to comment #35) > > So, I would assume that any mmapped pages that are resident would be included > > in the statistic. > > Seems like this isn't a regression, more of a new baseline. Why's that? Firefox uses more memory than it did before this patch. What do we get in return for a 3% increase in memory usage? Note: this also showed up on Linux and XP

(In reply to comment #36) > (In reply to comment #35) > > > So, I would assume that any mmapped pages that are resident would be included > > > in the statistic. > > > > Seems like this isn't a regression, more of a new baseline. > > Why's that? Firefox uses more memory than it did before this patch. What do we > get in return for a 3% increase in memory usage? > > Note: this also showed up on Linux and XP It's memory that will be paged out once the os determines it's no longer being accessed. I don't believe that this uses any more memory than before, it's just instead of showing up in the filesystem cache, it shows up in firefox. The benefit of this patch should be faster startup. This is just the first of a series of jar improvements(most important one), some of the mac boxes already show 80-200ms speedups

I think Taras is right, the kernel would use memory buffering the explicit i/o (read), now it charges the VM to the app due to implicit i/o (mmap). We should re-base. /be

The next step would be to, instead of memcpy from mmap into a buffer from which it is decoded, to directly decode it from the mmap'ed memory, essentially removing the need for mReadBuf. This means that less memory is allocated for decoding, and that a memcpy is saved. One could even consider then to inline the InflateStruct with nsJarInputStream saving a separate malloc/free, as it is then only about 20 bytes.

(In reply to comment #39) > The next step would be to, instead of memcpy from mmap into a buffer from which > it is decoded, to directly decode it from the mmap'ed memory, > essentially removing the need for mReadBuf. This means that less memory is > allocated for decoding, and that a memcpy is saved. > > One could even consider then to inline the InflateStruct with nsJarInputStream > saving a separate malloc/free, as it is then only about 20 bytes. Indeed. Personally, I'd like to focus on combining the rest of our io, ie bug 508421, bug 507288 and bug 509755. Additionally there needs to be a repacking step(ie files read on startup should be packed next to each other, which would reduce paging and enable readahead). There is a lot of room for improvement, any help will be appreciated. It would also be interesting to study platform-specific apis such as madvise, mlock, etc. However changes like that will have minimal effect until a higher proportion of file io originates in jars.

Just out of curiosity, all of the discussion here seems to be about non-Windows systems... was an implementation that would work for *nix-based systems AND Windows considered? It's not as if the Win32 API is short on memory mapping facilities.

(In reply to comment #41) > Just out of curiosity, all of the discussion here seems to be about non-Windows > systems... was an implementation that would work for *nix-based systems AND > Windows considered? It's not as if the Win32 API is short on memory mapping > facilities. Yes. This is for all platforms, Windows included.

v=ak