Skip to content

Remove early exit logic in GetSortKey-related code paths #1078

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

Merged
jkotas merged 1 commit into from
Dec 27, 2019
Merged

Remove early exit logic in GetSortKey-related code paths #1078

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
21 changes: 13 additions & 8 deletions src/libraries/System.Globalization/tests/CompareInfo/CompareInfoTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ public void Equals(CompareInfo compare1, object value, bool expected)
new object[] { "abc", CompareOptions.Ordinal, "ABC", CompareOptions.Ordinal, false },
new object[] { "abc", CompareOptions.Ordinal, "abc", CompareOptions.Ordinal, true },
new object[] { "abc", CompareOptions.None, "abc", CompareOptions.None, true },
new object[] { "", CompareOptions.None, "\u200c", CompareOptions.None, true }, // see comment at bottom of SortKey_TestData
};

[Theory]
Expand All @@ -65,12 +66,6 @@ public void GetHashCode(string source1, CompareOptions options1, string source2,
Assert.Equal(expected, invariantCompare.GetHashCode(source1, options1).Equals(invariantCompare.GetHashCode(source2, options2)));
}

[Fact]
public void GetHashCode_EmptyString()
{
Assert.Equal(0, CultureInfo.InvariantCulture.CompareInfo.GetHashCode("", CompareOptions.None));
}

[Fact]
public void GetHashCode_Invalid()
{
Expand Down Expand Up @@ -296,6 +291,12 @@ public static IEnumerable<object[]> SortKey_TestData()

// Spanish
yield return new object[] { new CultureInfo("es-ES").CompareInfo, "llegar", "lugar", CompareOptions.None, -1 };

// Zero-weight code points
// In both NLS (Windows) and ICU the code point U+200C ZERO WIDTH NON-JOINER has a zero weight,
// so it's compared as equal to the empty string. This means that we can't special-case GetHashCode("")
// and return a fixed value; we actually need to call the underlying OS or ICU API to calculate the sort key.
yield return new object[] { s_invariantCompare, "", "\u200c", CompareOptions.None, 0 };
}

public static IEnumerable<object[]> IndexOf_TestData()
Expand Down Expand Up @@ -439,9 +440,13 @@ public void GetHashCode_Span(string source1, CompareOptions options1, string sou
}

[Fact]
public void GetHashCode_EmptySpan()
public void GetHashCode_NullAndEmptySpan()
{
Assert.Equal(0, CultureInfo.InvariantCulture.CompareInfo.GetHashCode(ReadOnlySpan<char>.Empty, CompareOptions.None));
// Ensure that null spans and non-null empty spans produce the same hash code.

int hashCodeOfNullSpan = CultureInfo.InvariantCulture.CompareInfo.GetHashCode(ReadOnlySpan<char>.Empty, CompareOptions.None);
int hashCodeOfNotNullEmptySpan = CultureInfo.InvariantCulture.CompareInfo.GetHashCode("".AsSpan(), CompareOptions.None);
Assert.Equal(hashCodeOfNullSpan, hashCodeOfNotNullEmptySpan);
}

[Fact]
Expand Down
12 changes: 12 additions & 0 deletions src/libraries/System.Globalization/tests/Invariant/InvariantMode.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -627,6 +627,18 @@ public void TestSortVersion(string cultureName)
Assert.Equal(version, new CultureInfo(cultureName).CompareInfo.Version);
}

[Fact]
public void TestSortKey_ZeroWeightCodePoints()
{
// In the invariant globalization mode, there's no such thing as a zero-weight code point,
// so the U+200C ZERO WIDTH NON-JOINER code point contributes to the final sort key value.

CompareInfo compareInfo = CultureInfo.InvariantCulture.CompareInfo;
SortKey sortKeyForEmptyString = compareInfo.GetSortKey("");
SortKey sortKeyForZeroWidthJoiner = compareInfo.GetSortKey("\u200c");
Assert.NotEqual(0, SortKey.Compare(sortKeyForEmptyString, sortKeyForZeroWidthJoiner));
}

@VladimirReshetnikov VladimirReshetnikov Dec 20, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

What is the expected result here?

        Console.WriteLine(SortKey.Compare(
            CultureInfo.InvariantCulture.CompareInfo.GetSortKey("\uFFFD"),
            CultureInfo.InvariantCulture.CompareInfo.GetSortKey("\u200C")));

@GrabYourPitchforks GrabYourPitchforks Dec 20, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I believe that on NLS (Windows) they're equal and that on ICU (non-Windows) they're unequal, but I'm not current on a box where I can easily check.

@GrabYourPitchforks GrabYourPitchforks Dec 21, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I checked this today - my statement above is correct. Under the default / invariant culture, on ICU "\ufffd" and "\u200c" are not equal, but using Windows's CompareStringEx they are equal.



private static StringComparison GetStringComparison(CompareOptions options)
{
Expand Down
30 changes: 9 additions & 21 deletions src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Unix.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -821,24 +821,17 @@ private unsafe SortKey CreateSortKey(string source, CompareOptions options)
throw new ArgumentException(SR.Argument_InvalidFlag, nameof(options));
}

byte [] keyData;
if (source.Length == 0)
byte[] keyData;
fixed (char* pSource = source)
{
keyData = Array.Empty<byte>();
}
else
{
fixed (char* pSource = source)
{
int sortKeyLength = Interop.Globalization.GetSortKey(_sortHandle, pSource, source.Length, null, 0, options);
keyData = new byte[sortKeyLength];
int sortKeyLength = Interop.Globalization.GetSortKey(_sortHandle, pSource, source.Length, null, 0, options);
keyData = new byte[sortKeyLength];

fixed (byte* pSortKey = keyData)
fixed (byte* pSortKey = keyData)
{
if (Interop.Globalization.GetSortKey(_sortHandle, pSource, source.Length, pSortKey, sortKeyLength, options) != sortKeyLength)
{
if (Interop.Globalization.GetSortKey(_sortHandle, pSource, source.Length, pSortKey, sortKeyLength, options) != sortKeyLength)
{
throw new ArgumentException(SR.Arg_ExternalException);
}
throw new ArgumentException(SR.Arg_ExternalException);
}
}
}
Expand Down Expand Up @@ -894,11 +887,6 @@ internal unsafe int GetHashCodeOfStringCore(ReadOnlySpan<char> source, CompareOp
Debug.Assert(!GlobalizationMode.Invariant);
Debug.Assert((options & (CompareOptions.Ordinal | CompareOptions.OrdinalIgnoreCase)) == 0);

if (source.Length == 0)
{
return 0;
}

// according to ICU User Guide the performance of ucol_getSortKey is worse when it is called with null output buffer
// the solution is to try to fill the sort key in a temporary buffer of size equal 4 x string length
// 1MB is the biggest array that can be rented from ArrayPool.Shared without memory allocation
Expand All @@ -909,7 +897,7 @@ internal unsafe int GetHashCodeOfStringCore(ReadOnlySpan<char> source, CompareOp
? stackalloc byte[1024]
: (borrowedArray = ArrayPool<byte>.Shared.Rent(sortKeyLength));

fixed (char* pSource = &MemoryMarshal.GetReference(source))
fixed (char* pSource = &MemoryMarshal.GetNonNullPinnableReference(source))
{
fixed (byte* pSortKey = &MemoryMarshal.GetReference(sortKey))
{
Expand Down
68 changes: 38 additions & 30 deletions src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Windows.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,18 +129,24 @@ private unsafe int GetHashCodeOfStringCore(ReadOnlySpan<char> source, CompareOpt
Debug.Assert(!GlobalizationMode.Invariant);
Debug.Assert((options & (CompareOptions.Ordinal | CompareOptions.OrdinalIgnoreCase)) == 0);

if (source.Length == 0)

@tarekgh tarekgh Dec 22, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you please check if Windows/ICU returns the same hash value for the empty string with all locales or cultures? I am asking because if this is the case, we can cache this value and keep the fast path for the empty strings.

@tarekgh tarekgh Dec 22, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

By the way, I am not actively following up so either don't block on me or wait till I am back. Thanks!

@danmoseley danmoseley Dec 23, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Or at least maybe we could cache it for invariant culture (if not already)?

@GrabYourPitchforks GrabYourPitchforks Dec 23, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I thought about this but currently I don't have any evidence that anybody is trying to compute the culture-aware hash code of an empty string in a hot path.

And yeah, no rush whatsoever on this. It can wait until the new year. :)

// LCMapStringEx doesn't support passing cchSrc = 0, so if given a null or empty input
// we'll normalize it to an empty null-terminated string and pass -1 to indicate that
// the underlying OS function should read until it encounters the null terminator.

int sourceLength = source.Length;
if (sourceLength == 0)
{
return 0;
source = string.Empty;
sourceLength = -1;
}

uint flags = LCMAP_SORTKEY | (uint)GetNativeCompareFlags(options);

fixed (char* pSource = source)
fixed (char* pSource = &MemoryMarshal.GetReference(source))
{
int sortKeyLength = Interop.Kernel32.LCMapStringEx(_sortHandle != IntPtr.Zero ? null : _sortName,
flags,
pSource, source.Length /* in chars */,
pSource, sourceLength /* in chars */,
null, 0,
null, null, _sortHandle);
if (sortKeyLength == 0)
Expand All @@ -162,7 +168,7 @@ private unsafe int GetHashCodeOfStringCore(ReadOnlySpan<char> source, CompareOpt
{
if (Interop.Kernel32.LCMapStringEx(_sortHandle != IntPtr.Zero ? null : _sortName,
flags,
pSource, source.Length /* in chars */,
pSource, sourceLength /* in chars */,
pSortKey, sortKeyLength,
null, null, _sortHandle) != sortKeyLength)
{
Expand Down Expand Up @@ -505,38 +511,40 @@ private unsafe SortKey CreateSortKey(string source, CompareOptions options)
}

byte[] keyData;
if (source.Length == 0)
uint flags = LCMAP_SORTKEY | (uint)GetNativeCompareFlags(options);

// LCMapStringEx doesn't support passing cchSrc = 0, so if given an empty string
// we'll instead pass -1 to indicate a null-terminated empty string.

int sourceLength = source.Length;
if (sourceLength == 0)
{
keyData = Array.Empty<byte>();
sourceLength = -1;
}
else
{
uint flags = LCMAP_SORTKEY | (uint)GetNativeCompareFlags(options);

fixed (char* pSource = source)
fixed (char* pSource = source)
{
int sortKeyLength = Interop.Kernel32.LCMapStringEx(_sortHandle != IntPtr.Zero ? null : _sortName,
flags,
pSource, sourceLength,
null, 0,
null, null, _sortHandle);
if (sortKeyLength == 0)
{
int sortKeyLength = Interop.Kernel32.LCMapStringEx(_sortHandle != IntPtr.Zero ? null : _sortName,
flags,
pSource, source.Length,
null, 0,
null, null, _sortHandle);
if (sortKeyLength == 0)
{
throw new ArgumentException(SR.Arg_ExternalException);
}
throw new ArgumentException(SR.Arg_ExternalException);
}

keyData = new byte[sortKeyLength];
keyData = new byte[sortKeyLength];

fixed (byte* pBytes = keyData)
fixed (byte* pBytes = keyData)
{
if (Interop.Kernel32.LCMapStringEx(_sortHandle != IntPtr.Zero ? null : _sortName,
flags,
pSource, sourceLength,
pBytes, keyData.Length,
null, null, _sortHandle) != sortKeyLength)
{
if (Interop.Kernel32.LCMapStringEx(_sortHandle != IntPtr.Zero ? null : _sortName,
flags,
pSource, source.Length,
pBytes, keyData.Length,
null, null, _sortHandle) != sortKeyLength)
{
throw new ArgumentException(SR.Arg_ExternalException);
}
throw new ArgumentException(SR.Arg_ExternalException);
}
}
}
Expand Down