perf(runtime): optimize version selector cache under cluster churn - #10946
Conversation
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
It introduces substantial concurrency and caching behavior changes in a core runtime hot path which should receive final human review despite only minor issues found here.
Review tier: Lite
Findings: 2
New issues introduced by this change (2)
| Severity | Finding |
|---|---|
 |
src/Orleans.Runtime/Versions/CachedVersionSelectorManager.cs — GetSuitableSilos wraps its logic in while (true) but every path inside the loop returns, so the… |
|
test/Orleans.Core.Tests/Manifest/ClusterManifestProviderTests.cs — Test method name uses Ipv6 while other tests in this suite use IPv6 (e.g.,… |
What changed in this PR
This PR is a performance-focused follow-up in Orleans runtime version selection/placement, aiming to keep selection snapshot-consistent under cluster churn while significantly reducing contention and allocations in the selector/manifests hot paths.
Changes:
- Reworks
CachedVersionSelectorManagerto use per-key cache coordination (instead of a global lock) and to compute selections against an immutable captured manifest snapshot. - Updates
GrainVersionManifestto expose captured snapshots and uses linear-time manifest indexing plus allocation-light sorted set union/intersection (with IPv6 scope-consistent ordering). - Makes selector/director registries concurrency-safe via
ConcurrentDictionary+Volatile, and updates tests to cover snapshot consistency, reset races, and IPv6 scope behavior.
| File | Description |
|---|---|
| test/Orleans.Core.Tests/Runtime/PlacementServiceTests.cs | Updates test setup to match the new CachedVersionSelectorManager constructor (membership no longer injected). |
| test/Orleans.Core.Tests/Manifest/ClusterManifestProviderTests.cs | Replaces prior membership/manifest convergence retry test with new coverage for manifest snapshot capture consistency, reset race behavior, and IPv6 scope identity. |
| src/Orleans.Runtime/Versions/Selector/VersionDirectorManager.cs | Switches selector registry to concurrency-safe storage and makes default selector updates thread-safe. |
| src/Orleans.Runtime/Versions/Compatibility/CompatibilityDirectorManager.cs | Switches compatibility director registry to concurrency-safe storage and makes default director updates thread-safe. |
| src/Orleans.Runtime/Versions/CachedVersionSelectorManager.cs | Implements per-cache-key coordination + captured manifest snapshots; removes global-lock + membership-synchronized generation logic. |
| src/Orleans.Core/Manifest/GrainVersionManifest.cs | Adds captured snapshot API and optimized sorted set operations; ensures ordering distinguishes scoped IPv6 addresses consistently. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
It changes core concurrency and ordering semantics in runtime placement/version-selection paths, which warrants final human validation despite added tests.
Review tier: Lite
Findings: None
Issues resolved since last review (2)
| Severity | Finding |
|---|---|
|
test/Orleans.Core.Tests/Manifest/ClusterManifestProviderTests.cs — Test method name uses Ipv6 while other tests in this suite use IPv6 (e.g.,… View resolved comment |
|
src/Orleans.Runtime/Versions/CachedVersionSelectorManager.cs — GetSuitableSilos wraps its logic in while (true) but every path inside the loop returns, so the… View resolved comment |
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
test/Orleans.Core.Tests/Manifest/ClusterManifestProviderTests.cs:190
membershipis no longer passed intoCreateCachedVersionSelectorManager, so the membership service setup and subsequentmembership.Update(...)call in this test no longer affect the assertions and can be removed to avoid misleading future maintainers.
This issue also appears on line 213 of the same file.
test/Orleans.Core.Tests/Manifest/ClusterManifestProviderTests.cs:217
membershipis created here but no longer used now thatCachedVersionSelectorManagerno longer depends onIClusterMembershipService. Removing it keeps the test focused on the manifest minor-version behavior being asserted.
using var membership = new TestClusterMembershipService(CreateMembershipSnapshot(
1,
(localSilo, SiloStatus.Active),
(remoteSilo, SiloStatus.Active)));
var selectorManager = CreateCachedVersionSelectorManager(new GrainVersionManifest(clusterManifestProvider));
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The per-key cache update in CachedVersionSelectorManager can allow an older manifest snapshot computation to overwrite a newer cached value, causing avoidable churn-time cache thrash and stale publication.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Lite
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
 |
src/Orleans.Runtime/Versions/CachedVersionSelectorManager.cs — Within the per-key lock, the cache write is gated only on cacheGeneration, so an in-flight… |
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
It changes core runtime placement/versioning concurrency and caching behavior, so it warrants full human review with end-to-end CI validation and churn-focused runtime verification.
Review tier: Lite
Findings: None
Issues resolved since last review (1)
| Severity | Finding |
|---|---|
|
src/Orleans.Runtime/Versions/CachedVersionSelectorManager.cs — Within the per-key lock, the cache write is gated only on cacheGeneration, so an in-flight… View resolved comment |
af4b135 to
a2a4190
Compare
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The concurrency and snapshot-consistency changes appear correct, are covered by targeted new tests, and I did not find any remaining call-site or correctness issues in the updated code paths.
Review tier: Lite
Findings: None
PR #10597 established generation-consistent placement compatibility selection. In large clusters, the synchronized cache path and manifest index construction can dominate placement latency when membership changes frequently.
This follow-up preserves snapshot consistency while improving scaling:
BenchmarkDotNet ShortRun measurements on .NET 10 with 1,000 silos, 32 grain types, and 16 concurrent callers showed:
Microsoft Reviewers: Open in CodeFlow