565124 - imgContainer.h should use |PRPackedBool mFoo:1;| instead of |PRBool mFoo;|, for member data

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Daniel Holbert [:dholbert]]

I just noticed that the imgContainer class has 11 PRBool member-variables.  I think these should be declared as "PRPackedBool mFoo:1;" (and clustered together), to save space.  

Note that each PRBool occupies a full word, whereas a PRPackedBool is only a single unsigned byte (or a bit, if you use :1).

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: fix

This patch builds fine on my machine.  Running it through tryserver now, to make sure it doesn't mysteriously break any tests.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: fix v1a

(tweaked a comment)

Comment 3

[Joe Drew (not getting mail)]

Comment on attachment 444731 [details] [diff] [review]
fix v1a


>diff --git a/modules/libpr0n/src/imgContainer.h b/modules/libpr0n/src/imgContainer.h

>     //! Whether we can assume there will be no more frames
>     //! (and thus loop the animation)
>-    PRBool                     doneDecoding;
>+    PRPackedBool               doneDecoding;
>     //! Are we currently animating the image?
>-    PRBool                     animating;
>+    PRPackedBool               animating;

If we're going to be optimizing for packed size, you may as well reorder Anim so that the PRPackedBools come at the end, and we don't waste more space between them and the other members.

>+  // Boolean flags (clustered together to conserve space);

Same deal - put 'em at the end and you'll save more space!

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: fix v1b [r=joe]

Ok, moved PRPackedBools to end of class definition, for Anim & imgContainer.  Carrying forward r+.

Comment 5

[Daniel Holbert [:dholbert]]

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

Comment 6

[Johnny Stenback (:jst)]

The fix for this bug causes aborts in the imgContainer code in some cases (has to do with multipart-mixed-replace, but not sure what the exact requirements are). Reopening to track this, and fix coming up.

Comment 7

[Johnny Stenback (:jst)]

Attachment: Don't write bits other than the first one into 1-bit sized members.

The lack of this patch made this code set other members than what it intended to. This really was a pre-existing bug that was exposed by the initial fix for this bug, and I thought we had static analysis that caught stuff like this?

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 446941 [details] [diff] [review]
Don't write bits other than the first one into 1-bit sized members.

Good catch!

Comment 9

[Joe Drew (not getting mail)]

Comment on attachment 446941 [details] [diff] [review]
Don't write bits other than the first one into 1-bit sized members.

yee ikes

Comment 10

[Johnny Stenback (:jst)]

Followup landed as:

http://hg.mozilla.org/mozilla-central/rev/06c2864c6e4f