523065 - libjar: use malloc instead of calloc for zlib

Status: RESOLVED FIXED

(Core :: Networking: JAR, defect)

Description

[Alfred Kayser]

Attachment: Replace Calloc with Malloc

Currently in libjar/nsZipArchive, Calloc (from nsRecyle*) is used for zalloc. 
However, for zlib, the use of malloc is sufficient, and will save a memset of 32768 and 9250 bytes per deflation, which will have some performance impact, especially on Fennec.

Some documentation:
http://www.zlib.net/zlib_faq.html says:
36. Valgrind (or some similar memory access checker) says that deflate is performing a conditional jump that depends on an uninitialized value. Isn't that a bug?
No. That is intentional for performance reasons, and the output of deflate is not affected. This only started showing up recently since zlib 1.2.x uses malloc() by default for allocations, whereas earlier versions used calloc(), which zeros out the allocated memory. 

From http://www.zlib.net/ChangeLog.txt:
Changes in 1.2.0 (9 March 2003)
- Use malloc() instead of calloc() in zutil.c if int big enough.

Comment 1

[(dormant account)]

Comment on attachment 406982 [details] [diff] [review]
Replace Calloc with Malloc


> static void *
> zlibAlloc(void *opaque, uInt items, uInt size)
> {
>   nsRecyclingAllocator *zallocator = (nsRecyclingAllocator *)opaque;
>-  if (gZlibAllocator) {
>-    return gZlibAllocator->Calloc(items, size);

Can you check if there is any benefit to using this nsRecyclingAllocator? I suspect that's just speculative optimization and using zlib defaults would be no worse.

Comment 2

[Alfred Kayser]

Attachment: Trace of nsRecycle calls

See attached trace of nsRecycle calls: the allocs 9520 and 32768 performed by nsZipArchive are 'reused' a lot, so it seems that nsRecycleAllocator still has its use. The alloc's of 4096 are for necko's IO buffers.

Comment 3

[Alfred Kayser]

Pushed: http://hg.mozilla.org/mozilla-central/rev/b0b361366219

Comment 4

[Alfred Kayser]

128     return gZlibAllocator->Malloc(items * size);
should be:
128     return zallocator->Malloc(items * size);

Comment 5

[(dormant account)]

low risk for 192. Benefit is code cleanup. Above change should be applied to patch.

Comment 6

[Dietrich Ayala (:dietrich)]

Comment on attachment 406982 [details] [diff] [review]
Replace Calloc with Malloc

a=me per discussion w/ beltzner et al.

Comment 7

[(dormant account)]

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