Handle non-ASCII strings in GetNonRandomizedHashCodeOrdinalIgnoreCase

[EgorBo]

Fixes #44681

[Dotnet-GitSync-Bot]

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

[EgorBo]

@GrabYourPitchforks I'm not sure how to fix https://github.com/dotnet/runtime/blob/master/src/libraries/System.Collections/tests/Generic/Dictionary/HashCollisionScenarios/OutOfBoundsRegression.cs#L194-L196 test now - it relies on fact that we can easily generate collisions for a specific hash but actually 99.9.. % of such generated strings are non-ASCII so they go the slow path and don't produce collisions.

So I need to somehow generate 100 ascii string with the same hashcode 🤔

[GrabYourPitchforks]

@EgorBo I think the only reason they're not producing the same hash code is that we're calling the Marvin routine. If we update the fallback logic to perform an uppercase conversion but still use the naïve bit-shifting routines already present in GetNonRandomizedHashCodeOrdinalIgnoreCase, the unit tests should pass.

[EgorBo]

@EgorBo I think the only reason they're not producing the same hash code is that we're calling the Marvin routine. If we update the fallback logic to perform an uppercase conversion but still use the naïve bit-shifting routines already present in GetNonRandomizedHashCodeOrdinalIgnoreCase, the unit tests should pass.

ah ok, let me rewrite it then, thanks!

[EgorBo]

@GrabYourPitchforks updated, could you please take a look if it's what you meant

[GrabYourPitchforks]

LGTM! I left some perf nit comments. These don't need to be addressed; they're mainly to point out areas of low hanging fruit just in case we did end up harming performance and we're looking for some easy ways to knock out a few percentage here and there.

[GrabYourPitchforks]

I've reproed the unit test issue, one sec and I'll get a workaround out to you.

[GrabYourPitchforks]

@EgorBo The commit GrabYourPitchforks@113d1e6 in my private branch includes three changes that will be of interest here:

• The file RandomizedStringEqualityComparer.cs includes a fix for Dictionary sometimes uses Ordinal hash code calculation instead of OrdinalIgnoreCase #44695.
• The file OutOfBoundsRegression.cs contains a fix for the Assert.Equal(0x24716ca0, ...) that's failing in this PR.
• The file OutOfBoundsRegression.cs contains additional regression tests for both StringComparer.OrdinalIgnoreCase does not ignore case for several Cyrillic letters when used with Dictionary  #44681 and Dictionary sometimes uses Ordinal hash code calculation instead of OrdinalIgnoreCase #44695.

Might be worth merging the patch into this PR so that both issues can be closed at once?

(I was going to submit my patch as its own PR to address #44695, but since that patch relies on this PR being committed, if I were to submit it prematurely all of the unit tests would fail.)

[danmoseley]

Is there a more limited version of the change that might be lower risk to backport?

[GrabYourPitchforks]

The least code churn version of this change for servicing purposes would be:

• Remove all of the code from GetNonRandomizedHashCodeOrdinalIgnoreCase and have it forward to the method Marvin.ComputeHash32OrdinalIgnoreCase; and
• Take the RandomizedStringEqualityComparer.cs fix as-is from this PR; and
• Suppress the tests in OutOfBoundsRegression.cs, as they'll start failing.

Together, these will essentially disable the perf optimization that was done in #36252, and Dictionary<...>(OrdinalIgnoreCase) performance will revert to what it was in netcoreapp3.1.

[danmoseley]

Dictionary<...>(OrdinalIgnoreCase) performance will revert to what it was in netcoreapp3.1.

Hmm, that would be a significant takeback.

[GrabYourPitchforks]

/azp run runtime

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[jkotas]

I do not think it is correct to pass hash1 and hash2 into the slow method. We could have processed some number of characters already and so hash1 and hash2 may not have their original values.

It would be nice to add a test case that covers this case.

[EgorBo]

oops, indeed
I am not sure it can be tested anyhow other than hardcoding the expected hashcode value 🤔

[jkotas]

The new test is failing on Mono. Could you please investigate?

[GrabYourPitchforks]

I used runfo to pull the logs from the helix machine, but there don't seem to be any failures recorded? The zip file I downloaded contains the script used to run the tests, but I don't see anything that resembles output from the test run. Will dig in further tomorrow.

[jkotas]

I see this failures when I go to Details / View more details on Azure Pipelines / "go back" / Tests / select iOS failure:

https://dev.azure.com/dnceng/public/_build/results?buildId=890570&view=ms.vss-test-web.build-test-results-tab&runId=28497198&paneView=debug&resultId=138723

System.Collections.Generic.KeyNotFoundException : The given key 'А' was not present in the dictionary.


Stack trace
   at System.Collections.Generic.Dictionary`2[[System.String, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e],[System.Int32, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].get_Item(String key)
   at System.Collections.Tests.Dictionary_Tests.DictionaryOrdinalIgnoreCaseCyrillicKeys()
   at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)

[EgorBo]

The new test is failing on Mono. Could you please investigate?

Ah, it's because iOS is always invariant atm (I'm integrating ICU there).

"Ф".Equals("ф", StringComparison.OrdinalIgnoreCase); // "false" in the Invariant mode

Will ignore that test.

[jkotas]

Passed CI on Linux before. CI failures are #45061

[jkotas]

/backport to release/5.0

[github-actions]

Started backporting to release/5.0: https://github.com/dotnet/runtime/actions/runs/376611056