Separate module instantiate counter for asm and wasm.

Separate module instantiate counter for asm and wasm.

Currently, v8 uses the same flag to collect time for instantiating wasm modules
from js asm and wasm. This separates the v8 counter into two separate counters,
and then uses the appropriate counter when instantiating a module.

BUG=chromium:704922
R=aseemgarg@chromium.org,bradnelson@chromium.org,bbudge@chromium.org

Review-Url: https://codereview.chromium.org/2772773004
Cr-Commit-Position: refs/heads/master@{#44122}
Committed: https://chromium.googlesource.com/v8/v8/+/c7ec5bf414023242e03a5118c808844164e86542

[Karl, 2017-03-23 20:38:11]

Description was changed from

==========
Separate module instantiate counter for asm and wasm.

Currently, v8 uses the same flag to collect time for instantiating wasm modules
from js asm and wasm. This separates the v8 counter into two separate counters,
and then uses the appropriate counter when instantiating a module.

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

to

==========
Separate module instantiate counter for asm and wasm.

Currently, v8 uses the same flag to collect time for instantiating wasm modules
from js asm and wasm. This separates the v8 counter into two separate counters,
and then uses the appropriate counter when instantiating a module.

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

[Karl, 2017-03-23 20:38:11]

kschimpf@google.com changed reviewers:
+ bbudge@chromium.org

[Karl, 2017-03-23 20:38:49]

kschimpf@google.com changed reviewers:
+ bradnelson@chromium.org

[Karl, 2017-03-23 20:38:59]

kschimpf@google.com changed reviewers:
+ kschimpf@google.com

[Karl, 2017-03-23 20:42:18]

The CQ bit was checked by kschimpf@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-23 20:42:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Karl, 2017-03-23 20:42:32]

The CQ bit was unchecked by kschimpf@google.com

[Karl, 2017-03-23 20:42:44]

The CQ bit was checked by kschimpf@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-23 20:42:48]

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-03-23 20:46:11]

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

[commit-bot: I haz the power, 2017-03-23 20:46:12]

Dry run: Try jobs failed on following builders:
  v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/24769)
  v8_win_compile_dbg on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_compile_dbg/builds/3...)
  v8_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)

[Karl, 2017-03-23 20:54:59]

The CQ bit was checked by kschimpf@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-23 20:55:10]

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-03-23 21:17:25]

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

[commit-bot: I haz the power, 2017-03-23 21:17:26]

Dry run: This issue passed the CQ dry run.

[aseemgarg, 2017-03-23 21:45:58]

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

[aseemgarg, 2017-03-23 21:45:58]

lgtm

[bbudge, 2017-03-23 22:04:58]

https://codereview.chromium.org/2772773004/diff/20001/src/counters.h
File src/counters.h (right):

https://codereview.chromium.org/2772773004/diff/20001/src/counters.h#newcode961
src/counters.h:961: HT(wasm_instantiate_wasm_module_time,
V8.WasmInstantiateModuleMicroSeconds,  \
It seems strange to me to break out these separate flags, but then use the same
histogram bucket. Perhaps I don't understand?

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

https://codereview.chromium.org/2772773004/diff/20001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:1165: }
Why not return BuildInstance() here? Or why not just inline that code like it
was before? It would make the patch a lot easier to review :)

[bbudge, 2017-03-23 22:14:48]

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

https://codereview.chromium.org/2772773004/diff/20001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:1165: }
On 2017/03/23 22:04:58, bbudge wrote:
> Why not return BuildInstance() here? Or why not just inline that code like it
> was before? It would make the patch a lot easier to review :)

I didn't see the scope objects before. You could construct a single scope object
using asm/wasm condition to route the right counter to it.

[Karl, 2017-03-23 22:39:35]

The CQ bit was checked by kschimpf@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-23 22:39:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bbudge, 2017-03-23 22:43:59]

https://codereview.chromium.org/2772773004/diff/40001/src/wasm/wasm-module.cc
File src/wasm/wasm-module.cc (right):

https://codereview.chromium.org/2772773004/diff/40001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:1161: return BuildInstance();
Now that there's only a single call, if you move the body of BuildInstance back
here, the patch will be much smaller. Or do you anticipate multiple callers in
the future?

[commit-bot: I haz the power, 2017-03-23 23:02:36]

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

[commit-bot: I haz the power, 2017-03-23 23:02:37]

Dry run: This issue passed the CQ dry run.

[Karl, 2017-03-24 14:44:56]

Description was changed from

==========
Separate module instantiate counter for asm and wasm.

Currently, v8 uses the same flag to collect time for instantiating wasm modules
from js asm and wasm. This separates the v8 counter into two separate counters,
and then uses the appropriate counter when instantiating a module.

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

to

==========
Separate module instantiate counter for asm and wasm.

Currently, v8 uses the same flag to collect time for instantiating wasm modules
from js asm and wasm. This separates the v8 counter into two separate counters,
and then uses the appropriate counter when instantiating a module.

BUG=https://bugs.chromium.org/p/chromium/issues/detail?id=704922
==========

[Karl, 2017-03-24 15:42:02]

Description was changed from

==========
Separate module instantiate counter for asm and wasm.

Currently, v8 uses the same flag to collect time for instantiating wasm modules
from js asm and wasm. This separates the v8 counter into two separate counters,
and then uses the appropriate counter when instantiating a module.

BUG=https://bugs.chromium.org/p/chromium/issues/detail?id=704922
==========

to

==========
Separate module instantiate counter for asm and wasm.

Currently, v8 uses the same flag to collect time for instantiating wasm modules
from js asm and wasm. This separates the v8 counter into two separate counters,
and then uses the appropriate counter when instantiating a module.

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

[Karl, 2017-03-24 15:53:49]

Description was changed from

==========
Separate module instantiate counter for asm and wasm.

Currently, v8 uses the same flag to collect time for instantiating wasm modules
from js asm and wasm. This separates the v8 counter into two separate counters,
and then uses the appropriate counter when instantiating a module.

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

to

==========
Separate module instantiate counter for asm and wasm.

Currently, v8 uses the same flag to collect time for instantiating wasm modules
from js asm and wasm. This separates the v8 counter into two separate counters,
and then uses the appropriate counter when instantiating a module.

BUG= https://bugs.chromium.org/p/chromium/issues/detail?id=704922
==========

[kschimpf, 2017-03-24 15:54:08]

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

[commit-bot: I haz the power, 2017-03-24 15:54:18]

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-03-24 15:56:48]

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

[commit-bot: I haz the power, 2017-03-24 15:56:49]

Dry run: 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/37614)

[Karl, 2017-03-24 16:49:19]

https://codereview.chromium.org/2772773004/diff/20001/src/counters.h
File src/counters.h (right):

https://codereview.chromium.org/2772773004/diff/20001/src/counters.h#newcode961
src/counters.h:961: HT(wasm_instantiate_wasm_module_time,
V8.WasmInstantiateModuleMicroSeconds,  \
On 2017/03/23 22:04:58, bbudge wrote:
> It seems strange to me to break out these separate flags, but then use the
same
> histogram bucket. Perhaps I don't understand?

Since the V8.flags are from Chrome, I am trying to put off those changes and
update them all at once.

This way, I can split up many of these counters (based on asm/wasm origin) as
separate CLs without having to fight each one through a chrome review.

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

https://codereview.chromium.org/2772773004/diff/20001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:1165: }
On 2017/03/23 22:14:48, bbudge wrote:
> On 2017/03/23 22:04:58, bbudge wrote:
> > Why not return BuildInstance() here? Or why not just inline that code like
it
> > was before? It would make the patch a lot easier to review :)
> 
> I didn't see the scope objects before. You could construct a single scope
object
> using asm/wasm condition to route the right counter to it.

I factored out the rest of the code, as you note, to take advantage of the scope
object. However, you are also right that I could simplify the code by moving the
conditional into the argument passed to the scope.

Fixing.

https://codereview.chromium.org/2772773004/diff/40001/src/wasm/wasm-module.cc
File src/wasm/wasm-module.cc (right):

https://codereview.chromium.org/2772773004/diff/40001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:1161: return BuildInstance();
On 2017/03/23 22:43:58, bbudge wrote:
> Now that there's only a single call, if you move the body of BuildInstance
back
> here, the patch will be much smaller. Or do you anticipate multiple callers in
> the future?

Done.

[bradnelson, 2017-03-24 17:25:10]

https://codereview.chromium.org/2772773004/diff/60001/src/counters.h
File src/counters.h (right):

https://codereview.chromium.org/2772773004/diff/60001/src/counters.h#newcode959
src/counters.h:959: HT(wasm_instantiate_asm_module_time,
V8.WasmInstantiateModuleMicroSeconds,   \
You need to make up separate names for the chrome side part.
Maybe:
V8.AsmWasmInstantiateModuleMicroSeconds

https://codereview.chromium.org/2772773004/diff/60001/src/wasm/wasm-module.cc
File src/wasm/wasm-module.cc (right):

https://codereview.chromium.org/2772773004/diff/60001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:1158: module_->origin == ModuleOrigin::kWasmOrigin
Ah, good idea.

[bradnelson, 2017-03-24 17:30:23]

Description was changed from

==========
Separate module instantiate counter for asm and wasm.

Currently, v8 uses the same flag to collect time for instantiating wasm modules
from js asm and wasm. This separates the v8 counter into two separate counters,
and then uses the appropriate counter when instantiating a module.

BUG= https://bugs.chromium.org/p/chromium/issues/detail?id=704922
==========

to

==========
Separate module instantiate counter for asm and wasm.

Currently, v8 uses the same flag to collect time for instantiating wasm modules
from js asm and wasm. This separates the v8 counter into two separate counters,
and then uses the appropriate counter when instantiating a module.

BUG=chromium:704922
==========

[bradnelson, 2017-03-24 17:31:12]

Description was changed from

==========
Separate module instantiate counter for asm and wasm.

Currently, v8 uses the same flag to collect time for instantiating wasm modules
from js asm and wasm. This separates the v8 counter into two separate counters,
and then uses the appropriate counter when instantiating a module.

BUG=chromium:704922
==========

to

==========
Separate module instantiate counter for asm and wasm.

Currently, v8 uses the same flag to collect time for instantiating wasm modules
from js asm and wasm. This separates the v8 counter into two separate counters,
and then uses the appropriate counter when instantiating a module.

BUG=chromium:704922
R=aseemgarg@chromium.org,bradnelson@chromium.org,bbudge@chromium.org
==========

[bradnelson, 2017-03-24 17:33:14]

lgtm

https://codereview.chromium.org/2772773004/diff/60001/src/counters.h
File src/counters.h (right):

https://codereview.chromium.org/2772773004/diff/60001/src/counters.h#newcode959
src/counters.h:959: HT(wasm_instantiate_asm_module_time,
V8.WasmInstantiateModuleMicroSeconds,   \
On 2017/03/24 17:25:10, bradnelson wrote:
> You need to make up separate names for the chrome side part.
> Maybe:
> V8.AsmWasmInstantiateModuleMicroSeconds

Sounds like these are safe to direct to the same bucket until we're ready to
switch everything over in one step right before commiting to chromium.

[bradnelson, 2017-03-24 17:34:07]

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

[commit-bot: I haz the power, 2017-03-24 17:34:09]

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-03-24 17:35:37]

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

[commit-bot: I haz the power, 2017-03-24 17:35:38]

Dry run: This issue passed the CQ dry run.

[bbudge, 2017-03-24 18:39:21]

Thanks Karl, lgtm

[Karl, 2017-03-24 18:45:08]

The CQ bit was checked by kschimpf@google.com

[Karl, 2017-03-24 18:45:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
aseemgarg@chromium.org
Link to the patchset: https://codereview.chromium.org/2772773004/#ps60001
(title: "Simplify code back to previous form")

[commit-bot: I haz the power, 2017-03-24 18:45:18]

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-03-24 18:46:56]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1490381108807180,
"parent_rev": "c72c90bc7422e6a594604f00deb0d6c5d562e478", "commit_rev":
"c7ec5bf414023242e03a5118c808844164e86542"}

[commit-bot: I haz the power, 2017-03-24 18:47:02]

Description was changed from

==========
Separate module instantiate counter for asm and wasm.

Currently, v8 uses the same flag to collect time for instantiating wasm modules
from js asm and wasm. This separates the v8 counter into two separate counters,
and then uses the appropriate counter when instantiating a module.

BUG=chromium:704922
R=aseemgarg@chromium.org,bradnelson@chromium.org,bbudge@chromium.org
==========

to

==========
Separate module instantiate counter for asm and wasm.

Currently, v8 uses the same flag to collect time for instantiating wasm modules
from js asm and wasm. This separates the v8 counter into two separate counters,
and then uses the appropriate counter when instantiating a module.

BUG=chromium:704922
R=aseemgarg@chromium.org,bradnelson@chromium.org,bbudge@chromium.org

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

[commit-bot: I haz the power, 2017-03-24 18:47:03]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/v8/v8/+/c7ec5bf414023242e03a5118c808844164e...