Remove Enumeration Histograms from the Blink Platform API.

Remove Enumeration Histograms from the Blink Platform API.

Use the new API for logging enumeration histograms.

BUG=583012
Committed: https://crrev.com/23324d97abe98968829056934e544e64237d0acd
Cr-Commit-Position: refs/heads/master@{#373922}

[dtapuska, 2016-02-02 20:23:57]

Description was changed from

==========
Remove Enumeration Histograms from the Blink Platform API.

Use the new API for logging enumeration histograms.

BUG=583012
==========

to

==========
Remove Enumeration Histograms from the Blink Platform API.

Use the new API for logging enumeration histograms.

BUG=583012
==========

[dtapuska, 2016-02-02 20:23:57]

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

[dtapuska, 2016-02-02 20:24:48]

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

[dtapuska, 2016-02-02 20:24:48]

Similar to https://codereview.chromium.org/1659053002/

I hope that I have used
DEFINE_STATIC_LOCAL and
DEFINE_THREAD_SAFE_STATIC_LOCAL in the correct places.

When I couldn't fully understand if the class was only executed from a single
thread I used the thread safe version to be cautious. Owners please check this
carefully.

[haraken, 2016-02-03 00:37:57]

LGTM

I'm not 100% if you're using DEFINE_STATIC_LOCAL and DEFINE_THREAD_SAFE_LOCAL
correctly, but if you misuse DEFINE_STATIC_LOCAL in a thread-unsafe place,
you'll hit the assert in DEFINE_STATIC_LOCAL. So I'm fine with landing this CL
trusting the assert.

https://codereview.chromium.org/1652983005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/1652983005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:92:
DEFINE_SINGLE_RESOURCE_HISTOGRAM(prefix, Manifest)       \

Alphabetical order?

https://codereview.chromium.org/1652983005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/wtf/Partitions.h (right):

https://codereview.chromium.org/1652983005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/Partitions.h:43: typedef void
(*MaxObservedSizeFunction)(size_t sample);

Remove |sample|.

https://codereview.chromium.org/1652983005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/Partitions.h:43: typedef void
(*MaxObservedSizeFunction)(size_t sample);

ReportPartitionAllocSizeFunction ?

[Charlie Reis, 2016-02-03 00:52:26]

content/ LGTM.

[dtapuska, 2016-02-03 14:43:11]

https://codereview.chromium.org/1652983005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/1652983005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:92:
DEFINE_SINGLE_RESOURCE_HISTOGRAM(prefix, Manifest)       \
On 2016/02/03 00:37:57, haraken wrote:
> 
> Alphabetical order?

Done.

https://codereview.chromium.org/1652983005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/wtf/Partitions.h (right):

https://codereview.chromium.org/1652983005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/Partitions.h:43: typedef void
(*MaxObservedSizeFunction)(size_t sample);
On 2016/02/03 00:37:57, haraken wrote:
> 
> Remove |sample|.

Done.

https://codereview.chromium.org/1652983005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/Partitions.h:43: typedef void
(*MaxObservedSizeFunction)(size_t sample);
On 2016/02/03 00:37:57, haraken wrote:
> 
> ReportPartitionAllocSizeFunction ?

Done.

[haraken, 2016-02-03 15:18:53]

LGTM

[dtapuska, 2016-02-03 15:22:07]

On 2016/02/03 15:18:53, haraken wrote:
> LGTM

In order to land still need dependent change reviewed and esprehn's approval for
third_party/WebKit/public/

(haraken can you review the dependent change; you are listed on it)

[haraken, 2016-02-03 15:29:36]

On 2016/02/03 15:22:07, dtapuska wrote:
> On 2016/02/03 15:18:53, haraken wrote:
> > LGTM
> 
> In order to land still need dependent change reviewed and esprehn's approval
for
> third_party/WebKit/public/
> 
> (haraken can you review the dependent change; you are listed on it)

Sorry, I missed that. Done.

[esprehn, 2016-02-05 19:09:07]

Lgtm with a few comments. Btw I think we might want to move the Histograms to
WTF/ they're an isolated library, and that would allow partition alloc and other
WTF things to use Histograms without plumbing. Don't need to do that in this
patch though.

https://codereview.chromium.org/1652983005/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ScriptStreamer.cpp (right):

https://codereview.chromium.org/1652983005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptStreamer.cpp:32:
DEFINE_STATIC_LOCAL(EnumerationHistogram, parseBlockingHistogram,
("WebCore.Scripts.ParsingBlocking.StartedStreaming", 2));
You might want to ask haraken about this one. ScriptStreamer is used on another
thread, but I don't know if it's ever used on multiple threads.

https://codereview.chromium.org/1652983005/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/UseCounter.cpp (right):

https://codereview.chromium.org/1652983005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/UseCounter.cpp:573:
DEFINE_STATIC_LOCAL(EnumerationHistogram, histogram, ("WebCore.FeatureObserver",
UseCounter::NumberOfFeatures));
Do we not use UseCounters on workers? I think this one might need to be thread
safe. I wonder what workers do...

[dtapuska, 2016-02-05 19:25:33]

https://codereview.chromium.org/1652983005/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ScriptStreamer.cpp (right):

https://codereview.chromium.org/1652983005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptStreamer.cpp:32:
DEFINE_STATIC_LOCAL(EnumerationHistogram, parseBlockingHistogram,
("WebCore.Scripts.ParsingBlocking.StartedStreaming", 2));
On 2016/02/05 19:09:07, esprehn wrote:
> You might want to ask haraken about this one. ScriptStreamer is used on
another
> thread, but I don't know if it's ever used on multiple threads.

All of these methods are called before an ASSERT(isMainThread()) is on the
stack...

Look at notifyFinished, startStreamingInternal, etc..

https://codereview.chromium.org/1652983005/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/UseCounter.cpp (right):

https://codereview.chromium.org/1652983005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/UseCounter.cpp:573:
DEFINE_STATIC_LOCAL(EnumerationHistogram, histogram, ("WebCore.FeatureObserver",
UseCounter::NumberOfFeatures));
On 2016/02/05 19:09:07, esprehn wrote:
> Do we not use UseCounters on workers? I think this one might need to be thread
> safe. I wonder what workers do...

I see Page is the only user of UseCounter. Are Page objects ever constructed on
workers?

[esprehn, 2016-02-05 19:30:17]

On 2016/02/05 at 19:25:33, dtapuska wrote:
> ...
>
https://codereview.chromium.org/1652983005/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/frame/UseCounter.cpp:573:
DEFINE_STATIC_LOCAL(EnumerationHistogram, histogram, ("WebCore.FeatureObserver",
UseCounter::NumberOfFeatures));
> On 2016/02/05 19:09:07, esprehn wrote:
> > Do we not use UseCounters on workers? I think this one might need to be
thread
> > safe. I wonder what workers do...
> 
> I see Page is the only user of UseCounter. Are Page objects ever constructed
on workers?

Nope, looks like the way UseCounters are implemented they also count by posting
tasks to the main thread.

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

That's crazy, I hope we don't have many of them. :)

[dtapuska, 2016-02-05 19:41:51]

The CQ bit was checked by dtapuska@chromium.org

[dtapuska, 2016-02-05 19:41:51]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org,
haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1652983005/#ps60001
(title: "Apply similar changes from custom count histograms change to this
change")

[commit-bot: I haz the power, 2016-02-05 19:44:03]

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

[commit-bot: I haz the power, 2016-02-05 20:00:38]

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

[commit-bot: I haz the power, 2016-02-05 20:00:39]

Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dtapuska, 2016-02-05 20:44:01]

The CQ bit was checked by dtapuska@chromium.org

[dtapuska, 2016-02-05 20:44:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, creis@chromium.org, esprehn@chromium.org
Link to the patchset: https://codereview.chromium.org/1652983005/#ps80001
(title: "Rebase two new histograms were added today")

[commit-bot: I haz the power, 2016-02-05 20:44:28]

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

[commit-bot: I haz the power, 2016-02-05 22:07:41]

Description was changed from

==========
Remove Enumeration Histograms from the Blink Platform API.

Use the new API for logging enumeration histograms.

BUG=583012
==========

to

==========
Remove Enumeration Histograms from the Blink Platform API.

Use the new API for logging enumeration histograms.

BUG=583012
==========

[commit-bot: I haz the power, 2016-02-05 22:07:42]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-02-05 22:09:12]

Description was changed from

==========
Remove Enumeration Histograms from the Blink Platform API.

Use the new API for logging enumeration histograms.

BUG=583012
==========

to

==========
Remove Enumeration Histograms from the Blink Platform API.

Use the new API for logging enumeration histograms.

BUG=583012

Committed: https://crrev.com/23324d97abe98968829056934e544e64237d0acd
Cr-Commit-Position: refs/heads/master@{#373922}
==========

[commit-bot: I haz the power, 2016-02-05 22:09:13]

Patchset 5 (id:??) landed as
https://crrev.com/23324d97abe98968829056934e544e64237d0acd
Cr-Commit-Position: refs/heads/master@{#373922}

[sof, 2016-02-07 20:14:18]

sigbjornf@opera.com changed reviewers:
+ sigbjornf@opera.com

[sof, 2016-02-07 20:14:20]

i'm unconvinced if this represents an improvement; a considerable number of
extra static singletons (thread safe or otherwise) in Blink.

https://codereview.chromium.org/1652983005/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/1652983005/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.cpp:1209:
DEFINE_STATIC_LOCAL(EnumerationHistogram, headerValueCategoryHistogram,
("Blink.XHR.setRequestHeader.HeaderValueCategoryInRFC7230",
HeaderValueCategoryByRFC7230End));
XHR can be used by the main thread and workers, so this leads to cross-thread
use. e.g.,


https://storage.googleapis.com/chromium-layout-test-archives/mac_chromium_rel...

[haraken, 2016-02-07 23:47:25]

On 2016/02/07 20:14:20, sof wrote:
> i'm unconvinced if this represents an improvement; a considerable number of
> extra static singletons (thread safe or otherwise) in Blink.

I think the advantage is performance.
https://code.google.com/p/chromium/issues/detail?id=583012

> 
>
https://codereview.chromium.org/1652983005/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.cpp (right):
> 
>
https://codereview.chromium.org/1652983005/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.cpp:1209:
> DEFINE_STATIC_LOCAL(EnumerationHistogram, headerValueCategoryHistogram,
> ("Blink.XHR.setRequestHeader.HeaderValueCategoryInRFC7230",
> HeaderValueCategoryByRFC7230End));
> XHR can be used by the main thread and workers, so this leads to cross-thread
> use. e.g.,
> 
> 
>
https://storage.googleapis.com/chromium-layout-test-archives/mac_chromium_rel...

[esprehn, 2016-02-08 03:47:01]

It's definitely better to do this than allocate 2 string copies, take an
atomic lock, hash the string, and then do a map lookup every time we need
to tick a histogram... :)

Ex. We do this in a loop for every feature and CSS property your page uses.

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[esprehn, 2016-02-08 03:48:10]

It's definitely better to do this than allocate 2 string copies, take an
atomic lock, hash the string, and then do a map lookup every time we need
to tick a histogram... :)

Ex. We do this in a loop for every feature and CSS property your page uses.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sof, 2016-02-08 06:09:37]

On 2016/02/07 23:47:25, haraken wrote:
> On 2016/02/07 20:14:20, sof wrote:
> > i'm unconvinced if this represents an improvement; a considerable number of
> > extra static singletons (thread safe or otherwise) in Blink.
> 
> I think the advantage is performance.
> https://code.google.com/p/chromium/issues/detail?id=583012
> 

Why cannot AtomicStrings be used?

[haraken, 2016-02-08 06:19:21]

On 2016/02/08 06:09:37, sof wrote:
> On 2016/02/07 23:47:25, haraken wrote:
> > On 2016/02/07 20:14:20, sof wrote:
> > > i'm unconvinced if this represents an improvement; a considerable number
of
> > > extra static singletons (thread safe or otherwise) in Blink.
> > 
> > I think the advantage is performance.
> > https://code.google.com/p/chromium/issues/detail?id=583012
> > 
> 
> Why cannot AtomicStrings be used?

If you write
Platform::current()->histogramEnumelration(AtomicString("foo.bar")), you need to
calculate the hash of "foo.bar" every time, don't you?

[sof, 2016-02-08 06:29:56]

On 2016/02/08 06:19:21, haraken wrote:
> On 2016/02/08 06:09:37, sof wrote:
> > On 2016/02/07 23:47:25, haraken wrote:
> > > On 2016/02/07 20:14:20, sof wrote:
> > > > i'm unconvinced if this represents an improvement; a considerable number
> of
> > > > extra static singletons (thread safe or otherwise) in Blink.
> > > 
> > > I think the advantage is performance.
> > > https://code.google.com/p/chromium/issues/detail?id=583012
> > > 
> > 
> > Why cannot AtomicStrings be used?
> 
> If you write
> Platform::current()->histogramEnumelration(AtomicString("foo.bar")), you need
to
> calculate the hash of "foo.bar" every time, don't you?

I don't see why you would have to phrase it that way, as we don't do that
elsewhere. Interning the strings being passed around here seems simpler, at
first look.

[haraken, 2016-02-08 06:42:21]

On 2016/02/08 06:29:56, sof wrote:
> On 2016/02/08 06:19:21, haraken wrote:
> > On 2016/02/08 06:09:37, sof wrote:
> > > On 2016/02/07 23:47:25, haraken wrote:
> > > > On 2016/02/07 20:14:20, sof wrote:
> > > > > i'm unconvinced if this represents an improvement; a considerable
number
> > of
> > > > > extra static singletons (thread safe or otherwise) in Blink.
> > > > 
> > > > I think the advantage is performance.
> > > > https://code.google.com/p/chromium/issues/detail?id=583012
> > > > 
> > > 
> > > Why cannot AtomicStrings be used?
> > 
> > If you write
> > Platform::current()->histogramEnumelration(AtomicString("foo.bar")), you
need
> to
> > calculate the hash of "foo.bar" every time, don't you?
> 
> I don't see why you would have to phrase it that way, as we don't do that
> elsewhere. Interning the strings being passed around here seems simpler, at
> first look.

Another reason would be the overhead to build the histogram factory object. It
would be heavier to call Histogram::Factory::Build() every time.

https://code.google.com/p/chromium/codesearch#chromium/src/base/metrics/histo...

[tyoshino (SeeGerritForStatus), 2016-02-08 08:38:20]

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

[tyoshino (SeeGerritForStatus), 2016-02-08 08:38:21]

https://codereview.chromium.org/1652983005/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/1652983005/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.cpp:1209:
DEFINE_STATIC_LOCAL(EnumerationHistogram, headerValueCategoryHistogram,
("Blink.XHR.setRequestHeader.HeaderValueCategoryInRFC7230",
HeaderValueCategoryByRFC7230End));
On 2016/02/07 at 20:14:19, sof wrote:
> XHR can be used by the main thread and workers, so this leads to cross-thread
use. e.g.,
> 
> 
https://storage.googleapis.com/chromium-layout-test-archives/mac_chromium_rel...

Just received a bug report:
https://code.google.com/p/chromium/issues/detail?id=585025

[esprehn, 2016-02-08 10:01:49]

The histogram library is in base, AtomicString is in WTF, you can't use it
like that. Maybe someday but not today. Also this is faster than that since
it removes the map lookup entirely.

In blink we favor fast by default, perhaps with a little thorny edges. This
is a good example. :)
On Feb 8, 2016 12:38 AM, <tyoshino@chromium.org> wrote:

>
>
https://codereview.chromium.org/1652983005/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.cpp
> (right):
>
https://codereview.chromium.org/1652983005/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.cpp:1209
<https://codereview.chromium.org/1652983005/diff/80001/third_party/WebKit/Sour...>:
> DEFINE_STATIC_LOCAL(EnumerationHistogram, headerValueCategoryHistogram,
> ("Blink.XHR.setRequestHeader.HeaderValueCategoryInRFC7230",
> HeaderValueCategoryByRFC7230End));
> On 2016/02/07 at 20:14:19, sof wrote:
> > XHR can be used by the main thread and workers, so this leads to
> cross-thread use. e.g.,
> >
> >
https://storage.googleapis.com/chromium-layout-test-archives/mac_chromium_rel...
>
> Just received a bug
report:https://code.google.com/p/chromium/issues/detail?id=585025
> https://codereview.chromium.org/1652983005/
>
> --
> You received this message because you are subscribed to the Google Groups
> "Blink Reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to blink-reviews+unsubscribe@chromium.org.
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[esprehn, 2016-02-08 10:04:01]

The histogram library is in base, AtomicString is in WTF, you can't use it
like that. Maybe someday but not today. Also this is faster than that since
it removes the map lookup entirely.

In blink we favor fast by default, perhaps with a little thorny edges. This
is a good example. :)
On Feb 8, 2016 12:38 AM, <tyoshino@chromium.org> wrote:

>
>
https://codereview.chromium.org/1652983005/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.cpp
> (right):
>
https://codereview.chromium.org/1652983005/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.cpp:1209
<https://codereview.chromium.org/1652983005/diff/80001/third_party/WebKit/Sour...>:
> DEFINE_STATIC_LOCAL(EnumerationHistogram, headerValueCategoryHistogram,
> ("Blink.XHR.setRequestHeader.HeaderValueCategoryInRFC7230",
> HeaderValueCategoryByRFC7230End));
> On 2016/02/07 at 20:14:19, sof wrote:
> > XHR can be used by the main thread and workers, so this leads to
> cross-thread use. e.g.,
> >
> >
https://storage.googleapis.com/chromium-layout-test-archives/mac_chromium_rel...
>
> Just received a bug
report:https://code.google.com/p/chromium/issues/detail?id=585025
> https://codereview.chromium.org/1652983005/
>
> --
> You received this message because you are subscribed to the Google Groups
> "Blink Reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to blink-reviews+unsubscribe@chromium.org.
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[kinuko, 2016-02-08 12:31:32]

I have a feeling that the new histogram code is less readable / thread-safe
version seems a bit suboptimal, would we be able to hide DEFINE_*_STATIC_LOCAL
macro usage behind another macro (probably two versions for thread-safe one and
others) so that adding new histograms is as easy as before and we could easily
revise the histogram implementation when we have better idea / better
integration with base/?

[dtapuska, 2016-02-08 13:27:04]

On 2016/02/08 12:31:32, kinuko wrote:
> I have a feeling that the new histogram code is less readable / thread-safe
> version seems a bit suboptimal, would we be able to hide DEFINE_*_STATIC_LOCAL
> macro usage behind another macro (probably two versions for thread-safe one
and
> others) so that adding new histograms is as easy as before and we could easily
> revise the histogram implementation when we have better idea / better
> integration with base/?

I was told that adding new "sub"-macros was not preferred and that the more
verbose
approach was the way to go.

[kinuko, 2016-02-08 15:04:41]

On 2016/02/08 13:27:04, dtapuska wrote:
> On 2016/02/08 12:31:32, kinuko wrote:
> > I have a feeling that the new histogram code is less readable / thread-safe
> > version seems a bit suboptimal, would we be able to hide
DEFINE_*_STATIC_LOCAL
> > macro usage behind another macro (probably two versions for thread-safe one
> and
> > others) so that adding new histograms is as easy as before and we could
easily
> > revise the histogram implementation when we have better idea / better
> > integration with base/?
> 
> I was told that adding new "sub"-macros was not preferred and that the more
> verbose
> approach was the way to go.

Could a bit more context be shared with me-- why it's not preferred?  I want to
have a better understanding of the current policies (and why).

[dtapuska, 2016-02-08 15:21:58]

On 2016/02/08 15:04:41, kinuko wrote:
> On 2016/02/08 13:27:04, dtapuska wrote:
> > On 2016/02/08 12:31:32, kinuko wrote:
> > > I have a feeling that the new histogram code is less readable /
thread-safe
> > > version seems a bit suboptimal, would we be able to hide
> DEFINE_*_STATIC_LOCAL
> > > macro usage behind another macro (probably two versions for thread-safe
one
> > and
> > > others) so that adding new histograms is as easy as before and we could
> easily
> > > revise the histogram implementation when we have better idea / better
> > > integration with base/?
> > 
> > I was told that adding new "sub"-macros was not preferred and that the more
> > verbose
> > approach was the way to go.
> 
> Could a bit more context be shared with me-- why it's not preferred?  I want
to
> have a better understanding of the current policies (and why).

Discussion was originally here: https://codereview.chromium.org/1653043002/#msg7

[dtapuska, 2016-02-08 15:22:04]

On 2016/02/08 15:04:41, kinuko wrote:
> On 2016/02/08 13:27:04, dtapuska wrote:
> > On 2016/02/08 12:31:32, kinuko wrote:
> > > I have a feeling that the new histogram code is less readable /
thread-safe
> > > version seems a bit suboptimal, would we be able to hide
> DEFINE_*_STATIC_LOCAL
> > > macro usage behind another macro (probably two versions for thread-safe
one
> > and
> > > others) so that adding new histograms is as easy as before and we could
> easily
> > > revise the histogram implementation when we have better idea / better
> > > integration with base/?
> > 
> > I was told that adding new "sub"-macros was not preferred and that the more
> > verbose
> > approach was the way to go.
> 
> Could a bit more context be shared with me-- why it's not preferred?  I want
to
> have a better understanding of the current policies (and why).

Discussion was originally here: https://codereview.chromium.org/1653043002/#msg7

[esprehn, 2016-02-08 19:40:33]

I'd prefer we didn't add more macros, this makes it clear what this does,
and makes sure people reuse the object inside a loop instead of calling
through it repeatedly.

I guess I could be convinced otherwise. Haraken@ was the one driving the
removal of many of the macros. I think the idea being that fewer macros
makes for more obvious code, even if it does mean more verbosity.

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[esprehn, 2016-02-08 19:41:53]

I'd prefer we didn't add more macros, this makes it clear what this does,
and makes sure people reuse the object inside a loop instead of calling
through it repeatedly.

I guess I could be convinced otherwise. Haraken@ was the one driving the
removal of many of the macros. I think the idea being that fewer macros
makes for more obvious code, even if it does mean more verbosity.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[haraken, 2016-02-08 23:59:46]

On 2016/02/08 19:41:53, esprehn wrote:
> I'd prefer we didn't add more macros, this makes it clear what this does,
> and makes sure people reuse the object inside a loop instead of calling
> through it repeatedly.
> 
> I guess I could be convinced otherwise. Haraken@ was the one driving the
> removal of many of the macros. I think the idea being that fewer macros
> makes for more obvious code, even if it does mean more verbosity.

If the macro is nice and useful, I'm fan of the macro :)

kinuko-san: What macro do you have in mind for this particular case?

(If you are thinking of something like DEFINE_HISTOGRAM() and
DEFINE_THREAD_SAFE_HISTOGRAM() which just wraps DEFINE_STATIC_LOCAL and
DEFINE_THREAD_SAFE_STATIC_LOCAL, I might not be convinced with them though.)

[kinuko, 2016-02-09 02:04:20]

On 2016/02/08 23:59:46, haraken wrote:
> On 2016/02/08 19:41:53, esprehn wrote:
> > I'd prefer we didn't add more macros, this makes it clear what this does,
> > and makes sure people reuse the object inside a loop instead of calling
> > through it repeatedly.
> > 
> > I guess I could be convinced otherwise. Haraken@ was the one driving the
> > removal of many of the macros. I think the idea being that fewer macros
> > makes for more obvious code, even if it does mean more verbosity.
> 
> If the macro is nice and useful, I'm fan of the macro :)
> 
> kinuko-san: What macro do you have in mind for this particular case?
> 
> (If you are thinking of something like DEFINE_HISTOGRAM() and
> DEFINE_THREAD_SAFE_HISTOGRAM() which just wraps DEFINE_STATIC_LOCAL and
> DEFINE_THREAD_SAFE_STATIC_LOCAL, I might not be convinced with them though.)

In general I agree that we shouldn't add more macros, while the ones I was
asking for would look like these.

The reasons I asked this question are:
- I felt less comfortable having the current DEFINE_*_STATIC_LOCAL macros so
prevailed in the code base, because they're one of the macros that are less
readable (I always look into the definition to make sure how it's expanded
whenever I need to use it) and could be easily abused
- For defining histograms we extensively use macros in chromium, which made me
feel having Blink-version of macros would be ok too.

Given that I generally agree with no-more-macros policy I could be convinced
too.

[haraken, 2016-02-09 04:52:21]

On 2016/02/09 02:04:20, kinuko wrote:
> On 2016/02/08 23:59:46, haraken wrote:
> > On 2016/02/08 19:41:53, esprehn wrote:
> > > I'd prefer we didn't add more macros, this makes it clear what this does,
> > > and makes sure people reuse the object inside a loop instead of calling
> > > through it repeatedly.
> > > 
> > > I guess I could be convinced otherwise. Haraken@ was the one driving the
> > > removal of many of the macros. I think the idea being that fewer macros
> > > makes for more obvious code, even if it does mean more verbosity.
> > 
> > If the macro is nice and useful, I'm fan of the macro :)
> > 
> > kinuko-san: What macro do you have in mind for this particular case?
> > 
> > (If you are thinking of something like DEFINE_HISTOGRAM() and
> > DEFINE_THREAD_SAFE_HISTOGRAM() which just wraps DEFINE_STATIC_LOCAL and
> > DEFINE_THREAD_SAFE_STATIC_LOCAL, I might not be convinced with them though.)
> 
> In general I agree that we shouldn't add more macros, while the ones I was
> asking for would look like these.
> 
> The reasons I asked this question are:
> - I felt less comfortable having the current DEFINE_*_STATIC_LOCAL macros so
> prevailed in the code base, because they're one of the macros that are less
> readable (I always look into the definition to make sure how it's expanded
> whenever I need to use it) and could be easily abused
> - For defining histograms we extensively use macros in chromium, which made me
> feel having Blink-version of macros would be ok too.

Thanks, this is a good point.

Maybe would it be worth considering exporting the macros defined in
base/metrics/histogram_macros.h to Blink?

- From the perspective of readability, it would be the best if we could just use
the chromium-side macros.
- From the perspective of performance, the chromium-side macros will be slower
than the DEFINE_STATIC_STATIC versions but the chromium-side macros will be as
fast as the previous histogram APIs used in Blink (before landing this CL).

[esprehn, 2016-02-09 04:59:30]

I'd rather err on the side of speed here, Blink's workloads grow unbounded with
the size of pages, we also report several hundred counters during page unload.

I've almost never looked at the implementation of DEFINE_STATIC_LOCAL, is there
something specific about them you find hard to use kinuko?

[kinuko, 2016-02-09 06:47:02]

On 2016/02/09 04:59:30, esprehn wrote:
> I've almost never looked at the implementation of DEFINE_STATIC_LOCAL, is
there
> something specific about them you find hard to use kinuko?

It could be probably only me but I just don't like macros that can't be parsed
in the same way as regular c/c++ function calls in general.  But that's how
macros can be useful and I can live with that-- sorry for the rant.

One more ask for the series of changes: could we have a comment in Histogram.h
for the example usages and a brief description of why this model is chosen (e.g.
for the sake of speed), so that other people can quickly see how to add new
histograms & some of the rationales discussed here?  That'd probably save
someone else's time.

[esprehn, 2016-02-09 06:57:29]

On 2016/02/09 at 06:47:02, kinuko wrote:
> On 2016/02/09 04:59:30, esprehn wrote:
> > I've almost never looked at the implementation of DEFINE_STATIC_LOCAL, is
there
> > something specific about them you find hard to use kinuko?
> 
> It could be probably only me but I just don't like macros that can't be parsed
in the same way as regular c/c++ function calls in general.  But that's how
macros can be useful and I can live with that-- sorry for the rant.
> 

fwiw I prefer the WebKit NeverDestroyed<T> template instead of the macro. You
write:

static NeverDestroyed<X> thing(arg1, arg2);

and there's no more macros. I hope we can someday do that instead of migrating
to the CR_DEFINE_STATIC_LOCAL macro Chromium uses.

> One more ask for the series of changes: could we have a comment in Histogram.h
for the example usages and a brief description of why this model is chosen (e.g.
for the sake of speed), so that other people can quickly see how to add new
histograms & some of the rationales discussed here?  That'd probably save
someone else's time.

+1

[haraken, 2016-02-09 07:32:02]

On 2016/02/09 06:57:29, esprehn wrote:
> On 2016/02/09 at 06:47:02, kinuko wrote:
> > On 2016/02/09 04:59:30, esprehn wrote:
> > > I've almost never looked at the implementation of DEFINE_STATIC_LOCAL, is
> there
> > > something specific about them you find hard to use kinuko?
> > 
> > It could be probably only me but I just don't like macros that can't be
parsed
> in the same way as regular c/c++ function calls in general.  But that's how
> macros can be useful and I can live with that-- sorry for the rant.
> > 
> 
> fwiw I prefer the WebKit NeverDestroyed<T> template instead of the macro. You
> write:
> 
> static NeverDestroyed<X> thing(arg1, arg2);
> 
> and there's no more macros. I hope we can someday do that instead of migrating
> to the CR_DEFINE_STATIC_LOCAL macro Chromium uses.
> 
> > One more ask for the series of changes: could we have a comment in
Histogram.h
> for the example usages and a brief description of why this model is chosen
(e.g.
> for the sake of speed), so that other people can quickly see how to add new
> histograms & some of the rationales discussed here?  That'd probably save
> someone else's time.
> 
> +1

(I'm not planning to mess up the discussion here but let me leave one comment :)

I begin to prefer using the chromium-side macros (i.e.,
base/metrics/histogram_macros.h) in Blink. It would be slightly slower than the
DEFINE_STATIC_LOCAL version, so it would be problematic to use the macros in
performance-sensitive places. However, we shouldn't use histogram counters in
performance-sensitive places in the first place. Then it would make sense to
just use the same syntax as Chromium in Blink, which is one of the goals of the
Onion Soup project.

[sof, 2016-02-09 18:49:18]

On 2016/02/09 07:32:02, haraken wrote:
> On 2016/02/09 06:57:29, esprehn wrote:
> > On 2016/02/09 at 06:47:02, kinuko wrote:
> > > On 2016/02/09 04:59:30, esprehn wrote:
> > > > I've almost never looked at the implementation of DEFINE_STATIC_LOCAL,
is
> > there
> > > > something specific about them you find hard to use kinuko?
> > > 
> > > It could be probably only me but I just don't like macros that can't be
> parsed
> > in the same way as regular c/c++ function calls in general.  But that's how
> > macros can be useful and I can live with that-- sorry for the rant.
> > > 
> > 
> > fwiw I prefer the WebKit NeverDestroyed<T> template instead of the macro.
You
> > write:
> > 
> > static NeverDestroyed<X> thing(arg1, arg2);
> > 
> > and there's no more macros. I hope we can someday do that instead of
migrating
> > to the CR_DEFINE_STATIC_LOCAL macro Chromium uses.
> > 
> > > One more ask for the series of changes: could we have a comment in
> Histogram.h
> > for the example usages and a brief description of why this model is chosen
> (e.g.
> > for the sake of speed), so that other people can quickly see how to add new
> > histograms & some of the rationales discussed here?  That'd probably save
> > someone else's time.
> > 
> > +1
> 
> (I'm not planning to mess up the discussion here but let me leave one comment
:)
> 
> I begin to prefer using the chromium-side macros (i.e.,
> base/metrics/histogram_macros.h) in Blink. It would be slightly slower than
the
> DEFINE_STATIC_LOCAL version, so it would be problematic to use the macros in
> performance-sensitive places. However, we shouldn't use histogram counters in
> performance-sensitive places in the first place. Then it would make sense to
> just use the same syntax as Chromium in Blink, which is one of the goals of
the
> Onion Soup project.

I haven't seen this mentioned in this thread, so let me plant a seed... by
introducing & using a macro that packages up all the histogramming details you
also give yourself the ability to have that macro expand to nothing.

e.g., it's not a given that a low-end configuration should have an extra like
histogramming support baked in. ?

[esprehn, 2016-02-09 19:09:07]

On 2016/02/09 at 18:49:18, sigbjornf wrote:
> ...
> 
> I haven't seen this mentioned in this thread, so let me plant a seed... by
introducing & using a macro that packages up all the histogramming details you
also give yourself the ability to have that macro expand to nothing.
> 
> e.g., it's not a given that a low-end configuration should have an extra like
histogramming support baked in. ?

That doesn't require a macro, we can also add an ifdef around the Histogram
class that turns it off if you need that. :)

One of the primary design principles of Onion Soup is that we should be creating
platform/ wrappers around Chromium APIs to make them friendly and fast with our
workloads and the web platform, not just wholesale switching to whatever
Chromium uses. Chromium at large has different design goals and principles,
that's why we have things like WTF, PartitionAlloc, Oilpan, and toFoo() casting
macros and they don't. Hopefully someday we can merge our design principles,
Onion Soup is the first step!

This patch is a good example of variation in our design strategy. Chromium
(often) prefers "slow and safe by default" (UMA macros are always thread safe),
Blink (often) prefers "fast and sharp by default" (you can choose the wrong
static local macro, but it's faster when you choose right, asserts will likely
catch you).

I'm fine switching to the UMA_HISTOGRAM macros in blink, provided it's not a
measurable perf regression over this patch, but I still think we should have
some kind of wrapper for UseCounter and the Animation cases. Using those macros
in a loop is hitting the atomic read hundreds of times in a row which is silly.
:)

[haraken, 2016-02-10 00:20:40]

On 2016/02/09 19:09:07, esprehn wrote:
> On 2016/02/09 at 18:49:18, sigbjornf wrote:
> > ...
> > 
> > I haven't seen this mentioned in this thread, so let me plant a seed... by
> introducing & using a macro that packages up all the histogramming details you
> also give yourself the ability to have that macro expand to nothing.
> > 
> > e.g., it's not a given that a low-end configuration should have an extra
like
> histogramming support baked in. ?
> 
> That doesn't require a macro, we can also add an ifdef around the Histogram
> class that turns it off if you need that. :)
> 
> One of the primary design principles of Onion Soup is that we should be
creating
> platform/ wrappers around Chromium APIs to make them friendly and fast with
our
> workloads and the web platform, not just wholesale switching to whatever
> Chromium uses. Chromium at large has different design goals and principles,
> that's why we have things like WTF, PartitionAlloc, Oilpan, and toFoo()
casting
> macros and they don't. Hopefully someday we can merge our design principles,
> Onion Soup is the first step!
> 
> This patch is a good example of variation in our design strategy. Chromium
> (often) prefers "slow and safe by default" (UMA macros are always thread
safe),
> Blink (often) prefers "fast and sharp by default" (you can choose the wrong
> static local macro, but it's faster when you choose right, asserts will likely
> catch you).
> 
> I'm fine switching to the UMA_HISTOGRAM macros in blink, provided it's not a
> measurable perf regression over this patch, but I still think we should have
> some kind of wrapper for UseCounter and the Animation cases. Using those
macros
> in a loop is hitting the atomic read hundreds of times in a row which is
silly.
> :)

Maybe a good balancing point would be making UMA_HISTOGRAM macros by default &
using the nastier but faster version in a couple of performance-sensitive places
like Animation cases?

[esprehn, 2016-02-10 00:27:23]

On 2016/02/10 at 00:20:40, haraken wrote:
> On 2016/02/09 19:09:07, esprehn wrote:
> > On 2016/02/09 at 18:49:18, sigbjornf wrote:
> > ...
> 
> Maybe a good balancing point would be making UMA_HISTOGRAM macros by default &
using the nastier but faster version in a couple of performance-sensitive places
like Animation cases?

sgtm, post a patch! Perhaps dtapuska@ is willing to be a hero again? :)

[haraken, 2016-02-10 00:52:02]

On 2016/02/10 00:27:23, esprehn wrote:
> On 2016/02/10 at 00:20:40, haraken wrote:
> > On 2016/02/09 19:09:07, esprehn wrote:
> > > On 2016/02/09 at 18:49:18, sigbjornf wrote:
> > > ...
> > 
> > Maybe a good balancing point would be making UMA_HISTOGRAM macros by default
&
> using the nastier but faster version in a couple of performance-sensitive
places
> like Animation cases?
> 
> sgtm, post a patch! Perhaps dtapuska@ is willing to be a hero again? :)

I'm secretly hoping so :)

[kinuko, 2016-02-10 01:36:06]

On 2016/02/10 00:52:02, haraken wrote:
> On 2016/02/10 00:27:23, esprehn wrote:
> > On 2016/02/10 at 00:20:40, haraken wrote:
> > > On 2016/02/09 19:09:07, esprehn wrote:
> > > > On 2016/02/09 at 18:49:18, sigbjornf wrote:
> > > > ...
> > > 
> > > Maybe a good balancing point would be making UMA_HISTOGRAM macros by
default
> &
> > using the nastier but faster version in a couple of performance-sensitive
> places
> > like Animation cases?
> > 
> > sgtm, post a patch! Perhaps dtapuska@ is willing to be a hero again? :)
> 
> I'm secretly hoping so :)

SGTM++

[dtapuska, 2016-02-10 01:42:00]

On 2016/02/10 01:36:06, kinuko wrote:
> On 2016/02/10 00:52:02, haraken wrote:
> > On 2016/02/10 00:27:23, esprehn wrote:
> > > On 2016/02/10 at 00:20:40, haraken wrote:
> > > > On 2016/02/09 19:09:07, esprehn wrote:
> > > > > On 2016/02/09 at 18:49:18, sigbjornf wrote:
> > > > > ...
> > > > 
> > > > Maybe a good balancing point would be making UMA_HISTOGRAM macros by
> default
> > &
> > > using the nastier but faster version in a couple of performance-sensitive
> > places
> > > like Animation cases?
> > > 
> > > sgtm, post a patch! Perhaps dtapuska@ is willing to be a hero again? :)
> > 
> > I'm secretly hoping so :)
> 
> SGTM++

Shouldn't be that hard I hope; a couple of sed commands should do it. Just have
to find the time to do it..