Separate display item clients for negative and normal/positive z-order children

Separate display item clients for negative and normal/positive z-order children

Previously we have two subsequence types for negative z-order children
and normal/positive z-order children. Their cacheable status are set
separately, but our display item cache control is DisplayItemClient
based, causing errors when we skip cache for one type of children,
but not another.

Separate display item clients for the two types of children, so that
we can correctly control their cacheable status.

BUG=410097, 536999

[Xianzhu, 2015-11-04 22:06:06]

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

[Xianzhu, 2015-11-04 22:06:06]

This CL is tentative for now and I would like to hear your opinions.

Some consequences of it are:

1. It allows multiple display item clients for one object. This will allow us to
do less invalidation (e.g. invalidate background only instead of the whole
object when background style changes);

2. Will some future development need DisplayItemClient to be an interface? This
CL makes this more difficult because DisplayItemClient might not be a valid
pointer to an object.

Other possible ways to resolve the subsequence caching issue might be:

- During painting, when we need to skip caching for negative z-order children,
also skip caching for normal/positive z-order children. We can use this method
if this CL looks not good because of its complexity;

- Let DisplayItemClient be a pair <Object, subClientId>. This can avoid the
issue in above 2 if needed.

[Xianzhu, 2015-11-04 22:10:57]

Can we avoid two subsequences for negative and normal/positive z-order children
(back to outputting subsequences for layers), if we make subsequence independent
of paint properties and chunks?

[Xianzhu, 2015-11-04 22:12:18]

Description was changed from

==========
Separate display item clients for negative and normal/positive z-order children

Previously we have two subsequence types for negative z-order children
and normal/positive z-order children. Their cacheable status are set
separately, but our display item cache control is DisplayItemClient
based, causing errors when we skip cache for one type of children,
but not another.

Separate display item clients for the two types of children, so that
we can correctly control their cacheable status.

BUG=410097
==========

to

==========
Separate display item clients for negative and normal/positive z-order children

Previously we have two subsequence types for negative z-order children
and normal/positive z-order children. Their cacheable status are set
separately, but our display item cache control is DisplayItemClient
based, causing errors when we skip cache for one type of children,
but not another.

Separate display item clients for the two types of children, so that
we can correctly control their cacheable status.

BUG=410097,536999
==========

[chrishtr, 2015-11-04 22:38:30]

On 2015/11/04 at 22:10:57, wangxianzhu wrote:
> Can we avoid two subsequences for negative and normal/positive z-order
children (back to outputting subsequences for layers), if we make subsequence
independent of paint properties and chunks?

How do you mean? How would they be independent?

[Xianzhu, 2015-11-04 23:03:47]

On 2015/11/04 22:38:30, chrishtr wrote:
> On 2015/11/04 at 22:10:57, wangxianzhu wrote:
> > Can we avoid two subsequences for negative and normal/positive z-order
> children (back to outputting subsequences for layers), if we make subsequence
> independent of paint properties and chunks?
> 
> How do you mean? How would they be independent?

This involves why we made
https://chromium.googlesource.com/chromium/src/+/b99bf8b27b2a85a6bc9fe5204336....
It let it possible to map subsequences to property tree nodes and paint chunks,
which was supposed to ease paint property handling but actually I don't know
exactly how, and am wondering if it is still the case. pdr@ may have more
thoughts about this.

[chrishtr, 2015-11-04 23:31:46]

On 2015/11/04 at 23:03:47, wangxianzhu wrote:
> On 2015/11/04 22:38:30, chrishtr wrote:
> > On 2015/11/04 at 22:10:57, wangxianzhu wrote:
> > > Can we avoid two subsequences for negative and normal/positive z-order
> > children (back to outputting subsequences for layers), if we make
subsequence
> > independent of paint properties and chunks?
> > 
> > How do you mean? How would they be independent?
> 
> This involves why we made
https://chromium.googlesource.com/chromium/src/+/b99bf8b27b2a85a6bc9fe5204336....
It let it possible to map subsequences to property tree nodes and paint chunks,
which was supposed to ease paint property handling but actually I don't know
exactly how, and am wondering if it is still the case. pdr@ may have more
thoughts about this.

Ah now I remember. IIRC, the reason it helps is in order to distinguish between
painting of the element from its children, because different property tree
states can apply to the element and its children.

[chrishtr, 2015-11-04 23:37:11]

https://codereview.chromium.org/1425593007/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayer.h (right):

https://codereview.chromium.org/1425593007/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayer.h:619: DisplayItemClientWrapper
displayItemClientForNegativeZOrderChildren() const
Don't inline these.

https://codereview.chromium.org/1425593007/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/paint/DisplayItemClient.h
(right):

https://codereview.chromium.org/1425593007/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/DisplayItemClient.h:24: return
reinterpret_cast<DisplayItemClient>(reinterpret_cast<const char*>(object) +
subClientId);
This is kind of hacky. Why do it this way?

[Xianzhu, 2015-11-05 00:02:43]

On 2015/11/04 23:31:46, chrishtr wrote:
> On 2015/11/04 at 23:03:47, wangxianzhu wrote:
> > On 2015/11/04 22:38:30, chrishtr wrote:
> > > On 2015/11/04 at 22:10:57, wangxianzhu wrote:
> > > > Can we avoid two subsequences for negative and normal/positive z-order
> > > children (back to outputting subsequences for layers), if we make
> subsequence
> > > independent of paint properties and chunks?
> > > 
> > > How do you mean? How would they be independent?
> > 
> > This involves why we made
>
https://chromium.googlesource.com/chromium/src/+/b99bf8b27b2a85a6bc9fe5204336....
> It let it possible to map subsequences to property tree nodes and paint
chunks,
> which was supposed to ease paint property handling but actually I don't know
> exactly how, and am wondering if it is still the case. pdr@ may have more
> thoughts about this.
> 
> Ah now I remember. IIRC, the reason it helps is in order to distinguish
between
> painting of the element from its children, because different property tree
> states can apply to the element and its children.

So I meant: can subsequences and property tree states be independent like the
following?

<subsequence>
  <layer background> -> layer property tree state
  <negative children> -> child property tree state
  <layer contents> -> layer property tree state
  <normal and positive children> -> child property tree state
</subsequence>

[Xianzhu, 2015-11-05 00:30:28]

https://codereview.chromium.org/1425593007/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/paint/DisplayItemClient.h
(right):

https://codereview.chromium.org/1425593007/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/DisplayItemClient.h:24: return
reinterpret_cast<DisplayItemClient>(reinterpret_cast<const char*>(object) +
subClientId);
On 2015/11/04 23:37:11, chrishtr wrote:
> This is kind of hacky. Why do it this way?

To allow one object to have multiple DisplayItemClient, the above code treats an
object as char[sizeof(object)], so that we can use the address of each char to
identify a sub-DisplayItemClient.

Another way is to make DisplayItemClient a tuple <void*, subClientId>, but this
may increase memory and cpu cost dealing with DisplayItemClient.

[chrishtr, 2015-11-05 00:41:19]

On 2015/11/05 at 00:30:28, wangxianzhu wrote:
>
https://codereview.chromium.org/1425593007/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/platform/graphics/paint/DisplayItemClient.h
(right):
> 
>
https://codereview.chromium.org/1425593007/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/graphics/paint/DisplayItemClient.h:24:
return reinterpret_cast<DisplayItemClient>(reinterpret_cast<const char*>(object)
+ subClientId);
> On 2015/11/04 23:37:11, chrishtr wrote:
> > This is kind of hacky. Why do it this way?
> 
> To allow one object to have multiple DisplayItemClient, the above code treats
an object as char[sizeof(object)], so that we can use the address of each char
to identify a sub-DisplayItemClient.
> 
> Another way is to make DisplayItemClient a tuple <void*, subClientId>, but
this may increase memory and cpu cost dealing with DisplayItemClient.

Or new up a struct that is a DisplayItemClientWrapper?

[chrishtr, 2015-11-05 01:12:04]

On 2015/11/05 at 00:02:43, wangxianzhu wrote:
> On 2015/11/04 23:31:46, chrishtr wrote:
> > On 2015/11/04 at 23:03:47, wangxianzhu wrote:
> > > On 2015/11/04 22:38:30, chrishtr wrote:
> > > > On 2015/11/04 at 22:10:57, wangxianzhu wrote:
> > > > > Can we avoid two subsequences for negative and normal/positive z-order
> > > > children (back to outputting subsequences for layers), if we make
> > subsequence
> > > > independent of paint properties and chunks?
> > > > 
> > > > How do you mean? How would they be independent?
> > > 
> > > This involves why we made
> >
https://chromium.googlesource.com/chromium/src/+/b99bf8b27b2a85a6bc9fe5204336....
> > It let it possible to map subsequences to property tree nodes and paint
chunks,
> > which was supposed to ease paint property handling but actually I don't know
> > exactly how, and am wondering if it is still the case. pdr@ may have more
> > thoughts about this.
> > 
> > Ah now I remember. IIRC, the reason it helps is in order to distinguish
between
> > painting of the element from its children, because different property tree
> > states can apply to the element and its children.
> 
> So I meant: can subsequences and property tree states be independent like the
following?
> 
> <subsequence>
>   <layer background> -> layer property tree state
>   <negative children> -> child property tree state
>   <layer contents> -> layer property tree state
>   <normal and positive children> -> child property tree state
> </subsequence>

I agree. Think that can be done and not too hard, and would avoid this CL. pdr
might be in MTV tomorrow if he feels better, chat with him and trchen then?

[Xianzhu, 2015-11-05 01:19:06]

On 2015/11/05 00:41:19, chrishtr wrote:
> On 2015/11/05 at 00:30:28, wangxianzhu wrote:
> >
>
https://codereview.chromium.org/1425593007/diff/20001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/platform/graphics/paint/DisplayItemClient.h
> (right):
> > 
> >
>
https://codereview.chromium.org/1425593007/diff/20001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/platform/graphics/paint/DisplayItemClient.h:24:
> return reinterpret_cast<DisplayItemClient>(reinterpret_cast<const
char*>(object)
> + subClientId);
> > On 2015/11/04 23:37:11, chrishtr wrote:
> > > This is kind of hacky. Why do it this way?
> > 
> > To allow one object to have multiple DisplayItemClient, the above code
treats
> an object as char[sizeof(object)], so that we can use the address of each char
> to identify a sub-DisplayItemClient.
> > 
> > Another way is to make DisplayItemClient a tuple <void*, subClientId>, but
> this may increase memory and cpu cost dealing with DisplayItemClient.
> 
> Or new up a struct that is a DisplayItemClientWrapper?

We can define DisplayItemClient as a struct, but needs complexity to pack the
additional field into the current DisplayItem size, and a custom hash function
for the struct. Patch Set 3 is an incomplete implementation of this method
(missing the hash function). The reinterpret_cast<const char*> method is hacky,
but it's local to this part of DisplayItemClient.h and can avoid the above
complications.

Anyway, what do you think about #9? If we can decouple subsequences from
property tree, we can avoid more complications.

[Xianzhu, 2015-11-05 01:20:29]

On 2015/11/05 01:19:06, Xianzhu wrote:
> On 2015/11/05 00:41:19, chrishtr wrote:
> > On 2015/11/05 at 00:30:28, wangxianzhu wrote:
> > >
> >
>
https://codereview.chromium.org/1425593007/diff/20001/third_party/WebKit/Sour...
> > > File third_party/WebKit/Source/platform/graphics/paint/DisplayItemClient.h
> > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1425593007/diff/20001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/platform/graphics/paint/DisplayItemClient.h:24:
> > return reinterpret_cast<DisplayItemClient>(reinterpret_cast<const
> char*>(object)
> > + subClientId);
> > > On 2015/11/04 23:37:11, chrishtr wrote:
> > > > This is kind of hacky. Why do it this way?
> > > 
> > > To allow one object to have multiple DisplayItemClient, the above code
> treats
> > an object as char[sizeof(object)], so that we can use the address of each
char
> > to identify a sub-DisplayItemClient.
> > > 
> > > Another way is to make DisplayItemClient a tuple <void*, subClientId>, but
> > this may increase memory and cpu cost dealing with DisplayItemClient.
> > 
> > Or new up a struct that is a DisplayItemClientWrapper?
> 
> We can define DisplayItemClient as a struct, but needs complexity to pack the
> additional field into the current DisplayItem size, and a custom hash function
> for the struct. Patch Set 3 is an incomplete implementation of this method
> (missing the hash function). The reinterpret_cast<const char*> method is
hacky,
> but it's local to this part of DisplayItemClient.h and can avoid the above
> complications.
> 
> Anyway, what do you think about #9? If we can decouple subsequences from
> property tree, we can avoid more complications.

Never mind. Sent the above before saw your last comment. Will talk with pdr
tommorrow.

BTW, this patch will fix the last known issue blocking sync painting :)

[Xianzhu, 2015-11-05 18:26:26]

Discussed with pdr@ and we decided to take the simplest way: output subsequence
on layer first. We can rethink this when the requirements on subsequences get
clearer during the development of paint property trees.