712865 - Shrink CDBValueStorage

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Nicholas Nethercote [inactive]]

We have *tons* of CDBValueStorages in compressed data blocks.  Opening gmail in a 64-bit build I get 2MB+ worth of them.  It's defined like this:

  struct CDBValueStorage {
    nsCSSProperty property;
    nsCSSValue value;
  };

nsCSSProperty is an enum.  nsCSSValue has two fields, an enum, and a word-sized union.  On 32-bit platforms sizeof(CDBValueStorage) is 12 bytes, on 64-bit platforms it is 24 bytes.  The fact that each enum value takes up a word is wasteful.  If we inlined the nsCSSProperty into the nsCSSValue, and made both enum values 16 bits, we'd reduce memory usage by 1/3 -- 8 bytes on 32-bits, 12 bytes on 64-bits.

nsCSSValue is used in other places not in conjunction with nsCSSProperty;  in those cases the inlined field could just be zero, it'd be ignored anyway.

Comment 1

[Nicholas Nethercote [inactive]]

I tried putting an nsCSSProperty into nsCSSValue, and also creating a new type nsCSSPropertyAndValue, but neither of them worked nicely -- nsCSSCompressedDataBlock is designed very much around having an nsCSSProperty and an nsCSSValue, and changing this is very difficult.

I also investigated __attribute__((packed)) under GCC.  I can get CDBValueStorage down to 16 bytes on 64-bit if we are willing for nsCSSValue::mValue to only have 4-byte alignment.  But that doesn't help on 32-bit.  And we don't have cross-platform wrapping for packing structs, though MSVC also has pragmas for this so it might be possible.

Another possibility is to just store the properties separately from the values. (dbaron suggested this in bug 681161, it turns out.)  With 16-bit enums on 64-bit we'd drop from 24 bytes to 18 bytes (a 25% drop), and on 32-bit we'd drop from 12 bytes to 10 bytes (a 17% drop).  Not as good as 33%, but still reasonable.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: patch

This patch stores the properties and the values separately.  It reduces the memory used for Gmail by about 0.34MB on 64-bit, which is about 15%.  This is a bit lower than I was hoping, due to (a) the 8-byte nsCSSCompressedDataBlock header, which I forgot to account for, and (b) jemalloc's rounding up of request sizes.  Still, better than nothing.

Also, I find storing the number of properties and using an index to iterate through the CDBs instead of a cursor much easier to read.  (Look at Clone() for a particularly good example.)  The patch reduces code size by 72 lines.

Comment 3

[Nicholas Nethercote [inactive]]

Oh, due to an mq stuff-up this patch is missing one crucial piece:  it needs to change nsCSSProperty to |enum nsCSSProperty : PRInt16| to save another 2 bytes per value.

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: patch v2

> Oh, due to an mq stuff-up this patch is missing one crucial piece:  it needs
> to change nsCSSProperty to |enum nsCSSProperty : PRInt16| to save another 2
> bytes per value.

It turns out my 0.34MB Gmail reduction measurement was without this change;  with it, the reduction becomes 0.53MB.  The updated patch is attached.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Nicholas Nethercote [:njn] from comment #2)
> Also, I find storing the number of properties and using an index to iterate
> through the CDBs instead of a cursor much easier to read.  (Look at Clone()
> for a particularly good example.)  The patch reduces code size by 72 lines.

The old way of doing it was the only way that made sense prior to bug 576044; now that we have only one size thing stored in data blocks this is a clear improvement.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

nsCSSProperty.h
===============

>-enum nsCSSProperty {
>+enum nsCSSProperty : PRInt16 {

Boy, I didn't know you could do this.  Then again, it looks like you
couldn't until C++2011, so I'm not that far behind (yet).

nsCSSDataBlock.h
================

Call the ZeroNumProps() method SetNumPropsToZero().  Right now it's
unclear from the name whether it is "bool ZeroNumProps() const" or "void
ZeroNumProps()".


How about renaming {Raw,}CopyValueAtIndex to {Raw,}CopyValueToIndex
(i.e., At -> To)?

In DataSize, could you add a PR_STATIC_ASSERT that:
 1.  sizeof(this) % NS_ALIGNMENT_OF(nsCSSValue) == 0
 2.  NS_ALIGNMENT_OF(nsCSSValue) % NS_ALIGNMENT_OF(nsCSSProperty) == 0

ValueAtIndex() could probably just use (this + 1) instead of
((uintptr_t)this + sizeof(*this)).  And it should definitely use
"(...) + i" instead of "&(...)[i]".

All the *AtIndex / *ToIndex methods should probably assert that the
given index is less than mNumProps.  This would require making
PropertyAtIndex() use (ValueAtIndex(0) + mNumProps) instead of
ValueAtIndex(mNumProps), but I think that's worth it.  Maybe have helper
methods call Values() and Properties() that return pointers?

>+     * Compute the number of properties that will be present in the result of

Could you wrap comments at 72 characters?

nsCSSDataBlock.cpp
==================

In nsCSSDataBlock::DoExpand:

Don't remove this line:
>-        NS_ABORT_IF_FALSE(!nsCSSProps::IsShorthand(iProp), "out of range");

Not clear to me why you're changing ComputeNumProps, but I guess it's
ok, though it might be a little slower.


r=dbaron with those things fixed

Comment 7

[Jeff Walden [:Waldo]]

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

Review of attachment 586040 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/nsCSSProperty.h
@@ +52,2 @@
>   */
> +enum nsCSSProperty : PRInt16 {

Have you determined that this works with all the compilers we support?  I'd been under the impression it didn't, but I'd be happy to learn otherwise.

Comment 8

[Nicholas Nethercote [inactive]]

> > +enum nsCSSProperty : PRInt16 {
> 
> Have you determined that this works with all the compilers we support?  I'd
> been under the impression it didn't, but I'd be happy to learn otherwise.

Looks like you're right -- the g++-4.2 on Macs can't handle it:

/builds/slave/try-osx-dbg/build/layout/style/nsCSSProperty.h:53: error: use of enum 'nsCSSProperty' without previous declaration
/builds/slave/try-osx-dbg/build/layout/style/nsCSSProperty.h:53: error: expected unqualified-id before ':' token

Sigh.

Comment 9

[Nicholas Nethercote [inactive]]

> Could you wrap comments at 72 characters?

This surprised me... why wrap at 72 chars?  I just asked about this on #developers, everyone present was surprised at the notion.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Why not?

You could do 78 if you want, but I prefer 72, and I object to anything greater than 78.

Comment 11

[Nicholas Nethercote [inactive]]

78 seems the sensible number, esp. since it matches the code.  72 just makes for needlessly short lines.

Comment 12

[Nicholas Nethercote [inactive]]

Attachment: patch v3

Updated patch.  Because |enum X : PRInt16| isn't supported by all the compilers we use, I introduced a new type CompressedCSSProperty that is used within nsCSSCompressedDataBlock.  It's a 16-bit representation of an nsCSSProperty.  I also added eCSSProperty_REAL_COUNT in order to write a static assertion that the compression is always valid.

Comment 13

[Nicholas Nethercote [inactive]]

Unfortunately both Bugzilla's interdiff and my machine's local interdiff failed when comparing patch 2 and patch 3.  The most important differences are in the .h files.

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I don't use interdiff anyway; just diffing diffs works much better once you learn how to read the output.

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 607041 [details] [diff] [review]
patch v3

>+    // Ideally, |nsCSSProperty| would be |enum nsCSSProperty : PRInt16|.  But
>+    // not all of the compilers we use are modern enough to support small
>+    // enums.  So we manually squeeze nsCSSProperty into 16 bits ourselves.
>+    // The static assertion below ensures it fits.  The -1 in
>+    // MaxCompressedCSSProperty is for the sign bit, because nsCSSProperty can
>+    // be -1.
>+    typedef PRInt16 CompressedCSSProperty;
>+    static const size_t MaxCompressedCSSProperty =
>+        (1 << (8 * sizeof(CompressedCSSProperty) - 1)) - 1;

This isn't safe; you're using PRInt16 so at the very least the conversion back to a larger signed type will fail for anything larger than PR_INT16_MAX (since such a value would be represented as a negative in CompressedCSSProperty).  Just make MaxCompressedCSSProperty an alias for PR_INT16_MAX instead.  There's tons of room for that.

(In fact, it looks like there are currently 239 properties, so you could use a PRUint8 if you wanted to; we'll never use the -1 value nor the values >= eCSSProperty_COUNT_no_shorthands here.  But that probably won't last a whole lot longer, so I think it's probably best to leave it as is.)

>-    PRInt32 mStyleBits; // the structs for which we have data, according to
>-                        // |nsCachedStyleData::GetBitForSID|.
>+    PRInt32 mStyleBits; // the structs for which we have data, according
>+                        // to |nsCachedStyleData::GetBitForSID|.

Don't rewrap the "to" here.  It'll keep blame and make the diff substantially less confusing.

>+    // nsCSSValue elements are stored after these fields, and

Leave mNumProps above this comment, right below mStyleBits.  (Was this a merge error that just happened to compile?)

>+MOZ_STATIC_ASSERT(NS_ALIGNMENT_OF(nsCSSValue) == 4 ||
>+                  NS_ALIGNMENT_OF(nsCSSValue) == 8,
>+                  "nsCSSValue doesn't align with nsCSSCompressedDataBlock"); 
>+MOZ_STATIC_ASSERT(NS_ALIGNMENT_OF(nsCSSCompressedDataBlock::CompressedCSSProperty) == 2,
>+                  "CompressedCSSProperty doesn't align with nsCSSValue"); 

These might be a little stronger than you need.

>+// Make sure that sizeof(CompressedCSSProperty) is big enough.  The -1 is for
>+// the sign.
>+MOZ_STATIC_ASSERT(eCSSProperty_REAL_COUNT <=
>+                  nsCSSCompressedDataBlock::MaxCompressedCSSProperty,
>+                  "nsCSSProperty doesn't fit in StoredSizeOfCSSProperty");

Don't add the REAL_COUNT value.  The only nsCSSProperty values in data blocks should be <= eCSSProperty_COUNT_no_shorthands; you should use that here instead.

>diff --git a/layout/style/nsCSSProperty.h b/layout/style/nsCSSProperty.h

> /*
>    Declare the enum list using the magic of preprocessing
>    enum values are "eCSSProperty_foo" (where foo is the property)
> 
>    To change the list of properties, see nsCSSPropList.h
>-
>  */

Remove this whitespace change.

>   // Extra dummy values for nsCSSParser internal use.
>   eCSSPropertyExtra_x_none_value,
>-  eCSSPropertyExtra_x_auto_value
>+  eCSSPropertyExtra_x_auto_value,
>+
>+  eCSSProperty_REAL_COUNT   // This *must* be the last value.
> };

And, as I said above, remove this change.

Then you'll have no changes to this file.


r=dbaron with that

Comment 16

[Nicholas Nethercote [inactive]]

Thanks for the lightning fast review! :)

Comment 17

[Nicholas Nethercote [inactive]]

Landed and then backed out due to build bustage:

https://hg.mozilla.org/integration/mozilla-inbound/rev/c880f229fbf5
https://hg.mozilla.org/integration/mozilla-inbound/rev/090335f73382

Comment 18

[Nicholas Nethercote [inactive]]

Turns out the build bustage (comment 17) was just bad luck -- MOZ_STATIC_ASSERT is slightly broken on Mac and if you create static assertions on the same line number in two different files under unusual circumstances you get a compile error.

Second attempt at landing:

https://hg.mozilla.org/integration/mozilla-inbound/rev/6dd5b6014511

Comment 19

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/6dd5b6014511

Comment 20

[Nathan Froyd [:froydnj]]

(In reply to Nicholas Nethercote [:njn] from comment #8)
> > > +enum nsCSSProperty : PRInt16 {
> > 
> > Have you determined that this works with all the compilers we support?  I'd
> > been under the impression it didn't, but I'd be happy to learn otherwise.
> 
> Looks like you're right -- the g++-4.2 on Macs can't handle it:

I see that this problem has been solved differently, but I wanted to suggest that this style of bitfield enum might be useful in other situations, so you could have (in MFBT?) defined MOZ_ENUM_BITFIELD(enum, datatype) and then just made the appropriate compilers ignore the datatype field.