Reduce per-call allocations in ChannelRecreatingCallInvoker outcome tracking#782
Open
berndverst wants to merge 2 commits into
Open
Reduce per-call allocations in ChannelRecreatingCallInvoker outcome tracking#782berndverst wants to merge 2 commits into
berndverst wants to merge 2 commits into
Conversation
…racking ObserveOutcome previously passed a (this, methodFullName) ValueTuple as the object? state argument to Task.ContinueWith, which boxes the tuple on every unary RPC. It also relied on a non-capturing lambda for the continuation. Cache a single per-instance Action<Task, object?> delegate (onUnaryCallCompleted) created once in the constructor, and pass the method-name string directly as ContinueWith state (already a reference type, so no boxing). ObserveOutcome now takes a non-generic Task parameter so the call resolves to Task's inherited non-generic ContinueWith(Action<Task, object?>, object?, ...) overload for both generic and non-generic Task-returning calls. Success/failure classification, method-name diagnostics, TaskContinuationOptions.ExecuteSynchronously + TaskScheduler.Default scheduling, thread safety, channel lifecycle/disposal behavior, and the public API are all unchanged -- ObserveOutcome/OnUnaryCallCompleted are private implementation details. A standalone allocation comparison (200,000 iterations, Release, warmed up) showed a ~14.8% reduction in per-call allocation (216 -> 184 bytes/call) from removing the boxed tuple; the remainder is inherent to Task.ContinueWith's internal continuation/Task bookkeeping and out of scope per the issue. Added focused regression tests to GrpcDurableTaskClientChannelRecreationTests.cs covering: failure-count reset after an interleaved success, DeadlineExceeded suppression on long-poll methods, DeadlineExceeded counting/triggering recreate on regular methods, and cached-delegate identity across multiple calls. Fixes #777 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3c971e29-a4bb-494b-b272-dce0c7aede23
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| TaskScheduler.Default); | ||
| } | ||
|
|
||
| void OnUnaryCallCompleted(Task task, object? state) |
Contributor
There was a problem hiding this comment.
Pull request overview
Reduces allocation overhead in the gRPC client’s channel-recreation outcome tracking by removing per-unary-call boxing/closure creation in ChannelRecreatingCallInvoker, and adds targeted regression tests to validate failure-counting behavior and delegate reuse.
Changes:
- Cache the unary-call completion continuation delegate once per
ChannelRecreatingCallInvokerinstance and pass the method name as astringcontinuation state to avoid boxed tuple allocations. - Refactor unary outcome observation into a dedicated
OnUnaryCallCompleted(Task, object?)method. - Add regression tests covering success/failure streak behavior,
DeadlineExceededallow-list behavior, and cached-delegate reuse.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/Client/Grpc/ChannelRecreatingCallInvoker.cs | Avoids per-unary-call tuple boxing and per-call delegate allocation by reusing a cached continuation delegate and passing method name as string state. |
| test/Client/Grpc.Tests/GrpcDurableTaskClientChannelRecreationTests.cs | Adds regression tests for consecutive-failure tracking, allow-listed timeouts, recreate triggering, and delegate instance reuse. |
Comment on lines
+41
to
+45
| // Cached once per invoker instance (instead of once per RPC) so ObserveOutcome's ContinueWith call | ||
| // does not allocate a new delegate for every unary call. The method-name diagnostic is threaded | ||
| // through as the ContinueWith `state` argument (a string, already a reference type) instead of a | ||
| // boxed (self, methodName) tuple, eliminating the per-call heap allocation entirely. | ||
| readonly Action<Task, object?> onUnaryCallCompleted; |
Comment on lines
+249
to
+253
| // Arrange/Act: the outcome-observation delegate must be created exactly once per invoker | ||
| // instance (in the constructor) and reused for every call, rather than allocated per RPC. | ||
| GrpcChannel channel = GrpcChannel.ForAddress("http://cached-delegate.client.test"); | ||
| GrpcDurableTaskClientOptions options = new() { Channel = channel }; | ||
| options.SetChannelRecreator((existingChannel, ct) => Task.FromResult(existingChannel)); |
AsyncUnaryCall_MultipleCalls_ReuseSameCachedOutcomeDelegateInstance used GrpcChannel.ForAddress against a real (unresolvable) hostname with no deadline and a broad catch, which could hang or behave unpredictably in CI depending on DNS/proxy/network availability. Switch to the same deterministic CallbackHttpMessageHandler pattern used by the other tests in this file (always returns Unavailable), add an explicit short deadline, and narrow the catch to the expected RpcException. The delegate-identity assertion under test is unchanged. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3c971e29-a4bb-494b-b272-dce0c7aede23
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
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.
Fixes #777
Problem
ChannelRecreatingCallInvoker.ObserveOutcome<TResponse>passed a(this, methodFullName)ValueTuple<ChannelRecreatingCallInvoker, string>as theobject? stateargument toTask.ContinueWith. Sincestateis typedobject?, this boxes the tuple struct on every unary RPC call.Fix
readonly Action<Task, object?> onUnaryCallCompletedfield, initialized once in the constructor tothis.OnUnaryCallCompleted(an instance-method group), instead of allocating a lambda + tuple per call.ObserveOutcometo take a non-genericTaskparameter (leveraging the fact thatTask<TResponse>implicitly converts toTask) so the call resolves toTask's inherited non-genericContinueWith(Action<Task, object?>, object?, CancellationToken, TaskContinuationOptions, TaskScheduler)overload.stateargument as astring— already a reference type, so no boxing occurs converting it to/fromobject?.OnUnaryCallCompleted(Task, object?)instance method.What's preserved (unchanged)
RanToCompletion→RecordSuccess();RpcExceptioninner exception →RecordFailure(statusCode, methodFullName)).RecordFailure.TaskContinuationOptions.ExecuteSynchronously+TaskScheduler.Defaultscheduling (no sync-context capture).RecordSuccess/RecordFailureinternals untouched).ObserveOutcome/OnUnaryCallCompletedare private implementation details; no public surface changed.Allocation impact
A standalone scratch benchmark (not committed; 200,000 iterations, Release build, JIT warm-up,
GC.GetAllocatedBytesForCurrentThread) comparing the old boxed-tupleContinueWithcall against the new cached-delegate call showed:The remaining ~184 bytes/call is inherent to
Task.ContinueWith's internal continuation/Taskbookkeeping and is out of scope for this change (the issue explicitly calls for removing the boxed tuple/continuation overhead without a broader rewrite of the completion mechanism).Tests
Added four focused regression tests to
GrpcDurableTaskClientChannelRecreationTests.cs:AsyncUnaryCall_SuccessAfterFailure_ResetsConsecutiveFailureCountAndSuppressesRecreate— verifies an interleaved success resets the consecutive-failure streak and the recreator is not invoked.AsyncUnaryCall_DeadlineExceededOnAllowedLongPollMethod_DoesNotCountTowardThreshold— verifiesDeadlineExceededon the long-poll allow-listed method never counts toward the recreate threshold.AsyncUnaryCall_DeadlineExceededOnRegularMethod_CountsTowardThresholdAndTriggersRecreate— verifiesDeadlineExceededon a regular method does count and triggers a channel recreate.AsyncUnaryCall_MultipleCalls_ReuseSameCachedOutcomeDelegateInstance— verifies the sameonUnaryCallCompleteddelegate instance is reused across multiple RPCs (proving no per-call delegate allocation).All 51 tests in
Client.Grpc.Testspass (dotnet test test\Client\Grpc.Tests\Client.Grpc.Tests.csproj).Scope
This PR touches only
src/Client/Grpc/ChannelRecreatingCallInvoker.csand its dedicated test file, per the issue's scope. No other issues/changes are stacked on this branch.