Status

()

defect
VERIFIED FIXED
10 years ago
10 years ago

People

(Reporter: taras.mozilla, Assigned: taras.mozilla)

Tracking

Trunk
mozilla1.9.2b1
x86
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [ts])

Attachments

(1 attachment, 10 obsolete attachments)

(Assignee)

Description

10 years ago
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.
(Assignee)

Comment 1

10 years ago
Posted patch rough draft (obsolete) — Splinter Review
This one does really cheesy readahead(mainly testing whether it's a win..it is)
Assignee: nobody → tglek
Whiteboard: [ts]
What happens if there is no enough address space to map the entire file?
(Assignee)

Comment 3

10 years ago
(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)
(Assignee)

Updated

10 years ago
Depends on: 505784
(Assignee)

Comment 4

10 years ago
Posted patch this works (obsolete) — Splinter Review
Attachment #389211 - Attachment is obsolete: true
Who's a potential reviewer, here?
(Assignee)

Comment 6

10 years ago
(In reply to comment #5)
> Who's a potential reviewer, here?

Benjamin. I'll submit a patch for review on monday.
(Assignee)

Comment 7

10 years ago
Posted patch mmap io (obsolete) — Splinter Review
I didn't change the ExtractFile stuff to use mmap io.
Attachment #390106 - Attachment is obsolete: true
Attachment #390846 - Flags: review?(benjamin)
(Assignee)

Comment 8

10 years ago
Posted patch mmap io (obsolete) — Splinter Review
hg chewed up my previous patch
Attachment #390846 - Attachment is obsolete: true
Attachment #390969 - Flags: review?(benjamin)
Attachment #390846 - Flags: review?(benjamin)

Comment 9

10 years ago
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...
Attachment #390969 - Flags: review?(benjamin) → review-
(Assignee)

Comment 10

10 years ago
Posted patch mmap (obsolete) — Splinter Review
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.
Attachment #390969 - Attachment is obsolete: true
Attachment #391381 - Flags: review?(benjamin)
(Assignee)

Comment 11

10 years ago
Posted patch mmap (obsolete) — Splinter Review
sorry for spam, here is the real patch
Attachment #391381 - Attachment is obsolete: true
Attachment #391386 - Flags: review?(benjamin)
Attachment #391381 - Flags: review?(benjamin)

Comment 12

10 years ago
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.
Attachment #391386 - Flags: review?(benjamin) → review+
(Assignee)

Comment 13

10 years ago
Posted patch mmap fix + testcase (obsolete) — Splinter Review
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?
(Assignee)

Comment 14

10 years ago
Posted patch fancy patch (obsolete) — Splinter Review
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.
Attachment #391386 - Attachment is obsolete: true
Attachment #391505 - Attachment is obsolete: true
Attachment #392301 - Flags: review?(benjamin)
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 17

10 years ago
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
Attachment #392301 - Flags: review?(benjamin) → review+
(Assignee)

Comment 18

10 years ago
Posted patch fixup patch to checkin (obsolete) — Splinter Review
(Assignee)

Updated

10 years ago
Keywords: checkin-needed
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.

Comment 20

10 years ago
No, not NS_DECL_ISUPPORTS, that declares virtual methods and QueryInterface, neither of which is appropriate in this situation.
(Assignee)

Comment 21

10 years ago
Reed: thanks for review, cleaned the formatting in this one and added that oom check, this is good to go
Attachment #393021 - Attachment is obsolete: true
Comment on attachment 393285 [details] [diff] [review]
fixup patch
[Backout: Comment 24]


http://hg.mozilla.org/mozilla-central/rev/ff9eba3f8224
Attachment #393285 - Attachment description: fixup patch to checkin → fixup patch [Checkin: Comment 22]
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2b1
Attachment #392301 - Attachment is obsolete: true
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Attachment #393285 - Attachment description: fixup patch [Checkin: Comment 22] → fixup patch [Backout: Comment 24]
Attachment #393285 - Attachment is obsolete: true
(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.

Updated

10 years ago
Duplicate of this bug: 201224

Comment 27

10 years ago
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.
(Assignee)

Comment 28

10 years ago
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.
Blocks: 509755
(Assignee)

Comment 29

10 years ago
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.
Keywords: checkin-needed
(Assignee)

Comment 30

10 years ago
http://hg.mozilla.org/mozilla-central/rev/ad1b7a04fbba
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
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
(Assignee)

Comment 32

10 years ago
what does that mean?
(Assignee)

Comment 33

10 years ago
(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.
(Assignee)

Comment 35

10 years ago
> 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
(Assignee)

Comment 37

10 years ago
(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

Comment 39

10 years ago
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.
(Assignee)

Comment 40

10 years ago
(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.

Comment 41

10 years ago
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.
(Assignee)

Comment 42

10 years ago
(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.

Updated

10 years ago
Depends on: 510705

Comment 43

10 years ago
v=ak
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.