Skip to content
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

Optimize SqlClient primitive type writes #34048

Merged
merged 5 commits into from Dec 17, 2018

Conversation

Projects
None yet
3 participants
@Wraith2
Copy link
Collaborator

commented Dec 12, 2018

When writing some primitive types to the tds packet stream small arrays are allocated and passed to the WriteByteArray function. I have changed WriteByteArray to WriteBytes and added a span parameter so that either a span or an array can be passed in (but not both, the priority is explained in the comments). The function will attempt to write using only a span unless it encounters an end of packet and needs to use an async continuation, in that case it will allocate an array and copy the remainder of the span into it. For primitive types this will rarely be needed.

These changes can be used to allow the new BitConverter.TryWriteBytes overload with stackalloced buffers to remove memory allocations for writing of 16 and 32 bit reals. This can be extended to guids at a later time.

This PR was split from #32811 and into smaller commits for easier review. It has passed the manual and efcore tests in native mode, the tests cannot be successfully run in managed mode due to #33930 . Performance improvements for this and related PR's are in the original discussion.
/cc @keeratsingh @AfsanehR @saurabh500 @David-Engel

Wraith2 added some commits Dec 11, 2018

change public WriteArrayBytes to private WriteBytes and add span capa…
…ble path with fallback to array on continuation

add WriteByteSpan and WriteByteArray public methods
add netcoreapp specific implementations of WriteFloat and WriteDouble…
… which avoid byte array allocation in favour of span

@Wraith2 Wraith2 changed the title Optimize SqlClient primative type writes Optimize SqlClient primitive type writes Dec 12, 2018

{
byte[] tempArray = new byte[len];
Span<byte> copyTempTo = tempArray.AsSpan();
ReadOnlySpan<byte> copyTempFrom = b.Slice(remainder, len);

This comment has been minimized.

Copy link
@saurabh500

saurabh500 Dec 14, 2018

Member

I am confused. Why do you need b.Slice again here? b.Slice(remainder) was already called on line 3062

This comment has been minimized.

Copy link
@Wraith2

Wraith2 Dec 14, 2018

Author Collaborator

Good question. The len needs to be there so that we only copy data not empty buffer in the case where a small part of an array is used (pool rental for example). The remainder should be changed to 0 though. I've changed it and I'm re-running the test suites.

This comment has been minimized.

Copy link
@Wraith2

Wraith2 Dec 14, 2018

Author Collaborator

manual and efcore tests all pass with the push from earlier today.

This comment has been minimized.

Copy link
@saurabh500

saurabh500 Dec 14, 2018

Member

Is this testing status for Windows only or both Windows and Linux?

This comment has been minimized.

Copy link
@Wraith2

Wraith2 Dec 14, 2018

Author Collaborator

windows/native, this bit is shared though. I can force it into managed mode and test that way as well.

This comment has been minimized.

Copy link
@saurabh500

saurabh500 Dec 14, 2018

Member

OK. I will run the Unix validations.

@saurabh500

This comment has been minimized.

Copy link
Member

commented Dec 14, 2018

@dotnet-bot Test Windows x86 Release Build Please
Failure in System.Runtime.Tests

01:00 != 00:00:00, dn:(UTC+01:00) Casablanca, sn:(UTC+01:00) Casablanca
Expected: True
Actual: False

   at System.Tests.TimeZoneInfoTests.TimeZoneInfo_DisplayNameStartsWithOffset() in D:\j\workspace\windows-TGrou---f8ac6754\src\System.Runtime\tests\System\TimeZoneInfoTests.cs:line 2227

https://mc.dot.net/#/user/Wraith2/pr~2Fjenkins~2Fdotnet~2Fcorefx~2Fmaster~2F/test~2Ffunctional~2Fcli~2F/46d98751a960f8363255da765ff76a58bdba68d9/workItem/System.Runtime.Tests/analysis/xunit/System.Tests.TimeZoneInfoTests~2FTimeZoneInfo_DisplayNameStartsWithOffset

{
byte[] tempArray = new byte[len];
Span<byte> copyTempTo = tempArray.AsSpan();
ReadOnlySpan<byte> copyTempFrom = b.Slice(0, len);

This comment has been minimized.

Copy link
@saurabh500

saurabh500 Dec 14, 2018

Member

Can't you just get rid of the call at 3048. It doesn't seem to be doing anything useful. And call b.Slice(remainder, len) like you were doing earlier, at this location? if ( array == null) is the only location where you are going to get the rest of the content of b
Does this make sense ?

This comment has been minimized.

Copy link
@Wraith2

Wraith2 Dec 14, 2018

Author Collaborator

No, there is no guarantee that the WritePacket call will return a task so it's possible to go around the loop without allocating the array. This was a problem that occurred with one of the SplitPacket tests that was pointed out on the original PR.

This comment has been minimized.

Copy link
@saurabh500

saurabh500 Dec 14, 2018

Member

OK, now I understand. In that case, what is the effect of just doing a b = b.Slice(remainder, len) at 3048 and eliminate this slicing at 3066. Would that work? Then this line would read ReadOnlySpan<byte> copyTempFrom = b;

@saurabh500
Copy link
Member

left a comment

Merge pending product team validations.

@saurabh500

This comment has been minimized.

Copy link
Member

commented Dec 14, 2018

@dotnet-bot Test Packaging All Configurations x64 Debug Build please

@saurabh500

This comment has been minimized.

Copy link
Member

commented Dec 15, 2018

I am facing some issues with EF tests failure with this change. I don't think this PR caused it but I wanna be sure.
I will keep you posted.

@Wraith2

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 15, 2018

Managed or native? and what are you seeing fail?

@saurabh500

This comment has been minimized.

Copy link
Member

commented Dec 17, 2018

LGTM. Tested Manual Tests on Ubuntu,
EFCore Ubuntu with MARS enabled
EFCore Ubuntu with Rando, MARS.

@saurabh500 saurabh500 merged commit 245d532 into dotnet:master Dec 17, 2018

14 checks passed

Linux arm Release Build Build finished.
Details
Linux arm64 Release Build Build finished.
Details
Linux x64 Release Build Build finished.
Details
Linux-musl x64 Debug Build Build finished.
Details
NETFX x86 Release Build Build finished.
Details
OSX x64 Debug Build Build finished.
Details
Packaging All Configurations x64 Debug Build Build finished.
Details
Tizen armel Debug Build Build finished.
Details
UWP CoreCLR x64 Debug Build Build finished.
Details
UWP NETNative x86 Release Build Build finished.
Details
WIP Ready for review
Details
Windows x64 Debug Build Build finished.
Details
Windows x86 Release Build Build finished.
Details
license/cla All CLA requirements met.
Details

@Wraith2 Wraith2 deleted the Wraith2:sqlperf-writespan branch Dec 17, 2018

@karelz karelz added this to the 3.0 milestone Dec 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.