Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1783503002:
[Interpreter] Add Ignition profile visualization tool. (Closed)
|
Created:
4 years, 9 months ago by Stefano Sanfilippo Modified:
4 years, 8 months ago CC:
v8-reviews_googlegroups.com, rmcilroy Base URL:
https://chromium.googlesource.com/v8/v8.git@run-perf Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[Interpreter] Add Ignition profile visualization tool.
A new script is introduced, linux_perf_report.py, which reads Linux perf
data collected when running with FLAG_perf_basic_prof enabled and
produces an input file for flamegraph.pl, or a report of the hottest
bytecode handlers.
The bottom blocks of the produced flamegraph are bytecode handlers.
Special bottom blocks exist as well for compile routines, time spent
outside the interpreter and interpreter entry trampolines.
Because various Stubs and other pieces of JITted code do not maintain the
frame pointer, some sampled callchains might be incomplete even if V8 is
compiled with no_omit_framepointer=on. The script is able to detect the
most common anomaly where an entry trampoline appears in a chain, but not
on top, meaning that the frame of another bytecode handler is hidden. In
this case, the sample will be moved to a [misattributed] group to avoid
skewing the profile of unrelated handlers.
Misattributed samples and compilation routines are hidden by default.
BUG=v8:4899
LOG=N
Committed: https://crrev.com/8b5d4c74ec39dd773aa057ba12ed68fd2292eeb0
Cr-Commit-Position: refs/heads/master@{#35574}
Patch Set 1 #Patch Set 2 : Avoid uneeded unpacking. #
Total comments: 3
Patch Set 3 : Imports in alpha order. #Patch Set 4 : Fix pylint warnings. #Patch Set 5 : Handle anonymous namespaces. #Patch Set 6 : Pass callchains to counter routines. #Patch Set 7 : Test cases. #Patch Set 8 : Test that indentation is not relevant. #Patch Set 9 : Test symbol name regex w/ template instantiations. #
Total comments: 24
Patch Set 10 : Camel case to snake. #Patch Set 11 : Test multiple handlers in callchain. #Patch Set 12 : First review. #Patch Set 13 : Executable bit for ignition_perf_report.py #Patch Set 14 : Fix command line in examples, move strings inside tests, private strings. #Patch Set 15 : Rename viz script and move to ignition/ subdir. #Patch Set 16 : Move unit tests file. #Patch Set 17 : Fix all references to old script name and location. #Patch Set 18 : Better error message, do not strip symbols, add [unattributed] group. #Patch Set 19 : Group other, misattributed and compiler samples, fix split blocks issue w/ workaround, flatten tram… #
Total comments: 18
Patch Set 20 #Patch Set 21 : Review. #Patch Set 22 : Always emit a trailing semicolon, no wait() #
Messages
Total messages: 52 (22 generated)
ssanfilippo@chromium.org changed reviewers: + machenbach@chromium.org
Here's a tool for processing perf sampled data and yield either a heatmap of Ignition bytecodes, or an input file for flamegraph.pl. PTAL.
Python lg. I'm not familiar with the perf format, please find another reviewer who is, e.g. from the ignition team. I'd also usually prefer seeing some tests added for new tools, e.g, in tools/unittests to test the non-obvious functions. https://codereview.chromium.org/1783503002/diff/20001/tools/ignition_perf_rep... File tools/ignition_perf_report.py (right): https://codereview.chromium.org/1783503002/diff/20001/tools/ignition_perf_rep... tools/ignition_perf_report.py:2: # nit: Not sure if presubmit allows this empty comment line. We'll see. https://codereview.chromium.org/1783503002/diff/20001/tools/ignition_perf_rep... tools/ignition_perf_report.py:10: import collections nit: alpha order https://codereview.chromium.org/1783503002/diff/20001/tools/ignition_perf_rep... tools/ignition_perf_report.py:75: handler = callchain[-1].split(":", 1)[1] Assuming that there is no other : in callchain[-1].
On 2016/03/10 11:50:36, Michael Achenbach wrote: > Python lg. I'm not familiar with the perf format, please find another reviewer > who is, e.g. from the ignition team. Added rmcilroy. > I'd also usually prefer seeing some tests added for new tools, e.g, in > tools/unittests to test the non-obvious functions. Sure, let's see what I can do. > https://codereview.chromium.org/1783503002/diff/20001/tools/ignition_perf_rep... > File tools/ignition_perf_report.py (right): > > https://codereview.chromium.org/1783503002/diff/20001/tools/ignition_perf_rep... > tools/ignition_perf_report.py:2: # > nit: Not sure if presubmit allows this empty comment line. We'll see. I have scheduled a dry run of the presubmit trybot. > https://codereview.chromium.org/1783503002/diff/20001/tools/ignition_perf_rep... > tools/ignition_perf_report.py:10: import collections > nit: alpha order Done. > https://codereview.chromium.org/1783503002/diff/20001/tools/ignition_perf_rep... > tools/ignition_perf_report.py:75: handler = callchain[-1].split(":", 1)[1] > Assuming that there is no other : in callchain[-1]. Sorry, I am not sure I am following you here... Cheers, --S
Description was changed from ========== [Interpreter] Add Ignition profile visualization tool. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] Add Ignition profile visualization tool. BUG=v8:4280 LOG=N ==========
ssanfilippo@chromium.org changed reviewers: + rmcilroy@chromium.org
> https://codereview.chromium.org/1783503002/diff/20001/tools/ignition_perf_rep... > > tools/ignition_perf_report.py:75: handler = callchain[-1].split(":", 1)[1] > > Assuming that there is no other : in callchain[-1]. > > Sorry, I am not sure I am following you here... Never mind, didn't see the ", 1".
On 2016/03/10 12:33:00, Stefano Sanfilippo wrote: > On 2016/03/10 11:50:36, Michael Achenbach wrote: > > nit: Not sure if presubmit allows this empty comment line. We'll see. > > I have scheduled a dry run of the presubmit trybot. Presubmit appears to pass (red because lgtm missing). --S
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/1783503002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1783503002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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/1783503002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1783503002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Looks really good, thanks! https://codereview.chromium.org/1783503002/diff/160001/tools/ignition_perf_re... File tools/ignition_perf_report.py (right): https://codereview.chromium.org/1783503002/diff/160001/tools/ignition_perf_re... tools/ignition_perf_report.py:21: EPILOGUE = """ HELP_EPILOGUE. Also make these all private by adding two leading underscores. https://codereview.chromium.org/1783503002/diff/160001/tools/ignition_perf_re... tools/ignition_perf_report.py:45: def yield_collapsed_callchains(perf_stream, hide_compile_time=False): Probably better named collapsed_callchains_generator. Also please add comments to the to of the function stating what it does and what the return value is - also for count_callchains and count_handler_samples) https://codereview.chromium.org/1783503002/diff/160001/tools/ignition_perf_re... tools/ignition_perf_report.py:49: if line[0] == "#": Add some comments (e.g., # Skip comments, # Empty line signals a new call chain, etc.) https://codereview.chromium.org/1783503002/diff/160001/tools/ignition_perf_re... tools/ignition_perf_report.py:58: symbol = SYMBOL_NAME_RE.match(line.split(" ", 1)[1]).group(1) nit newline above https://codereview.chromium.org/1783503002/diff/160001/tools/ignition_perf_re... tools/ignition_perf_report.py:67: def count_callchains(callchains): get_callchain_sample_counts ? https://codereview.chromium.org/1783503002/diff/160001/tools/ignition_perf_re... tools/ignition_perf_report.py:75: def count_handler_samples(callchains): get_bytecode_handler_sample_counts ? https://codereview.chromium.org/1783503002/diff/160001/tools/ignition_perf_re... tools/ignition_perf_report.py:82: return sorted(handler_counters.items(), Maybe just do the sort in main below, to keep these two functions similar in their API. https://codereview.chromium.org/1783503002/diff/160001/tools/ignition_perf_re... tools/ignition_perf_report.py:129: output_stream.write("{} {}\n".format(callchain, count)) nit - move to a seperate function (for clarity) and the same for the code to output hottest bytecodes. https://codereview.chromium.org/1783503002/diff/160001/tools/unittests/igniti... File tools/unittests/ignition_perf_report_test.py (right): https://codereview.chromium.org/1783503002/diff/160001/tools/unittests/igniti... tools/unittests/ignition_perf_report_test.py:100: class IgnitionPerfReportTest(unittest.TestCase): Are these tests run on the bots? If not they are probably just going to rot. Machenbach - is there a mechanism to run the python tool unittests on the bots? https://codereview.chromium.org/1783503002/diff/160001/tools/unittests/igniti... tools/unittests/ignition_perf_report_test.py:114: self.assertItemsEqual(counters, CALLCHAINS_COUNTERS) nit - could you make CALLCHAINS_COUNTERS a constant in this function (so the expected values are in the test themself rather than hidden up above. Same with all other expected results which are only used once.
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/1783503002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1783503002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1783503002/diff/160001/tools/ignition_perf_re... File tools/ignition_perf_report.py (right): https://codereview.chromium.org/1783503002/diff/160001/tools/ignition_perf_re... tools/ignition_perf_report.py:21: EPILOGUE = """ On 2016/03/11 11:26:49, rmcilroy wrote: > HELP_EPILOGUE. Also make these all private by adding two leading underscores. Done. https://codereview.chromium.org/1783503002/diff/160001/tools/ignition_perf_re... tools/ignition_perf_report.py:45: def yield_collapsed_callchains(perf_stream, hide_compile_time=False): On 2016/03/11 11:26:48, rmcilroy wrote: > Probably better named collapsed_callchains_generator. > > Also please add comments to the to of the function stating what it does and what > the return value is - also for count_callchains and count_handler_samples) Done. https://codereview.chromium.org/1783503002/diff/160001/tools/ignition_perf_re... tools/ignition_perf_report.py:49: if line[0] == "#": On 2016/03/11 11:26:49, rmcilroy wrote: > Add some comments (e.g., # Skip comments, # Empty line signals a new call chain, > etc.) Done. https://codereview.chromium.org/1783503002/diff/160001/tools/ignition_perf_re... tools/ignition_perf_report.py:58: symbol = SYMBOL_NAME_RE.match(line.split(" ", 1)[1]).group(1) On 2016/03/11 11:26:49, rmcilroy wrote: > nit newline above Done. https://codereview.chromium.org/1783503002/diff/160001/tools/ignition_perf_re... tools/ignition_perf_report.py:67: def count_callchains(callchains): On 2016/03/11 11:26:49, rmcilroy wrote: > get_callchain_sample_counts ? I think calculate_* better conveys the idea that there's some computation going on, get_* seems more a plain getter than anything. https://codereview.chromium.org/1783503002/diff/160001/tools/ignition_perf_re... tools/ignition_perf_report.py:75: def count_handler_samples(callchains): On 2016/03/11 11:26:49, rmcilroy wrote: > get_bytecode_handler_sample_counts ? Same as above. https://codereview.chromium.org/1783503002/diff/160001/tools/ignition_perf_re... tools/ignition_perf_report.py:82: return sorted(handler_counters.items(), On 2016/03/11 11:26:49, rmcilroy wrote: > Maybe just do the sort in main below, to keep these two functions similar in > their API. Done. https://codereview.chromium.org/1783503002/diff/160001/tools/ignition_perf_re... tools/ignition_perf_report.py:129: output_stream.write("{} {}\n".format(callchain, count)) On 2016/03/11 11:26:49, rmcilroy wrote: > nit - move to a seperate function (for clarity) and the same for the code to > output hottest bytecodes. Done. https://codereview.chromium.org/1783503002/diff/160001/tools/unittests/igniti... File tools/unittests/ignition_perf_report_test.py (right): https://codereview.chromium.org/1783503002/diff/160001/tools/unittests/igniti... tools/unittests/ignition_perf_report_test.py:114: self.assertItemsEqual(counters, CALLCHAINS_COUNTERS) On 2016/03/11 11:26:49, rmcilroy wrote: > nit - could you make CALLCHAINS_COUNTERS a constant in this function (so the > expected values are in the test themself rather than hidden up above. Same with > all other expected results which are only used once. I'd prefer to keep them all outside, in case we add other tests.
LGTM once last comments are addressed. Thanks. https://codereview.chromium.org/1783503002/diff/160001/tools/ignition_perf_re... File tools/ignition_perf_report.py (right): https://codereview.chromium.org/1783503002/diff/160001/tools/ignition_perf_re... tools/ignition_perf_report.py:21: EPILOGUE = """ On 2016/03/11 16:33:51, Stefano Sanfilippo wrote: > On 2016/03/11 11:26:49, rmcilroy wrote: > > HELP_EPILOGUE. Also make these all private by adding two leading underscores. > > Done. You never made the fields private. https://codereview.chromium.org/1783503002/diff/160001/tools/unittests/igniti... File tools/unittests/ignition_perf_report_test.py (right): https://codereview.chromium.org/1783503002/diff/160001/tools/unittests/igniti... tools/unittests/ignition_perf_report_test.py:114: self.assertItemsEqual(counters, CALLCHAINS_COUNTERS) On 2016/03/11 16:33:51, Stefano Sanfilippo wrote: > On 2016/03/11 11:26:49, rmcilroy wrote: > > nit - could you make CALLCHAINS_COUNTERS a constant in this function (so the > > expected values are in the test themself rather than hidden up above. Same > with > > all other expected results which are only used once. > > I'd prefer to keep them all outside, in case we add other tests. I disagree - the tests should be easy to reason about locally, even if that ends up causing some duplication in the test.
https://codereview.chromium.org/1783503002/diff/160001/tools/ignition_perf_re... File tools/ignition_perf_report.py (right): https://codereview.chromium.org/1783503002/diff/160001/tools/ignition_perf_re... tools/ignition_perf_report.py:21: EPILOGUE = """ On 2016/03/14 12:26:52, rmcilroy wrote: > On 2016/03/11 16:33:51, Stefano Sanfilippo wrote: > > On 2016/03/11 11:26:49, rmcilroy wrote: > > > HELP_EPILOGUE. Also make these all private by adding two leading > underscores. > > > > Done. > > You never made the fields private. Wooops, sorry about that. Done for the help strings. About the regex, we have test cases for those, so they need to be exportable. How should we handle this case? https://codereview.chromium.org/1783503002/diff/160001/tools/unittests/igniti... File tools/unittests/ignition_perf_report_test.py (right): https://codereview.chromium.org/1783503002/diff/160001/tools/unittests/igniti... tools/unittests/ignition_perf_report_test.py:114: self.assertItemsEqual(counters, CALLCHAINS_COUNTERS) On 2016/03/14 12:26:52, rmcilroy wrote: > On 2016/03/11 16:33:51, Stefano Sanfilippo wrote: > > On 2016/03/11 11:26:49, rmcilroy wrote: > > > nit - could you make CALLCHAINS_COUNTERS a constant in this function (so the > > > expected values are in the test themself rather than hidden up above. Same > > with > > > all other expected results which are only used once. > > > > I'd prefer to keep them all outside, in case we add other tests. > > I disagree - the tests should be easy to reason about locally, even if that ends > up causing some duplication in the test. I see. Done.
https://codereview.chromium.org/1783503002/diff/160001/tools/ignition_perf_re... File tools/ignition_perf_report.py (right): https://codereview.chromium.org/1783503002/diff/160001/tools/ignition_perf_re... tools/ignition_perf_report.py:21: EPILOGUE = """ On 2016/03/14 13:14:20, Stefano Sanfilippo wrote: > On 2016/03/14 12:26:52, rmcilroy wrote: > > On 2016/03/11 16:33:51, Stefano Sanfilippo wrote: > > > On 2016/03/11 11:26:49, rmcilroy wrote: > > > > HELP_EPILOGUE. Also make these all private by adding two leading > > underscores. > > > > > > Done. > > > > You never made the fields private. > > Wooops, sorry about that. Done for the help strings. About the regex, we have > test cases for those, so they need to be exportable. How should we handle this > case? This is fine, thanks.
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/1783503002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1783503002/260001
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 profile visualization tool. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] Add Ignition profile visualization tool. BUG=v8:4899 LOG=N ==========
Patchset #17 (id:320001) has been deleted
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/1783503002/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1783503002/380001
Description was changed from ========== [Interpreter] Add Ignition profile visualization tool. BUG=v8:4899 LOG=N ========== to ========== [Interpreter] Add Ignition profile visualization tool. A new script is introduced, linux_perf_report.py, which reads Linux perf data collected when running with FLAG_perf_basic_prof enabled and produces an input file for flamegraph.pl, or a report of the hottest bytecode handlers. The bottom blocks of the produced flamegraph are bytecode handlers. Special bottom blocks exist as well for compile routines, time spent outside the interpreter and interpreter entry trampolines. Because various Stubs and other pieces of JITted code do not maintain the frame pointer, some sampled callchains might be incomplete even if V8 is compiled with no_omit_framepointer=on. The script is able to detect the most common anomaly where an entry trampoline appears in a chain, but not on top, meaning that the frame of another bytecode handler is hidden. In this case, the sample will be moved to a [misattributed] group to avoid skewing the profile of unrelated handlers. Misattributed samples and compilation routines are hidden by default. BUG=v8:4899 LOG=N ==========
Code, tests and description updated, PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1783503002/diff/380001/tools/ignition/linux_p... File tools/ignition/linux_perf_report.py (right): https://codereview.chromium.org/1783503002/diff/380001/tools/ignition/linux_p... tools/ignition/linux_perf_report.py:24: # without considering the time spent compiling JS code, misattributed samples nit - I wouldn't mention misattributed samples here since we want that to go away once there is support for DWARF-style unwinding. https://codereview.chromium.org/1783503002/diff/380001/tools/ignition/linux_p... tools/ignition/linux_perf_report.py:102: symbol = get_function_name(symbol) nit - s/get_function_name/strip_function_parameters/ https://codereview.chromium.org/1783503002/diff/380001/tools/ignition/linux_p... tools/ignition/linux_perf_report.py:116: # The chain cannot be empty at this point Not sure what this comment means. I think what you want is: # If we see an InterpreterEntryTrampoline which is not at the top of the chain, and doesn't have a BytecodeHandler above it, then we have skipped the top BytecodeHandler due to the top-level stub not building a frame. File the chain in a misattributed bucket. Also put it in the "elif show_other" https://codereview.chromium.org/1783503002/diff/380001/tools/ignition/linux_p... tools/ignition/linux_perf_report.py:131: key = callchain[0] + ";" Could you just always add a ";" to the end rather than just if len == 1? https://codereview.chromium.org/1783503002/diff/380001/tools/ignition/linux_p... tools/ignition/linux_perf_report.py:185: "--show-other", "-r", -r is a strange shortening. How about "--show-all" and "-a" ? https://codereview.chromium.org/1783503002/diff/380001/tools/ignition/linux_p... tools/ignition/linux_perf_report.py:222: perf.communicate() Why is this needed? Shouldn't it be before reading stdout if needed?
https://codereview.chromium.org/1783503002/diff/380001/tools/ignition/linux_p... File tools/ignition/linux_perf_report.py (right): https://codereview.chromium.org/1783503002/diff/380001/tools/ignition/linux_p... tools/ignition/linux_perf_report.py:24: # without considering the time spent compiling JS code, misattributed samples On 2016/04/15 14:58:41, rmcilroy wrote: > nit - I wouldn't mention misattributed samples here since we want that to go > away once there is support for DWARF-style unwinding. Done. https://codereview.chromium.org/1783503002/diff/380001/tools/ignition/linux_p... tools/ignition/linux_perf_report.py:102: symbol = get_function_name(symbol) On 2016/04/15 14:58:41, rmcilroy wrote: > nit - s/get_function_name/strip_function_parameters/ Done. https://codereview.chromium.org/1783503002/diff/380001/tools/ignition/linux_p... tools/ignition/linux_perf_report.py:116: # The chain cannot be empty at this point On 2016/04/15 14:58:41, rmcilroy wrote: > Not sure what this comment means. I think what you want is: > > # If we see an InterpreterEntryTrampoline which is not at the top of the chain, > and doesn't have a BytecodeHandler above it, then we have skipped the top > BytecodeHandler due to the top-level stub not building a frame. File the chain > in a misattributed bucket. > > Also put it in the "elif show_other" The comment was because the else branch assumes the chain has at least one element. Since the case of a single element is handled in the if branch, we can enter the else with many elements or none, and the latter case should not happen because the chain can never be empty. This is because it necessarily contains at least the current symbol. Anwyay, I pasted your comment in the else branch. Should we add an assert as well for the condition above? https://codereview.chromium.org/1783503002/diff/380001/tools/ignition/linux_p... tools/ignition/linux_perf_report.py:131: key = callchain[0] + ";" On 2016/04/15 14:58:42, rmcilroy wrote: > Could you just always add a ";" to the end rather than just if len == 1? This is not the format shown in flamegraph examples (and the one produced by the various converter scripts), but it works anyway. The trailing ; was originally supposed to be a workaround for split blocks, for me is the same though. https://codereview.chromium.org/1783503002/diff/380001/tools/ignition/linux_p... tools/ignition/linux_perf_report.py:185: "--show-other", "-r", On 2016/04/15 14:58:41, rmcilroy wrote: > -r is a strange shortening. How about "--show-all" and "-a" ? Done. https://codereview.chromium.org/1783503002/diff/380001/tools/ignition/linux_p... tools/ignition/linux_perf_report.py:222: perf.communicate() On 2016/04/15 14:58:42, rmcilroy wrote: > Why is this needed? Shouldn't it be before reading stdout if needed? This was intended to work as a wait() to ensure we exit only after our subprocess. I suppose directly using wait() should not cause any deadlock because we are reading the stdout of the subprocess till EOF.
LGTM https://codereview.chromium.org/1783503002/diff/380001/tools/ignition/linux_p... File tools/ignition/linux_perf_report.py (right): https://codereview.chromium.org/1783503002/diff/380001/tools/ignition/linux_p... tools/ignition/linux_perf_report.py:116: # The chain cannot be empty at this point On 2016/04/15 16:13:18, Stefano Sanfilippo wrote: > On 2016/04/15 14:58:41, rmcilroy wrote: > > Not sure what this comment means. I think what you want is: > > > > # If we see an InterpreterEntryTrampoline which is not at the top of the > chain, > > and doesn't have a BytecodeHandler above it, then we have skipped the top > > BytecodeHandler due to the top-level stub not building a frame. File the chain > > in a misattributed bucket. > > > > Also put it in the "elif show_other" > > The comment was because the else branch assumes the chain has at least one > element. Since the case of a single element is handled in the if branch, we can > enter the else with many elements or none, and the latter case should not happen > because the chain can never be empty. This is because it necessarily contains at > least the current symbol. > > Anwyay, I pasted your comment in the else branch. Should we add an assert as > well for the condition above? I don't think it is necessary, you unconditinally insert an element into the chain at the top of the whole if block. https://codereview.chromium.org/1783503002/diff/380001/tools/ignition/linux_p... tools/ignition/linux_perf_report.py:131: key = callchain[0] + ";" On 2016/04/15 16:13:19, Stefano Sanfilippo wrote: > On 2016/04/15 14:58:42, rmcilroy wrote: > > Could you just always add a ";" to the end rather than just if len == 1? > > This is not the format shown in flamegraph examples (and the one produced by the > various converter scripts), but it works anyway. > > The trailing ; was originally supposed to be a workaround for split blocks, for > me is the same though. I think it would be better to make things consistent (i.e., always insert the dummy ';' at the end of the chain). If so, please update the comment above too. https://codereview.chromium.org/1783503002/diff/380001/tools/ignition/linux_p... tools/ignition/linux_perf_report.py:222: perf.communicate() On 2016/04/15 16:13:18, Stefano Sanfilippo wrote: > On 2016/04/15 14:58:42, rmcilroy wrote: > > Why is this needed? Shouldn't it be before reading stdout if needed? > > This was intended to work as a wait() to ensure we exit only after our > subprocess. I suppose directly using wait() should not cause any deadlock > because we are reading the stdout of the subprocess till EOF. I don't see why this is necessary at all. You are already waiting for EOF, which should only be sent if the subprocess exits normally (unless it does something strange).
https://codereview.chromium.org/1783503002/diff/380001/tools/ignition/linux_p... File tools/ignition/linux_perf_report.py (right): https://codereview.chromium.org/1783503002/diff/380001/tools/ignition/linux_p... tools/ignition/linux_perf_report.py:116: # The chain cannot be empty at this point On 2016/04/18 09:22:24, rmcilroy wrote: > On 2016/04/15 16:13:18, Stefano Sanfilippo wrote: > > On 2016/04/15 14:58:41, rmcilroy wrote: > > > Not sure what this comment means. I think what you want is: > > > > > > # If we see an InterpreterEntryTrampoline which is not at the top of the > > chain, > > > and doesn't have a BytecodeHandler above it, then we have skipped the top > > > BytecodeHandler due to the top-level stub not building a frame. File the > chain > > > in a misattributed bucket. > > > > > > Also put it in the "elif show_other" > > > > The comment was because the else branch assumes the chain has at least one > > element. Since the case of a single element is handled in the if branch, we > can > > enter the else with many elements or none, and the latter case should not > happen > > because the chain can never be empty. This is because it necessarily contains > at > > least the current symbol. > > > > Anwyay, I pasted your comment in the else branch. Should we add an assert as > > well for the condition above? > > I don't think it is necessary, you unconditinally insert an element into the > chain at the top of the whole if block. Acknowledged. https://codereview.chromium.org/1783503002/diff/380001/tools/ignition/linux_p... tools/ignition/linux_perf_report.py:131: key = callchain[0] + ";" On 2016/04/18 09:22:24, rmcilroy wrote: > On 2016/04/15 16:13:19, Stefano Sanfilippo wrote: > > On 2016/04/15 14:58:42, rmcilroy wrote: > > > Could you just always add a ";" to the end rather than just if len == 1? > > > > This is not the format shown in flamegraph examples (and the one produced by > the > > various converter scripts), but it works anyway. > > > > The trailing ; was originally supposed to be a workaround for split blocks, > for > > me is the same though. > > I think it would be better to make things consistent (i.e., always insert the > dummy ';' at the end of the chain). If so, please update the comment above too. No problem, done. https://codereview.chromium.org/1783503002/diff/380001/tools/ignition/linux_p... tools/ignition/linux_perf_report.py:222: perf.communicate() On 2016/04/18 09:22:24, rmcilroy wrote: > On 2016/04/15 16:13:18, Stefano Sanfilippo wrote: > > On 2016/04/15 14:58:42, rmcilroy wrote: > > > Why is this needed? Shouldn't it be before reading stdout if needed? > > > > This was intended to work as a wait() to ensure we exit only after our > > subprocess. I suppose directly using wait() should not cause any deadlock > > because we are reading the stdout of the subprocess till EOF. > > I don't see why this is necessary at all. You are already waiting for EOF, which > should only be sent if the subprocess exits normally (unless it does something > strange). Agreed, removed.
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/1783503002/440001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1783503002/440001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_asan_rel_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
The CQ bit was checked by ssanfilippo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rmcilroy@chromium.org Link to the patchset: https://codereview.chromium.org/1783503002/#ps440001 (title: "Always emit a trailing semicolon, no wait()")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1783503002/440001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1783503002/440001
Message was sent while issue was closed.
Description was changed from ========== [Interpreter] Add Ignition profile visualization tool. A new script is introduced, linux_perf_report.py, which reads Linux perf data collected when running with FLAG_perf_basic_prof enabled and produces an input file for flamegraph.pl, or a report of the hottest bytecode handlers. The bottom blocks of the produced flamegraph are bytecode handlers. Special bottom blocks exist as well for compile routines, time spent outside the interpreter and interpreter entry trampolines. Because various Stubs and other pieces of JITted code do not maintain the frame pointer, some sampled callchains might be incomplete even if V8 is compiled with no_omit_framepointer=on. The script is able to detect the most common anomaly where an entry trampoline appears in a chain, but not on top, meaning that the frame of another bytecode handler is hidden. In this case, the sample will be moved to a [misattributed] group to avoid skewing the profile of unrelated handlers. Misattributed samples and compilation routines are hidden by default. BUG=v8:4899 LOG=N ========== to ========== [Interpreter] Add Ignition profile visualization tool. A new script is introduced, linux_perf_report.py, which reads Linux perf data collected when running with FLAG_perf_basic_prof enabled and produces an input file for flamegraph.pl, or a report of the hottest bytecode handlers. The bottom blocks of the produced flamegraph are bytecode handlers. Special bottom blocks exist as well for compile routines, time spent outside the interpreter and interpreter entry trampolines. Because various Stubs and other pieces of JITted code do not maintain the frame pointer, some sampled callchains might be incomplete even if V8 is compiled with no_omit_framepointer=on. The script is able to detect the most common anomaly where an entry trampoline appears in a chain, but not on top, meaning that the frame of another bytecode handler is hidden. In this case, the sample will be moved to a [misattributed] group to avoid skewing the profile of unrelated handlers. Misattributed samples and compilation routines are hidden by default. BUG=v8:4899 LOG=N ==========
Message was sent while issue was closed.
Committed patchset #22 (id:440001)
Message was sent while issue was closed.
Description was changed from ========== [Interpreter] Add Ignition profile visualization tool. A new script is introduced, linux_perf_report.py, which reads Linux perf data collected when running with FLAG_perf_basic_prof enabled and produces an input file for flamegraph.pl, or a report of the hottest bytecode handlers. The bottom blocks of the produced flamegraph are bytecode handlers. Special bottom blocks exist as well for compile routines, time spent outside the interpreter and interpreter entry trampolines. Because various Stubs and other pieces of JITted code do not maintain the frame pointer, some sampled callchains might be incomplete even if V8 is compiled with no_omit_framepointer=on. The script is able to detect the most common anomaly where an entry trampoline appears in a chain, but not on top, meaning that the frame of another bytecode handler is hidden. In this case, the sample will be moved to a [misattributed] group to avoid skewing the profile of unrelated handlers. Misattributed samples and compilation routines are hidden by default. BUG=v8:4899 LOG=N ========== to ========== [Interpreter] Add Ignition profile visualization tool. A new script is introduced, linux_perf_report.py, which reads Linux perf data collected when running with FLAG_perf_basic_prof enabled and produces an input file for flamegraph.pl, or a report of the hottest bytecode handlers. The bottom blocks of the produced flamegraph are bytecode handlers. Special bottom blocks exist as well for compile routines, time spent outside the interpreter and interpreter entry trampolines. Because various Stubs and other pieces of JITted code do not maintain the frame pointer, some sampled callchains might be incomplete even if V8 is compiled with no_omit_framepointer=on. The script is able to detect the most common anomaly where an entry trampoline appears in a chain, but not on top, meaning that the frame of another bytecode handler is hidden. In this case, the sample will be moved to a [misattributed] group to avoid skewing the profile of unrelated handlers. Misattributed samples and compilation routines are hidden by default. BUG=v8:4899 LOG=N Committed: https://crrev.com/8b5d4c74ec39dd773aa057ba12ed68fd2292eeb0 Cr-Commit-Position: refs/heads/master@{#35574} ==========
Message was sent while issue was closed.
Patchset 22 (id:??) landed as https://crrev.com/8b5d4c74ec39dd773aa057ba12ed68fd2292eeb0 Cr-Commit-Position: refs/heads/master@{#35574} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
