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

feat: convert Node.Print to use switch (node.Kind()) #1509

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

TimothyMakkison
Copy link
Contributor

@TimothyMakkison TimothyMakkison commented Feb 24, 2025

  • Use switch (syntaxNode.Kind()) instead of is. Should be slightly faster 🤞
  • Add Debug.Assert in the default branch to ensure that future changes to roslyn / mistakes with SyntaxKind are caught

Benchmarks

Ran FormatAsync in a for loop 10 times to try and average performance. Seems to show a small 1.5% increase in performance at the cost of more complexity and the potential for errors. Do you think its worth merging this pr @belav ? TBH I'm a little dissapointed 😅

I did wonder if using chained or might affect the generated code 🤷 the massive SyntaxKind switch statements in roslyn use several case tags instead. Perhaps this is lowered into better code? Looking at the IL I think it creates a jump table, although I'm not sure - I'm not fluent in IL 😄 .

Before

Method Mean Error StdDev Gen0 Gen1 Gen2 Allocated
Default_CodeFormatter 2.115 s 0.0132 s 0.0103 s 77000.0000 35000.0000 9000.0000 661.69 MB

After

Method Mean Error StdDev Gen0 Gen1 Gen2 Allocated
Default_CodeFormatter 2.085 s 0.0168 s 0.0149 s 77000.0000 35000.0000 9000.0000 661.7 MB

@belav
Copy link
Owner

belav commented Feb 25, 2025

The special cases seem to be a bit painful to manage but I don't really see special cases being changed though, possibly just new ones added as the language evolves. The last few language versions have been really straightforward for CSharpier to support though and a few of them didn't even require any changes beyond updating the CodeAnalysis version. With the extra complexity hidden away I'm not super concerned with it.

I thought CSharpier's code coverage was solid until you uncovered the missing tests for unchecked. I do have a process for formatting everything in csharpier-repos which has caught quite a few bugs. I'm confident that would catch any errors this may introduce.

There may be a tiny benefit to using RawSyntaxKind which I added a while back.

I just heard of FrozenDictionary today although with the dictionary you added only being in the generator I don't know how beneficial it would be.

belav added a commit that referenced this pull request Feb 25, 2025
Noticed that tests weren't failing for #1509 when I forgot to account
for `SyntaxKind.UncheckedStatement`. This will hopefully catch it

Co-authored-by: Bela VanderVoort <[email protected]>
@TimothyMakkison
Copy link
Contributor Author

TimothyMakkison commented Feb 26, 2025

The special cases seem to be a bit painful to manage but I don't really see special cases being changed though, possibly just new ones added as the language evolves. The last few language versions have been really straightforward for CSharpier to support though and a few of them didn't even require any changes beyond updating the CodeAnalysis version. With the extra complexity hidden away I'm not super concerned with it.

Fair enough, for whats its worth I have manually checked each printer type case I missed any SyntaxNode types that don't share their name with a SyntaxKind. It's how I discovered that unchecked didn't have unit tests.

I thought CSharpier's code coverage was solid until you uncovered the missing tests for unchecked. I do have a process for formatting everything in csharpier-repos which has caught quite a few bugs. I'm confident that would catch any errors this may introduce.

Did you change anything with CLI tests? 4 days ago they started failing.
image

There may be a tiny benefit to using RawSyntaxKind which I added a while back.

Good shout, it can't hurt to add it.

I just heard of FrozenDictionary today although with the dictionary you added only being in the generator I don't know how beneficial it would be.

Yeah, I've been looking for an excuse to use it for ages. Unfortunately it would be a bad fit for the generator 😞 . Briefly considered it for Node.Print but the compiler is faster.

@TimothyMakkison TimothyMakkison marked this pull request as ready for review February 26, 2025 00:33
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