511754 - make nsZipItems point at ZipCentral references to mmapped jar area(lazy jar parsing)

Status: VERIFIED FIXED

(Core :: Networking: JAR, defect, P3)

Description

[(dormant account)]

This will save some ram and allow to lazily parse various fields that are currently being parsed eagerly causing jar-opening slowness for jars with lots of files.

Comment 1

[Alfred Kayser]

The biggest part of nsZipItem is the name, which is in the jar file not zero-terminated, but it needs to be zero-terminated for ns_Wildcard, but may be that can be done using a temporary copy during FindNext itself.

Comment 2

[(dormant account)]

I'm mostly interested in this from mobile perspective and making jars with lots of files open quicker, so even without doing the filename lazily there should be a bit of a win.

Comment 3

[Alfred Kayser]

Attachment: V1: Make nsZipItem 'lazy'

Make nsZipItem, by getting the members from the memmapped file on demand, including the name. This saves a lot of allocations and copies.
In short nsZipItem is reduced to 12 bytes compared to 30+length of name.

Comment 4

[Dietrich Ayala (:dietrich)]

thanks Alfred! does this need a reviewer, or is a WIP?

Comment 5

[Alfred Kayser]

I am currently testing this on tryserver to be really sure of the stability of this. A new version of the patch will be there real soon.

Comment 6

[Alfred Kayser]

Attachment: V2: Fixed issue on Unix builds with macro.

This version is tested on tryserver, and passes the mochitests.

Comment 7

[(dormant account)]

#define XTOINT(ii) ((PRUint16) ((ii[0]) | (ii[1] << 8)))

Alfred, I don't believe that there is a good reason to convert functions to macros.

Comment 8

[Alfred Kayser]

According to some assembler dumps from an optimized build (vc9), the xtolong and xtoint functions are not inlined, but as they have become more important as part of the lazy getters, I wanted to squeeze a little bit more performance out of this, so that a getter doesn't need to call xtolong, but can do the byte shifting inline.

Comment 9

[(dormant account)]

(In reply to comment #8)
> According to some assembler dumps from an optimized build (vc9), the xtolong
> and xtoint functions are not inlined, but as they have become more important as
> part of the lazy getters, I wanted to squeeze a little bit more performance out
> of this, so that a getter doesn't need to call xtolong, but can do the byte
> shifting inline.

I doubt inlining matters in that there is only a few hundred ziptimes, so i doubt the cost of a functioncall would be measurable. I'm inclined to trust the compiler's judgement on inlining decisions.

On the other hand, why don't we just fix xtoint(this name is so misleading) and xtolong and use platform equivalents ntohl and ntohs. Seems like they are supported on all of our targets.. 

Also I see that this code introduces additional #defined constants, please use C++ consts instead.

Comment 10

[(dormant account)]

Comment on attachment 406429 [details] [diff] [review]
V2: Fixed issue on Unix builds with macro.

These changes are pretty awesome. I love how BuidFileList went from being incomphensible in the original code to the minimal state in the current code.

in nsZipItem::IsDirectory
  return isSynthetic ? true : ('/' == Name()[nameLength - 1]); 
 The code does not seem to gurantee that nameLength > 0, this assumtion is also present in BuildSynthetics

PRUint16 const nsZipItem::Mode()
{
  if (isSynthetic) return 0755;
  return ((PRUint16)(central->external_attributes[2]) | 0x100);
}

nit: this seems like it should be a PRUint8, old code was silly


-      found = (PL_strcmp(mItem->name, mPattern) == 0);
-
+      found = ((mItem->nameLength == strlen(mPattern)) &&
+               (strcmp(mItem->Name(), mPattern) == 0));
I believe that's supposed to be a memcmp like elsewhere


(0*37+p[0])*37+p[1]

-static PRUint32 HashName(const char* aName)
+static PRUint32 HashName(const char* aName, PRUint16 len)
 {
   PR_ASSERT(aName != 0);
 
+  const PRUint8* p = (const PRUint8*)aName;
   PRUint32 val = 0;
-  for (PRUint8* c = (PRUint8*)aName; *c != 0; c++) {
-    val = val*37 + *c;
+  while (len--) {
+    val = val*37 + *p++;
   }
 
While on the subject of ugly optimization, I wonder if it'd be more efficient to do const PRUint8* endp = p + len...and then just do while(p != endp)

+  nsZipItem *        next;
+  const ZipCentral * central;

-  nsresult      FindNext(const char ** aResult);
+  nsresult      FindNext(const char ** aResult, PRUint16 * aNameLen);
 nit: no spaces before *

I think these changes + some demacroing i mentioned above, it's an r+

If you'd like can hold off on hton[ls] until another bug

Comment 11

[Brendan Eich [:brendan]]

(In reply to comment #10)
> in nsZipItem::IsDirectory
>   return isSynthetic ? true : ('/' == Name()[nameLength - 1]); 

This is a use-case for || not ?:.

/be

Comment 12

[Alfred Kayser]

Attachment: V3: Addressed comments from tglek

Comment 13

[Alfred Kayser]

http://hg.mozilla.org/mozilla-central/rev/092b0f9e39a3

Comment 14

[(dormant account)]

This baked on the trunk for a while, so it should be very safe. It's a Ts/RAM optimization for mobile. Would to take this on 192 in interests of keeping code in sync.

Comment 15

[Dietrich Ayala (:dietrich)]

Comment on attachment 406678 [details] [diff] [review]
V3: Addressed comments from tglek

a=me per discussion w/ beltzner et al.

Comment 16

[(dormant account)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/ebe315caa591