Add commits index and sync benchmarks#2254
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR introduces comprehensive BenchmarkDotNet-based performance testing for CRDT sync operations. It adds benchmark infrastructure (logging, configuration), refactors test fixtures to use xUnit collections, implements two benchmark suites (first-sync and mutation-sync with five mutation profiles), and includes a database index optimization. ChangesBenchmark Infrastructure and Performance Testing
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
the
💻 FW Lite
C# Unit Tests165 tests 146 ✅ 20s ⏱️ For more details on these failures, see this check. Results for commit 53d6f8e. ♻️ This comment has been updated with latest results. |
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
myieye
force-pushed
the
sync-perf-benchmarks
branch
4 times, most recently
from
403f0a7 to
f29fa44
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
rmunn
left a comment
rmunn
left a comment
There was a problem hiding this comment.
Minor change needed: dotnet-version 9.x in the GHA workflow should be 10.x now, and I want to ask about the NuGet package version being outdated. Other than that this looks good.
myieye
force-pushed
the
sync-perf-benchmarks
branch
from
f29fa44 to
da3929a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
backend/FwLite/LcmCrdt/Migrations/20260506150734_AddCommitsOrderIndex.cs (1)
8-10: ⚡ Quick winConsider using EFCore.ComplexIndexes now that .NET 10 is in place.
The comment mentions moving to EFCore.ComplexIndexes "when we're on .NET 10," and that condition is now met. The package (version 2.0.2+) is available and explicitly supports EF Core 10. This would allow replacing the hand-written index with a fluent API approach in the DbContext, making the code more maintainable and aligned with the current framework capabilities. There's also an EFCore.ComplexIndexes.PostgreSQL variant if provider-specific index types are needed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/FwLite/LcmCrdt/Migrations/20260506150734_AddCommitsOrderIndex.cs` around lines 8 - 10, The migration comment in AddCommitsOrderIndex.cs notes a hand‑written index because EFCore lacked complex index support; since .NET 10 is now used, replace the manual migration approach by removing the handwritten index and instead define the index via EFCore.ComplexIndexes in the DbContext (use the ComplexProperty mapping for the ComplexProperty + regular property combination), adding the EFCore.ComplexIndexes (and EFCore.ComplexIndexes.PostgreSQL if needed) package (v2.0.2+) and updating the model configuration where Commit ordering is configured so the fluent API creates the same index instead of the custom migration.backend/FwLite/FwLiteProjectSync.Tests/SyncMutationBenchmark.cs (1)
96-97: ⚡ Quick winMake the mutation workload deterministic per profile.
Each iteration reshuffles entries and re-samples component pairs from a shared RNG, so the same threshold is gating a different workload on every run. That noise is already visible in the high-variance profile comments. Seed the RNG per profile/iteration, or precompute a fixed mutation plan and replay it.
Also applies to: 136-138, 221-238
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/FwLite/FwLiteProjectSync.Tests/SyncMutationBenchmark.cs` around lines 96 - 97, The benchmark uses a shared RNG causing non-deterministic reshuffles and resampling; make each profile's workload deterministic by creating a seeded Random (e.g., seed = Hash(profileName) ^ iterationIndex) inside the SyncMutationBenchmark class and use that RNG instead of the shared one for all shuffles and component-pair sampling (replace calls in the regions around lines referenced, e.g., the shuffling/sampling code at 136-138 and 221-238); alternatively precompute a mutationPlan List for each profile/iteration using AutoFaker and the seeded RNG and replay that plan so the same threshold always gates the same mutation sequence.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/FwLite/FwLiteProjectSync.Tests/SyncBenchmark.cs`:
- Around line 50-83: The SyncFromEmpty benchmark synchronously blocks on an
async call via .Result; change the method to an async benchmark (e.g. make
SyncFromEmpty async Task or async Task<SyncResult> and await
_syncService.Sync(...)) so it uses await instead of .Result; update the
corresponding benchmark in MutationSyncBench the same way; leave
IterationSetup's GetAwaiter().GetResult() as-is unless you opt to upgrade
BenchmarkDotNet to v0.16.0+ to allow async [IterationSetup].
In `@backend/FwLite/FwLiteProjectSync.Tests/SyncMutationBenchmark.cs`:
- Around line 231-245: The bare catch in the block around
api.CreateComplexFormComponent(...) hides all failures; change it to catch only
InvalidOperationException so only validation-related skips are swallowed (the
ones from the .Single() after AddComplexFormComponent), allowing other
exceptions to propagate; locate the try block that calls
api.CreateComplexFormComponent(new ComplexFormComponent { ... }) in
SyncMutationBenchmark.cs and replace the generic catch with catch
(InvalidOperationException) to preserve visibility into infrastructure or API
errors.
---
Nitpick comments:
In `@backend/FwLite/FwLiteProjectSync.Tests/SyncMutationBenchmark.cs`:
- Around line 96-97: The benchmark uses a shared RNG causing non-deterministic
reshuffles and resampling; make each profile's workload deterministic by
creating a seeded Random (e.g., seed = Hash(profileName) ^ iterationIndex)
inside the SyncMutationBenchmark class and use that RNG instead of the shared
one for all shuffles and component-pair sampling (replace calls in the regions
around lines referenced, e.g., the shuffling/sampling code at 136-138 and
221-238); alternatively precompute a mutationPlan List for each
profile/iteration using AutoFaker and the seeded RNG and replay that plan so the
same threshold always gates the same mutation sequence.
In `@backend/FwLite/LcmCrdt/Migrations/20260506150734_AddCommitsOrderIndex.cs`:
- Around line 8-10: The migration comment in AddCommitsOrderIndex.cs notes a
hand‑written index because EFCore lacked complex index support; since .NET 10 is
now used, replace the manual migration approach by removing the handwritten
index and instead define the index via EFCore.ComplexIndexes in the DbContext
(use the ComplexProperty mapping for the ComplexProperty + regular property
combination), adding the EFCore.ComplexIndexes (and
EFCore.ComplexIndexes.PostgreSQL if needed) package (v2.0.2+) and updating the
model configuration where Commit ordering is configured so the fluent API
creates the same index instead of the custom migration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ea880447-924c-44a4-9c7e-5a138ac9e061
📒 Files selected for processing (11)
.github/workflows/fw-lite.yamlbackend/Directory.Packages.propsbackend/FwLite/FwLiteProjectSync.Tests/BenchmarkSupport.csbackend/FwLite/FwLiteProjectSync.Tests/Fixtures/Sena3Collection.csbackend/FwLite/FwLiteProjectSync.Tests/FwLiteProjectSync.Tests.csprojbackend/FwLite/FwLiteProjectSync.Tests/Sena3SyncTests.csbackend/FwLite/FwLiteProjectSync.Tests/SyncBenchmark.csbackend/FwLite/FwLiteProjectSync.Tests/SyncMutationBenchmark.csbackend/FwLite/FwLiteProjectSync.Tests/XUnitBenchmarkLogger.csbackend/FwLite/LcmCrdt/Migrations/20260506150734_AddCommitsOrderIndex.Designer.csbackend/FwLite/LcmCrdt/Migrations/20260506150734_AddCommitsOrderIndex.cs
| // BenchmarkDotNet has no async support. .Result is intentional and will not deadlock. | ||
| #pragma warning disable VSTHRD002 | ||
|
|
||
| public class FirstSyncBench | ||
| { | ||
| // CI initial result: mean 49.5s, StdDev 2.4s (medium variance) => 57s (~3σ above mean) | ||
| // CI with commits order index: mean 44.5s, StdDev 3.5s (medium variance) => 55s (~3σ above mean) | ||
| public const double ThresholdSeconds = 55.0; | ||
|
|
||
| internal static Sena3Fixture Fixture = null!; | ||
|
|
||
| private TestProject _project = null!; | ||
| private ProjectSnapshot _projectSnapshot = null!; | ||
| private CrdtFwdataProjectSyncService _syncService = null!; | ||
|
|
||
| [IterationSetup] | ||
| public void IterationSetup() | ||
| { | ||
| _project = Fixture.SetupProjects().GetAwaiter().GetResult(); | ||
| _syncService = _project.Services.GetRequiredService<CrdtFwdataProjectSyncService>(); | ||
|
|
||
| // Save an empty snapshot so the first sync runs as Sync, not Import. | ||
| ProjectSnapshotService.SaveProjectSnapshot(_project.FwDataProject, ProjectSnapshot.Empty) | ||
| .GetAwaiter().GetResult(); | ||
| _projectSnapshot = _project.Services.GetRequiredService<ProjectSnapshotService>() | ||
| .GetProjectSnapshot(_project.FwDataProject).GetAwaiter().GetResult() | ||
| ?? throw new InvalidOperationException("Expected snapshot to exist after saving"); | ||
| } | ||
|
|
||
| [Benchmark] | ||
| public SyncResult SyncFromEmpty() | ||
| { | ||
| return _syncService.Sync(_project.CrdtApi, _project.FwDataApi, _projectSnapshot).Result; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does BenchmarkDotNet 0.15.8 support async Taskbenchmark methods and async[IterationSetup]/[IterationCleanup] methods?
💡 Result:
Yes, BenchmarkDotNet 0.15.8 supports async Task benchmark methods, as well as async [IterationSetup] and [IterationCleanup] methods [1][2][3]. BenchmarkDotNet has long supported benchmarking asynchronous methods that return Task or ValueTask [2]. Regarding setup and cleanup methods: 1. Global Setup/Cleanup: Support for async [GlobalSetup] and [GlobalCleanup] methods has been available for several versions [4]. 2. Iteration Setup/Cleanup: Support for async [IterationSetup] and [IterationCleanup] methods was recently added and is included in version 0.16.0 [3]. While version 0.15.8 does not include this specific feature (as it was tracked for v0.16.0) [3][5][6], the library generally handles asynchronous operations by awaiting them correctly in most standard execution scenarios [2]. If you encounter issues with async methods not being awaited properly in specific configurations (such as the InProcess toolchain), it is often recommended to ensure you are using the latest version or, as a workaround, to synchronously wait on the task using task.GetAwaiter().GetResult() [7][8].
Citations:
- 1: Reduce async overhead dotnet/BenchmarkDotNet#2111
- 2: Possibility to benchmark asynchronous methods dotnet/BenchmarkDotNet#236
- 3: Please, add async support for iteration setup and cleanup dotnet/BenchmarkDotNet#2442
- 4: Support async Setup/Cleanup dotnet/BenchmarkDotNet#521
- 5: https://github.com/dotnet/BenchmarkDotNet/releases/tag/v0.15.8
- 6: https://benchmarkdotnet.org/changelog/v0.15.8.html
- 7: Async GlobalSetup does not work with InProcess toolchain dotnet/BenchmarkDotNet#1738
- 8: https://stackoverflow.com/questions/76948555/benchmarkdotnet-globalsetup-does-not-handle-async-properly
Convert the SyncFromEmpty() benchmark method to async Task — BenchmarkDotNet 0.15.8 supports async Task benchmarks.
Line 82 uses .Result to synchronously block an async API, violating the backend/**/*.cs guideline "Use async/await for asynchronous operations, not .Result or .Wait()". BenchmarkDotNet 0.15.8 supports async Task benchmark methods natively, so the benchmark can be refactored to public async Task SyncFromEmpty() without requiring the blocking call.
The IterationSetup method (lines 68–75) uses GetAwaiter().GetResult() due to a limitation in BenchmarkDotNet 0.15.8: async [IterationSetup] was not added until v0.16.0. Either upgrade the package to v0.16.0+ to enable async setup, or keep the synchronous workaround if a version upgrade is not feasible.
Apply the same fix to the matching benchmark in MutationSyncBench.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/FwLite/FwLiteProjectSync.Tests/SyncBenchmark.cs` around lines 50 -
83, The SyncFromEmpty benchmark synchronously blocks on an async call via
.Result; change the method to an async benchmark (e.g. make SyncFromEmpty async
Task or async Task<SyncResult> and await _syncService.Sync(...)) so it uses
await instead of .Result; update the corresponding benchmark in
MutationSyncBench the same way; leave IterationSetup's GetAwaiter().GetResult()
as-is unless you opt to upgrade BenchmarkDotNet to v0.16.0+ to allow async
[IterationSetup].
6f28cea to
0d0a59b
Compare
Monitoring strategy with WarmupCount=1, IterationCount=4 — same total work as before, but iteration 1's JIT/EF-model-build/cold-cache cost is discarded instead of folded into the measurement, so StdDev tightens and thresholds can come down to genuine 3σ above the with-index mean. Both comment lines (baseline + with-index) are re-measured under this config so the speedup comparison is apples-to-apples.
myieye
force-pushed
the
sync-perf-benchmarks
branch
from
0d0a59b to
53d6f8e
Compare
|
The sync button in VS Code merged this into develop 🤦. |
2 participants
Adds various sync benchmark tests.
The motivation for this is that I was exploring performance optimizations and having a benchmark in place lets me/us actually measure those optimizations.
This PR includes the first and smallest optimization: a DB index on the commit order columns.
So, now there are 2 Comment lines per benchmark documenting the approximate speedup that the index resulted in.
Hopefully future PRs will add more radical speedups.