-
-
Notifications
You must be signed in to change notification settings - Fork 111
feat: source generator benchmarks #4579
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
Conversation
AI-Generated AnalysisI investigated this PR and identified the root causes of the issues. Here's what needs to be fixed: Root Cause 1: Missing Microsoft.Build.Locator (PRIMARY CAUSE)MSBuildWorkspace requires From Mapperly's working benchmark (which you referenced), they call: // MUST be called before any MSBuild types are accessed
MSBuildLocator.RegisterDefaults();
// Then create workspace
var workspace = MSBuildWorkspace.Create();Root Cause 2: Roslyn Version MismatchesThe PR has inconsistent Roslyn versions in the benchmark project:
All Microsoft.CodeAnalysis. packages must use the same version.* Root Cause 3: Unnecessary Changes to Existing ProjectsThe PR modifies existing generator projects which shouldn't be needed:
These changes break TUnit's multi-Roslyn version architecture and should be reverted. Recommended Fixes1. Add Microsoft.Build.Locator to Directory.Packages.props<PackageVersion Include="Microsoft.Build.Locator" Version="1.6.10" />2. Fix TUnit.SourceGenerator.Benchmarks.csprojUse consistent versions (4.12.0 matches the existing CSharp.Workspaces version in Directory.Packages.props): <Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<OutputType>Exe</OutputType>
<TargetFramework>net9.0</TargetFramework>
<ImplicitUsings>enable</ImplicitUsings>
<Nullable>enable</Nullable>
<EmitCompilerGeneratedFiles>true</EmitCompilerGeneratedFiles>
</PropertyGroup>
<ItemGroup>
<PackageReference Include="BenchmarkDotNet" />
<!-- Critical: MSBuild.Locator is required for MSBuildWorkspace -->
<PackageReference Include="Microsoft.Build.Locator" />
<!-- All CodeAnalysis packages must be the same version -->
<PackageReference Include="Microsoft.CodeAnalysis.Workspaces.MSBuild" VersionOverride="4.12.0" />
<PackageReference Include="Microsoft.CodeAnalysis.CSharp.Workspaces" PrivateAssets="all" />
<PackageReference Include="Microsoft.CodeAnalysis.CSharp" PrivateAssets="all" VersionOverride="4.12.0" />
<PackageReference Include="Microsoft.CodeAnalysis.Common" PrivateAssets="all" VersionOverride="4.12.0" />
</ItemGroup>
<ItemGroup>
<ProjectReference Include="..\TUnit.Core.SourceGenerator\TUnit.Core.SourceGenerator.csproj" />
<ProjectReference Include="..\TUnit.Core\TUnit.Core.csproj" />
</ItemGroup>
</Project>3. Fix Program.cs - Add MSBuildLocator Initializationusing System.Runtime.CompilerServices;
using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Loggers;
using BenchmarkDotNet.Running;
using Microsoft.Build.Locator;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CSharp;
using Microsoft.CodeAnalysis.MSBuild;
using TUnit.Core.SourceGenerator.Generators;
// CRITICAL: Must be called before ANY MSBuild types are accessed
// Must be in a separate method that's called first to avoid JIT loading the types early
InitializeMSBuild();
BenchmarkRunner.Run<TestMetadataGeneratorBenchmarks>();
static void InitializeMSBuild()
{
MSBuildLocator.RegisterDefaults();
}
[MemoryDiagnoser]
public class TestMetadataGeneratorBenchmarks
{
private const string SampleProjectPath = "../TUnit.TestProject/TUnit.TestProject.csproj";
private MSBuildWorkspace? _workspace;
private GeneratorDriver? _sampleDriver;
private Compilation? _sampleCompilation;
private static string GetDirectoryRelativePath(string projectPath, [CallerFilePath] string callerFilePath = null!) =>
Path.Combine(Path.GetDirectoryName(callerFilePath)!, projectPath);
private async Task<(Compilation, CSharpGeneratorDriver)> SetupAsync(string projectPath)
{
_workspace = MSBuildWorkspace.Create();
_workspace.WorkspaceFailed += (sender, args) =>
{
ConsoleLogger.Default.WriteLineError("-------------------------");
ConsoleLogger.Default.WriteLineError(args.Diagnostic.ToString());
ConsoleLogger.Default.WriteLineError("-------------------------");
};
var projectFile = GetDirectoryRelativePath(projectPath);
if (!File.Exists(projectFile))
throw new Exception($"Project doesn't exist at: {projectFile}");
ConsoleLogger.Default.WriteLine($"Project exists at {projectFile}");
Project project;
try
{
ConsoleLogger.Default.WriteLine("Loading project\n");
project = await _workspace.OpenProjectAsync(projectFile);
ConsoleLogger.Default.WriteLine("\nLoaded project");
}
catch (Exception ex)
{
ConsoleLogger.Default.WriteError($"Error: {ex.Message}");
throw;
}
var compilation = await project.GetCompilationAsync();
if (compilation == null)
throw new InvalidOperationException("Compilation returned null");
var generator = new TestMetadataGenerator().AsSourceGenerator();
var driver = CSharpGeneratorDriver.Create([generator], parseOptions: (CSharpParseOptions)project.ParseOptions!);
return (compilation, driver);
}
[GlobalSetup(Target = nameof(Compile))]
public void SetupCompile() => (_sampleCompilation, _sampleDriver) = SetupAsync(SampleProjectPath).GetAwaiter().GetResult();
[Benchmark]
public GeneratorDriver Compile() => _sampleDriver!.RunGeneratorsAndUpdateCompilation(_sampleCompilation!, out _, out _);
[GlobalCleanup]
public void Cleanup()
{
_workspace?.Dispose();
}
}4. Revert Changes to Existing ProjectsRemove the changes to:
Key Points
|
I'll try doing this, I don't think I've done this before, not sure what the difference is this time, It appears I need to import more packages with special exclusions.
👍
I've tried this along with not using overrides, former didn't do much, latter just caused conflicts between |
|
Added Same error message 😢 |
Follow-up: The Real FixAfter looking at your latest error more closely and comparing with Mapperly's working benchmark, I found the actual issue. The Problem: BenchmarkDotNet runs benchmarks out-of-process by default. It:
The auto-generated project doesn't include the The Fix: Mapperly uses the [ArtifactsPath("artifacts")]
[MemoryDiagnoser]
[InProcess] // <-- Add this!
public class TestMetadataGeneratorBenchmarksThis tells BenchmarkDotNet to run the benchmark in the same process, avoiding the need to generate and compile a separate project entirely. Correction to my earlier comment:
|
|
Thank you so much for your help 🙏 with a bit of tweaking it appears to run now!
I'll clean it up tomorrow and see what else I should add, is the name of project okay etc? |
|
No problem 😄 Will this benchmark all source generators, or just the Core? If all, naming looks good to me. If it's just Core, I'd probably put Core in the name too, as the Assertions library also has a source generator |
I was thinking it could be used for all source generators in the solution (if possible). I'm hoping it will be easy to a benchmark for each source generator as needed. I just need to make the build logic generic |
f249d87 to
ee78800
Compare
|
Okay, should be good to go, I've made the build logic completely reusable so it can be used for the other projects 👍 |
SummaryAdds a benchmark project for measuring TUnit source generator performance, now working after resolving assembly reference issues. Critical IssuesNone found ✅ Suggestions1. Update PR titleThe title says "feat: add broken source gen benchmarks" but per the comments, the benchmarks are now working. Consider updating to "feat: add source generator benchmarks" or similar. 2. Consider TargetFramework versionThe project targets 3. Add Directory.Packages.props entry for Microsoft.Build.LocatorThe comments mentioned adding <PackageVersion Include="Microsoft.Build.Locator" Version="1.7.1" />4. Minor: Whitespace changesThe changes to Previous Review StatusThe maintainer (thomhurst) provided extensive troubleshooting help in comments. Key issues resolved:
Verdict✅ APPROVE - No critical issues. The suggestions are optional improvements. The project structure looks good and follows the patterns from similar projects (Mapperly, Mediator, Refit) that were referenced. |
Trying to add benchmarks for the source generators, I've been able to do this for Mapperly, Mediator and Refit, but I can't get it to work here.
I'd welcome someone else to try, I think it's an issue with the references.
Error I usually get