Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1890773002:
Adding chrome side entries for Asm.js / WebAssembly metrics. (Closed)
|
Created:
4 years, 8 months ago by bradnelson Modified:
3 years, 10 months ago CC:
asvitkine+watch_chromium.org, chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdding chrome side entries for Asm.js / WebAssembly metrics.
BUG=687759
BUG=v8:4203
BUG=575167
TEST=None
R=ahaas@chromium.org,isherman@chromium.org
LOG=N
Review-Url: https://codereview.chromium.org/1890773002
Cr-Commit-Position: refs/heads/master@{#447689}
Committed: https://chromium.googlesource.com/chromium/src/+/f2fb300bb003df54f9e6b8699fbb2d02077bc60c
Patch Set 1 #
Total comments: 10
Patch Set 2 : fix #
Total comments: 2
Patch Set 3 : fix #
Total comments: 4
Patch Set 4 : fix #Messages
Total messages: 22 (10 generated)
bradnelson@google.com changed reviewers: + bradnelson@google.com
Description was changed from ========== Adding chrome side entries for Asm.js / WebAssembly. 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,isherman@chromium.org LOG=N ========== to ========== Adding chrome side entries for Asm.js / WebAssembly metrics. 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,isherman@chromium.org LOG=N ==========
lgtm https://codereview.chromium.org/1890773002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1890773002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:56688: + <summary>Time to compile a WebAssembly function.</summary> Should the summary say that the measurement unit is micro seconds?
For future reference, please include a metrics team reviewer on any CLs that add new histograms, as well as on the histograms.xml review. Often, the most valuable part of the review affects the histogram design itself (or at least the naming or bucket layout). https://codereview.chromium.org/1890773002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1890773002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:56685: +<histogram name="V8.WasmCompileFunctionMicroSeconds"> Please add a units attribute, here and for the other histograms as well. https://codereview.chromium.org/1890773002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:56694: + <summary>Memory used to compile a WebAssembly function.</summary> nit: Please mention "peak" somewhere in this description. Ditto for the other peak memory histograms. https://codereview.chromium.org/1890773002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:56697: +<histogram name="V8.WasmCompileMicroSeconds"> nit: I think it would be better for this histogram name to include "Module" in it. Ditto for the other module-focused histograms that don't include "module" in the name. https://codereview.chromium.org/1890773002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:56742: + <summary>Maximum pages needed by a WebAssembly module.</summary> I don't quite understand what distribution this is the maximum over. Could you please write a bit more detail about what exactly is being computed/recorded here? Likewise for the min histogram.
PTAL, and at the V8 half. https://codereview.chromium.org/1890773002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1890773002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:56685: +<histogram name="V8.WasmCompileFunctionMicroSeconds"> On 2016/04/14 21:21:31, Ilya Sherman wrote: > Please add a units attribute, here and for the other histograms as well. Done. https://codereview.chromium.org/1890773002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:56688: + <summary>Time to compile a WebAssembly function.</summary> On 2016/04/14 08:07:11, ahaas wrote: > Should the summary say that the measurement unit is micro seconds? Done. https://codereview.chromium.org/1890773002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:56694: + <summary>Memory used to compile a WebAssembly function.</summary> On 2016/04/14 21:21:31, Ilya Sherman wrote: > nit: Please mention "peak" somewhere in this description. Ditto for the other > peak memory histograms. Done. https://codereview.chromium.org/1890773002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:56697: +<histogram name="V8.WasmCompileMicroSeconds"> On 2016/04/14 21:21:31, Ilya Sherman wrote: > nit: I think it would be better for this histogram name to include "Module" in > it. Ditto for the other module-focused histograms that don't include "module" > in the name. Revised and also in v8 CL. https://codereview.chromium.org/1890773002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:56742: + <summary>Maximum pages needed by a WebAssembly module.</summary> On 2016/04/14 21:21:31, Ilya Sherman wrote: > I don't quite understand what distribution this is the maximum over. Could you > please write a bit more detail about what exactly is being computed/recorded > here? Likewise for the min histogram. Done.
https://codereview.chromium.org/1890773002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1890773002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:56899: + Maximum number of 64KiB pages needed by a WebAssembly module. Sorry, this is still not very clear to me. Is this value recording the maximum number of pages used by a single module, out of all of the modules (that is, recording how hungry the single hungriest module is)? Or is it recording the maximum number of pages used by a module over that module's lifetime, one record for each module? When exactly is this sampled? I guess my question boils down to: the word "maximum" implies that there's some total space that's being sampled from, and I don't understand what that total space is.
Hi, what's the status of this CL?
We've been fiddling the stats on the other end. I'll revisit it next week. Sorry to leave it lingering.
The CQ bit was checked by bradnelson@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Adding chrome side entries for Asm.js / WebAssembly metrics. 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,isherman@chromium.org LOG=N ========== to ========== Adding chrome side entries for Asm.js / WebAssembly metrics. BUG=687759 BUG=v8:4203 BUG=575167 TEST=None R=ahaas@chromium.org,isherman@chromium.org LOG=N ==========
bradnelson@google.com changed reviewers: + tizer@chromium.org
Hi Ilya. Could you take a look again? (hoping to merge to M57). https://codereview.chromium.org/1890773002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1890773002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:56899: + Maximum number of 64KiB pages needed by a WebAssembly module. On 2016/04/19 23:38:59, Ilya Sherman wrote: > Sorry, this is still not very clear to me. Is this value recording the maximum > number of pages used by a single module, out of all of the modules (that is, > recording how hungry the single hungriest module is)? Or is it recording the > maximum number of pages used by a module over that module's lifetime, one record > for each module? When exactly is this sampled? > > I guess my question boils down to: the word "maximum" implies that there's some > total space that's being sampled from, and I don't understand what that total > space is. Reworded, hopefully clearer.
Metrics LGTM. Note that there's no need to merge histograms.xml to a branch -- we always pull this file from ToT. https://codereview.chromium.org/1890773002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1890773002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:71931: +<histogram name="V8.AsmWasmTranslationMicroSeconds" units="microseconds"> Optional nit: I'd s/MicroSeconds/Duration in the name. The units are already specified elsewhere. https://codereview.chromium.org/1890773002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:71935: + <summary>Time to convert asm.js code to WebAssembly in microseconds.</summary> Optional nit: I'd include a comma before "in microseconds"; or perhaps just omit it altogether, since it's listed in the units attribute.
https://codereview.chromium.org/1890773002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1890773002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:71931: +<histogram name="V8.AsmWasmTranslationMicroSeconds" units="microseconds"> On 2017/02/02 00:28:57, Ilya Sherman wrote: > Optional nit: I'd s/MicroSeconds/Duration in the name. The units are already > specified elsewhere. These mirror several of the others V8 side (and I'd need to cherrypick to get for M57 at this point). Will note for future reference. https://codereview.chromium.org/1890773002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:71935: + <summary>Time to convert asm.js code to WebAssembly in microseconds.</summary> On 2017/02/02 00:28:57, Ilya Sherman wrote: > Optional nit: I'd include a comma before "in microseconds"; or perhaps just omit > it altogether, since it's listed in the units attribute. Done.
The CQ bit was checked by bradnelson@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from ahaas@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/1890773002/#ps60001 (title: "fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1485995940889650,
"parent_rev": "1c96270769554e8cab52297f141afc49ee868bce", "commit_rev":
"f2fb300bb003df54f9e6b8699fbb2d02077bc60c"}
Message was sent while issue was closed.
Description was changed from ========== Adding chrome side entries for Asm.js / WebAssembly metrics. BUG=687759 BUG=v8:4203 BUG=575167 TEST=None R=ahaas@chromium.org,isherman@chromium.org LOG=N ========== to ========== Adding chrome side entries for Asm.js / WebAssembly metrics. BUG=687759 BUG=v8:4203 BUG=575167 TEST=None R=ahaas@chromium.org,isherman@chromium.org LOG=N Review-Url: https://codereview.chromium.org/1890773002 Cr-Commit-Position: refs/heads/master@{#447689} Committed: https://chromium.googlesource.com/chromium/src/+/f2fb300bb003df54f9e6b8699fbb... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/f2fb300bb003df54f9e6b8699fbb... |
