Fix use of history timers in background threads.

Fix use of history timers in background threads.

HistoryTimer's can't run in the background because they use a timer
with a simple api of Start() and Stop(). This CL fixes this problem
by building a base class TimedHistogram that doesn't have a timer.

The class HistoryTimer is modified to use this base class so that
uses that run on the foreground thread do not need to be modified.

It also adds a new class TimedHistogramScope that defines the timer
in this class. This allows the corresopnding TimedHistogram class to
be type safe.

BUG=v8:6361
Review-Url: https://codereview.chromium.org/2929853003
Cr-Commit-Position: refs/heads/master@{#46150}
Committed: https://chromium.googlesource.com/v8/v8/+/d4a108078d93a58fb2c296a59328f25980ded3ea

[kschimpf, 2017-06-08 21:06:29]

The CQ bit was checked by kschimpf@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-08 21:06:40]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kschimpf, 2017-06-08 21:07:09]

Description was changed from

==========
Fix use of history timers in background threads.

HistoryTimer's can't run in the background because they use a timer
with a simple api of Start() and Stop(). This CL fixes this problem
by building a base class TimedHistogram that doesn't have a timer.

The class HistoryTimer is modified to use this base class so that
uses that run on the foreground thread do not need to be modified.

It also adds a new class TimedHistogramScope that defines the timer
in this class. This allows the corresopnding TimedHistogram class to
be type safe.

BUG=v8:6361
==========

to

==========
Fix use of history timers in background threads.

HistoryTimer's can't run in the background because they use a timer
with a simple api of Start() and Stop(). This CL fixes this problem
by building a base class TimedHistogram that doesn't have a timer.

The class HistoryTimer is modified to use this base class so that
uses that run on the foreground thread do not need to be modified.

It also adds a new class TimedHistogramScope that defines the timer
in this class. This allows the corresopnding TimedHistogram class to
be type safe.

BUG=v8:6361
==========

[kschimpf, 2017-06-08 21:07:09]

kschimpf@chromium.org changed reviewers:
+ cbruni@chromium.org, jochen@chromium.org, mtrofin@chromium.org

[commit-bot: I haz the power, 2017-06-08 21:35:09]

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

[commit-bot: I haz the power, 2017-06-08 21:35:10]

Dry run: This issue passed the CQ dry run.

[kschimpf, 2017-06-08 21:35:19]

Please review. Thanks.

[jochen (gone - plz use gerrit), 2017-06-08 22:15:36]

https://codereview.chromium.org/2929853003/diff/20001/src/wasm/module-compile...
File src/wasm/module-compiler.cc (right):

https://codereview.chromium.org/2929853003/diff/20001/src/wasm/module-compile...
src/wasm/module-compiler.cc:327: ?
isolate_->counters()->wasm_compile_wasm_module_time()
you can't access isolate_ if !is_sync

https://codereview.chromium.org/2929853003/diff/20001/src/wasm/module-decoder.cc
File src/wasm/module-decoder.cc (right):

https://codereview.chromium.org/2929853003/diff/20001/src/wasm/module-decoder...
src/wasm/module-decoder.cc:1229: IsWasm(origin) ?
isolate->counters()->wasm_decode_wasm_module_time()
same here

[kschimpf, 2017-06-09 20:02:42]

The CQ bit was checked by kschimpf@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-09 20:02:49]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kschimpf, 2017-06-09 20:03:37]

This cl is ready to be reviewed (again).

https://codereview.chromium.org/2929853003/diff/20001/src/wasm/module-compile...
File src/wasm/module-compiler.cc (right):

https://codereview.chromium.org/2929853003/diff/20001/src/wasm/module-compile...
src/wasm/module-compiler.cc:327: ?
isolate_->counters()->wasm_compile_wasm_module_time()
On 2017/06/08 22:15:36, jochen wrote:
> you can't access isolate_ if !is_sync

Your right.

https://codereview.chromium.org/2929853003/diff/20001/src/wasm/module-decoder.cc
File src/wasm/module-decoder.cc (right):

https://codereview.chromium.org/2929853003/diff/20001/src/wasm/module-decoder...
src/wasm/module-decoder.cc:1229: IsWasm(origin) ?
isolate->counters()->wasm_decode_wasm_module_time()
On 2017/06/08 22:15:36, jochen wrote:
> same here

Good point. This code was massively reorganized (in the middle of my changes). I
wasn't careful enough to make sure I updated everything in the changes. While
fixing this, I noticed some regressions as well and fixed those also (mostly the
removal of field counters_, since counters_shared_ was sufficient).

https://codereview.chromium.org/2929853003/diff/50001/src/compiler/wasm-compi...
File src/compiler/wasm-compiler.h (right):

https://codereview.chromium.org/2929853003/diff/50001/src/compiler/wasm-compi...
src/compiler/wasm-compiler.h:53: wasm::FunctionBody body, wasm::WasmName name,
int index);
Note: the compilation step is always run on the foreground thread.

https://codereview.chromium.org/2929853003/diff/50001/src/isolate.h
File src/isolate.h (right):

https://codereview.chromium.org/2929853003/diff/50001/src/isolate.h#newcode870
src/isolate.h:870: Counters* counters() {
Added check that this is only called on the foreground thread.

https://codereview.chromium.org/2929853003/diff/50001/src/isolate.h#newcode878
src/isolate.h:878: DCHECK(!IsIsolateInBackground());
Same here.

https://codereview.chromium.org/2929853003/diff/50001/src/wasm/wasm-module.h
File src/wasm/wasm-module.h (right):

https://codereview.chromium.org/2929853003/diff/50001/src/wasm/wasm-module.h#...
src/wasm/wasm-module.h:516: void CompileFunction(Isolate*,
Handle<WasmInstanceObject>, int func_index);
Removed "counters" argument because function compilation always happens on the
foreground thread.

[commit-bot: I haz the power, 2017-06-09 20:04:17]

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

[commit-bot: I haz the power, 2017-06-09 20:04:18]

Dry run: Try jobs failed on following builders:
  v8_android_arm_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...)
  v8_linux64_gcc_compile_dbg on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gcc_compile_dbg/...)
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)
  v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)
  v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/23786)
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/42749)

[kschimpf, 2017-06-09 20:10:38]

Patch error. Remerging with master. Will let you know when merge succeeds.

[kschimpf, 2017-06-09 20:17:05]

The CQ bit was checked by kschimpf@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-09 20:17:12]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-09 20:44:50]

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

[commit-bot: I haz the power, 2017-06-09 20:44:51]

Dry run: This issue passed the CQ dry run.

[kschimpf, 2017-06-09 20:45:02]

Dry run succeeded. Ready for review. Thanks.

[Camillo Bruni, 2017-06-12 08:14:03]

https://codereview.chromium.org/2929853003/diff/90001/src/counters.cc
File src/counters.cc (right):

https://codereview.chromium.org/2929853003/diff/90001/src/counters.cc#newcode86
src/counters.cc:86: if (Enabled()) {
Given that this might be slightly performance critical I'd suggest putting the
ENABLED / FLAG_log_internal_timer_events / isolate check TimedHistogramScope in
counters-inl.h and then call Stop/Start that does the actual work.

I've done the same thing fort the RuntimeCallTimerScope and the actual Counter.

WDYT? (we could also postpone this to a follow-up CL)

https://codereview.chromium.org/2929853003/diff/90001/src/counters.cc#newcode94
src/counters.cc:94: if (isolate != nullptr)
nit: style violation, please add braces.

[jochen (gone - plz use gerrit), 2017-06-12 09:18:34]

https://codereview.chromium.org/2929853003/diff/90001/src/compiler/wasm-compi...
File src/compiler/wasm-compiler.cc (right):

https://codereview.chromium.org/2929853003/diff/90001/src/compiler/wasm-compi...
src/compiler/wasm-compiler.cc:3957:
isolate_->counters()->wasm_compile_function_time());
no isolate_ deref on background threads

https://codereview.chromium.org/2929853003/diff/90001/src/isolate.h
File src/isolate.h (right):

https://codereview.chromium.org/2929853003/diff/90001/src/isolate.h#newcode870
src/isolate.h:870: Counters* counters() {
why not have counters() always return the shared_ptr?

[kschimpf, 2017-06-12 16:16:30]

The CQ bit was checked by kschimpf@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-12 16:16:41]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-12 16:45:29]

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

[commit-bot: I haz the power, 2017-06-12 16:45:30]

Dry run: This issue passed the CQ dry run.

[kschimpf, 2017-06-12 16:47:05]

https://codereview.chromium.org/2929853003/diff/90001/src/compiler/wasm-compi...
File src/compiler/wasm-compiler.cc (right):

https://codereview.chromium.org/2929853003/diff/90001/src/compiler/wasm-compi...
src/compiler/wasm-compiler.cc:3957:
isolate_->counters()->wasm_compile_function_time());
On 2017/06/12 09:18:34, jochen wrote:
> no isolate_ deref on background threads

It turns out that this step is not run in the background. Hence, this is safe.

https://codereview.chromium.org/2929853003/diff/90001/src/counters.cc
File src/counters.cc (right):

https://codereview.chromium.org/2929853003/diff/90001/src/counters.cc#newcode86
src/counters.cc:86: if (Enabled()) {
On 2017/06/12 08:14:03, Camillo Bruni wrote:
> Given that this might be slightly performance critical I'd suggest putting the
> ENABLED / FLAG_log_internal_timer_events / isolate check TimedHistogramScope
in
> counters-inl.h and then call Stop/Start that does the actual work.
> 
> I've done the same thing fort the RuntimeCallTimerScope and the actual
Counter.
> 
> WDYT? (we could also postpone this to a follow-up CL)

This was previously in HistogramTimer as non-inlined code, so how are we adding
more overhead? Is there proof that we should be paying the code size penalty to
get performance?

On a side note, I do agree that the HistogramTimer::Start()/Stop() should be
inlined (to not add two levels of call).

https://codereview.chromium.org/2929853003/diff/90001/src/counters.cc#newcode94
src/counters.cc:94: if (isolate != nullptr)
On 2017/06/12 08:14:03, Camillo Bruni wrote:
> nit: style violation, please add braces.

Done.

https://codereview.chromium.org/2929853003/diff/90001/src/isolate.h
File src/isolate.h (right):

https://codereview.chromium.org/2929853003/diff/90001/src/isolate.h#newcode870
src/isolate.h:870: Counters* counters() {
On 2017/06/12 09:18:34, jochen wrote:
> why not have counters() always return the shared_ptr?

Because too much code accesses it like this:

   Counters* counters = isolate->counter();

and then reference 'counters' multiple times.

I did not want this CL to get (very) large.

[kschimpf, 2017-06-17 14:30:36]

Ping?

[jochen (gone - plz use gerrit), 2017-06-19 13:14:10]

deferring to cbruni@

[kschimpf, 2017-06-20 14:59:57]

Ping? Is anyone looking at this CL?

[Camillo Bruni, 2017-06-22 07:26:14]

lgtm

https://codereview.chromium.org/2929853003/diff/90001/src/counters.cc
File src/counters.cc (right):

https://codereview.chromium.org/2929853003/diff/90001/src/counters.cc#newcode86
src/counters.cc:86: if (Enabled()) {
On 2017/06/12 at 16:47:04, kschimpf wrote:
> On 2017/06/12 08:14:03, Camillo Bruni wrote:
> > Given that this might be slightly performance critical I'd suggest putting
the
> > ENABLED / FLAG_log_internal_timer_events / isolate check TimedHistogramScope
in
> > counters-inl.h and then call Stop/Start that does the actual work.
> > 
> > I've done the same thing fort the RuntimeCallTimerScope and the actual
Counter.
> > 
> > WDYT? (we could also postpone this to a follow-up CL)
> 
> This was previously in HistogramTimer as non-inlined code, so how are we
adding more overhead? Is there proof that we should be paying the code size
penalty to get performance?
> 
> On a side note, I do agree that the HistogramTimer::Start()/Stop() should be
inlined (to not add two levels of call).

SGTM, let's see how this behaves in general first.

[Camillo Bruni, 2017-06-22 10:53:18]

cbruni@chromium.org changed reviewers:
+ ulan@chromium.org

[Camillo Bruni, 2017-06-22 10:53:19]

PTAL ulan@

[kschimpf, 2017-06-22 17:18:00]

The CQ bit was checked by kschimpf@chromium.org

[kschimpf, 2017-06-22 17:18:01]

The patchset sent to the CQ was uploaded after l-g-t-m from cbruni@chromium.org
Link to the patchset: https://codereview.chromium.org/2929853003/#ps130001
(title: "Merge with master")

[commit-bot: I haz the power, 2017-06-22 17:18:03]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-22 17:21:12]

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

[commit-bot: I haz the power, 2017-06-22 17:21:13]

Try jobs failed on following builders:
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/43744)

[kschimpf, 2017-06-22 17:22:21]

The CQ bit was checked by kschimpf@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-22 17:22:32]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-22 17:48:02]

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

[commit-bot: I haz the power, 2017-06-22 17:48:03]

Dry run: This issue passed the CQ dry run.

[Mircea Trofin, 2017-06-22 18:42:24]

https://codereview.chromium.org/2929853003/diff/130001/src/counters.cc
File src/counters.cc (right):

https://codereview.chromium.org/2929853003/diff/130001/src/counters.cc#newcode82
src/counters.cc:82: if (isolate) Logger::CallEventLogger(isolate, name(),
Logger::START, true);
why would it not have an isolate?

also, for consistency, check isolate != nullptr

https://codereview.chromium.org/2929853003/diff/130001/src/counters.cc#newcod...
src/counters.cc:155: TimedHistogram(timer.caption, 0, timer.max, timer.res, 50,
this);
what's "50" - I realize it's done the same above, but could we add a constant to
document what it represents?

https://codereview.chromium.org/2929853003/diff/130001/src/counters.h
File src/counters.h (right):

https://codereview.chromium.org/2929853003/diff/130001/src/counters.h#newcode254
src/counters.h:254: // (non-nested) timed results.
why is non-nested in brackets?

https://codereview.chromium.org/2929853003/diff/130001/src/counters.h#newcode294
src/counters.h:294: // A HistogramTimer allows distributions of (non-nested)
timed results
non-nested in brackets

https://codereview.chromium.org/2929853003/diff/130001/src/counters.h#newcode297
src/counters.h:297: class HistogramTimer : public TimedHistogram {
Is the difference between the two their thread safety? Should the names capture
that, otherwise it's a bit tricky to tell them apart.

Also, is the inheritance relationship necessary?

https://codereview.chromium.org/2929853003/diff/130001/src/isolate.h
File src/isolate.h (right):

https://codereview.chromium.org/2929853003/diff/130001/src/isolate.h#newcode882
src/isolate.h:882: std::shared_ptr<Counters> counters_shared() {
nit: shared_counters?

https://codereview.chromium.org/2929853003/diff/130001/src/wasm/module-compil...
File src/wasm/module-compiler.cc (right):

https://codereview.chromium.org/2929853003/diff/130001/src/wasm/module-compil...
src/wasm/module-compiler.cc:366: module_->is_wasm() ?
counters()->wasm_compile_wasm_module_time()
can this test be encapsulated in a method, it appears in a few places.

https://codereview.chromium.org/2929853003/diff/130001/src/wasm/module-compil...
src/wasm/module-compiler.cc:2039: kWasmOrigin, job_->counters_shared_);
why not the accessor call (job_->counters()) ?

https://codereview.chromium.org/2929853003/diff/130001/src/wasm/module-compil...
File src/wasm/module-compiler.h (right):

https://codereview.chromium.org/2929853003/diff/130001/src/wasm/module-compil...
src/wasm/module-compiler.h:84: Counters* counters() const { return
counters_shared_.get(); }
for consistency, should this have the "shared" word in it? Either that, or we
can drop the "shared" from the field.

[kschimpf, 2017-06-22 20:18:07]

The CQ bit was checked by kschimpf@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-22 20:18:21]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kschimpf, 2017-06-22 20:33:42]

The CQ bit was checked by kschimpf@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-22 20:33:45]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kschimpf, 2017-06-22 20:38:31]

https://codereview.chromium.org/2929853003/diff/130001/src/counters.cc
File src/counters.cc (right):

https://codereview.chromium.org/2929853003/diff/130001/src/counters.cc#newcode82
src/counters.cc:82: if (isolate) Logger::CallEventLogger(isolate, name(),
Logger::START, true);
On 2017/06/22 18:42:23, Mircea Trofin wrote:
> why would it not have an isolate?
> 
> also, for consistency, check isolate != nullptr

The only need for the isolate is for logging, and only that can be done with the
(main thread) isolate.

The previous definition of HistogramTimer always did logging. For thread safety,
we can't do this for collection of background threads.

https://codereview.chromium.org/2929853003/diff/130001/src/counters.cc#newcod...
src/counters.cc:155: TimedHistogram(timer.caption, 0, timer.max, timer.res, 50,
this);
On 2017/06/22 18:42:23, Mircea Trofin wrote:
> what's "50" - I realize it's done the same above, but could we add a constant
to
> document what it represents?

Its the number of buckets. The current macros do not specify a value. I'll make
a symbolic constant for it.

Note: I also fixed the 50 in line 168.

https://codereview.chromium.org/2929853003/diff/130001/src/counters.h
File src/counters.h (right):

https://codereview.chromium.org/2929853003/diff/130001/src/counters.h#newcode254
src/counters.h:254: // (non-nested) timed results.
On 2017/06/22 18:42:24, Mircea Trofin wrote:
> why is non-nested in brackets?

Removing parenthesis and fixing to say nested. This class is intended to be the
timed histogram of choice.

https://codereview.chromium.org/2929853003/diff/130001/src/counters.h#newcode294
src/counters.h:294: // A HistogramTimer allows distributions of (non-nested)
timed results
On 2017/06/22 18:42:23, Mircea Trofin wrote:
> non-nested in brackets

Done.

https://codereview.chromium.org/2929853003/diff/130001/src/counters.h#newcode297
src/counters.h:297: class HistogramTimer : public TimedHistogram {
On 2017/06/22 18:42:23, Mircea Trofin wrote:
> Is the difference between the two their thread safety? Should the names
capture
> that, otherwise it's a bit tricky to tell them apart.
> 
> Also, is the inheritance relationship necessary?

The base class is the more general solution, as it allows both thread safety and
nesting.

There are a lot of current user of a HistogramTimer, and would like to remove
this in a separate CL.

https://codereview.chromium.org/2929853003/diff/130001/src/isolate.h
File src/isolate.h (right):

https://codereview.chromium.org/2929853003/diff/130001/src/isolate.h#newcode882
src/isolate.h:882: std::shared_ptr<Counters> counters_shared() {
On 2017/06/22 18:42:24, Mircea Trofin wrote:
> nit: shared_counters?

Actually, to make clear it's context, I'll rename it to async_counters().

https://codereview.chromium.org/2929853003/diff/130001/src/wasm/module-compil...
File src/wasm/module-compiler.cc (right):

https://codereview.chromium.org/2929853003/diff/130001/src/wasm/module-compil...
src/wasm/module-compiler.cc:366: module_->is_wasm() ?
counters()->wasm_compile_wasm_module_time()
On 2017/06/22 18:42:24, Mircea Trofin wrote:
> can this test be encapsulated in a method, it appears in a few places.

When I tried that in an earlier review, there was quite a bit of push back from
the reviewers. So I left it in its current form.

https://codereview.chromium.org/2929853003/diff/130001/src/wasm/module-compil...
src/wasm/module-compiler.cc:2039: kWasmOrigin, job_->counters_shared_);
On 2017/06/22 18:42:24, Mircea Trofin wrote:
> why not the accessor call (job_->counters()) ?

Because the API expected a shared_ptr<Counters>. However, I changed the API
since this (now) the only calling place.

https://codereview.chromium.org/2929853003/diff/130001/src/wasm/module-compil...
File src/wasm/module-compiler.h (right):

https://codereview.chromium.org/2929853003/diff/130001/src/wasm/module-compil...
src/wasm/module-compiler.h:84: Counters* counters() const { return
counters_shared_.get(); }
On 2017/06/22 18:42:24, Mircea Trofin wrote:
> for consistency, should this have the "shared" word in it? Either that, or we
> can drop the "shared" from the field.

This is an artifact that we used to have both fields "counters_" and
"counters_shared_".

Fixing.

[commit-bot: I haz the power, 2017-06-22 21:00:15]

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

[commit-bot: I haz the power, 2017-06-22 21:00:17]

Dry run: This issue passed the CQ dry run.

[Mircea Trofin, 2017-06-22 22:12:45]

On 2017/06/22 20:38:31, kschimpf wrote:
> https://codereview.chromium.org/2929853003/diff/130001/src/counters.cc
> File src/counters.cc (right):
> 
>
https://codereview.chromium.org/2929853003/diff/130001/src/counters.cc#newcode82
> src/counters.cc:82: if (isolate) Logger::CallEventLogger(isolate, name(),
> Logger::START, true);
> On 2017/06/22 18:42:23, Mircea Trofin wrote:
> > why would it not have an isolate?
> > 
> > also, for consistency, check isolate != nullptr
> 
> The only need for the isolate is for logging, and only that can be done with
the
> (main thread) isolate.
> 
> The previous definition of HistogramTimer always did logging. For thread
safety,
> we can't do this for collection of background threads.
> 
>
https://codereview.chromium.org/2929853003/diff/130001/src/counters.cc#newcod...
> src/counters.cc:155: TimedHistogram(timer.caption, 0, timer.max, timer.res,
50,
> this);
> On 2017/06/22 18:42:23, Mircea Trofin wrote:
> > what's "50" - I realize it's done the same above, but could we add a
constant
> to
> > document what it represents?
> 
> Its the number of buckets. The current macros do not specify a value. I'll
make
> a symbolic constant for it.
> 
> Note: I also fixed the 50 in line 168.
> 
> https://codereview.chromium.org/2929853003/diff/130001/src/counters.h
> File src/counters.h (right):
> 
>
https://codereview.chromium.org/2929853003/diff/130001/src/counters.h#newcode254
> src/counters.h:254: // (non-nested) timed results.
> On 2017/06/22 18:42:24, Mircea Trofin wrote:
> > why is non-nested in brackets?
> 
> Removing parenthesis and fixing to say nested. This class is intended to be
the
> timed histogram of choice.
> 
>
https://codereview.chromium.org/2929853003/diff/130001/src/counters.h#newcode294
> src/counters.h:294: // A HistogramTimer allows distributions of (non-nested)
> timed results
> On 2017/06/22 18:42:23, Mircea Trofin wrote:
> > non-nested in brackets
> 
> Done.
> 
>
https://codereview.chromium.org/2929853003/diff/130001/src/counters.h#newcode297
> src/counters.h:297: class HistogramTimer : public TimedHistogram {
> On 2017/06/22 18:42:23, Mircea Trofin wrote:
> > Is the difference between the two their thread safety? Should the names
> capture
> > that, otherwise it's a bit tricky to tell them apart.
> > 
> > Also, is the inheritance relationship necessary?
> 
> The base class is the more general solution, as it allows both thread safety
and
> nesting.
> 
> There are a lot of current user of a HistogramTimer, and would like to remove
> this in a separate CL.
> 
> https://codereview.chromium.org/2929853003/diff/130001/src/isolate.h
> File src/isolate.h (right):
> 
>
https://codereview.chromium.org/2929853003/diff/130001/src/isolate.h#newcode882
> src/isolate.h:882: std::shared_ptr<Counters> counters_shared() {
> On 2017/06/22 18:42:24, Mircea Trofin wrote:
> > nit: shared_counters?
> 
> Actually, to make clear it's context, I'll rename it to async_counters().
> 
>
https://codereview.chromium.org/2929853003/diff/130001/src/wasm/module-compil...
> File src/wasm/module-compiler.cc (right):
> 
>
https://codereview.chromium.org/2929853003/diff/130001/src/wasm/module-compil...
> src/wasm/module-compiler.cc:366: module_->is_wasm() ?
> counters()->wasm_compile_wasm_module_time()
> On 2017/06/22 18:42:24, Mircea Trofin wrote:
> > can this test be encapsulated in a method, it appears in a few places.
> 
> When I tried that in an earlier review, there was quite a bit of push back
from
> the reviewers. So I left it in its current form.
> 
>
https://codereview.chromium.org/2929853003/diff/130001/src/wasm/module-compil...
> src/wasm/module-compiler.cc:2039: kWasmOrigin, job_->counters_shared_);
> On 2017/06/22 18:42:24, Mircea Trofin wrote:
> > why not the accessor call (job_->counters()) ?
> 
> Because the API expected a shared_ptr<Counters>. However, I changed the API
> since this (now) the only calling place.
> 
>
https://codereview.chromium.org/2929853003/diff/130001/src/wasm/module-compil...
> File src/wasm/module-compiler.h (right):
> 
>
https://codereview.chromium.org/2929853003/diff/130001/src/wasm/module-compil...
> src/wasm/module-compiler.h:84: Counters* counters() const { return
> counters_shared_.get(); }
> On 2017/06/22 18:42:24, Mircea Trofin wrote:
> > for consistency, should this have the "shared" word in it? Either that, or
we
> > can drop the "shared" from the field.
> 
> This is an artifact that we used to have both fields "counters_" and
> "counters_shared_".
> 
> Fixing.

lgtm

[Mircea Trofin, 2017-06-22 22:12:53]

The CQ bit was checked by mtrofin@chromium.org

[Mircea Trofin, 2017-06-22 22:12:53]

The patchset sent to the CQ was uploaded after l-g-t-m from cbruni@chromium.org
Link to the patchset: https://codereview.chromium.org/2929853003/#ps170001
(title: "Make explicit when async_counters is needed.")

[commit-bot: I haz the power, 2017-06-22 22:13:01]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-22 22:14:25]

CQ is committing da patch.

Bot data: {"patchset_id": 170001, "attempt_start_ts": 1498169573436210,
"parent_rev": "696f31ba169299fe092a962248e15e0b37dd7f4f", "commit_rev":
"d4a108078d93a58fb2c296a59328f25980ded3ea"}

[commit-bot: I haz the power, 2017-06-22 22:14:34]

Description was changed from

==========
Fix use of history timers in background threads.

HistoryTimer's can't run in the background because they use a timer
with a simple api of Start() and Stop(). This CL fixes this problem
by building a base class TimedHistogram that doesn't have a timer.

The class HistoryTimer is modified to use this base class so that
uses that run on the foreground thread do not need to be modified.

It also adds a new class TimedHistogramScope that defines the timer
in this class. This allows the corresopnding TimedHistogram class to
be type safe.

BUG=v8:6361
==========

to

==========
Fix use of history timers in background threads.

HistoryTimer's can't run in the background because they use a timer
with a simple api of Start() and Stop(). This CL fixes this problem
by building a base class TimedHistogram that doesn't have a timer.

The class HistoryTimer is modified to use this base class so that
uses that run on the foreground thread do not need to be modified.

It also adds a new class TimedHistogramScope that defines the timer
in this class. This allows the corresopnding TimedHistogram class to
be type safe.

BUG=v8:6361

Review-Url: https://codereview.chromium.org/2929853003
Cr-Commit-Position: refs/heads/master@{#46150}
Committed:
https://chromium.googlesource.com/v8/v8/+/d4a108078d93a58fb2c296a59328f25980d...
==========

[commit-bot: I haz the power, 2017-06-22 22:14:35]

Committed patchset #10 (id:170001) as
https://chromium.googlesource.com/v8/v8/+/d4a108078d93a58fb2c296a59328f25980d...

[kschimpf, 2017-06-23 17:15:19]

Description was changed from

==========
Fix use of history timers in background threads.

HistoryTimer's can't run in the background because they use a timer
with a simple api of Start() and Stop(). This CL fixes this problem
by building a base class TimedHistogram that doesn't have a timer.

The class HistoryTimer is modified to use this base class so that
uses that run on the foreground thread do not need to be modified.

It also adds a new class TimedHistogramScope that defines the timer
in this class. This allows the corresopnding TimedHistogram class to
be type safe.

BUG=v8:6361

Review-Url: https://codereview.chromium.org/2929853003
Cr-Commit-Position: refs/heads/master@{#46150}
Committed:
https://chromium.googlesource.com/v8/v8/+/d4a108078d93a58fb2c296a59328f25980d...
==========

to

==========
Fix use of history timers in background threads.

HistoryTimer's can't run in the background because they use a timer
with a simple api of Start() and Stop(). This CL fixes this problem
by building a base class TimedHistogram that doesn't have a timer.

The class HistoryTimer is modified to use this base class so that
uses that run on the foreground thread do not need to be modified.

It also adds a new class TimedHistogramScope that defines the timer
in this class. This allows the corresopnding TimedHistogram class to
be type safe.

BUG=v8:6361

Review-Url: https://codereview.chromium.org/2929853003
Cr-Commit-Position: refs/heads/master@{#46150}
Committed:
https://chromium.googlesource.com/v8/v8/+/d4a108078d93a58fb2c296a59328f25980d...
==========

[kschimpf, 2017-06-23 17:17:18]

Reopened CL because blink code is (unfortunately) getting counters from the
isolate in the background thread.

Hence, this CL was reverted. Need to remove the DCHECK before resubmitting.

[kschimpf, 2017-07-04 23:48:10]

On 2017/06/23 17:17:18, kschimpf wrote:
> Reopened CL because blink code is (unfortunately) getting counters from the
> isolate in the background thread.
> 
> Hence, this CL was reverted. Need to remove the DCHECK before resubmitting.

Closing.  Changes committed in other CLs.