cc : Add UMA metric for finding closest matching layer to a point

cc : Add UMA metric for finding closest matching layer to a point

The logic to find the closest matching layer is going to change. Adding
a UMA metric to track performance before and after the change.

BUG=685379
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2655853004
Cr-Commit-Position: refs/heads/master@{#447117}
Committed: https://chromium.googlesource.com/chromium/src/+/ee0ab199b6e9bd10d179fa1cfb379eff943d8b9e

[jaydasika, 2017-01-25 22:17:22]

Description was changed from

==========
cc : Add UMA metric for finding closest matching layer to a point

The logic to find the closest matching layer is going to change. Adding
a UMA metric to track performance before and after the change.

BUG=685379
==========

to

==========
cc : Add UMA metric for finding closest matching layer to a point

The logic to find the closest matching layer is going to change. Adding
a UMA metric to track performance before and after the change.

BUG=685379
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[jaydasika, 2017-01-25 22:18:36]

jaydasika@chromium.org changed reviewers:
+ ajuma@chromium.org, dtapuska@chromium.org

[jaydasika, 2017-01-25 22:18:37]

PTAL

[jaydasika, 2017-01-25 22:18:50]

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

[commit-bot: I haz the power, 2017-01-25 22:19:35]

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

[ajuma, 2017-01-25 22:32:59]

https://codereview.chromium.org/2655853004/diff/1/cc/trees/layer_tree_impl.cc
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/2655853004/diff/1/cc/trees/layer_tree_impl.cc...
cc/trees/layer_tree_impl.cc:1914: if (const char* client_name =
GetClientNameForMetrics()) {
I don't think we do any of this hit testing in the browser process (since, e.g.,
we don't do any scrolling there), and in that case we don't need separate
renderer/browser histograms.

[jaydasika, 2017-01-25 22:52:55]

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

[commit-bot: I haz the power, 2017-01-25 22:53:32]

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

[jaydasika, 2017-01-25 22:53:47]

https://codereview.chromium.org/2655853004/diff/1/cc/trees/layer_tree_impl.cc
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/2655853004/diff/1/cc/trees/layer_tree_impl.cc...
cc/trees/layer_tree_impl.cc:1914: if (const char* client_name =
GetClientNameForMetrics()) {
On 2017/01/25 22:32:59, ajuma wrote:
> I don't think we do any of this hit testing in the browser process (since,
e.g.,
> we don't do any scrolling there), and in that case we don't need separate
> renderer/browser histograms.

Done.

[ajuma, 2017-01-25 22:55:41]

Thanks, lgtm

[dtapuska, 2017-01-25 23:16:21]

On 2017/01/25 22:55:41, ajuma wrote:
> Thanks, lgtm

Why the sprintf and the fetch of the client name it isn't used. Likewise using a
non construction with the uma histogram macro is dangerous I believe. So the
code you copied this from is actually wrong.

[jaydasika, 2017-01-25 23:34:56]

On 2017/01/25 23:16:21, dtapuska wrote:
> On 2017/01/25 22:55:41, ajuma wrote:
> > Thanks, lgtm
> 
> Why the sprintf and the fetch of the client name it isn't used. Likewise using
a
> non construction with the uma histogram macro is dangerous I believe. So the
> code you copied this from is actually wrong.

Removed sprintf and fetch of client name. What do you mean by non construction
with the uma histogram macro ?

[dtapuska, 2017-01-25 23:51:43]

On 2017/01/25 23:34:56, jaydasika wrote:
> On 2017/01/25 23:16:21, dtapuska wrote:
> > On 2017/01/25 22:55:41, ajuma wrote:
> > > Thanks, lgtm
> > 
> > Why the sprintf and the fetch of the client name it isn't used. Likewise
using
> a
> > non construction with the uma histogram macro is dangerous I believe. So the
> > code you copied this from is actually wrong.
> 
> Removed sprintf and fetch of client name. What do you mean by non construction
> with the uma histogram macro ?

That was supposed to be a non const expression string.

[jaydasika, 2017-01-26 00:09:22]

On 2017/01/25 23:51:43, dtapuska wrote:
> On 2017/01/25 23:34:56, jaydasika wrote:
> > On 2017/01/25 23:16:21, dtapuska wrote:
> > > On 2017/01/25 22:55:41, ajuma wrote:
> > > > Thanks, lgtm
> > > 
> > > Why the sprintf and the fetch of the client name it isn't used. Likewise
> using
> > a
> > > non construction with the uma histogram macro is dangerous I believe. So
the
> > > code you copied this from is actually wrong.
> > 
> > Removed sprintf and fetch of client name. What do you mean by non
construction
> > with the uma histogram macro ?
> 
> That was supposed to be a non const expression string.

You mean for the name parameter of the UMA_HISTOGRAM_COUNT macro? Code search
for UMA_HISTOGRAM_COUNT outputs a lot of usages with name as a (non-constructed)
const string.
https://cs.chromium.org/chromium/src/base/metrics/histogram_macros.h?l=95 says
|name| should be a runtime constant.

[jaydasika, 2017-01-27 18:09:17]

On 2017/01/26 00:09:22, jaydasika wrote:
> On 2017/01/25 23:51:43, dtapuska wrote:
> > On 2017/01/25 23:34:56, jaydasika wrote:
> > > On 2017/01/25 23:16:21, dtapuska wrote:
> > > > On 2017/01/25 22:55:41, ajuma wrote:
> > > > > Thanks, lgtm
> > > > 
> > > > Why the sprintf and the fetch of the client name it isn't used. Likewise
> > using
> > > a
> > > > non construction with the uma histogram macro is dangerous I believe. So
> the
> > > > code you copied this from is actually wrong.
> > > 
> > > Removed sprintf and fetch of client name. What do you mean by non
> construction
> > > with the uma histogram macro ?
> > 
> > That was supposed to be a non const expression string.
> 
> You mean for the name parameter of the UMA_HISTOGRAM_COUNT macro? Code search
> for UMA_HISTOGRAM_COUNT outputs a lot of usages with name as a
(non-constructed)
> const string.
> https://cs.chromium.org/chromium/src/base/metrics/histogram_macros.h?l=95 says
> |name| should be a runtime constant.

Ping.

[dtapuska, 2017-01-30 20:56:27]

dtapuska@chromium.org changed reviewers:
+ isherman@chromium.org

[dtapuska, 2017-01-30 20:57:06]

On 2017/01/30 20:56:28, dtapuska wrote:

I was hoping that a histogram OWNER would chome in on this but I guess you
didn't have one added. I've added Ilya to comment.

[Ilya Sherman, 2017-01-30 21:33:55]

Histograms LGTM

On 2017/01/26 00:09:22, jaydasika wrote:
> On 2017/01/25 23:51:43, dtapuska wrote:
> > On 2017/01/25 23:34:56, jaydasika wrote:
> > > On 2017/01/25 23:16:21, dtapuska wrote:
> > > > On 2017/01/25 22:55:41, ajuma wrote:
> > > > > Thanks, lgtm
> > > > 
> > > > Why the sprintf and the fetch of the client name it isn't used. Likewise
> > using
> > > a
> > > > non construction with the uma histogram macro is dangerous I believe. So
> the
> > > > code you copied this from is actually wrong.
> > > 
> > > Removed sprintf and fetch of client name. What do you mean by non
> construction
> > > with the uma histogram macro ?
> > 
> > That was supposed to be a non const expression string.
> 
> You mean for the name parameter of the UMA_HISTOGRAM_COUNT macro? Code search
> for UMA_HISTOGRAM_COUNT outputs a lot of usages with name as a
(non-constructed)
> const string.
> https://cs.chromium.org/chromium/src/base/metrics/histogram_macros.h?l=95 says
> |name| should be a runtime constant.

Correct, the histogram name needs to be a runtime constant within the process
that it's running in.  So, different names for browser vs. renderer is generally
ok... though, I just recalled that WebView runs in single process mode, IIRC. 
So it's possible that for WebView-reported histograms, these names are not
runtime constants, and therefore are sending polluted data...  (Not a concern
for this CL, which now uses a compile-time constant name.  But it might be a
concern for other histograms that use the sprintf pattern.)

[dtapuska, 2017-01-30 21:35:14]

On 2017/01/30 21:33:55, Ilya Sherman wrote:
> Histograms LGTM
> 
> On 2017/01/26 00:09:22, jaydasika wrote:
> > On 2017/01/25 23:51:43, dtapuska wrote:
> > > On 2017/01/25 23:34:56, jaydasika wrote:
> > > > On 2017/01/25 23:16:21, dtapuska wrote:
> > > > > On 2017/01/25 22:55:41, ajuma wrote:
> > > > > > Thanks, lgtm
> > > > > 
> > > > > Why the sprintf and the fetch of the client name it isn't used.
Likewise
> > > using
> > > > a
> > > > > non construction with the uma histogram macro is dangerous I believe.
So
> > the
> > > > > code you copied this from is actually wrong.
> > > > 
> > > > Removed sprintf and fetch of client name. What do you mean by non
> > construction
> > > > with the uma histogram macro ?
> > > 
> > > That was supposed to be a non const expression string.
> > 
> > You mean for the name parameter of the UMA_HISTOGRAM_COUNT macro? Code
search
> > for UMA_HISTOGRAM_COUNT outputs a lot of usages with name as a
> (non-constructed)
> > const string.
> > https://cs.chromium.org/chromium/src/base/metrics/histogram_macros.h?l=95
says
> > |name| should be a runtime constant.
> 
> Correct, the histogram name needs to be a runtime constant within the process
> that it's running in.  So, different names for browser vs. renderer is
generally
> ok... though, I just recalled that WebView runs in single process mode, IIRC. 
> So it's possible that for WebView-reported histograms, these names are not
> runtime constants, and therefore are sending polluted data...  (Not a concern
> for this CL, which now uses a compile-time constant name.  But it might be a
> concern for other histograms that use the sprintf pattern.)

lgtm for this CL

[jaydasika, 2017-01-30 22:04:14]

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

[commit-bot: I haz the power, 2017-01-30 22:05:20]

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

[commit-bot: I haz the power, 2017-01-30 23:46:07]

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

[commit-bot: I haz the power, 2017-01-30 23:46:08]

Dry run: This issue passed the CQ dry run.

[jaydasika, 2017-01-30 23:56:45]

The CQ bit was checked by jaydasika@chromium.org

[jaydasika, 2017-01-30 23:56:45]

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

[commit-bot: I haz the power, 2017-01-30 23:57:31]

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

[commit-bot: I haz the power, 2017-01-31 00:03:52]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1485820605436730,
"parent_rev": "4c36e3f30274cefde2d324e3eed83848df03c8a4", "commit_rev":
"ee0ab199b6e9bd10d179fa1cfb379eff943d8b9e"}

[commit-bot: I haz the power, 2017-01-31 00:04:30]

Description was changed from

==========
cc : Add UMA metric for finding closest matching layer to a point

The logic to find the closest matching layer is going to change. Adding
a UMA metric to track performance before and after the change.

BUG=685379
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
cc : Add UMA metric for finding closest matching layer to a point

The logic to find the closest matching layer is going to change. Adding
a UMA metric to track performance before and after the change.

BUG=685379
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2655853004
Cr-Commit-Position: refs/heads/master@{#447117}
Committed:
https://chromium.googlesource.com/chromium/src/+/ee0ab199b6e9bd10d179fa1cfb37...
==========

[commit-bot: I haz the power, 2017-01-31 00:04:32]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/ee0ab199b6e9bd10d179fa1cfb37...