Abort canvas analysis after a few operations are rejected

Abort canvas analysis after a few operations are rejected

This is to avoid costly analysis for a display item containing huge
number of operations, especially drawRect operations drawing small
rects.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/6ffa6bc9931431c5f5357659ea931086c1eea625
Cr-Commit-Position: refs/heads/master@{#426507}

[Xianzhu, 2016-10-18 22:00:10]

Description was changed from

==========
Don't analyze solid color if display list contains too many operations
==========

to

==========
Don't analyze solid color if display list contains too many operations
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[Xianzhu, 2016-10-18 23:21:01]

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

[Xianzhu, 2016-10-18 23:21:01]

I'm not sure if this is correct, but it does improve performance of a test
(	third_party/WebKit/PerformanceTests/Paint/large-table-repaint.html in
https://codereview.chromium.org/2429623004/) by 1 fold.

[vmpstr, 2016-10-18 23:31:34]

https://codereview.chromium.org/2425323002/diff/1/cc/playback/raster_source.cc
File cc/playback/raster_source.cc (right):

https://codereview.chromium.org/2425323002/diff/1/cc/playback/raster_source.c...
cc/playback/raster_source.cc:227: if
(!display_list_->ShouldBeAnalyzedForSolidColor())
That's not quite correct, since the given content_rect may actually clip out a
lot of the operations and still find a solid color tile. This patch would make
it so that no tile is ever solid color on a page with enough operations
anywhere.

I think I can construct a page that has blank content at the top and some
content (enough for for ShouldBeAnalyzedForSolidColor to be false) below that.
Then this would prevent solid color analysis to be done on tiles above.

[Xianzhu, 2016-10-19 01:27:53]

https://codereview.chromium.org/2425323002/diff/1/cc/playback/raster_source.cc
File cc/playback/raster_source.cc (right):

https://codereview.chromium.org/2425323002/diff/1/cc/playback/raster_source.c...
cc/playback/raster_source.cc:227: if
(!display_list_->ShouldBeAnalyzedForSolidColor())
On 2016/10/18 23:31:33, vmpstr wrote:
> That's not quite correct, since the given content_rect may actually clip out a
> lot of the operations and still find a solid color tile. This patch would make
> it so that no tile is ever solid color on a page with enough operations
> anywhere.
> 
> I think I can construct a page that has blank content at the top and some
> content (enough for for ShouldBeAnalyzedForSolidColor to be false) below that.
> Then this would prevent solid color analysis to be done on tiles above. 
> 

For my test, this does avoid big cost when analyzing a display list containing
hundreds of thousands of operations. It takes 30ms to analyze each tile.

How bad is it when we treat a solid-color tile non-solid-color? Can we use a
different threshold for tile solid-color analysis (for example, 1000
operations)?

[Xianzhu, 2016-10-19 15:44:11]

I just read more code and found that you already created early-out mechanism in
AnalysisCanvas to avoid costly solid-color analysis. Debugging to see why this
doesn't work for my case in which a big display item contains a huge number of
drawing operations (which only occurs with a local experiment CL).

[Xianzhu, 2016-10-19 16:18:57]

Description was changed from

==========
Don't analyze solid color if display list contains too many operations
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Abort canvas analysis after many operations are rejected

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[Xianzhu, 2016-10-19 16:21:11]

Description was changed from

==========
Abort canvas analysis after many operations are rejected

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Abort canvas analysis after a few operations are rejected

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[Xianzhu, 2016-10-19 16:22:17]

Description was changed from

==========
Abort canvas analysis after a few operations are rejected

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Abort canvas analysis after a few operations are rejected

This is to avoid costly analysis for a display item containing huge
number of operations, especially drawRect operations drawing a small
rects.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[Xianzhu, 2016-10-19 16:41:44]

On 2016/10/19 15:44:11, Xianzhu wrote:
> I just read more code and found that you already created early-out mechanism
in
> AnalysisCanvas to avoid costly solid-color analysis. Debugging to see why this
> doesn't work for my case in which a big display item contains a huge number of
> drawing operations (which only occurs with a local experiment CL).

I think I found the reason. In my case the huge display item contains 384000
drawRect operations drawing small rects. When analyzing solid-color in a rect at
the bottom-right corner, we check eligibility of many operations before seeing
eligible operations and aborting.

The huge display item defeats R-tree and the current solid-color analysis.
Uploaded a CL to abort analysis after rejecting a few drawRect operations. PTAL.

[vmpstr, 2016-10-19 18:06:40]

lgtm thanks! Could you update the title of this CL before landing as well?

[vmpstr, 2016-10-19 18:07:03]

On 2016/10/19 18:06:40, vmpstr wrote:
> lgtm thanks! Could you update the title of this CL before landing as well?

(Also I'm not an owner in skia/ext, so you'll need someone else to stamp)

[Xianzhu, 2016-10-19 18:07:49]

Description was changed from

==========
Abort canvas analysis after a few operations are rejected

This is to avoid costly analysis for a display item containing huge
number of operations, especially drawRect operations drawing a small
rects.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Abort canvas analysis after a few operations are rejected

This is to avoid costly analysis for a display item containing huge
number of operations, especially drawRect operations drawing small
rects.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[Xianzhu, 2016-10-19 18:10:25]

The CQ bit was checked by wangxianzhu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-19 18:10:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Xianzhu, 2016-10-19 18:11:43]

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

[Xianzhu, 2016-10-19 18:11:44]

On 2016/10/19 18:07:03, vmpstr wrote:
> On 2016/10/19 18:06:40, vmpstr wrote:
> > lgtm thanks! Could you update the title of this CL before landing as well?
> 

Done.

> (Also I'm not an owner in skia/ext, so you'll need someone else to stamp)

+rmistry

[rmistry, 2016-10-19 18:15:09]

rmistry@google.com changed reviewers:
+ fmalita@gmail.com, reed@google.com, rmistry@google.com

[commit-bot: I haz the power, 2016-10-19 20:48:18]

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

[commit-bot: I haz the power, 2016-10-19 20:48:19]

Dry run: This issue passed the CQ dry run.

[Xianzhu, 2016-10-20 16:34:42]

ping...

[reed1, 2016-10-20 16:36:51]

owner lgtm

[Xianzhu, 2016-10-20 16:37:35]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2016-10-20 16:37:36]

The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org
Link to the patchset: https://codereview.chromium.org/2425323002/#ps40001
(title: "Update comments")

[commit-bot: I haz the power, 2016-10-20 16:37:57]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[f(malita), 2016-10-20 16:38:32]

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

[f(malita), 2016-10-20 16:38:32]

The code change looks fine, but the heuristic feels quite arbitrary.

I presume this is driven by some data though, so LGTM.

[Xianzhu, 2016-10-20 16:55:53]

On 2016/10/20 16:38:32, f(malita) wrote:
> The code change looks fine, but the heuristic feels quite arbitrary.
> 
> I presume this is driven by some data though, so LGTM.

Actually this is not based on data from experiments, but just observations from
the code base: most of our drawing display items contain just 1 drawing
operation.

https://codereview.chromium.org/2430313004/ will cause big display items
containing many drawRect operations (4 * number of cells in a table) and this CL
is to improve solid-color analysis performance of such display items. There will
be no obvious difference if we set the number to 5 or 10 because both numbers of
rejecting drawRect operations are fast. It just prevents cost of rejecting
hundreds of thousands of drawRect operations before aborting the analysis.

[commit-bot: I haz the power, 2016-10-20 16:57:41]

Description was changed from

==========
Abort canvas analysis after a few operations are rejected

This is to avoid costly analysis for a display item containing huge
number of operations, especially drawRect operations drawing small
rects.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Abort canvas analysis after a few operations are rejected

This is to avoid costly analysis for a display item containing huge
number of operations, especially drawRect operations drawing small
rects.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[commit-bot: I haz the power, 2016-10-20 16:57:42]

Committed patchset #3 (id:40001)

[vmpstr, 2016-10-20 17:53:40]

On 2016/10/20 16:38:32, f(malita) wrote:
> The code change looks fine, but the heuristic feels quite arbitrary.
> 
> I presume this is driven by some data though, so LGTM.

Analysis canvas we have does an early out after one draw op, so although 5
rejected ops is arbitrary, I feel like it's fine. It's just meant to say "don't
try too hard".

[commit-bot: I haz the power, 2016-10-21 13:19:16]

Description was changed from

==========
Abort canvas analysis after a few operations are rejected

This is to avoid costly analysis for a display item containing huge
number of operations, especially drawRect operations drawing small
rects.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Abort canvas analysis after a few operations are rejected

This is to avoid costly analysis for a display item containing huge
number of operations, especially drawRect operations drawing small
rects.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/6ffa6bc9931431c5f5357659ea931086c1eea625
Cr-Commit-Position: refs/heads/master@{#426507}
==========

[commit-bot: I haz the power, 2016-10-21 13:19:17]

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