333129 - libjar: Only create synthetic entries when really needed

Status: VERIFIED FIXED

(Core :: Networking, defect)

Description

[Alfred Kayser]

This is a followup to bug 309296, which introduced creation of 'synthetic directory entries' for jar browsing. 
However, two issues:
* They are always created, with give overhead, especially to startup where jar's are read and only a few entries are loaded via JarInputStream, not needing nsZipFind...
* They are sometimes duplicated (if a synthetic item is created before a real directory item is found), so that the FindNext code needs to check for duplicates which is also costly.

Solution is simple: build synthetic entries not during the BuildFileList itself, but thereafter, and only build synthetic items when needed (when FindInit is called). So, I moved the BuildSynthetic entries code to a separate function, to be called from FindInit when needed. 

Patch following in a sec.

Comment 1

[Alfred Kayser]

Attachment: Postpone creation of synthetic entries to prevent startup time impact and duplications

Comment 2

[Alfred Kayser]

Attachment: Oops wrong version, but this is the right version!

Comment 3

[Alfred Kayser]

Attachment: V3: fixed issue when directly jarbrowsing a synthetic directory

When opening directly to a synthetic directory in a jar, the synthetics were not build yet, so the GetEntry failed. With this version (V3), the synthetic entries are also build when someone (like nsJARDirectory) is doing a GetEntry on a directory. Note that before this patch GetEntry could return a synthetic entry even if a real directory entry is also present.

Comment 4

[Alfred Kayser]

Attachment: V4: Updated to commit of bug 214672

Comment 5

[Alfred Kayser]

Attachment: V5: Updated to current trunk

Updated to also contain the 'crash at CloseArchive' and the 'don't test for synthetic entries' patches.
Extended that last one with don't test for directory entries in general, even when only testing a single entry. (directories cannot be tested, synthetic or not)

Comment 6

[Jeff Walden [:Waldo]]

I'll get to this over either this weekend or sometime soon after next Thursday.

Comment 7

[Jeff Walden [:Waldo]]

Comment on attachment 221290 [details] [diff] [review]
V5: Updated to current trunk

It doesn't seem possible to postpone creation of synthetic entries past when some entry in the zip is actually "accessed" (getting a stream, testing existence, accessing metadata, testing the crc32, etc.).  Since a zip archive is effectively useless without being able to do any of those things (and indeed, at least one of these things must happen for each accessed zip at startup), is there really any benefit to postponing synthetic creation beyond nsZipArchive initialization?  If there is a benefit, I don't see it.  Why not just call |BuildSynthetics| at the end of |BuildFileList| and do away with all the is-this-already-synthesized logic?

Comment 8

[Alfred Kayser]

It would certainly seem so, however most jars (especially at startup) are only opened to get file entries (through GetInputStream) for files such as 'global.css' ,etc... Not building the synthetic entries for these jars does save startup time and memory usage. 
So, for now, I am not directly accepting to just always build the synthetic entries...

Comment 9

[Alfred Kayser]

Comment on attachment 221290 [details] [diff] [review]
V5: Updated to current trunk

Darin, can you review this one?
This patch not only postpones the 'BuildSynthetics' generation (until only really needed), which really improves startup time, but also it improve s on the algorithm, so that it doesn't generate duplicate synthetic entries. 
Note, synthetic entries are only generated for directory entries, so all of the (file) extraction and testing access never need to access the synthetic entries...

Comment 10

[Darin Fisher]

By how much does this improve startup time?  And, are there any issues with the fact that JAR files are accessed from multiple threads?

Comment 11

[Alfred Kayser]

Some performance testing results:
On a 1.7 Ghz Pentium M, WinXP Sp2, (IBM T41), the additional startup time taken by 'BuildSynthetics' is on average 15 milliseconds (small but not negligable).
The testing also proved that for normal operation (anything but installation of extensions/themes and but for jarfile browsing), BuildSynthetics is indeed never called. So this will not just save startup time, but also memory usage (create only synthetic entries when really needed...)

About MT: The test at the start of BuildSynthetics(): 
if (isSynt) return; isSyn = TRUE; makes it almost impossible to run this code multiple times (except when it switches between the if and the assignment).
Even so, the following code is harmless when run multiple times: before creation of a new synthetic entry it verifies for existance. If even that fails, then the only thing that can happen is that multiple entries are created (which is also happening in the current code, but then always!)

Summary:
* It really improves startup time (by about 15 msecs on a fast machine)
* It really saves memory, building only when needed
* And it doesn't create multiple synthetic entries
* FindNext doesn't need to check for these duplicated entries

Comment 12

[Darin Fisher]

Comment on attachment 221290 [details] [diff] [review]
V5: Updated to current trunk

r=darin

Comment 13

[Boris Zbarsky [:bzbarsky]]

I'm kinda swamped right now... biesi, do you think you have time to sr this?

Comment 14

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 221290 [details] [diff] [review]
V5: Updated to current trunk

this review may take me a few days

Comment 15

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 221290 [details] [diff] [review]
V5: Updated to current trunk

@@ -708,87 +719,60 @@ nsZipArchive::FindInit(const char * aPat
+  // Create synthetic directory entries if needed
+  return BuildSynthetics();

if this returns failure,  then *aFind will be leaked... this should be handled better somehow

I'd kind of prefer the old structure in nsZipFind::FindNext, i.e. this part:
-    if (!mItem)
-      ++mSlot;                          // no more in this chain, move to next slot
-    else if (!mPattern)
-      found = PR_TRUE;            // always match
-    else if (mRegExp)
-      found = (NS_WildCardMatch(mItem->name, mPattern, PR_FALSE) == MATCH);
-    else

IMO that was easier to understand because the possible cases were clearly visible.

nsZipArchive.h
+  // Build the synthetic directories only when needed
+  PRPackedBool  mIsSynthesized;

That's not a very useful comment... it doesn't tell you anything about what the variable does. The name of the variable is also not very intuitive (since it's not the archive that's synthesized). How about:

  // Whether we synthesized the directory entries
  PRPackedBool mHaveSynthetics;

(this variable name also parallels the naming of BuildSynthetics())

Comment 16

[Christian :Biesinger (don't email me, ping me on IRC)]

or even mBuiltSynthetics.

Comment 17

[Alfred Kayser]

Attachment: V6: Comments from biesinger incorporated

+  // Create synthetic directory entries if needed
+  return BuildSynthetics();
if this returns failure,  then *aFind will be leaked... this should be handled
better somehow
> Fixed by moving this before the allocs, so nothing gets leaked.

I'd kind of prefer the old structure in nsZipFind::FindNext, i.e. this part:
> Ok, reverted to the old structure for clarity

nsZipArchive.h
+  // Build the synthetic directories only when needed
+  PRPackedBool  mIsSynthesized;
> Changed to mBuiltSynthesized

Comment 18

[:Gavin Sharp [email: [e-mail]]]

mozilla/modules/libjar/nsZipArchive.cpp 	1.88
mozilla/modules/libjar/nsZipArchive.h 	1.43

Comment 19

[:Gavin Sharp [email: [e-mail]]]

I checked in the following temporary bustage fix in addition to the patch here:
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/modules/libjar&command=DIFF_FRAMESET&file=nsZipArchive.cpp&rev1=1.88&rev2=1.89&root=/cvsroot

I don't know how this should best be fixed (define NS_FAILED in zipstub.h, maybe?), so I'll leave that up to you Alfred.

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The rest of the module seems to check results against ZIP_OK.

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I changed gavin's bustage fix so that it's:

-  if (NS_FAILED(rv)) return rv;
+  if (rv != ZIP_OK)
+    return rv;

(diff against before the bustage fix)

Comment 22

[Alfred Kayser]

Shame on me!

Indeed the fix by David Baron is the right one.