GraphicsContext culling support.

GraphicsContext culling support.

This CL plumbs the Skia culling API in GraphicsContext, and uses it for
container (RenderSVGContainer & RenderBlock) culling (currently behind
an experimental setting).

R=schenney@chromium.org,senorblanco@chromium.org,pdr@chromium.org

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

[f(malita), 2014-03-21 16:54:49]

This is mainly intended to expose the culling API in GC, but it locks it in
using a couple of container clients (behind a currently-unplumbed runtime flag):
RenderBlock & RenderSVGContainer.

[Stephen Chennney, 2014-03-21 19:44:19]

I don't have any serious complaints, to be honest. Do you have buy-in from all
the stakeholders?

Why land behind a flag? It seems like it's a win either way. More importantly,
if you planning to plumb this all the way through to the Chromium command line,
then I would make this a setting dependent on the go-fast-and-break-things
setting, as it really falls under the same umbrella in my mind.

https://codereview.chromium.org/204093002/diff/20001/Source/platform/graphics...
File Source/platform/graphics/GraphicsContextCullSaver.h (right):

https://codereview.chromium.org/204093002/diff/20001/Source/platform/graphics...
Source/platform/graphics/GraphicsContextCullSaver.h:11: class PLATFORM_EXPORT
GraphicsContextCullSaver {
You don't need this for a fully inlined class.

[f(malita), 2014-03-24 15:41:15]

On 2014/03/21 19:44:19, Stephen Chenney wrote:
> I don't have any serious complaints, to be honest. Do you have buy-in from all
> the stakeholders?
> 
> Why land behind a flag? It seems like it's a win either way. More importantly,
> if you planning to plumb this all the way through to the Chromium command
line,
> then I would make this a setting dependent on the go-fast-and-break-things
> setting, as it really falls under the same umbrella in my mind.

Are you thinking of --enable-experimental-web-platform-features? My concern is
that it conflates several features which may affect perf, making it hard to
measure the impact of culling in isolation.

At this point, the culling logic is pure overhead for impl-side painting: bbox
hierarchy recorders do not take advantage of this information yet, and their
synthetic hierarchies are more efficient than the Blink implicit bboxes. There
are two ways to make use of it going forward:

1) as a BBH recorder replacement - the initial two cull pushers are a step in
this direction, but the current perf numbers are not there yet: ~-12% record
time but ~+3% rasterize time (cluster Telemetry run on top 10K). Based on
previous prototyping, it will also take culling at the RenderLayer level to get
competitive numbers. 

2) as a BBH recorder bounds source (to avoid re-measuring each op) - this will
require Blink to provide tight bounds for individual draw operations, which
doesn't seem to be the case at the moment.

For either use case, more work and accurate measurements are needed to make it
competitive with the current numbers - hence the runtime flag. But I think it'
important to get the API in place in order to enable such work.


>
https://codereview.chromium.org/204093002/diff/20001/Source/platform/graphics...
> File Source/platform/graphics/GraphicsContextCullSaver.h (right):
> 
>
https://codereview.chromium.org/204093002/diff/20001/Source/platform/graphics...
> Source/platform/graphics/GraphicsContextCullSaver.h:11: class PLATFORM_EXPORT
> GraphicsContextCullSaver {
> You don't need this for a fully inlined class.

Done.

(Strangely, GraphicsContextCullSaver seems to cause Win link errors with the
export in place, even thought it is fully mimicking GraphicsContextStateSaver
AFAICT. Will have to investigate.)

[Stephen Chennney, 2014-03-24 15:53:12]

On 2014/03/24 15:41:15, Florin Malita wrote:
> On 2014/03/21 19:44:19, Stephen Chenney wrote:
> > I don't have any serious complaints, to be honest. Do you have buy-in from
all
> > the stakeholders?
> > 
> > Why land behind a flag? It seems like it's a win either way. More
importantly,
> > if you planning to plumb this all the way through to the Chromium command
> line,
> > then I would make this a setting dependent on the go-fast-and-break-things
> > setting, as it really falls under the same umbrella in my mind.
> 
> Are you thinking of --enable-experimental-web-platform-features? My concern is
> that it conflates several features which may affect perf, making it hard to
> measure the impact of culling in isolation.

Fair enough.

> At this point, the culling logic is pure overhead for impl-side painting: bbox
> hierarchy recorders do not take advantage of this information yet, and their
> synthetic hierarchies are more efficient than the Blink implicit bboxes. There
> are two ways to make use of it going forward:
> 
> 1) as a BBH recorder replacement - the initial two cull pushers are a step in
> this direction, but the current perf numbers are not there yet: ~-12% record
> time but ~+3% rasterize time (cluster Telemetry run on top 10K). Based on
> previous prototyping, it will also take culling at the RenderLayer level to
get
> competitive numbers. 
> 
> 2) as a BBH recorder bounds source (to avoid re-measuring each op) - this will
> require Blink to provide tight bounds for individual draw operations, which
> doesn't seem to be the case at the moment.
> 
> For either use case, more work and accurate measurements are needed to make it
> competitive with the current numbers - hence the runtime flag. But I think it'
> important to get the API in place in order to enable such work.

OK. I'm on board with that.

> (Strangely, GraphicsContextCullSaver seems to cause Win link errors with the
> export in place, even thought it is fully mimicking GraphicsContextStateSaver
> AFAICT. Will have to investigate.)

Is it held in an OwnPtr? That introduces the need for certain constructors so
you probably want WTF_NONCOPYABLE.

[Stephen Chennney, 2014-03-24 15:55:17]

lgtm

[f(malita), 2014-03-24 16:01:07]

Thanks.

On 2014/03/24 15:53:12, Stephen Chenney wrote:
> On 2014/03/24 15:41:15, Florin Malita wrote:
> > (Strangely, GraphicsContextCullSaver seems to cause Win link errors with the
> > export in place, even thought it is fully mimicking
GraphicsContextStateSaver
> > AFAICT. Will have to investigate.)
> 
> Is it held in an OwnPtr? That introduces the need for certain constructors so
> you probably want WTF_NONCOPYABLE.

Just stack allocated ATM. Still looking.

[f(malita), 2014-03-24 19:37:00]

On 2014/03/24 16:01:07, Florin Malita wrote:
> Thanks.
> 
> On 2014/03/24 15:53:12, Stephen Chenney wrote:
> > On 2014/03/24 15:41:15, Florin Malita wrote:
> > > (Strangely, GraphicsContextCullSaver seems to cause Win link errors with
the
> > > export in place, even thought it is fully mimicking
> GraphicsContextStateSaver
> > > AFAICT. Will have to investigate.)
> > 
> > Is it held in an OwnPtr? That introduces the need for certain constructors
so
> > you probably want WTF_NONCOPYABLE.
> 
> Just stack allocated ATM. Still looking.

Well, looks like it's linking fine now. I'll blame it on some un-clobbered bot.

[f(malita), 2014-03-25 12:37:21]

The CQ bit was checked by fmalita@chromium.org

[commit-bot: I haz the power, 2014-03-25 12:37:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fmalita@chromium.org/204093002/30001

[commit-bot: I haz the power, 2014-03-25 12:45:07]

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

[commit-bot: I haz the power, 2014-03-25 12:45:07]

Try jobs failed on following builders:
  tryserver.blink on blink_presubmit

[f(malita), 2014-03-25 12:52:41]

On 2014/03/25 12:45:07, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   tryserver.blink on blink_presubmit

+jamesr for web/ setting plumbing.

[f(malita), 2014-03-27 01:16:43]

On 2014/03/25 12:52:41, Florin Malita wrote:
> On 2014/03/25 12:45:07, I haz the power (commit-bot) wrote:
> > Try jobs failed on following builders:
> >   tryserver.blink on blink_presubmit
> 
> +jamesr for web/ setting plumbing.

+abarth for the same (jamesr is ooo).

[abarth-chromium, 2014-03-27 03:44:25]

lgtm

[abarth-chromium, 2014-03-27 03:44:31]

The CQ bit was checked by abarth@chromium.org

[commit-bot: I haz the power, 2014-03-27 03:44:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fmalita@chromium.org/204093002/50001

[commit-bot: I haz the power, 2014-03-27 03:47:36]

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

[commit-bot: I haz the power, 2014-03-27 03:47:37]

Try jobs failed on following builders:
  tryserver.blink on blink_presubmit

[f(malita), 2014-03-27 13:11:39]

The CQ bit was checked by fmalita@chromium.org

[commit-bot: I haz the power, 2014-03-27 13:11:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fmalita@chromium.org/204093002/50001

[commit-bot: I haz the power, 2014-03-27 13:22:36]

Change committed as 170174