Blink-side contiguous allocation of display items.

Implement blink-side contiguous allocation of display items.

This patch removes heap allocation of display items using ListContainer,
an in-place vector of DisplayItems (and subclasses). This patch is
inspired by both danakj's https://codereview.chromium.org/1123983002
and jbroman's https://codereview.chromium.org/1192443003.

Major changes:
1) Switch backing of DisplayItemList from the DisplayItems subclass
which contained a Vector of OwnPtrs to a ListContainer<DisplayItem>.
2) Remove all DisplayItem::Create functions in favor of calling
constructors directly.
3) Switch from ItemHandle::isGone to DisplayItem::isIgnoredFromDisplayList
4) Switch from DisplayItemList::add to DisplayItemList::allocateAndConstruct.

An Android cluster telemetry tryjob from patchset 15:
https://storage.cloud.google.com/cluster-telemetry/tasks/chromium_perf_runs/pdr-1435537875.2/html/index.html
Record time caching disabled: -1.565%
Record time construction disabled: -1.634%
Record time painting disabled: -6.427%

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

[pdr., 2015-06-19 06:34:49]

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

[Xianzhu, 2015-06-19 16:19:11]

https://codereview.chromium.org/1193433004/diff/60001/Source/platform/graphic...
File Source/platform/graphics/paint/DisplayItem.h (right):

https://codereview.chromium.org/1193433004/diff/60001/Source/platform/graphic...
Source/platform/graphics/paint/DisplayItem.h:363: class
DisplayItemClientWrapperHelper {
I think we can just use DisplayItemClientWrapper if we rename
DisplayItem::clientDebugString() to debugName() and make it declared in both
debug and release build.

https://codereview.chromium.org/1193433004/diff/60001/Source/platform/graphic...
File Source/platform/graphics/paint/DisplayItemList.cpp (left):

https://codereview.chromium.org/1193433004/diff/60001/Source/platform/graphic...
Source/platform/graphics/paint/DisplayItemList.cpp:71: ASSERT_NOT_REACHED();
Is the above code moved elsewhere or removed?

https://codereview.chromium.org/1193433004/diff/60001/Source/platform/graphic...
File Source/platform/graphics/paint/DisplayItemList.h (right):

https://codereview.chromium.org/1193433004/diff/60001/Source/platform/graphic...
Source/platform/graphics/paint/DisplayItemList.h:39: {
Can we put displayItemConstructionIsDisabled() check here so that we can the
call to it before each createAndAppendIfNeeded?

[chrishtr, 2015-06-19 18:32:37]

The patch looks good outside of the DisplayItems code.

[pdr., 2015-06-29 00:37:51]

Over the past week I've rewritten this patch and I think it's ready for review
again. Major changes are listed in the change description, along with some perf
data.

[chrishtr, 2015-06-29 17:19:16]

lgtm

https://codereview.chromium.org/1193433004/diff/280001/Source/platform/graphi...
File Source/platform/graphics/ListContainerTest.cpp (right):

https://codereview.chromium.org/1193433004/diff/280001/Source/platform/graphi...
Source/platform/graphics/ListContainerTest.cpp:889: EXPECT_EQ(list1.back(),
movedElement2);
Assert the pointers are not equal.

https://codereview.chromium.org/1193433004/diff/280001/Source/platform/graphi...
File Source/platform/graphics/paint/DisplayItem.h (right):

https://codereview.chromium.org/1193433004/diff/280001/Source/platform/graphi...
Source/platform/graphics/paint/DisplayItem.h:180: DisplayItem()
Add a comment about the use case. Also maybe private and friend classe'ed.

[danakj, 2015-06-29 17:43:50]

danakj@chromium.org changed reviewers:
+ danakj@chromium.org

[danakj, 2015-06-29 17:43:52]

https://codereview.chromium.org/1193433004/diff/280001/Source/core/paint/BoxC...
File Source/core/paint/BoxClipper.cpp (right):

https://codereview.chromium.org/1193433004/diff/280001/Source/core/paint/BoxC...
Source/core/paint/BoxClipper.cpp:78: if (m_clipType ==
DisplayItem::UninitializedType)
now the assert below will not run if slimming paint is on but
displayitemcontruction is off. Intentional?

https://codereview.chromium.org/1193433004/diff/280001/Source/platform/graphi...
File Source/platform/graphics/ListContainer.h (right):

https://codereview.chromium.org/1193433004/diff/280001/Source/platform/graphi...
Source/platform/graphics/ListContainer.h:237: return *iteratorAt(index);
Is this a change we could/should make in cc/ too?

https://codereview.chromium.org/1193433004/diff/280001/Source/platform/graphi...
Source/platform/graphics/ListContainer.h:255: BaseElementType& operator[](size_t
index)
I recommend not adding these. It looks like it should be cheap or O(1), like
arrays or vectors but it is not.

The elementAt() was added to ListContainer mostly for tests. Production code is
probably doing something wrong if it is using them.

Similar thoughts about iteratorAt().

I didn't see where these are used in the delta, can you point it out?

https://codereview.chromium.org/1193433004/diff/280001/Source/platform/graphi...
Source/platform/graphics/ListContainer.h:265: // Allocate a new, uninitialized
DerivedElementType. Use with caution!
Why do we need this?

https://codereview.chromium.org/1193433004/diff/280001/Source/platform/graphi...
Source/platform/graphics/ListContainer.h:307: DerivedElementType*
appendByMoving(DerivedElementType* item)
Can we do this to the cc/ class too?

https://codereview.chromium.org/1193433004/diff/280001/Source/platform/graphi...
File Source/platform/graphics/paint/DisplayItemList.h (right):

https://codereview.chromium.org/1193433004/diff/280001/Source/platform/graphi...
Source/platform/graphics/paint/DisplayItemList.h:48: DisplayItemClass*
displayItem = m_newDisplayItems.allocateWithoutConstruction<DisplayItemClass>();
why isnt it allocateAndContruct<DisplayItemClass>(WTF::forward<Args>(args)...)?

[jbroman, 2015-06-29 18:11:56]

https://codereview.chromium.org/1193433004/diff/280001/Source/core/layout/Lay...
File Source/core/layout/LayoutBox.h (right):

https://codereview.chromium.org/1193433004/diff/280001/Source/core/layout/Lay...
Source/core/layout/LayoutBox.h:583: virtual LayoutRect overflowClipRect(const
LayoutPoint& location, OverlayScrollbarSizeRelevancy =
IgnoreOverlayScrollbarSize) const;
These constness changes don't look like they need to be in this CL; can they be
landed separately?

https://codereview.chromium.org/1193433004/diff/280001/Source/core/paint/BoxC...
File Source/core/paint/BoxClipper.cpp (right):

https://codereview.chromium.org/1193433004/diff/280001/Source/core/paint/BoxC...
Source/core/paint/BoxClipper.cpp:57: OwnPtr<Vector<FloatRoundedRect>>
roundedRects;
Not introduced in this change, but why the double indirection here?

https://codereview.chromium.org/1193433004/diff/280001/Source/core/paint/BoxC...
Source/core/paint/BoxClipper.cpp:57: OwnPtr<Vector<FloatRoundedRect>>
roundedRects;
This code is used in both paths; suggest lifting it outside the
RuntimeEnabledFeatures::slimmingPaintEnabled condition?

https://codereview.chromium.org/1193433004/diff/280001/Source/core/paint/Floa...
File Source/core/paint/FloatClipRecorder.cpp (right):

https://codereview.chromium.org/1193433004/diff/280001/Source/core/paint/Floa...
Source/core/paint/FloatClipRecorder.cpp:43: EndFloatClipDisplayItem
floatClipDisplayItem(m_client, endType);
Why this variable name change? The old name seems at least as good as the new
one.

https://codereview.chromium.org/1193433004/diff/280001/Source/platform/graphi...
File Source/platform/graphics/paint/DisplayItem.h (right):

https://codereview.chromium.org/1193433004/diff/280001/Source/platform/graphi...
Source/platform/graphics/paint/DisplayItem.h:176: UninitializedType,
Superduper nit: to me it seems nicer for the in-memory representation of this to
be zero. WDYT?

https://codereview.chromium.org/1193433004/diff/280001/Source/platform/graphi...
File Source/platform/graphics/paint/DisplayItemList.h (right):

https://codereview.chromium.org/1193433004/diff/280001/Source/platform/graphi...
Source/platform/graphics/paint/DisplayItemList.h:24: static const size_t
kMaximumDisplayItemSize = sizeof(BeginTransform3DDisplayItem);
I think you should be able to move this to a source file, and then make it more
durable, with something like:

static const size_t kMaximumDisplayItemSize = MaxSize<ThisDisplayItem,
ThatDisplayItem, SomeOtherDisplayItem>::value;

with

template <typename... Types> struct MaxSize;
template <> struct MaxSize<> { static const size_t value = 0; };
template <typename T, typename... Rest> struct MaxSize<T, Rest...> {
  static const size_t value = sizeof(T) >= MaxSize<Rest...>::value ? sizeof(T) :
MaxSize<Rest...>::value;
};

On a related note: it hasn't bitten us yet in cc, but ListContainer doesn't take
alignment into account, so I'm concerned we could end up with unaligned data in
the future. In particular, TransformationMatrix wants its data to be aligned on
16-byte boundaries (and if this doesn't happen, we could get runtime
exceptions). Our tests would probably catch this, so I won't block this CL on
figuring this out, but wanted to make sure it's on your radar.

https://codereview.chromium.org/1193433004/diff/280001/Source/platform/graphi...
Source/platform/graphics/paint/DisplayItemList.h:48: DisplayItemClass*
displayItem = m_newDisplayItems.allocateWithoutConstruction<DisplayItemClass>();
On 2015/06/29 17:43:51, danakj OOO back june 28 wrote:
> why isnt it
allocateAndContruct<DisplayItemClass>(WTF::forward<Args>(args)...)?

Agreed; we can do this in one step.

https://codereview.chromium.org/1193433004/diff/280001/Source/platform/graphi...
File Source/platform/graphics/paint/DisplayItems.h (left):

https://codereview.chromium.org/1193433004/diff/280001/Source/platform/graphi...
Source/platform/graphics/paint/DisplayItems.h:1: // Copyright 2015 The Chromium
Authors. All rights reserved.
I take it you removed this abstraction for performance reasons?

I think I'm okay with it as long as we have an idea about the trajectory of how
DisplayItems will be represented. This CL reintroduces the assumption that
Blink's display item list actually contains instances of blink::DisplayItem
(which isn't what cc holds).

My current favourite plan is, after the repo merge, to put subclasses of
cc::DisplayItem (possibly custom ones for Blink) in a cc::DisplayItemList. In
that case I don't think we'll need this abstraction. What do you envision?

[danakj, 2015-06-29 18:15:29]

https://codereview.chromium.org/1193433004/diff/280001/Source/platform/graphi...
File Source/platform/graphics/paint/DisplayItemList.h (right):

https://codereview.chromium.org/1193433004/diff/280001/Source/platform/graphi...
Source/platform/graphics/paint/DisplayItemList.h:24: static const size_t
kMaximumDisplayItemSize = sizeof(BeginTransform3DDisplayItem);
On 2015/06/29 18:11:56, jbroman wrote:
> I think you should be able to move this to a source file, and then make it
more
> durable, with something like:
> 
> static const size_t kMaximumDisplayItemSize = MaxSize<ThisDisplayItem,
> ThatDisplayItem, SomeOtherDisplayItem>::value;
> 
> with
> 
> template <typename... Types> struct MaxSize;
> template <> struct MaxSize<> { static const size_t value = 0; };
> template <typename T, typename... Rest> struct MaxSize<T, Rest...> {
>   static const size_t value = sizeof(T) >= MaxSize<Rest...>::value ? sizeof(T)
:
> MaxSize<Rest...>::value;
> };
> 
> On a related note: it hasn't bitten us yet in cc, but ListContainer doesn't
take
> alignment into account, so I'm concerned we could end up with unaligned data
in
> the future. In particular, TransformationMatrix wants its data to be aligned
on
> 16-byte boundaries (and if this doesn't happen, we could get runtime
> exceptions). Our tests would probably catch this, so I won't block this CL on
> figuring this out, but wanted to make sure it's on your radar.

FWIW there is a static assert that makes it impossible for this to be wrong for
any DisplayItem type actually used. This seems more complicated? We could static
assert alignment by checking size % 16?

[jbroman, 2015-06-29 18:32:38]

On 2015/06/29 18:15:29, danakj OOO back june 28 wrote:
>
https://codereview.chromium.org/1193433004/diff/280001/Source/platform/graphi...
> File Source/platform/graphics/paint/DisplayItemList.h (right):
> 
>
https://codereview.chromium.org/1193433004/diff/280001/Source/platform/graphi...
> Source/platform/graphics/paint/DisplayItemList.h:24: static const size_t
> kMaximumDisplayItemSize = sizeof(BeginTransform3DDisplayItem);
> On 2015/06/29 18:11:56, jbroman wrote:
> > I think you should be able to move this to a source file, and then make it
> more
> > durable, with something like:
> > 
> > static const size_t kMaximumDisplayItemSize = MaxSize<ThisDisplayItem,
> > ThatDisplayItem, SomeOtherDisplayItem>::value;
> > 
> > with
> > 
> > template <typename... Types> struct MaxSize;
> > template <> struct MaxSize<> { static const size_t value = 0; };
> > template <typename T, typename... Rest> struct MaxSize<T, Rest...> {
> >   static const size_t value = sizeof(T) >= MaxSize<Rest...>::value ?
sizeof(T)
> :
> > MaxSize<Rest...>::value;
> > };
> > 
> > On a related note: it hasn't bitten us yet in cc, but ListContainer doesn't
> take
> > alignment into account, so I'm concerned we could end up with unaligned data
> in
> > the future. In particular, TransformationMatrix wants its data to be aligned
> on
> > 16-byte boundaries (and if this doesn't happen, we could get runtime
> > exceptions). Our tests would probably catch this, so I won't block this CL
on
> > figuring this out, but wanted to make sure it's on your radar.
> 
> FWIW there is a static assert that makes it impossible for this to be wrong
for
> any DisplayItem type actually used. This seems more complicated?

Is this referring to size or alignment?

> We could static assert alignment by checking size % 16?

Yes, as long as all of our alignments divide 16 (today, I believe they do).
In an ideal world I think the compiler would figure out what max size and
alignment are required, but I don't want to bikeshed that here. :)

[danakj, 2015-06-29 18:45:40]

On Mon, Jun 29, 2015 at 11:32 AM, <jbroman@chromium.org> wrote:

> On 2015/06/29 18:15:29, danakj OOO back june 28 wrote:
>
>
>
https://codereview.chromium.org/1193433004/diff/280001/Source/platform/graphi...
>
>> File Source/platform/graphics/paint/DisplayItemList.h (right):
>>
>
>
>
>
https://codereview.chromium.org/1193433004/diff/280001/Source/platform/graphi...
>
>> Source/platform/graphics/paint/DisplayItemList.h:24: static const size_t
>> kMaximumDisplayItemSize = sizeof(BeginTransform3DDisplayItem);
>> On 2015/06/29 18:11:56, jbroman wrote:
>> > I think you should be able to move this to a source file, and then make
>> it
>> more
>> > durable, with something like:
>> >
>> > static const size_t kMaximumDisplayItemSize = MaxSize<ThisDisplayItem,
>> > ThatDisplayItem, SomeOtherDisplayItem>::value;
>> >
>> > with
>> >
>> > template <typename... Types> struct MaxSize;
>> > template <> struct MaxSize<> { static const size_t value = 0; };
>> > template <typename T, typename... Rest> struct MaxSize<T, Rest...> {
>> >   static const size_t value = sizeof(T) >= MaxSize<Rest...>::value ?
>>
> sizeof(T)
>
>> :
>> > MaxSize<Rest...>::value;
>> > };
>> >
>> > On a related note: it hasn't bitten us yet in cc, but ListContainer
>> doesn't
>> take
>> > alignment into account, so I'm concerned we could end up with unaligned
>> data
>> in
>> > the future. In particular, TransformationMatrix wants its data to be
>> aligned
>> on
>> > 16-byte boundaries (and if this doesn't happen, we could get runtime
>> > exceptions). Our tests would probably catch this, so I won't block this
>> CL
>>
> on
>
>> > figuring this out, but wanted to make sure it's on your radar.
>>
>
>  FWIW there is a static assert that makes it impossible for this to be
>> wrong
>>
> for
>
>> any DisplayItem type actually used. This seems more complicated?
>>
>
> Is this referring to size or alignment?


The Max size computation instead of just verification.


>
>
>  We could static assert alignment by checking size % 16?
>>
>
> Yes, as long as all of our alignments divide 16 (today, I believe they do).
> In an ideal world I think the compiler would figure out what max size and
> alignment are required, but I don't want to bikeshed that here. :)
>
> https://codereview.chromium.org/1193433004/
>

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

[pdr., 2015-06-29 22:20:03]

Thanks for the reviews! PTAL

https://codereview.chromium.org/1193433004/diff/280001/Source/core/layout/Lay...
File Source/core/layout/LayoutBox.h (right):

https://codereview.chromium.org/1193433004/diff/280001/Source/core/layout/Lay...
Source/core/layout/LayoutBox.h:583: virtual LayoutRect overflowClipRect(const
LayoutPoint& location, OverlayScrollbarSizeRelevancy =
IgnoreOverlayScrollbarSize) const;
On 2015/06/29 at 18:11:56, jbroman wrote:
> These constness changes don't look like they need to be in this CL; can they
be landed separately?

Pulled out into https://codereview.chromium.org/1212293003 (rebase forthcoming).

https://codereview.chromium.org/1193433004/diff/280001/Source/core/paint/Floa...
File Source/core/paint/FloatClipRecorder.cpp (right):

https://codereview.chromium.org/1193433004/diff/280001/Source/core/paint/Floa...
Source/core/paint/FloatClipRecorder.cpp:43: EndFloatClipDisplayItem
floatClipDisplayItem(m_client, endType);
On 2015/06/29 at 18:11:56, jbroman wrote:
> Why this variable name change? The old name seems at least as good as the new
one.

Reverted back to the original name.

https://codereview.chromium.org/1193433004/diff/280001/Source/platform/graphi...
File Source/platform/graphics/ListContainer.h (right):

https://codereview.chromium.org/1193433004/diff/280001/Source/platform/graphi...
Source/platform/graphics/ListContainer.h:237: return *iteratorAt(index);
On 2015/06/29 at 17:43:51, danakj OOO back june 28 wrote:
> Is this a change we could/should make in cc/ too?

Done in https://codereview.chromium.org/1219753003.

https://codereview.chromium.org/1193433004/diff/280001/Source/platform/graphi...
Source/platform/graphics/ListContainer.h:255: BaseElementType& operator[](size_t
index)
On 2015/06/29 at 17:43:51, danakj OOO back june 28 wrote:
> I recommend not adding these. It looks like it should be cheap or O(1), like
arrays or vectors but it is not.
> 
> The elementAt() was added to ListContainer mostly for tests. Production code
is probably doing something wrong if it is using them.
> 
> Similar thoughts about iteratorAt().
> 
> I didn't see where these are used in the delta, can you point it out?

I added these for similar reasons. These are heavily used in both tests and
assert code, but not release code, and the patch would be difficult to review
with these changes. Would you be okay landing these for the same reasons they
were added in cc?

https://codereview.chromium.org/1193433004/diff/280001/Source/platform/graphi...
Source/platform/graphics/ListContainer.h:265: // Allocate a new, uninitialized
DerivedElementType. Use with caution!
On 2015/06/29 at 17:43:51, danakj OOO back june 28 wrote:
> Why do we need this?

We no longer do because I've made allocateAndConstruct take arguments.

https://codereview.chromium.org/1193433004/diff/280001/Source/platform/graphi...
Source/platform/graphics/ListContainer.h:307: DerivedElementType*
appendByMoving(DerivedElementType* item)
On 2015/06/29 at 17:43:51, danakj OOO back june 28 wrote:
> Can we do this to the cc/ class too?

Done in https://codereview.chromium.org/1219753003.

https://codereview.chromium.org/1193433004/diff/280001/Source/platform/graphi...
File Source/platform/graphics/ListContainerTest.cpp (right):

https://codereview.chromium.org/1193433004/diff/280001/Source/platform/graphi...
Source/platform/graphics/ListContainerTest.cpp:889: EXPECT_EQ(list1.back(),
movedElement2);
On 2015/06/29 at 17:19:16, chrishtr wrote:
> Assert the pointers are not equal.

Done.

https://codereview.chromium.org/1193433004/diff/280001/Source/platform/graphi...
File Source/platform/graphics/paint/DisplayItem.h (right):

https://codereview.chromium.org/1193433004/diff/280001/Source/platform/graphi...
Source/platform/graphics/paint/DisplayItem.h:176: UninitializedType,
On 2015/06/29 at 18:11:56, jbroman wrote:
> Superduper nit: to me it seems nicer for the in-memory representation of this
to be zero. WDYT?

Oops, I landed this in the BoxClipper patch
(https://codereview.chromium.org/1212293003) without addressing this.

Lets fix this in a followup to keep this patch reviewable, but I agree 100%.

https://codereview.chromium.org/1193433004/diff/280001/Source/platform/graphi...
Source/platform/graphics/paint/DisplayItem.h:180: DisplayItem()
On 2015/06/29 at 17:19:16, chrishtr wrote:
> Add a comment about the use case. Also maybe private and friend classe'ed.

Moved to private and just friended ListContainer::appendByMoving. Also added a
comment explaining wtf.

https://codereview.chromium.org/1193433004/diff/280001/Source/platform/graphi...
File Source/platform/graphics/paint/DisplayItemList.h (right):

https://codereview.chromium.org/1193433004/diff/280001/Source/platform/graphi...
Source/platform/graphics/paint/DisplayItemList.h:24: static const size_t
kMaximumDisplayItemSize = sizeof(BeginTransform3DDisplayItem);
On 2015/06/29 at 18:11:56, jbroman wrote:
> I think you should be able to move this to a source file, and then make it
more durable, with something like:
> 
> static const size_t kMaximumDisplayItemSize = MaxSize<ThisDisplayItem,
ThatDisplayItem, SomeOtherDisplayItem>::value;
> 
> with
> 
> template <typename... Types> struct MaxSize;
> template <> struct MaxSize<> { static const size_t value = 0; };
> template <typename T, typename... Rest> struct MaxSize<T, Rest...> {
>   static const size_t value = sizeof(T) >= MaxSize<Rest...>::value ? sizeof(T)
: MaxSize<Rest...>::value;
> };

This is a lot of complexity without much benefit. If you make a mistake and set
kMaximumDisplayItemSize to sizeof(DisplayItem), you'll get a static assert that
describes how to fix the issue. I think this is more robust than keeping a list
of all subclasses up to date.

https://codereview.chromium.org/1193433004/diff/280001/Source/platform/graphi...
Source/platform/graphics/paint/DisplayItemList.h:48: DisplayItemClass*
displayItem = m_newDisplayItems.allocateWithoutConstruction<DisplayItemClass>();
On 2015/06/29 at 18:11:56, jbroman wrote:
> On 2015/06/29 17:43:51, danakj OOO back june 28 wrote:
> > why isnt it
allocateAndContruct<DisplayItemClass>(WTF::forward<Args>(args)...)?
> 
> Agreed; we can do this in one step.

Agreed x2. Done.

This also removes the ListContainer::allocateWithoutConstruction cray-cray.

https://codereview.chromium.org/1193433004/diff/280001/Source/platform/graphi...
File Source/platform/graphics/paint/DisplayItems.h (left):

https://codereview.chromium.org/1193433004/diff/280001/Source/platform/graphi...
Source/platform/graphics/paint/DisplayItems.h:1: // Copyright 2015 The Chromium
Authors. All rights reserved.
On 2015/06/29 at 18:11:56, jbroman wrote:
> I take it you removed this abstraction for performance reasons?

Yeah :/

> 
> I think I'm okay with it as long as we have an idea about the trajectory of
how DisplayItems will be represented. This CL reintroduces the assumption that
Blink's display item list actually contains instances of blink::DisplayItem
(which isn't what cc holds).
> 
> My current favourite plan is, after the repo merge, to put subclasses of
cc::DisplayItem (possibly custom ones for Blink) in a cc::DisplayItemList. In
that case I don't think we'll need this abstraction. What do you envision?

I think Blink will build up the list and set a bit when it's ready for cc to use
it. I don't have a very concrete vision for how this will work. There are so
many unknowns at the moment that I don't really see the benefit DisplayItems was
providing once we decided against building up the list in cc.

[danakj, 2015-06-29 22:29:47]

https://codereview.chromium.org/1193433004/diff/280001/Source/platform/graphi...
File Source/platform/graphics/ListContainer.h (right):

https://codereview.chromium.org/1193433004/diff/280001/Source/platform/graphi...
Source/platform/graphics/ListContainer.h:255: BaseElementType& operator[](size_t
index)
On 2015/06/29 22:20:03, pdr wrote:
> On 2015/06/29 at 17:43:51, danakj OOO back june 28 wrote:
> > I recommend not adding these. It looks like it should be cheap or O(1), like
> arrays or vectors but it is not.
> > 
> > The elementAt() was added to ListContainer mostly for tests. Production code
> is probably doing something wrong if it is using them.
> > 
> > Similar thoughts about iteratorAt().
> > 
> > I didn't see where these are used in the delta, can you point it out?
> 
> I added these for similar reasons. These are heavily used in both tests and
> assert code, but not release code, and the patch would be difficult to review
> with these changes. Would you be okay landing these for the same reasons they
> were added in cc?

ah OK. iteratorat sounds OK to me. operator[] less so.

[pdr., 2015-06-29 22:59:42]

On 2015/06/29 at 22:29:47, danakj wrote:
>
https://codereview.chromium.org/1193433004/diff/280001/Source/platform/graphi...
> File Source/platform/graphics/ListContainer.h (right):
> 
>
https://codereview.chromium.org/1193433004/diff/280001/Source/platform/graphi...
> Source/platform/graphics/ListContainer.h:255: BaseElementType&
operator[](size_t index)
> On 2015/06/29 22:20:03, pdr wrote:
> > On 2015/06/29 at 17:43:51, danakj OOO back june 28 wrote:
> > > I recommend not adding these. It looks like it should be cheap or O(1),
like
> > arrays or vectors but it is not.
> > > 
> > > The elementAt() was added to ListContainer mostly for tests. Production
code
> > is probably doing something wrong if it is using them.
> > > 
> > > Similar thoughts about iteratorAt().
> > > 
> > > I didn't see where these are used in the delta, can you point it out?
> > 
> > I added these for similar reasons. These are heavily used in both tests and
> > assert code, but not release code, and the patch would be difficult to
review
> > with these changes. Would you be okay landing these for the same reasons
they
> > were added in cc?
> 
> ah OK. iteratorat sounds OK to me. operator[] less so.

That's reasonable. Operator[] has been removed in the latest patchset.

[jbroman, 2015-06-30 14:40:50]

lgtm; this seems reasonable (if danakj agrees)

https://codereview.chromium.org/1193433004/diff/280001/Source/platform/graphi...
File Source/platform/graphics/paint/DisplayItemList.h (right):

https://codereview.chromium.org/1193433004/diff/280001/Source/platform/graphi...
Source/platform/graphics/paint/DisplayItemList.h:24: static const size_t
kMaximumDisplayItemSize = sizeof(BeginTransform3DDisplayItem);
On 2015/06/29 22:20:03, pdr wrote:
> On 2015/06/29 at 18:11:56, jbroman wrote:
> > I think you should be able to move this to a source file, and then make it
> more durable, with something like:
> > 
> > static const size_t kMaximumDisplayItemSize = MaxSize<ThisDisplayItem,
> ThatDisplayItem, SomeOtherDisplayItem>::value;
> > 
> > with
> > 
> > template <typename... Types> struct MaxSize;
> > template <> struct MaxSize<> { static const size_t value = 0; };
> > template <typename T, typename... Rest> struct MaxSize<T, Rest...> {
> >   static const size_t value = sizeof(T) >= MaxSize<Rest...>::value ?
sizeof(T)
> : MaxSize<Rest...>::value;
> > };
> 
> This is a lot of complexity without much benefit. If you make a mistake and
set
> kMaximumDisplayItemSize to sizeof(DisplayItem), you'll get a static assert
that
> describes how to fix the issue. I think this is more robust than keeping a
list
> of all subclasses up to date.

You'd still have the other static_assert, but in any case I'm okay with leaving
this as-is (I'm just not thrilled that including DisplayItemList.h transitively
includes Transform3DDisplayItem.h, but I'll get over it).

[danakj, 2015-06-30 18:49:01]

LGTM but

https://codereview.chromium.org/1193433004/diff/320001/Source/platform/graphi...
File Source/platform/graphics/ListContainer.h (right):

https://codereview.chromium.org/1193433004/diff/320001/Source/platform/graphi...
Source/platform/graphics/ListContainer.h:260: return new
(allocate(sizeof(DerivedElementType)))
DerivedElementType(WTF::forward<Args>(args)...);
Can you add a unit test with non-0 number of arguments? Maybe something moveonly
like OwnPtr?

https://codereview.chromium.org/1193433004/diff/320001/Source/platform/graphi...
File Source/platform/graphics/paint/DisplayItemList.cpp (right):

https://codereview.chromium.org/1193433004/diff/320001/Source/platform/graphi...
Source/platform/graphics/paint/DisplayItemList.cpp:157: const DisplayItem&
existingItem = *list.elementAt(index);
This is using elementAt() in non-assert code AFAICT, and in a loop :(

Is it possible to restructure code such that you hold pointers instead of
indicies or something? Pointers into a ListContainer are stable (as opposed to
pointers into a vector which are not)

[jbroman, 2015-06-30 18:59:17]

https://codereview.chromium.org/1193433004/diff/320001/Source/platform/graphi...
File Source/platform/graphics/paint/DisplayItemList.cpp (right):

https://codereview.chromium.org/1193433004/diff/320001/Source/platform/graphi...
Source/platform/graphics/paint/DisplayItemList.cpp:157: const DisplayItem&
existingItem = *list.elementAt(index);
On 2015/06/30 18:49:01, danakj OOO back july 6 wrote:
> This is using elementAt() in non-assert code AFAICT, and in a loop :(
> 
> Is it possible to restructure code such that you hold pointers instead of
> indicies or something? Pointers into a ListContainer are stable (as opposed to
> pointers into a vector which are not)

I'd suggest attempting this in a followup CL. I eliminated a bunch of these
instances when I refactored DisplayItems previously, but I'd left these then
because changing them involved changing a bunch of the caching code.

If this CL is already a perf win, then why not land the additional perf win
separately, if it's straightforward?

[danakj, 2015-06-30 19:00:44]

On Tue, Jun 30, 2015 at 11:59 AM, <jbroman@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/1193433004/diff/320001/Source/platform/graphi...
> File Source/platform/graphics/paint/DisplayItemList.cpp (right):
>
>
>
https://codereview.chromium.org/1193433004/diff/320001/Source/platform/graphi...
> Source/platform/graphics/paint/DisplayItemList.cpp:157: const
> DisplayItem& existingItem = *list.elementAt(index);
> On 2015/06/30 18:49:01, danakj OOO back july 6 wrote:
>
>> This is using elementAt() in non-assert code AFAICT, and in a loop :(
>>
>
>  Is it possible to restructure code such that you hold pointers instead
>>
> of
>
>> indicies or something? Pointers into a ListContainer are stable (as
>>
> opposed to
>
>> pointers into a vector which are not)
>>
>
> I'd suggest attempting this in a followup CL. I eliminated a bunch of
> these instances when I refactored DisplayItems previously, but I'd left
> these then because changing them involved changing a bunch of the
> caching code.
>
> If this CL is already a perf win, then why not land the additional perf
> win separately, if it's straightforward?
>

That's fine, with a TODO pointing to a bug.


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

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

[pdr., 2015-06-30 22:16:05]

The CQ bit was checked by pdr@chromium.org

[pdr., 2015-06-30 22:16:06]

Thanks for the detailed reviews of this big patch. Reviewing these isn't much
fun...

Off to the CQ!

https://codereview.chromium.org/1193433004/diff/320001/Source/platform/graphi...
File Source/platform/graphics/ListContainer.h (right):

https://codereview.chromium.org/1193433004/diff/320001/Source/platform/graphi...
Source/platform/graphics/ListContainer.h:260: return new
(allocate(sizeof(DerivedElementType)))
DerivedElementType(WTF::forward<Args>(args)...);
On 2015/06/30 at 18:49:01, danakj OOO back july 6 wrote:
> Can you add a unit test with non-0 number of arguments? Maybe something
moveonly like OwnPtr?

Done, added ListContainerTest::AllocateAndConstructWithArguments which uses an
OwnPtr<SimpleDerivedElement>.

https://codereview.chromium.org/1193433004/diff/320001/Source/platform/graphi...
File Source/platform/graphics/paint/DisplayItemList.cpp (right):

https://codereview.chromium.org/1193433004/diff/320001/Source/platform/graphi...
Source/platform/graphics/paint/DisplayItemList.cpp:157: const DisplayItem&
existingItem = *list.elementAt(index);
On 2015/06/30 at 18:59:17, jbroman wrote:
> On 2015/06/30 18:49:01, danakj OOO back july 6 wrote:
> > This is using elementAt() in non-assert code AFAICT, and in a loop :(
> > 
> > Is it possible to restructure code such that you hold pointers instead of
> > indicies or something? Pointers into a ListContainer are stable (as opposed
to
> > pointers into a vector which are not)
> 
> I'd suggest attempting this in a followup CL. I eliminated a bunch of these
instances when I refactored DisplayItems previously, but I'd left these then
because changing them involved changing a bunch of the caching code.
> 
> If this CL is already a perf win, then why not land the additional perf win
separately, if it's straightforward?

I added a TODO here and filed crbug.com/505965.

[pdr., 2015-06-30 22:16:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org, danakj@chromium.org, jbroman@chromium.org
Link to the patchset: https://codereview.chromium.org/1193433004/#ps340001
(title: "Add ListContainer::AllocateAndConstructWithArguments and a TODO in
DisplayItemList::findMatchingIte…")

[commit-bot: I haz the power, 2015-06-30 22:16:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1193433004/340001

[commit-bot: I haz the power, 2015-06-30 23:26:21]

Committed patchset #18 (id:340001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=198107