379929 - Lazy creation of nsProperties object for imgContainer

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Alfred Kayser]

The imgContainer object always creates and initializes an nsProperties object with all the overhead involved. In fact, only CUR and XBM provide properties (hotspotX/Y), and these are very rare. Instead of always allocating nsProperties, one could defer to the first Set call on it. Code is simple, copy the 'NS_FORWARD_SAFE' define and change it to not return failure on a non-existing properties object for Get,Undefine,Has and GetKeys, and to allocate the object on the first Set call.

Comment 1

[Alfred Kayser]

Attachment: Patch to lazily allocate the nsProperties object

Comment 2

[tor]

Comment on attachment 263984 [details] [diff] [review]
Patch to lazily allocate the nsProperties object

> imgContainer::imgContainer() :
>   mSize(0,0),
>+  mProperties(nsnull),

nsCOMPtrs initialize themselves to null with their default constructor.

>+NS_IMETHODIMP imgContainer::GetKeys(PRUint32 *count, char ***keys)
>+{
>+  if (!mProperties) {
>+    *count = 0;
>+    *keys = (char **) nsMemory::Alloc(0);

Is malloc(0) behavior well defined on all the platforms we support?

Comment 3

[Alfred Kayser]

Attachment: V2: Fixed comments from TOR

Replaced Alloc(0) with just nsnull.
Common sense seems to be to return null in case of an empty property list.
Note that nsProperties.cpp in case of no entries also performs Alloc(n*size), so effectively also is platform unpredictable... 

Also: check for valid _retval pointer before setting it to false.

Comment 4

[Nickolay_Ponomarev]

Checked in, I hoped you tested the CUR/XBM cases, because I didn't.

Checking in modules/libpr0n//src/imgContainer.cpp;
/cvsroot/mozilla/modules/libpr0n/src/imgContainer.cpp,v  <--  imgContainer.cpp
new revision: 1.48; previous revision: 1.47
done
Checking in modules/libpr0n//src/imgContainer.h;
/cvsroot/mozilla/modules/libpr0n/src/imgContainer.h,v  <--  imgContainer.h
new revision: 1.25; previous revision: 1.24
done


>Note that nsProperties.cpp in case of no entries also performs Alloc(n*size),
>so effectively also is platform unpredictable... 

Should a bug be filed about that?

Comment 5

[Alfred Kayser]

According to tinderbox, the number of allocated nsProperties is now halved:

Before:
   527 nsProperties                                    4        0      165        0 (   82.50 +/-    47.71)      495        0 (   83.17 +/-    47.66)
After:
   527 nsProperties                                    4        0       83        0 (   41.50 +/-    24.04)      249        0 (   42.17 +/-    23.99)