Adds some histograms from various omnibox queries. I'm doing this as

Adds some histograms from various omnibox queries. I'm doing this as
with the quick provider enabled the omnibox feels a bit sluggish and I
would like to quantify that. I'll update the xml file separately.

BUG=none
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=78263

[jar (doing other things), 2011-03-14 20:30:20]

I only read how you used the data in the histograms, so you need someone else to
review the acquisition of the data.

That use LGTM... with the concern below.  Your call.

http://codereview.chromium.org/6679049/diff/1/chrome/browser/autocomplete/his...
File chrome/browser/autocomplete/history_quick_provider.cc (right):

http://codereview.chromium.org/6679049/diff/1/chrome/browser/autocomplete/his...
chrome/browser/autocomplete/history_quick_provider.cc:74:
scoped_refptr<base::Histogram> counter = base::Histogram::FactoryGet(
nit/concern: This is all correct... and even makes the code thread safe.... but
you are incurring a perf cost.  The macro used a static to avoid that problem
(at the cost of a static... which is a problem in multi-threaded code!).  If you
are concerned about performance, you should have a switch in here for each of
the 6 cases.

The recurring perf involved is:
a) acquisition of a central lock.
b) find operation in a map.
c) mess with a smart pointer on acquisition and release.

The lock is probably the big cost, but the find operation may be working on a
map having about 400+ histogram names.

I haven't tried to quantify this cost.... if this is temporary... then don't
sweat it.  If this is long term (destined for stable??) then you might want to
<smile> histogram how long it takes to do this stuff, and be sure you're not
hurting too much (use the macro form to do that uber histogram).

[sky, 2011-03-14 20:35:09]

It was my understanding I should only use the macro if the name won't
change. In the cases I'm adding the metric the name varies (last
character is based on input).

  -Scott

On Mon, Mar 14, 2011 at 1:30 PM,  <jar@chromium.org> wrote:
> I only read how you used the data in the histograms, so you need someone
> else to
> review the acquisition of the data.
>
> That use LGTM... with the concern below.  Your call.
>
>
>
http://codereview.chromium.org/6679049/diff/1/chrome/browser/autocomplete/his...
> File chrome/browser/autocomplete/history_quick_provider.cc (right):
>
>
http://codereview.chromium.org/6679049/diff/1/chrome/browser/autocomplete/his...
> chrome/browser/autocomplete/history_quick_provider.cc:74:
> scoped_refptr<base::Histogram> counter = base::Histogram::FactoryGet(
> nit/concern: This is all correct... and even makes the code thread
> safe.... but you are incurring a perf cost.  The macro used a static to
> avoid that problem (at the cost of a static... which is a problem in
> multi-threaded code!).  If you are concerned about performance, you
> should have a switch in here for each of the 6 cases.
>
> The recurring perf involved is:
> a) acquisition of a central lock.
> b) find operation in a map.
> c) mess with a smart pointer on acquisition and release.
>
> The lock is probably the big cost, but the find operation may be working
> on a map having about 400+ histogram names.
>
> I haven't tried to quantify this cost.... if this is temporary... then
> don't sweat it.  If this is long term (destined for stable??) then you
> might want to <smile> histogram how long it takes to do this stuff, and
> be sure you're not hurting too much (use the macro form to do that uber
> histogram).
>
> http://codereview.chromium.org/6679049/
>

[jar (doing other things), 2011-03-14 21:01:25]

Right.  You can't use the macro if the name changes.  What I was trying to
say was that if you wanted (needed?) the performance, you'd have to have 6
copies of teh macro, each with a different name, in a switch statement.

Jim

On Mon, Mar 14, 2011 at 1:35 PM, Scott Violet <sky@chromium.org> wrote:

> It was my understanding I should only use the macro if the name won't
> change. In the cases I'm adding the metric the name varies (last
> character is based on input).
>
>  -Scott
>
> On Mon, Mar 14, 2011 at 1:30 PM,  <jar@chromium.org> wrote:
> > I only read how you used the data in the histograms, so you need someone
> > else to
> > review the acquisition of the data.
> >
> > That use LGTM... with the concern below.  Your call.
> >
> >
> >
>
http://codereview.chromium.org/6679049/diff/1/chrome/browser/autocomplete/his...
> > File chrome/browser/autocomplete/history_quick_provider.cc (right):
> >
> >
>
http://codereview.chromium.org/6679049/diff/1/chrome/browser/autocomplete/his...
> > chrome/browser/autocomplete/history_quick_provider.cc:74:
> > scoped_refptr<base::Histogram> counter = base::Histogram::FactoryGet(
> > nit/concern: This is all correct... and even makes the code thread
> > safe.... but you are incurring a perf cost.  The macro used a static to
> > avoid that problem (at the cost of a static... which is a problem in
> > multi-threaded code!).  If you are concerned about performance, you
> > should have a switch in here for each of the 6 cases.
> >
> > The recurring perf involved is:
> > a) acquisition of a central lock.
> > b) find operation in a map.
> > c) mess with a smart pointer on acquisition and release.
> >
> > The lock is probably the big cost, but the find operation may be working
> > on a map having about 400+ histogram names.
> >
> > I haven't tried to quantify this cost.... if this is temporary... then
> > don't sweat it.  If this is long term (destined for stable??) then you
> > might want to <smile> histogram how long it takes to do this stuff, and
> > be sure you're not hurting too much (use the macro form to do that uber
> > histogram).
> >
> > http://codereview.chromium.org/6679049/
> >
>

[sky, 2011-03-14 22:35:57]

This is temporary. Also, while this code is hit on every key stroke in
the omnibox, I can't imagine the cost you mention is going to be
noticeable.

  -Scott

On Mon, Mar 14, 2011 at 2:01 PM, Jim Roskind <jar@chromium.org> wrote:
> Right.  You can't use the macro if the name changes.  What I was trying to
> say was that if you wanted (needed?) the performance, you'd have to have 6
> copies of teh macro, each with a different name, in a switch statement.
> Jim
>
> On Mon, Mar 14, 2011 at 1:35 PM, Scott Violet <sky@chromium.org> wrote:
>>
>> It was my understanding I should only use the macro if the name won't
>> change. In the cases I'm adding the metric the name varies (last
>> character is based on input).
>>
>>  -Scott
>>
>> On Mon, Mar 14, 2011 at 1:30 PM,  <jar@chromium.org> wrote:
>> > I only read how you used the data in the histograms, so you need someone
>> > else to
>> > review the acquisition of the data.
>> >
>> > That use LGTM... with the concern below.  Your call.
>> >
>> >
>> >
>> >
http://codereview.chromium.org/6679049/diff/1/chrome/browser/autocomplete/his...
>> > File chrome/browser/autocomplete/history_quick_provider.cc (right):
>> >
>> >
>> >
http://codereview.chromium.org/6679049/diff/1/chrome/browser/autocomplete/his...
>> > chrome/browser/autocomplete/history_quick_provider.cc:74:
>> > scoped_refptr<base::Histogram> counter = base::Histogram::FactoryGet(
>> > nit/concern: This is all correct... and even makes the code thread
>> > safe.... but you are incurring a perf cost.  The macro used a static to
>> > avoid that problem (at the cost of a static... which is a problem in
>> > multi-threaded code!).  If you are concerned about performance, you
>> > should have a switch in here for each of the 6 cases.
>> >
>> > The recurring perf involved is:
>> > a) acquisition of a central lock.
>> > b) find operation in a map.
>> > c) mess with a smart pointer on acquisition and release.
>> >
>> > The lock is probably the big cost, but the find operation may be working
>> > on a map having about 400+ histogram names.
>> >
>> > I haven't tried to quantify this cost.... if this is temporary... then
>> > don't sweat it.  If this is long term (destined for stable??) then you
>> > might want to <smile> histogram how long it takes to do this stuff, and
>> > be sure you're not hurting too much (use the macro form to do that uber
>> > histogram).
>> >
>> > http://codereview.chromium.org/6679049/
>> >
>
>

[mrossetti, 2011-03-15 17:33:50]

LGTM, I'll be particularly interested in times for the first 1 to 3 characters
so this will be very valuable. I've got some concern that certain cases should
be short-circuited after grabbing the result set and before drilling down and
scoring every result, probably based on the size of the result set.