328571 - Change the type of PRBool SVG class members to PRPackedBool

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jonathan Watt [:jwatt]]

PRBool is 4 bytes whereas PRPackedBool is 1 byte, so we should always use the latter for class members.

We should also use bit fields where appropriate, but not having much experience with bit fields I'm unsure when that is. I can find lots of docs that say bit fields can be non-portable and cause you to use more space (and runtime code) than if you hadn't used them. Unfortunately I can't find any that explain when this can happen. I also notice that in our source we have lots of places where adjacent PRPackedBool class members are used with bit fields, and lots of places where they aren't. For the moment I'll go with using a bit field for adjacent members, and no bit field for single members. At least until someone can shed some light on this for me.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 213176 [details] [diff] [review]
patch

You should learn the C struct alignment/padding rules. I don't have a reference but someone must have explained it on the Web.

Anyway a good rule of thumb is that bools should go last in the class, and should be PRPackedBool, unless there are more than four consecutive bools in which case they should be :1 bitfields. (Almost all classes are padded to at least a multiple of 4 bytes, so 4 PRPackedBools doesn't really use any more space than 4 PRPackedBool :1.

Comment 3

[Jonathan Watt [:jwatt]]

Comment on attachment 213176 [details] [diff] [review]
patch

No need for a reference, the terms to search on should be enough. Thanks.

Comment 4

[Robert Longson [:longsonr]]

The basic rule of thumb is to put things in sizeof order from big to small.

You can't take the address of a bitfield so if you need to do that a bitfield is not appropriate, the compiler will refuse to compile the code.

Non portability is mainly an issue when bitfields are used in unions.

The compiler has to mask out bits to assign a bitfield which it does not with a non bitfield assignment hence more (code) space. It's probably worth trying to avoid having bitfields cross byte boundaries where possible e.g. x:7, y:7, z:1 would be better as x:7, z:1 y:7.

There are more opportunities to save space by changing the types of variables used to store data to ones which are sufficient for the values allowed. E.g. nsSVGLength has a PRUint16 mSpecifiedUnitType; field but this variable is used to store values only in the following range: 0 (SVG_LENGTHTYPE_UNKNOWN) to 10 (SVG_LENGTHTYPE_PC) 

Thus you could use PRUint8 mSpecifiedUnitType : 5; for this variable. You might want to add a NS_ASSERT in the constructor to catch invalid constructions (c.f. the recent change to nsSVGMarkerElement.cpp)

Comment 5

[Robert Longson [:longsonr]]

I meant PRUint8 mSpecifiedUnitType : 4;

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #4)
> The compiler has to mask out bits to assign a bitfield which it does not with
> a non bitfield assignment hence more (code) space.

Because of this, it's best to only use bitfields when you know that you will save space even after the class has been padded, and for bools that leads to my comment #2.

Comment 7

[Jonathan Watt [:jwatt]]

Attachment: patch v2

Yup, that's what I read and all that makes sense. I just also came across somewhere that specifically said you can unintentionally end up making your _objects_ bigger by using bit fields (as well as resulting in more complex code for packing and unpacking). That makes less sense to me. Anyway, on with this bug, here's a new patch...

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 213563 [details] [diff] [review]
patch v2

I'll take the liberty of usurping r+ for what in the end is a very simple patch... but thanks for doing it!

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Actually it just occurred to me that you could get rid of these PRPackedBools altogether by using frame state bits :-|. Let's get this in and file another bug for that...

Comment 10

[Jonathan Watt [:jwatt]]

Checked in on trunk. I filed bug 328950 for using frame state bits instead of PRPackedBool.

Comment 11

[Jonathan Watt [:jwatt]]

> I'll take the liberty of usurping r+ for what in the end is a very simple
> patch... but thanks for doing it!

Thanks for pointing out the issue. I learnt something new. :-)

Comment 12

[Gabriel Sjoberg]

Does anyone have any stats on how much improvement we actually gain by making this change?

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Not much. 4 bytes per nsSVGPathSeg, 4 bytes per <foreignObject>, 4 bytes per <svg>, 8 bytes per nsSVGTextFrame. On the other hand it wasn't hard to get ... low effort, low reward :-)