[Internal] Binary Encoding: Adds performance tests with binary encoding enabled. #4943
Conversation
| @@ -137,6 +137,7 @@ jobs: | |||
| condition: and(succeeded(), eq(${{ parameters.IncludePerformance }}, true)) | |||
| pool: | |||
| name: 'OneES' | |||
| timeoutInMinutes: 70 | |||
There was a problem hiding this comment.
What are the current run times of the tests? Is this just to conform to the rest of the benchmarks that have this timeout or do the tests actually take 70+ minutes to run?
There was a problem hiding this comment.
Default timeout is 60 minutes and with my current setup the time was going to 63 minutes. As part of this PR, all existing MockedItmeBenchmark tests are running twice. Once with binary encoding enabled and once with binary encoding disabled.
There was a problem hiding this comment.
And will this run every PR or on the nightly/release pipelines only?
There was a problem hiding this comment.
Performance tests don't run with every PR. We manually run them here
https://cosmos-db-sdk-public.visualstudio.com/cosmos-db-sdk-public/_build?definitionId=78&_a=summary
| includeDiagnosticsToString: false, | ||
| isClientMetricsEnabled: true) } | ||
| }; | ||
| public static IItemBenchmark[] IterParameters = new IItemBenchmark[] |
There was a problem hiding this comment.
Can it be initialized always only in GlobalSetup?
| "MockedItemBulkBenchmark.UpsertItem;[Type=OfTWithDistributedTracingEnabled]": "1208600.25", | ||
| "MockedItemBulkBenchmark.UpsertItem;[Type=Stream]": "768928.75" | ||
|
|
||
| "MockedItemBenchmark.CreateItem;[Type=OfT][EnableBinaryResponseOnPointOperations=True]": "38127.25", |
There was a problem hiding this comment.
Wow no impact at-all in any of the combinations? Can you please double check once again.
There was a problem hiding this comment.
so for checking regression, I added the exact same value we initially had these tests and set it for both flag enabled and disabled.
There was a problem hiding this comment.
But I can try adding assertion within the test class to ensure binary encoding with flag enabled code path is working through.
There was a problem hiding this comment.
Yah the gates also might need to be refresh to include binary runs.
| if (request.ResourceAddress.EndsWith(MockedItemBenchmarkHelper.ExistingItemId)) | ||
| { | ||
| return new DocumentServiceResponse( | ||
| response = request.ResourceAddress.EndsWith(MockedItemBenchmarkHelper.ExistingItemId) |
There was a problem hiding this comment.
nit: Conditional part can be scoped to just the MemoryStream part.
There was a problem hiding this comment.
Can this be done outside of the above If so-that all operations can use it?
| new MemoryStream(MockRequestHelper.testItemFeedResponsePayload), | ||
| headers, | ||
| System.Net.HttpStatusCode.OK | ||
| ); | ||
| } | ||
| else |
There was a problem hiding this comment.
If response declaration initialized to null then this else block is not needed right?
There was a problem hiding this comment.
removed the else block
|
|
||
| return null; | ||
| // Check if binary encoding is enabled and this is a supported point operation | ||
| if (ConfigurationManager.IsBinaryEncodingEnabled() && MockRequestHelper.IsPointOperationSupportedForBinaryEncoding(request)) |
There was a problem hiding this comment.
Can it be based on the incoming request it-self?
There was a problem hiding this comment.
I.e. avoid ConfigurationManager part
There was a problem hiding this comment.
removed the check for ConfigurationManager.IsBinaryEncodingEnabled() and instead checking the request header instead
(request.Headers.Get(HttpConstants.HttpHeaders.SupportedSerializationFormats)
|
|
||
| // Converts the response payload into binary if needed | ||
| // This simulates binary serialization using the same approach as in MockedItemBenchmarkHelper. | ||
| private static DocumentServiceResponse ConvertToBinaryIfNeeded(DocumentServiceResponse response) |
There was a problem hiding this comment.
Thoughts on doing the conversation once, cache and clone?
Will keep the side-affects/variability from test code as minimal as possible.
There was a problem hiding this comment.
updated the code and using a dictionary instead to avoid repeated re-serialization
| private static byte[] GetOrAddBinaryPayload(byte[] textPayload) | ||
| { | ||
| // A simple key is the base64 of the payload. For large payloads, consider hashing. | ||
| string key = Convert.ToBase64String(textPayload); |
There was a problem hiding this comment.
One other option is to have choose binary vs non-binary byte[] at the source it-self. This will avoid the Dict nad Base64 key encoding cost as well.
There was a problem hiding this comment.
BatchResponse is interesting it's the only dynamic data part, that might need to pay the binary conversation cost.
So for batch benchmarks by-definition see the regression (might not be
Pull Request Template
Description
Update existing MockedItemBenchmark performance tests for methods CreateItemAsync, CreateItemStreamAsync, ReadItemAsync, ReadItemStreamAsync, UpsertItemAsync, UpsertItemStreamAsync, ReplaceItemAsync, ReplaceItemStreamAsync, DeleteItemAsync, DeleteItemStreamAsync which have binary encoding support. These tests will run in the pipeline and help determine in determining regression when binary encoding feature is enabled.
https://cosmos-db-sdk-public.visualstudio.com/cosmos-db-sdk-public/_build/results?buildId=52990&view=logs&j=aa2fd561-a338-5537-45ee-49ae22aea958&t=65500a81-e1c7-5931-1116-cabb8c832591
Type of change
Please delete options that are not relevant.
Closing issues
To automatically close an issue: closes #IssueNumber