Make DisplayItemClient an interface

Make DisplayItemClient an interface

Pros:
- Avoid tricky DisplayItemClientWrapper;
- We can cache some data in client instead of hashmaps in
  PaintController:
  * debugName;
  * invalidation flags;
  * paint invalidation rect.
  The HashMaps may be a reason of performance issues.
- We can add other callbacks when needed.

Cons:
- Increase object size of some sub-classes by 1 pointer.
  However, based on haraken@'s presentation [1] on blinkon 5,
  "sizeof(Node) and sizeof(LayoutObject) don’t really matter".

[1] https://docs.google.com/presentation/d/1_LRxXp30j60npShHSRVit0B5tLOS4TLiZ68gQ8nFLsM/edit#slide=id.g72a3d66d2_1_319

Committed: https://crrev.com/cfdee1dd1f08ba62ea739d7c2a45c09f5657c8a2
Cr-Commit-Position: refs/heads/master@{#363822}

[Xianzhu, 2015-12-04 00:29:53]

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

[Xianzhu, 2015-12-04 00:29:54]

This is just tentative. I would like to know you opinions.

[pdr., 2015-12-04 21:12:28]

On 2015/12/04 at 00:29:54, wangxianzhu wrote:
> This is just tentative. I would like to know you opinions.

I support this proposal. The backflips we were doing to avoid this were pretty
gross (DisplayItemClientWrapper! don't let the door hit ya' on the way out). Can
you do a cluster telemetry run on both linux and android over the weekend to see
if we can observe a difference on rasterize and record?

I'm curious of the actual memory effect of this change on real pages. Do you
have data, or even just a feeling for what it will look like?

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

On 2015/12/04 21:12:28, pdr wrote:
> On 2015/12/04 at 00:29:54, wangxianzhu wrote:
> > This is just tentative. I would like to know you opinions.
> 
> I support this proposal. The backflips we were doing to avoid this were pretty
> gross (DisplayItemClientWrapper! don't let the door hit ya' on the way out).
Can
> you do a cluster telemetry run on both linux and android over the weekend to
see
> if we can observe a difference on rasterize and record?
> 

Tasks scheduled.

I don't think there will be any performance difference. If any, there may be a
few less instructions previously used to initialize and copy
DisplayItemClientWrappers.

> I'm curious of the actual memory effect of this change on real pages. Do you
> have data, or even just a feeling for what it will look like?

I have no data. My feeling is that increasing LayoutObject size by one pointer
will have little impact on overall memory usage, based on haraken@'s memory
profiling results.

[Xianzhu, 2015-12-05 20:43:58]

Cluster telemetry runs show no difference:

https://doc-1j88k-8ing-s-googleusercontent.commondatastorage.googleapis.com/g...

https://doc-1j5na-8ing-s-googleusercontent.commondatastorage.googleapis.com/g...

[Xianzhu, 2015-12-05 20:45:06]

On 2015/12/05 20:43:58, Xianzhu wrote:
> Cluster telemetry runs show no difference:
> 
s/no difference/no obvious difference/

[pdr., 2015-12-05 21:27:11]

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

[pdr., 2015-12-05 21:39:19]

That's good that cluster telemetry showed no regressions. Can you share the
shorter link from the cluster telemetry results email? The longer links aren't
visible for me.

https://codereview.chromium.org/1497873002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/LocalFrame.h (right):

https://codereview.chromium.org/1497873002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/LocalFrame.h:183: String debugName() const
override { return "LocalFrame"; }
Should this be final?

https://codereview.chromium.org/1497873002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.h
(right):

https://codereview.chromium.org/1497873002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.h:71:
class CORE_EXPORT CompositedLayerMapping final : public GraphicsLayerClient,
public DisplayItemClient {
If the plan is to move invalidation bits and rects to DisplayItemClient, CLM
feels pretty weird having those. Should we be using the GraphicsLayer instead of
CLM as a client?

https://codereview.chromium.org/1497873002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/graphics/SVGImage.h (right):

https://codereview.chromium.org/1497873002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/graphics/SVGImage.h:45: class SVGImage final
: public Image, public DisplayItemClient {
This could be problematic because a single SVGImage can be shared by multiple
documents since it lives in the memorycache. For example, if two tabs both
reference happy.svg, and the two tabs happen to be in the same process, they
will share one SVGImage instance.

It looks like SVGImage is only a superclass of DisplayItemClient for two
SkPictureBuilder calls. Can we switch those to use a null DisplayItemClient* so
SVGImage doesn't need to be a DisplayItemClient?

https://codereview.chromium.org/1497873002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/exported/WebScrollbarThemeClientImpl.h
(right):

https://codereview.chromium.org/1497873002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/exported/WebScrollbarThemeClientImpl.h:86:
String debugName() const override;
Should this be final?

https://codereview.chromium.org/1497873002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/ImageBuffer.h (right):

https://codereview.chromium.org/1497873002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/ImageBuffer.h:70: class
PLATFORM_EXPORT ImageBuffer /* : public DisplayItemClient */ {
Is this not needed? Can we remove both DisplayItemClient displayItemClient() and
String debugName() ?

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

https://codereview.chromium.org/1497873002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/DisplayItemClient.h:13: class
PLATFORM_EXPORT DisplayItemClient {
Lets add a comment about what this class is for.

This is sort of a general thought (trchen or jbroman?) but if we named this
something more general, we could re-use this object as the base class for things
that can have paint properties too.

https://codereview.chromium.org/1497873002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/DisplayItemClient.h:17:
virtual String debugName() const = 0;
I think we have a potential diamond inheritance issue on LayoutObject. I'm
surprised the compiler doesn't complain.

class LayoutObject : public ImageResourceClient, public DisplayItemClient
class ImageResourceClient : public ResourceClient

Both have a debugName:
virtual String ResourceClient::debugName const
and
virtual String DisplayItemClient::debugName const

https://codereview.chromium.org/1497873002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scroll/Scrollbar.h (right):

https://codereview.chromium.org/1497873002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/Scrollbar.h:152: String debugName()
const override { return m_orientation == HorizontalScrollbar ?
"HorizontalScrollbar" : "VerticalScrollbar"; }
Should this be final too?

[Xianzhu, 2015-12-06 20:43:43]

On 2015/12/05 21:39:19, pdr wrote:
> That's good that cluster telemetry showed no regressions. Can you share the
> shorter link from the cluster telemetry results email? The longer links aren't
> visible for me.
> 

Please try goo.gl/AhRDLA and goo.gl/u0KxCk

[pdr., 2015-12-06 21:24:27]

On 2015/12/06 at 20:43:43, wangxianzhu wrote:
> On 2015/12/05 21:39:19, pdr wrote:
> > That's good that cluster telemetry showed no regressions. Can you share the
> > shorter link from the cluster telemetry results email? The longer links
aren't
> > visible for me.
> > 
> 
> Please try goo.gl/AhRDLA and goo.gl/u0KxCk

Those also don't work, but I looked up the links on cluster telemetry:
linux:
https://storage.cloud.google.com/cluster-telemetry/tasks/chromium_perf_runs/w...
android:
https://storage.cloud.google.com/cluster-telemetry/tasks/chromium_perf_runs/w...

The ACLs for cluster telemetry are a little weird so I can view these, but not
the full links. As you said, these results look pretty much flat (maybe a small
win, if anything).

[Xianzhu, 2015-12-07 17:14:01]

https://codereview.chromium.org/1497873002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/LocalFrame.h (right):

https://codereview.chromium.org/1497873002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/LocalFrame.h:183: String debugName() const
override { return "LocalFrame"; }
On 2015/12/05 21:39:19, pdr wrote:
> Should this be final?

Yes. Done.

https://codereview.chromium.org/1497873002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.h
(right):

https://codereview.chromium.org/1497873002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.h:71:
class CORE_EXPORT CompositedLayerMapping final : public GraphicsLayerClient,
public DisplayItemClient {
On 2015/12/05 21:39:19, pdr wrote:
> If the plan is to move invalidation bits and rects to DisplayItemClient, CLM
> feels pretty weird having those. Should we be using the GraphicsLayer instead
of
> CLM as a client?

Yes. Done.

Perhaps we can also have a CacheableDisplayItemClient for invalidation bits and
rects, so that non-cacheable display item clients don't need to care about these
methods.

https://codereview.chromium.org/1497873002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/graphics/SVGImage.h (right):

https://codereview.chromium.org/1497873002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/graphics/SVGImage.h:45: class SVGImage final
: public Image, public DisplayItemClient {
On 2015/12/05 21:39:19, pdr wrote:
> This could be problematic because a single SVGImage can be shared by multiple
> documents since it lives in the memorycache. For example, if two tabs both
> reference happy.svg, and the two tabs happen to be in the same process, they
> will share one SVGImage instance.

If you meant the problem that the invalidation bits would be modified by
different PaintControllers, I think it can be avoided by adding a
CacheableDisplayItemClient type which contains methods for accessing
invalidation bits, while SVGImage can be still a normal non-cacheable
DisplayItemClient.     

> 
> It looks like SVGImage is only a superclass of DisplayItemClient for two
> SkPictureBuilder calls. Can we switch those to use a null DisplayItemClient*
so
> SVGImage doesn't need to be a DisplayItemClient?

I don't quite understand the use cases you mentioned. Can you provide links to
the code?

The current patch requires non-null DisplayItemClients. Null DisplayItemClient
seems to increase complexity and eliminate the ability to track display item
sources for debugging.

https://codereview.chromium.org/1497873002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/exported/WebScrollbarThemeClientImpl.h
(right):

https://codereview.chromium.org/1497873002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/exported/WebScrollbarThemeClientImpl.h:86:
String debugName() const override;
On 2015/12/05 21:39:19, pdr wrote:
> Should this be final?

Done.

https://codereview.chromium.org/1497873002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/ImageBuffer.h (right):

https://codereview.chromium.org/1497873002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/ImageBuffer.h:70: class
PLATFORM_EXPORT ImageBuffer /* : public DisplayItemClient */ {
On 2015/12/05 21:39:19, pdr wrote:
> Is this not needed? Can we remove both DisplayItemClient displayItemClient()
and
> String debugName() ?

I added it and tried compile to see if it's really needed but forgot to remove
it.

Done.

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

https://codereview.chromium.org/1497873002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/DisplayItemClient.h:13: class
PLATFORM_EXPORT DisplayItemClient {
On 2015/12/05 21:39:19, pdr wrote:
> Lets add a comment about what this class is for.
> 
> This is sort of a general thought (trchen or jbroman?) but if we named this
> something more general, we could re-use this object as the base class for
things
> that can have paint properties too.

Added an one-line comment.

I think we can address it later when we need to share it for other usages.

https://codereview.chromium.org/1497873002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/DisplayItemClient.h:17:
virtual String debugName() const = 0;
On 2015/12/05 21:39:19, pdr wrote:
> I think we have a potential diamond inheritance issue on LayoutObject. I'm
> surprised the compiler doesn't complain.
> 
> class LayoutObject : public ImageResourceClient, public DisplayItemClient
> class ImageResourceClient : public ResourceClient
> 
> Both have a debugName:
> virtual String ResourceClient::debugName const
> and
> virtual String DisplayItemClient::debugName const

It seems that the compilers allow multiple inherited interfaces to all have a
method with the same signature. In a LayoutObject the entries in the vtables for
the interfaces point to the same implementation method. This is different from
the case that a class inherits one super class through multiple paths.

https://codereview.chromium.org/1497873002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scroll/Scrollbar.h (right):

https://codereview.chromium.org/1497873002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/Scrollbar.h:152: String debugName()
const override { return m_orientation == HorizontalScrollbar ?
"HorizontalScrollbar" : "VerticalScrollbar"; }
On 2015/12/05 21:39:19, pdr wrote:
> Should this be final too?

Done.

[chrishtr, 2015-12-07 22:55:31]

lgtm

[Xianzhu, 2015-12-08 18:56:35]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2015-12-08 18:57:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1497873002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1497873002/80001

[commit-bot: I haz the power, 2015-12-08 19:01:24]

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

[commit-bot: I haz the power, 2015-12-08 19:01:25]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Xianzhu, 2015-12-08 19:12:59]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2015-12-08 19:13:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org
Link to the patchset: https://codereview.chromium.org/1497873002/#ps100001
(title: "Rebase")

[commit-bot: I haz the power, 2015-12-08 19:14:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1497873002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1497873002/100001

[commit-bot: I haz the power, 2015-12-08 19:30:15]

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

[commit-bot: I haz the power, 2015-12-08 19:30:16]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Xianzhu, 2015-12-08 19:31:19]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2015-12-08 19:33:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1497873002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1497873002/100001

[commit-bot: I haz the power, 2015-12-08 20:30:11]

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

[commit-bot: I haz the power, 2015-12-08 20:30:12]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Xianzhu, 2015-12-08 20:31:55]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2015-12-08 20:34:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1497873002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1497873002/100001

[commit-bot: I haz the power, 2015-12-08 21:41:41]

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

[commit-bot: I haz the power, 2015-12-08 21:41:42]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Xianzhu, 2015-12-08 21:55:35]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2015-12-08 21:57:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1497873002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1497873002/100001

[commit-bot: I haz the power, 2015-12-08 23:23:31]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-12-08 23:25:02]

Description was changed from

==========
Make DisplayItemClient an interface

Pros:
- Avoid tricky DisplayItemClientWrapper;
- We can cache some data in client instead of hashmaps in
  PaintController:
  * debugName;
  * invalidation flags;
  * paint invalidation rect.
  The HashMaps may be a reason of performance issues.
- We can add other callbacks when needed.

Cons:
- Increase object size of some sub-classes by 1 pointer.
  However, based on haraken@'s presentation [1] on blinkon 5,
  "sizeof(Node) and sizeof(LayoutObject) don’t really matter".

[1]
https://docs.google.com/presentation/d/1_LRxXp30j60npShHSRVit0B5tLOS4TLiZ68gQ...
==========

to

==========
Make DisplayItemClient an interface

Pros:
- Avoid tricky DisplayItemClientWrapper;
- We can cache some data in client instead of hashmaps in
  PaintController:
  * debugName;
  * invalidation flags;
  * paint invalidation rect.
  The HashMaps may be a reason of performance issues.
- We can add other callbacks when needed.

Cons:
- Increase object size of some sub-classes by 1 pointer.
  However, based on haraken@'s presentation [1] on blinkon 5,
  "sizeof(Node) and sizeof(LayoutObject) don’t really matter".

[1]
https://docs.google.com/presentation/d/1_LRxXp30j60npShHSRVit0B5tLOS4TLiZ68gQ...

Committed: https://crrev.com/cfdee1dd1f08ba62ea739d7c2a45c09f5657c8a2
Cr-Commit-Position: refs/heads/master@{#363822}
==========

[commit-bot: I haz the power, 2015-12-08 23:25:04]

Patchset 6 (id:??) landed as
https://crrev.com/cfdee1dd1f08ba62ea739d7c2a45c09f5657c8a2
Cr-Commit-Position: refs/heads/master@{#363822}