Fix benchmark request-size measurement and buffer disposal for PooledContentStream marshallers#4449
Open
muhammad-othman wants to merge 1 commit into
Conversation
muhammad-othman
requested review from
dscpinheiro and
peterrsongg
and removed request for
a team
dscpinheiro
requested review from
boblodgett
and removed request for
peterrsongg
muhammad-othman
force-pushed
the
muhamoth/DOTNET-8587-optimize-cbor-serilaization-benchmarks-changes
branch
from
76ee5f5 to
ecb137a
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Updates the performance benchmark runners to measure marshalled request payload sizes correctly across marshallers that use either legacy byte[] Content or pooled-buffer-backed PooledContentStream, and to dispose marshalled requests to better mirror the production pipeline and avoid skewed allocation measurements.
Changes:
- Added a shared helper to read request body size from either
ContentorPooledContentStreamand dispose the marshalled request. - Updated SerdeBenchmarks marshalling benchmarks to return the request body size and dispose each marshalled request.
- Updated CBOR performance benchmark utilities to read body size from
PooledContentStream(and added a missing using for event-stream headers).
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| sdk/test/Performance/SerdeBenchmarks/SerdeBenchmarksRunner/TestDataHelpers.cs | Adds helper to measure marshalled request size and dispose marshalled IRequest. |
| sdk/test/Performance/SerdeBenchmarks/SerdeBenchmarksRunner/RpcV2CborBenchmarks.cs | Updates marshalling benchmarks to return size via helper and dispose requests. |
| sdk/test/Performance/SerdeBenchmarks/SerdeBenchmarksRunner/RestXmlBenchmarks.cs | Updates REST-XML request benchmarks to return size via helper and dispose requests. |
| sdk/test/Performance/SerdeBenchmarks/SerdeBenchmarksRunner/RestJson1Benchmarks.cs | Updates REST-JSON request benchmarks to return size via helper and dispose requests. |
| sdk/test/Performance/SerdeBenchmarks/SerdeBenchmarksRunner/AwsQueryBenchmarks.cs | Updates AWS Query request benchmarks to return size via helper and dispose requests. |
| sdk/test/Performance/SerdeBenchmarks/SerdeBenchmarksRunner/AwsJson10Benchmarks.cs | Updates AWS JSON 1.0 request benchmarks to return size via helper and dispose requests. |
| sdk/test/Performance/CborPerformanceBenchmarks/CborPerformanceBenchmarksRunner/Utils.cs | Adds GetContentLength(IRequest) to read from pooled content streams or legacy Content. |
| sdk/test/Performance/CborPerformanceBenchmarks/CborPerformanceBenchmarksRunner/FakeWebResponseData.cs | Adds required Amazon.Runtime.EventStreams using for IEventStreamHeader. |
| sdk/test/Performance/CborPerformanceBenchmarks/CborPerformanceBenchmarksRunner/BaseBenchmarks.cs | Switches request-size measurement to Utils.GetContentLength(...). |
| sdk/test/Performance/CborPerformanceBenchmarks/CborPerformanceBenchmarksRunner/BaseDoubleBenchmarks.cs | Switches request-size measurement to Utils.GetContentLength(...) for the second benchmark slot. |
Comment on lines
+34
to
+43
| public static long GetContentLengthAndDispose(IRequest request) | ||
| { | ||
| try | ||
| { | ||
| #if !NETFRAMEWORK | ||
| if (request.ContentStream is PooledContentStream pooled) | ||
| return pooled.Content.Length; | ||
| #endif | ||
| return request.Content?.Length ?? 0; | ||
| } |
Comment on lines
43
to
47
| [GlobalCleanup(Target = nameof(Marshall))] | ||
| public virtual void AfterMarshall() | ||
| { | ||
| RequestSizeBytes = Math.Max(RequestSizeBytes, MarshalledRequest.Content.Length); | ||
| RequestSizeBytes = Math.Max(RequestSizeBytes, Utils.GetContentLength(MarshalledRequest)); | ||
|
|
Comment on lines
30
to
34
| [GlobalCleanup(Target = nameof(Marshall2))] | ||
| public virtual void AfterMarshall2() | ||
| { | ||
| RequestSizeBytes2 = Math.Max(RequestSizeBytes2, MarshalledRequest2.Content.Length); | ||
| RequestSizeBytes2 = Math.Max(RequestSizeBytes2, Utils.GetContentLength(MarshalledRequest2)); | ||
|
|
…ContentStream marshallers
muhammad-othman
force-pushed
the
muhamoth/DOTNET-8587-optimize-cbor-serilaization-benchmarks-changes
branch
from
ecb137a to
b4e8f15
Compare
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.
Description
Updates the serde and CBOR performance benchmarks to read the marshalled request body size correctly and to dispose each marshalled request, matching the production pipeline.
Request body size is now read from whichever field the marshaller populated, so the benchmarks work whether or not a marshaller writes its body into a pooled buffer.
Each marshalled request is now disposed after measuring. The production pipeline disposes the request after marshalling so when the body is backed by a pooled buffer, that returns the buffer and keeps the pool warm. Without it, every iteration forced a fresh allocation and
[MemoryDiagnoser]overstated the real per-request cost.Motivation and Context
DOTNET-8581Testing
Ran the updated benchmarks.
Dry-runs
Breaking Changes Assessment
Screenshots (if appropriate)
Types of changes
Checklist
License