Reduce allocations

[DoctorKrolic]

Roslyn nodes are immutable, meaning that creating them "step by step" using .With... or .Add... methods allocates quite a lot of waste. So I refactored internal factory methods to take most common node parts as parameters directly and used collection expressions and params to avoid intermediate allocations arrays of nodes by replacing them with creating syntax lists directly. I believe this also makes code simpler and easier to follow.

I ran the full set of tests, got a failure with unresolved Marshal name (why didn't CI catch it earlier?), so also added qualification for it. I don't see value in separating this change, it is very little

[jevansaks]

/azp run

[jevansaks]

D:\a\1\s\src\Microsoft.Windows.CsWin32\Generator.Struct.cs(416,13): error SA1515: Single-line comment should be preceded by blank line (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1515.md) [D:\a\1\s\src\Microsoft.Windows.CsWin32\Microsoft.Windows.CsWin32.csproj]

[jevansaks]

why didn't CI catch it earlier

The CI pipelines don't run the full set of tests because they don't have enough memory. :|

[DoctorKrolic]

Single-line comment should be preceded by blank line

This is a comment above a collection expression element here: https://github.com/microsoft/CsWin32/pull/1620/changes#diff-c2092e68e35fb17337bed2f51873aed87b806bd730c2976081ced52e47482ec3R416. In this particular case I don't agree with the rule. Should I just suppress it?

The CI pipelines don't run the full set of tests because they don't have enough memory. :|

Then we are on the right track)

[jevansaks]

In this particular case I don't agree with the rule. Should I just suppress it?

No. We only suppress these rules in the generated files and some test files. Elsewhere we follow them. Just put the comment back where it was.

[jevansaks]

Is the impact of this change measurable or meaningful? I'm not necessarily opposed, but eventually we will move away from this as a source generator and go exclusively to the build task version and so the overall allocations will matter quite a lot less in that world.

Especially if your plan is to continue to try to clean up the code to address this, I'd like for you to set expectations about what your plan is. It's not worth our time on such large cleanup changes unless there's an impactful end goal we're working towards.

[DoctorKrolic]

Is the impact of this change measurable or meaningful?

I find it challenging to accuratly measure impact on the generator, but on my machine FullGenerationTests.Everything now finishes around ~3 seconds faster and consistently reports less heap usage in test output (although I believe this includes only actual heap size at the end of the test, so it has been GC'd multiple times already by then).

It is realtively easy to benchmark individual changes thouth. For instance, this one:

using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CSharp;
using Microsoft.CodeAnalysis.CSharp.Syntax;

BenchmarkRunner.Run<Benchmarks>();

[MemoryDiagnoser(false)]
public class Benchmarks
{
    private StatementSyntax _statement;

    [GlobalSetup]
    public void Setup()
    {
        _statement = SyntaxFactory.EmptyStatement();
    }

    [Benchmark]
    public BlockSyntax CreateBlockByAdding()
    {
        return SyntaxFactory.Block().AddStatements(_statement);
    }

    [Benchmark]
    public BlockSyntax CreateBlockDirectly()
    {
        return SyntaxFactory.Block(new SyntaxList<StatementSyntax>(_statement));
    }
}

My results:

BenchmarkDotNet v0.15.8, Windows 11 (10.0.26100.7623/24H2/2024Update/HudsonValley)
AMD Ryzen 7 5800X 3.80GHz, 1 CPU, 16 logical and 8 physical cores
.NET SDK 10.0.102
  [Host]     : .NET 10.0.2 (10.0.2, 10.0.225.61305), X64 RyuJIT x86-64-v3
  DefaultJob : .NET 10.0.2 (10.0.2, 10.0.225.61305), X64 RyuJIT x86-64-v3


| Method              | Mean     | Error    | StdDev   | Allocated |
|-------------------- |---------:|---------:|---------:|----------:|
| CreateBlockByAdding | 86.61 ns | 0.720 ns | 0.673 ns |     440 B |
| CreateBlockDirectly | 11.38 ns | 0.090 ns | 0.084 ns |     120 B |

Especially if your plan is to continue to try to clean up the code to address this

My plan was to not heavily invest into perf, but rather collect low-hanging fruit and leave it in this sate. And as I mentioned in the PR description, IMHO this improves reading in most cases. E.g. it is easier to follow Block(stetement1, statement2) than Block().AddStatements(statement1, statement2), esp. when multiple such changes happen in a large node construction expression, don't you agree?