Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1869423002:
[Interpreter] Add visualization tool for Ignition dispatch counters. (Closed)
|
Created:
4 years, 8 months ago by Stefano Sanfilippo Modified:
4 years, 8 months ago Reviewers:
rmcilroy CC:
v8-reviews_googlegroups.com, oth, rmcilroy, Michael Hablich Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[Interpreter] Add visualization tool for Ignition dispatch counters.
A new script, bytecode_dispatches_report.py reads the table produced
when FLAG_trace_ignition_dispatches is enabled and produces either a
report of top source-destination pairs, or a heatmap that can be
viewed interactively and saved to file.
BUG=v8:4899
LOG=N
Committed: https://crrev.com/b4df8dab45bee9fd2f44ddfb3dbd20da03cefc56
Cr-Commit-Position: refs/heads/master@{#35438}
Patch Set 1 : #Patch Set 2 : Better name for variable. #
Total comments: 14
Patch Set 3 : Use matrix only for printing, renamings, change default behaviour, longer description. #
Total comments: 10
Patch Set 4 : Review. #Patch Set 5 : Typo fix. #Patch Set 6 : Extracting code to test, adding unit tests. #
Total comments: 3
Dependent Patchsets: Messages
Total messages: 26 (13 generated)
Patchset #1 (id:1) has been deleted
ssanfilippo@chromium.org changed reviewers: + rmcilroy@chromium.org
Here's the followup to https://codereview.chromium.org/1828633003/ with the viz script. PTAL.
Description was changed from ========== [Intepreter] Add Ignition dispatch counters visualization tool. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] Add Ignition dispatch counters visualization tool. BUG=v8:4280 LOG=N ==========
https://codereview.chromium.org/1869423002/diff/40001/tools/ignition/plot_byt... File tools/ignition/plot_bytecode_dispatches.py (right): https://codereview.chromium.org/1869423002/diff/40001/tools/ignition/plot_byt... tools/ignition/plot_bytecode_dispatches.py:13: import numpy as np nit - don't use "as" in imports here, I think it just makes the code more difficult to understand. https://codereview.chromium.org/1869423002/diff/40001/tools/ignition/plot_byt... tools/ignition/plot_bytecode_dispatches.py:19: Can you add a couple of unittests (not for the matplot stuff obviously, but for parsing the data and getting top dispatches). https://codereview.chromium.org/1869423002/diff/40001/tools/ignition/plot_byt... tools/ignition/plot_bytecode_dispatches.py:36: def check_saturated_counters(labels, counters_matrix): warn_if_counters_are_saturated https://codereview.chromium.org/1869423002/diff/40001/tools/ignition/plot_byt... tools/ignition/plot_bytecode_dispatches.py:54: top_counters = heapq.nlargest( As discussed offline, I think this tool would be much simpler if you kept the results in the JSON (nested dictionary) form and only moved to a matrix when you are printing the plot. Could you have another stab at this please. https://codereview.chromium.org/1869423002/diff/40001/tools/ignition/plot_byt... tools/ignition/plot_bytecode_dispatches.py:108: description="Plot Ignition counters file.", Could you add more description of what this tool is and how to use it (like the one in linux_perf_report.py). https://codereview.chromium.org/1869423002/diff/40001/tools/ignition/plot_byt... tools/ignition/plot_bytecode_dispatches.py:122: "--print_top", "-t", I think you need to rename the file since this isn't just about plotting a file. How about bytecode_dispatch_report.py https://codereview.chromium.org/1869423002/diff/40001/tools/ignition/plot_byt... tools/ignition/plot_bytecode_dispatches.py:127: ) You should probably have another option for doing the plot, and make the default operation of this script to print the top 10 dispatches without doing a plot (since that's probably more immediately useful to implementers).
https://codereview.chromium.org/1869423002/diff/40001/tools/ignition/plot_byt... File tools/ignition/plot_bytecode_dispatches.py (right): https://codereview.chromium.org/1869423002/diff/40001/tools/ignition/plot_byt... tools/ignition/plot_bytecode_dispatches.py:13: import numpy as np On 2016/04/11 16:35:16, rmcilroy wrote: > nit - don't use "as" in imports here, I think it just makes the code more > difficult to understand. Done. https://codereview.chromium.org/1869423002/diff/40001/tools/ignition/plot_byt... tools/ignition/plot_bytecode_dispatches.py:19: On 2016/04/11 16:35:16, rmcilroy wrote: > Can you add a couple of unittests (not for the matplot stuff obviously, but for > parsing the data and getting top dispatches). The two functions that we should test are: * warn_if_counter_may_have_saturated * print_top_counters However, both of them print something to screen, without returning anything. In order to make the two unit testable, we could return a list to print externally or, similarly, turn the function into a generator. We could also pass a file stream to print to as a function parameter, then use a StringIO object to retrieve the output as string while testing. Optionally, we might want to test the logic in the prologue of plot_dispatches_table, when we turn the dictionary of dictionaries into a matrix. In that case, the cleanest solution is splitting that part from the actual plotting commands. https://codereview.chromium.org/1869423002/diff/40001/tools/ignition/plot_byt... tools/ignition/plot_bytecode_dispatches.py:36: def check_saturated_counters(labels, counters_matrix): On 2016/04/11 16:35:16, rmcilroy wrote: > warn_if_counters_are_saturated Done. https://codereview.chromium.org/1869423002/diff/40001/tools/ignition/plot_byt... tools/ignition/plot_bytecode_dispatches.py:54: top_counters = heapq.nlargest( On 2016/04/11 16:35:16, rmcilroy wrote: > As discussed offline, I think this tool would be much simpler if you kept the > results in the JSON (nested dictionary) form and only moved to a matrix when you > are printing the plot. Could you have another stab at this please. Done. https://codereview.chromium.org/1869423002/diff/40001/tools/ignition/plot_byt... tools/ignition/plot_bytecode_dispatches.py:108: description="Plot Ignition counters file.", On 2016/04/11 16:35:16, rmcilroy wrote: > Could you add more description of what this tool is and how to use it (like the > one in linux_perf_report.py). Done. https://codereview.chromium.org/1869423002/diff/40001/tools/ignition/plot_byt... tools/ignition/plot_bytecode_dispatches.py:122: "--print_top", "-t", On 2016/04/11 16:35:16, rmcilroy wrote: > I think you need to rename the file since this isn't just about plotting a file. > How about bytecode_dispatch_report.py Done. https://codereview.chromium.org/1869423002/diff/40001/tools/ignition/plot_byt... tools/ignition/plot_bytecode_dispatches.py:127: ) On 2016/04/11 16:35:16, rmcilroy wrote: > You should probably have another option for doing the plot, and make the default > operation of this script to print the top 10 dispatches without doing a plot > (since that's probably more immediately useful to implementers). Done.
LGTM with comments, thanks. https://codereview.chromium.org/1869423002/diff/60001/tools/ignition/bytecode... File tools/ignition/bytecode_dispatches_report.py (right): https://codereview.chromium.org/1869423002/diff/60001/tools/ignition/bytecode... tools/ignition/bytecode_dispatches_report.py:34: $ tools/ignition/bytecode_dispatches_report.py -t 0 -p -o data.svg Please update this example if you take the comment below of printing of the top X if --plot is not set. https://codereview.chromium.org/1869423002/diff/60001/tools/ignition/bytecode... tools/ignition/bytecode_dispatches_report.py:46: __COUNTER_MAX = 2**__COUNTER_BITS - 1 nit - remove newlines between __COUNTER_BITS and __COUNTER_MAX https://codereview.chromium.org/1869423002/diff/60001/tools/ignition/bytecode... tools/ignition/bytecode_dispatches_report.py:80: # Reverse y axis for a nicer appeareance appearance https://codereview.chromium.org/1869423002/diff/60001/tools/ignition/bytecode... tools/ignition/bytecode_dispatches_report.py:140: "--print_top", "-t", name "top_count" https://codereview.chromium.org/1869423002/diff/60001/tools/ignition/bytecode... tools/ignition/bytecode_dispatches_report.py:172: if program_options.print_top > 0: This is default 10, so the only way to avoid it is to explicitly set it to '0'. How about we only either print the top X or do the plot (i.e., only do plot if the --plot flag is set).
https://codereview.chromium.org/1869423002/diff/60001/tools/ignition/bytecode... File tools/ignition/bytecode_dispatches_report.py (right): https://codereview.chromium.org/1869423002/diff/60001/tools/ignition/bytecode... tools/ignition/bytecode_dispatches_report.py:34: $ tools/ignition/bytecode_dispatches_report.py -t 0 -p -o data.svg On 2016/04/12 14:08:51, rmcilroy wrote: > Please update this example if you take the comment below of printing of the top > X if --plot is not set. Done. https://codereview.chromium.org/1869423002/diff/60001/tools/ignition/bytecode... tools/ignition/bytecode_dispatches_report.py:46: __COUNTER_MAX = 2**__COUNTER_BITS - 1 On 2016/04/12 14:08:51, rmcilroy wrote: > nit - remove newlines between __COUNTER_BITS and __COUNTER_MAX Done. https://codereview.chromium.org/1869423002/diff/60001/tools/ignition/bytecode... tools/ignition/bytecode_dispatches_report.py:80: # Reverse y axis for a nicer appeareance On 2016/04/12 14:08:51, rmcilroy wrote: > appearance Done. https://codereview.chromium.org/1869423002/diff/60001/tools/ignition/bytecode... tools/ignition/bytecode_dispatches_report.py:140: "--print_top", "-t", On 2016/04/12 14:08:51, rmcilroy wrote: > name "top_count" Done. https://codereview.chromium.org/1869423002/diff/60001/tools/ignition/bytecode... tools/ignition/bytecode_dispatches_report.py:172: if program_options.print_top > 0: On 2016/04/12 14:08:51, rmcilroy wrote: > This is default 10, so the only way to avoid it is to explicitly set it to '0'. > How about we only either print the top X or do the plot (i.e., only do plot if > the --plot flag is set). Done.
LGTM, but please update description / title. https://codereview.chromium.org/1869423002/diff/120001/tools/ignition/bytecod... File tools/ignition/bytecode_dispatches_report_test.py (right): https://codereview.chromium.org/1869423002/diff/120001/tools/ignition/bytecod... tools/ignition/bytecode_dispatches_report_test.py:21: two newlines
https://codereview.chromium.org/1869423002/diff/120001/tools/ignition/bytecod... File tools/ignition/bytecode_dispatches_report_test.py (right): https://codereview.chromium.org/1869423002/diff/120001/tools/ignition/bytecod... tools/ignition/bytecode_dispatches_report_test.py:21: On 2016/04/12 16:25:52, rmcilroy wrote: > two newlines These are methods of the test class, am I wrong in saying that it should be one blank line between them?
Description was changed from ========== [Interpreter] Add Ignition dispatch counters visualization tool. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] Add Ignition dispatch counters visualization tool. bytecode_dispatches_report.py reads the table produced when FLAG_trace_ignition_dispatches is enabled and either prints a report of top source-destination pairs and dispatch sources, or plots a heatmap that can be viewed interactively and saved to file. BUG=v8:4899 LOG=N ==========
The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1869423002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1869423002/120001
Description was changed from ========== [Interpreter] Add Ignition dispatch counters visualization tool. bytecode_dispatches_report.py reads the table produced when FLAG_trace_ignition_dispatches is enabled and either prints a report of top source-destination pairs and dispatch sources, or plots a heatmap that can be viewed interactively and saved to file. BUG=v8:4899 LOG=N ========== to ========== [Interpreter] Add Ignition dispatch counters visualization tool. bytecode_dispatches_report.py reads the table produced when FLAG_trace_ignition_dispatches is enabled and either prints a report of top source-destination pairs, or plots a heatmap that can be viewed interactively and saved to file. BUG=v8:4899 LOG=N ==========
Description was changed from ========== [Interpreter] Add Ignition dispatch counters visualization tool. bytecode_dispatches_report.py reads the table produced when FLAG_trace_ignition_dispatches is enabled and either prints a report of top source-destination pairs, or plots a heatmap that can be viewed interactively and saved to file. BUG=v8:4899 LOG=N ========== to ========== [Interpreter] Add Ignition dispatch counters visualization tool. bytecode_dispatches_report.py reads the table produced when FLAG_trace_ignition_dispatches is enabled and either prints a report of top source-destination pairs, or plots a heatmap that can be viewed interactively or saved to file. BUG=v8:4899 LOG=N ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== [Interpreter] Add Ignition dispatch counters visualization tool. bytecode_dispatches_report.py reads the table produced when FLAG_trace_ignition_dispatches is enabled and either prints a report of top source-destination pairs, or plots a heatmap that can be viewed interactively or saved to file. BUG=v8:4899 LOG=N ========== to ========== [Interpreter] Add Ignition dispatch counters visualization tool. bytecode_dispatches_report.py reads the table produced when FLAG_trace_ignition_dispatches is enabled and produces either a report of top source-destination pairs, or a heatmap that can be viewed interactively and saved to file. BUG=v8:4899 LOG=N ==========
https://codereview.chromium.org/1869423002/diff/120001/tools/ignition/bytecod... File tools/ignition/bytecode_dispatches_report_test.py (right): https://codereview.chromium.org/1869423002/diff/120001/tools/ignition/bytecod... tools/ignition/bytecode_dispatches_report_test.py:21: On 2016/04/12 16:56:32, Stefano Sanfilippo wrote: > On 2016/04/12 16:25:52, rmcilroy wrote: > > two newlines > > These are methods of the test class, am I wrong in saying that it should be one > blank line between them? Sorry you are right.
Description was changed from ========== [Interpreter] Add Ignition dispatch counters visualization tool. bytecode_dispatches_report.py reads the table produced when FLAG_trace_ignition_dispatches is enabled and produces either a report of top source-destination pairs, or a heatmap that can be viewed interactively and saved to file. BUG=v8:4899 LOG=N ========== to ========== [Interpreter] Add visualization tool for Ignition dispatch counters. A new script, bytecode_dispatches_report.py reads the table produced when FLAG_trace_ignition_dispatches is enabled and produces either a report of top source-destination pairs, or a heatmap that can be viewed interactively and saved to file. BUG=v8:4899 LOG=N ==========
The CQ bit was checked by ssanfilippo@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1869423002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1869423002/120001
Message was sent while issue was closed.
Description was changed from ========== [Interpreter] Add visualization tool for Ignition dispatch counters. A new script, bytecode_dispatches_report.py reads the table produced when FLAG_trace_ignition_dispatches is enabled and produces either a report of top source-destination pairs, or a heatmap that can be viewed interactively and saved to file. BUG=v8:4899 LOG=N ========== to ========== [Interpreter] Add visualization tool for Ignition dispatch counters. A new script, bytecode_dispatches_report.py reads the table produced when FLAG_trace_ignition_dispatches is enabled and produces either a report of top source-destination pairs, or a heatmap that can be viewed interactively and saved to file. BUG=v8:4899 LOG=N ==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== [Interpreter] Add visualization tool for Ignition dispatch counters. A new script, bytecode_dispatches_report.py reads the table produced when FLAG_trace_ignition_dispatches is enabled and produces either a report of top source-destination pairs, or a heatmap that can be viewed interactively and saved to file. BUG=v8:4899 LOG=N ========== to ========== [Interpreter] Add visualization tool for Ignition dispatch counters. A new script, bytecode_dispatches_report.py reads the table produced when FLAG_trace_ignition_dispatches is enabled and produces either a report of top source-destination pairs, or a heatmap that can be viewed interactively and saved to file. BUG=v8:4899 LOG=N Committed: https://crrev.com/b4df8dab45bee9fd2f44ddfb3dbd20da03cefc56 Cr-Commit-Position: refs/heads/master@{#35438} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/b4df8dab45bee9fd2f44ddfb3dbd20da03cefc56 Cr-Commit-Position: refs/heads/master@{#35438} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
