Move DisplayItemList complex inline method implementations to .cpp.

Move DisplayItemList complex inline method implementations to .cpp.

Committed: https://crrev.com/811e81b3595e049ecaeb4b6b3a65f2275b952c1c
Cr-Commit-Position: refs/heads/master@{#384469}

[wkorman, 2016-03-18 00:39:24]

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

[wkorman, 2016-03-18 00:39:25]

For discussion -- I found working in DIL and reading it hard due to the
non-trivial logic in the header file. This would be cleaner IMO.

Is having them in the header for (presumably) inlining purposes actually
important? We could try it and see?

[pdr., 2016-03-18 01:19:32]

On 2016/03/18 at 00:39:25, wkorman wrote:
> For discussion -- I found working in DIL and reading it hard due to the
non-trivial logic in the header file. This would be cleaner IMO.
> 
> Is having them in the header for (presumably) inlining purposes actually
important? We could try it and see?

Can you kick off a rasterize and record tryjob no cluster telemetry with this
patch (https://ct.skia.org)? I agree we should do this if it doesn't regress
perf. I am almost positive it will, but definitely worth a try.

[pdr., 2016-03-18 01:19:40]

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

[jbroman, 2016-03-18 01:37:34]

assertDisplayItemClientsAreAlive can almost certainly move, because we don't
even ship it (who cares if we regress debug performance slightly?).

The other one...was ~1 line when originally written, but has since acquired
debug hooks. I suspect it's probably fine to outline it anyhow (but a perf check
would be a reasonable idea, given how hot this method potentially is), but it's
a little unfortunate that the debug checks make it so much longer. Makes me
wonder if it could be refactored slightly to solve this.

[wkorman, 2016-03-18 21:13:13]

On 2016/03/18 at 01:19:32, pdr wrote:
> On 2016/03/18 at 00:39:25, wkorman wrote:
> > For discussion -- I found working in DIL and reading it hard due to the
non-trivial logic in the header file. This would be cleaner IMO.
> > 
> > Is having them in the header for (presumably) inlining purposes actually
important? We could try it and see?
> 
> Can you kick off a rasterize and record tryjob no cluster telemetry with this
patch (https://ct.skia.org)? I agree we should do this if it doesn't regress
perf. I am almost positive it will, but definitely worth a try.

ct.skia.org seems to have a bug currently. When I click 'Queue Task' it pops up
a 'Proceed with queueing task?' dialog. When I click OK, it shows an empty black
toast-type popup at bottom left, the 'Proceed...?' dialog never closes, and
eventually the empty black popup disappears. The task is not queued. Test in
both Chrome and FF. I see a server error 500 in dev tools network response per
below. Should I file a bug on someone for Cluster Telemetry?

Request URL:https://ct.skia.org/_/add_chromium_perf_task
Request Method:POST
Status Code:500 Internal Server Error
Remote Address:104.154.112.102:443
Response Headers
view source
Connection:keep-alive
Content-Encoding:gzip
Content-Length:25
Content-Type:text/plain; charset=utf-8
Date:Fri, 18 Mar 2016 21:12:14 GMT
Server:nginx

[pdr., 2016-03-18 21:30:32]

On 2016/03/18 at 21:13:13, wkorman wrote:
> On 2016/03/18 at 01:19:32, pdr wrote:
> > On 2016/03/18 at 00:39:25, wkorman wrote:
> > > For discussion -- I found working in DIL and reading it hard due to the
non-trivial logic in the header file. This would be cleaner IMO.
> > > 
> > > Is having them in the header for (presumably) inlining purposes actually
important? We could try it and see?
> > 
> > Can you kick off a rasterize and record tryjob no cluster telemetry with
this patch (https://ct.skia.org)? I agree we should do this if it doesn't
regress perf. I am almost positive it will, but definitely worth a try.
> 
> ct.skia.org seems to have a bug currently. When I click 'Queue Task' it pops
up a 'Proceed with queueing task?' dialog. When I click OK, it shows an empty
black toast-type popup at bottom left, the 'Proceed...?' dialog never closes,
and eventually the empty black popup disappears. The task is not queued. Test in
both Chrome and FF. I see a server error 500 in dev tools network response per
below. Should I file a bug on someone for Cluster Telemetry?
> 
> Request URL:https://ct.skia.org/_/add_chromium_perf_task
> Request Method:POST
> Status Code:500 Internal Server Error
> Remote Address:104.154.112.102:443
> Response Headers
> view source
> Connection:keep-alive
> Content-Encoding:gzip
> Content-Length:25
> Content-Type:text/plain; charset=utf-8
> Date:Fri, 18 Mar 2016 21:12:14 GMT
> Server:nginx

I just hit this too. I filed
https://bugs.chromium.org/p/chromium/issues/detail?id=596183

[wkorman, 2016-03-31 04:20:00]

At long last! CT run here:

https://storage.cloud.google.com/cluster-telemetry/tasks/chromium_perf_runs/w...

It's all green. Opinions? Re-do on Android? It's an interesting experiment.

[pdr., 2016-03-31 04:24:56]

On 2016/03/31 at 04:20:00, wkorman wrote:
> At long last! CT run here:
> 
>
https://storage.cloud.google.com/cluster-telemetry/tasks/chromium_perf_runs/w...
> 
> It's all green. Opinions? Re-do on Android? It's an interesting experiment.

A perf improvement!?

We should 100% land this if that's the case. I don't understand how this could
happen though. I don't want to delay this awesome but could you do one more run
on both linux and android to double-check that this wasn't a fluke?

[wkorman, 2016-03-31 18:48:39]

On 2016/03/31 at 04:24:56, pdr wrote:
> On 2016/03/31 at 04:20:00, wkorman wrote:
> > At long last! CT run here:
> > 
> >
https://storage.cloud.google.com/cluster-telemetry/tasks/chromium_perf_runs/w...
> > 
> > It's all green. Opinions? Re-do on Android? It's an interesting experiment.
> 
> A perf improvement!?
> 
> We should 100% land this if that's the case. I don't understand how this could
happen though. I don't want to delay this awesome but could you do one more run
on both linux and android to double-check that this wasn't a fluke?

Re-run of CT:

Android:
https://storage.cloud.google.com/cluster-telemetry/tasks/chromium_perf_runs/w...
Linux:
https://storage.cloud.google.com/cluster-telemetry/tasks/chromium_perf_runs/w...

Linux: Previously showed a -0.327% improvement in record_time, now shows
+0.111%.
Android: Only run we've got, shows +0.168% in record_time.

If +/-0.1xx% record_time is within typical volatility range, which currently it
seems like it is, then I think this change is fine. I'll do another Android run,
thoughts welcome.

[pdr., 2016-03-31 18:50:15]

On 2016/03/31 at 18:48:39, wkorman wrote:
> On 2016/03/31 at 04:24:56, pdr wrote:
> > On 2016/03/31 at 04:20:00, wkorman wrote:
> > > At long last! CT run here:
> > > 
> > >
https://storage.cloud.google.com/cluster-telemetry/tasks/chromium_perf_runs/w...
> > > 
> > > It's all green. Opinions? Re-do on Android? It's an interesting
experiment.
> > 
> > A perf improvement!?
> > 
> > We should 100% land this if that's the case. I don't understand how this
could happen though. I don't want to delay this awesome but could you do one
more run on both linux and android to double-check that this wasn't a fluke?
> 
> Re-run of CT:
> 
> Android:
https://storage.cloud.google.com/cluster-telemetry/tasks/chromium_perf_runs/w...
> Linux:
https://storage.cloud.google.com/cluster-telemetry/tasks/chromium_perf_runs/w...
> 
> Linux: Previously showed a -0.327% improvement in record_time, now shows
+0.111%.
> Android: Only run we've got, shows +0.168% in record_time.
> 
> If +/-0.1xx% record_time is within typical volatility range, which currently
it seems like it is, then I think this change is fine. I'll do another Android
run, thoughts welcome.

That looks more like I'd expect. LGTM, likely an incredibly minor regression and
a not-minor progression in code cleanliness.

[wkorman, 2016-03-31 18:53:21]

On 2016/03/31 at 18:50:15, pdr wrote:
> On 2016/03/31 at 18:48:39, wkorman wrote:
> > On 2016/03/31 at 04:24:56, pdr wrote:
> > > On 2016/03/31 at 04:20:00, wkorman wrote:
> > > > At long last! CT run here:
> > > > 
> > > >
https://storage.cloud.google.com/cluster-telemetry/tasks/chromium_perf_runs/w...
> > > > 
> > > > It's all green. Opinions? Re-do on Android? It's an interesting
experiment.
> > > 
> > > A perf improvement!?
> > > 
> > > We should 100% land this if that's the case. I don't understand how this
could happen though. I don't want to delay this awesome but could you do one
more run on both linux and android to double-check that this wasn't a fluke?
> > 
> > Re-run of CT:
> > 
> > Android:
https://storage.cloud.google.com/cluster-telemetry/tasks/chromium_perf_runs/w...
> > Linux:
https://storage.cloud.google.com/cluster-telemetry/tasks/chromium_perf_runs/w...
> > 
> > Linux: Previously showed a -0.327% improvement in record_time, now shows
+0.111%.
> > Android: Only run we've got, shows +0.168% in record_time.
> > 
> > If +/-0.1xx% record_time is within typical volatility range, which currently
it seems like it is, then I think this change is fine. I'll do another Android
run, thoughts welcome.
> 
> That looks more like I'd expect. LGTM, likely an incredibly minor regression
and a not-minor progression in code cleanliness.

Sweet. It's really nice to have the bulk of code in .cpp when actively working
on it as compile times are much faster.

[wkorman, 2016-03-31 23:21:24]

The CQ bit was checked by wkorman@chromium.org

[wkorman, 2016-03-31 23:21:25]

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

[commit-bot: I haz the power, 2016-03-31 23:21:55]

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

[commit-bot: I haz the power, 2016-04-01 00:41:33]

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

[commit-bot: I haz the power, 2016-04-01 00:41:34]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[wkorman, 2016-04-01 00:56:54]

The CQ bit was checked by wkorman@chromium.org

[commit-bot: I haz the power, 2016-04-01 00:57:18]

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

[commit-bot: I haz the power, 2016-04-01 01:44:26]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-04-01 01:46:05]

Description was changed from

==========
Move DisplayItemList complex inline method implementations to .cpp.
==========

to

==========
Move DisplayItemList complex inline method implementations to .cpp.

Committed: https://crrev.com/811e81b3595e049ecaeb4b6b3a65f2275b952c1c
Cr-Commit-Position: refs/heads/master@{#384469}
==========

[commit-bot: I haz the power, 2016-04-01 01:46:06]

Patchset 4 (id:??) landed as
https://crrev.com/811e81b3595e049ecaeb4b6b3a65f2275b952c1c
Cr-Commit-Position: refs/heads/master@{#384469}