Introduce RuntimeCallStats::IsEnabled

Introduce RuntimeCallStats::IsEnabled

Run all "is enabled" checks through a single point, which makes it
easier to adjust the condition.

[alph, 2016-10-28 21:29:44]

alph@chromium.org changed reviewers:
+ cbruni@chromium.org, fmeawad@chromium.org

[fmeawad, 2016-10-28 21:54:25]

https://codereview.chromium.org/2461733005/diff/1/src/counters.h
File src/counters.h (right):

https://codereview.chromium.org/2461733005/diff/1/src/counters.h#newcode810
src/counters.h:810: static bool IsEnabled() {
Wouldn't that add an extra call per check? Why a method and not a macro?

[fmeawad, 2016-10-28 21:54:38]

fmeawad@chromium.org changed reviewers:
+ lpy@chromium.org

[fmeawad, 2016-10-28 21:54:38]

+ lpy

[lpy, 2016-10-28 21:57:37]

We are going to make runtime stats use V8 flag. See
https://codereview.chromium.org/2460973003/

[alph, 2016-10-28 22:03:56]

On 2016/10/28 21:54:25, fmeawad wrote:
> https://codereview.chromium.org/2461733005/diff/1/src/counters.h
> File src/counters.h (right):
> 
> https://codereview.chromium.org/2461733005/diff/1/src/counters.h#newcode810
> src/counters.h:810: static bool IsEnabled() {
> Wouldn't that add an extra call per check? Why a method and not a macro?

I trust the compiler will be capable to inline it. I'm fine with turning it into
a macro though.
But we need to be cautious to not making a habit of turning all small functions
into macros. ;-)

[Camillo Bruni, 2016-10-31 09:57:18]

This CL will conflict with lpy's CL
(https://codereview.chromium.org/2460973003).

Given that that both CL's do the almost the same I have not much of a
preference.
Maybe you can merge lpy's CL into this one.

Additionally we see whether this has a performance impact, I usually just
micro-benchmark a cheap runtime function with %RunningInSimulator() which should
give you enough feedback.

https://codereview.chromium.org/2461733005/diff/1/src/counters.h
File src/counters.h (right):

https://codereview.chromium.org/2461733005/diff/1/src/counters.h#newcode810
src/counters.h:810: static bool IsEnabled() {
On 2016/10/28 at 21:54:25, fmeawad wrote:
> Wouldn't that add an extra call per check? Why a method and not a macro?

From what I've seen so far is that clang inlines very aggressively, so I'm
pretty confident that this actually works as intended.

[Camillo Bruni, 2016-10-31 09:57:58]

https://codereview.chromium.org/2461733005/diff/1/src/counters.h
File src/counters.h (right):

https://codereview.chromium.org/2461733005/diff/1/src/counters.h#newcode812
src/counters.h:812: FLAG_runtime_call_stats;
nit: put the V8_UNLIKELY around the whole clause, we generally expect the
runtime stats to be disabled

[lpy, 2016-10-31 16:32:49]

On 2016/10/31 09:57:18, Camillo Bruni wrote:
> This CL will conflict with lpy's CL
> (https://codereview.chromium.org/2460973003).
> 
> Given that that both CL's do the almost the same I have not much of a
> preference.
> Maybe you can merge lpy's CL into this one.

Just to clarify, the purposes of these two CLs are not the same, my CL is trying
to move tracing category check in V8 into V8 flags, and use flags only.

[lpy, 2016-10-31 20:20:34]

I suggest us not landing this CL, since in my CL we are using one single flag,
FLAG_runtime_stats everywhere.

Plus, I suggest us having a separate bit in FLAG_runtime_stats to indicate the
runtime stats is triggered by sampling, and a separate
disabled-by-default-v8.runtime_stats_sampling category, so that we avoid having
runtime stats in trace file twice by default.

WDYT?

[alph, 2016-10-31 20:26:32]

On 2016/10/31 20:20:34, lpy wrote:
> I suggest us not landing this CL, since in my CL we are using one single flag,
> FLAG_runtime_stats everywhere.
> 
> Plus, I suggest us having a separate bit in FLAG_runtime_stats to indicate the
> runtime stats is triggered by sampling, and a separate
> disabled-by-default-v8.runtime_stats_sampling category, so that we avoid
having
> runtime stats in trace file twice by default.
> 
> WDYT?

Yes. That's my plan.

[lpy, 2016-10-31 20:28:59]

On 2016/10/31 20:26:32, alph wrote:
> On 2016/10/31 20:20:34, lpy wrote:
> > I suggest us not landing this CL, since in my CL we are using one single
flag,
> > FLAG_runtime_stats everywhere.
> > 
> > Plus, I suggest us having a separate bit in FLAG_runtime_stats to indicate
the
> > runtime stats is triggered by sampling, and a separate
> > disabled-by-default-v8.runtime_stats_sampling category, so that we avoid
> having
> > runtime stats in trace file twice by default.
> > 
> > WDYT?
> 
> Yes. That's my plan.

Great, I can do a follow-up patch once I land my patch.