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 one layer of abstraction and multiple intermediate types #2040

Merged
merged 2 commits into from
Feb 2, 2023

Conversation

adamsitnik
Copy link
Member

Explanation: In #2033 I've moved most of the validation logic to the CommandResult, making ParseResultVisitor a very simple class. This included moving all hacks from ParseResultVisitor to earlier stages of parsing. Thanks to that, we can make ParseOperation create SymbolResult instances directly, rather than creating intermediate SyntaxNode instaces and later letting ParseResultVisitor to map it to SymbolResult instances.

This has a really nice impact on startup performance. The number of JIT compiled methods drops from 221 to 198, the startup improves by 1-2ms for the simple case scenario.

Method Args Mean Ratio
Empty 67.62 ms 0.81
EmptyAOT 13.27 ms 0.16
Corefxlab 83.71 ms 1.00
SystemCommandLine2021 101.97 ms 1.22
SystemCommandLine2022 84.46 ms 1.01
SystemCommandLineNow 83.60 ms 1.00
SystemCommandLineNowR2R 77.46 ms 0.93
SystemCommandLineNowAOT 12.81 ms 0.15
Empty --bool -s test 64.42 ms 0.73
EmptyAOT --bool -s test 12.57 ms 0.14
Corefxlab --bool -s test 84.14 ms 0.95
SystemCommandLine2021 --bool -s test 105.74 ms 1.19
SystemCommandLine2022 --bool -s test 90.46 ms 1.02
SystemCommandLineNow --bool -s test 88.85 ms 1.00
SystemCommandLineNowR2R --bool -s test 80.65 ms 0.91
SystemCommandLineNowAOT --bool -s test 12.99 ms 0.15

The allocations drop is also very nice: few hundreds of bytes for simple scenario.

Copy link
Member

@Keboo Keboo left a comment

Choose a reason for hiding this comment

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

Very nice work

@adamsitnik adamsitnik merged commit c95d825 into dotnet:main Feb 2, 2023
@adamsitnik adamsitnik deleted the removeLayer2 branch February 2, 2023 17:07
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.

2 participants