Use 'inflateBack' instead of 'inflate' in ZipArchive.cpp

RESOLVED INCOMPLETE

Status

()

Core
Networking: JAR
--
enhancement
RESOLVED INCOMPLETE
14 years ago
2 years ago

People

(Reporter: Alfred Kayser, Assigned: Alfred Kayser)

Tracking

({helpwanted})

Trunk
Future
helpwanted
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

14 years ago
Now that zlib 1.2.1 is used in Mozilla, we 
can try to see if 'inflateBack' can reduce buffer copies
in nsZipArchive.cpp.

See manual of zlib 1.2.1 for 'inflateBack'

Comment 1

14 years ago
Alfred: feel free to contribute a patch :)
Keywords: helpwanted
Target Milestone: --- → Future
(Assignee)

Comment 2

13 years ago
Just a quick update:
inflateBack doesn't seem usefull for the 'JARInputStream' with its own streaming
capabilities. However it is usefull for the 'extracting' of files.
Using inflateBack the 'window' used by inflate will also double as output buffer
to the file writes. inflateBack wants a 32K window buffer be passed, instead of
allocating itself.

So, an idea is to allocate this 32K window once for the ZIPReader, and re-use it
for each extraction (just as the filepointer is re-used). Furthermore instead of
using two (stack) buffers of 4K (one for read, and for write), only one (stack)
8K buffer could be used to read (or even more?).
Codesize wouldn't change much, but it would improve the speed of extracting
files (for installation of mozilla apps as well as for extensions and themes).

It is a small change, and a patch will be posted soon.
(Assignee)

Comment 3

13 years ago
Created attachment 168041 [details] [diff] [review]
Proof-of-concept patch

This patch is diff against my patched version of libjar.
See Bug 214672, attachment jar_patch7.txt.

Changes:
* Use inflateBack for extraction (saves a buffer copy in writing)
* Use a re-used mExtractWindow for both inflation and copy
* Removed wildcard matching from Find* for the standalone (see bug 272451)
* Removed use of RecyclingAllocator as zlib only does one or two allocs per
'inflate' (one for the window, in case of normal inflate, and one for its own
data). 
* Removed nsJarShutdown, was only needed for the RecyclingAllocator...
* In total 1.5K code for normal and about 3K code reduction in standalone.

So, this saves code as well as improves the speed of extraction (less buffer
copying...). All in the quest for smaller and faster browsers (and other apps).
Assignee: darin → alfredkayser
Status: NEW → ASSIGNED
(Assignee)

Updated

13 years ago
Depends on: 214672
(Assignee)

Comment 4

12 years ago
Created attachment 221429 [details] [diff] [review]
V2: updated to current trunk and some cleanups
Attachment #168041 - Attachment is obsolete: true
Attachment #221429 - Flags: review?(darin)
(Assignee)

Comment 5

12 years ago
Created attachment 221430 [details] [diff] [review]
V3: Small fix in CopyItemToDisk

Some notes:
* Use gzlibAllocator to also allocate the 32K 'inflateBack' buffer
* Changed calloc's into malloc's according to zlib documentation
* Removed the BY4ALLOC_ITEMS stuff (deadcode)
* CopyItemToDisk and InflateItem now both static functions (better for optimization)
Attachment #221429 - Attachment is obsolete: true
Attachment #221430 - Flags: review?(darin)
Attachment #221429 - Flags: review?(darin)
(Assignee)

Comment 6

12 years ago
Created attachment 221431 [details] [diff] [review]
v4: Don't mix the readbuffer with inflate buffer...

Oops, the readbuffer for inflateBack must not be the same as the inflate/write window buffer.

The impact of this all is thus to save buffer copies, and to use a 32K buffer instead of 4K buffers to the file I/O (sharing the inflation buffer with the file write buffer).
Attachment #221430 - Attachment is obsolete: true
Attachment #221430 - Flags: review?(darin)
(Assignee)

Updated

12 years ago
Component: Networking: File → Networking: JAR
Assignee: alfredkayser → nobody
Status: ASSIGNED → NEW
QA Contact: benc → networking.jar
(Assignee)

Comment 7

9 years ago
There may be chance that this can still be done.
Assignee: nobody → alfredkayser
Status: NEW → ASSIGNED
I'm going to close this as inactive. Feel free to reopen if you've got an updated patch and benchmark
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.