Pack StyleColor members in StyleRareInheritedData tighter to save memory

Pack StyleColor members in StyleRareInheritedData tighter to save memory

Since StyleColor stores the color value (32-bit RGBA) plus a single flag,
it is a bit inefficient.  In particular when multiple StyleColor objects
are members of another object, there will be a lot of unnecessary padding.

This padding can be avoided by storing the color value and the flag as
separate members of the containing structure, which allows the flags to
be packed into a bitfield.

The actual saving from this change, measured in a 64-bit Linux build, is
20 bytes per StyleRareInheritedData object.

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

[Jens Widell, 2014-03-24 11:39:52]

Please take a look when you have a moment.

Some comments:

- This change doesn't exactly make the code simpler and prettier, so this is an
early upload just to find out if you find it at all acceptable.

- The same optimization can be applied to StyleRareNonInheritedData, which
contains 8 StyleColor objects.

- At least in 64-bit Linux builds, another 16 bytes can be saved simply by
reordering remaining members in StyleRareInheritedData; I'll follow-up with that
change later.

- These objects are supposed to be rare, of course, so actual gain is limited. 
But you never know for sure, I guess.  The single-page HTML 5 specification (a
very typical web-page, for sure) has almost 2000 of them.

[eae, 2014-03-24 15:42:07]

On 2014/03/24 11:39:52, Jens Lindström wrote:
> Please take a look when you have a moment.
> 
> Some comments:
> 
> - This change doesn't exactly make the code simpler and prettier, so this is
an
> early upload just to find out if you find it at all acceptable.
> 
> - The same optimization can be applied to StyleRareNonInheritedData, which
> contains 8 StyleColor objects.
> 
> - At least in 64-bit Linux builds, another 16 bytes can be saved simply by
> reordering remaining members in StyleRareInheritedData; I'll follow-up with
that
> change later.
> 
> - These objects are supposed to be rare, of course, so actual gain is limited.

> But you never know for sure, I guess.  The single-page HTML 5 specification (a
> very typical web-page, for sure) has almost 2000 of them.

Wouldn't it be easier just to store Colors instead of StyleColors and have the
isCurrent as separate flags? This code adds a lot of extra complexity to support
preserving the StyleColor type.

[Jens Widell, 2014-03-24 15:58:41]

On 2014/03/24 15:42:07, eae wrote:

> Wouldn't it be easier just to store Colors instead of StyleColors and have the
> isCurrent as separate flags? This code adds a lot of extra complexity to
support
> preserving the StyleColor type.

You mean dropping the StyleColor type entirely and split the RenderStyle
accessors from (for instance)

  StyleColor textFillColor() const { ... }

to

  Color textFillColor() const { ... }
  bool textFillColorIsCurrentColor() const { ... }

?

Or do you mean doing something more similar to what I'm doing here, just with
less magic?  That is, more like what's done for a single color in the
BorderValue type.

[eae, 2014-03-24 16:07:55]

On 2014/03/24 15:58:41, Jens Lindström wrote:
> On 2014/03/24 15:42:07, eae wrote:
> 
> > Wouldn't it be easier just to store Colors instead of StyleColors and have
the
> > isCurrent as separate flags? This code adds a lot of extra complexity to
> support
> > preserving the StyleColor type.
> 
> You mean dropping the StyleColor type entirely and split the RenderStyle
> accessors from (for instance)
> 
>   StyleColor textFillColor() const { ... }
> 
> to
> 
>   Color textFillColor() const { ... }
>   bool textFillColorIsCurrentColor() const { ... }
> 
> ?
> 
> Or do you mean doing something more similar to what I'm doing here, just with
> less magic?  That is, more like what's done for a single color in the
> BorderValue type.

Similar to what you're doing but with less magic (i.e. retain the API but store
it differently).

[Jens Widell, 2014-03-24 16:12:45]

On 2014/03/24 16:07:55, eae wrote:

> Similar to what you're doing but with less magic (i.e. retain the API but
store
> it differently).

Okay, I'll upload a simplified version.

[eae, 2014-03-24 16:12:58]

On 2014/03/24 16:12:45, Jens Lindström wrote:
> On 2014/03/24 16:07:55, eae wrote:
> 
> > Similar to what you're doing but with less magic (i.e. retain the API but
> store
> > it differently).
> 
> Okay, I'll upload a simplified version.

Thanks!

[Jens Widell, 2014-03-24 16:43:46]

On 2014/03/24 16:12:58, eae wrote:
> On 2014/03/24 16:12:45, Jens Lindström wrote:
> > On 2014/03/24 16:07:55, eae wrote:
> > 
> > > Similar to what you're doing but with less magic (i.e. retain the API but
> > store
> > > it differently).
> > 
> > Okay, I'll upload a simplified version.
> 
> Thanks!

Uploaded.  This version saves another 8 bytes on 64-bit Linux, since the 6
isCurrentColor bits could be packed into the existing bitfield.  (But I suspect
those 8 bytes are half of the 16 bytes I claimed to additionally be saved by
reordering members.)

Ideally, the Color members I renamed and bitfield members I added ought to be
private, since there are now accessor functions that should be used to read and
write them.  Doing that as the class looks today would probably reintroduce some
padding and thus lose some of the savings.  It might be possible to arrange the
other members in a way that achieves both access control and optimum packing,
but I don't know how much we care about access control in these classes.  Do we?

[eae, 2014-03-24 16:46:16]

On 2014/03/24 16:43:46, Jens Lindström wrote:
> On 2014/03/24 16:12:58, eae wrote:
> > On 2014/03/24 16:12:45, Jens Lindström wrote:
> > > On 2014/03/24 16:07:55, eae wrote:
> > > 
> > > > Similar to what you're doing but with less magic (i.e. retain the API
but
> > > store
> > > > it differently).
> > > 
> > > Okay, I'll upload a simplified version.
> > 
> > Thanks!
> 
> Uploaded.  This version saves another 8 bytes on 64-bit Linux, since the 6
> isCurrentColor bits could be packed into the existing bitfield.  (But I
suspect
> those 8 bytes are half of the 16 bytes I claimed to additionally be saved by
> reordering members.)

Thanks, a bit more code but less magic. I like it, LGTM.

> Ideally, the Color members I renamed and bitfield members I added ought to be
> private, since there are now accessor functions that should be used to read
and
> write them.  Doing that as the class looks today would probably reintroduce
some
> padding and thus lose some of the savings.  It might be possible to arrange
the
> other members in a way that achieves both access control and optimum packing,
> but I don't know how much we care about access control in these classes.  Do
we?

Would be nice to have them private but we can deal with that in a later change.

[eseidel, 2014-03-24 17:04:32]

How many StyleRareInheritedData objects do we have in a typical page?

[Jens Widell, 2014-03-24 17:09:22]

On 2014/03/24 17:04:32, eseidel wrote:
> How many StyleRareInheritedData objects do we have in a typical page?

I browsed randomly on a few sites and saw a few 100 at most. So this particular
patch will in no way save massive amounts of memory.

[eseidel, 2014-03-24 22:46:55]

Also, if we're going to reduce memory in Blink we should have a bug to relate
all these changes to so we can CC the right folks and coordinate our efforts
between offices/countries. :)  One of the things I learned when looking into
Chrome memory usage before is that it can be surprising where its going. :)  We
may for example save much more memory by splitting this object into pieces based
on usage instead of breaking through the StyleColor abstraction.

[Jens Widell, 2014-03-25 07:29:52]

On 2014/03/24 22:46:55, eseidel wrote:
> Also, if we're going to reduce memory in Blink we should have a bug to relate
> all these changes to so we can CC the right folks and coordinate our efforts
> between offices/countries. :)  One of the things I learned when looking into
> Chrome memory usage before is that it can be surprising where its going. :)

If you meant having a very broad "reduce all memory usage" kind of bug, maybe
it'd be better reported by someone a bit more central than me.  You, perhaps? 
:-)  A bug about packing this type of data structures I can report myself, of
course.

[Jens Widell, 2014-03-25 12:30:52]

On 2014/03/25 07:29:52, Jens Lindström wrote:
> On 2014/03/24 22:46:55, eseidel wrote:
> > Also, if we're going to reduce memory in Blink we should have a bug to
relate
> > all these changes to so we can CC the right folks and coordinate our efforts
> > between offices/countries. :)  One of the things I learned when looking into
> > Chrome memory usage before is that it can be surprising where its going. :)
> 
> If you meant having a very broad "reduce all memory usage" kind of bug, maybe
> it'd be better reported by someone a bit more central than me.  You, perhaps? 
> :-)  A bug about packing this type of data structures I can report myself, of
> course.

Reported https://code.google.com/p/chromium/issues/detail?id=356124 to link this
particular change to, and changes that I intend to follow up with; unless
someone wants to take a step back and discuss the overall approach first.

[Jens Widell, 2014-03-26 08:00:51]

The CQ bit was checked by jl@opera.com

[commit-bot: I haz the power, 2014-03-26 08:00:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jl@opera.com/196103013/40001

[commit-bot: I haz the power, 2014-03-26 08:52:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-26 08:52:15]

Try jobs failed on following builders:
  tryserver.blink on win_blink_rel

[Jens Widell, 2014-03-26 09:11:21]

The CQ bit was checked by jl@opera.com

[commit-bot: I haz the power, 2014-03-26 09:11:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jl@opera.com/196103013/40001

[commit-bot: I haz the power, 2014-03-26 09:46:10]

Change committed as 170038