Implement performance benchmarks for Always Encrypted scenarios - #4502
Implement performance benchmarks for Always Encrypted scenarios#4502edwardneal wants to merge 9 commits into
Conversation
* Replace X hardcoded test methods with a single ParamsSource. * Merge DataTypeReaderRunner and its async variant together. * Implement base class, and two child classes - one for AlwaysEncrypted, one for plaintext. * Move the connection open and the table setup logic out of the benchmark code.
This was actually running both sync and async tests. * Implement base class, and two child classes - one for AlwaysEncrypted, one for plaintext. * Expand test to cover CommandBehavior.Default and SequentialAccess. * Include GetFieldValue and GetFieldValueAsync. * Move repeated allocation of the buffer out of the main ReadLargeDataSync_GetBytes benchmark loop.
* File-scoped namespaces. * Remove references to removed benchmarks.
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
| Table t = Table.Build(nameof(SqlCommandRunner)) | ||
| .AddColumn(new Column(type)) | ||
| .CreateTable(sqlConnection) | ||
| .InsertBulkRows(s_rowCount, sqlConnection); |
There was a problem hiding this comment.
This is the flaw in the DataTypeReader[Async]Runner methodology. RunBenchmarkAsync doesn't just cover the cost of reading a result set of s_rowCount rows - it also covers the cost of using SqlBulkCopy to write these rows to the table.
| using var insertCmd = new SqlCommand( | ||
| $@"INSERT INTO {_tableName} (Data) | ||
| SELECT SUBSTRING( | ||
| CONVERT( | ||
| varbinary(max), | ||
| REPLICATE( | ||
| CONVERT(varchar(max), CRYPT_GEN_RANDOM(8000), 2), | ||
| (@dataSizeBytes + 7999) / 8000 | ||
| ), | ||
| 2 | ||
| ), | ||
| 1, | ||
| @dataSizeBytes | ||
| );", conn); |
There was a problem hiding this comment.
This had to change, since Always Encrypted requires that values come from the client. The memory allocations aren't included in the benchmark though.
| using var conn = new SqlConnection(_connectionString); | ||
| conn.Open(); | ||
| using var cmd = new SqlCommand($"SELECT Data FROM {_tableName}", conn); | ||
| using var reader = cmd.ExecuteReader(CommandBehavior.SequentialAccess); |
There was a problem hiding this comment.
Always Encrypted doesn't support using CommandBehavior.SequentialAccess, or calling GetBytes or GetStream.
Besides this, there are genuine performance variations between GetBytes/GetStream with a CommandBehavior of SequentialAccess vs Default. In plaintext scenarios, we can test both methods in both scenarios. Always Encrypted scenarios will just use GetFieldValue[Async] with CommandBehavior.Default.
cheenamalhotra
added
Performance 📈
PerformanceTests now references TestCommon. In Package mode the perf pipeline pins a released MDS baseline via -p:MdsPackageVersion, but TestCommon ignored that property and resolved the CPM-managed in-development version instead. That version is not published, so restore failed with NU1102 and the mismatch produced an NU1605 downgrade error. Apply the same conditional VersionOverride pattern used by PerformanceTests, including the 7.1.0-preview1.26124.5 transitive dependency pins. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 82c3df83-8f5b-487a-b57a-31d5acb40abe
The large data read benchmarks failed with:
System.InvalidOperationException: Benchmark
Plaintext.ReadLargeDataSync_GetBytes ... takes too long to run.
Prefer to use out-of-process toolchains for long-running benchmarks.
BenchmarkDotNet's in-process executor aborts a benchmark case after a
hard-coded 5 minute default. A single case here reads up to 20 MB per
operation across 20 iterations, plus the extra iterations that
MemoryDiagnoser and ThreadingDiagnoser each add, so it exceeds that.
Switching to an out-of-process toolchain (BenchmarkDotNet's own
suggestion) is not viable for this suite. The AppContext switches set in
Program.SetupConfigurations - managed SNI, connection pool V2, optimized
async behaviour - only apply to the process running Main. An
out-of-process toolchain spawns a generated host where they revert to
their defaults, so the benchmarks would silently measure the wrong code
paths.
Instead, add two optional per-benchmark knobs to runnerconfig.jsonc and
apply them to the two large data read runners:
TimeoutMinutes - overrides the in-process execution timeout (30
minutes here); omitted elsewhere, so other benchmarks keep the
BenchmarkDotNet default.
RunStrategy - selects the BenchmarkDotNet RunStrategy. The large data
read runners use Monitoring, which skips the harness-overhead
measurement Throughput performs; that measurement is meaningless
when a single operation takes seconds.
Their IterationCount also drops from 20 to 5, since network-bound 20 MB
reads gain little statistical value from the extra iterations.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 82c3df83-8f5b-487a-b57a-31d5acb40abe
|
Please consider changes in edwardneal#1 as they unblock running perf benchmarks with new changes. |
Fix perf-pipeline restore and in-process benchmark timeout for AE benchmarks
This was never used by Always Encrypted.
|
Thanks @cheenamalhotra, I've just checked and merged; all of the new scenarios continue to benchmark. They also confirm the performance issue I'm chasing down with my other PRs: every AE-protected MB we read from SQL Server results in 5 MB of allocations within the driver. Looking more widely, there seems to be quite a bit of performance pipeline work which is required because the benchmarks are running in-process - is this solely to make sure that the AppContext switches are being set correctly? I've got a local benchmarking harness (gist) which does this while running out-of-process and might simplify the overall work. |
|
/azp run |
|
Azure Pipelines: Successfully started running 3 pipeline(s). |
Yes, we are aware of that too and are starting to run benchmarks internally. The goal is to prevent perf regressions and also confirm app context switches do continue to resolve perf issues as promised and not regress other paths. We wouldn't be running multiple benchmarks at once, as the goal is to run single test in isolation against a fixed baseline. But I'll look into out-of-process approach to see if it can be useful for developer testing. Thanks for sharing :) |
4 participants
Description
This builds on a couple of PRs:
In this PR, I merge a few benchmarks (
DataTypeReaderRunnerandDataTypeReaderAsyncRunner) together, then add an Always Encrypted benchmark on top of them. In the process, I also noticed that the benchmark methodology for these was flawed. I've corrected this, and deliberately changed the benchmark name to provide a clean start for any data analytics.I've also worked with
AsyncLargeDataReadRunner. This wasn't truly async, and it missed a few data pathways which read large binary blobs. Although I've changed this benchmark name too, I've done so on the basis that the performance tests pipeline is new enough that we can afford to make the name consistent without losing a critical amount of historical data. I'm happy to change it back if that's not the case.Underpinning this are two new RAII types:
ColumnMasterKeyandColumnEncryptionKey. There's not much to see here, besides an integration point with the existing certificate fixtures.This isn't the end of the AE benchmarks - SqlBulkCopy is important too. This is very tightly bound to some custom logic to create/drop database objects though. A future PR can remove this custom logic and then start to make the bulk copy benchmarks AE-agnostic.
Issues
No covering issue, but this continues work flagged as a TODO in code.
Testing
All of these benchmarks continue to run.