Perf | Port PR #328 to improve performance in .NET Framework

[cheenamalhotra]

+#603

[Wraith2]

Are you porting from the #328 diffs or from the existing netcore code? I'd advise using the existing netcore code if possible because some changes were made after #328 which improve performance, like moving the cached instances down to the sql internal connection.

There is also #925 open which is worth looking at in case you want to do the same in netfx at the same time.

[cheenamalhotra]

Only the #328 diffs here, I see many more differences in async design which I will target to do later, thanks for checking in!

I tried doing that too but ended up with a very large change-set. I'll try to bring few at a time so we can acknowledge all differences in portions and eventually merge these files (optimism 😌).

The PR #328 brought perf benefits in 'NextResultAsync' flow and it's a considerably major improvement to port.

[Wraith2]

Ok, the only really important thing is to make sure you port over #603 which fixed an error in this rework, everything else is just additional improvements.

[cheenamalhotra]

Thanks, I was stuck on exactly that test!

[Wraith2]

You might want to include #925 (which I think is the source of conflicts). When the two versions of the Async context classes are the same I can put together a merge PR to share some of those files.

[cheenamalhotra]

Thanks @Wraith2

When the two versions of the Async context classes are the same I can put together a merge PR to share some of those files.

I think we should plan bringing Async design to parity after this PR gets fixed and merged.

[Wraith2]

after this PR gets fixed and merged.

What needs fixing?

[cheenamalhotra]

@Wraith2

The test "AsyncMultiPacketStreamRead" is giving me hard time as it was brought in with #603, but even if netcore changes from #603 were ported to NetFx, the test continues to fail.

I'm looking at my last resort to do all the work from #925, #528, #499 in NetFx to bring code to parity, but do you know if there's anything else that NetFx is missing that might be contributing to this test failure?

[Wraith2]

I looked through the history when I saw you open the PR which is when I flagged up #925 . Nothing else stood out is being needed.

I'll reimplement this changeset on my local copy and see if I can reproduce the error and possibly help track it down.

[Wraith2]

How are you running the net461 tests? when i try i get this error:

========== Test discovery finished: 0 Tests found in 1.6 sec ==========
Starting test discovery for requested test run
========== Starting test discovery ==========
Test project Microsoft.Data.SqlClient.Tests does not reference any .NET NuGet adapter. Test discovery or execution might not work for this project.
It's recommended to reference NuGet test adapters in each test project in the solution.
Microsoft.VisualStudio.TestPlatform.ObjectModel.TestPlatformException: Unable to find E:\Programming\csharp7\SqlClient\artifacts\Project\bin\Windows_NT\Debug.AnyCPU.FunctionalTests\net461\win\FunctionalTests.deps.json. Make sure test project has a nuget reference of package "Microsoft.NET.Test.Sdk".
   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, String runSettings)
   at Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client.ProxyDiscoveryManager.DiscoverTests(DiscoveryCriteria discoveryCriteria, ITestDiscoveryEventsHandler2 eventHandler)
========== Test discovery aborted: 0 Tests found in 6 ms ==========

which is right that the deps file doesn't exist but then I don't think it's needed on netfx so I'm not sure what is looking for it.

[cheenamalhotra]

Run Manual Tests

I could debug in Visual Studio, so maybe try again with a "Rebuild Solution"

[Wraith2]

Tried that. Then I closed VS and did msbuild /t:clean to make sure everything was cleaned up completely, reopened VS and did a full build. Same occurs. The build works, the files for the tests are there but something wants a deps file and I've no idea what. this is a clean branch from the current master. netcore tests run, but that doesn't help in this situation.

[Wraith2]

This set of commands should work to build and run the tests from what i can see in the docs:

@echo off

set netfx=net461
set config=Debug

msbuild /t:clean
msbuild /p:Configuration=%config%
msbuild /t:BuildTestsNetFx /p:TargetNetFxVersion=%netfx%

dotnet test "src\Microsoft.Data.SqlClient\tests\ManualTests\Microsoft.Data.SqlClient.ManualTesting.Tests.csproj" -p:Platform="AnyCPU" -p:Configuration="%config%" -p:TestTargetOS="Windowsnetfx" --no-build -v n --filter "AsyncMultiPacketStreamRead"

but fails with:

Build FAILED.

"E:\Programming\csharp7\SqlClient\build.proj" (BuildTestsNetFx target) (1) ->
"E:\Programming\csharp7\SqlClient\src\Microsoft.Data.SqlClient\tests\FunctionalTests\Microsoft.Data.SqlClient.Tests.csp
roj" (default target) (17:15) ->
"E:\Programming\csharp7\SqlClient\src\Microsoft.Data.SqlClient\tests\FunctionalTests\Microsoft.Data.SqlClient.Tests.csp
roj" (Build target) (17:16) ->
(_CheckForMismatchingPlatform target) ->
  C:\Program Files\dotnet\sdk\5.0.400\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.RuntimeIdentifierInference.targets(1
68,5): error NETSDK1032: The RuntimeIdentifier platform 'win' and the PlatformTarget 'x64' must be compatible. [E:\Prog
ramming\csharp7\SqlClient\src\Microsoft.Data.SqlClient\tests\FunctionalTests\Microsoft.Data.SqlClient.Tests.csproj]

    0 Warning(s)
    1 Error(s)

So i don't know what's different between our setups but I'm using a clean checkout from main and things just don't work properly. If I can get to the point of being able to compile and debug the netfx tests then I'll be able to help but at the moment that isn't possible.

[Wraith2]

I managed to get the test case working using Debugger.Launch().

netfx is failing to move from seek mode into read mode in the get bytes async call. It does this because the _cancelAsyncOnCloseTokenSource has been cancelled. That token source has been cancelled because the reader has been closed on netfx while it hasn't on netcore. In the SqlDataReader.ctor if you add:

            _cancelAsyncOnCloseTokenSource.Token.Register(
                () => 
                Debug.WriteLine($"{_cancelAsyncOnCloseTokenSource} cancelled")
             );

you'll find that it is called very quickly on netfx and only when the operations has finished on netcore. We need to know why the Task that is being used by CopyStream from the stream reader is signalling completion early on netfx.

Got to do my day job at the moment but I thought I'd leave these notes here so when either of us get back to it we have a place to start.

[Wraith2]

Got it:

In this block in GetBytesAsyncReadDataStage there's a missing return statement

                Task<int> retryTask = ExecuteAsyncCall(context);
                if (isContinuation)
                {
                    // Let the caller handle cleanup\completing
                    return retryTask;
                }
                else
                {
                    Debug.Assert(context._source != null, "context._source shuld not be null when continuing");
                    // setup for cleanup\completing
                    retryTask.ContinueWith(
                        continuationAction: AAsyncCallContext<int>.s_completeCallback,
                        state: context,
                        TaskScheduler.Default
                    );
+                    return source.Task;
                }

Without it the first packet crossing read doesn't get waited for. It isn't waited for because falling out of that block returns null at the bottom of the method and that is treated as a synchronous completion by the caller. So you end up in a state where the caller thinks the call completed and returned 22 bytes and the call is actually still being worked on internally so when the caller then tries to initiate another call it hits the existing one and trips the exception.

Not fun to debug.

[cheenamalhotra]

In this block in GetBytesAsyncReadDataStage there's a missing return statement

I would have liked a compile error like "Not all paths return value" but we return null in the end. 😞

[Wraith2]

Can you consider changing the test from using stream.CopyToAsync to a local version of the function? To debug I needed to lookup the implementation of CopyToAsync so I could single step through and add tracing. When I did that I found that it's is using ArrayPool<byte>.Shared.Rent(bufferSize); which works but the behaviour of rent is to return an array sized equal or greater than the requested size and which is a power of 2 in size. In the test case we ask for a buffer of length 37 to trip up the packet spanning logic but the buffer we actually end up using it 64 bytes in size which isn't going to be as helpful.

I'd suggest something like the following which respect the buffersize:

#if NETFRAMEWORK
        private static async Task LocalCopyTo(Stream source, Stream destination, int bufferSize, CancellationToken cancellationToken)
        {
            byte[] buffer = ArrayPool<byte>.Shared.Rent(bufferSize);
            try
            {
                int bytesRead;
                while ((bytesRead = await source.ReadAsync(buffer,0, bufferSize, cancellationToken).ConfigureAwait(false)) != 0)
                {
                    await destination.WriteAsync(buffer, 0, bytesRead, cancellationToken).ConfigureAwait(false);
                }
            }
            finally
            {
                ArrayPool<byte>.Shared.Return(buffer);
            }
        }
#else
        private static async Task LocalCopyTo(Stream source, Stream destination, int bufferSize, CancellationToken cancellationToken)
        {
            byte[] buffer = ArrayPool<byte>.Shared.Rent(bufferSize);
            try
            {
                int bytesRead;
                while ((bytesRead = await source.ReadAsync(new Memory<byte>(buffer,0, bufferSize), cancellationToken).ConfigureAwait(false)) != 0)
                {
                    await destination.WriteAsync(new ReadOnlyMemory<byte>(buffer, 0, bytesRead), cancellationToken).ConfigureAwait(false);
                }
            }
            finally
            {
                ArrayPool<byte>.Shared.Return(buffer);
            }
        }
#endif 

[Wraith2]

@DavoudEshtehari I wouldn't worry too much about documentation at this point. Most of the async support functions are going to change names when the later PR's from netcore are ported on top of this. Once netfx and netcore are in sync I intend to share a lot of the code like the async call context classes between the projects to help keep everything in sync.