Use `NativeMemory` in `System.Data.Odbc` by xtqqczze · Pull Request #85966 · dotnet/runtime

xtqqczze · 2023-05-09T09:28:25Z

Contributes to #54297.

Although target frameworks for System.Data.Odbc include netstandard2.0 and $(NetFrameworkMinimum), these platforms are not supported (see #78550), therefore we can use the .NET 6 NativeMemory APIs.

ghost · 2023-05-09T09:28:37Z

Tagging subscribers to this area: @roji, @ajcvickers
See info in area-owners.md if you want to be subscribed.

Issue Details

Contributes to #54297.

Author:	xtqqczze
Assignees:	-
Labels:	`area-System.Data`, `community-contribution`
Milestone:	-

xtqqczze · 2023-05-09T09:29:09Z

Related: #54468, cc: @reflectronic.

Wraith2 · 2023-05-09T13:29:06Z

Does this have any specific benefits?

xtqqczze · 2023-05-09T14:11:50Z

Does this have any specific benefits?

Use of LocalAlloc is deprecated and NativeMemory.Alloc is faster.

EgorBo · 2023-05-09T14:36:05Z

src/libraries/System.Data.Odbc/src/Common/System/Data/Common/SafeNativeMethods.cs

-            var zeroes = new byte[length];
-            Marshal.Copy(zeroes, 0, ptr, length);
+#if !NET7_0_OR_GREATER
+            new Span<byte>((void*)ptr, length).Clear();


why not just Span.Clear for all TFMs ?

Codegen is marginally better, see https://www.diffchecker.com/Gxzd78oe

More significantly - the #if indicates we could just use NativeMemory.Clear once .NET 6 is end-of-life.

Codegen is marginally better, see https://www.diffchecker.com/Gxzd78oe

It seems that you compared codegen for constant length which is not the case here. Overall, Span.Clear should be better e.g. it's faster for small sizes as it won't pay price for the interop machinery so I assume it's better to just always use that instead of #if-else. If it's for some reason slower it can be a good motivation for us to fix that in BCL/JIT

Codegen is not for constant length, see https://csharp.godbolt.org/z/a157vhTT9

The implementation for NativeMemory.Clear actually uses SpanHelpers.ClearWithoutReferences(ref *(byte*)ptr, byteCount)

Actually, since NativeMemory.Alloc guarantees ptr is pointer-aligned, and Length is always IntPtr aligned, we can use Unsafe.InitBlock.

xtqqczze · 2023-05-09T17:32:22Z

Test failures are unrelated and have been linked to existing issues.

stephentoub · 2023-05-15T14:14:29Z

src/libraries/System.Data.Odbc/src/Common/System/Data/ProviderBase/DbBuffer.cs

-        protected DbBuffer(int initialSize) : base(IntPtr.Zero, true)
+        protected unsafe DbBuffer(int initialSize) : base(IntPtr.Zero, true)
        {
+            Debug.Assert(initialSize % IntPtr.Size == 0, $"Expected aligned {nameof(initialSize)}.");


What guarantees this?

DbBuffer is is only instantiated from the derived class CNativeBuffer.

The buffer size is either 4096 bytes, or a larger value calculated in CalcParameterBufferSize.

runtime/src/libraries/System.Data.Odbc/src/System/Data/Odbc/OdbcParameterCollection.cs

Lines 112 to 128 in b593c9b

internal int CalcParameterBufferSize(OdbcCommand command)

{

// Calculate the size of the buffer we need

int parameterBufferSize = 0;

for (int i = 0; i < Count; ++i)

{

if (_rebindCollection)

{

this[i].HasChanged = true;

}

this[i].PrepareForBind(command, (short)(i + 1), ref parameterBufferSize);

parameterBufferSize = (parameterBufferSize + (IntPtr.Size - 1)) & ~(IntPtr.Size - 1); // align buffer;

}

return parameterBufferSize;

}

Perhaps it makes sense to calculate aligned buffer size in the ctor instead.

Ref: dotnet/runtime#85996

ghost · 2023-10-11T17:01:18Z

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

ghost added community-contribution Indicates that the PR has been added by a community member area-System.Data labels May 9, 2023

Use NativeMemory in System.Data.Odbc

a99952d

xtqqczze force-pushed the NativeMemory.Odbc branch from aae5920 to a99952d Compare May 9, 2023 09:43

build-analysis bot mentioned this pull request May 9, 2023

Could not find chrome snapshot folder for Linux_x64 #85949

Closed

Workaround lack of NativeMemory.Clear on .NET 6

68c1005

EgorBo reviewed May 9, 2023

View reviewed changes

runfoapp bot mentioned this pull request May 9, 2023

Test failure: System.Security.Cryptography.X509Certificates.Tests.CertificateCreation.CertificateRequestChainTests/CreateChain_Hybrid #25979

Closed

Use Unsafe.InitBlock

b593c9b

xtqqczze force-pushed the NativeMemory.Odbc branch from bf12034 to b593c9b Compare May 9, 2023 21:42

build-analysis bot mentioned this pull request May 10, 2023

System.Security.Cryptography.X509Certificates.Tests.ChainTests.BuildInvalidSignatureTwice failure #82837

Open

stephentoub reviewed May 15, 2023

View reviewed changes

rolfbjarne added a commit to rolfbjarne/macios that referenced this pull request Jun 2, 2023

[builds] Automatically apply dotnet/runtime#85966.

1efc89e

Ref: dotnet/runtime#85996

rolfbjarne added a commit to rolfbjarne/macios that referenced this pull request Jun 6, 2023

[builds] Automatically apply dotnet/runtime#85966.

19a2c9f

Ref: dotnet/runtime#85996

rolfbjarne added a commit to rolfbjarne/macios that referenced this pull request Jun 6, 2023

[builds] Automatically apply dotnet/runtime#85966.

2843098

Ref: dotnet/runtime#85996

rolfbjarne added a commit to rolfbjarne/macios that referenced this pull request Jun 7, 2023

[builds] Automatically apply dotnet/runtime#85966.

b44c546

Ref: dotnet/runtime#85996

rolfbjarne added a commit to rolfbjarne/macios that referenced this pull request Jun 9, 2023

[builds] Automatically apply dotnet/runtime#85966.

1b3a525

Ref: dotnet/runtime#85996

rolfbjarne added a commit to rolfbjarne/macios that referenced this pull request Jun 13, 2023

[builds] Automatically apply dotnet/runtime#85966.

323d2ea

Ref: dotnet/runtime#85996

rolfbjarne added a commit to rolfbjarne/macios that referenced this pull request Jun 15, 2023

[builds] Automatically apply dotnet/runtime#85966.

76495ad

Ref: dotnet/runtime#85996

Merge branch 'main' into NativeMemory.Odbc

08e2ed0

xtqqczze marked this pull request as draft September 11, 2023 14:52

build-analysis bot mentioned this pull request Sep 11, 2023

CI error: System.Net.Quic.QuicException: The connection timed out from inactivity #91757

Closed

ghost closed this Oct 11, 2023

ghost locked as resolved and limited conversation to collaborators Nov 10, 2023

This pull request was closed.

	internal int CalcParameterBufferSize(OdbcCommand command)
	{
	// Calculate the size of the buffer we need
	int parameterBufferSize = 0;

	for (int i = 0; i < Count; ++i)
	{
	if (_rebindCollection)
	{
	this[i].HasChanged = true;
	}
	this[i].PrepareForBind(command, (short)(i + 1), ref parameterBufferSize);

	parameterBufferSize = (parameterBufferSize + (IntPtr.Size - 1)) & ~(IntPtr.Size - 1); // align buffer;
	}
	return parameterBufferSize;
	}

Conversation

xtqqczze commented May 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented May 9, 2023

Uh oh!

xtqqczze commented May 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Wraith2 commented May 9, 2023

Uh oh!

xtqqczze commented May 9, 2023

Uh oh!

EgorBo May 9, 2023

Choose a reason for hiding this comment

Uh oh!

xtqqczze May 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EgorBo May 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xtqqczze May 9, 2023

Choose a reason for hiding this comment

Uh oh!

xtqqczze May 9, 2023

Choose a reason for hiding this comment

Uh oh!

xtqqczze May 9, 2023

Choose a reason for hiding this comment

Uh oh!

xtqqczze commented May 9, 2023

Uh oh!

stephentoub May 15, 2023

Choose a reason for hiding this comment

Uh oh!

xtqqczze May 25, 2023

Choose a reason for hiding this comment

Uh oh!

xtqqczze May 25, 2023

Choose a reason for hiding this comment

Uh oh!

ghost commented Oct 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

xtqqczze commented May 9, 2023 •

edited

Loading

xtqqczze commented May 9, 2023 •

edited

Loading

xtqqczze May 9, 2023 •

edited

Loading

EgorBo May 9, 2023 •

edited

Loading