cc: Reduce size of cc::ResourceProvider::Resource class

cc: Reduce size of cc::ResourceProvider::Resource class

The compositor will at times keep several hundreds instances of this
class when a tab is visible. This patch reduces the size of every
instance by 20 bytes (from 300 bytes to 280 bytes) on 32 bit ARM
compiled with GCC 4.6.

Also adds missing initializers for a field added in
https://codereview.chromium.org/197223003

BUG=

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=259319

[jadahl, 2014-03-21 10:32:52]

Hi,

This patch makes the cc::ResourceProvider::Resource class more compact. I'm not
sure you are interested in this kind of optimizations, but uploading just in
case. I thought of the idea of figuring out how to split the fields into
different sub-structs and unions, as I suspect not every field is used by every
resource but thats another story.

Jonas

[danakj, 2014-03-21 16:04:22]

https://codereview.chromium.org/207943002/diff/1/cc/resources/resource_provid...
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/207943002/diff/1/cc/resources/resource_provid...
cc/resources/resource_provider.h:415: uint16 lock_for_read_count;
style guide says to use signed int except for things like size of an array which
returns size_of.

https://codereview.chromium.org/207943002/diff/1/cc/resources/resource_provid...
cc/resources/resource_provider.h:420: bool dirty_image : 1;
Do you have any data to show this affects our performance?

[jadahl, 2014-03-21 16:54:50]

https://codereview.chromium.org/207943002/diff/1/cc/resources/resource_provid...
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/207943002/diff/1/cc/resources/resource_provid...
cc/resources/resource_provider.h:415: uint16 lock_for_read_count;
On 2014/03/21 16:04:22, danakj wrote:
> style guide says to use signed int except for things like size of an array
which
> returns size_of.

I suppose a 16 bit signed int should be enough as well?

https://codereview.chromium.org/207943002/diff/1/cc/resources/resource_provid...
cc/resources/resource_provider.h:420: bool dirty_image : 1;
On 2014/03/21 16:04:22, danakj wrote:
> Do you have any data to show this affects our performance?

No, I have not done any measurements; do you really think this change could have
a measurable effect? Are the fields are already placed in order to optimize
cache behavior (I couldn't see that it was)?

[danakj, 2014-03-21 18:04:01]

https://codereview.chromium.org/207943002/diff/1/cc/resources/resource_provid...
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/207943002/diff/1/cc/resources/resource_provid...
cc/resources/resource_provider.h:420: bool dirty_image : 1;
On 2014/03/21 16:54:51, jadahl wrote:
> On 2014/03/21 16:04:22, danakj wrote:
> > Do you have any data to show this affects our performance?
> 
> No, I have not done any measurements; do you really think this change could
have
> a measurable effect? Are the fields are already placed in order to optimize
> cache behavior (I couldn't see that it was)?

No, they're not placed in any specific order. I don't mind moving things around
to pack them nicely, but I feel more resistant to CLs that nitpick at the size
of fields in structs without some data to show an improvement. Cuz there are an
infinite number of CLs that could be submitted to tweak the sizes of various
fields all over the place. And it would probably be mostly a waste of time, you
know?

[jadahl, 2014-03-24 09:54:26]

https://codereview.chromium.org/207943002/diff/1/cc/resources/resource_provid...
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/207943002/diff/1/cc/resources/resource_provid...
cc/resources/resource_provider.h:420: bool dirty_image : 1;
On 2014/03/21 18:04:01, danakj wrote:
> On 2014/03/21 16:54:51, jadahl wrote:
> > On 2014/03/21 16:04:22, danakj wrote:
> > > Do you have any data to show this affects our performance?
> > 
> > No, I have not done any measurements; do you really think this change could
> have
> > a measurable effect? Are the fields are already placed in order to optimize
> > cache behavior (I couldn't see that it was)?
> 
> No, they're not placed in any specific order. I don't mind moving things
around
> to pack them nicely, but I feel more resistant to CLs that nitpick at the size
> of fields in structs without some data to show an improvement. Cuz there are
an
> infinite number of CLs that could be submitted to tweak the sizes of various
> fields all over the place. And it would probably be mostly a waste of time,
you
> know?

The data I have is that I measured the maximum number of active instances
rendering; and how much less memory this struct would theoretically use by
making the size smaller. When I did the measurements the struct was smaller to
begin with, and the size improvement smaller, and at that time some (~6 KiB)
memory would *theoretically* be saved.

I could remove the uint16 bit if you'd like the ": 1" and grouping of bools, and
we'd still get a small improvement. It is not a big improvement, so we could
drop it as well if that what you think is the best option.

[danakj, 2014-03-24 16:18:02]

On Mar 24, 2014 5:54 AM, <jadahl@opera.com> wrote:
>
>
>
https://codereview.chromium.org/207943002/diff/1/cc/resources/resource_provid...
> File cc/resources/resource_provider.h (right):
>
>
https://codereview.chromium.org/207943002/diff/1/cc/resources/resource_provid...
> cc/resources/resource_provider.h:420: bool dirty_image : 1;
> On 2014/03/21 18:04:01, danakj wrote:
>>
>> On 2014/03/21 16:54:51, jadahl wrote:
>> > On 2014/03/21 16:04:22, danakj wrote:
>> > > Do you have any data to show this affects our performance?
>> >
>> > No, I have not done any measurements; do you really think this
>
> change could
>>
>> have
>> > a measurable effect? Are the fields are already placed in order to
>
> optimize
>>
>> > cache behavior (I couldn't see that it was)?
>
>
>> No, they're not placed in any specific order. I don't mind moving
>
> things around
>>
>> to pack them nicely, but I feel more resistant to CLs that nitpick at
>
> the size
>>
>> of fields in structs without some data to show an improvement. Cuz
>
> there are an
>>
>> infinite number of CLs that could be submitted to tweak the sizes of
>
> various
>>
>> fields all over the place. And it would probably be mostly a waste of
>
> time, you
>>
>> know?
>
>
> The data I have is that I measured the maximum number of active
> instances rendering; and how much less memory this struct would
> theoretically use by making the size smaller. When I did the
> measurements the struct was smaller to begin with, and the size
> improvement smaller, and at that time some (~6 KiB) memory would
> *theoretically* be saved.
>
> I could remove the uint16 bit if you'd like the ": 1" and grouping of
> bools, and we'd still get a small improvement. It is not a big
> improvement, so we could drop it as well if that what you think is the
> best option.

Grouping bools and : 1 sounds okay.

> https://codereview.chromium.org/207943002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jadahl, 2014-03-24 16:52:38]

Uploaded a new version that just groups bools and limits their field size to 1
bit.

[danakj, 2014-03-24 18:05:22]

lgtm

[jadahl, 2014-03-25 08:16:29]

The CQ bit was checked by jadahl@opera.com

[commit-bot: I haz the power, 2014-03-25 08:16:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jadahl@opera.com/207943002/20001

[commit-bot: I haz the power, 2014-03-25 08:16:40]

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

[commit-bot: I haz the power, 2014-03-25 08:16:40]

Failed to apply patch for cc/resources/resource_provider.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file cc/resources/resource_provider.cc
  Hunk #1 FAILED at 210.
  Hunk #2 succeeded at 255 (offset 1 line).
  Hunk #3 succeeded at 297 (offset 1 line).
  Hunk #4 succeeded at 340 (offset 1 line).
  1 out of 4 hunks FAILED -- saving rejects to file
cc/resources/resource_provider.cc.rej

Patch:       cc/resources/resource_provider.cc
Index: cc/resources/resource_provider.cc
diff --git a/cc/resources/resource_provider.cc
b/cc/resources/resource_provider.cc
index
1cb215bd20993c51ead0dc647575d86e6f535eb3..30dc77e0b60b508a1b5861f9f9ad4c3748771fa7
100644
--- a/cc/resources/resource_provider.cc
+++ b/cc/resources/resource_provider.cc
@@ -210,28 +210,28 @@ ResourceProvider::Resource::Resource()
       lock_for_read_count(0),
       imported_count(0),
       exported_count(0),
+      dirty_image(false),
       locked_for_write(false),
-      origin(Internal),
+      lost(false),
       marked_for_deletion(false),
       pending_set_pixels(false),
       set_pixels_completion_forced(false),
       allocated(false),
       enable_read_lock_fences(false),
+      has_shared_bitmap_id(false),
       read_lock_fence(NULL),
       size(),
+      origin(Internal),
       target(0),
       original_filter(0),
       filter(0),
       image_id(0),
       bound_image_id(0),
-      dirty_image(false),
       texture_pool(0),
       wrap_mode(0),
-      lost(false),
       hint(TextureUsageAny),
       type(InvalidType),
       format(RGBA_8888),
-      has_shared_bitmap_id(false),
       shared_bitmap(NULL) {}
 
 ResourceProvider::Resource::~Resource() {}
@@ -254,28 +254,28 @@ ResourceProvider::Resource::Resource(GLuint texture_id,
       lock_for_read_count(0),
       imported_count(0),
       exported_count(0),
+      dirty_image(false),
       locked_for_write(false),
-      origin(origin),
+      lost(false),
       marked_for_deletion(false),
       pending_set_pixels(false),
       set_pixels_completion_forced(false),
       allocated(false),
       enable_read_lock_fences(false),
+      has_shared_bitmap_id(false),
       read_lock_fence(NULL),
       size(size),
+      origin(origin),
       target(target),
       original_filter(filter),
       filter(filter),
       image_id(0),
       bound_image_id(0),
-      dirty_image(false),
       texture_pool(texture_pool),
       wrap_mode(wrap_mode),
-      lost(false),
       hint(hint),
       type(GLTexture),
       format(format),
-      has_shared_bitmap_id(false),
       shared_bitmap(NULL) {
   DCHECK(wrap_mode == GL_CLAMP_TO_EDGE || wrap_mode == GL_REPEAT);
   DCHECK_EQ(origin == Internal, !!texture_pool);
@@ -296,28 +296,28 @@ ResourceProvider::Resource::Resource(uint8_t* pixels,
       lock_for_read_count(0),
       imported_count(0),
       exported_count(0),
+      dirty_image(false),
       locked_for_write(false),
-      origin(origin),
+      lost(false),
       marked_for_deletion(false),
       pending_set_pixels(false),
       set_pixels_completion_forced(false),
       allocated(false),
       enable_read_lock_fences(false),
+      has_shared_bitmap_id(!!bitmap),
       read_lock_fence(NULL),
       size(size),
+      origin(origin),
       target(0),
       original_filter(filter),
       filter(filter),
       image_id(0),
       bound_image_id(0),
-      dirty_image(false),
       texture_pool(0),
       wrap_mode(wrap_mode),
-      lost(false),
       hint(TextureUsageAny),
       type(Bitmap),
       format(RGBA_8888),
-      has_shared_bitmap_id(!!bitmap),
       shared_bitmap(bitmap) {
   DCHECK(wrap_mode == GL_CLAMP_TO_EDGE || wrap_mode == GL_REPEAT);
   DCHECK(origin == Delegated || pixels);
@@ -339,28 +339,28 @@ ResourceProvider::Resource::Resource(const SharedBitmapId&
bitmap_id,
       lock_for_read_count(0),
       imported_count(0),
       exported_count(0),
+      dirty_image(false),
       locked_for_write(false),
-      origin(origin),
+      lost(false),
       marked_for_deletion(false),
       pending_set_pixels(false),
       set_pixels_completion_forced(false),
       allocated(false),
       enable_read_lock_fences(false),
+      has_shared_bitmap_id(true),
       read_lock_fence(NULL),
       size(size),
+      origin(origin),
       target(0),
       original_filter(filter),
       filter(filter),
       image_id(0),
       bound_image_id(0),
-      dirty_image(false),
       texture_pool(0),
       wrap_mode(wrap_mode),
-      lost(false),
       hint(TextureUsageAny),
       type(Bitmap),
       format(RGBA_8888),
-      has_shared_bitmap_id(true),
       shared_bitmap_id(bitmap_id),
       shared_bitmap(NULL) {
   DCHECK(wrap_mode == GL_CLAMP_TO_EDGE || wrap_mode == GL_REPEAT);

[jadahl, 2014-03-25 09:49:53]

New upload (rebased with conflicts). Also added some missing initializers for a
bool added by another patch (see updated description).

[danakj, 2014-03-25 16:15:57]

LGTM

[danakj, 2014-03-25 16:15:59]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-03-25 16:16:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jadahl@opera.com/207943002/30001

[commit-bot: I haz the power, 2014-03-25 20:20:16]

Change committed as 259319