Client side display item cache flag

Client side display item cache flag

This is to reduce cost of hash lookups in PaintController.
See platform/graphics/paint/README.md and DisplayItemClient.h
for how it works.

BUG=550044
Committed: https://crrev.com/93b971703db1f49a431ddeec2ebc884a232ee6de
Cr-Commit-Position: refs/heads/master@{#390763}

[Xianzhu, 2015-12-11 16:58:09]

Description was changed from

==========
Client side display item cache flag

BUG=550044
==========

to

==========
Client side display item cache flag

To reduce cost of hash lookups in PaintController

BUG=550044
==========

[Xianzhu, 2015-12-11 16:59:00]

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

[Xianzhu, 2015-12-11 16:59:00]

FIY the other change to add methods into DisplayItemClient.

[chrishtr, 2015-12-11 17:05:00]

On 2015/12/11 at 16:59:00, wangxianzhu wrote:
> FIY the other change to add methods into DisplayItemClient.

Cool thanks.

[Xianzhu, 2015-12-29 20:20:34]

Description was changed from

==========
Client side display item cache flag

To reduce cost of hash lookups in PaintController

BUG=550044
==========

to

==========
Client side display item cache flag

This is to reduce cost of hash lookups in PaintController.

BUG=550044
==========

[Xianzhu, 2015-12-29 20:26:02]

Currently there are not much data showing the performance gain before we land
https://codereview.chromium.org/1552693002/.

http://storage.googleapis.com/chromium-telemetry/html-results/results-2015-12...
shows 7.7% performance gain for
blink_perf.layout:large-table-background-change-with-visible-collapsed-borders
which test for typical small changed painting in a big document. The performance
gain would be bigger with https://codereview.chromium.org/1549693002/ which
reduces cost of cached collapsed borders. Based on calculation, the performance
gain might be around 10%.

[pdr., 2016-01-04 21:19:30]

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

[pdr., 2016-01-04 21:19:31]

https://codereview.chromium.org/1508223005/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp
(right):

https://codereview.chromium.org/1508223005/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp:334: //
ASSERT(clientCacheIsValid(newDisplayItem.client()) ||
(RuntimeEnabledFeatures::slimmingPaintOffsetCachingEnabled() &&
!paintOffsetWasInvalidated(newDisplayItem.client())));
Drive by: did you forget this?

https://codereview.chromium.org/1508223005/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp:365:
/*ASSERT(!newDisplayItem.isDrawing()
Drive by: did you forget this?

https://codereview.chromium.org/1508223005/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp:506: if
(newItem.isCacheable()) { /////// EEE &&
!m_validlyCachedClients.contains(&newItem.client())) {
Drive by: did you forget this?

[chrishtr, 2016-01-20 21:15:38]

Xianzhu, could you remind me of what you think blocks this CL? Is it making the
objects too large?

[Xianzhu, 2016-01-20 21:33:58]

On 2016/01/20 21:15:38, chrishtr wrote:
> Xianzhu, could you remind me of what you think blocks this CL? Is it making
the
> objects too large?

Yes, it adds 4 bytes (or 8 bytes if we think the 4 byte cache tag is too small
to hold enough cache versions) to each LayoutObject. This might not be strictly
blocking, but perhaps it's better to try this CL after integrating paint
invalidation into paint property update (when we'll remove/shrink some fields of
LayoutObject).

You can still review this CL for the general method of it.

[pdr., 2016-04-18 18:46:54]

I found that calls to the cache hashtable represent 10% of
http://bl.ocks.org/jwhitfieldseed/de685d52af21264fad81. I suspect we'd see wins
other by switching this bit to a local on layoutobject.

[Xianzhu, 2016-04-18 19:19:52]

Restart this work.

Rebased and uploaded Patch Set 9.

Will profile the benchmark and schedule a cluster telemetry run.

pdr@ you can profile with the patch to see if the bottleneck still exists.

[pdr., 2016-04-18 23:54:53]

On 2016/04/18 at 19:19:52, wangxianzhu wrote:
> Restart this work.
> 
> Rebased and uploaded Patch Set 9.
> 
> Will profile the benchmark and schedule a cluster telemetry run.
> 
> pdr@ you can profile with the patch to see if the bottleneck still exists.

The results are in and they look good. I ran the benchmark
(http://bl.ocks.org/jwhitfieldseed/de685d52af21264fad81) before and after this
patch and this patch completely removes 12.3% of the time (6.3% in the hash
lookup, and 6% in adding hash entries). I don't see any new functions that are
expensive as a result of this patch.

Curious to see how this does on cluster telemetry!

[pdr., 2016-04-18 23:58:22]

On 2016/04/18 at 19:19:52, wangxianzhu wrote:
> Restart this work.
> 
> Rebased and uploaded Patch Set 9.
> 
> Will profile the benchmark and schedule a cluster telemetry run.
> 
> pdr@ you can profile with the patch to see if the bottleneck still exists.

The results are in and they look good. I ran the benchmark
(http://bl.ocks.org/jwhitfieldseed/de685d52af21264fad81) before and after this
patch and this patch completely removes 12.3% of the time (6.3% in the hash
lookup, and 6% in adding hash entries). I don't see any new functions that are
expensive as a result of this patch.

Curious to see how this does on cluster telemetry!

[Xianzhu, 2016-04-19 00:06:10]

On 2016/04/18 23:58:22, pdr wrote:
> On 2016/04/18 at 19:19:52, wangxianzhu wrote:
> > Restart this work.
> > 
> > Rebased and uploaded Patch Set 9.
> > 
> > Will profile the benchmark and schedule a cluster telemetry run.
> > 
> > pdr@ you can profile with the patch to see if the bottleneck still exists.
> 
> The results are in and they look good. I ran the benchmark
> (http://bl.ocks.org/jwhitfieldseed/de685d52af21264fad81) before and after this
> patch and this patch completely removes 12.3% of the time (6.3% in the hash
> lookup, and 6% in adding hash entries). I don't see any new functions that are
> expensive as a result of this patch.

Cool. Thanks for profiling.
 
> Curious to see how this does on cluster telemetry!

Run 566, 567 and 772 are cluster telemetry results of this patch. There seems
much noise in the result.

[pdr., 2016-04-19 17:49:17]

On 2016/04/19 at 00:06:10, wangxianzhu wrote:
> On 2016/04/18 23:58:22, pdr wrote:
> > On 2016/04/18 at 19:19:52, wangxianzhu wrote:
> > > Restart this work.
> > > 
> > > Rebased and uploaded Patch Set 9.
> > > 
> > > Will profile the benchmark and schedule a cluster telemetry run.
> > > 
> > > pdr@ you can profile with the patch to see if the bottleneck still exists.
> > 
> > The results are in and they look good. I ran the benchmark
> > (http://bl.ocks.org/jwhitfieldseed/de685d52af21264fad81) before and after
this
> > patch and this patch completely removes 12.3% of the time (6.3% in the hash
> > lookup, and 6% in adding hash entries). I don't see any new functions that
are
> > expensive as a result of this patch.
> 
> Cool. Thanks for profiling.
>  
> > Curious to see how this does on cluster telemetry!
> 
> Run 566, 567 and 772 are cluster telemetry results of this patch. There seems
much noise in the result.

I've queued up two more CT runs (one r&r, one repaint) because I don't believe
the 772 results which show a ~1% regression.
https://ct.skia.org/chromium_perf_runs

[pdr., 2016-04-20 02:13:45]

On 2016/04/19 at 17:49:17, pdr wrote:
> > > Curious to see how this does on cluster telemetry!
> > 
> > Run 566, 567 and 772 are cluster telemetry results of this patch. There
seems much noise in the result.
> 
> I've queued up two more CT runs (one r&r, one repaint) because I don't believe
the 772 results which show a ~1% regression.
https://ct.skia.org/chromium_perf_runs

The latest CT runs came back as regressions too. Some of it is noise, but I
worry that this patch is really a slight regression.

[chrishtr, 2016-04-20 16:39:14]

On 2016/04/20 at 02:13:45, pdr wrote:
> On 2016/04/19 at 17:49:17, pdr wrote:
> > > > Curious to see how this does on cluster telemetry!
> > > 
> > > Run 566, 567 and 772 are cluster telemetry results of this patch. There
seems much noise in the result.
> > 
> > I've queued up two more CT runs (one r&r, one repaint) because I don't
believe the 772 results which show a ~1% regression.
https://ct.skia.org/chromium_perf_runs
> 
> The latest CT runs came back as regressions too. Some of it is noise, but I
worry that this patch is really a slight regression.

Ok. This concern is the reason the original patch was not committed. The
assumption is that the regression comes from LayoutObject being bigger.
Xianzhu, maybe it's possible to test this hypothesis by hacking out the paint
invalidation position field from LayoutObject?

We could also commit it and look closely at Telemetry.

[Xianzhu, 2016-04-25 17:12:29]

Ran several CT tasks which proved the following things:
- Increasing LayoutObject size by 4 bytes has no visible effect on performance
(Run 794 with LayoutObject size not increased);
- Caching display items for LayoutObjects, PaintLayers and InlineBoxes is good
enough (Run 797 cache all DisplayItemClients (except if they are on layers that
are not allowed to use client side flags) got the similar result as Run 794 and
previous runs);
- The main factor is PaintController::canUseClientCacheStatus() which disables
display item caching for some GraphicsLayers when the same object may be on
multiple GraphicsLayers.

Will create a new patch which will change the caching policy when
PaintController::canUseClientCacheStatus() is false: fall back to HashSet-based
caching instead of no caching.

[chrishtr, 2016-04-25 17:28:34]

On 2016/04/25 at 17:12:29, wangxianzhu wrote:
> Ran several CT tasks which proved the following things:
> - Increasing LayoutObject size by 4 bytes has no visible effect on performance
(Run 794 with LayoutObject size not increased);
> - Caching display items for LayoutObjects, PaintLayers and InlineBoxes is good
enough (Run 797 cache all DisplayItemClients (except if they are on layers that
are not allowed to use client side flags) got the similar result as Run 794 and
previous runs);
> - The main factor is PaintController::canUseClientCacheStatus() which disables
display item caching for some GraphicsLayers when the same object may be on
multiple GraphicsLayers.

What's an example of when this would happen?

> 
> Will create a new patch which will change the caching policy when
PaintController::canUseClientCacheStatus() is false: fall back to HashSet-based
caching instead of no caching.

[Xianzhu, 2016-04-25 18:44:16]

On 2016/04/25 17:28:34, chrishtr wrote:
> On 2016/04/25 at 17:12:29, wangxianzhu wrote:
> > Ran several CT tasks which proved the following things:
> > - Increasing LayoutObject size by 4 bytes has no visible effect on
performance
> (Run 794 with LayoutObject size not increased);
> > - Caching display items for LayoutObjects, PaintLayers and InlineBoxes is
good
> enough (Run 797 cache all DisplayItemClients (except if they are on layers
that
> are not allowed to use client side flags) got the similar result as Run 794
and
> previous runs);
> > - The main factor is PaintController::canUseClientCacheStatus() which
disables
> display item caching for some GraphicsLayers when the same object may be on
> multiple GraphicsLayers.
> 
> What's an example of when this would happen?

An object may paint its mask image on maskLayer, background on the main layer or
backgroundLayer, foreground on the main layer, scrollingContentsLayer or
foregroundLayer. The situation should be very rare but we need to handle it.

Just thought of that perhaps we can avoid canUseClientCacheStatus() because of
the nature of cacheGeneration (which is unique across all PaintControllers): if
an object is painted on multiple PaintControllers, its cacheGeneration will be
valid on only one of the PaintControllers, so the object will be treated as
invalidated on other PaintControllers. As the situation is rare, its performance
impact may be much less than that of canUseClientCacheStatus() which totally
disables caching on some layers. Will create a patch for this and schedule a CT
task.

[chrishtr, 2016-04-25 19:41:32]

On 2016/04/25 at 18:44:16, wangxianzhu wrote:
> On 2016/04/25 17:28:34, chrishtr wrote:
> > On 2016/04/25 at 17:12:29, wangxianzhu wrote:
> > > Ran several CT tasks which proved the following things:
> > > - Increasing LayoutObject size by 4 bytes has no visible effect on
performance
> > (Run 794 with LayoutObject size not increased);
> > > - Caching display items for LayoutObjects, PaintLayers and InlineBoxes is
good
> > enough (Run 797 cache all DisplayItemClients (except if they are on layers
that
> > are not allowed to use client side flags) got the similar result as Run 794
and
> > previous runs);
> > > - The main factor is PaintController::canUseClientCacheStatus() which
disables
> > display item caching for some GraphicsLayers when the same object may be on
> > multiple GraphicsLayers.
> > 
> > What's an example of when this would happen?
> 
> An object may paint its mask image on maskLayer, background on the main layer
or backgroundLayer, foreground on the main layer, scrollingContentsLayer or
foregroundLayer. The situation should be very rare but we need to handle it.
> 
> Just thought of that perhaps we can avoid canUseClientCacheStatus() because of
the nature of cacheGeneration (which is unique across all PaintControllers): if
an object is painted on multiple PaintControllers, its cacheGeneration will be
valid on only one of the PaintControllers, so the object will be treated as
invalidated on other PaintControllers. As the situation is rare, its performance
impact may be much less than that of canUseClientCacheStatus() which totally
disables caching on some layers. Will create a patch for this and schedule a CT
task.

How rare is it? Maybe you could also count on CT?

[pdr., 2016-04-26 04:04:11]

On 2016/04/25 at 18:44:16, wangxianzhu wrote:
> On 2016/04/25 17:28:34, chrishtr wrote:
> > On 2016/04/25 at 17:12:29, wangxianzhu wrote:
> > > Ran several CT tasks which proved the following things:
> > > - Increasing LayoutObject size by 4 bytes has no visible effect on
performance
> > (Run 794 with LayoutObject size not increased);
> > > - Caching display items for LayoutObjects, PaintLayers and InlineBoxes is
good
> > enough (Run 797 cache all DisplayItemClients (except if they are on layers
that
> > are not allowed to use client side flags) got the similar result as Run 794
and
> > previous runs);
> > > - The main factor is PaintController::canUseClientCacheStatus() which
disables
> > display item caching for some GraphicsLayers when the same object may be on
> > multiple GraphicsLayers.
> > 
> > What's an example of when this would happen?
> 
> An object may paint its mask image on maskLayer, background on the main layer
or backgroundLayer, foreground on the main layer, scrollingContentsLayer or
foregroundLayer. The situation should be very rare but we need to handle it.
> 
> Just thought of that perhaps we can avoid canUseClientCacheStatus() because of
the nature of cacheGeneration (which is unique across all PaintControllers): if
an object is painted on multiple PaintControllers, its cacheGeneration will be
valid on only one of the PaintControllers, so the object will be treated as
invalidated on other PaintControllers. As the situation is rare, its performance
impact may be much less than that of canUseClientCacheStatus() which totally
disables caching on some layers. Will create a patch for this and schedule a CT
task.

Do we really need to support multiple paint controllers? Having to support that
usecase may cause complexity issues in the future. I poked around and the
non-graphics-layer ones seem to be all temporaries that we could refactor to not
use paint controllers (or use them in a mode that is safer with the new design).
Is there ever a case where a LayoutObject's display item is in two paint
controllers that are on different graphics layers?


I have an unrelated question about the generation id... Can you help me
understand why it's needed? Is the generation id just an optimization so we
don't need to manually mark display item clients as uncached? An alternative
would be to store the SkPictures (1x per phase) directly on the LayoutObject so
paint invalidation just frees the cached pictures directly (RefPtrs keep
everything safe).

[Xianzhu, 2016-04-26 16:59:35]

On 2016/04/26 04:04:11, pdr wrote:
> On 2016/04/25 at 18:44:16, wangxianzhu wrote:
> > On 2016/04/25 17:28:34, chrishtr wrote:
> > > On 2016/04/25 at 17:12:29, wangxianzhu wrote:
> > > > Ran several CT tasks which proved the following things:
> > > > - Increasing LayoutObject size by 4 bytes has no visible effect on
> performance
> > > (Run 794 with LayoutObject size not increased);
> > > > - Caching display items for LayoutObjects, PaintLayers and InlineBoxes
is
> good
> > > enough (Run 797 cache all DisplayItemClients (except if they are on layers
> that
> > > are not allowed to use client side flags) got the similar result as Run
794
> and
> > > previous runs);
> > > > - The main factor is PaintController::canUseClientCacheStatus() which
> disables
> > > display item caching for some GraphicsLayers when the same object may be
on
> > > multiple GraphicsLayers.
> > > 
> > > What's an example of when this would happen?
> > 
> > An object may paint its mask image on maskLayer, background on the main
layer
> or backgroundLayer, foreground on the main layer, scrollingContentsLayer or
> foregroundLayer. The situation should be very rare but we need to handle it.
> > 
> > Just thought of that perhaps we can avoid canUseClientCacheStatus() because
of
> the nature of cacheGeneration (which is unique across all PaintControllers):
if
> an object is painted on multiple PaintControllers, its cacheGeneration will be
> valid on only one of the PaintControllers, so the object will be treated as
> invalidated on other PaintControllers. As the situation is rare, its
performance
> impact may be much less than that of canUseClientCacheStatus() which totally
> disables caching on some layers. Will create a patch for this and schedule a
CT
> task.
> 
> Do we really need to support multiple paint controllers? Having to support
that
> usecase may cause complexity issues in the future. I poked around and the
> non-graphics-layer ones seem to be all temporaries that we could refactor to
not
> use paint controllers (or use them in a mode that is safer with the new
design).
> Is there ever a case where a LayoutObject's display item is in two paint
> controllers that are on different graphics layers?
> 

The complexity is for spv1 only. In some cases an object may paint its mask,
background, foreground on different graphics layers.

> 
> I have an unrelated question about the generation id... Can you help me
> understand why it's needed? Is the generation id just an optimization so we
> don't need to manually mark display item clients as uncached?

The id indicates whether an object has up-to-date painting on a PaintController.
It has the following benefits (compared to using a bool flag):
- For clients not painted (e.g. because it's out of interest rect or obscured)
we don't need to clear there cache flags. Clearing the flags is non-trivial when
we skip painting of a big subtree.
- When a PaintController is fully invalidated or recreated, we don't need to
bother cache flags of the clients. They are automatically treated as invalidated
because there ids mismatch.
- If a client is painted on multiple PaintControllers, its treated as valid on
one of the only. This avoids complexity to deal with the rare case. (Will add
stat result soon.)

> An alternative
> would be to store the SkPictures (1x per phase) directly on the LayoutObject
so
> paint invalidation just frees the cached pictures directly (RefPtrs keep
> everything safe).

I think this is worth exploring. We might have to store SkPictures 1 per drawing
display item id in order to keep the order of display items of ancestors and
descendants in the same paint phase (to support cases like
parent-display-item-a, child-display-item-b, parent-display-item-c). The most
difficult part might be finding a proper data structure.

[Xianzhu, 2016-04-26 17:04:05]

On 2016/04/25 19:41:32, chrishtr wrote:
> On 2016/04/25 at 18:44:16, wangxianzhu wrote:
> > On 2016/04/25 17:28:34, chrishtr wrote:
> > > On 2016/04/25 at 17:12:29, wangxianzhu wrote:
> > > > Ran several CT tasks which proved the following things:
> > > > - Increasing LayoutObject size by 4 bytes has no visible effect on
> performance
> > > (Run 794 with LayoutObject size not increased);
> > > > - Caching display items for LayoutObjects, PaintLayers and InlineBoxes
is
> good
> > > enough (Run 797 cache all DisplayItemClients (except if they are on layers
> that
> > > are not allowed to use client side flags) got the similar result as Run
794
> and
> > > previous runs);
> > > > - The main factor is PaintController::canUseClientCacheStatus() which
> disables
> > > display item caching for some GraphicsLayers when the same object may be
on
> > > multiple GraphicsLayers.
> > > 
> > > What's an example of when this would happen?
> > 
> > An object may paint its mask image on maskLayer, background on the main
layer
> or backgroundLayer, foreground on the main layer, scrollingContentsLayer or
> foregroundLayer. The situation should be very rare but we need to handle it.
> > 
> > Just thought of that perhaps we can avoid canUseClientCacheStatus() because
of
> the nature of cacheGeneration (which is unique across all PaintControllers):
if
> an object is painted on multiple PaintControllers, its cacheGeneration will be
> valid on only one of the PaintControllers, so the object will be treated as
> invalidated on other PaintControllers. As the situation is rare, its
performance
> impact may be much less than that of canUseClientCacheStatus() which totally
> disables caching on some layers. Will create a patch for this and schedule a
CT
> task.
> 
> How rare is it? Maybe you could also count on CT?

https://ct.skia.org/chromium_perf_runs/ run 803:

clients_painted_on_1: 4134853
clients_painted_on_2: 2865
clients_painted_on_3: 0
clients_painted_on_4+: 40

The percentage of clients painted on multiple GraphicsLayers is about 0.07%.

[chrishtr, 2016-04-26 18:48:36]

On 2016/04/26 at 16:59:35, wangxianzhu wrote:
> On 2016/04/26 04:04:11, pdr wrote:
> > On 2016/04/25 at 18:44:16, wangxianzhu wrote:
> > > On 2016/04/25 17:28:34, chrishtr wrote:
> > > > On 2016/04/25 at 17:12:29, wangxianzhu wrote:
> > > > > Ran several CT tasks which proved the following things:
> > > > > - Increasing LayoutObject size by 4 bytes has no visible effect on
> > performance
> > > > (Run 794 with LayoutObject size not increased);
> > > > > - Caching display items for LayoutObjects, PaintLayers and InlineBoxes
is
> > good
> > > > enough (Run 797 cache all DisplayItemClients (except if they are on
layers
> > that
> > > > are not allowed to use client side flags) got the similar result as Run
794
> > and
> > > > previous runs);
> > > > > - The main factor is PaintController::canUseClientCacheStatus() which
> > disables
> > > > display item caching for some GraphicsLayers when the same object may be
on
> > > > multiple GraphicsLayers.
> > > > 
> > > > What's an example of when this would happen?
> > > 
> > > An object may paint its mask image on maskLayer, background on the main
layer
> > or backgroundLayer, foreground on the main layer, scrollingContentsLayer or
> > foregroundLayer. The situation should be very rare but we need to handle it.
> > > 
> > > Just thought of that perhaps we can avoid canUseClientCacheStatus()
because of
> > the nature of cacheGeneration (which is unique across all PaintControllers):
if
> > an object is painted on multiple PaintControllers, its cacheGeneration will
be
> > valid on only one of the PaintControllers, so the object will be treated as
> > invalidated on other PaintControllers. As the situation is rare, its
performance
> > impact may be much less than that of canUseClientCacheStatus() which totally
> > disables caching on some layers. Will create a patch for this and schedule a
CT
> > task.
> > 
> > Do we really need to support multiple paint controllers? Having to support
that
> > usecase may cause complexity issues in the future. I poked around and the
> > non-graphics-layer ones seem to be all temporaries that we could refactor to
not
> > use paint controllers (or use them in a mode that is safer with the new
design).
> > Is there ever a case where a LayoutObject's display item is in two paint
> > controllers that are on different graphics layers?
> > 
> 
> The complexity is for spv1 only. In some cases an object may paint its mask,
background, foreground on different graphics layers.
> 
> > 
> > I have an unrelated question about the generation id... Can you help me
> > understand why it's needed? Is the generation id just an optimization so we
> > don't need to manually mark display item clients as uncached?
> 
> The id indicates whether an object has up-to-date painting on a
PaintController. It has the following benefits (compared to using a bool flag):
> - For clients not painted (e.g. because it's out of interest rect or obscured)
we don't need to clear there cache flags. Clearing the flags is non-trivial when
we skip painting of a big subtree.
> - When a PaintController is fully invalidated or recreated, we don't need to
bother cache flags of the clients. They are automatically treated as invalidated
because there ids mismatch.
> - If a client is painted on multiple PaintControllers, its treated as valid on
one of the only. This avoids complexity to deal with the rare case. (Will add
stat result soon.)
> 
> > An alternative
> > would be to store the SkPictures (1x per phase) directly on the LayoutObject
so
> > paint invalidation just frees the cached pictures directly (RefPtrs keep
> > everything safe).
> 
> I think this is worth exploring. We might have to store SkPictures 1 per
drawing display item id in order to keep the order of display items of ancestors
and descendants in the same paint phase (to support cases like
parent-display-item-a, child-display-item-b, parent-display-item-c). The most
difficult part might be finding a proper data structure.

I'm not so much in favor of this. Especially since it's an issue for SPv1 only.
It's a big departure from our current implementation in which the painting
system is outside of layout.

[pdr., 2016-04-26 18:56:16]

On 2016/04/26 at 16:59:35, wangxianzhu wrote:
> On 2016/04/26 04:04:11, pdr wrote:
> > Do we really need to support multiple paint controllers? Having to support
that
> > usecase may cause complexity issues in the future. I poked around and the
> > non-graphics-layer ones seem to be all temporaries that we could refactor to
not
> > use paint controllers (or use them in a mode that is safer with the new
design).
> > Is there ever a case where a LayoutObject's display item is in two paint
> > controllers that are on different graphics layers?
> > 
> 
> The complexity is for spv1 only. In some cases an object may paint its mask,
background, foreground on different graphics layers.

Just wondering... if this is so rare, could we disable caching in this case? I
think that's effectively what the current code will do, because the cache
generation id will increment for each of the 3 graphics layers in your example,
which will cause all display items to be invalid in the other 2 graphics layers.

[chrishtr, 2016-04-26 19:01:08]

On 2016/04/26 at 18:56:16, pdr wrote:
> On 2016/04/26 at 16:59:35, wangxianzhu wrote:
> > On 2016/04/26 04:04:11, pdr wrote:
> > > Do we really need to support multiple paint controllers? Having to support
that
> > > usecase may cause complexity issues in the future. I poked around and the
> > > non-graphics-layer ones seem to be all temporaries that we could refactor
to not
> > > use paint controllers (or use them in a mode that is safer with the new
design).
> > > Is there ever a case where a LayoutObject's display item is in two paint
> > > controllers that are on different graphics layers?
> > > 
> > 
> > The complexity is for spv1 only. In some cases an object may paint its mask,
background, foreground on different graphics layers.
> 
> Just wondering... if this is so rare, could we disable caching in this case? I
think that's effectively what the current code will do, because the cache
generation id will increment for each of the 3 graphics layers in your example,
which will cause all display items to be invalid in the other 2 graphics layers.

I think this is what Xianzhu's code is doing. He's just trying to find a way to
contain the perf issue to the % of time it happens.

[Xianzhu, 2016-04-26 19:03:20]

On 2016/04/26 18:56:16, pdr wrote:
> On 2016/04/26 at 16:59:35, wangxianzhu wrote:
> > On 2016/04/26 04:04:11, pdr wrote:
> > > Do we really need to support multiple paint controllers? Having to support
> that
> > > usecase may cause complexity issues in the future. I poked around and the
> > > non-graphics-layer ones seem to be all temporaries that we could refactor
to
> not
> > > use paint controllers (or use them in a mode that is safer with the new
> design).
> > > Is there ever a case where a LayoutObject's display item is in two paint
> > > controllers that are on different graphics layers?
> > > 
> > 
> > The complexity is for spv1 only. In some cases an object may paint its mask,
> background, foreground on different graphics layers.
> 
> Just wondering... if this is so rare, could we disable caching in this case? I
> think that's effectively what the current code will do, because the cache
> generation id will increment for each of the 3 graphics layers in your
example,
> which will cause all display items to be invalid in the other 2 graphics
layers.

The current patch fully disables caching for some
GraphicsLayers/PaintControllers which caused the regression.

The next patch will enable cache for the display items on one layer and
automatically disable cache of the item on other layers. This is just to remove
PaintController::canUseClientCacheStatus().

[Xianzhu, 2016-04-27 17:07:15]

It's ready for review. Ptal.

CT run 808 shows performance gain: record_time_subsequence_caching_disabled
-2.6%.

[Xianzhu, 2016-04-27 19:26:29]

The latest patch changes back to use 32-bit cache generation. Assuming there is
an animation in 60fps needing main-thread repaint, the cache generation will
overflow after 2^32/86400/60 = 828 days. When it overflows, we may treat some
object that is not cached as cached if the following conditions are all met:
- the object was painted when the cache generation was n;
- the object has not been painted since cache generation n;
- when the cache generation wraps back to exact n (after several years), the
object happens to be painted again and treated as cached. As the paint
controller doesn't have cached display items for the object, there will be
corrupted painting or assertion failure. I think the chance is too low to be
concerned.

We can treat a 64-bit cache generation never overflow, but it will increase size
of LayoutObject by more bytes.

[chrishtr, 2016-04-28 00:53:37]

https://codereview.chromium.org/1508223005/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/1508223005/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:143: // unsigned
cacheGeneration;
?

https://codereview.chromium.org/1508223005/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/line/InlineBox.cpp (right):

https://codereview.chromium.org/1508223005/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/line/InlineBox.cpp:46: // unsigned e;
?

https://codereview.chromium.org/1508223005/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/DisplayItemClient.h
(right):

https://codereview.chromium.org/1508223005/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/DisplayItemClient.h:55: #if 0
#if 0?

https://codereview.chromium.org/1508223005/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/DisplayItemClient.h:70:
#define DISPLAY_ITEM_CACHE_STATUS_UNCACHEABLE_IMPLEMENTATION
DISPLAY_ITEM_CACHE_STATUS_IMPLEMENTATION
Uncached and cached are the same?

[Xianzhu, 2016-04-28 16:37:55]

On 2016/04/28 00:53:37, chrishtr wrote:
>
https://codereview.chromium.org/1508223005/diff/220001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):
> 
>
https://codereview.chromium.org/1508223005/diff/220001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/layout/LayoutObject.cpp:143: // unsigned
> cacheGeneration;
> ?
> 
>
https://codereview.chromium.org/1508223005/diff/220001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/layout/line/InlineBox.cpp (right):
> 
>
https://codereview.chromium.org/1508223005/diff/220001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/layout/line/InlineBox.cpp:46: // unsigned e;
> ?
> 
>
https://codereview.chromium.org/1508223005/diff/220001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/platform/graphics/paint/DisplayItemClient.h
> (right):
> 
>
https://codereview.chromium.org/1508223005/diff/220001/third_party/WebKit/Sou...
> third_party/WebKit/Source/platform/graphics/paint/DisplayItemClient.h:55: #if
0
> #if 0?
> 
>
https://codereview.chromium.org/1508223005/diff/220001/third_party/WebKit/Sou...
> third_party/WebKit/Source/platform/graphics/paint/DisplayItemClient.h:70:
> #define DISPLAY_ITEM_CACHE_STATUS_UNCACHEABLE_IMPLEMENTATION
> DISPLAY_ITEM_CACHE_STATUS_IMPLEMENTATION
> Uncached and cached are the same?

You looked at a version that I uploaded to diagnose the performance regression.
The version tries not to increase size of any object, and just treat any item
validly cached during rasterize_and_record_micro test without extra cost.

Please review the Patch Set 17 instead :)

[chrishtr, 2016-04-28 20:22:15]

I don't see how the current patch set handles elements which might be painted
into multiple
PaintControllers.

https://codereview.chromium.org/1508223005/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/svg/graphics/SVGImage.h (right):

https://codereview.chromium.org/1508223005/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/graphics/SVGImage.h:92:
DISPLAY_ITEM_CACHE_STATUS_UNCACHEABLE_IMPLEMENTATION
Why can't we cache SVGImage?

https://codereview.chromium.org/1508223005/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/exported/WebFont.cpp (right):

https://codereview.chromium.org/1508223005/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/exported/WebFont.cpp:45:
DISPLAY_ITEM_CACHE_STATUS_UNCACHEABLE_IMPLEMENTATION
Ditto.

https://codereview.chromium.org/1508223005/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/exported/WebScrollbarThemeClientImpl.h
(right):

https://codereview.chromium.org/1508223005/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/exported/WebScrollbarThemeClientImpl.h:89:
DISPLAY_ITEM_CACHE_STATUS_UNCACHEABLE_IMPLEMENTATION
Ditto.

[Xianzhu, 2016-04-28 23:27:07]

On 2016/04/28 20:22:15, chrishtr wrote:
> I don't see how the current patch set handles elements which might be painted
> into multiple
> PaintControllers.
> 

If an object is painted on multiple paint controllers, because cache generations
are unique, the object's cache generation matches the last paint controller
only. We'll have to repaint the object on other paint controllers, but because
the situation is very rare, the performance penalty will be trivial. This is a
benefit of cache generation over bool flag.

>
https://codereview.chromium.org/1508223005/diff/340001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/svg/graphics/SVGImage.h (right):
> 
>
https://codereview.chromium.org/1508223005/diff/340001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/svg/graphics/SVGImage.h:92:
> DISPLAY_ITEM_CACHE_STATUS_UNCACHEABLE_IMPLEMENTATION
> Why can't we cache SVGImage?
> 
>
https://codereview.chromium.org/1508223005/diff/340001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/platform/exported/WebFont.cpp (right):
> 
>
https://codereview.chromium.org/1508223005/diff/340001/third_party/WebKit/Sou...
> third_party/WebKit/Source/platform/exported/WebFont.cpp:45:
> DISPLAY_ITEM_CACHE_STATUS_UNCACHEABLE_IMPLEMENTATION
> Ditto.
> 
>
https://codereview.chromium.org/1508223005/diff/340001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/platform/exported/WebScrollbarThemeClientImpl.h
> (right):
> 
>
https://codereview.chromium.org/1508223005/diff/340001/third_party/WebKit/Sou...
> third_party/WebKit/Source/platform/exported/WebScrollbarThemeClientImpl.h:89:
> DISPLAY_ITEM_CACHE_STATUS_UNCACHEABLE_IMPLEMENTATION
> Ditto.

Most of the above UNCACHEBLES are because they are painted by SkPictureBuilder
only where caching is not applicable.
https://codereview.chromium.org/1931133002/ lets SkPictureBuilder be
DisplayItemClient, to avoid such objects from implementing DisplayItemClient.

[chrishtr, 2016-04-29 00:11:59]

On 2016/04/28 at 23:27:07, wangxianzhu wrote:
> On 2016/04/28 20:22:15, chrishtr wrote:
> > I don't see how the current patch set handles elements which might be
painted
> > into multiple
> > PaintControllers.
> > 
> 
> If an object is painted on multiple paint controllers, because cache
generations are unique, the object's cache generation matches the last paint
controller only. We'll have to repaint the object on other paint controllers,
but because the situation is very rare, the performance penalty will be trivial.
This is a benefit of cache generation over bool flag.

Ah, yes! A very clever observation and implementation.

I don't think this subtlety is documented. Please document it in the code and
also somewhere in Markdown, and also explicitly
note that some DisplayItemClients may paint into multiple PaintControllers, but
the uniqueness property magically handles this case.

> 
> >
https://codereview.chromium.org/1508223005/diff/340001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/core/svg/graphics/SVGImage.h (right):
> > 
> >
https://codereview.chromium.org/1508223005/diff/340001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/svg/graphics/SVGImage.h:92:
> > DISPLAY_ITEM_CACHE_STATUS_UNCACHEABLE_IMPLEMENTATION
> > Why can't we cache SVGImage?
> > 
> >
https://codereview.chromium.org/1508223005/diff/340001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/platform/exported/WebFont.cpp (right):
> > 
> >
https://codereview.chromium.org/1508223005/diff/340001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/platform/exported/WebFont.cpp:45:
> > DISPLAY_ITEM_CACHE_STATUS_UNCACHEABLE_IMPLEMENTATION
> > Ditto.
> > 
> >
https://codereview.chromium.org/1508223005/diff/340001/third_party/WebKit/Sou...
> > File
third_party/WebKit/Source/platform/exported/WebScrollbarThemeClientImpl.h
> > (right):
> > 
> >
https://codereview.chromium.org/1508223005/diff/340001/third_party/WebKit/Sou...
> >
third_party/WebKit/Source/platform/exported/WebScrollbarThemeClientImpl.h:89:
> > DISPLAY_ITEM_CACHE_STATUS_UNCACHEABLE_IMPLEMENTATION
> > Ditto.
> 
> Most of the above UNCACHEBLES are because they are painted by SkPictureBuilder
only where caching is not applicable.
https://codereview.chromium.org/1931133002/ lets SkPictureBuilder be
DisplayItemClient, to avoid such objects from implementing DisplayItemClient.

Ok. I'll review that now then. Will we be able to avoid this macro?

[Xianzhu, 2016-04-29 01:10:25]

Description was changed from

==========
Client side display item cache flag

This is to reduce cost of hash lookups in PaintController.

BUG=550044
==========

to

==========
Client side display item cache flag

This is to reduce cost of hash lookups in PaintController.
See platform/graphics/paint/README.md and DisplayItemClient.h
for how it works.

BUG=550044
==========

[Xianzhu, 2016-04-29 01:24:46]

On 2016/04/29 00:11:59, chrishtr wrote:
> On 2016/04/28 at 23:27:07, wangxianzhu wrote:
> > On 2016/04/28 20:22:15, chrishtr wrote:
> > > I don't see how the current patch set handles elements which might be
> painted
> > > into multiple
> > > PaintControllers.
> > > 
> > 
> > If an object is painted on multiple paint controllers, because cache
> generations are unique, the object's cache generation matches the last paint
> controller only. We'll have to repaint the object on other paint controllers,
> but because the situation is very rare, the performance penalty will be
trivial.
> This is a benefit of cache generation over bool flag.
> 
> Ah, yes! A very clever observation and implementation.
> 
> I don't think this subtlety is documented. Please document it in the code and
> also somewhere in Markdown, and also explicitly
> note that some DisplayItemClients may paint into multiple PaintControllers,
but
> the uniqueness property magically handles this case.
> 

Added doc in DisplayItemClient.h and platform/graphics/paint/README.md.

> > 
> > >
>
https://codereview.chromium.org/1508223005/diff/340001/third_party/WebKit/Sou...
> > > File third_party/WebKit/Source/core/svg/graphics/SVGImage.h (right):
> > > 
> > >
>
https://codereview.chromium.org/1508223005/diff/340001/third_party/WebKit/Sou...
> > > third_party/WebKit/Source/core/svg/graphics/SVGImage.h:92:
> > > DISPLAY_ITEM_CACHE_STATUS_UNCACHEABLE_IMPLEMENTATION
> > > Why can't we cache SVGImage?
> > > 
> > >
>
https://codereview.chromium.org/1508223005/diff/340001/third_party/WebKit/Sou...
> > > File third_party/WebKit/Source/platform/exported/WebFont.cpp (right):
> > > 
> > >
>
https://codereview.chromium.org/1508223005/diff/340001/third_party/WebKit/Sou...
> > > third_party/WebKit/Source/platform/exported/WebFont.cpp:45:
> > > DISPLAY_ITEM_CACHE_STATUS_UNCACHEABLE_IMPLEMENTATION
> > > Ditto.
> > > 
> > >
>
https://codereview.chromium.org/1508223005/diff/340001/third_party/WebKit/Sou...
> > > File
> third_party/WebKit/Source/platform/exported/WebScrollbarThemeClientImpl.h
> > > (right):
> > > 
> > >
>
https://codereview.chromium.org/1508223005/diff/340001/third_party/WebKit/Sou...
> > >
> third_party/WebKit/Source/platform/exported/WebScrollbarThemeClientImpl.h:89:
> > > `kInvalidCacheGeneration` is a special cache generation value which
matches no
other cache generations. When a `DisplayItemClient` is invalidated, we set its
cache generation to `kInvalidCacheGeneration`. When a `PaintController` is
cleared (e.g. when the corresponding `GraphicsLayer` is fully invalidated), we
also set its cache generation to `kInvalidCacheGeneration`. 


> > > Ditto.
> > 
> > Most of the above UNCACHEBLES are because they are painted by
SkPictureBuilder
> only where caching is not applicable.
> https://codereview.chromium.org/1931133002/ lets SkPictureBuilder be
> DisplayItemClient, to avoid such objects from implementing DisplayItemClient.
> 
> Ok. I'll review that now then. Will we be able to avoid this macro?

Removed DISPLAY_ITEM_CACHE_STATUS_UNCACHEABLE_IMPLEMENTATION, and added default
implementation for those DisplayItemClients not caring about caching (there are
still several of them: some testing stubs and page overlay).

[chrishtr, 2016-04-29 15:55:17]

https://codereview.chromium.org/1508223005/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/DisplayItemClient.h
(right):

https://codereview.chromium.org/1508223005/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/DisplayItemClient.h:69: // Use
DISPLAY_ITEM_CACHE_STATUS_IMPLEMENTATION for cacheable implementation.
I think it would be better to do the opposite, so we don't accidentally omit
caching where
not intended.

https://codereview.chromium.org/1508223005/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/README.md (right):

https://codereview.chromium.org/1508223005/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/README.md:208: overflow after
2^32/86400/60 = 828 days. When it overflows, we may treat some
This is only with one PaintController. Could be less with more.

[Xianzhu, 2016-04-29 16:37:11]

https://codereview.chromium.org/1508223005/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/DisplayItemClient.h
(right):

https://codereview.chromium.org/1508223005/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/DisplayItemClient.h:69: // Use
DISPLAY_ITEM_CACHE_STATUS_IMPLEMENTATION for cacheable implementation.
On 2016/04/29 15:55:17, chrishtr wrote:
> I think it would be better to do the opposite, so we don't accidentally omit
> caching where
> not intended.

Agreed. Done.

https://codereview.chromium.org/1508223005/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/README.md (right):

https://codereview.chromium.org/1508223005/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/README.md:208: overflow after
2^32/86400/60 = 828 days. When it overflows, we may treat some
On 2016/04/29 15:55:17, chrishtr wrote:
> This is only with one PaintController. Could be less with more.

Yes. Added a sentence for this fact.

[chrishtr, 2016-04-29 16:51:15]

https://codereview.chromium.org/1508223005/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/DisplayItemClient.h
(right):

https://codereview.chromium.org/1508223005/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/DisplayItemClient.h:68:
virtual bool displayItemsAreCached(DisplayItemCacheGeneration) const = 0;
Just move the definitions from DISPLAY_ITEM_CACHE_STATUS_IMPLEMENTATION here?

[Xianzhu, 2016-04-29 17:17:31]

https://codereview.chromium.org/1508223005/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/DisplayItemClient.h
(right):

https://codereview.chromium.org/1508223005/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/DisplayItemClient.h:68:
virtual bool displayItemsAreCached(DisplayItemCacheGeneration) const = 0;
On 2016/04/29 16:51:15, chrishtr wrote:
> Just move the definitions from DISPLAY_ITEM_CACHE_STATUS_IMPLEMENTATION here?

That would make DisplayItemClient no longer a pure interface which may violate
style guide about multiple inheritance (though I'm not sure the real problems,
perhaps pointer offset when cast?).

Another problem is that the 32-bit data field may cause a 32-bit hole in the
derived class because of alignment. With the macro, the subclass can put the
macro at a proper place to avoid the hole. In this Cl I put the macro at the end
of the classes.

[chrishtr, 2016-04-29 19:51:58]

lgtm

[Xianzhu, 2016-04-29 19:56:53]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2016-04-29 19:57:18]

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

[commit-bot: I haz the power, 2016-04-29 21:21:33]

Description was changed from

==========
Client side display item cache flag

This is to reduce cost of hash lookups in PaintController.
See platform/graphics/paint/README.md and DisplayItemClient.h
for how it works.

BUG=550044
==========

to

==========
Client side display item cache flag

This is to reduce cost of hash lookups in PaintController.
See platform/graphics/paint/README.md and DisplayItemClient.h
for how it works.

BUG=550044
==========

[commit-bot: I haz the power, 2016-04-29 21:21:35]

Committed patchset #20 (id:380001)

[commit-bot: I haz the power, 2016-04-30 17:29:07]

Patchset 20 (id:??) landed as
https://crrev.com/93b971703db1f49a431ddeec2ebc884a232ee6de
Cr-Commit-Position: refs/heads/master@{#390763}

[commit-bot: I haz the power, 2016-04-30 18:02:42]

Description was changed from

==========
Client side display item cache flag

This is to reduce cost of hash lookups in PaintController.
See platform/graphics/paint/README.md and DisplayItemClient.h
for how it works.

BUG=550044
==========

to

==========
Client side display item cache flag

This is to reduce cost of hash lookups in PaintController.
See platform/graphics/paint/README.md and DisplayItemClient.h
for how it works.

BUG=550044

Committed: https://crrev.com/93b971703db1f49a431ddeec2ebc884a232ee6de
Cr-Commit-Position: refs/heads/master@{#390763}
==========

[Xianzhu, 2016-05-16 18:03:33]

A revert of this CL (patchset #20 id:380001) has been created in
https://codereview.chromium.org/1979133003/ by wangxianzhu@chromium.org.

The reason for reverting is: This caused crbug.com/609218. It seems that there
is some short-lived
DisplayItemClient but needs more investigations to find which
DisplayItemClient it is.

Revert this to unblock m-52 branch and beta release.

Will reland on m-53 and add some code in release builds to find the
reason.

BUG=609218.