use nsAutoTArray for nsCSSDeclaration::mOrder

RESOLVED FIXED

Status

()

P1
normal
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: dwitte, Assigned: dwitte)

Tracking

({memory-footprint, perf})

Trunk
memory-footprint, perf
Points:
---
Bug Flags:
blocking1.9 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

11 years ago
Created attachment 294978 [details]
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...
(Assignee)

Comment 1

11 years ago
Created attachment 294979 [details] [diff] [review]
patch v1
Attachment #294979 - Flags: superreview?(dbaron)
Attachment #294979 - Flags: review?(dbaron)
(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
No longer blocks: 410360
(Assignee)

Comment 3

11 years ago
Created attachment 294985 [details] [diff] [review]
patch v1.1

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

thanks ted!
Attachment #294979 - Attachment is obsolete: true
Attachment #294985 - Flags: superreview?(dbaron)
Attachment #294985 - Flags: review?(dbaron)
Attachment #294979 - Flags: superreview?(dbaron)
Attachment #294979 - Flags: review?(dbaron)

Comment 4

11 years ago
Moving to blocking list so we can wrap up footprint work...
Flags: blocking1.9+
Priority: -- → P1
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
Attachment #294985 - Flags: superreview?(dbaron)
Attachment #294985 - Flags: superreview+
Attachment #294985 - Flags: review?(dbaron)
Attachment #294985 - Flags: review+
(Assignee)

Comment 6

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

Comment 8

11 years ago
checked for additional warnings (gcc 4.1.2), found none, and checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.