Add sync benchmarks#2276
Open
myieye wants to merge 14 commits into
Open
Conversation
github-actions
Bot
added
the
💻 FW Lite
|
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 infrastructure to measure FwLite sync performance, adds two benchmark test suites (first-sync and post-mutation scenarios), integrates benchmarks into CI, and optimizes the database with a composite index on the Commits table. The harmony submodule is also updated. ChangesSync Performance Measurement and Optimization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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)
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 |
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
Contributor
UI unit Tests 1 files 62 suites 33s ⏱️ Results for commit cb89aee. ♻️ This comment has been updated with latest results. |
myieye
force-pushed
the
sync-perf-benchmarks
branch
from
61ea495 to
e1a5eac
Compare
Contributor
C# Unit Tests165 tests 165 ✅ 22s ⏱️ Results for commit cb89aee. ♻️ This comment has been updated with latest results. |
myieye
force-pushed
the
sync-perf-benchmarks
branch
from
e1a5eac to
6082b4a
Compare
github-actions
Bot
added
the
📦 Lexbox
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/fw-lite.yaml (1)
70-97: ⚡ Quick winConsider hardening the benchmark job's security posture.
The static analysis tool flagged several security concerns in the new benchmark job:
- Unpinned actions (lines 78, 81, 93):
actions/checkout@v4,actions/setup-dotnet@v4, andactions/upload-artifact@v4use mutable tags rather than commit SHAs.- Missing
persist-credentials: false(lines 78-80): Credentials may persist in the checked-out repository.- Missing permissions block: The job inherits default broad permissions.
While these issues are consistent with the existing workflow style in this file (other jobs use the same pattern), consider addressing them as part of a broader workflow security hardening effort across the repository.
🔒 Example hardening for the benchmark job
benchmark: name: Run FW Lite sync benchmarks + permissions: + contents: read + actions: read # BenchmarkDotNet timings are only meaningful in Release; this job runs the FwLiteProjectSync.Tests # project in Release with --filter "Category=Benchmark" so threshold assertions actually fire. timeout-minutes: 40 runs-on: windows-latest steps: - name: Checkout - uses: actions/checkout@v4 + uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 with: submodules: true + persist-credentials: false - - uses: actions/setup-dotnet@v4 + - uses: actions/setup-dotnet@3e891f8b4d438e9a0a674af8e3360b3c38e14aa1 # v4.2.0 with: dotnet-version: '10.x'Note: If adopting SHA pinning, apply it consistently across all workflow files.
🤖 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 @.github/workflows/fw-lite.yaml around lines 70 - 97, Update the benchmark job to harden security: replace mutable action tags (actions/checkout@v4, actions/setup-dotnet@v4, actions/upload-artifact@v4) with pinned commit SHAs or fully-qualified refs, add persist-credentials: false to the actions/checkout step to avoid leaking runner credentials, and add an explicit permissions block scoped to the least privileges the job needs (e.g., minimal read permissions or none for contents, and only write/artifact permissions if upload-artifact requires it); apply these changes in the benchmark job steps that reference the Checkout, setup-dotnet, and Upload benchmark results steps so the CI uses pinned actions, does not persist credentials, and runs with minimal permissions.
🤖 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/SyncMutationBenchmark.cs`:
- Around line 96-97: The benchmark's mutation selection is nondeterministic
(using unseeded randomness via AutoFaker/unspecified RNG) causing flaky
thresholds; make mutation generation deterministic by creating a Random seeded
from the benchmark profile (e.g., profile index or name hash) and use it for
selection and shuffling instead of unseeded calls. Add a helper
Shuffle<T>(IReadOnlyList<T> source, Random rng) and use it wherever mutations
are chosen (the mutation setup code around AutoFaker and the blocks referenced
at the mutation generation sites) and, if AutoFaker is used to produce random
values, initialize or replace its RNG with the same seeded Random so the whole
mutation shape is reproducible per profile. Ensure the seed derivation is stable
and deterministic for each profile so CI results are stable.
---
Nitpick comments:
In @.github/workflows/fw-lite.yaml:
- Around line 70-97: Update the benchmark job to harden security: replace
mutable action tags (actions/checkout@v4, actions/setup-dotnet@v4,
actions/upload-artifact@v4) with pinned commit SHAs or fully-qualified refs, add
persist-credentials: false to the actions/checkout step to avoid leaking runner
credentials, and add an explicit permissions block scoped to the least
privileges the job needs (e.g., minimal read permissions or none for contents,
and only write/artifact permissions if upload-artifact requires it); apply these
changes in the benchmark job steps that reference the Checkout, setup-dotnet,
and Upload benchmark results steps so the CI uses pinned actions, does not
persist credentials, and runs with minimal permissions.
🪄 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: a4a521b7-a9b5-420d-b662-ffb766c48d55
📒 Files selected for processing (13)
.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/20260528091629_AddCommitsOrderIndex.Designer.csbackend/FwLite/LcmCrdt/Migrations/20260528091629_AddCommitsOrderIndex.csbackend/FwLite/LcmCrdt/Migrations/LcmCrdtDbContextModelSnapshot.csbackend/harmony
myieye
force-pushed
the
sync-perf-benchmarks
branch
6 times, most recently
from
32f8d6e to
a119903
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.
BenchmarkDotNet has supported async benchmark methods for a long time — the old comment claiming otherwise was wrong, and the .Result blocking call was unnecessary. IterationSetup stays sync (async support there needs BDN 0.16+).
Hosted CI runners are noisy enough that the previous ~3σ thresholds flaked. Bump bounds to catch large regressions only, and rewrite the threshold comments to reflect what we actually believe holds up under run-to-run variance (patch-heavy and first-sync show real speedups; the other profiles are kept as regression guards, not perf claims).
myieye
force-pushed
the
sync-perf-benchmarks
branch
from
a119903 to
f0278fd
Compare
- Pin benchmark job's actions to SHAs (match the rest of the workflow) - Gate the benchmark job to push-on-develop (off the PR critical path) - Surface the last error when complex-form link setup can't reach its target - Fail artifact upload when no benchmark results are produced Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
myieye
force-pushed
the
sync-perf-benchmarks
branch
from
f0278fd to
f45447c
Compare
- Bump the benchmark job timeout to 90 min: the suite re-imports Sena-3 per iteration across all profiles, so it runs well past the 40 min used elsewhere. - Revert benchmark artifact upload to if-no-files-found: warn — with if: always() an error would add a misleading second failure on any build/crash that emits no artifacts, while a threshold-assertion failure still writes artifacts first. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Prior runs (before the job was disabled on feature branches) took ~30 min, worst observed ~34. 60 min clears that plus hosted-runner variance without the excessive headroom of the earlier value. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds BenchmarkDotNet-based sync benchmarks for
FwLiteProjectSyncso we can actually measure sync-performance work. This started as exploration of perf optimizations and gives us a baseline to measure future speedups against.SyncBenchmark— first sync of the full Sena-3 project from an empty CRDT snapshot.SyncMutationBenchmark— post-mutation syncs across several profiles (patch-heavy, reorder, complex-form components, mixed) to stress different change paths.Category=Benchmarktests and uploads BenchmarkDotNet artifacts; the main test job excludes them. Benchmark comments record the approximate speedup from the commits-order index.No schema/migration changes. An earlier revision of this branch added a DB migration for the commits-order index; that was dropped during rebase because the index now ships from the Harmony submodule already on
develop(CommitEntityConfigviaEFCore.ComplexIndexes).dotnet ef migrations has-pending-model-changesis clean.CI trigger: the benchmark job runs post-merge on
developonly — not on PRs (too slow / too variance-prone on shared runners to gate every PR) and not onmain(so it can't delay releases). We can move it onto the PR critical path once it's fast enough.Also note:
Sena3SyncTestsmoved fromIClassFixtureto a shared[Collection]so it serializes with the benchmark classes against the shared on-disk Sena-3 fixture.(Reopening of #2254, which got accidentally merged into develop via the VS Code "Sync" button.)