-
Notifications
You must be signed in to change notification settings - Fork 65
Migrate four tools to commandline v2 #362
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from 1 commit
Commits
Show all changes
23 commits
Select commit
Hold shift + click to select a range
8c9f7bf
Use System.CommandLine v2 in cijobs
am11 c83640e
Use System.CommandLine v2 in jit-analyze
am11 817444b
Use System.CommandLine v2 in jit-dasm-pmi
am11 f93be71
Use System.CommandLine v2 in jit-dasm
am11 66b27ce
Update configs
am11 32c3deb
Fix custom parsing and defaults
am11 1b61f17
Import common props in mutate-test
am11 2b316e9
Nitpicks
am11 1c52e3b
Merge dotnet/main into feature/command-line-api-v2
am11 bf68548
Bump version: 2.0.0-beta4.22564.1
am11 95056b2
Publish tools as single-file
am11 6e01009
Skip pmi projects and fix shell-check warnings
am11 4a347ba
Fix an error message
am11 eb01579
Align version option in System.CommandLine usages
am11 9840926
Only exclude PMI project from singlefile publishing
am11 5920112
Flip condition
am11 a9b0dd1
Updat comments
am11 08eb48d
Revert src/pmi/pmi.cs
am11 e4a8166
Make AssemblyList an argument to the command to preserve existing beh…
jakobbotsch b163b08
Fix some formatting
jakobbotsch 7efb530
Fix NullReferenceException when not supplying any args to jit-dasm
jakobbotsch e9443b0
Also fix jit-dasm-pmi
jakobbotsch a51a231
Fix a typo
am11 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Use System.CommandLine v2 in jit-analyze
- Loading branch information
commit c83640e9d900c63cf9f3655193ad48f80e861b5a
There are no files selected for viewing
This file contains hidden or 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
| // See the LICENSE file in the project root for more information. | ||
|
|
||
| using System; | ||
| using System.Collections.Generic; | ||
| using System.Linq; | ||
|
|
||
| namespace ManagedCodeGen | ||
| { | ||
| // Allow Linq to be able to sum up MetricCollections | ||
| public static class IEnumerableExtensions | ||
| { | ||
| public static MetricCollection Sum(this IEnumerable<MetricCollection> source) | ||
| { | ||
| MetricCollection result = new MetricCollection(); | ||
|
|
||
| foreach (MetricCollection s in source) | ||
| { | ||
| result.Add(s); | ||
| } | ||
|
|
||
| return result; | ||
| } | ||
|
|
||
| public static MetricCollection Sum<T>(this IEnumerable<T> source, Func<T, MetricCollection> selector) | ||
| { | ||
| return source.Select(x => selector(x)).Sum(); | ||
| } | ||
| } | ||
| } |
This file contains hidden or 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,128 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
| // See the LICENSE file in the project root for more information. | ||
|
|
||
| using System; | ||
| using System.Collections.Generic; | ||
| using System.CommandLine; | ||
|
|
||
| namespace ManagedCodeGen | ||
| { | ||
| internal sealed class JitAnalyzeRootCommand : RootCommand | ||
| { | ||
| public Option<string> BasePath { get; } = | ||
| new(new[] { "--base", "-b" }, "Base file or directory"); | ||
| public Option<string> DiffPath { get; } = | ||
| new(new[] { "--diff", "-d" }, "Diff file or directory"); | ||
| public Option<bool> Recursive { get; } = | ||
| new(new[] { "--recursive", "-r" }, "Search directories recursively"); | ||
| public Option<string> FileExtension { get; } = | ||
| new("--file-extension", _ => ".dasm", true, "File extension to look for"); | ||
| public Option<int> Count { get; } = | ||
| new(new[] { "--count", "-c" }, _ => 20, true, "Count of files and methods (at most) to output in the summary. (count) improvements and (count) regressions of each will be included"); | ||
| public Option<bool> Warn { get; } = | ||
| new(new[] { "--warn", "-w" }, "Generate warning output for files/methods that only exists in one dataset or the other (only in base or only in diff)"); | ||
| public Option<List<string>> Metrics { get; } = | ||
| new(new[] { "--metrics", "-m" }, _ => new List<string> { "CodeSize" }, true, $"Comma-separated metric to use for diff computations. Available metrics: {MetricCollection.ListMetrics()}"); | ||
| public Option<string> Note { get; } = | ||
| new("--note", "Descriptive note to add to summary output"); | ||
| public Option<bool> NoReconcile { get; } = | ||
| new("--no-reconcile", "Do not reconcile unique methods in base/diff"); | ||
| public Option<string> Json { get; } = | ||
| new("--json", "Dump analysis data to specified file in JSON format"); | ||
| public Option<string> Tsv { get; } = | ||
| new("--tsv", "Dump analysis data to specified file in tab-separated format"); | ||
| public Option<string> MD { get; } = | ||
| new("--md", "Dump analysis data to specified file in markdown format"); | ||
| public Option<string> Filter { get; } = | ||
| new("--filter", "Only consider assembly files whose names match the filter"); | ||
| public Option<bool> SkipTextDiff { get; } = | ||
| new("--skip-text-diff", "Skip analysis that checks for files that have textual diffs but no metric diffs"); | ||
| public Option<bool> RetainOnlyTopFiles { get; } = | ||
| new("--retain-only-top-files", "Retain only the top 'count' improvements/regressions .dasm files. Delete other files. Useful in CI scenario to reduce the upload size"); | ||
| public Option<double> OverrideTotalBaseMetric { get; } = | ||
| new("--override-total-base-metric", "Override the total base metric shown in the output with this value. Useful when only changed .dasm files are present and these values are known"); | ||
| public Option<double> OverrideTotalDiffMetric { get; } = | ||
| new("--override-total-diff-metric", "Override the total diff metric shown in the output with this value. Useful when only changed .dasm files are present and these values are known"); | ||
| public Option<bool> IsDiffsOnly { get; } = | ||
| new("--is-diffs-only", "Specify that the disassembly files are only produced for contexts with diffs, so avoid producing output making assumptions about the number of contexts"); | ||
| public Option<bool> IsSubsetOfDiffs { get; } = | ||
| new("--is-subset-of-diffs", "Specify that the disassembly files are only a subset of the contexts with diffs, so avoid producing output making assumptions about the remaining diffs"); | ||
|
|
||
| public ParseResult Result; | ||
|
|
||
| public JitAnalyzeRootCommand(string[] args) : base("Compare and analyze `*.dasm` files from baseline/diff") | ||
| { | ||
| AddOption(BasePath); | ||
| AddOption(DiffPath); | ||
| AddOption(Recursive); | ||
| AddOption(FileExtension); | ||
| AddOption(Count); | ||
| AddOption(Warn); | ||
| AddOption(Metrics); | ||
| AddOption(Note); | ||
| AddOption(NoReconcile); | ||
| AddOption(Json); | ||
| AddOption(Tsv); | ||
| AddOption(MD); | ||
| AddOption(Filter); | ||
| AddOption(SkipTextDiff); | ||
| AddOption(RetainOnlyTopFiles); | ||
| AddOption(OverrideTotalBaseMetric); | ||
| AddOption(OverrideTotalDiffMetric); | ||
| AddOption(IsDiffsOnly); | ||
| AddOption(IsSubsetOfDiffs); | ||
|
|
||
| this.SetHandler(context => | ||
| { | ||
| Result = context.ParseResult; | ||
|
|
||
| try | ||
| { | ||
| List<string> errors = new(); | ||
| if (Result.GetValueForOption(BasePath) == null) | ||
| { | ||
| errors.Add("Base path (--base) is required"); | ||
| } | ||
|
|
||
| if (Result.GetValueForOption(DiffPath) == null) | ||
| { | ||
| errors.Add("Diff path (--diff) is required"); | ||
| } | ||
|
|
||
| foreach (string metricName in Result.GetValueForOption(Metrics)) | ||
| { | ||
| if (!MetricCollection.ValidateMetric(metricName)) | ||
| { | ||
| errors.Add($"Unknown metric '{metricName}'. Available metrics: {MetricCollection.ListMetrics()}"); | ||
| } | ||
| } | ||
|
|
||
| if ((Result.FindResultFor(OverrideTotalBaseMetric) == null) != (Result.FindResultFor(OverrideTotalDiffMetric) == null)) | ||
| { | ||
| errors.Add("override-total-base-metric and override-total-diff-metric must either both be specified or both not be specified"); | ||
| } | ||
|
|
||
| if (errors.Count > 0) | ||
| { | ||
| throw new Exception(string.Join(Environment.NewLine, errors)); | ||
| } | ||
|
|
||
| context.ExitCode = new Program(this).Run(); | ||
| } | ||
| catch (Exception e) | ||
| { | ||
| Console.ResetColor(); | ||
| Console.ForegroundColor = ConsoleColor.Red; | ||
|
|
||
| Console.Error.WriteLine("Error: " + e.Message); | ||
| Console.Error.WriteLine(e.ToString()); | ||
|
|
||
| Console.ResetColor(); | ||
|
|
||
| context.ExitCode = 1; | ||
| } | ||
| }); | ||
| } | ||
| } | ||
| } | ||
This file contains hidden or 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,152 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
| // See the LICENSE file in the project root for more information. | ||
|
|
||
| using System.Collections.Generic; | ||
| using System.Linq; | ||
| using System.Reflection; | ||
| using System.Text; | ||
| using System.Text.Json.Serialization; | ||
|
|
||
| namespace ManagedCodeGen | ||
| { | ||
| public class MetricCollection | ||
| { | ||
| private static Dictionary<string, int> s_metricNameToIndex; | ||
| private static Metric[] s_metrics; | ||
|
|
||
| static MetricCollection() | ||
| { | ||
| var derivedType = typeof(Metric); | ||
| var currentAssembly = Assembly.GetAssembly(derivedType); | ||
| s_metrics = currentAssembly.GetTypes() | ||
| .Where(t => t != derivedType && derivedType.IsAssignableFrom(t)) | ||
| .Select(t => currentAssembly.CreateInstance(t.FullName)).Cast<Metric>().ToArray(); | ||
|
|
||
| s_metricNameToIndex = new Dictionary<string, int>(s_metrics.Length); | ||
|
|
||
| for (int i = 0; i < s_metrics.Length; i++) | ||
| { | ||
| Metric m = s_metrics[i]; | ||
| s_metricNameToIndex[m.Name] = i; | ||
| } | ||
| } | ||
|
|
||
| [JsonInclude] | ||
| private Metric[] metrics; | ||
|
|
||
| public MetricCollection() | ||
| { | ||
| metrics = new Metric[s_metrics.Length]; | ||
| for (int i = 0; i < s_metrics.Length; i++) | ||
| { | ||
| metrics[i] = s_metrics[i].Clone(); | ||
| } | ||
| } | ||
|
|
||
| public MetricCollection(MetricCollection other) : this() | ||
| { | ||
| this.SetValueFrom(other); | ||
| } | ||
|
|
||
| public static IEnumerable<Metric> AllMetrics => s_metrics; | ||
|
|
||
| public Metric GetMetric(string metricName) | ||
| { | ||
| int index; | ||
| if (s_metricNameToIndex.TryGetValue(metricName, out index)) | ||
| { | ||
| return metrics[index]; | ||
| } | ||
| return null; | ||
| } | ||
|
|
||
| public static bool ValidateMetric(string name) | ||
| { | ||
| return s_metricNameToIndex.TryGetValue(name, out _); | ||
| } | ||
|
|
||
| public static string DisplayName(string metricName) | ||
| { | ||
| int index; | ||
| if (s_metricNameToIndex.TryGetValue(metricName, out index)) | ||
| { | ||
| return s_metrics[index].DisplayName; | ||
| } | ||
| return "Unknown metric"; | ||
| } | ||
|
|
||
| public static string ListMetrics() | ||
| { | ||
| StringBuilder sb = new StringBuilder(); | ||
| bool isFirst = true; | ||
| foreach (string s in s_metricNameToIndex.Keys) | ||
| { | ||
| if (!isFirst) sb.Append(", "); | ||
| sb.Append(s); | ||
| isFirst = false; | ||
| } | ||
| return sb.ToString(); | ||
| } | ||
|
|
||
| public override string ToString() | ||
| { | ||
| StringBuilder sb = new StringBuilder(); | ||
| bool isFirst = true; | ||
| foreach (Metric m in metrics) | ||
| { | ||
| if (!isFirst) sb.Append(", "); | ||
| sb.Append($"{m.Name} {m.Unit} {m.ValueString}"); | ||
| isFirst = false; | ||
| } | ||
| return sb.ToString(); | ||
| } | ||
|
|
||
| public void Add(MetricCollection other) | ||
| { | ||
| for (int i = 0; i < metrics.Length; i++) | ||
| { | ||
| metrics[i].Add(other.metrics[i]); | ||
| } | ||
| } | ||
|
|
||
| public void Add(string metricName, double value) | ||
| { | ||
| Metric m = GetMetric(metricName); | ||
| m.Value += value; | ||
| } | ||
|
|
||
| public void Sub(MetricCollection other) | ||
| { | ||
| for (int i = 0; i < metrics.Length; i++) | ||
| { | ||
| metrics[i].Sub(other.metrics[i]); | ||
| } | ||
| } | ||
|
|
||
| public void Rel(MetricCollection other) | ||
| { | ||
| for (int i = 0; i < metrics.Length; i++) | ||
| { | ||
| metrics[i].Rel(other.metrics[i]); | ||
| } | ||
| } | ||
|
|
||
| public void SetValueFrom(MetricCollection other) | ||
| { | ||
| for (int i = 0; i < metrics.Length; i++) | ||
| { | ||
| metrics[i].SetValueFrom(other.metrics[i]); | ||
| } | ||
| } | ||
|
|
||
| public bool IsZero() | ||
| { | ||
| for (int i = 0; i < metrics.Length; i++) | ||
| { | ||
| if (metrics[i].Value != 0) return false; | ||
| } | ||
| return true; | ||
| } | ||
| } | ||
| } |
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it still true that this can be specified with comma-separated syntax?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope. :)
I have reworded it to reflect unlimited arity of argument.
Also,
_ => <const>, true/falsewas incorrect for frozen default values, replaced with() => <const>.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that superpmi.py in dotnet/runtime is passing this arg so it will need changes too.
It does introduce some weirdness if you try to run superpmi on the servicing branches, but I'm not sure if we really need to worry about it (running superpmi.py on servicing branches is rare, and so is specifying custom metrics to superpmi.py). I would be fine with changing the syntax here, but also cc @dotnet/jit-contrib for a chance to express their concerns.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want to submit the fix for this too? Specifically around here: https://github.com/dotnet/runtime/blob/db717e30839c532cad5b269ac11c5d2c91dad639/src/coreclr/scripts/superpmi.py#L1790-L1791
Looks like the help message for the
-metricsarg in superpmi.py needs an update too.