Make sure ImmutableStylePropertySet uses the new 16 bits size of StylePropertyMetadata.

Make sure ImmutableStylePropertySet uses the new 16 bits size of StylePropertyMetadata.
    
We know that StylePropertyMetadata is 16 bits so we can use uint16_t as the type for the bitfields so that sizeof(StylePropertyMetadata) is equals to 2 bytes.
    
Cleanup two casts not necessary at the same time.
    
This should save up ~1.8Mb on Apple membuster test.

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=152910

[abarth-chromium, 2013-06-21 18:55:05]

@jyasskin: Would you be willing to help us out with the CL?

What we're trying to do is store a bunch of StylePropertyMetadata objects in
memory.  Each StylePropertyMetadata only takes 16 bits to represent, so we'd
like to only use N * 16 bits to represent N StylePropertyMetadata. 
Unfortunately, sizeof(StylePropertyMetadata) is 4, which means we need to do
some manual packing and unpacking.

Is there a sane way of doing this that won't make the compiler's author sad?

[Jeffrey Yasskin, 2013-06-21 19:56:27]

https://codereview.chromium.org/17448021/diff/1/Source/core/css/CSSProperty.h
File Source/core/css/CSSProperty.h (right):

https://codereview.chromium.org/17448021/diff/1/Source/core/css/CSSProperty.h...
Source/core/css/CSSProperty.h:46: memcpy(this, &data,
sizeof(StylePropertyMetadata));
memcpy is dangerous because of endian-ness issues, so either be sure to test
this on a big-endian machine (to be sure that data is always an in-memory value)
or do the shifting yourself in this constructor.

https://codereview.chromium.org/17448021/diff/1/Source/core/css/CSSProperty.h...
Source/core/css/CSSProperty.h:51: unsigned m_propertyID : 10;
If you change the type of these fields to uint16_t, that should reduce
sizeof(StylePropertyMetadata) to 2 on x86, and probably on all 8-bit-byte
architectures. (Inferring from "bit-fields must be contained in a storage unit
appropriate for its declared type" in the AMD64 ABI v0.99, which I can't find on
the internet anymore. :( )

This probably obviates the rest of the CL, but I've left my C++-correctness
comments anyway.

https://codereview.chromium.org/17448021/diff/1/Source/core/css/StyleProperty...
File Source/core/css/StylePropertySet.cpp (right):

https://codereview.chromium.org/17448021/diff/1/Source/core/css/StyleProperty...
Source/core/css/StylePropertySet.cpp:87: metadataArray[i] =
*(reinterpret_cast<uint16_t*>(&metadata));
That reinterpret_cast is undefined behavior; use memcpy instead.

https://codereview.chromium.org/17448021/diff/1/Source/core/css/StyleProperty...
File Source/core/css/StylePropertySet.h (right):

https://codereview.chromium.org/17448021/diff/1/Source/core/css/StyleProperty...
Source/core/css/StylePropertySet.h:173: return reinterpret_cast<const
uint16_t*>(&reinterpret_cast<const char*>((&static_cast<const
ImmutableStylePropertySet*>(this)->m_storage))[m_arraySize *
sizeof(CSSValue*)]);
Do you actually need all these casts? 'this' should already be a "const
ImmutableStylePropertySet*". You only need a static_cast to go from void* to
const char*. Only the char*->uint16_t needs a reinterpret_cast.

[abarth-chromium, 2013-06-21 21:38:04]

LGTM.  Yay for not fighting the language :)

[darktears, 2013-06-21 21:38:44]

On 2013/06/21 21:38:04, abarth wrote:
> LGTM.  Yay for not fighting the language :)

The merit goes to Jeffrey for his suggestion :D.

[commit-bot: I haz the power, 2013-06-21 21:39:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexis.menard@intel.com/17448021/11001

[esprehn, 2013-06-21 21:39:38]

Way better! Thanks for fixing this. :)

[commit-bot: I haz the power, 2013-06-21 23:15:39]

Change committed as 152910