Improve OpenApiWalker performance #2470
Conversation
Publish benchmark results to GitHub Actions workflow artifacts.
- Ignore BenchmarkDotNet profiler files. - Ignore Visual Studio profiler session files.
- Avoid allocations from lambda closures. - Avoid allocations from context strings. - Avoid allocations from copying arrays. - Remove redundant null checks.
There was a problem hiding this comment.
Pull Request Overview
This PR optimizes the performance of the OpenApiWalker class by eliminating allocations from lambda closures, string concatenations, and array copying operations. The changes focus on replacing the existing Walk(context, action) pattern with more efficient WalkItem and WalkDictionary helper methods that avoid capturing variables in closures.
Key changes include:
- Replace lambda closures with direct method calls to avoid allocation overhead
- Add specialized
WalkItemandWalkDictionaryhelper methods for efficient traversal - Implement optimized iteration patterns for collections to avoid array copying
- Publish benchmark results as GitHub Actions artifacts for performance tracking
Reviewed Changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/Microsoft.OpenApi/Services/OpenApiWalker.cs | Core performance optimizations replacing lambda-based traversal with direct method calls |
| .github/workflows/ci-cd.yml | Add artifact publishing for benchmark results |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
FYI I ran each of the benchmarks using the Visual Studio profiler to get the allocations, and these are the top types: 
Even more refactoring of the walker in a future PR to remove the delegates would probably significantly reduce the allocations: 
|
|
Some of these might be new from the refactoring actually, so I'll see if I can shave it down more here... |
Make all delegates static.
|
With the latest changes:
|
|
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Resolve two CodeQL warnings.
|
Here's all the relevant benchmark data taken from the CI logs. TL;DR:
10b46b9
90b3966
This PR
|
|
With the factoring suggestions the numbers get slightly worse than without, but it's probably within the realms of noise.
|
- Create helpers to reduce code duplication. - Remove unused method.
|
@baywet Assuming you're otherwise happy with the PR and the changes to the perf numbers, do you want me to take the files from the latest CI run on this PR and check them in as the updated baseline (which should then make the PR green on the next run)? |
Create common implementation to walk `OpenApiTag` and `OpenApiTagReference`.
Remove redundant comment.
|
Latest numbers:
Would you like me to commit the results from that build into this PR before merge? |
|
@martincostello yes please go ahead! |
Add results from 1c49855.
2 participants
Benchmark results: #2459 (comment)
Given that the hardware between my local setup and GitHub Actions is quite different, I suggest merging this PR once happy with the code changes with the failing benchmark comparison, then downloading the results from the run in main and opening a new PR to use those files to replace the contents of
performance/benchmark/BenchmarkDotNet.Artifacts/resultsand update the baseline.Changes