115752 - gif_write : memory churn

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect, P3)

Description

[Suresh Duddi (gone)]

gif_write() allocates memory for prefix, suffix and stack for lzw decompression
for every image. These buffers are fixed size 4097, 4097, 8194 bytes.

We could recycle these buffers easily. We read about 120 image files on startup.

Comment 1

[Suresh Duddi (gone)]

Attachment: Reduce gif_write memory churn

On startup this would reduce:

360 [120 * 3] allocations/free
2,063,760 [120 * 8k + 240 * 4k] of memory chrun

Comment 2

[Suresh Duddi (gone)]

Ccing simon for sr=

Comment 3

[Simon Fraser [no longer active]]

+#define NBUCKETS 3 

Prefer const PRInt32 kNumBuckets = 3;

It's also the case that you can't have more than 32 buckets (limited by the 
mFlags), so you should comment this.

+    // Clear the buckets of memory we have for zlib
+    int ClearBuckets();

int? Also, what does the return value mean? You always return 0.

+
+#define IS_IN_USE(i) (mFlag & (1 << i))
+#define MARK_USED(i)  mFlag |= (1 << i)
+#define CLEAR_USED(i) mFlag &= ~(1 << i)

Prefer inline functions (you are such the old unix hacker!). Then you can assert 
if i > 31.

+    nsGifAllocator() : mFlag(0)

You shoudl initialize mLock to nsnull too.

+  for (int i = 0; i < NBUCKETS; i++)

PRInt32?

+      mMemBucket[i] = 0;

minor niggle; I prefer using nsnull for null pointers, to disambiguate with 
coding mistakes where you thought mMemBucket[] was a value, rather than a 
pointer.

+  int freeAllocatedBucket = -1;

PRInt32?

+static inline void
+gif_free(void *ptr)
+{
...
+  if (!gGifAllocator)
+    gGifAllocator = new nsGifAllocator;
...

Is it ever worth making the allocator in a gif_free? If you can't guarantee 
you've called gif_calloc before, you might as well just free().

Finally, do we want to hook up this cache to the memory flusher? It could hold up 
to 128K of memory.

Comment 4

[Suresh Duddi (gone)]

Attachment: Includes comments from Simon

This keeps only 16k (not 128k). For now it gets freed at module shutdown. Later
we will think about rolling a timer and cleaning it up.

Comment 5

[Stuart Parmenter]

Comment on attachment 62150 [details] [diff] [review]
Includes comments from Simon

r=pavlov

Comment 6

[Simon Fraser [no longer active]]

Kill those 'returns' in the inline methods, and sr=sfraser

Comment 7

[Suresh Duddi (gone)]

Removed the returns. Fix checked in.

Comment 8

[John Morrison]

Hey dp, you left a naked printf in the checkin, and it's spewing dozens of
messages into the console while running the page-loader test
(libpr0n/decoders/gif/GIF2.cpp

DEBUG: global_colormap - 16 [4 x 4]
DEBUG: global_colormap - 1024 [256 x 4]
DEBUG: global_colormap - 1024 [256 x 4]
DEBUG: global_colormap - 1024 [256 x 4]
DEBUG: global_colormap - 512 [128 x 4]

If someone kills that printf then mark this fixed again.

Comment 9

[Robert John Churchill]

seawood checked in a fix for this, BTW

Comment 10

[Suresh Duddi (gone)]

Thanks seawood.

Comment 11

[Hixie (not reading bugmail)]

The Image Conversion Library component is going away.