-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Use System.CommandLine (v2) in dotnet-pgo #76467
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
Use System.CommandLine (v2) in dotnet-pgo #76467
Conversation
|
Tagging subscribers to this area: @hoyosjs Issue DetailsAlso delete the (now-unused) internal CommandLine implementation.
|
Co-authored-by: Jakob Botsch Nielsen <[email protected]>
jkotas
left a comment
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.
LGTM. @jakobbotsch Any other feedback?
| mibcReaders[i] = MIbcProfileParser.OpenMibcAsPEReader(commandLineOptions.InputFilesToMerge[i].FullName); | ||
| string path = paths.ElementAt(i).Value; | ||
| PrintMessage($"Opening {path}"); | ||
| mibcReaders[i] = MIbcProfileParser.OpenMibcAsPEReader(path); |
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.
The switch to Dictionary for references and the input .mibc files is concerning to me. It introduces non-determinism into the tool since these are enumerated. It seems like we should have a variant that expands into a List instead.
The fact that ilc/crossgen2 have the same problem for --input-file-path is a little concerning to me too, but I can see that it is preexisting to your changes.
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.
@jakobbotsch, in the existing implementation, we are using the same AppendExpandedPaths helper in:
| List<FileInfo> DefineFileOptionList(string name, string help) |
although it returns a list, the result is equally non-deterministic (across multiple executions with same 'path patterns' arguments), due to the usage of Dictionary internally.
Note that we are (and were) using the same helper in ilc, crossgen2, ilverify and pgo.
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.
I wouldn't call it equally non-deterministic. In the common case without patterns the dictionaries have just one entry for each specified path and thus the existing version ends up being deterministic. This is for instance the case in our internal uses of dotnet-pgo merge in our training scenarios. There we are calling dotnet-pgo merge with a bunch of exact file names. The order was deterministic before but is going to be non-deterministic after this change.
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.
AppendExpandedPaths always uses dictionary.Add(simpleName, fullFileName) even for the exact file names.
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.
Yes, but DefineFileOptionList is creating a new dictionary for each path. Each dictionary ends up having only one entry and the list returned ends with the same order as the order given in the input.
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.
You are right. In ilc and crossgen2, we were passing the same dictionary instance to the helper before
runtime/src/coreclr/tools/aot/crossgen2/Program.cs
Lines 141 to 142 in 9b9b0c9
| foreach (var input in _commandLineOptions.InputFilePaths) | |
| Helpers.AppendExpandedPaths(_inputFilePaths, input, true); |
BuildPathDictionary) helper didn't change the behavior. Pgo is creating a new dictionary for each pattern, which is better.
BTW, this globbing implementation does not support patterns like /path/**/*.mbic, given: /path/1/foo.mbic and /path/2/foo.mbic, it will only give us /path/2/foo.mbic. The better approach would be to use something like MSBuildGlob from this package.
Also delete the (now-unused) internal CommandLine implementation.