Use strings for keys in the histogram map.

Use strings for keys in the histogram map.

Using the MD5 hash of the name for the key makes for an
efficient mapping but calculating that hash is expensive.

A simple string-compare that first checks the length
cuts over 40% off the run-time according to the timing
tests. And since timing tests are skewed towards names
of only a few distinct lengths, the real-world
improvement should be even more.

BUG=601572
Committed: https://crrev.com/fca9ef5451d1dc700bbf2370e1147b05ad704cfc
Cr-Commit-Position: refs/heads/master@{#385893}

[bcwhite, 2016-04-07 20:05:31]

bcwhite@chromium.org changed reviewers:
+ asvitkine@chromium.org

[bcwhite, 2016-04-07 20:10:19]

Description was changed from

==========
Use strings for keys in the histogram map.

Using the MD5 hash of the name for the key makes for an
efficient mapping but calculating that hash is expensive.

A simple string-compare that first checks the length
cuts over 40% off the run-time according to the timing
tests. And since timing tests are skewed towards names
of only a few lengths, the real-world improvement should
be even more.

BUG=
==========

to

==========
Use strings for keys in the histogram map.

Using the MD5 hash of the name for the key makes for an
efficient mapping but calculating that hash is expensive.

A simple string-compare that first checks the length
cuts over 40% off the run-time according to the timing
tests. And since timing tests are skewed towards names
of only a few distinct lengths, the real-world
improvement should be even more.

BUG=
==========

[Alexei Svitkine (slow), 2016-04-07 20:21:10]

lgtm

Can you file a crbug for this?

https://codereview.chromium.org/1866423002/diff/20001/base/metrics/statistics...
File base/metrics/statistics_recorder.h (right):

https://codereview.chromium.org/1866423002/diff/20001/base/metrics/statistics...
base/metrics/statistics_recorder.h:34: // A class used as a key for the
histogram map below. It always references
Nit: Move this within the public section below.

https://codereview.chromium.org/1866423002/diff/20001/base/metrics/statistics...
base/metrics/statistics_recorder.h:57: return wordmemcmp(data(), rhs.data(),
length()) < 0;
Nit: How about "return StringPiece::operator<(rhs);" so that it doesn't require
having knowledge of wordmemcmp internal StringPiece impl.

[bcwhite, 2016-04-07 20:24:33]

https://codereview.chromium.org/1866423002/diff/20001/base/metrics/statistics...
File base/metrics/statistics_recorder.h (right):

https://codereview.chromium.org/1866423002/diff/20001/base/metrics/statistics...
base/metrics/statistics_recorder.h:34: // A class used as a key for the
histogram map below. It always references
On 2016/04/07 20:21:10, Alexei Svitkine wrote:
> Nit: Move this within the public section below.

Oh?  It's not something that should be accessed outside of this class.

https://codereview.chromium.org/1866423002/diff/20001/base/metrics/statistics...
base/metrics/statistics_recorder.h:57: return wordmemcmp(data(), rhs.data(),
length()) < 0;
On 2016/04/07 20:21:10, Alexei Svitkine wrote:
> Nit: How about "return StringPiece::operator<(rhs);" so that it doesn't
require
> having knowledge of wordmemcmp internal StringPiece impl.

I had something like that previously but removed it so that it wouldn't do yet
another set of length checks.  Perhaps the compiler can optimize that out.

[bcwhite, 2016-04-07 20:27:16]

Description was changed from

==========
Use strings for keys in the histogram map.

Using the MD5 hash of the name for the key makes for an
efficient mapping but calculating that hash is expensive.

A simple string-compare that first checks the length
cuts over 40% off the run-time according to the timing
tests. And since timing tests are skewed towards names
of only a few distinct lengths, the real-world
improvement should be even more.

BUG=
==========

to

==========
Use strings for keys in the histogram map.

Using the MD5 hash of the name for the key makes for an
efficient mapping but calculating that hash is expensive.

A simple string-compare that first checks the length
cuts over 40% off the run-time according to the timing
tests. And since timing tests are skewed towards names
of only a few distinct lengths, the real-world
improvement should be even more.

BUG=601572
==========

[Alexei Svitkine (slow), 2016-04-07 20:49:09]

https://codereview.chromium.org/1866423002/diff/20001/base/metrics/statistics...
File base/metrics/statistics_recorder.h (right):

https://codereview.chromium.org/1866423002/diff/20001/base/metrics/statistics...
base/metrics/statistics_recorder.h:34: // A class used as a key for the
histogram map below. It always references
On 2016/04/07 20:24:33, bcwhite wrote:
> On 2016/04/07 20:21:10, Alexei Svitkine wrote:
> > Nit: Move this within the public section below.
> 
> Oh?  It's not something that should be accessed outside of this class.

From the style guide:

"Use the specified order of declarations within a class: public: before
private:, methods before data members (variables), etc.

Your class definition should start with its public: section, followed by its
protected: section and then its private: section. If any of these sections are
empty, omit them."

Your code as-is isn't following this. So, either follow my suggestion or find a
way to put this in a private section while following the style guide practice.

Another option is to move this outside the StatisticsRecorder class into a
separate "namespace internal" inside the base namespace.

https://codereview.chromium.org/1866423002/diff/20001/base/metrics/statistics...
base/metrics/statistics_recorder.h:57: return wordmemcmp(data(), rhs.data(),
length()) < 0;
On 2016/04/07 20:24:33, bcwhite wrote:
> On 2016/04/07 20:21:10, Alexei Svitkine wrote:
> > Nit: How about "return StringPiece::operator<(rhs);" so that it doesn't
> require
> > having knowledge of wordmemcmp internal StringPiece impl.
> 
> I had something like that previously but removed it so that it wouldn't do yet
> another set of length checks.  Perhaps the compiler can optimize that out.

I see - I guess since we don't if the compiler will optimize those out, I'm fine
to keep as-is, just mention explicitly that this doesn't call the base operator
to avoid re-doing those length checks.

[bcwhite, 2016-04-07 21:09:16]

https://codereview.chromium.org/1866423002/diff/20001/base/metrics/statistics...
File base/metrics/statistics_recorder.h (right):

https://codereview.chromium.org/1866423002/diff/20001/base/metrics/statistics...
base/metrics/statistics_recorder.h:34: // A class used as a key for the
histogram map below. It always references
> Your class definition should start with its public: section, followed by
its...

It does say "should".  I think that would allow for variations if language
limitations (such as the order of definitions of types) made it impractical.

But whatever.  It's not important so I'll just move it back into "public".

Done.

https://codereview.chromium.org/1866423002/diff/20001/base/metrics/statistics...
base/metrics/statistics_recorder.h:57: return wordmemcmp(data(), rhs.data(),
length()) < 0;
On 2016/04/07 20:49:08, Alexei Svitkine wrote:
> On 2016/04/07 20:24:33, bcwhite wrote:
> > On 2016/04/07 20:21:10, Alexei Svitkine wrote:
> > > Nit: How about "return StringPiece::operator<(rhs);" so that it doesn't
> > require
> > > having knowledge of wordmemcmp internal StringPiece impl.
> > 
> > I had something like that previously but removed it so that it wouldn't do
yet
> > another set of length checks.  Perhaps the compiler can optimize that out.
> 
> I see - I guess since we don't if the compiler will optimize those out, I'm
fine
> to keep as-is, just mention explicitly that this doesn't call the base
operator
> to avoid re-doing those length checks.

Done.

[Alexei Svitkine (slow), 2016-04-07 21:10:41]

lgtm

[bcwhite, 2016-04-07 21:16:10]

The CQ bit was checked by bcwhite@chromium.org

[commit-bot: I haz the power, 2016-04-07 21:17:53]

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

[commit-bot: I haz the power, 2016-04-07 22:06:37]

Description was changed from

==========
Use strings for keys in the histogram map.

Using the MD5 hash of the name for the key makes for an
efficient mapping but calculating that hash is expensive.

A simple string-compare that first checks the length
cuts over 40% off the run-time according to the timing
tests. And since timing tests are skewed towards names
of only a few distinct lengths, the real-world
improvement should be even more.

BUG=601572
==========

to

==========
Use strings for keys in the histogram map.

Using the MD5 hash of the name for the key makes for an
efficient mapping but calculating that hash is expensive.

A simple string-compare that first checks the length
cuts over 40% off the run-time according to the timing
tests. And since timing tests are skewed towards names
of only a few distinct lengths, the real-world
improvement should be even more.

BUG=601572
==========

[commit-bot: I haz the power, 2016-04-07 22:06:38]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-04-07 22:07:44]

Description was changed from

==========
Use strings for keys in the histogram map.

Using the MD5 hash of the name for the key makes for an
efficient mapping but calculating that hash is expensive.

A simple string-compare that first checks the length
cuts over 40% off the run-time according to the timing
tests. And since timing tests are skewed towards names
of only a few distinct lengths, the real-world
improvement should be even more.

BUG=601572
==========

to

==========
Use strings for keys in the histogram map.

Using the MD5 hash of the name for the key makes for an
efficient mapping but calculating that hash is expensive.

A simple string-compare that first checks the length
cuts over 40% off the run-time according to the timing
tests. And since timing tests are skewed towards names
of only a few distinct lengths, the real-world
improvement should be even more.

BUG=601572

Committed: https://crrev.com/fca9ef5451d1dc700bbf2370e1147b05ad704cfc
Cr-Commit-Position: refs/heads/master@{#385893}
==========

[commit-bot: I haz the power, 2016-04-07 22:07:45]

Patchset 3 (id:??) landed as
https://crrev.com/fca9ef5451d1dc700bbf2370e1147b05ad704cfc
Cr-Commit-Position: refs/heads/master@{#385893}