Skip to content

Remove one layer of abstraction and multiple intermediate types #2039

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

Closed
wants to merge 16 commits into from

Conversation

adamsitnik
Copy link
Member

It's #2033 + one commit: 1aa6937 I did not want to include it in #2023 as it would make reviewing hard. I am going to make it a draft for now and make it ready for review when #2033 is merged.

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 allocaitons drop is also very nice: few hundreds of bytes for simple scenario.

adamsitnik and others added 16 commits January 24, 2023 13:34
* rename ReportError to AddError
* test coverage improvement (multiple errors for one result)
* move CommandResult validation logic to CommandResult type
* simplify command handler validation
* don't search for ArgumentResult when we know if up front
…should be None

improve perf: don't use .Single as it's on hot path and here we know it's only 1 (Arity has been validated)
remove null checks as it's internal API and we are covered by nullable annotations
@adamsitnik
Copy link
Member Author

Closing in favor of #2040

@adamsitnik adamsitnik closed this Feb 2, 2023
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.

1 participant