Skip to content
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

Remove dependencies #2127

Merged
merged 7 commits into from
Mar 29, 2023
Merged

Remove dependencies #2127

merged 7 commits into from
Mar 29, 2023

Conversation

adamsitnik
Copy link
Member

@adamsitnik adamsitnik commented Mar 29, 2023

At the cost of requiring the users to provide explicit custom parser for Uri, IPAddress and IPEndPoint and a removal of non-generic collections support I was able to remove following dependencies:

- System.Private.Uri.dll
- System.Net.Primitives.dll
- System.Collections.dll
- System.Collections.Concurrent.dll
- System.Runtime.Serialization.Formatters.dll

The most tricky part was removing dependency to System.Collections.dll, which was caused by the fact that we were using Stack<T> (it's generic, but not defined in System.Collections.Generic.dll or CoreLib...)

The only dependency that an app that uses S.CL has and a Hello World app does not, is System.Linq (and S.CL of course).

For following app:

using System;
using System.CommandLine;

public class Program
{
    static void Run(bool boolean, string text)
    {
        Console.WriteLine($"Bool option: {text}");
        Console.WriteLine($"String option: {boolean}");
    }

    private static int Main(string[] args)
    {
        Option<bool> boolOption = new Option<bool>("--bool", "-b") { Description = "Bool option" };
        Option<string> stringOption = new Option<string>("--string", "-s") { Description = "String option" };

        RootCommand command = new ()
        {
            boolOption,
            stringOption,
        };

        command.SetAction(ctx => Run(ctx.GetValue(boolOption), ctx.GetValue(stringOption)));

        return new CommandLineConfiguration(command).Invoke(args);
    }
}

And

dotnet publish -c Release -r win-x64 /p:PublishTrimmed=true

The number of files dropped from 34 to 29, size from 17.5 MB to 17.3.
For NativeAOT app the size dropped from 4.65 MB to 4.5 MB.

fixes #1794

Useful link for repeating the process in the future: dotnet/runtime#78671 (comment)

@adamsitnik adamsitnik requested a review from jonsequitur March 29, 2023 18:09
@adamsitnik
Copy link
Member Author

adamsitnik commented Mar 29, 2023

Updated perf numbers for startup scenario benchmarks (https://github.com/adamsitnik/commandline-perf/tree/update):

Default settings

BenchmarkDotNet=v0.13.4, OS=Windows 11 (10.0.22621.1413)
AMD Ryzen Threadripper PRO 3945WX 12-Cores, 1 CPU, 24 logical and 12 physical cores
.NET SDK=8.0.100-alpha.1.22558.1
[Host] : .NET 7.0.4 (7.0.423.11508), X64 RyuJIT AVX2
Job-LDBVHT : .NET 7.0.4 (7.0.423.11508), X64 RyuJIT AVX2

EvaluateOverhead=False OutlierMode=DontRemove InvocationCount=1
IterationCount=100 UnrollFactor=1 WarmupCount=3

Method Args Mean Error StdDev Median Ratio RatioSD
Empty 70.30 ms 1.166 ms 3.437 ms 70.32 ms 0.83 0.05
EmptyAOT 14.95 ms 0.436 ms 1.286 ms 14.67 ms 0.18 0.02
Corefxlab 86.82 ms 1.185 ms 3.493 ms 85.91 ms 1.03 0.04
SystemCommandLine2021 117.35 ms 1.117 ms 3.293 ms 116.69 ms 1.39 0.05
SystemCommandLine2022 97.13 ms 1.120 ms 3.301 ms 96.44 ms 1.15 0.04
SystemCommandLineNow 84.32 ms 0.755 ms 2.227 ms 83.83 ms 1.00 0.00
SystemCommandLineNowR2R 79.25 ms 0.830 ms 2.448 ms 78.71 ms 0.94 0.04
SystemCommandLineNowAOT 15.04 ms 0.242 ms 0.714 ms 14.86 ms 0.18 0.01
Empty --bool -s test 66.51 ms 0.563 ms 1.661 ms 66.47 ms 0.75 0.03
EmptyAOT --bool -s test 13.49 ms 0.221 ms 0.653 ms 13.39 ms 0.15 0.01
Corefxlab --bool -s test 86.60 ms 0.934 ms 2.753 ms 85.86 ms 0.97 0.04
SystemCommandLine2021 --bool -s test 119.95 ms 0.913 ms 2.691 ms 119.66 ms 1.34 0.05
SystemCommandLine2022 --bool -s test 100.62 ms 0.561 ms 1.654 ms 100.22 ms 1.13 0.04
SystemCommandLineNow --bool -s test 89.25 ms 0.869 ms 2.563 ms 88.66 ms 1.00 0.00
SystemCommandLineNowR2R --bool -s test 82.15 ms 0.921 ms 2.717 ms 81.67 ms 0.92 0.04
SystemCommandLineNowAOT --bool -s test 15.03 ms 0.253 ms 0.747 ms 14.93 ms 0.17 0.01

@adamsitnik adamsitnik merged commit 0760996 into dotnet:main Mar 29, 2023
@adamsitnik adamsitnik deleted the fewerDependencies branch March 29, 2023 19:48
Comment on lines +166 to +167
[UnconditionalSuppressMessage("ReflectionAnalysis", "IL3050", Justification = "https://github.com/dotnet/command-line-api/issues/1638")]
[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2091", Justification = "https://github.com/dotnet/command-line-api/issues/1638")]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why the referenced issue is justification for suppressing these warnings. It might be better if we wrote it out here.

I believe you could fix IL2091 by annotating the T with [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)]. See https://learn.microsoft.com/en-us/dotnet/core/deploying/trimming/fixing-warnings#dynamicallyaccessedmembers

{
if (typeof(T).IsArray)
{
return (T?)(object)Array.CreateInstance(typeof(T).GetElementType()!, 0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be able to use dotnet/runtime#76478 once it is implemented in .NET 8. That will allow us to do this without a warning.

genericTypeDefinition == typeof(IList<>) ||
genericTypeDefinition == typeof(ICollection<>))
{
return (T?)(object)Array.CreateInstance(typeof(T).GenericTypeArguments[0], 0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't guaranteed to work in a NativeAOT app. Just because the app uses a IList<double>, it doesn't mean that double[]'s code is going to get preserved/generated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

System.CommandLine defaults have large dependency closure
4 participants