perf: Port rpc changes from corefx #209

Wraith2 · 2019-09-22T14:57:40Z

This is a port of dotnet/corefx#34049 which changes how rpc object are constructed and re-used so avoid repeated creation of new SqlParameter instances where they aren't needed.

This change was not brought across from corefx initially because the introduction of AlwaysEncrypted required additionally changes. Those changes are part of this new PR. manual tests pass in debug mode (with all asserts) so it looks like the AE changes are ok but I'd like some oversight to make sure I've understood what it's doing correctly.

I think I've found a weakness in the batch rpc mode with AE. It assumes that the command will be in sysemParams[0] but this is only true if the batch rpc was constructed as an sql command and not as a stored procedure. This isn't currently causing a problem but might need tightening up.

cheenamalhotra · 2019-09-24T19:37:51Z

Hi @Wraith2

Please check and confirm code coverage (as close to 100% as possible) on changes made here. If RPC tests don't exist, please add them to the suite so they cover maximum line changes here.

Wraith2 · 2019-09-24T19:51:51Z

How do I check coverage? Given that this effects how you do queries it'd be quite surprising if it wasn't covered.

Wraith2 · 2019-09-25T18:39:48Z

I'm definitely going to need help getting coverage. I've tried getting the manual tests running in visual studio and i'm unable to apply the required environment variables (launchsettings.json doesn't work for some reason) and the tests themselves won't execute because of an nuint serialization exception. So either I need some guidance on how you get it working in visual studio or how to get coverage numbers from msbuild and use them sensibly.

cheenamalhotra · 2019-09-25T19:05:28Z

Sure, I'd expect it to be covered too, we just want to verify if we have everything covered here. So maybe try running tests in Visual Studio with coverage plugins?

Also extracted this coverage report for this PR, hope it serves well as a starting point.
20190922.2...13627.coverage.zip

cheenamalhotra · 2019-09-25T19:22:17Z

FYI, you can also run dotnet test command with appended --collect:"code coverage" --collect-format:"coverage" or --collect:"code coverage" --collect-format:"coveragexml" that will give you similar file for your specific test run.

Wraith2 · 2019-09-25T22:26:42Z

Should tests run from within visual studio? When I try I get this error which stops the entire run:

[25/09/2019 11:13:44.175 PM Informational] Store opened in 0.320 sec.
[25/09/2019 11:13:45.071 PM Informational] ---------- Discovery started ----------
[25/09/2019 11:13:45.285 PM Error] Microsoft.VisualStudio.TestPlatform.ObjectModel.TestPlatformException: Unable to find E:\Programming\csharp7\SqlClient\bin\Windows_NT\Debug.AnyCPU\CoreFx.Private.TestUtilities\netcoreapp2.1\testhost.dll. Please publish your test project and retry.
   at Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Hosting.DotnetTestHostManager.GetTestHostPath(String runtimeConfigDevPath, String depsFilePath, String sourceDirectory)
   at Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Hosting.DotnetTestHostManager.GetTestHostProcessStartInfo(IEnumerable`1 sources, IDictionary`2 environmentVariables, TestRunnerConnectionInfo connectionInfo)
   at Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client.ProxyOperationManager.SetupChannel(IEnumerable`1 sources)
   at Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client.ProxyDiscoveryManager.DiscoverTests(DiscoveryCriteria discoveryCriteria, ITestDiscoveryEventsHandler2 eventHandler)
[25/09/2019 11:13:45.899 PM Informational] [xUnit.net 00:00:00.3081508]   Discovering: Microsoft.Data.SqlClient.Tests
[25/09/2019 11:13:45.970 PM Informational] [xUnit.net 00:00:00.3776758]   Discovering: Microsoft.Data.SqlClient.ManualTesting.Tests
[25/09/2019 11:13:46.350 PM Informational] [xUnit.net 00:00:00.6369007]   Discovered:  Microsoft.Data.SqlClient.ManualTesting.Tests
[25/09/2019 11:13:46.456 PM Informational] [xUnit.net 00:00:00.7167884]   Discovered:  Microsoft.Data.SqlClient.Tests
[25/09/2019 11:13:46.962 PM Warning] No test is available in E:\Programming\csharp7\SqlClient\bin\Windows_NT\Debug.AnyCPU\Microsoft.Data.SqlClient\netfx\Microsoft.Data.SqlClient.dll. Make sure that test discoverer & executors are registered and platform & framework version settings are appropriate and try again.
[25/09/2019 11:13:47.024 PM Informational] ========== Discovery aborted: -1 tests found (0:00:01.9150349) ==========

I can't use the coverage file you provided because I'm only on visual studio pro. Code coverage is an enterprise or ultimate feature and since I buy my own visual studio license rather than getting it through an employer or volume license both editions are way out of my reach. If I could get the test running resharper should provide coverage, but that hits the error above.

cheenamalhotra · 2019-09-25T22:29:25Z

Hi @Wraith2
Try running in Release configuration, we're aware of Debug Configuration test issues in Visual Studio.

Wraith2 · 2019-09-25T23:20:04Z

None of the release mode configurations work for me either. I get an exception from inside xunit extensions. The coverage cli commands suggested aren't recognised at the commandline when appended to the test commands and the dotnet cli docs don't list them even though I can find the feature request for them being added, perhaps post 2.1 dotnet only. using /p:CollectCoverage=true doesn't seem to do anything either.

I've opened a couple of tracking issues for test and coverage. At the moment I can't provide any coverage data.

Wraith2 · 2019-11-21T14:16:33Z

Now that 1.1.0 is out can this be reviewed please? It's been rebased and is only failing on that AE tests that fail in master as well. This work is needed for the batch implementation.

cheenamalhotra · 2019-11-21T17:02:43Z

Hi @Wraith2

Yes, please update your branch with changes from master to trigger a fresh CI build.

Wraith2 · 2019-11-22T10:07:20Z

Updated.

cheenamalhotra · 2019-11-22T18:35:45Z

Thanks @Wraith2

It looks like the Enclave Provider tests are failing for .NET Core, please check logs in the checks (they're public now) and review implementation to find related code changes. It's tricky to get the setup for Enclave Provider for debugging, so I'd recommend going down your changes and understanding failing tests to fix what went wrong.

You'd be the best person to review first and fix the issues being creator of the PR, we'll start reviewing once we have all tests passing and changes are ready to check-in. If you need any help let us know.

From the logs:

System.NullReferenceException : Object reference not set to an instance of an object.
   at Microsoft.Data.SqlClient.SqlCommand.ReadDescribeEncryptionParameterResults(SqlDataReader ds, ReadOnlyDictionary`2 describeParameterEncryptionRpcOriginalRpcMap) in E:\ci-agent4\1\s\src\Microsoft.Data.SqlClient\netcore\src\Microsoft\Data\SqlClient\SqlCommand.cs:line 3762
   at Microsoft.Data.SqlClient.SqlCommand.PrepareForTransparentEncryption(CommandBehavior cmdBehavior, Boolean returnStream, Boolean isAsync, Int32 timeout, TaskCompletionSource`1 completion, Task& returnTask, Boolean asyncWrite, Boolean& usedCache, Boolean inRetry) in E:\ci-agent4\1\s\src\Microsoft.Data.SqlClient\netcore\src\Microsoft\Data\SqlClient\SqlCommand.cs:line 3153
   at Microsoft.Data.SqlClient.SqlCommand.RunExecuteReader(CommandBehavior cmdBehavior, RunBehavior runBehavior, Boolean returnStream, TaskCompletionSource`1 completion, Int32 timeout, Task& task, Boolean& usedCache, Boolean asyncWrite, Boolean inRetry, String method) in E:\ci-agent4\1\s\src\Microsoft.Data.SqlClient\netcore\src\Microsoft\Data\SqlClient\SqlCommand.cs:line 3967
   at Microsoft.Data.SqlClient.SqlCommand.RunExecuteReader(CommandBehavior cmdBehavior, RunBehavior runBehavior, Boolean returnStream, String method) in E:\ci-agent4\1\s\src\Microsoft.Data.SqlClient\netcore\src\Microsoft\Data\SqlClient\SqlCommand.cs:line 3916
   at Microsoft.Data.SqlClient.SqlCommand.ExecuteReader(CommandBehavior behavior) in E:\ci-agent4\1\s\src\Microsoft.Data.SqlClient\netcore\src\Microsoft\Data\SqlClient\SqlCommand.cs:line 1759
   at Microsoft.Data.SqlClient.SqlCommand.ExecuteReader() in E:\ci-agent4\1\s\src\Microsoft.Data.SqlClient\netcore\src\Microsoft\Data\SqlClient\SqlCommand.cs:line 1736
   at Microsoft.Data.SqlClient.ManualTesting.Tests.AlwaysEncrypted.End2EndSmokeTests.TestSelectOnEncryptedNonEncryptedColumns(String connString, String selectQuery, Int32 totalColumnsInSelect, String[] types) in E:\ci-agent4\1\s\src\Microsoft.Data.SqlClient\tests\ManualTests\AlwaysEncrypted\End2EndSmokeTests.cs:line 49

Wraith2 · 2019-11-22T19:45:08Z

So does SQL17W-E mean with enclave? I can guess from stack traces but realistically if I'm (any anyone else) going to need to ensure enclaves work we'll need instructions on how to set up and run those tests correctly, can you provide something?

cheenamalhotra · 2019-11-22T20:00:46Z

Sure, you can begin from here: https://docs.microsoft.com/en-us/sql/relational-databases/security/tutorial-getting-started-with-always-encrypted-enclaves for setup, and use following tutorials to setup .NET Applications, same instructions apply for Microsoft.Data.SqlClient, except that Enclave provider is internally available and does not need additional package reference.

We are also just-in-process of putting specific documentation for Microsoft.Data.SqlClient, since now Enclave provider is available in GA 1.1

Wraith2 · 2019-11-29T11:51:29Z

The changes are done and pass locally on the same tests as master. The SQL12E-E leg is failing with timeouts and flaky tests from what I can see, can you retry that leg?

cheenamalhotra · 2019-11-29T23:50:51Z

Hi @Wraith2

Please trigger a new build by pushing a commit. DevOps recently has some issues with Rebuild.

Wraith2 · 2019-12-02T09:29:52Z

New build triggered and looks like it's been stuck overnight.

David-Engel

Other than the few small nits, looks good to me. Thanks!

src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlCommand.cs

cheenamalhotra

Please resolve conflicts and comments, and then we're good!

This was referenced Sep 24, 2019

Implement new ADO.NET batching API mysql-net/MySqlConnector#650

Closed

SqlClient: Implement optimized version of the new ADO.NET batching API #19

Closed

cheenamalhotra added the 📈 Performance Issues that are targeted to performance improvements. label Oct 7, 2019

Wraith2 force-pushed the rpc-rework branch from f936224 to c7f1b41 Compare November 10, 2019 17:07

cheenamalhotra requested review from cheenamalhotra, David-Engel, JRahnama and karinazhou November 21, 2019 17:52

cheenamalhotra added this to the 1.2.0-preview1 milestone Nov 21, 2019

port rpc changes from corefx

1e3a141

Wraith2 force-pushed the rpc-rework branch from c7f1b41 to 1e3a141 Compare November 22, 2019 09:59

Wraith2 added 4 commits November 27, 2019 19:21

fixup encryption rpc

7a15a0b

Merge remote-tracking branch 'dotnet/master' into rpc-rework

bc27066

reuse parameter variables

bfd7b7e

fixup parameter access to use collection

b312bd5

merge DoneProc implementations

280ea94

David-Engel approved these changes Dec 5, 2019

View reviewed changes

cheenamalhotra approved these changes Dec 5, 2019

View reviewed changes

Wraith2 added 2 commits December 6, 2019 13:53

address feedback

b3f254b

address feedback

14063d2

David-Engel approved these changes Dec 6, 2019

View reviewed changes

cheenamalhotra merged commit 688adb1 into dotnet:master Dec 6, 2019

Wraith2 deleted the rpc-rework branch March 19, 2020 10:56

Wraith2 mentioned this pull request Oct 24, 2022

Port SqlParameter changes from netcore to netfx #1812

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf: Port rpc changes from corefx #209

perf: Port rpc changes from corefx #209

Wraith2 commented Sep 22, 2019

cheenamalhotra commented Sep 24, 2019

Wraith2 commented Sep 24, 2019

Wraith2 commented Sep 25, 2019

cheenamalhotra commented Sep 25, 2019

cheenamalhotra commented Sep 25, 2019

Wraith2 commented Sep 25, 2019

cheenamalhotra commented Sep 25, 2019

Wraith2 commented Sep 25, 2019

Wraith2 commented Nov 21, 2019

cheenamalhotra commented Nov 21, 2019

Wraith2 commented Nov 22, 2019

cheenamalhotra commented Nov 22, 2019 •

edited

Loading

Wraith2 commented Nov 22, 2019

cheenamalhotra commented Nov 22, 2019 •

edited

Loading

Wraith2 commented Nov 29, 2019

cheenamalhotra commented Nov 29, 2019

Wraith2 commented Dec 2, 2019

David-Engel left a comment

cheenamalhotra left a comment •

edited

Loading

perf: Port rpc changes from corefx #209

perf: Port rpc changes from corefx #209

Conversation

Wraith2 commented Sep 22, 2019

cheenamalhotra commented Sep 24, 2019

Wraith2 commented Sep 24, 2019

Wraith2 commented Sep 25, 2019

cheenamalhotra commented Sep 25, 2019

cheenamalhotra commented Sep 25, 2019

Wraith2 commented Sep 25, 2019

cheenamalhotra commented Sep 25, 2019

Wraith2 commented Sep 25, 2019

Wraith2 commented Nov 21, 2019

cheenamalhotra commented Nov 21, 2019

Wraith2 commented Nov 22, 2019

cheenamalhotra commented Nov 22, 2019 • edited Loading

Wraith2 commented Nov 22, 2019

cheenamalhotra commented Nov 22, 2019 • edited Loading

Wraith2 commented Nov 29, 2019

cheenamalhotra commented Nov 29, 2019

Wraith2 commented Dec 2, 2019

David-Engel left a comment

Choose a reason for hiding this comment

cheenamalhotra left a comment • edited Loading

Choose a reason for hiding this comment

cheenamalhotra commented Nov 22, 2019 •

edited

Loading

cheenamalhotra commented Nov 22, 2019 •

edited

Loading

cheenamalhotra left a comment •

edited

Loading