Fix alignment issue of ContiguousContainer

Fix alignment issue of ContiguousContainer

- Fix crbug.com/625381 that alignment is missing during
  ContiguousContainer::appendByMoving();
- Allow mixed alignment of objects to make them more compacted
  in the buffer

BUG=625381
TEST=ContiguousContainerTest.Alignment
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

[Xianzhu, 2016-07-02 23:00:22]

Description was changed from

==========
Fix alignment issue of ContiguousContainer

- Fix crbug.com/625381 that alignment is missing during
  ContiguousContainer::appendByMoving();
- Fix alignment of the first object in the buffer;
- Allow mixed alignment of objects to make them more compacted
  in the buffer

BUG=625381
TEST=ContiguousContainerTest.Alignment
==========

to

==========
Fix alignment issue of ContiguousContainer

- Fix crbug.com/625381 that alignment is missing during
  ContiguousContainer::appendByMoving();
- Fix alignment of the first object in the buffer;
- Allow mixed alignment of objects to make them more compacted
  in the buffer

BUG=625381
TEST=ContiguousContainerTest.Alignment
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Xianzhu, 2016-07-04 01:32:15]

Description was changed from

==========
Fix alignment issue of ContiguousContainer

- Fix crbug.com/625381 that alignment is missing during
  ContiguousContainer::appendByMoving();
- Fix alignment of the first object in the buffer;
- Allow mixed alignment of objects to make them more compacted
  in the buffer

BUG=625381
TEST=ContiguousContainerTest.Alignment
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Fix alignment issue of ContiguousContainer

- Fix crbug.com/625381 that alignment is missing during
  ContiguousContainer::appendByMoving();
- Allow mixed alignment of objects to make them more compacted
  in the buffer

BUG=625381
TEST=ContiguousContainerTest.Alignment
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Xianzhu, 2016-07-04 16:31:55]

wangxianzhu@chromium.org changed reviewers:
+ jbroman@chromium.org, pdr@chromium.org

[Xianzhu, 2016-07-04 16:31:56]

https://codereview.chromium.org/2122573002/ is a much simpler change, but it
causes 2% regression of viewport_picture_size in rasterize_and_record_micro
tests (ct.skia.org chromium run 1043). I think the measurement without patch
happened to be taken when the DisplayItemList contained many copied by moving
items which are smaller than the items created directly, and with the patch they
are of the same size.

With this CL, there was no regression of viewport_picture_size (ct.skia.org
chromium run 1042). There were also progressions of record_time_caching_disabled
and record_time_construction_disabled. Don't know exactly why. Rerunning the
test to see if there was flakiness.

I also wonder if we can combine the versions of ContiguousContainer in cc and
blink.

[jbroman, 2016-07-04 19:21:28]

Hmm, I'm interested to see if those speed differences are reliable.

I'm not convinced this complexity is worth the 2% size, especially since I'd
expect that to go away with SPv2, which doesn't use BeginTransform3DDisplayItem
(and hence would allow us to go to pointer alignment, which should be perfect).

On 2016/07/04 at 16:31:56, wangxianzhu wrote:
> https://codereview.chromium.org/2122573002/ is a much simpler change, but it
causes 2% regression of viewport_picture_size in rasterize_and_record_micro
tests (ct.skia.org chromium run 1043). I think the measurement without patch
happened to be taken when the DisplayItemList contained many copied by moving
items which are smaller than the items created directly, and with the patch they
are of the same size.
> 
> With this CL, there was no regression of viewport_picture_size (ct.skia.org
chromium run 1042). There were also progressions of record_time_caching_disabled
and record_time_construction_disabled. Don't know exactly why. Rerunning the
test to see if there was flakiness.
> 
> I also wonder if we can combine the versions of ContiguousContainer in cc and
blink.

It's non-trivial, because Blink probably wants to use partition alloc but cc
cannot, etc. This is bug 580224, but there doesn't seem to be an obvious
solution that doesn't involve a fairly large amount of magic to abstract the
differences (templating on the allocator, etc).

[jbroman, 2016-07-04 19:21:35]

https://codereview.chromium.org/2119033003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/ContiguousContainer.cpp
(right):

https://codereview.chromium.org/2119033003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/ContiguousContainer.cpp:24:
(((reinterpret_cast<size_t>(address) - 1) >> log2Alignment) + 1) <<
log2Alignment);
why size_t and not uintptr_t?

https://codereview.chromium.org/2119033003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/ContiguousContainer.cpp:79: // and
m_end because of alignment of the deallocated object.
...which seems weird to me.

https://codereview.chromium.org/2119033003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/paint/DisplayItem.h (right):

https://codereview.chromium.org/2119033003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/DisplayItem.h:213:
SECURITY_CHECK(derivedSize == m_derivedSize);
isn't the replacement for ASSERT_WITH_SECURITY_IMPLICATION SECURITY_DCHECK,
rather than CHECK? (The difference being that SECURITY_CHECK still compiles in
release builds.)

https://codereview.chromium.org/2119033003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/DisplayItem.h:394: class
DisplayItemBase : public DisplayItem {
nit: it seems a little weird to have DisplayItemBase be a derived class of
DisplayItem, rather than its base class.

[Xianzhu, 2016-07-04 19:30:23]

On 2016/07/04 19:21:28, jbroman wrote:
> Hmm, I'm interested to see if those speed differences are reliable.
> 
> I'm not convinced this complexity is worth the 2% size, especially since I'd
> expect that to go away with SPv2, which doesn't use
BeginTransform3DDisplayItem
> (and hence would allow us to go to pointer alignment, which should be
perfect).
> 
> On 2016/07/04 at 16:31:56, wangxianzhu wrote:
> > https://codereview.chromium.org/2122573002/ is a much simpler change, but it
> causes 2% regression of viewport_picture_size in rasterize_and_record_micro
> tests (http://ct.skia.org chromium run 1043). I think the measurement without
patch
> happened to be taken when the DisplayItemList contained many copied by moving
> items which are smaller than the items created directly, and with the patch
they
> are of the same size.
> > 
> > With this CL, there was no regression of viewport_picture_size
(http://ct.skia.org
> chromium run 1042). There were also progressions of
record_time_caching_disabled
> and record_time_construction_disabled. Don't know exactly why. Rerunning the
> test to see if there was flakiness.
> > 
> > I also wonder if we can combine the versions of ContiguousContainer in cc
and
> blink.
> 
> It's non-trivial, because Blink probably wants to use partition alloc but cc
> cannot, etc. This is bug 580224, but there doesn't seem to be an obvious
> solution that doesn't involve a fairly large amount of magic to abstract the
> differences (templating on the allocator, etc).

Thanks for the reply. Now I think we should go though the simpler way.
Abandoning this CL.

[Xianzhu, 2016-07-05 04:24:58]

On 2016/07/04 19:30:23, Xianzhu wrote:
> On 2016/07/04 19:21:28, jbroman wrote:
> > Hmm, I'm interested to see if those speed differences are reliable.
> > 
> > I'm not convinced this complexity is worth the 2% size, especially since I'd
> > expect that to go away with SPv2, which doesn't use
> BeginTransform3DDisplayItem
> > (and hence would allow us to go to pointer alignment, which should be
> perfect).
> > 
> > On 2016/07/04 at 16:31:56, wangxianzhu wrote:
> > > https://codereview.chromium.org/2122573002/ is a much simpler change, but
it
> > causes 2% regression of viewport_picture_size in rasterize_and_record_micro
> > tests (http://ct.skia.org chromium run 1043). I think the measurement
without
> patch
> > happened to be taken when the DisplayItemList contained many copied by
moving
> > items which are smaller than the items created directly, and with the patch
> they
> > are of the same size.
> > > 
> > > With this CL, there was no regression of viewport_picture_size
> (http://ct.skia.org
> > chromium run 1042). There were also progressions of
> record_time_caching_disabled
> > and record_time_construction_disabled. Don't know exactly why. Rerunning the
> > test to see if there was flakiness.
> > > 
> > > I also wonder if we can combine the versions of ContiguousContainer in cc
> and
> > blink.
> > 
> > It's non-trivial, because Blink probably wants to use partition alloc but cc
> > cannot, etc. This is bug 580224, but there doesn't seem to be an obvious
> > solution that doesn't involve a fairly large amount of magic to abstract the
> > differences (templating on the allocator, etc).
> 
> Thanks for the reply. Now I think we should go though the simpler way.
> Abandoning this CL.

Another run (1046) also showed progression of this CL. Guessed the cause might
be about more compact data but run 1047 proved not. It might be worth
investigating the cause.