command-line-api
command-line-api copied to clipboard
System.CommandLine defaults have large dependency closure
Repro
dotnet publish -c Release -r win-x64 /p:PublishTrimmed=true
using System.CommandLine;
internal class Program
{
private static int Main(string[] args)
{
var fileOption = new Option<FileInfo?>(
name: "--file",
description: "The file to read and display on the console.");
var rootCommand = new RootCommand("Sample app for System.CommandLine");
rootCommand.AddOption(fileOption);
rootCommand.SetHandler(
(FileInfo file) => ReadFile(file),
fileOption);
return rootCommand.Invoke(args);
}
private static void ReadFile(FileInfo file)
=> File.ReadLines(file.FullName).ToList()
.ForEach(line => Console.WriteLine(line));
}
Actual result:
The trimmed application has referenced to many types and binaries that should not be needed:
System.Private.Uri.dll
System.Net.Primitives.dll
System.ObjectModel.dll
System.ComponentModel.dll
System.ComponentModel.Primitives.dll
System.ComponentModel.TypeConverter.dll
System.Collections.NonGeneric.dll
System.Runtime.Serialization.Formatters.dll
System.Diagnostics.Process.dll
...
Expected result:
The trimmed application should only reference types and binaries that are actually needed.
Context: https://github.com/dotnet/runtime/pull/72082#issuecomment-1185706575
We have made some progress recently, but we are definitely not done yet:
System.Private.Uri.dll
System.Net.Primitives.dll
- System.ObjectModel.dll
- System.ComponentModel.dll
- System.ComponentModel.Primitives.dll
- System.ComponentModel.TypeConverter.dll
- System.Collections.NonGeneric.dll
System.Runtime.Serialization.Formatters.dll
- System.Diagnostics.Process.dll
My guess is that all of the redundant references come from here:
https://github.com/dotnet/command-line-api/blob/c63e231c1e5102c51bd9247a4f3feec9175d601d/src/System.CommandLine/Binding/ArgumentConverter.StringConverters.cs#L14
which is just a dictionary of parsers that in theory can be used by the app:
https://github.com/dotnet/command-line-api/blob/c63e231c1e5102c51bd9247a4f3feec9175d601d/src/System.CommandLine/Binding/ArgumentConverter.StringConverters.cs#L172-L182
Since Argument
and Option
are generic now, we could introduce a parse method similar to:
internal bool TryParse(string token, out T value)
{
if (typeof(T) == typeof(IPAddress))
{
if (IPAddress.TryParse(token, out var ip))
{
value = (T)(object)ip;
return true;
}
value = default;
return false;
}
// else if (typeof(T)...
}
@jkotas Does trimmer recognize such typeof checks? If not, how can I get rid of these references?
@adamsitnik, very nice! Should/can we update https://github.com/dotnet/runtime/blob/f561a058b13523c4b045803c9876e823b4cfe0fc/eng/Versions.props#L174 to latest revision? It's four months old and seems like we don't have a darc subscription in runtime repo for command-line-api (dotnet/format, dotnet/sdk etc. have such a subscription).
Does trimmer recognize such typeof checks?
Trimmer does not recognize patterns like these.
If not, how can I get rid of these references?
This is yet another deserializer. You can look at e.g. how we have solved this problem for System.Text.Json deserializer. The general shape of the solution is:
- Avoid collections of built-in deserializers or discovering deserializers using reflection.
- Expose each built-in deserializer as a public entrypoint with concrete types.
- Use source generators to generate the glue code if the entrypoints with concrete types are cumbersome to use.
Should/can we update
We should, but I should not do it myself, as I introduced all the breaking changes and they are reasonable and easy to fix for me.
I am about to write a doc that lists all the breaking changes, explains the reasoning behind them and how to solve the issues.
@am11 would you be interested in updating dotnet/runtime once I have the doc ready? And share some feedback about whether the changes are move in the right direction or not?
With #2127 the only dependency is System.Linq
And updated perf numbers for startup scenario with default settings (all features enables, https://github.com/adamsitnik/commandline-perf/tree/update):
Method | Args | Mean | Error | StdDev | Median | Ratio | RatioSD |
---|---|---|---|---|---|---|---|
Empty | 65.59 ms | 0.297 ms | 0.875 ms | 65.44 ms | 0.79 | 0.01 | |
EmptyAOT | 12.95 ms | 0.132 ms | 0.388 ms | 12.88 ms | 0.16 | 0.01 | |
Corefxlab | 82.84 ms | 0.347 ms | 1.022 ms | 82.77 ms | 1.00 | 0.02 | |
SystemCommandLine2021 | 113.17 ms | 0.704 ms | 2.075 ms | 112.73 ms | 1.36 | 0.03 | |
SystemCommandLine2022 | 94.72 ms | 0.420 ms | 1.240 ms | 94.37 ms | 1.14 | 0.02 | |
SystemCommandLineNow | 82.94 ms | 0.346 ms | 1.019 ms | 82.81 ms | 1.00 | 0.00 | |
SystemCommandLineNowR2R | 77.37 ms | 0.357 ms | 1.054 ms | 77.22 ms | 0.93 | 0.02 | |
SystemCommandLineNowAOT | 14.55 ms | 0.118 ms | 0.348 ms | 14.54 ms | 0.18 | 0.00 | |
Empty | --bool -s test | 65.06 ms | 0.409 ms | 1.205 ms | 64.84 ms | 0.74 | 0.02 |
EmptyAOT | --bool -s test | 13.01 ms | 0.139 ms | 0.411 ms | 12.94 ms | 0.15 | 0.00 |
Corefxlab | --bool -s test | 84.97 ms | 0.791 ms | 2.333 ms | 84.66 ms | 0.97 | 0.03 |
SystemCommandLine2021 | --bool -s test | 117.71 ms | 0.624 ms | 1.841 ms | 117.39 ms | 1.34 | 0.02 |
SystemCommandLine2022 | --bool -s test | 99.46 ms | 0.375 ms | 1.107 ms | 99.15 ms | 1.14 | 0.02 |
SystemCommandLineNow | --bool -s test | 87.58 ms | 0.332 ms | 0.979 ms | 87.46 ms | 1.00 | 0.00 |
SystemCommandLineNowR2R | --bool -s test | 79.16 ms | 0.341 ms | 1.006 ms | 79.17 ms | 0.90 | 0.02 |
SystemCommandLineNowAOT | --bool -s test | 14.88 ms | 0.170 ms | 0.503 ms | 14.85 ms | 0.17 | 0.01 |
Expose each built-in deserializer as a public entrypoint with concrete types.
This doesn't seem to have been done, so anyone using Argument<Uri> will have to set a custom parser function and custom localization of error messages.
This doesn't seem to have been done, so anyone using Argument<Uri> will have to set a custom parser function and custom localization of error messages.
That is true. But in the long term (.NET 9) we are going to provide a source-generator for System.CommandLine and all parsable types will be supported out of the box.