-
Notifications
You must be signed in to change notification settings - Fork 495
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Internal] Binary Encoding: Adds performance tests with binary encoding enabled. #4911
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good!
private MockedItemBenchmarkHelper benchmarkHelper; | ||
private Container container; | ||
|
||
[Params(true)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please have it run with both true and false to see the impact of enabling it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code path when binary encoding is set to false is covered by existing MockedItemBenchmark.cs. But I can include false parameter here also if it adds more clarity.
.../tests/Microsoft.Azure.Cosmos.Performance.Tests/Benchmarks/BinaryEncodingEnabledBenchmark.cs
Outdated
Show resolved
Hide resolved
[Params(true, false)] | ||
public bool EnableBinaryResponseOnPointOperations; | ||
|
||
[GlobalSetup] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is GlobalCleanup needed?
[MemoryDiagnoser] | ||
[BenchmarkCategory("GateBenchmark")] | ||
[Config(typeof(BinaryEncodingEnabledBenchmark.CustomBenchmarkConfig))] | ||
public class BinaryEncodingEnabledBenchmark |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to model it as a param on existing Benchmark? (To avoid code duplication)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or derive from existing ItemBenchmark tests?
@@ -90,5 +90,25 @@ | |||
"MockedItemBulkBenchmark.UpsertItem;[Type=OfTWithClientMetricsEnabled]": "1206121.5", | |||
"MockedItemBulkBenchmark.UpsertItem;[Type=OfTWithDiagnosticsToString]": "1204822", | |||
"MockedItemBulkBenchmark.UpsertItem;[Type=OfTWithDistributedTracingEnabled]": "1208600.25", | |||
"MockedItemBulkBenchmark.UpsertItem;[Type=Stream]": "768928.75" | |||
"MockedItemBulkBenchmark.UpsertItem;[Type=Stream]": "768928.75", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The values are same for both binary and non-binary flow correct ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes keeping the same values for all point operations for both binary encoding flag enabled and disabled.
Please review this PR #4943 for performance tests. |
Description
Add 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=52826&view=logs&j=aa2fd561-a338-5537-45ee-49ae22aea958&t=65500a81-e1c7-5931-1116-cabb8c832591
Type of change
Closing issues
To automatically close an issue: closes #IssueNumber