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.

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.

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.

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.

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

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.

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

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

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.

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

(In reply to comment #10) > in nsZipItem::IsDirectory > return isSynthetic ? true : ('/' == Name()[nameLength - 1]); This is a use-case for || not ?:. /be

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 on attachment 406678 [details] [diff] [review] V3: Addressed comments from tglek a=me per discussion w/ beltzner et al.