Skip to content

Commit

Permalink
avoid expensive ref counting for CompareInfo on Linux (dotnet/coreclr…
Browse files Browse the repository at this point in the history 
…#25117)

* change SafeSortHandle to be CriticalSortHandle to avoid expensive ref counting

* seal the CriticalSortHandle class

* use IntPtr instead of CriticalHandle to avoid resurrection issues. It's ok to never free the handle

* introduce cache for sort handles to prevent from memory leak in certain scenarios

* move the  sort key logic to a dedicated helper type to avoid static field initialization order issues

* lock the dictionary

* simplify the code

* set the charset to Ansi

* Apply suggestions from code review

Co-Authored-By: Jan Kotas <[email protected]>

* don't use var


Commit migrated from dotnet/coreclr@9b0dcaa
  • Loading branch information
@adamsitnik
adamsitnik authored Jun 16, 2019
2 parents c2f9f9a + 731abe1 commit 3fa0f7d
Show file tree
Hide file tree
Showing 3 changed files with 55 additions and 63 deletions.
44 changes: 12 additions & 32 deletions .../System.Private.CoreLib/src/Interop/Unix/System.Globalization.Native/Interop.Collation.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,20 +11,20 @@ internal static partial class Interop
{
internal static partial class Globalization
{
[DllImport(Libraries.GlobalizationNative, CharSet = CharSet.Unicode, EntryPoint = "GlobalizationNative_GetSortHandle")]
internal static extern unsafe ResultCode GetSortHandle(byte[] localeName, out SafeSortHandle sortHandle);
[DllImport(Libraries.GlobalizationNative, CharSet = CharSet.Ansi, EntryPoint = "GlobalizationNative_GetSortHandle")]
internal static extern unsafe ResultCode GetSortHandle(string localeName, out IntPtr sortHandle);

[DllImport(Libraries.GlobalizationNative, CharSet = CharSet.Unicode, EntryPoint = "GlobalizationNative_CloseSortHandle")]
[DllImport(Libraries.GlobalizationNative, EntryPoint = "GlobalizationNative_CloseSortHandle")]
internal static extern unsafe void CloseSortHandle(IntPtr handle);

[DllImport(Libraries.GlobalizationNative, CharSet = CharSet.Unicode, EntryPoint = "GlobalizationNative_CompareString")]
internal static extern unsafe int CompareString(SafeSortHandle sortHandle, char* lpStr1, int cwStr1Len, char* lpStr2, int cwStr2Len, CompareOptions options);
internal static extern unsafe int CompareString(IntPtr sortHandle, char* lpStr1, int cwStr1Len, char* lpStr2, int cwStr2Len, CompareOptions options);

[DllImport(Libraries.GlobalizationNative, CharSet = CharSet.Unicode, EntryPoint = "GlobalizationNative_IndexOf")]
internal static extern unsafe int IndexOf(SafeSortHandle sortHandle, char* target, int cwTargetLength, char* pSource, int cwSourceLength, CompareOptions options, int* matchLengthPtr);
internal static extern unsafe int IndexOf(IntPtr sortHandle, char* target, int cwTargetLength, char* pSource, int cwSourceLength, CompareOptions options, int* matchLengthPtr);

[DllImport(Libraries.GlobalizationNative, CharSet = CharSet.Unicode, EntryPoint = "GlobalizationNative_LastIndexOf")]
internal static extern unsafe int LastIndexOf(SafeSortHandle sortHandle, char* target, int cwTargetLength, char* pSource, int cwSourceLength, CompareOptions options);
internal static extern unsafe int LastIndexOf(IntPtr sortHandle, char* target, int cwTargetLength, char* pSource, int cwSourceLength, CompareOptions options);

[DllImport(Libraries.GlobalizationNative, CharSet = CharSet.Unicode, EntryPoint = "GlobalizationNative_IndexOfOrdinalIgnoreCase")]
internal static extern unsafe int IndexOfOrdinalIgnoreCase(string target, int cwTargetLength, char* pSource, int cwSourceLength, bool findLast);
Expand All @@ -33,47 +33,27 @@ internal static partial class Globalization

[DllImport(Libraries.GlobalizationNative, CharSet = CharSet.Unicode, EntryPoint = "GlobalizationNative_StartsWith")]
[return: MarshalAs(UnmanagedType.Bool)]
internal static extern unsafe bool StartsWith(SafeSortHandle sortHandle, char* target, int cwTargetLength, char* source, int cwSourceLength, CompareOptions options);
internal static extern unsafe bool StartsWith(IntPtr sortHandle, char* target, int cwTargetLength, char* source, int cwSourceLength, CompareOptions options);

[DllImport(Libraries.GlobalizationNative, CharSet = CharSet.Unicode, EntryPoint = "GlobalizationNative_EndsWith")]
[return: MarshalAs(UnmanagedType.Bool)]
internal static extern unsafe bool EndsWith(SafeSortHandle sortHandle, char* target, int cwTargetLength, char* source, int cwSourceLength, CompareOptions options);
internal static extern unsafe bool EndsWith(IntPtr sortHandle, char* target, int cwTargetLength, char* source, int cwSourceLength, CompareOptions options);

[DllImport(Libraries.GlobalizationNative, CharSet = CharSet.Unicode, EntryPoint = "GlobalizationNative_StartsWith")]
[return: MarshalAs(UnmanagedType.Bool)]
internal static extern unsafe bool StartsWith(SafeSortHandle sortHandle, string target, int cwTargetLength, string source, int cwSourceLength, CompareOptions options);
internal static extern unsafe bool StartsWith(IntPtr sortHandle, string target, int cwTargetLength, string source, int cwSourceLength, CompareOptions options);

[DllImport(Libraries.GlobalizationNative, CharSet = CharSet.Unicode, EntryPoint = "GlobalizationNative_EndsWith")]
[return: MarshalAs(UnmanagedType.Bool)]
internal static extern unsafe bool EndsWith(SafeSortHandle sortHandle, string target, int cwTargetLength, string source, int cwSourceLength, CompareOptions options);
internal static extern unsafe bool EndsWith(IntPtr sortHandle, string target, int cwTargetLength, string source, int cwSourceLength, CompareOptions options);

[DllImport(Libraries.GlobalizationNative, CharSet = CharSet.Unicode, EntryPoint = "GlobalizationNative_GetSortKey")]
internal static extern unsafe int GetSortKey(SafeSortHandle sortHandle, char* str, int strLength, byte* sortKey, int sortKeyLength, CompareOptions options);
internal static extern unsafe int GetSortKey(IntPtr sortHandle, char* str, int strLength, byte* sortKey, int sortKeyLength, CompareOptions options);

[DllImport(Libraries.GlobalizationNative, CharSet = CharSet.Unicode, EntryPoint = "GlobalizationNative_CompareStringOrdinalIgnoreCase")]
internal static extern unsafe int CompareStringOrdinalIgnoreCase(char* lpStr1, int cwStr1Len, char* lpStr2, int cwStr2Len);

[DllImport(Libraries.GlobalizationNative, EntryPoint = "GlobalizationNative_GetSortVersion")]
internal static extern int GetSortVersion(SafeSortHandle sortHandle);

internal class SafeSortHandle : SafeHandle
{
private SafeSortHandle() :
base(IntPtr.Zero, true)
{
}

public override bool IsInvalid
{
get { return handle == IntPtr.Zero; }
}

protected override bool ReleaseHandle()
{
CloseSortHandle(handle);
SetHandle(IntPtr.Zero);
return true;
}
}
internal static extern int GetSortVersion(IntPtr sortHandle);
}
}
66 changes: 41 additions & 25 deletions src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Unix.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,12 @@
// See the LICENSE file in the project root for more information.

using System.Buffers;
using System.Collections.Generic;
using System.Diagnostics;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;
using System.Security;
using System.Threading;

using Internal.Runtime.CompilerServices;

Expand All @@ -15,7 +17,7 @@ namespace System.Globalization
public partial class CompareInfo
{
[NonSerialized]
private Interop.Globalization.SafeSortHandle _sortHandle = null!; // initialized in helper called by ctors
private IntPtr _sortHandle;

[NonSerialized]
private bool _isAsciiEqualityOrdinal;
Expand All @@ -30,17 +32,9 @@ private void InitSort(CultureInfo culture)
}
else
{
Interop.Globalization.ResultCode resultCode = Interop.Globalization.GetSortHandle(GetNullTerminatedUtf8String(_sortName), out _sortHandle);
if (resultCode != Interop.Globalization.ResultCode.Success)
{
_sortHandle.Dispose();

if (resultCode == Interop.Globalization.ResultCode.OutOfMemory)
throw new OutOfMemoryException();

throw new ExternalException(SR.Arg_ExternalException);
}
_isAsciiEqualityOrdinal = (_sortName == "en-US" || _sortName == "");

_sortHandle = SortHandleCache.GetCachedSortHandle(_sortName);
}
}

Expand Down Expand Up @@ -918,20 +912,6 @@ private static bool CanUseAsciiOrdinalForOptions(CompareOptions options)
return (options & CompareOptions.IgnoreSymbols) == 0;
}

private static byte[] GetNullTerminatedUtf8String(string s)
{
int byteLen = System.Text.Encoding.UTF8.GetByteCount(s);

// Allocate an extra byte (which defaults to 0) as the null terminator.
byte[] buffer = new byte[byteLen + 1];

int bytesWritten = System.Text.Encoding.UTF8.GetBytes(s, 0, s.Length, buffer, 0);

Debug.Assert(bytesWritten == byteLen);

return buffer;
}

private SortVersion GetSortVersion()
{
Debug.Assert(!GlobalizationMode.Invariant);
Expand All @@ -944,6 +924,42 @@ private SortVersion GetSortVersion()
(byte) (LCID & 0xFF)));
}

private static class SortHandleCache
{
// in most scenarios there is a limited number of cultures with limited number of sort options
// so caching the sort handles and not freeing them is OK, see https://github.com/dotnet/coreclr/pull/25117 for more
private static readonly Dictionary<string, IntPtr> s_sortNameToSortHandleCache = new Dictionary<string, IntPtr>();

internal static IntPtr GetCachedSortHandle(string sortName)
{
lock (s_sortNameToSortHandleCache)
{
if (!s_sortNameToSortHandleCache.TryGetValue(sortName, out IntPtr result))
{
Interop.Globalization.ResultCode resultCode = Interop.Globalization.GetSortHandle(sortName, out result);

if (resultCode == Interop.Globalization.ResultCode.OutOfMemory)
throw new OutOfMemoryException();
else if (resultCode != Interop.Globalization.ResultCode.Success)
throw new ExternalException(SR.Arg_ExternalException);

try
{
s_sortNameToSortHandleCache.Add(sortName, result);
}
catch
{
Interop.Globalization.CloseSortHandle(result);

throw;
}
}

return result;
}
}
}

// See https://github.com/dotnet/coreclr/blob/master/src/utilcode/util_nodependencies.cpp#L970
private static readonly bool[] s_highCharTable = new bool[0x80]
{
Expand Down
8 changes: 2 additions & 6 deletions src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/CriticalHandle.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,11 +80,7 @@ the general type of handle they support (null is invalid, -1 is invalid etc.)
back into memory, etc), then they can provide a finalizer that will be
guaranteed to run before the CriticalHandle's critical finalizer.
Subclasses are expected to be written as follows (note that
SuppressUnmanagedCodeSecurity should always be used on any P/Invoke methods
invoked as part of ReleaseHandle, in order to switch the security check from
runtime to jit time and thus remove a possible failure path from the
invocation of the method):
Subclasses are expected to be written as follows:
internal sealed MyCriticalHandleSubclass : CriticalHandle {
// Called by P/Invoke when returning CriticalHandles
Expand All @@ -99,7 +95,7 @@ public override bool IsInvalid {
get { return handle == IntPtr.Zero; }
}
[DllImport(Interop.Libraries.Kernel32), SuppressUnmanagedCodeSecurity, ReliabilityContract(Consistency.WillNotCorruptState, Cer.Success)]
[DllImport(Interop.Libraries.Kernel32), ReliabilityContract(Consistency.WillNotCorruptState, Cer.Success)]
private static extern bool CloseHandle(IntPtr handle);
override protected bool ReleaseHandle()
Expand Down

0 comments on commit 3fa0f7d

Please sign in to comment.