Add datetime read span path for netcore #31044

Wraith2 · 2018-07-13T00:42:18Z

Second attempt at the PR with a clean branch and including all feedback from the original at corefx/30371

Locally I'm getting a problem with the allconfigurations build not finding mscorlib but I can't fathom why my changes would be responsible for that so I'm hoping CI doesn't have the same issue.

fixes https://github.com/dotnet/corefx/issues/28270

/cc @EgorBo @afsanehr @cod7alex

Anipik · 2018-07-17T04:30:43Z

src/System.Data.SqlClient/src/System/Data/SqlClient/SqlBuffer.NetCoreApp.cs

+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+


can you please remove line 5- line 9

Done, they were copied from original non-netcore file so perhaps some cleanup elsewhere might be a good idea.

Anipik · 2018-07-17T04:34:51Z

src/System.Data.SqlClient/src/System.Data.SqlClient.csproj

@@ -18,6 +18,8 @@
  </ItemGroup>
  <ItemGroup Condition="'$(IsPartialFacadeAssembly)' != 'true' AND '$(OSGroup)' != 'AnyOS' AND '$(TargetGroup)' == 'netcoreapp'">
    <Compile Include="System\Data\SqlClient\PoolBlockingPeriod.cs" />
+    <Compile Include="System\Data\SqlClient\SqlBuffer.NetCoreApp.cs" />


change to lowercase netcoreapp

why are we spliting the implementation into different files as netfx and netstandard dont use this implementation ?

I followed the existing use of mixed case in the project, I can make it lower for this one but it won't be consistent throughout.

It was split apart at request of @afsanehr in the original review on #30371

we are using the lowercase netcoreapp everywhere else in the repo when it is appearing as an extension.

@Anipik The reason I used .NetCoreApp in #30371 is because we previously had this file in SqlClient named as .NetCoreApp. Do you propose changing this file as well?

We are pretty inconsistent about using the .netcoreapp.cs suffix for implementation files that are conditionalized with '$(TargetGroup)' == 'netcoreapp'. The pattern is mainly for test files, since those are more often shared with .NET Framework. Personally I wouldn't use the suffix in implementation files but I don't think it matters although hopefully it's consistent within a project.
Casing should be lower case.

Anipik · 2018-07-17T04:39:17Z

src/System.Data.SqlClient/src/System/Data/SqlClient/SqlBuffer.cs

@@ -1109,7 +1109,11 @@ internal void SetToDateTimeOffset(DateTimeOffset dateTimeOffset, byte scale)
            _isNull = false;
        }

+#if netcoreapp
+        private static void FillInTimeInfo(ref TimeInfo timeInfo, ReadOnlySpan<byte> timeBytes, int length, byte scale)


why cant we just keep the span version ?

Only the netcore build has a dependency on System.Memory which is required for span. Rather than force all netfx and netcore consumers to pull in that and possibly a full set of interdependent assemblies supporting it I talked with @karelz in gitter and he suggested making it netcore specific. If someone wants to make the decision to require span for all versions then everyone can use the span implementation.

The netfx wont use this implementation. AFAIK the implementatation in the src folder is only built and compiled for netcoreapp which makes the if def redundant.

@danmosemsft am i missing something here ?

@stephentoub thoughts about this pattern?
We try to avoid #if platform in our implementation.

I'm definitely missing something here. The conditions for inclusion of SqlBuffer and TdsStateParserObject in the project file are simply Condition="'$(IsPartialFacadeAssembly)' != 'true' AND '$(OSGroup)' != 'AnyOS'" You assert that netfx won't use this, how do you know this and where can I determine this? If this is the case then surely it still needs to build with build -allconfigurations because, again in the original PR I referenced, it didn't and I was asked to change it to pass the CI.

If neither code level #define's or conditionally included *.netcoreapp.cs files are appropriate to separate out particular functionality just how should it be done? How should this change be made without forcing a dependency on System.Memory on all consumers?

This source code is going to be compiled against netstandard, netcoreapp, uap and netfx, however for netfx this will be a partial facade assembly and none of the source files are included for netfx. We know this because IsPartialFacadeAssembly is set to true when targeting netfx:

corefx/src/System.Data.SqlClient/src/System.Data.SqlClient.csproj

Line 6 in c26eb56

<IsPartialFacadeAssembly Condition="'$(TargetsNetFx)' == 'true'">true</IsPartialFacadeAssembly>

And System.Memory is inbox in Uap and Netcoreapp so there shouldn't be a problem on adding a reference to it:

corefx/src/System.Memory/Directory.Build.props

Lines 9 to 10 in c26eb56

<IsNETCoreApp>true</IsNETCoreApp>

<IsUAP>true</IsUAP>

System.Memory is already referenced in this project when TargetsWindows == true, so it won't hurt moving this to when IsPartialFacade != true and AnyOS != true. For netstandard it doesn't hurt anything to add this dependency, as System.Data.SqlClient in netstandard is an OOB package.

This source code is going to be compiled against netstandard, netcoreapp, uap and netfx, however for netfx this will be a partial facade assembly and none of the source files are included for netfx.

Agreed.

And System.Memory is inbox in Uap and Netcoreapp so there shouldn't be a problem on adding a reference to it

That is correct.

Anipik · 2018-07-17T04:39:51Z

src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParser.cs

@@ -4540,9 +4540,15 @@ internal bool TryReadSqlValue(SqlBuffer value, SqlMetaDataPriv md, int length, T

        private bool TryReadSqlDateTime(SqlBuffer value, byte tdsType, int length, byte scale, TdsParserStateObject stateObj)
        {
+#if netcoreapp
+            Span<byte> datetimeBuffer = length <= 10 ? stackalloc byte[length] : new byte[length];


why not just the span version ?

System.Memory is only included in specific builds.

Anipik · 2018-07-17T04:44:25Z

src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParserStateObject.NetCoreApp.cs

+            }
+
+            AssertValidState();
+


nit extra line

Wraith2 · 2018-08-08T18:36:41Z

Is more work needed on this to get it merged?

Anipik · 2018-08-08T18:44:49Z

@stephentoub @safern can you take a final look ?

Wraith2 · 2018-08-12T12:46:47Z

General spannification style questions.
Converting a method signature from bool TryReadByteArray(byte[] buff, int offset, int len) to bool TryReadByteArray(Span<byte> buff, int len),
At call sites prefer extension buff.AsSpan(offset) or explicit span creation new Span(buff, offset).
At call sites where offset is 0 prefer explicit 0 slice (or new as above) or drop the 0 parameter and make it implicit?

karelz · 2018-08-15T15:45:48Z

@safern can you please help push it forward?
@keeratsingh @afsanehr @David-Engel can you please review it too?

safern · 2018-08-15T18:22:23Z

I'm waiting for this comment to be addressed: #31044 (comment)

@ahsonkhan could you help with the span question?

ahsonkhan · 2018-08-15T21:57:42Z

I'm waiting for this comment to be addressed

Is more work needed on this to get it merged?

You do not need to add netcoreapp specific ifdefs here at all. The only change required is changing the existing internal APIs to take Span<byte> instead of arrays, and potentially remove the offset/length arguments (where possible). You should use Slice/AsSpan to enforce the bounds checks (and simplify some Debug.Asserts).

See these commits as an example of what could be done to "spanify" the implementation: 9f6ace6 & 6ae0ffd(which will build successfully for all configurations). Feel free to use this as a starting point and remove offsets/lengths from the callers, however much is feasible.

General spannification style questions.
Converting a method signature from bool TryReadByteArray(byte[] buff, int offset, int len) to bool TryReadByteArray(Span<byte> buff, int len),
At call sites prefer extension buff.AsSpan(offset) or explicit span creation new Span(buff, offset).
At call sites where offset is 0 prefer explicit 0 slice (or new as above) or drop the 0 parameter and make it implicit?

Using AsSpan() or creating a Span using the new keyword should have no difference. If you want to chain multiple calls together, or passing it as an argument, the AsSpan() API is cleaner.
Slicing with offset 0 should be OK (since the operation is fairly cheap). If this is a really performance critical code path, you can add an if check and avoid slicing. But unless benchmarks show otherwise, I would keep it simple. Do you have performance numbers showing before and after? Looking at https://github.com/dotnet/corefx/issues/28270#issuecomment-395999251, the results are in milliseconds, so optimizing on when to Slice (which is on the order of nanoseconds) shouldn't matter.
In some cases, you should be able to encapsulate offset & length into a span slice (not just offset).

Feel free to ask additional questions, and I will help as best I can :)

ahsonkhan · 2018-08-15T21:59:39Z

src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParserStateObject.netcoreapp.cs

+        public bool TryReadByteArray(Span<byte> buff, int len)
+        {
+            int ignored;
+            return TryReadByteArray(buff, len, out ignored);


nit, can be changed to:

return TryReadByteArray(buff, len, out _);

Or even:

return TryReadByteArray(buff.Slice(0, len), out _);

Wraith2 · 2018-08-24T07:58:21Z

All addressed, it still needs signoff from @GrabYourPitchforks on the span security changes and @afsanehr to review.

ahsonkhan · 2018-08-24T20:09:49Z

@Wraith2, since this is a performance improvement (fixing System.Data.SqlClient.TdsParser performance improvement #28270), do you have performance measurements showing the improvement?

Wraith2 · 2018-08-24T20:31:42Z

I did have on the original PR. The intent was to remove frequent byte[] allocations and it does that so heap allocs on reading datatime's now don't happen. Any cpu perf would be entirely accidental.

Found them, on the references issue: https://github.com/dotnet/corefx/issues/28270#issuecomment-395999251

danmoseley · 2018-09-03T02:51:20Z

@GrabYourPitchforks @afsanehr can you please take a look so we can get this finished off.

karelz · 2018-09-05T17:22:45Z

@afsanehr can you please merge the PR if there is nothing that needs further scrutiny? Thanks!

AfsanehR-zz · 2018-09-05T17:31:57Z

Sure, waiting for @GrabYourPitchforks to approve the changes.

Wraith2 · 2018-09-13T19:00:55Z

Can anyone in the geographic locale of @GrabYourPitchforks organise a search party?

GrabYourPitchforks · 2018-09-13T19:49:03Z

/me puts his ears back and hisses as the search party enters his cave, interrupting his hibernation.

AfsanehR-zz · 2018-09-13T22:30:11Z

@Wraith2 I am going to hold off merging this pr for now as I noticed some failures in ManualTests in SqlClient. One of them being SplitPacketTest. Could you try the following command to build the test and see if you see the same failure?

pushd C:\Users\Documents\corefx\bin\tests\System.Data.SqlClient.ManualTesting.Tests\netcoreapp-Windows_NT-Debug-x64\ 
call C:\Users\Documents\corefx\bin\testhost\netcoreapp-Windows_NT-Debug-x64\\dotnet.exe xunit.console.netcore.exe System.Data.SqlClient.ManualTesting.Tests.dll  -xml testResults.xml -notrait Benchmark=true -notrait category=nonnetcoreapptests -notrait category=nonwindowstests -class System.Data.SqlClient.ManualTesting.Tests.SplitPacketTest  -notrait category=OuterLoop -notrait category=failing
  popd

Wraith2 · 2018-09-14T16:21:26Z

Yes, I can replicate those test failures. I've no way to debug them though since I can only run tests through the commandline, there's simply too much code to start trying to trace it all the way from the network layer that way.

AfsanehR-zz · 2018-09-18T16:57:22Z

@Wraith2 Try using recommendation for debugging in here

I suspect the issue might be coming from this line:

corefx/src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParser.cs

Line 707 in 18789a8

payloadOffset = payload[offset++] << 8 | payload[offset++];

where payloadLength is:

corefx/src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParser.cs

Line 684 in 18789a8

byte[] payload = new byte[_physicalStateObj._inBytesPacket];

karelz · 2018-10-02T00:03:12Z

Any update? Is it still blocked on test failures? I see CI all green.

stephentoub · 2018-10-02T14:10:30Z

src/System.Data.SqlClient/src/System/Data/SqlClient/SqlBuffer.cs

            _isNull = false;
        }

-        internal void SetToTime(TimeSpan timeSpan, byte scale)
+        internal void SetToDateTime2(ReadOnlySpan<byte> bytes, byte scale)


What does the 2 mean in this method name? It's not very descriptive.

It means that it's using the data layout for an sql type SQLDATETIME2, which represents a datetime2 and I agree that it's a silly name but that's what it's called.

Ugh. Ok, thanks.

Wraith2 · 2018-10-02T19:05:12Z

      Any update? Is it still blocked on test failures? I see CI all green.

Still blocked on the manual tests. I've got to find a way to work out why a test is failing without being able to sensibly debug it and the network io being split into individual bytes. After that I will be attempting to juggle flaming chainsaws.

Wraith2 · 2018-10-03T10:31:28Z

The span translation of TryReadByteArray wasn't using handling offsets correctly which only surfaced when the manual tests fragmented the input stream so that full reads weren't possible. This is fixed, the standard and manual tests now pass. @afsanehr can you please review?

ahsonkhan · 2018-10-03T18:50:29Z

src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParserStateObject.cs

                {
-                    Buffer.BlockCopy(_inBuff, _inBytesUsed, buff, offset + totalRead, bytesToRead);
+                    var copyFrom = new ReadOnlySpan<byte>(_inBuff, _inBytesUsed, bytesToRead);
+                    var copyTo = buff.Slice(totalRead, bytesToRead);


nit: don't use var here since the right-hand side doesn't indicate the type explicitly.

AfsanehR-zz · 2018-10-03T20:27:01Z

@Wraith2 Thanks! I can also confirm the manual tests are passing now. Once the comment from @ahsonkhan is addressed we are good to merge this pr. 🎉

Wraith2 · 2018-10-04T10:49:16Z

Done.

Port PR 31044 Relevant description of problem For each call of the method we allocate the array of bytes `( byte[] datetimeBuffer = new byte[length];)` instead of allocating it once and then reuse existing array. Let's imagine that we read 1 million DateTime values from database. Using existing implementation of TdsParser we will have 1 million allocations of byte[] what causes unnecessary memory traffic and isn't so good for GC. Related Link dotnet/corefx#31044

Add datetime read span path for netcore Commit migrated from dotnet/corefx@10ccbc1

add datetime read span path for netcore

09ce37f

Wraith2 changed the title ~~[WIP] add datetime read span path for netcore~~ Add datetime read span path for netcore Jul 13, 2018

AfsanehR-zz self-requested a review July 17, 2018 00:42

Anipik reviewed Jul 17, 2018

View reviewed changes

src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParserStateObject.NetCoreApp.cs Outdated

}

AssertValidState();

Copy link

Anipik Jul 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit extra line

address feedback

6358d2a

karelz added this to the 3.0 milestone Jul 19, 2018

Wraith2 added 4 commits July 23, 2018 00:19

correct filename case in project

b1c7cfe

remove netcore files

9514fc7

add netcore files with correct case

221498f

fix other case in project

2bec7ea

Wraith2 added 2 commits August 12, 2018 14:03

remove netcore #ifdefs

2f78a0b

Merge remote-tracking branch 'origin/master' into sqldatespan2

0fb752f

karelz added the area-System.Data.SqlClient label Aug 15, 2018

karelz assigned Wraith2, AfsanehR-zz, David-Engel and keeratsingh Aug 15, 2018

ahsonkhan reviewed Aug 15, 2018

View reviewed changes

Wraith2 added 2 commits August 15, 2018 23:15

Merge branch 'master' into sqldatespan2

c0b5e0b

change out variable to c#7 dicard

75fb446

remove length parameters from spanified functions

18789a8

ahsonkhan approved these changes Aug 24, 2018

View reviewed changes

AfsanehR-zz approved these changes Sep 5, 2018

View reviewed changes

GrabYourPitchforks approved these changes Sep 13, 2018

View reviewed changes

joshfree approved these changes Sep 13, 2018

View reviewed changes

stephentoub reviewed Oct 2, 2018

View reviewed changes

fixed buffer copy to correctly use offset

8a2c3aa

ahsonkhan reviewed Oct 3, 2018

View reviewed changes

makes type declaration of copy target explicit

8618058

AfsanehR-zz merged commit 10ccbc1 into dotnet:master Oct 4, 2018

Wraith2 deleted the sqldatespan2 branch October 4, 2018 23:55

Wraith2 mentioned this pull request Dec 21, 2021

Share SqlBuffer dotnet/SqlClient#1438

Merged

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Merge pull request dotnet/corefx#31044 from Wraith2/sqldatespan2

3b8fae6

Add datetime read span path for netcore Commit migrated from dotnet/corefx@10ccbc1

Add datetime read span path for netcore #31044

Add datetime read span path for netcore #31044

Conversation

Wraith2 commented Jul 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wraith2 commented Aug 8, 2018

Anipik commented Aug 8, 2018

Wraith2 commented Aug 12, 2018

karelz commented Aug 15, 2018

safern commented Aug 15, 2018

ahsonkhan commented Aug 15, 2018 • edited Loading

Choose a reason for hiding this comment

Wraith2 commented Aug 24, 2018 • edited Loading

ahsonkhan commented Aug 24, 2018

Wraith2 commented Aug 24, 2018 • edited Loading

danmoseley commented Sep 3, 2018

karelz commented Sep 5, 2018

AfsanehR-zz commented Sep 5, 2018

Wraith2 commented Sep 13, 2018

GrabYourPitchforks commented Sep 13, 2018

AfsanehR-zz commented Sep 13, 2018 • edited Loading

Wraith2 commented Sep 14, 2018

AfsanehR-zz commented Sep 18, 2018

karelz commented Oct 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wraith2 commented Oct 2, 2018

Wraith2 commented Oct 3, 2018

Choose a reason for hiding this comment

AfsanehR-zz commented Oct 3, 2018

Wraith2 commented Oct 4, 2018

ahsonkhan commented Aug 15, 2018 •

edited

Loading

Wraith2 commented Aug 24, 2018 •

edited

Loading

Wraith2 commented Aug 24, 2018 •

edited

Loading

AfsanehR-zz commented Sep 13, 2018 •

edited

Loading