410357 - use nsAutoTArray for nsCSSDeclaration::mOrder

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P1)

Description

[[e-mail]]

Attachment: mOrder size histogram

this is one of the two in-tree consumers of nsValueArray, and would be better off as an nsTArray<PRUint8> - we lose runtime sizing of the elements, but we save 4 bytes per instance by not having to store the size. it turns out we create a lot of these things (1400 on startup alone, but 8000 loading gmail - which is relatively CSS-intensive, i guess). so avoiding allocations here would also be nice.

i collected data on the distribution of maximum mOrder sizes for each instance, when loading gmail (see attachment). the total memory consumed by all 8000 mOrder arrays, making reasonable assumptions about malloc overhead and taking into account the array growth algorithms, would be:

using present nsValueArray:        278kb
using an nsTArray<PRUint8>:        224kb
using an nsAutoTArray<PRUint8, 8>: 230kb

and in the latter case, we'd avoid allocations in about 80% of cases. (the reason it's only a small increase over nsTArray is because we don't pay any malloc overhead for array sizes <= 8 entries, which mostly offsets the overhead for the auto storage when we aren't using it.) so, i think this a pretty good tradeoff to make.

now, about the runtime sizing - we currently have 251 CSS properties defined, including everything (shorthand, internal, and SVG). taking out the shorthand, which mOrder isn't interested in, we have 209 or so. so this can comfortably fit in 8 bits now. i can't see an easy way to check that 209 < 256 at compile time... instead, we could add an assertion in the constructor, and it'd fire like crazy if anyone were to actually hit it.

of course, it'd be nice to remove mOrder completely (bug 208727), since the nsCSSDeclaration::ValueAppended() codepath gets hit about 50,000 times loading gmail. but i don't know if that can be done...

Comment 1

[[e-mail]]

Attachment: patch v1

Comment 2

[(not currently active) Ted Mielczarek]

(In reply to comment #0)
> now, about the runtime sizing - we currently have 251 CSS properties defined,
> including everything (shorthand, internal, and SVG). taking out the shorthand,
> which mOrder isn't interested in, we have 209 or so. so this can comfortably
> fit in 8 bits now. i can't see an easy way to check that 209 < 256 at compile
> time... instead, we could add an assertion in the constructor, and it'd fire
> like crazy if anyone were to actually hit it.

I think you want PR_STATIC_ASSERT, for example:
http://lxr.mozilla.org/mozilla/source/xpcom/ds/nsExpirationTracker.h#109

Comment 3

[[e-mail]]

Attachment: patch v1.1

oh wow! that's the neatest thing i've seen all day :)

thanks ted!

Comment 4

[Mike Schroepfer]

Moving to blocking list so we can wrap up footprint work...

Comment 5

[David Baron :dbaron:]

Comment on attachment 294985 [details] [diff] [review]
patch v1.1

You've checked for warnings on a gcc that warns about signed/unsigned comparisons, right?

>+  PR_STATIC_ASSERT(eCSSProperty_COUNT_no_shorthands <= PR_UINT8_MAX + 1);

Why the +1 ?


With that, r+sr=dbaron

Comment 6

[[e-mail]]

(In reply to comment #5)
> You've checked for warnings on a gcc that warns about signed/unsigned
> comparisons, right?

will check before landing.

> >+  PR_STATIC_ASSERT(eCSSProperty_COUNT_no_shorthands <= PR_UINT8_MAX + 1);
> 
> Why the +1 ?

eCSSProperty_COUNT_no_shorthands is the maximum eCSSProperty enum value +1, and it's fine for that max value to be 255, right? maybe this would be clearer as |eCSSProperty_COUNT_no_shorthands - 1 <= PR_UINT8_MAX|.

Comment 7

[David Baron :dbaron:]

(In reply to comment #6)
> > >+  PR_STATIC_ASSERT(eCSSProperty_COUNT_no_shorthands <= PR_UINT8_MAX + 1);
> > 
> > Why the +1 ?
> 
> eCSSProperty_COUNT_no_shorthands is the maximum eCSSProperty enum value +1, and
> it's fine for that max value to be 255, right? maybe this would be clearer as
> |eCSSProperty_COUNT_no_shorthands - 1 <= PR_UINT8_MAX|.

Er, right.  Yeah, it probably would be clearer that way.

Comment 8

[[e-mail]]

checked for additional warnings (gcc 4.1.2), found none, and checked in.