Add generic perf trace instrument #388
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
9 tasks
Introduce a class allowing to programmatically generate CLI commands in a clear, reliable and robust way for any calling code manipulating raw command strings.
Introduce a perf collector that is more generic than the previous one and which is expected to be able to handle all potential calls to perf (irrespective of the subcommand, flags, options or arguments being used).
ptosi
changed the title
marcbonnici
reviewed
Jun 26, 2019
| kwflags_sep='=', | ||
| end_of_options='--', | ||
| **parameters) | ||
| if 'stat'in parameters['command']: |
There was a problem hiding this comment.
Missing space before in
| for label, command in self.post_commands.items(): | ||
| self.execute(str(command), label) | ||
|
|
||
| def _target_runnable_command(self, command, label=None): |
There was a problem hiding this comment.
"Private" methods should go after the "normal" methods.
| if sys.version_info >= (3, 0): | ||
| from shlex import quote | ||
| else: | ||
| from pipes import quote |
There was a problem hiding this comment.
This implies that compatibility with Python2.7 is attempted to be retained however is not present elsewhere in this PR. Is this aimed to be merged before the next release aka supporting Python 2.7 or to be held until after support has been dropped?
|
See the comment on ARM-software/workload-automation#979. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As discussed in #382, this PR modifies the
perftrace instrument so that it accepts anyperfcommand. It implements the back-end of ARM-software/workload-automation#979 and requires fix #387 to avoid #386. It is arguably simple as it receives the commands to run as strings (prepending the path toperf) and runs them "at the right time".Based on the subcommands
perfcurrently has, it looks like these can be placed in 3 categories:commands): used to trace/measure some system properties (stat,record,mem,sched,kmem,kvm,c2c, ...);post_commands): used to process a trace file from a "measuring" command (report,script,timechart,annotate,c2c report`, ...);pre_commands): used to configure the system or get general information (list,config,buildid-list,help, ...)Therefore, we take 3 dictionaries of commands that are run in order (
pre_, thencommands, thenpost_) in accord with these definitions. I'm still not perfectly sure about "when" in the WA/devlib process these are to be run, though. Please check that.The reason why we have dictionaries for commands (instead of lists) is that the keys can act as a label which seemed to be a desired feature of the previous implementation. As we have multiple dictionaries, a same label can appear multiple times (i.e. as a key in more than a single dictionary). In that case, it makes sense to "group" these commands in their own "execution lane" e.g. a
recordand its correspondingreport. Maybe, these labels could be used as directory names on the target and then on the host, once having been extracted(?) For example, commands within the same label would "see" the same files (as they would be in the same subfolder) while commands with different labels wouldn't and would therefore be able to re-use the same filenames. A similar approach with respect to the filename would be to prepend/inject/append the label to the filename.The
Commandclass is implemented to be used by the corresponding WAperfinstrument when generating theperfcommands passed to the devlibPerfCollector. I thought it would make sense to have that class in devlib instead of WA. See the front-end PR for a description of the constructor parameters.The
get_tracehasn't been implemented yet. If we're going with a function that extracts oneoutfileat a time, the caller (WA) will have to keep track of which files are generated by which commands (e.g. thestdout,stderr,perf.data, ...) which is hard to do. Another approach might be to extract a pull a complete folder from the target (e.g./sdcard/devlib-target/perf) in whichperfwould have run. This decision is also influenced by whether labels are used as subfolders or not.Parsing of the output(s) is the responsibility of WA and won't be done here.
Please note that the proposed implementation is not an in-place replacement for the previous implementation of
PerfCollector. Therefore, the previous implementation has been moved toperf_stat, should be deprecated (not sure what the best way to do that is) and should eventually be removed.TODO:
pre_,commands,post_in the WA/devlib flow;DeprecatePerfStatCollectorPerfCollector(what is expected?)