Refactor DrawingRecorders to check for cached drawings earlier

Refactor DrawingRecorders to check for cached drawings earlier

This patch refactors the DrawingRecorders to call
[LayoutObject]DrawingRecorder::useCachedDrawingIfPossible(...) before
creating the recorder which lets us skip expensive cull rect calc.

Perf data: -3% record time:
https://storage.cloud.google.com/cluster-telemetry/tasks/chromium_perf_runs/pdr-1436182745.58/html/index.html

Callsites have been optimized as little as possible to keep this patch
reviewable. We may want to create a DrawingRecorderCacheParams
struct to avoid copying the parameters (context, client, type) twice
at all the callsites.

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

[pdr., 2015-07-03 00:43:53]

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

[Xianzhu, 2015-07-03 01:24:42]

https://codereview.chromium.org/1220583004/diff/20001/Source/core/paint/Block...
File Source/core/paint/BlockFlowPainter.cpp (right):

https://codereview.chromium.org/1220583004/diff/20001/Source/core/paint/Block...
Source/core/paint/BlockFlowPainter.cpp:72: // TODO(wkorman): Rework below to
process paint invalidation rects during layout rather than paint.
If this TODO is still valid, then we can't early return here. Otherwise we
should remove the TODO. I guess it's still valid.

https://codereview.chromium.org/1220583004/diff/20001/Source/web/PageOverlayT...
File Source/web/PageOverlayTest.cpp (right):

https://codereview.chromium.org/1220583004/diff/20001/Source/web/PageOverlayT...
Source/web/PageOverlayTest.cpp:108: if
(!DrawingRecorder::useCachedDrawingIfPossible(graphicsContext, *this,
DisplayItem::PageOverlay)) {
Nit:

'if (DrawingRecorder::...)
  return;'
seems better.

https://codereview.chromium.org/1220583004/diff/20001/Source/web/PageWidgetDe...
File Source/web/PageWidgetDelegate.cpp (right):

https://codereview.chromium.org/1220583004/diff/20001/Source/web/PageWidgetDe...
Source/web/PageWidgetDelegate.cpp:98: if
(!DrawingRecorder::useCachedDrawingIfPossible(paintContext, root,
DisplayItem::PageWidgetDelegateBackgroundFallback)) {
Nit: can use 'else if' to avoid indentation change.

https://codereview.chromium.org/1220583004/diff/20001/Source/web/WebFontImpl.cpp
File Source/web/WebFontImpl.cpp (right):

https://codereview.chromium.org/1220583004/diff/20001/Source/web/WebFontImpl....
Source/web/WebFontImpl.cpp:102: if
(!DrawingRecorder::useCachedDrawingIfPossible(*context, *this,
DisplayItem::WebFont)) {
Nit: Another similar place still uses ASSERT. Better to keep consistence.

[pdr., 2015-07-03 02:38:05]

Thanks for the review.

How do you feel about the DrawingRecorderCacheParams idea? It's a little more
verbose but it will force us to keep the phase in sync between the
useCachedDrawingIfPossible call and the recorder.

This patch:
if (LayoutObjectDrawingRecorder::useCachedDrawingIfPossible(context, client,
PaintPhase::SomePaintPhase)
    return;
LayoutObjectDrawingRecorder recorder(context, client,
PaintPhase::SomePaintPhase, ...);


Proposal:
DrawingRecorder::CacheParameters recorderCacheParameters(context, client,
PaintPhase::SomePhase);
if
(LayoutObjectDrawingRecorder::useCachedDrawingIfPossible(recorderCacheParameters))
    return;
LayoutObjectDrawingRecorder recorder(recorderCacheParameters, ...);

https://codereview.chromium.org/1220583004/diff/20001/Source/core/paint/Block...
File Source/core/paint/BlockFlowPainter.cpp (right):

https://codereview.chromium.org/1220583004/diff/20001/Source/core/paint/Block...
Source/core/paint/BlockFlowPainter.cpp:72: // TODO(wkorman): Rework below to
process paint invalidation rects during layout rather than paint.
On 2015/07/03 at 01:24:41, Xianzhu wrote:
> If this TODO is still valid, then we can't early return here. Otherwise we
should remove the TODO. I guess it's still valid.

Good catch. Fixing this required a small amount of logic. We now use
Optional<DrawingRecorder> to skip the creation of the recorder when the cache is
used, but still create it when the cache is not used. There are some simple
optimizations we can make here but I opted to keep this as simple as possible
because wkorman will be removing selection rects soon.

https://codereview.chromium.org/1220583004/diff/20001/Source/web/PageOverlayT...
File Source/web/PageOverlayTest.cpp (right):

https://codereview.chromium.org/1220583004/diff/20001/Source/web/PageOverlayT...
Source/web/PageOverlayTest.cpp:108: if
(!DrawingRecorder::useCachedDrawingIfPossible(graphicsContext, *this,
DisplayItem::PageOverlay)) {
On 2015/07/03 at 01:24:42, Xianzhu wrote:
> Nit:
> 
> 'if (DrawingRecorder::...)
>   return;'
> seems better.

I agree. Done.

https://codereview.chromium.org/1220583004/diff/20001/Source/web/PageWidgetDe...
File Source/web/PageWidgetDelegate.cpp (right):

https://codereview.chromium.org/1220583004/diff/20001/Source/web/PageWidgetDe...
Source/web/PageWidgetDelegate.cpp:98: if
(!DrawingRecorder::useCachedDrawingIfPossible(paintContext, root,
DisplayItem::PageWidgetDelegateBackgroundFallback)) {
On 2015/07/03 at 01:24:42, Xianzhu wrote:
> Nit: can use 'else if' to avoid indentation change.

Done.

https://codereview.chromium.org/1220583004/diff/20001/Source/web/WebFontImpl.cpp
File Source/web/WebFontImpl.cpp (right):

https://codereview.chromium.org/1220583004/diff/20001/Source/web/WebFontImpl....
Source/web/WebFontImpl.cpp:102: if
(!DrawingRecorder::useCachedDrawingIfPossible(*context, *this,
DisplayItem::WebFont)) {
On 2015/07/03 at 01:24:42, Xianzhu wrote:
> Nit: Another similar place still uses ASSERT. Better to keep consistence.

Good catch. Fixed.

[Xianzhu, 2015-07-03 20:03:21]

lgtm.

On 2015/07/03 02:38:05, pdr wrote:
> Thanks for the review.
> 
> How do you feel about the DrawingRecorderCacheParams idea? It's a little more
> verbose but it will force us to keep the phase in sync between the
> useCachedDrawingIfPossible call and the recorder.
> 
> This patch:
> if (LayoutObjectDrawingRecorder::useCachedDrawingIfPossible(context, client,
> PaintPhase::SomePaintPhase)
>     return;
> LayoutObjectDrawingRecorder recorder(context, client,
> PaintPhase::SomePaintPhase, ...);
> 
> 
> Proposal:
> DrawingRecorder::CacheParameters recorderCacheParameters(context, client,
> PaintPhase::SomePhase);
> if
>
(LayoutObjectDrawingRecorder::useCachedDrawingIfPossible(recorderCacheParameters))
>     return;
> LayoutObjectDrawingRecorder recorder(recorderCacheParameters, ...);
> 

I still like the less-verbose version. The ASSERT in DrawingRecorder should
catch the inconsistency.

[pdr., 2015-07-03 21:08:15]

On 2015/07/03 at 20:03:21, wangxianzhu wrote:
> lgtm.
> 
> On 2015/07/03 02:38:05, pdr wrote:
> > Thanks for the review.
> > 
> > How do you feel about the DrawingRecorderCacheParams idea? It's a little
more
> > verbose but it will force us to keep the phase in sync between the
> > useCachedDrawingIfPossible call and the recorder.
> > 
> > This patch:
> > if (LayoutObjectDrawingRecorder::useCachedDrawingIfPossible(context, client,
> > PaintPhase::SomePaintPhase)
> >     return;
> > LayoutObjectDrawingRecorder recorder(context, client,
> > PaintPhase::SomePaintPhase, ...);
> > 
> > 
> > Proposal:
> > DrawingRecorder::CacheParameters recorderCacheParameters(context, client,
> > PaintPhase::SomePhase);
> > if
> >
(LayoutObjectDrawingRecorder::useCachedDrawingIfPossible(recorderCacheParameters))
> >     return;
> > LayoutObjectDrawingRecorder recorder(recorderCacheParameters, ...);
> > 
> 
> I still like the less-verbose version. The ASSERT in DrawingRecorder should
catch the inconsistency.

SG

Patchset 2 actually showed a regression on cluster telemetry:
https://storage.cloud.google.com/cluster-telemetry/tasks/chromium_perf_runs/p...

Locally, with patchset 3, everything is a progression. I also confirmed the
profile with and without patch makes sense. I'm going to wait for a cluster
telemetry run of patchset 3 before landing.

[pdr., 2015-07-06 17:29:07]

On 2015/07/03 at 21:08:15, pdr wrote:
> On 2015/07/03 at 20:03:21, wangxianzhu wrote:
> > lgtm.
> > 
> > On 2015/07/03 02:38:05, pdr wrote:
> > > Thanks for the review.
> > > 
> > > How do you feel about the DrawingRecorderCacheParams idea? It's a little
more
> > > verbose but it will force us to keep the phase in sync between the
> > > useCachedDrawingIfPossible call and the recorder.
> > > 
> > > This patch:
> > > if (LayoutObjectDrawingRecorder::useCachedDrawingIfPossible(context,
client,
> > > PaintPhase::SomePaintPhase)
> > >     return;
> > > LayoutObjectDrawingRecorder recorder(context, client,
> > > PaintPhase::SomePaintPhase, ...);
> > > 
> > > 
> > > Proposal:
> > > DrawingRecorder::CacheParameters recorderCacheParameters(context, client,
> > > PaintPhase::SomePhase);
> > > if
> > >
(LayoutObjectDrawingRecorder::useCachedDrawingIfPossible(recorderCacheParameters))
> > >     return;
> > > LayoutObjectDrawingRecorder recorder(recorderCacheParameters, ...);
> > > 
> > 
> > I still like the less-verbose version. The ASSERT in DrawingRecorder should
catch the inconsistency.
> 
> SG
> 
> Patchset 2 actually showed a regression on cluster telemetry:
>
https://storage.cloud.google.com/cluster-telemetry/tasks/chromium_perf_runs/p...
> 
> Locally, with patchset 3, everything is a progression. I also confirmed the
profile with and without patch makes sense. I'm going to wait for a cluster
telemetry run of patchset 3 before landing.

This patch progresses record time by 3% but regresses record time with caching
,painting, or construction disabled.
https://storage.cloud.google.com/cluster-telemetry/tasks/chromium_perf_runs/p...

I think we should go ahead with this patch because record time is what really
matters, but I'd like a second opinion. This patch will also open up some
additional optimizations. WDYT?

[Xianzhu, 2015-07-06 17:54:17]

On 2015/07/06 17:29:07, pdr wrote:
> On 2015/07/03 at 21:08:15, pdr wrote:
> > On 2015/07/03 at 20:03:21, wangxianzhu wrote:
> > > lgtm.
> > > 
> > > On 2015/07/03 02:38:05, pdr wrote:
> > > > Thanks for the review.
> > > > 
> > > > How do you feel about the DrawingRecorderCacheParams idea? It's a little
> more
> > > > verbose but it will force us to keep the phase in sync between the
> > > > useCachedDrawingIfPossible call and the recorder.
> > > > 
> > > > This patch:
> > > > if (LayoutObjectDrawingRecorder::useCachedDrawingIfPossible(context,
> client,
> > > > PaintPhase::SomePaintPhase)
> > > >     return;
> > > > LayoutObjectDrawingRecorder recorder(context, client,
> > > > PaintPhase::SomePaintPhase, ...);
> > > > 
> > > > 
> > > > Proposal:
> > > > DrawingRecorder::CacheParameters recorderCacheParameters(context,
client,
> > > > PaintPhase::SomePhase);
> > > > if
> > > >
>
(LayoutObjectDrawingRecorder::useCachedDrawingIfPossible(recorderCacheParameters))
> > > >     return;
> > > > LayoutObjectDrawingRecorder recorder(recorderCacheParameters, ...);
> > > > 
> > > 
> > > I still like the less-verbose version. The ASSERT in DrawingRecorder
should
> catch the inconsistency.
> > 
> > SG
> > 
> > Patchset 2 actually showed a regression on cluster telemetry:
> >
>
https://storage.cloud.google.com/cluster-telemetry/tasks/chromium_perf_runs/p...
> > 
> > Locally, with patchset 3, everything is a progression. I also confirmed the
> profile with and without patch makes sense. I'm going to wait for a cluster
> telemetry run of patchset 3 before landing.
> 
> This patch progresses record time by 3% but regresses record time with caching
> ,painting, or construction disabled.
>
https://storage.cloud.google.com/cluster-telemetry/tasks/chromium_perf_runs/p...
> 
> I think we should go ahead with this patch because record time is what really
> matters, but I'd like a second opinion. This patch will also open up some
> additional optimizations. WDYT?

I think record_time_caching_disabled also matters because it reflects the
performance of the first paint. However, the regression is tiny and it sgtm to
ignore it and go ahead with the patch.

[pdr., 2015-07-06 17:56:03]

The CQ bit was checked by pdr@chromium.org

[pdr., 2015-07-06 17:56:04]

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

[pdr., 2015-07-06 17:56:19]

On 2015/07/06 at 17:54:17, wangxianzhu wrote:
> On 2015/07/06 17:29:07, pdr wrote:
> > On 2015/07/03 at 21:08:15, pdr wrote:
> > > On 2015/07/03 at 20:03:21, wangxianzhu wrote:
> > > > lgtm.
> > > > 
> > > > On 2015/07/03 02:38:05, pdr wrote:
> > > > > Thanks for the review.
> > > > > 
> > > > > How do you feel about the DrawingRecorderCacheParams idea? It's a
little
> > more
> > > > > verbose but it will force us to keep the phase in sync between the
> > > > > useCachedDrawingIfPossible call and the recorder.
> > > > > 
> > > > > This patch:
> > > > > if (LayoutObjectDrawingRecorder::useCachedDrawingIfPossible(context,
> > client,
> > > > > PaintPhase::SomePaintPhase)
> > > > >     return;
> > > > > LayoutObjectDrawingRecorder recorder(context, client,
> > > > > PaintPhase::SomePaintPhase, ...);
> > > > > 
> > > > > 
> > > > > Proposal:
> > > > > DrawingRecorder::CacheParameters recorderCacheParameters(context,
client,
> > > > > PaintPhase::SomePhase);
> > > > > if
> > > > >
> >
(LayoutObjectDrawingRecorder::useCachedDrawingIfPossible(recorderCacheParameters))
> > > > >     return;
> > > > > LayoutObjectDrawingRecorder recorder(recorderCacheParameters, ...);
> > > > > 
> > > > 
> > > > I still like the less-verbose version. The ASSERT in DrawingRecorder
should
> > catch the inconsistency.
> > > 
> > > SG
> > > 
> > > Patchset 2 actually showed a regression on cluster telemetry:
> > >
> >
https://storage.cloud.google.com/cluster-telemetry/tasks/chromium_perf_runs/p...
> > > 
> > > Locally, with patchset 3, everything is a progression. I also confirmed
the
> > profile with and without patch makes sense. I'm going to wait for a cluster
> > telemetry run of patchset 3 before landing.
> > 
> > This patch progresses record time by 3% but regresses record time with
caching
> > ,painting, or construction disabled.
> >
https://storage.cloud.google.com/cluster-telemetry/tasks/chromium_perf_runs/p...
> > 
> > I think we should go ahead with this patch because record time is what
really
> > matters, but I'd like a second opinion. This patch will also open up some
> > additional optimizations. WDYT?
> 
> I think record_time_caching_disabled also matters because it reflects the
performance of the first paint. However, the regression is tiny and it sgtm to
ignore it and go ahead with the patch.

Thanks! Off to the CQ

[commit-bot: I haz the power, 2015-07-06 17:56:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1220583004/60001

[commit-bot: I haz the power, 2015-07-06 19:13:22]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=198330