Keep a cache of caret painters to avoid recomputing all the time.

Keep a cache of caret painters to avoid recomputing all the time.

PaintDataCache lives throughout the lifetime of a high-level paint call, and
caches data which isn't safe to cache long term, but doesn't change during
the paint lifecycle phase.

[jbroman, 2015-07-03 13:24:45]

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

[jbroman, 2015-07-03 13:24:45]

I'm kinda loath to add yet another kind of caching, but on the other hand the
expense of finding the caret painters has bothered me for awhile.

Cluster Telemetry results for Patch Set 1 show a 3% record time win:
https://storage.cloud.google.com/cluster-telemetry/tasks/chromium_perf_runs/j...

I don't have perf numbers for Patch Set 2 yet, but it's an example of one colour
for the deduplication bikeshed after Patch Set 1. I'm open to other shapes.

(N.B. You don't have to review this on a holiday.)

[pdr., 2015-07-03 20:07:10]

On 2015/07/03 at 13:24:45, jbroman wrote:
> I'm kinda loath to add yet another kind of caching, but on the other hand the
expense of finding the caret painters has bothered me for awhile.
> 
> Cluster Telemetry results for Patch Set 1 show a 3% record time win:
https://storage.cloud.google.com/cluster-telemetry/tasks/chromium_perf_runs/j...
> 
> I don't have perf numbers for Patch Set 2 yet, but it's an example of one
colour for the deduplication bikeshed after Patch Set 1. I'm open to other
shapes.
> 
> (N.B. You don't have to review this on a holiday.)

It's crazy that hasCaret can be so high on profiles so I think you're looking in
the right area.

Is a new cache necessary though? CaretBase has a cache of the rect
(m_caretLocalRect) and there are hooks to invalidate this. Can we just cache
m_paintingBlock similarly, in CaretBase (or DragController)?

[jbroman, 2015-07-06 15:03:40]

On 2015/07/03 20:07:10, pdr wrote:
> On 2015/07/03 at 13:24:45, jbroman wrote:
> > I'm kinda loath to add yet another kind of caching, but on the other hand
the
> expense of finding the caret painters has bothered me for awhile.
> > 
> > Cluster Telemetry results for Patch Set 1 show a 3% record time win:
>
https://storage.cloud.google.com/cluster-telemetry/tasks/chromium_perf_runs/j...
> > 
> > I don't have perf numbers for Patch Set 2 yet, but it's an example of one
> colour for the deduplication bikeshed after Patch Set 1. I'm open to other
> shapes.
> > 
> > (N.B. You don't have to review this on a holiday.)
> 
> It's crazy that hasCaret can be so high on profiles so I think you're looking
in
> the right area.
> 
> Is a new cache necessary though? CaretBase has a cache of the rect
> (m_caretLocalRect) and there are hooks to invalidate this. Can we just cache
> m_paintingBlock similarly, in CaretBase (or DragController)?

I'll take another look (when I'd tried this approach awhile ago I didn't
succeed, but it's worth another look). The profiling suggested a bunch of the
work is just following the series of pointers to reach the CaretBase instances
(the first half or so is hot anyways because we access lots of other document
settings).

m_layoutBlock --> m_node --> m_treeScope --> m_document --> m_frame -->
m_selection
m_layoutBlock --> m_node --> m_treeScope --> m_document --> m_frame --> m_host
--> m_page --> m_dragCaretController

Unrelated, but for the record, patch set 2
(https://storage.cloud.google.com/cluster-telemetry/tasks/chromium_perf_runs/j...)
underperforms patch set 1, so my deduplication cost more than I'd hoped.

[jbroman, 2015-07-07 15:22:04]

On 2015/07/06 15:03:40, jbroman wrote:
> On 2015/07/03 20:07:10, pdr wrote:
> > On 2015/07/03 at 13:24:45, jbroman wrote:
> > > I'm kinda loath to add yet another kind of caching, but on the other hand
> the
> > expense of finding the caret painters has bothered me for awhile.
> > > 
> > > Cluster Telemetry results for Patch Set 1 show a 3% record time win:
> >
>
https://storage.cloud.google.com/cluster-telemetry/tasks/chromium_perf_runs/j...
> > > 
> > > I don't have perf numbers for Patch Set 2 yet, but it's an example of one
> > colour for the deduplication bikeshed after Patch Set 1. I'm open to other
> > shapes.
> > > 
> > > (N.B. You don't have to review this on a holiday.)
> > 
> > It's crazy that hasCaret can be so high on profiles so I think you're
looking
> in
> > the right area.
> > 
> > Is a new cache necessary though? CaretBase has a cache of the rect
> > (m_caretLocalRect) and there are hooks to invalidate this. Can we just cache
> > m_paintingBlock similarly, in CaretBase (or DragController)?
> 
> I'll take another look (when I'd tried this approach awhile ago I didn't
> succeed, but it's worth another look). The profiling suggested a bunch of the
> work is just following the series of pointers to reach the CaretBase instances
> (the first half or so is hot anyways because we access lots of other document
> settings).

Okay, done as a separate CL.

This gets most (but not quite all, presumably due to the indirection) of the
win.
But the reduced complexity probably makes it worthwhile.

https://codereview.chromium.org/1226833007/
https://storage.cloud.google.com/cluster-telemetry/tasks/chromium_perf_runs/j...

[jbroman, 2015-07-07 20:57:39]

https://codereview.chromium.org/1226833007/ has LGTM; closing this CL.