perf(codegen): pool generated IInvokable request objects - #10070
perf(codegen): pool generated IInvokable request objects#10070ReubenBond wants to merge 7 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces pooling for generated IInvokable request objects (primarily for non-generic grain methods) and extends the Orleans runtime messaging pipeline to correctly manage request-body ownership/disposal so pooled invokables are not reused too early.
Changes:
- Added
InvokablePool<T>(thread-local, instance-scoped pool) and registered it with serializer services for DI injection into generated activators. - Updated source generation/proxies/codecs/copiers so invokables are created via cached activators and returned to their pool on
Dispose(). - Updated messaging/runtime send/receive/local/one-way/request-response paths to transfer message-body ownership and dispose pooled invokables at the correct time; updated unit tests and generator snapshot baselines.
Show a summary per file
| File | Description |
|---|---|
| test/Orleans.Serialization.UnitTests/InvokablePoolTests.cs | Adds unit tests validating pool instance isolation and safe behavior after disposal. |
| test/Orleans.CodeGenerator.Tests/snapshots/OrleansSourceGeneratorTests.TestWithUseActivatorAnnotation.verified.cs | Updates expected generated output to include invocation/pooling-related usings. |
| test/Orleans.CodeGenerator.Tests/snapshots/OrleansSourceGeneratorTests.TestWithSuppressReferenceTrackingAttribute.verified.cs | Updates expected generated output to include invocation/pooling-related usings. |
| test/Orleans.CodeGenerator.Tests/snapshots/OrleansSourceGeneratorTests.TestWithSerializerTransparentAnnotation.verified.cs | Updates expected generated output to include invocation/pooling-related usings. |
| test/Orleans.CodeGenerator.Tests/snapshots/OrleansSourceGeneratorTests.TestWithOmitDefaultMemberValuesAnnotation.verified.cs | Updates expected generated output to include invocation/pooling-related usings. |
| test/Orleans.CodeGenerator.Tests/snapshots/OrleansSourceGeneratorTests.TestRecords.verified.cs | Updates expected generated output to include invocation/pooling-related usings. |
| test/Orleans.CodeGenerator.Tests/snapshots/OrleansSourceGeneratorTests.TestGrainWithMultipleInterfaces.verified.cs | Updates expected generated grain proxy/invokable output to use activators and return invokables to pools. |
| test/Orleans.CodeGenerator.Tests/snapshots/OrleansSourceGeneratorTests.TestGrainWithDifferentKeyTypes.verified.cs | Updates expected generated grain proxy/invokable output to use activators and return invokables to pools. |
| test/Orleans.CodeGenerator.Tests/snapshots/OrleansSourceGeneratorTests.TestGrainMethodAnnotatedWithResponseTimeout.verified.cs | Updates expected generated grain proxy/invokable output to use activators and return invokables to pools. |
| test/Orleans.CodeGenerator.Tests/snapshots/OrleansSourceGeneratorTests.TestGrainMethodAnnotatedWithInvokableBaseType.verified.cs | Updates expected generated grain proxy/invokable output to use activators and return invokables to pools. |
| test/Orleans.CodeGenerator.Tests/snapshots/OrleansSourceGeneratorTests.TestGrainComplexGrain.verified.cs | Updates expected generated grain proxy/invokable output to use activators and return invokables to pools. |
| test/Orleans.CodeGenerator.Tests/snapshots/OrleansSourceGeneratorTests.TestGenericClassWithConstructorParameters.verified.cs | Updates expected generated output to include invocation/pooling-related usings. |
| test/Orleans.CodeGenerator.Tests/snapshots/OrleansSourceGeneratorTests.TestGenericClass.verified.cs | Updates expected generated output to include invocation/pooling-related usings. |
| test/Orleans.CodeGenerator.Tests/snapshots/OrleansSourceGeneratorTests.TestCompoundTypeAlias.verified.cs | Updates expected generated output to include invocation/pooling-related usings. |
| test/Orleans.CodeGenerator.Tests/snapshots/OrleansSourceGeneratorTests.TestClassesWithGeneratedActivatorConstructorAnnotation.verified.cs | Updates expected generated output to include invocation/pooling-related usings. |
| test/Orleans.CodeGenerator.Tests/snapshots/OrleansSourceGeneratorTests.TestClassWithParameterizedConstructor.verified.cs | Updates expected generated output to include invocation/pooling-related usings. |
| test/Orleans.CodeGenerator.Tests/snapshots/OrleansSourceGeneratorTests.TestClassWithOptionalConstructorParameters.verified.cs | Updates expected generated output to include invocation/pooling-related usings. |
| test/Orleans.CodeGenerator.Tests/snapshots/OrleansSourceGeneratorTests.TestClassWithNoPublicConstructors.verified.cs | Updates expected generated output to include invocation/pooling-related usings. |
| test/Orleans.CodeGenerator.Tests/snapshots/OrleansSourceGeneratorTests.TestClassWithInterfaceConstructorParameter.verified.cs | Updates expected generated output to include invocation/pooling-related usings. |
| test/Orleans.CodeGenerator.Tests/snapshots/OrleansSourceGeneratorTests.TestClassWithGenerateSerializerAnnotation.verified.cs | Updates expected generated output to include invocation/pooling-related usings. |
| test/Orleans.CodeGenerator.Tests/snapshots/OrleansSourceGeneratorTests.TestClassWithGenerateMethodSerializersAnnotation.verified.cs | Updates expected generated grain proxy/invokable output to use activators and return invokables to pools. |
| test/Orleans.CodeGenerator.Tests/snapshots/OrleansSourceGeneratorTests.TestClassReferenceProperties.verified.cs | Updates expected generated output to include invocation/pooling-related usings. |
| test/Orleans.CodeGenerator.Tests/snapshots/OrleansSourceGeneratorTests.TestClassPrimitiveTypesUsingFullName.verified.cs | Updates expected generated output to include invocation/pooling-related usings. |
| test/Orleans.CodeGenerator.Tests/snapshots/OrleansSourceGeneratorTests.TestClassPrimitiveTypes.verified.cs | Updates expected generated output to include invocation/pooling-related usings. |
| test/Orleans.CodeGenerator.Tests/snapshots/OrleansSourceGeneratorTests.TestClassNestedTypes.verified.cs | Updates expected generated output to include invocation/pooling-related usings. |
| test/Orleans.CodeGenerator.Tests/snapshots/OrleansSourceGeneratorTests.TestBasicStruct.verified.cs | Updates expected generated output to include invocation/pooling-related usings. |
| test/Orleans.CodeGenerator.Tests/snapshots/OrleansSourceGeneratorTests.TestBasicGrain.verified.cs | Updates expected generated grain proxy/invokable output to use activators and return invokables to pools. |
| test/Orleans.CodeGenerator.Tests/snapshots/OrleansSourceGeneratorTests.TestBasicClassWithoutNamespace.verified.cs | Updates expected generated output to include invocation/pooling-related usings. |
| test/Orleans.CodeGenerator.Tests/snapshots/OrleansSourceGeneratorTests.TestBasicClassWithInheritance.verified.cs | Updates expected generated output to include invocation/pooling-related usings. |
| test/Orleans.CodeGenerator.Tests/snapshots/OrleansSourceGeneratorTests.TestBasicClassWithDifferentAccessModifiers.verified.cs | Updates expected generated output to include invocation/pooling-related usings. |
| test/Orleans.CodeGenerator.Tests/snapshots/OrleansSourceGeneratorTests.TestBasicClassWithAnnotatedFields.verified.cs | Updates expected generated output to include invocation/pooling-related usings. |
| test/Orleans.CodeGenerator.Tests/snapshots/OrleansSourceGeneratorTests.TestBasicClass.verified.cs | Updates expected generated output to include invocation/pooling-related usings. |
| test/Orleans.CodeGenerator.Tests/snapshots/OrleansSourceGeneratorTests.TestAlias.verified.cs | Updates expected generated output to include invocation/pooling-related usings. |
| src/Orleans.Serialization/Invocation/Pools/InvokablePool.cs | Introduces InvokablePool<T> and keeps the prior static helper for backward compatibility. |
| src/Orleans.Serialization/Invocation/Pools/ConcurrentObjectPool.cs | Enables nullable annotations and adjusts ThreadLocal usage. |
| src/Orleans.Serialization/Hosting/ServiceCollectionExtensions.cs | Registers open-generic InvokablePool<> with serializer services. |
| src/Orleans.Runtime/Messaging/MessageCenter.cs | Copies shared request bodies for local delivery to avoid premature reuse of pooled invokables. |
| src/Orleans.Runtime/Core/InsideRuntimeClient.cs | Adds explicit request-body ownership transfer and disposal on the receiver side. |
| src/Orleans.Core/Runtime/InvokableObjectManager.cs | Adds explicit request-body ownership transfer and disposal in the local object manager path. |
| src/Orleans.Core/Runtime/GrainReferenceRuntime.cs | Ensures requests are disposed at the correct time for request/response vs one-way. |
| src/Orleans.Core/Networking/Connection.cs | Disposes one-way request bodies after they have been sent. |
| src/Orleans.Core/Messaging/MessageSerializer.cs | Marks deserialized request/one-way bodies as disposable invokables and not shared. |
| src/Orleans.Core/Messaging/MessageFactory.cs | Initializes message body ownership/disposal flags and adds a helper to copy body objects. |
| src/Orleans.Core/Messaging/Message.cs | Adds body ownership/disposal flags plus helper methods to dispose/reset the message body. |
| src/Orleans.Core.Abstractions/Runtime/GrainReference.cs | Adds cached activator storage on GrainReferenceShared and exposes proxy helpers. |
| src/Orleans.CodeGenerator/ProxyGenerator.cs | Generates proxies which cache activators and allocate requests via activators where applicable. |
| src/Orleans.CodeGenerator/LibraryTypes.cs | Adds InvokablePool<> to known library types for generation. |
| src/Orleans.CodeGenerator/InvokableGenerator.cs | Generates invokables with pool support and emits code to return to pool on Dispose(). |
| src/Orleans.CodeGenerator/CodeGenerator.cs | Adds Orleans.Serialization.Invocation to generated file usings. |
| src/Orleans.CodeGenerator/ActivatorGenerator.cs | Generates pool-aware activators which rent from InvokablePool<T> before allocating. |
Copilot's findings
- Files reviewed: 50/50 changed files
- Comments generated: 9
d5e0598 to
7c75b3c
Compare
7c75b3c to
3b1fb95
Compare
There was a problem hiding this comment.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
src/Orleans.Runtime/Messaging/MessageCenter.cs:216
- Local request dispatch deep-copies invokable bodies for every loopback request. This adds a full DeepCopier pass (and extra pooled instances) on the hottest path, which may undermine the pooling gains. Since the sender now disposes request bodies when invocation completes, consider transferring ownership by clearing DisposeBodyObject for loopback Request messages instead of copying.
if (msg.Direction == Message.Directions.Request && msg.BodyObjectIsShared && msg.BodyObject is { } body)
{
msg.BodyObject = this.messageFactory.CopyBodyObject(body);
msg.DisposeBodyObject = msg.BodyObject is IInvokable;
msg.BodyObjectIsShared = false;
src/Orleans.Core/Runtime/CallbackData.cs:55
- SubscribeForCancellation can leak the CancellationTokenRegistration: if the callback completes between the IsCompleted check and assigning _cancellationTokenRegistration, the completion paths will dispose the default registration and this method will store the real registration without disposing it. Store the registration field first, then re-check completion and dispose/reset if already completed.
if (IsCompleted)
{
registration.Dispose();
}
else
- Files reviewed: 50/50 changed files
- Comments generated: 2
- Review effort level: Lite
There was a problem hiding this comment.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/Orleans.Core/Runtime/CallbackData.cs:58
- SubscribeForCancellation has a race where the cancellation token can be canceled after IsCompleted is checked but before _cancellationTokenRegistration is published (or before completion paths dispose it). In that case OnCancellation/OnTimeout/etc will dispose the default registration, then this method stores the real registration and it is never disposed, keeping CallbackData (and the CTS) alive longer than intended.
Consider reinstating a publish/complete state machine (like the previous pending/published flags) or otherwise making publication + disposal coordination atomic (eg, publish the registration, then re-check completion and dispose/clear it in a thread-safe way).
public void SubscribeForCancellation(CancellationToken cancellationToken)
{
if (!cancellationToken.CanBeCanceled)
{
return;
}
_cancellationToken = cancellationToken;
var registration = cancellationToken.UnsafeRegister(static arg =>
{
var callbackData = (CallbackData)arg!;
callbackData.OnCancellation();
}, this);
if (IsCompleted)
{
registration.Dispose();
}
else
{
_cancellationTokenRegistration = registration;
}
- Files reviewed: 50/50 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
The retried .NET CI workflow confirms PR-specific source-generator snapshot mismatches on Windows, Linux, and macOS for both net8.0 and net10.0. Numerous |
98abe2b to
0cfa5d4
Compare
There was a problem hiding this comment.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/Orleans.Core/Runtime/CallbackData.cs:55
- SubscribeForCancellation has a race where cancellation/completion can occur after the
if (IsCompleted)check but before_cancellationTokenRegistration = registrationexecutes. In that case,OnCancellationwill dispose the default registration (no-op) and the realregistrationis never disposed, potentially retaining the callback and leaking the registration.
A simple fix is to publish _cancellationTokenRegistration before the completion check and, if already completed, clear it and dispose the local registration.
if (IsCompleted)
{
registration.Dispose();
}
else
- Files reviewed: 26/51 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
CI root cause: pooling made non-generic generated invokables constructor-injected only, but runtime and test paths still use parameterless reflection activation. That caused matrix-wide BVT failures with \MissingMethodException: No parameterless constructor defined. The branch now emits a compatibility constructor for pooled invokables, keeps pool-aware activation for normal calls, and only returns pool-created instances to the pool. The reproduced durable-jobs BVT passes, generator tests pass on net8.0/net10.0, and \Orleans.slnx\ builds. |
There was a problem hiding this comment.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/Orleans.Core/Runtime/CallbackData.cs:58
- There is a race between the completion check and publishing
_cancellationTokenRegistration: if the token is canceled afterIsCompletedis read but before_cancellationTokenRegistrationis assigned,OnCancellation()will run and dispose the default registration, and the real registration will remain undisposed, keeping this CallbackData instance rooted longer than necessary.
}
else
{
_cancellationTokenRegistration = registration;
}
- Files reviewed: 26/51 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/Orleans.CodeGenerator/ProxyGenerator.cs:298
- Potential NullReferenceException: AttributeData.AttributeClass can be null (eg, unresolved attribute types during compilation errors). This code dereferences it unconditionally, which can crash the source generator instead of emitting diagnostics.
if (attr.AttributeClass.GetAttributes(LibraryTypes.InvokeMethodNameAttribute, out var attrs))
- Files reviewed: 26/51 changed files
- Comments generated: 0 new
- Review effort level: Lite
7bd82da to
f667a7b
Compare
There was a problem hiding this comment.
Review details
Suppressed comments (1)
src/Orleans.Core/Runtime/GrainReferenceRuntime.cs:164
- Same pooling concern as the generic overload: if RuntimeClient.SendRequest throws before the caller awaits the ValueTask (common for cancellation/disposed paths), the ResponseCompletionSource instance will never have GetResult called and will not be returned to ResponseCompletionSourcePool.
catch
{
DisposeRequest(request);
throw;
}
- Files reviewed: 38/38 changed files
- Comments generated: 1
- Review effort level: Lite
Code coverage77.60% line coverage - 97,774 / 125,992 lines Coverage details
|
There was a problem hiding this comment.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
test/Orleans.Serialization.UnitTests/InvokablePoolTests.cs:85
- The test uses unbounded waits/joins (ready.Wait, start.Wait, Thread.Join). If a regression causes a thread to fail before signaling, the test run can hang indefinitely. Add timeouts and assert success so failures surface as test failures instead of deadlocks.
This issue also appears on line 97 of the same file.
test/Orleans.Serialization.UnitTests/InvokablePoolTests.cs:99
- The worker thread blocks on start.Wait() without a timeout. If the main thread never signals (or the event is disposed unexpectedly), the test can hang. Use a timeout and assert to fail fast.
- Files reviewed: 39/39 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
Copilot review overview
Review tier: Lite
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
test/Orleans.Core.Tests/Runtime/GrainReferenceRuntimeTests.cs — Unused using directive Microsoft.Extensions.DependencyInjection will produce CS8019 (and may fail… |
e05fc11 to
797df61
Compare
There was a problem hiding this comment.
Copilot review overview
Review tier: Lite
Findings: 2
New issues introduced by this change (2)
| Severity | Finding |
|---|---|
test/Orleans.Core.Tests/Runtime/GrainReferenceRuntimeTests.cs — Unused using directive will trigger CS8019 (and likely fail the build with TreatWarningsAsErrors).… |
|
test/Orleans.Core.Tests/Messaging/InvokableMessageOwnershipTests.cs — Unused using directive (System.Buffers) will produce CS8019 and can fail the build when warnings… |
Pre-existing issues (1)
| Severity | Finding |
|---|---|
test/Orleans.Core.Tests/Runtime/GrainReferenceRuntimeTests.cs — Unused using directive Microsoft.Extensions.DependencyInjection will produce CS8019 (and may fail… View comment |
There was a problem hiding this comment.
Copilot review overview
Review tier: Lite
Findings: None
Issues resolved since last review (3)
| Severity | Finding |
|---|---|
test/Orleans.Core.Tests/Messaging/InvokableMessageOwnershipTests.cs — Unused using directive (System.Buffers) will produce CS8019 and can fail the build when warnings… View resolved comment |
|
test/Orleans.Core.Tests/Runtime/GrainReferenceRuntimeTests.cs — Unused using directive will trigger CS8019 (and likely fail the build with TreatWarningsAsErrors).… View resolved comment |
|
test/Orleans.Core.Tests/Runtime/GrainReferenceRuntimeTests.cs — Unused using directive Microsoft.Extensions.DependencyInjection will produce CS8019 (and may fail… View resolved comment |


Summary
IInvokablerequest objects for non-generic grain methods and registersInvokablePool<T>with serializer services.Validation
git diff --checkdotnet build src\Orleans.Serialization\Orleans.Serialization.csproj -mdotnet build src\Orleans.CodeGenerator\Orleans.CodeGenerator.csproj -mTypeEncodingTestspassed on net8/net10; netcoreapp3.1 skipped by xUnit loaderDependencies / notes
main, but is logically related to message pooling/ref-counting work and may need coordination with that PR.Microsoft Reviewers: Open in CodeFlow