Closed Bug 523065 Opened 12 years ago Closed 12 years ago

libjar: use malloc instead of calloc for zlib

Categories

(Core :: Networking: JAR, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
status1.9.2 --- beta5-fixed

People

(Reporter: alfredkayser, Assigned: alfredkayser)

References

()

Details

(Keywords: perf)

Attachments

(2 files)

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.
Attachment #406982 - Flags: review?(tglek)
Flags: wanted-fennec1.0?
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.
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.
Attachment #406982 - Flags: review?(tglek) → review+
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
128     return gZlibAllocator->Malloc(items * size);
should be:
128     return zallocator->Malloc(items * size);
Flags: wanted1.9.2?
Attachment #406982 - Flags: approval1.9.2?
low risk for 192. Benefit is code cleanup. Above change should be applied to patch.
Depends on: 521227
Comment on attachment 406982 [details] [diff] [review]
Replace Calloc with Malloc

a=me per discussion w/ beltzner et al.
Attachment #406982 - Flags: approval1.9.2? → approval1.9.2+
You need to log in before you can comment on or make changes to this bug.