Skip to content

Use NativeMemory in System.Data.Odbc #85966

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.

Sign up for GitHub

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

Closed
xtqqczze wants to merge 4 commits into from
Closed

Use NativeMemory in System.Data.Odbc #85966

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a99952d
Use `NativeMemory` in `System.Data.Odbc`
xtqqczze May 9, 2023
68c1005
Workaround lack of `NativeMemory.Clear` on .NET 6
xtqqczze May 9, 2023
b593c9b
Use `Unsafe.InitBlock`
xtqqczze May 9, 2023
08e2ed0
Merge branch 'main' into NativeMemory.Odbc
xtqqczze Sep 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Workaround lack of NativeMemory.Clear on .NET 6
  • Loading branch information
@xtqqczze
xtqqczze committed May 9, 2023
commit 68c1005a22a6cce415bad3834e470286ddf324bc
21 changes: 21 additions & 0 deletions src/libraries/System.Data.Odbc/src/Common/System/Data/Common/SafeNativeMethods.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;

namespace System.Data
{
internal static partial class SafeNativeMethods
{
[MethodImpl(MethodImplOptions.AggressiveInlining)]
internal static unsafe void ZeroMemory(IntPtr ptr, int length)
{
#if !NET7_0_OR_GREATER
new Span<byte>((void*)ptr, length).Clear();
Copy link
Member

@EgorBo EgorBo May 9, 2023

Choose a reason for hiding this comment

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

why not just Span.Clear for all TFMs ?

Copy link
Contributor Author

@xtqqczze xtqqczze May 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

@EgorBo EgorBo May 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@xtqqczze xtqqczze May 9, 2023

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@xtqqczze xtqqczze May 9, 2023

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@xtqqczze xtqqczze May 9, 2023

Choose a reason for hiding this comment

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

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

#else
NativeMemory.Clear((void*)ptr, (uint)length);
#endif
}
}
}
4 changes: 2 additions & 2 deletions src/libraries/System.Data.Odbc/src/Common/System/Data/ProviderBase/DbBuffer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -627,7 +627,7 @@ internal unsafe void WriteSingle(int offset, float value)
WriteInt32(offset, BitConverter.SingleToInt32Bits(value));
}

internal unsafe void ZeroMemory()
internal void ZeroMemory()
{
bool mustRelease = false;

Expand All @@ -636,7 +636,7 @@ internal unsafe void ZeroMemory()
DangerousAddRef(ref mustRelease);

IntPtr ptr = DangerousGetHandle();
NativeMemory.Clear((void*)ptr, (uint)Length);
SafeNativeMethods.ZeroMemory(ptr, Length);
}
finally
{
Expand Down
1 change: 1 addition & 0 deletions src/libraries/System.Data.Odbc/src/System.Data.Odbc.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ System.Data.Odbc.OdbcTransaction</PackageDescription>
<Compile Include="$(CommonPath)System\Data\Common\NameValuePair.cs"
Link="Common\System\Data\Common\NameValuePair.cs" />
<Compile Include="Common\System\Data\Common\NameValuePermission.cs" />
<Compile Include="Common\System\Data\Common\SafeNativeMethods.cs" />
<Compile Include="Common\System\Data\DataStorage.cs" />
<Compile Include="Common\System\Data\ProviderBase\DbBuffer.cs" />
<Compile Include="$(CommonPath)System\Data\Common\FieldNameLookup.cs"
Expand Down