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

RESOLVED FIXED

Status

()

Core
ImageLib
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
x86
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

8 years ago
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).
(Assignee)

Comment 1

8 years ago
Created attachment 444729 [details] [diff] [review]
fix

This patch builds fine on my machine.  Running it through tryserver now, to make sure it doesn't mysteriously break any tests.
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attachment #444729 - Flags: review?(joe)
(Assignee)

Comment 2

8 years ago
Created attachment 444731 [details] [diff] [review]
fix v1a

(tweaked a comment)
Attachment #444729 - Attachment is obsolete: true
Attachment #444729 - Flags: review?(joe)
(Assignee)

Updated

8 years ago
Attachment #444731 - Flags: review?(joe)
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!
Attachment #444731 - Flags: review?(joe) → review+
(Assignee)

Comment 4

8 years ago
Created attachment 444764 [details] [diff] [review]
fix v1b [r=joe]

Ok, moved PRPackedBools to end of class definition, for Anim & imgContainer.  Carrying forward r+.
Attachment #444731 - Attachment is obsolete: true
Attachment #444764 - Flags: review+
(Assignee)

Comment 5

8 years ago
Pushed: http://hg.mozilla.org/mozilla-central/rev/cd6413bdc6de
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Created attachment 446941 [details] [diff] [review]
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?
Attachment #446941 - Flags: review?(dholbert)

Updated

8 years ago
Attachment #446941 - Flags: review?(joe)
(Assignee)

Comment 8

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

Good catch!
Attachment #446941 - Flags: review?(dholbert) → review+
Comment on attachment 446941 [details] [diff] [review]
Don't write bits other than the first one into 1-bit sized members.

yee ikes
Attachment #446941 - Flags: review?(joe) → review+
Followup landed as:

http://hg.mozilla.org/mozilla-central/rev/06c2864c6e4f
Status: REOPENED → RESOLVED
Last Resolved: 8 years ago8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.