fix(GWC-29): drop the wasted request clone on the Invoke hot path
Invoke deep-cloned the whole MxCommandRequest — including its command payload, potentially a large bulk-write graph — only to overwrite the cloned command with commandToInvoke and discard it. MapCommand then did the one clone actually needed. Net cost: a full wasted command deep-clone per Invoke, worst for exactly the bulk writes that are largest. Adds a MapCommand(MxCommand) overload (MapCommand reads nothing else off the request) and has Invoke pass commandToInvoke directly; the request overload delegates so other callers are untouched. The remaining clone inside MapCommand stays and is now documented as required rather than incidental: commandToInvoke may be the gRPC-owned request.Command, and the caller reads it again after dispatch via TrackCommandReply, so ownership transfer (à la GWC-07) is not safe here. That clone is what keeps WorkerClient.CreateCommandEnvelope's no-aliasing invariant true. Tests: MxAccessGrpcMapperTests.MapCommandFromCommandClonesPayload (mutating the input leaves the mapped command untouched; both overloads produce equal results under a fixed TimeProvider).
This commit is contained in:
@@ -116,9 +116,12 @@ public sealed class MxAccessGatewayService(
|
|||||||
return bulkConstraintPlan.CreateDeniedReply(request);
|
return bulkConstraintPlan.CreateDeniedReply(request);
|
||||||
}
|
}
|
||||||
|
|
||||||
MxCommandRequest invokeRequest = request.Clone();
|
// Map from the command alone: cloning the whole request only to overwrite its command with
|
||||||
invokeRequest.Command = commandToInvoke;
|
// commandToInvoke deep-cloned the (potentially large) original payload for nothing, since
|
||||||
WorkerCommand workerCommand = mapper.MapCommand(invokeRequest);
|
// MapCommand reads nothing but the command (GWC-29). The one clone that matters still
|
||||||
|
// happens inside MapCommand, which is what keeps the worker-bound graph unaliased from
|
||||||
|
// commandToInvoke — the caller still reads it below via TrackCommandReply.
|
||||||
|
WorkerCommand workerCommand = mapper.MapCommand(commandToInvoke);
|
||||||
WorkerCommandReply workerReply = await sessionManager
|
WorkerCommandReply workerReply = await sessionManager
|
||||||
.InvokeAsync(request.SessionId, workerCommand, context.CancellationToken)
|
.InvokeAsync(request.SessionId, workerCommand, context.CancellationToken)
|
||||||
.ConfigureAwait(false);
|
.ConfigureAwait(false);
|
||||||
|
|||||||
@@ -29,9 +29,27 @@ public sealed class MxAccessGrpcMapper
|
|||||||
ArgumentNullException.ThrowIfNull(request);
|
ArgumentNullException.ThrowIfNull(request);
|
||||||
ArgumentNullException.ThrowIfNull(request.Command);
|
ArgumentNullException.ThrowIfNull(request.Command);
|
||||||
|
|
||||||
|
return MapCommand(request.Command);
|
||||||
|
}
|
||||||
|
|
||||||
|
/// <summary>
|
||||||
|
/// Maps a gRPC MX command to a worker command. Callers that already hold the command — including
|
||||||
|
/// the constraint pipeline, whose rewritten command is not the one on the request — use this
|
||||||
|
/// overload rather than cloning a whole request to carry a single field (GWC-29); nothing outside
|
||||||
|
/// the command is read here.
|
||||||
|
/// </summary>
|
||||||
|
/// <param name="command">Command payload.</param>
|
||||||
|
/// <returns>The mapped <see cref="WorkerCommand"/> ready for worker dispatch.</returns>
|
||||||
|
public WorkerCommand MapCommand(MxCommand command)
|
||||||
|
{
|
||||||
|
ArgumentNullException.ThrowIfNull(command);
|
||||||
|
|
||||||
|
// The clone is required and must stay: the caller may hand us the gRPC-owned request command,
|
||||||
|
// and the caller keeps reading it after dispatch (TrackCommandReply). Cloning here is what makes
|
||||||
|
// WorkerClient.CreateCommandEnvelope's no-aliasing invariant true.
|
||||||
return new WorkerCommand
|
return new WorkerCommand
|
||||||
{
|
{
|
||||||
Command = request.Command.Clone(),
|
Command = command.Clone(),
|
||||||
EnqueueTimestamp = Timestamp.FromDateTimeOffset(_timeProvider.GetUtcNow()),
|
EnqueueTimestamp = Timestamp.FromDateTimeOffset(_timeProvider.GetUtcNow()),
|
||||||
};
|
};
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -1,3 +1,4 @@
|
|||||||
|
using Microsoft.Extensions.Time.Testing;
|
||||||
using ZB.MOM.WW.MxGateway.Contracts.Proto;
|
using ZB.MOM.WW.MxGateway.Contracts.Proto;
|
||||||
using ZB.MOM.WW.MxGateway.Server.Grpc;
|
using ZB.MOM.WW.MxGateway.Server.Grpc;
|
||||||
|
|
||||||
@@ -38,6 +39,48 @@ public sealed class MxAccessGrpcMapperTests
|
|||||||
Assert.NotNull(workerCommand.EnqueueTimestamp);
|
Assert.NotNull(workerCommand.EnqueueTimestamp);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// <summary>
|
||||||
|
/// The command-only overload exists so <c>Invoke</c> does not deep-clone the whole request just to
|
||||||
|
/// overwrite and discard its command (GWC-29). It must still perform the one clone that keeps the
|
||||||
|
/// worker-bound graph unaliased from the caller-owned gRPC command, and must produce the same
|
||||||
|
/// <see cref="WorkerCommand"/> as the request overload.
|
||||||
|
/// </summary>
|
||||||
|
[Fact]
|
||||||
|
public void MapCommandFromCommandClonesPayload()
|
||||||
|
{
|
||||||
|
FakeTimeProvider timeProvider = new(new DateTimeOffset(2026, 8, 7, 12, 0, 0, TimeSpan.Zero));
|
||||||
|
MxAccessGrpcMapper mapper = new(timeProvider);
|
||||||
|
MxCommand command = new()
|
||||||
|
{
|
||||||
|
Kind = MxCommandKind.Write,
|
||||||
|
Write = new WriteCommand
|
||||||
|
{
|
||||||
|
ServerHandle = 10,
|
||||||
|
ItemHandle = 20,
|
||||||
|
UserId = 30,
|
||||||
|
Value = new MxValue
|
||||||
|
{
|
||||||
|
DataType = MxDataType.String,
|
||||||
|
StringValue = "value",
|
||||||
|
},
|
||||||
|
},
|
||||||
|
};
|
||||||
|
MxCommandRequest request = new()
|
||||||
|
{
|
||||||
|
SessionId = "session-1",
|
||||||
|
Command = command.Clone(),
|
||||||
|
};
|
||||||
|
|
||||||
|
WorkerCommand fromCommand = mapper.MapCommand(command);
|
||||||
|
WorkerCommand fromRequest = mapper.MapCommand(request);
|
||||||
|
command.Write.Value.StringValue = "changed";
|
||||||
|
|
||||||
|
Assert.Equal(MxCommandKind.Write, fromCommand.Command.Kind);
|
||||||
|
Assert.Equal("value", fromCommand.Command.Write.Value.StringValue);
|
||||||
|
Assert.NotNull(fromCommand.EnqueueTimestamp);
|
||||||
|
Assert.Equal(fromRequest, fromCommand);
|
||||||
|
}
|
||||||
|
|
||||||
/// <summary>Verifies that command reply mapping preserves HRESULT and status information.</summary>
|
/// <summary>Verifies that command reply mapping preserves HRESULT and status information.</summary>
|
||||||
[Fact]
|
[Fact]
|
||||||
public void MapCommandReply_PreservesHresultStatusesAndPayload()
|
public void MapCommandReply_PreservesHresultStatusesAndPayload()
|
||||||
|
|||||||
Reference in New Issue
Block a user