[wasm] Adding metrics for Asm/Wasm.

[wasm] Adding metrics for Asm/Wasm.

Collecting:
Time histograms for: instantiate, decode module, decode function, compile, compile function.
Memory histograms for: decode peak, instantiate peak, compile function peak, min mem pages, max mem pages, function bytes, module bytes.
Range histograms of: functions per module.

BUG= https://code.google.com/p/v8/issues/detail?id=4203
BUG= https://bugs.chromium.org/p/chromium/issues/detail?id=575167
TEST=None
R=ahaas@chromium.org,bmeurer@chromium.org
LOG=N

Committed: https://crrev.com/53bc649e8862935dc31acf88015d91fb1e2995e0
Cr-Commit-Position: refs/heads/master@{#35467}

[bradn, 2016-04-08 19:34:19]

Description was changed from

==========
Adding metrics for Asm/Wasm.

BUG=None
TEST=None
R=ahaas@chromium.org
LOG=N
==========

to

==========
[wasm] Adding metrics for Asm/Wasm.

Collecting:
Time histograms for: instantiate, decode module, decode function, compile,
compile function.
Memory histograms for: decode peak, instantiate peak, compile function peak, min
mem pages, max mem pages, function bytes, module bytes.
Range histograms of: functions per module.

BUG= https://code.google.com/p/v8/issues/detail?id=4203
BUG= https://bugs.chromium.org/p/chromium/issues/detail?id=575167
TEST=None
R=ahaas@chromium.org,bmeurer@chromium.org
LOG=N
==========

[bradn, 2016-04-08 19:34:19]

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

[bradn, 2016-04-08 19:36:56]

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

[ahaas, 2016-04-11 08:03:08]

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

https://codereview.chromium.org/1866873002/diff/60001/src/wasm/wasm-js.cc#new...
src/wasm/wasm-js.cc:199: static_cast<int>(zone.allocation_size()));
What exactly do you want to measure with this counter? When I first looked at
this code I thought this counter measures the peak memory consumption for the
whole module instantiation. However, the zone you measure is only used to decode
a wasm module, and there are also other zones in use to instantiate a module.
For example, there exists a zone to construct the graph in
wasm-compiler.cc::CompileWasmFunction, and there exists a whole zone pool in
pipeline.cc::GenerateCodeForTesting to compile the graph.

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

https://codereview.chromium.org/1866873002/diff/60001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:559: if (this->start_function_index >= 0) {
Is it intended that the execution of the start function is part of the
compilation time?

[bradn, 2016-04-13 00:22:45]

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

https://codereview.chromium.org/1866873002/diff/60001/src/wasm/wasm-js.cc#new...
src/wasm/wasm-js.cc:199: static_cast<int>(zone.allocation_size()));
On 2016/04/11 08:03:08, ahaas wrote:
> What exactly do you want to measure with this counter? When I first looked at
> this code I thought this counter measures the peak memory consumption for the
> whole module instantiation. However, the zone you measure is only used to
decode
> a wasm module, and there are also other zones in use to instantiate a module.
> For example, there exists a zone to construct the graph in
> wasm-compiler.cc::CompileWasmFunction, and there exists a whole zone pool in
> pipeline.cc::GenerateCodeForTesting to compile the graph.

Erg, yeah this one doesn't make much sense.
Ideally I might aggregate them, but more important to get some measurements in
before the branch.
Dropping this one.

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

https://codereview.chromium.org/1866873002/diff/60001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:559: if (this->start_function_index >= 0) {
On 2016/04/11 08:03:08, ahaas wrote:
> Is it intended that the execution of the start function is part of the
> compilation time?

Oops, done.
I've embedded it in a block, as I'd like to avoid a big refactor in this change
to allow it to be cherry picked.

[ahaas, 2016-04-13 14:53:38]

On 2016/04/13 at 00:22:45, bradnelson wrote:
> https://codereview.chromium.org/1866873002/diff/60001/src/wasm/wasm-js.cc
> File src/wasm/wasm-js.cc (right):
> 
>
https://codereview.chromium.org/1866873002/diff/60001/src/wasm/wasm-js.cc#new...
> src/wasm/wasm-js.cc:199: static_cast<int>(zone.allocation_size()));
> On 2016/04/11 08:03:08, ahaas wrote:
> > What exactly do you want to measure with this counter? When I first looked
at
> > this code I thought this counter measures the peak memory consumption for
the
> > whole module instantiation. However, the zone you measure is only used to
decode
> > a wasm module, and there are also other zones in use to instantiate a
module.
> > For example, there exists a zone to construct the graph in
> > wasm-compiler.cc::CompileWasmFunction, and there exists a whole zone pool in
> > pipeline.cc::GenerateCodeForTesting to compile the graph.
> 
> Erg, yeah this one doesn't make much sense.
> Ideally I might aggregate them, but more important to get some measurements in
before the branch.
> Dropping this one.
> 
> https://codereview.chromium.org/1866873002/diff/60001/src/wasm/wasm-module.cc
> File src/wasm/wasm-module.cc (right):
> 
>
https://codereview.chromium.org/1866873002/diff/60001/src/wasm/wasm-module.cc...
> src/wasm/wasm-module.cc:559: if (this->start_function_index >= 0) {
> On 2016/04/11 08:03:08, ahaas wrote:
> > Is it intended that the execution of the start function is part of the
> > compilation time?
> 
> Oops, done.
> I've embedded it in a block, as I'd like to avoid a big refactor in this
change to allow it to be cherry picked.

lgtm

[bradn, 2016-04-13 14:56:20]

The CQ bit was checked by bradnelson@google.com

[commit-bot: I haz the power, 2016-04-13 14:56:29]

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

[commit-bot: I haz the power, 2016-04-13 14:59:59]

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

[commit-bot: I haz the power, 2016-04-13 15:00:00]

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

[Mircea Trofin, 2016-04-13 17:51:24]

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

[Mircea Trofin, 2016-04-13 17:51:25]

LGTM for the compiler part, with nits + a comment.

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

https://codereview.chromium.org/1866873002/diff/80001/src/compiler/wasm-compi...
src/compiler/wasm-compiler.cc:2785:
isolate->counters()->wasm_compile_function_peak_memory()->AddSample(
I'm a bit concerned with the downcast here from size_t to int, given the value
represents allocated memory size.

https://codereview.chromium.org/1866873002/diff/80001/src/counters.h
File src/counters.h (right):

https://codereview.chromium.org/1866873002/diff/80001/src/counters.h#newcode673
src/counters.h:673: HM(wasm_function_bytes, V8.WasmFunctionBytes)               
           \
Nit: wasm_{min|max}_mem_pages_memory has the word "memory" twice (once
abbreviated, true). wasm_function_bytes isn't the bytes of the function, but its
size in bytes, I assume.

How about:
wasm_{decode|compile}_function_peak_memory_bytes
wasm_{min|max}_mem_pages_count (if we just count pages)
wasm_function_size_bytes

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

https://codereview.chromium.org/1866873002/diff/80001/src/wasm/module-decoder...
src/wasm/module-decoder.cc:781: static_cast<int>(zone->allocation_size() -
decode_memory_start));
Same concern with the loss of precision I expressed elsewhere.

[bradnelson, 2016-04-14 07:05:22]

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

https://codereview.chromium.org/1866873002/diff/80001/src/compiler/wasm-compi...
src/compiler/wasm-compiler.cc:2785:
isolate->counters()->wasm_compile_function_peak_memory()->AddSample(
On 2016/04/13 17:51:25, Mircea Trofin wrote:
> I'm a bit concerned with the downcast here from size_t to int, given the value
> represents allocated memory size.

Agreed, but as above, the static_casts for these functions are a common pattern
with these histograms.
Added a TODO.

https://codereview.chromium.org/1866873002/diff/80001/src/counters.h
File src/counters.h (right):

https://codereview.chromium.org/1866873002/diff/80001/src/counters.h#newcode673
src/counters.h:673: HM(wasm_function_bytes, V8.WasmFunctionBytes)               
           \
On 2016/04/13 17:51:25, Mircea Trofin wrote:
> Nit: wasm_{min|max}_mem_pages_memory has the word "memory" twice (once
> abbreviated, true). wasm_function_bytes isn't the bytes of the function, but
its
> size in bytes, I assume.
> 
> How about:
> wasm_{decode|compile}_function_peak_memory_bytes
> wasm_{min|max}_mem_pages_count (if we just count pages)
> wasm_function_size_bytes

Changed, although leaving wasm_decode_function_peak_memory_bytes, as we share a
single
zone for decoding (no memory allocation in decoding function bodies).

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

https://codereview.chromium.org/1866873002/diff/80001/src/wasm/module-decoder...
src/wasm/module-decoder.cc:781: static_cast<int>(zone->allocation_size() -
decode_memory_start));
On 2016/04/13 17:51:25, Mircea Trofin wrote:
> Same concern with the loss of precision I expressed elsewhere.

Added TODO.

[bradnelson, 2016-04-14 07:10:04]

The CQ bit was checked by bradnelson@chromium.org

[bradnelson, 2016-04-14 07:10:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
mtrofin@chromium.org, ahaas@chromium.org
Link to the patchset: https://codereview.chromium.org/1866873002/#ps140001
(title: "fix")

[commit-bot: I haz the power, 2016-04-14 07:10:08]

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

[commit-bot: I haz the power, 2016-04-14 07:28:26]

Description was changed from

==========
[wasm] Adding metrics for Asm/Wasm.

Collecting:
Time histograms for: instantiate, decode module, decode function, compile,
compile function.
Memory histograms for: decode peak, instantiate peak, compile function peak, min
mem pages, max mem pages, function bytes, module bytes.
Range histograms of: functions per module.

BUG= https://code.google.com/p/v8/issues/detail?id=4203
BUG= https://bugs.chromium.org/p/chromium/issues/detail?id=575167
TEST=None
R=ahaas@chromium.org,bmeurer@chromium.org
LOG=N
==========

to

==========
[wasm] Adding metrics for Asm/Wasm.

Collecting:
Time histograms for: instantiate, decode module, decode function, compile,
compile function.
Memory histograms for: decode peak, instantiate peak, compile function peak, min
mem pages, max mem pages, function bytes, module bytes.
Range histograms of: functions per module.

BUG= https://code.google.com/p/v8/issues/detail?id=4203
BUG= https://bugs.chromium.org/p/chromium/issues/detail?id=575167
TEST=None
R=ahaas@chromium.org,bmeurer@chromium.org
LOG=N
==========

[commit-bot: I haz the power, 2016-04-14 07:28:27]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-04-14 07:30:25]

Description was changed from

==========
[wasm] Adding metrics for Asm/Wasm.

Collecting:
Time histograms for: instantiate, decode module, decode function, compile,
compile function.
Memory histograms for: decode peak, instantiate peak, compile function peak, min
mem pages, max mem pages, function bytes, module bytes.
Range histograms of: functions per module.

BUG= https://code.google.com/p/v8/issues/detail?id=4203
BUG= https://bugs.chromium.org/p/chromium/issues/detail?id=575167
TEST=None
R=ahaas@chromium.org,bmeurer@chromium.org
LOG=N
==========

to

==========
[wasm] Adding metrics for Asm/Wasm.

Collecting:
Time histograms for: instantiate, decode module, decode function, compile,
compile function.
Memory histograms for: decode peak, instantiate peak, compile function peak, min
mem pages, max mem pages, function bytes, module bytes.
Range histograms of: functions per module.

BUG= https://code.google.com/p/v8/issues/detail?id=4203
BUG= https://bugs.chromium.org/p/chromium/issues/detail?id=575167
TEST=None
R=ahaas@chromium.org,bmeurer@chromium.org
LOG=N

Committed: https://crrev.com/53bc649e8862935dc31acf88015d91fb1e2995e0
Cr-Commit-Position: refs/heads/master@{#35467}
==========

[commit-bot: I haz the power, 2016-04-14 07:30:26]

Patchset 8 (id:??) landed as
https://crrev.com/53bc649e8862935dc31acf88015d91fb1e2995e0
Cr-Commit-Position: refs/heads/master@{#35467}

[Ilya Sherman, 2016-04-14 21:20:10]

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

[Ilya Sherman, 2016-04-14 21:20:11]

https://codereview.chromium.org/1866873002/diff/140001/src/counters.h
File src/counters.h (right):

https://codereview.chromium.org/1866873002/diff/140001/src/counters.h#newcode593
src/counters.h:593: HR(wasm_functions_per_module, V8.WasmFunctionsPerModule, 0,
10000, 101)
Hmm, do you really need 100 buckets, or would 50 suffice?  (For some context, 50
buckets tends to suffice for most users, and uses half the resources that 100
buckets would.)

[bradn, 2016-04-19 04:14:18]

No particular reason. Cargo cult from the ones above.
Sorry about that. Should have checked with you guys first.
Sending a CL to reduce.