Skip to content

fix(benchmarks): restore server call admission hot-path benchmark - #790

Merged
SunSi12138 merged 7 commits into
devfrom
fix/756-server-call-admission-benchmark
Oct 9, 2026
Merged

SunSi12138 merged 7 commits into
devfrom
fix/756-server-call-admission-benchmark

Conversation

@SunSi12138

@SunSi12138 SunSi12138 commented Oct 9, 2026 •

Copy link
Copy Markdown
Owner

Fixes #756.

Summary

  • Replace obsolete reflection over SharpLinkServer._connections with _connectionRegistry.SnapshotActive().
  • Use the existing BenchmarkEnvironment.Server property instead of reflecting into its private _server field.
  • Add --server-call-admission-smoke-test to execute GlobalSetup -> AcquireAndRelease -> GlobalCleanup, asserting balanced active calls; run it in allocation-gate CI.
  • Keep the RPC session flush same-machine A/B base worktree compilable when it copies the PR-head Program.cs: copy the corresponding ServerCallAdmissionHotPathBenchmarks.cs as well (addresses review P2).
  • Add .github/workflows/server-call-admission-benchmark-evidence.yml to execute the actual exact-head BenchmarkDotNet workload in Release, validate the generated ns/op and B/op results, and upload measurements and environment metadata (addresses review P3).
  • Document the smoke command.

No changes to production src/, public API, or wire protocol.

Validation

Local commands

dotnet run --project test/SharpLink.Benchmarks/SharpLink.Benchmarks.csproj -c Release -- --server-call-admission-smoke-test
dotnet run --project test/SharpLink.Benchmarks/SharpLink.Benchmarks.csproj -c Release -- --filter '*ServerCallAdmissionHotPathBenchmarks*' --exporters json

@SunSi12138 SunSi12138 left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review result: changes needed.

The benchmark setup fix itself looks correct and the new smoke passes on the PR head, but this PR currently breaks an existing same-machine A/B workflow.

RPC Session Flush Performance Evidence run 37878976018 fails deterministically while building the exact base benchmark harness: that workflow copies the PR-head Program.cs into the base worktree but does not copy the updated ServerCallAdmissionHotPathBenchmarks.cs, so the base build sees the new RunSmokeTestAsync() call without that member and fails with CS0117 before any measurements run.

Please update the A/B harness preparation so candidate-only CLI entry points do not make the historical/base worktree uncompilable, then rerun the workflow.

Separately, #756's completion definition asks for the actual BenchmarkDotNet run to produce ns/op and allocation data. The new smoke is useful for setup regression coverage, but it only executes Setup -> one AcquireAndRelease -> Cleanup. Before closing #756 / marking this ready, please attach at least one exact-head full ServerCallAdmissionHotPathBenchmarks run (or add an appropriate non-fast validation) showing that the complete BDN lifecycle and MemoryDiagnoser output work.

Comment thread test/SharpLink.Benchmarks/Program.cs
Comment thread .github/actions/allocation-gate/action.yml
@SunSi12138
SunSi12138 marked this pull request as ready for review October 9, 2026 03:52
@SunSi12138
SunSi12138 merged commit 072a3a1 into dev Oct 9, 2026
35 checks passed
@SunSi12138
SunSi12138 deleted the fix/756-server-call-admission-benchmark branch October 9, 2026 03:58
SunSi12138 added a commit that referenced this pull request Oct 9, 2026
…ontexts

Sync dev after #790. Detach notification dispatch from owned result/error state,
release payload and context roots at consumption, preserve async-wrapper
ExecutionContext and scheduling semantics, and throw faults outside the gate.

Add deterministic retention, context, scheduler, reentrancy, and exception-filter
regressions. Keep #753 as Draft research; issue #740 No-Go remains unchanged.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant