Support caching histograms so that a lookup isn't done in each iteration.

Support caching histograms so that a lookup isn't done in each iteration.

Create a histogram object in blink and use it directly so that we can
cache the base::HistogramBase reference.

Adding these cache values yields approximately 10% gain on Android
blink_perf.svg HitTest performance benchmark.

BUG=566913
Committed: https://crrev.com/6b12d45cab8c0433eb702f1079c469b692d70208
Cr-Commit-Position: refs/heads/master@{#372725}

[dtapuska, 2016-01-29 22:28:26]

Description was changed from

==========
Support caching histograms so that a lookup isn't done in each iteration.

Adjust the blink API to take an additional parameter. When the parameter
is provided the author must ensure it is providing a thread safe pointer;
either locked my a mutex or ensure that a static is executed only from
one thread.

Adding these cache values yields approximately 10% gain on Android
blink_perf.svg HitTest performance benchmark.

BUG=566913
==========

to

==========
Support caching histograms so that a lookup isn't done in each iteration.

Adjust the blink API to take an additional parameter. When the parameter
is provided the author must ensure it is providing a thread safe pointer;
either locked my a mutex or ensure that a static is executed only from
one thread.

Adding these cache values yields approximately 10% gain on Android
blink_perf.svg HitTest performance benchmark.

BUG=566913
==========

[dtapuska, 2016-01-29 22:28:26]

dtapuska@chromium.org changed reviewers:
+ esprehn@chromium.org, rbyers@chromium.org

[dtapuska, 2016-01-29 22:29:17]

See the perf tryjob here:

https://codereview.chromium.org/1631143003/

Looking for comments on my approach. I don't necessarily
care for it due to threading concerns; but I don't have
a better suggestion.

[esprehn, 2016-01-29 23:01:19]

We don't need all this platform abstraction anymore, Platform can just use base
directly and these just look like static methods?

I think you want to add a Histogram object to platform, and an API directly
there. No need to plumb out to content. I'd suggest a static object API so you
can just write:

DEFINE_STATIC_LOCAL(Histogram, myCounter, (a, b, c));

myCounter.count(...);

[esprehn, 2016-01-29 23:05:41]

https://codereview.chromium.org/1647883004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutView.cpp (right):

https://codereview.chromium.org/1647883004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutView.cpp:71:
Platform::current()->histogramCustomCounts("Event.Latency.HitTestRecursive",
duration, 0, 10000000, 100, &histogramCache);
DEFINE_STATIC_LOCAL(Histogram, hitTestLatency,
("Event.Latency.HitTestRecursive", 0, 10000000, 100));

hitTestLatency.record(duration);

[dtapuska, 2016-02-01 16:10:57]

Description was changed from

==========
Support caching histograms so that a lookup isn't done in each iteration.

Adjust the blink API to take an additional parameter. When the parameter
is provided the author must ensure it is providing a thread safe pointer;
either locked my a mutex or ensure that a static is executed only from
one thread.

Adding these cache values yields approximately 10% gain on Android
blink_perf.svg HitTest performance benchmark.

BUG=566913
==========

to

==========
Support caching histograms so that a lookup isn't done in each iteration.
    
Create a histogram object in blink and use it directly so that we can
cache the base::HistogramBase reference.
    
Adding these cache values yields approximately 10% gain on Android
blink_perf.svg HitTest performance benchmark.
    
BUG=566913
==========

[dtapuska, 2016-02-01 16:13:03]

On 2016/01/29 23:05:41, esprehn wrote:
>
https://codereview.chromium.org/1647883004/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/layout/LayoutView.cpp (right):
> 
>
https://codereview.chromium.org/1647883004/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/layout/LayoutView.cpp:71:
> Platform::current()->histogramCustomCounts("Event.Latency.HitTestRecursive",
> duration, 0, 10000000, 100, &histogramCache);
> DEFINE_STATIC_LOCAL(Histogram, hitTestLatency,
> ("Event.Latency.HitTestRecursive", 0, 10000000, 100));
> 
> hitTestLatency.record(duration);

Ok; done. Can you take a look at this?

I did not handle the problem with multi-threaded static variable initialization.
Let me know if I should.

Ideally I'd like to remove all of the BlinkPlatform::APIs for histograms and go
with this. Although I think I need to tackle the multi-threaded static
initialization; I believe only MSVC doesn't do the C++11 magic statics.

[esprehn, 2016-02-01 16:21:01]

I think you just want to use DEFINE_THREAD_SAFE_STATIC_LOCAL whenever you need a
thread safe one (until we have C++11 magic statics). Please don't make a new
static macro, just use the one we have. :)

https://codereview.chromium.org/1647883004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/Histogram.h (right):

https://codereview.chromium.org/1647883004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/Histogram.h:18: static type name constructor;
please use DEFINE_STATIC_LOCAL, don't invent new macros

https://codereview.chromium.org/1647883004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/Histogram.h:39: SparseHistogram(const char*
name);
explicit

[dtapuska, 2016-02-01 16:28:51]

https://codereview.chromium.org/1647883004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/Histogram.h (right):

https://codereview.chromium.org/1647883004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/Histogram.h:18: static type name constructor;
On 2016/02/01 16:21:00, esprehn wrote:
> please use DEFINE_STATIC_LOCAL, don't invent new macros

Sorry didn't know that actually existed.

https://codereview.chromium.org/1647883004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/Histogram.h:39: SparseHistogram(const char*
name);
On 2016/02/01 16:21:00, esprehn wrote:
> explicit

Done.

[esprehn, 2016-02-01 16:35:08]

lgtm

https://codereview.chromium.org/1647883004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/Histogram.h (right):

https://codereview.chromium.org/1647883004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/Histogram.h:23:
CustomCountHistogram(base::HistogramBase*);
explicit, note that even private and protected methods need explicit otherwise
you can inadvertently construct an object inside a member function. C++ is
crazy.

[dtapuska, 2016-02-01 16:43:31]

https://codereview.chromium.org/1647883004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/Histogram.h (right):

https://codereview.chromium.org/1647883004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/Histogram.h:23:
CustomCountHistogram(base::HistogramBase*);
On 2016/02/01 16:35:08, esprehn wrote:
> explicit, note that even private and protected methods need explicit otherwise
> you can inadvertently construct an object inside a member function. C++ is
> crazy.

Done.

[dtapuska, 2016-02-01 18:26:36]

The CQ bit was checked by dtapuska@chromium.org

[dtapuska, 2016-02-01 18:26:37]

The patchset sent to the CQ was uploaded after l-g-t-m from esprehn@chromium.org
Link to the patchset: https://codereview.chromium.org/1647883004/#ps60001
(title: "Add explicit to protected ctor as well")

[commit-bot: I haz the power, 2016-02-01 18:28:02]

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

[commit-bot: I haz the power, 2016-02-01 19:48:24]

Description was changed from

==========
Support caching histograms so that a lookup isn't done in each iteration.
    
Create a histogram object in blink and use it directly so that we can
cache the base::HistogramBase reference.
    
Adding these cache values yields approximately 10% gain on Android
blink_perf.svg HitTest performance benchmark.
    
BUG=566913
==========

to

==========
Support caching histograms so that a lookup isn't done in each iteration.
    
Create a histogram object in blink and use it directly so that we can
cache the base::HistogramBase reference.
    
Adding these cache values yields approximately 10% gain on Android
blink_perf.svg HitTest performance benchmark.
    
BUG=566913
==========

[commit-bot: I haz the power, 2016-02-01 19:48:25]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-02-01 19:49:37]

Description was changed from

==========
Support caching histograms so that a lookup isn't done in each iteration.
    
Create a histogram object in blink and use it directly so that we can
cache the base::HistogramBase reference.
    
Adding these cache values yields approximately 10% gain on Android
blink_perf.svg HitTest performance benchmark.
    
BUG=566913
==========

to

==========
Support caching histograms so that a lookup isn't done in each iteration.

Create a histogram object in blink and use it directly so that we can
cache the base::HistogramBase reference.

Adding these cache values yields approximately 10% gain on Android
blink_perf.svg HitTest performance benchmark.

BUG=566913

Committed: https://crrev.com/6b12d45cab8c0433eb702f1079c469b692d70208
Cr-Commit-Position: refs/heads/master@{#372725}
==========

[commit-bot: I haz the power, 2016-02-01 19:49:37]

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

[Alexei Svitkine (slow), 2016-02-01 19:55:31]

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

[Alexei Svitkine (slow), 2016-02-01 19:55:32]

https://codereview.chromium.org/1647883004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/Histogram.cpp (right):

https://codereview.chromium.org/1647883004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/Histogram.cpp:14: m_histogram =
base::Histogram::FactoryGet(name, min, max, bucketCount,
base::HistogramBase::kNoFlags);
I think you want kUmaTargetedHistogramFlag here and on line 28, else these
histograms won't be sent up to UMA server.

[bcwhite, 2016-02-01 20:06:17]

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

[bcwhite, 2016-02-01 20:06:18]

https://codereview.chromium.org/1647883004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/Histogram.h (right):

https://codereview.chromium.org/1647883004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/Histogram.h:17: class PLATFORM_EXPORT
CustomCountHistogram {
Why not have a HistogramBase equivalent rather than specific
CustomCountHistogram, EnumerationHistogram, etc.  That would make a more
extensible system for the future.

[dtapuska, 2016-02-01 20:25:06]

On 2016/02/01 20:06:18, bcwhite wrote:
>
https://codereview.chromium.org/1647883004/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/platform/Histogram.h (right):
> 
>
https://codereview.chromium.org/1647883004/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/Histogram.h:17: class PLATFORM_EXPORT
> CustomCountHistogram {
> Why not have a HistogramBase equivalent rather than specific
> CustomCountHistogram, EnumerationHistogram, etc.  That would make a more
> extensible system for the future.

I thought construction via this was was easiest in that you can't confuse which
type you are creating without
having a whole bunch of targeted macros.