Make nsIZipEntry redundant, and remove it.

RESOLVED INVALID

Status

()

Core
Networking: JAR
--
minor
RESOLVED INVALID
15 years ago
10 years ago

People

(Reporter: Alfred Kayser, Assigned: Alfred Kayser)

Tracking

({memory-footprint})

Trunk
x86
All
memory-footprint
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(8 obsolete attachments)

(Assignee)

Description

15 years ago
As a followup of bug 203344, this bug is about removing nsIZipEntry.
It is (after bug 203344) only used in :
http://lxr.mozilla.org/seamonkey/source/rdf/chrome/src/nsChromeRegistry.cpp#822
and only to test whether the entry exists in the jar/zipfile.

So may be in ZipReader interface replace getEntry with existsEntry, to
just return a bool, to indicate existance?

Just like 203344, this saves an object creation and immediate deletion, just
for a simple value retrieval.
(Assignee)

Updated

15 years ago
No longer depends on: 203344
(Assignee)

Comment 1

15 years ago
*** Bug 203626 has been marked as a duplicate of this bug. ***

Comment 2

15 years ago
sounds good to me.  alecf: what do you say?
Severity: normal → minor
Status: NEW → ASSIGNED
Keywords: footprint
Target Milestone: --- → Future

Comment 3

15 years ago
this sounds great!
(Assignee)

Comment 4

15 years ago
Ok, a summary of things to do:

nsIZipReader.idl:
1. change nsISimpleEnumerator/*<nsIZipEntry>*/ findEntries(in string aPattern);
   to return an enumeration of strings (just the entryname).
2. change getEntry to containsEntry returning a boolean
3. remove nsIZipEntry

Ad 1. Fix callers of findEntries:
http://lxr.mozilla.org/seamonkey/source/xpinstall/src/nsInstall.cpp#2741
http://lxr.mozilla.org/seamonkey/source/xpinstall/src/nsSoftwareUpdateRun.cpp#105
http://lxr.mozilla.org/seamonkey/source/modules/libjar/nsXPTZipLoader.cpp#68
http://lxr.mozilla.org/seamonkey/source/modules/libjar/nsJAR.cpp#507
http://lxr.mozilla.org/seamonkey/source/modules/libjar/nsJAR.cpp#542
All five use the ZipEntry returned by findEntries to get the name of the entry,
so patch is simple, and even addressing performance/footprint.
Cleanup of FindEntries, etc in nsJAR/nsZipArchive.

Ad 2. Fix callers of GetEntry
http://lxr.mozilla.org/seamonkey/source/rdf/chrome/src/nsChromeRegistry.cpp#823
(and bug 203344 of course)
nsJAR.cpp: Change GetEntry to containsEntry...

Ad 3. Remove the nsIZipEntry from idl (and .cpp/.h)
Depends on: 203344

Comment 5

15 years ago
note that there is now nsIStringEnumerator if you need it..
(Assignee)

Comment 6

15 years ago
Which looks perfect for this situation. 
However I'm not to sure with (A)(C)Strings yet...
(Assignee)

Comment 8

15 years ago
Created attachment 122104 [details] [diff] [review]
Patch for modules\libjar

Removing zipEntry from IDL, replacing GetEntry with ContainsEntry, etc.

Some test info:
		 New	     Old
    jar50.dll	94.268	    98.364
    jar.xpt	   814	       941
Savings: about 4K of dllsize.	 

Timings(5 starts):  3.95     3.96, so measureable difference
(Assignee)

Comment 9

15 years ago
Created attachment 122105 [details] [diff] [review]
Patch for rdf\chrome\src

Second part of patch, to be applied in rdf\chrome\src.

Comment 10

15 years ago
dveditz: thought this bug might interest you...
(Assignee)

Comment 11

15 years ago
Created attachment 122202 [details] [diff] [review]
Patch for xpinstall\src

Third part: for xpinstall.
Changes:
 nsInstall.cpp: 
    replaced ExtractDirEntries with UTF8StringEnumerator
    removed DeleteVector (redundant now)
 nsSoftwareUpdateRun.cpp:
    replaced SimpleEnum with ZipEntry's to UTF8StringEnumerator

Comment 12

15 years ago
this is great stuff. I look forward to reviewing it later... :)
(Assignee)

Comment 13

15 years ago
Further investigation showed that Open and Close are also obsolete. 
Open is always called directly after Init, so Init and Open can be
combined into one function. Close is nowhere used, except
in the destructur of nsJAR itself. 
So, IMHO Open and Close can be removed from the nsIZipReader.idl.

Init/Open:
http://lxr.mozilla.org/seamonkey/source/caps/src/nsScriptSecurityManager.cpp#2307
http://lxr.mozilla.org/seamonkey/source/xpinstall/src/nsSoftwareUpdateRun.cpp#239

Other issues:
** is the nsIZipReader.idl include is still needed?
http://lxr.mozilla.org/seamonkey/source/caps/idl/nsICertificatePrincipal.idl#39
 39 /* Defines an interface for certificate principals. */
 40 #include "nsIPrincipal.idl"
 41 #include "nsIZipReader.idl"

** Here is a cache created for 32 entries, but only used to get one zip?
http://lxr.mozilla.org/seamonkey/source/rdf/chrome/src/nsChromeRegistry.cpp#873
873   //
874   // also cache the zip file itself
875   //
876   nsCOMPtr<nsIZipReaderCache> readerCache =
877     do_CreateInstance("@mozilla.org/libjar/zip-reader-cache;1", &rv);
878   if (NS_FAILED(rv)) return rv;
879 
880   rv = readerCache->Init(32);
881   
882   rv = readerCache->GetZip(overrideFile, getter_AddRefs(mOverrideJAR));

So, do I create separate bugs for these issues?

Comment 14

15 years ago
alfred: i don't have much opinion on the matter of separate bugs.  i'm fine if
you want to just add patches here for those other changes.
(Assignee)

Comment 15

15 years ago
I've got a patch to cleanup the nsIZipReader, as discussed, including the
ZipEntry removal, the redundant Open/Close, and lots of small fixes in libjar.
The patch is probably a bit bitrotted, as the tree progresses.
I'll upload a clean patch somewhere next week, depending on connectivity issues.
(Assignee)

Comment 16

15 years ago
Created attachment 127932 [details] [diff] [review]
modules\libjar: Removal of ZipEntry, and corresponding cleanup

This is complete, but needs some more testing, and
I will provide some more documentation on the changes.
Attachment #122104 - Attachment is obsolete: true
(Assignee)

Comment 17

15 years ago
Created attachment 128159 [details] [diff] [review]
Updated patch for xpinstall\src

This also removes the redundant 'Open', and
uses nsnull as argument for FindEntries (which is same but faster than '*').
Attachment #122202 - Attachment is obsolete: true
(Assignee)

Comment 18

15 years ago
Created attachment 128162 [details] [diff] [review]
Updated patch for modules\libjar

+----------------------------------------------------------------------------
| Summary of changes
+----------------------------------------------------------------------------
1: Removal of ZipEntry from IDL, including corresponding nsJarItem and
nsZipItem
2: Removal of Open/Close from nsZipReader.idl: merged with Init and Destructor
3: Change errorcodes to nsresult 
4: FindEntries: use nsIUTF8StringEnumerator for finds, returning names.
5: ZipItem changed to struct (saves class overhead)
6: In xpinstall use 'nsnull' as find pattern instead of '"*"' (same but
faster):
   
http://lxr.mozilla.org/seamonkey/source/xpinstall/src/nsSoftwareUpdateRun.cpp#134

    FindEntries("*", ...)
7: nsJar.cpp: Only allocate and init ObjectHashtable when needed:
    Made mManifestData a pointer to the ObjectHashtable, and allocate
    only when really needed. Reduces size of nsJAR from 1164 to 1104
    (saves 60 bytes per JAR), excluding the extra memory allocated by 
    ObjectHashtable itself.
    Also remove mParsedManifest (mManifestData indicates parsedManifest status)

8: nsJar.cpp: VerifyEntry: Call LoadEntry only when really needed.
9: class nsJARManifestItem has three (3) flags:
    Changed Enum into PRInt8 (to save bits), and grouped with other flags
    (sizeof nsJARManifestItem reduced from 24 to 20).
10: ZipReader: replace GetEntry with ContainsEntry: returns boolean instead of
ZipEntry
11: Removal of redundant nsJarItem

+----------------------------------------------------------------------------
| Footprint data
+----------------------------------------------------------------------------
- Structure/class size changes
	nsJar: 1168 to 1104
	nsJarEnumerator: 17 to 8
	nsJarManifestDataItem: 24 to 20
	nsJarItem: gone
- Mallocs: 
	FindEntries: returns names instead of ZipItem structures
	Only allocate ObjectHashtable when really needed
	VerifyEntries: only load (and allocate buffer) when really needed
	BuildFileList: nsZipItem allocate together with name (replace 2 mallocs
with 1 malloc in STANDALONE)
- Codesize (debug) jar50.dll: from 98.364 to 86.076 bytes (.dll filesize)      
 
	jar50_s.lib: from 57.624 to 49.062 
	(total about 12% reduction)

TODO:
** is the destructor of ManifestItem really called?
nsJARManifestItem::~nsJARManifestItem()
{
  // Delete digests if necessary
  PR_FREEIF(calculatedSectionDigest);
  PR_FREEIF(storedEntryDigest);
}
PR_STATIC_CALLBACK(PRBool)
DeleteManifestEntry(nsHashKey* aKey, void* aData, void* closure)
{
//-- deletes an entry in  mManifestData.
  PR_FREEIF(aData);
  return PR_TRUE;
}
Does the 'free' in DeleteManifestEntry calls the destructor??



+----------------------------------------------------------------------------
| Detailed description of changes (see patch.txt)
+----------------------------------------------------------------------------
Changes:
nsIZipReader.idl: 
* removal of ZipEntry
* ZipReader: removal of 'file' attribute
* ZipReader: removal of 'open' and 'close' members
* ZipReader: replace GetEntry with ContainsEntry: returns boolean instead of
ZipEntry
* ZipReader: attribute name consistency: aEntryName
* ZipReader: FindEntries: returns nsIUTF8StringEnumerator

nsJar.h:
* add nsJar:GetJarPath
* remove nsJarItem (=ZipEntry)
* nsJarEnumerator implements nsIUTF8StringEnumerator
* nsJarEnumerator: removal of mArchive and mIsCurrStale (size: 17 -> 8)

nsJar.cpp:
* removal of ziperr2nsresult
* removal of JAR_NULLFREE
* nsJAR: new member 'GetJarPath'
* nsJAR: replace GetEntry with ContainsEntry
* nsJAR: FindEntries: change to use nsIUTF8StringEnumerator
* replace .Length()==0 with .IsEmpty
* nsJAREnumerator: return nsIUTF8StringEnumerator
* nsJAREnumerator: remove mIsCurrStale (mCurr==NULL indicates 'stale')
* Replace archive->FindFree(mFind) with delete mFind
* removal of nsJarItem (ZipEntry)

nsInputStream.cpp:
* use nsresult errorcodes instead of ZIP_...

nsXPTZipLoader.cpp:
* use the new FindEntries with the nsIUTF8StringEnumerator

nsZipArchive.cpp:
* make ExtractMode and IsSymLink only for XP_UNIX
* change PRInt32 return codes to nsresult
* Zip_FindFree does just 'delete find' insteadof
'find->GetArchive()->FindFree(find)'
* ::Test: simplified
* ::GetItem return item as returnvalue directly
* consistency: attribute ZipEntry to aEntryName
* nsZipFind add member FindNext, to allow hiding of mArchive
* nsZipArchive::FindNext: also update mSlot and mItem when nothing found, so
that next find request
will not search whole table again.
* removal of ::FindFree, just use 'delete'
* BuildFileList: 
    - general cleanup, 
    - nsZipItem allocate together with name (replace 2 mallocs with 1 malloc in
STANDALONE)
    - malloc nsZipItem instead of new (new sets to zero, and other overhead).
    - extract 'external_attributes' only for XP_UNIX
    - removal of the 'debug' code (errors)
* ::TestItem: removal of 'goto' and 'bInflating'
* removal of nsZipItem
* removal of nsZipFind::GetArchive

nsZipArchive.h:
* nsZipItem changed to struct, to remove class overhead
* nsZipItem: name as last member, to alloc name buffer together with nsZipItem
* nsZipItem: 'mode' only for XP_UNIX
* nsZipFind: removal of FindFree, add of FindNext
* nsZipFind: move mSlot after mItem, for better byte alignment
* nsZipArchive::FindNext returns entryName instead of nsZipItem

zipfile.h:
* use nsresult codes
Attachment #128159 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #128159 - Attachment is obsolete: false
(Assignee)

Updated

15 years ago
Attachment #127932 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #122105 - Flags: review?(darin)
(Assignee)

Updated

15 years ago
Attachment #128159 - Flags: review?(darin)
(Assignee)

Updated

15 years ago
Attachment #128162 - Flags: review?(darin)

Comment 19

15 years ago
sorry, i haven't had a chance to look over this patch yet... it may take me
another week or so to get to this patch :-(
(Assignee)

Updated

15 years ago
Blocks: 214672
(Assignee)

Comment 20

15 years ago
Created attachment 132044 [details] [diff] [review]
patch for /netwerk/protocl/jar/src

Required patch for the IDL change: 
* remove calls to Open, Close (not needed anymore)
(Assignee)

Updated

15 years ago
Attachment #132044 - Flags: review?(darin)

Comment 21

15 years ago
applying the patch from attachment 128162 [details] [diff] [review], i get a number of conflicts:

7 out of 25 hunks FAILED -- saving rejects to file nsJAR.cpp.rej
1 out of 6 hunks FAILED -- saving rejects to file nsJAR.h.rej
2 out of 42 hunks FAILED -- saving rejects to file nsZipArchive.cpp.rej
1 out of 16 hunks FAILED -- saving rejects to file nsZipArchive.h.rej

my appologies for not getting to this patch sooner... i tried digging in to fix
up the conflicts myself, but it looks like (at least some of) the conflicts are
actually with your previous patch for bug 201226 (or maybe i'm mistaken).  at
any rate, if you could update the patches (maybe put everything in one diff),
then i will review them promptly.
(Assignee)

Updated

15 years ago
No longer blocks: 214672
Depends on: 214672
(Assignee)

Comment 22

15 years ago
Comment on attachment 122105 [details] [diff] [review]
Patch for rdf\chrome\src

See bug 214672 for the integrated patch
Attachment #122105 - Flags: review?(darin)
(Assignee)

Comment 23

15 years ago
Comment on attachment 128159 [details] [diff] [review]
Updated patch for xpinstall\src

See bug 214672 for the integrated patch
Attachment #128159 - Attachment is obsolete: true
Attachment #128159 - Flags: review?(darin)
(Assignee)

Comment 24

15 years ago
Comment on attachment 128162 [details] [diff] [review]
Updated patch for modules\libjar

See bug 214672 for the integrated patch
Attachment #128162 - Attachment is obsolete: true
Attachment #128162 - Flags: review?(darin)
(Assignee)

Comment 25

15 years ago
Comment on attachment 132044 [details] [diff] [review]
patch for /netwerk/protocl/jar/src

See bug 214672 for the integrated patch
Attachment #132044 - Attachment is obsolete: true
Attachment #132044 - Flags: review?(darin)
(Assignee)

Updated

15 years ago
Attachment #122105 - Attachment is obsolete: true
(Assignee)

Comment 26

12 years ago
With bug 214672 fixed, this one becomes slightly simpler to solve, however JARInputDirectoryStream introduces a new user of nsZipEntry, which can be fixed by making that one calling directly nsZipArchive/nsZipItem.
One other caller is for checking 'IsSynthetic' in jarfile validation, but that should be checking for 'isDirectory' anyway (there is no way that a directory can be validated against a signature ;-) ).

nsIZipEntry users are:
/modules/libjar/nsJARDirectoryInputStream.cpp, line 98 -- nsCOMPtr<nsIZipEntry> ze;
/modules/libjar/nsJARDirectoryInputStream.cpp, line 208 -- nsCOMPtr<nsIZipEntry> ze;
/xpinstall/src/nsSoftwareUpdateRun.cpp, line 146 -- nsCOMPtr<nsIZipEntry> entry;
/xulrunner/setup/nsXULAppInstall.js, line 48 -- const nsIZipEntry = Components.interfaces.nsIZipEntry;
/xulrunner/setup/nsXULAppInstall.js, line 172 -- var entry = entries.getNext().QueryInterface(nsIZipEntry);
(Assignee)

Comment 27

12 years ago
bug 337561 makes it that nsJARDirectoryInputStream no longer needs nsIZipEntry.
So that only rdf/chrome is the last user of nsIZipEntry, to check for IsSynthetic, to prevent jarfile validation on synthetic entries, but actually all directory entries should not be checked for valid signature (as a directory as no data to base the signature on!), so a check for '/' at the end of the entryname is sufficient, instead of doing GetEntry->IsSynthethic (which is very costly).
Depends on: 337561

Updated

12 years ago
Assignee: darin → nobody
Status: ASSIGNED → NEW
QA Contact: benc → networking.file
Target Milestone: Future → ---
(Assignee)

Updated

12 years ago
Depends on: 346088
(Assignee)

Comment 28

12 years ago
Created attachment 242479 [details] [diff] [review]
Completely remove nsIZipEntry

Simple patch to remove nsIZipEntry.
Note, this also removes nsJARDirectoryInputStream.h (leftover from bug 337561).
Attachment #242479 - Flags: review?(darin)

Comment 29

12 years ago
Comment on attachment 242479 [details] [diff] [review]
Completely remove nsIZipEntry

ok, r=darin
Attachment #242479 - Flags: review?(darin) → review+
(Assignee)

Updated

12 years ago
Attachment #242479 - Flags: superreview?(cbiesinger)
(Assignee)

Updated

12 years ago
Component: Networking: File → Networking: JAR

Updated

12 years ago
QA Contact: networking.file → networking.jar
(Assignee)

Comment 30

10 years ago
Christian Biesinger, any chance of sr'ing this patch?
Assignee: nobody → alfredkayser
(Assignee)

Updated

10 years ago
Status: NEW → ASSIGNED
Comment on attachment 242479 [details] [diff] [review]
Completely remove nsIZipEntry

Bug 379633 brought code that actually uses this back into existence, so this isn't going to work.
Attachment #242479 - Attachment is obsolete: true
Attachment #242479 - Flags: superreview?(cbiesinger)
(Assignee)

Comment 32

10 years ago
Marking this one as 'INVALID' (see comment #31).

Created bug 412818 to cvs remove nsJARDirectoryInputStream.h
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.