Disable optimization which sometimes results in incorrect case sensitivity in FrozenCollections

[andrewjsaid]

See #93986 for full context

Fix #93974

[ghost]

Tagging subscribers to this area: @dotnet/area-system-collections
See info in area-owners.md if you want to be subscribed.

Issue Details

---

See #93986 for full context

Author:	andrewjsaid
Assignees:	-
Labels:	area-System.Collections, community-contribution
Milestone:	-

[eiriktsarpalis]

To get a feel of potential perf regressions, would it be possible to share benchmarks comparing this to main in the impacted cases?

[andrewjsaid]

@eiriktsarpalis That would require a PR in the performance repo. Is it possible for somebody from the MS team to do that as I am pretty busy. If not I'll try find some time.

[stephentoub]

Thanks. Do we have an understanding of what uses this might regress and by how much? We should make the change, regardless, but I'd like to understand the ramifications and what we should be looking at following-up on.

[andrewjsaid]

Do we have an understanding of what uses this might regress

Removing the optimization will be a regression (haven't got a measurement*) only in the case where we are case insensitive, have identified a partial substring to hash, and that substring contains no letters. In all other cases it matches .NET 8 release.

and by how much?

Perf benchmarks in #93986 (comment) show that for case sensitive there is no effect. However that doesn't answer your question as this particular optimization being disabled was never benchmarked by the performance repo. If I find time to add a benchmark I'll add it here but I don't foresee having the time before the weekend.

[stephentoub]

Do we have an understanding of what uses this might regress

Removing the optimization will be a regression (haven't got a measurement*) only in the case where we are case insensitive, have identified a partial substring to hash, and that substring contains no letters. In all other cases it matches .NET 8 release.

and by how much?

Perf benchmarks in #93986 (comment) show that for case sensitive there is no effect. However that doesn't answer your question as this particular optimization being disabled was never benchmarked by the performance repo. If I find time to add a benchmark I'll add it here but I don't foresee having the time before the weekend.

Ok, thanks. As noted, we don't need to block on it. @eiriktsarpalis, if you have some time to get some quick numbers just to aid in our understanding of the impact and whether there's an important follow-up here, that'd be helpful.

[eiriktsarpalis]

I tweaked the Perf_SubstringFrozenDictionary benchmark to use case insensitive comparison and here's what I got:

Method	Job	Toolchain	Count	Mean	Error	StdDev	Median	Min	Max	Ratio	MannWhitney(3%)	RatioSD	Gen0	Gen1	Gen2	Allocated	Alloc Ratio
ToFrozenDictionary	Job-LOTKLJ	main	10	1,389.48 ns	31.906 ns	36.743 ns	1,382.71 ns	1,327.53 ns	1,473.59 ns	1.00	Base	0.00	0.1660	-	-	1720 B	1.00
ToFrozenDictionary	Job-QETJTA	PR	10	1,393.58 ns	27.624 ns	31.812 ns	1,399.03 ns	1,323.29 ns	1,453.86 ns	1.00	Same	0.04	0.1708	-	-	1720 B	1.00
TryGetValue_True_FrozenDictionary	Job-LOTKLJ	main	10	54.00 ns	1.288 ns	1.322 ns	54.13 ns	51.35 ns	55.60 ns	1.00	Base	0.00	-	-	-	-	NA
TryGetValue_True_FrozenDictionary	Job-QETJTA	PR	10	73.24 ns	1.078 ns	1.008 ns	73.38 ns	70.06 ns	74.24 ns	1.35	Slower	0.04	-	-	-	-	NA
ToFrozenDictionary	Job-LOTKLJ	main	100	8,446.66 ns	158.800 ns	163.076 ns	8,471.45 ns	8,134.23 ns	8,717.36 ns	1.00	Base	0.00	1.1780	-	-	12112 B	1.00
ToFrozenDictionary	Job-QETJTA	PR	100	8,771.04 ns	237.326 ns	273.305 ns	8,836.49 ns	8,160.74 ns	9,217.86 ns	1.04	Same	0.03	1.2019	-	-	12112 B	1.00
TryGetValue_True_FrozenDictionary	Job-LOTKLJ	main	100	563.85 ns	10.168 ns	9.014 ns	567.79 ns	547.12 ns	576.15 ns	1.00	Base	0.00	-	-	-	-	NA
TryGetValue_True_FrozenDictionary	Job-QETJTA	PR	100	1,006.73 ns	15.021 ns	14.050 ns	1,006.87 ns	985.24 ns	1,036.20 ns	1.79	Slower	0.04	-	-	-	-	NA
ToFrozenDictionary	Job-LOTKLJ	main	1000	74,531.61 ns	1,469.417 ns	1,508.983 ns	74,819.41 ns	71,279.16 ns	77,269.59 ns	1.00	Base	0.00	9.2703	1.1962	-	94864 B	1.00
ToFrozenDictionary	Job-QETJTA	PR	1000	72,473.19 ns	1,866.260 ns	2,149.188 ns	73,203.68 ns	68,539.46 ns	75,121.58 ns	0.98	Same	0.03	9.2262	1.1905	-	94864 B	1.00
TryGetValue_True_FrozenDictionary	Job-LOTKLJ	main	1000	48,575.95 ns	935.546 ns	918.831 ns	48,985.45 ns	46,654.11 ns	50,021.83 ns	1.00	Base	0.00	-	-	-	-	NA
TryGetValue_True_FrozenDictionary	Job-QETJTA	PR	1000	50,132.24 ns	976.166 ns	958.725 ns	50,439.63 ns	47,947.67 ns	51,144.53 ns	1.03	Same	0.03	-	-	-	-	NA
ToFrozenDictionary	Job-LOTKLJ	main	10000	1,367,777.25 ns	31,636.638 ns	32,488.498 ns	1,378,004.69 ns	1,300,021.88 ns	1,428,109.38 ns	1.00	Base	0.00	148.4375	148.4375	148.4375	926138 B	1.00
ToFrozenDictionary	Job-QETJTA	PR	10000	1,361,651.79 ns	26,136.206 ns	25,669.249 ns	1,371,019.27 ns	1,298,396.35 ns	1,389,535.42 ns	1.00	Same	0.03	145.8333	145.8333	145.8333	926134 B	1.00
TryGetValue_True_FrozenDictionary	Job-LOTKLJ	main	10000	429,049.49 ns	7,951.179 ns	7,048.514 ns	430,354.73 ns	412,395.95 ns	438,879.39 ns	1.00	Base	0.00	-	-	-	1 B	1.00
TryGetValue_True_FrozenDictionary	Job-QETJTA	PR	10000	433,232.35 ns	7,438.534 ns	6,958.009 ns	435,446.22 ns	419,605.92 ns	442,993.75 ns	1.01	Same	0.03	-	-	-	11 B	11.00
ToDictionary	Job-LOTKLJ	main	10	140.89 ns	2.840 ns	2.656 ns	142.01 ns	136.03 ns	144.31 ns	1.00	Base	0.00	0.0433	-	-	440 B	1.00
ToDictionary	Job-QETJTA	PR	10	141.67 ns	2.772 ns	3.081 ns	142.03 ns	135.25 ns	146.24 ns	1.01	Same	0.04	0.0435	-	-	440 B	1.00
ToImmutableDictionary	Job-LOTKLJ	main	10	915.37 ns	21.383 ns	24.625 ns	916.86 ns	868.44 ns	952.28 ns	1.00	Base	0.00	0.0712	-	-	736 B	1.00
ToImmutableDictionary	Job-QETJTA	PR	10	1,064.95 ns	20.525 ns	21.961 ns	1,068.78 ns	1,016.33 ns	1,093.58 ns	1.16	Slower	0.03	0.0703	-	-	736 B	1.00
TryGetValue_True_Dictionary	Job-LOTKLJ	main	10	108.24 ns	2.142 ns	2.103 ns	109.02 ns	104.05 ns	110.36 ns	1.00	Base	0.00	-	-	-	-	NA
TryGetValue_True_Dictionary	Job-QETJTA	PR	10	108.94 ns	2.030 ns	1.899 ns	109.56 ns	104.94 ns	111.03 ns	1.01	Same	0.03	-	-	-	-	NA
TryGetValue_True_ImmutableDictionary	Job-LOTKLJ	main	10	173.50 ns	3.458 ns	3.552 ns	173.71 ns	166.58 ns	179.82 ns	1.00	Base	0.00	-	-	-	-	NA
TryGetValue_True_ImmutableDictionary	Job-QETJTA	PR	10	178.48 ns	3.450 ns	3.543 ns	179.64 ns	170.79 ns	183.23 ns	1.03	Same	0.03	-	-	-	-	NA
ToDictionary	Job-LOTKLJ	main	100	1,039.01 ns	21.634 ns	24.913 ns	1,042.85 ns	984.15 ns	1,069.44 ns	1.00	Base	0.00	0.3080	-	-	3128 B	1.00
ToDictionary	Job-QETJTA	PR	100	1,001.41 ns	15.964 ns	14.152 ns	1,006.79 ns	967.14 ns	1,018.20 ns	0.96	Faster	0.02	0.3090	-	-	3128 B	1.00
ToImmutableDictionary	Job-LOTKLJ	main	100	13,437.35 ns	262.918 ns	258.220 ns	13,528.59 ns	12,911.66 ns	13,739.11 ns	1.00	Base	0.00	0.6020	-	-	6496 B	1.00
ToImmutableDictionary	Job-QETJTA	PR	100	13,385.87 ns	263.227 ns	281.650 ns	13,508.10 ns	12,705.08 ns	13,655.47 ns	1.00	Same	0.02	0.6111	-	-	6496 B	1.00
TryGetValue_True_Dictionary	Job-LOTKLJ	main	100	1,270.08 ns	24.849 ns	24.405 ns	1,274.04 ns	1,228.82 ns	1,306.02 ns	1.00	Base	0.00	-	-	-	-	NA
TryGetValue_True_Dictionary	Job-QETJTA	PR	100	1,355.12 ns	25.952 ns	27.768 ns	1,364.22 ns	1,293.32 ns	1,391.48 ns	1.07	Slower	0.04	-	-	-	-	NA
TryGetValue_True_ImmutableDictionary	Job-LOTKLJ	main	100	2,051.36 ns	40.157 ns	44.634 ns	2,051.86 ns	1,952.25 ns	2,141.73 ns	1.00	Base	0.00	-	-	-	-	NA
TryGetValue_True_ImmutableDictionary	Job-QETJTA	PR	100	2,024.26 ns	39.026 ns	41.758 ns	2,030.78 ns	1,918.59 ns	2,087.70 ns	0.99	Same	0.04	-	-	-	-	NA
ToDictionary	Job-LOTKLJ	main	1000	10,196.84 ns	251.988 ns	290.190 ns	10,254.18 ns	9,696.03 ns	10,705.83 ns	1.00	Base	0.00	3.0448	0.2469	-	31016 B	1.00
ToDictionary	Job-QETJTA	PR	1000	9,639.19 ns	187.533 ns	192.583 ns	9,678.42 ns	9,206.36 ns	9,917.35 ns	0.94	Faster	0.03	3.0545	0.2741	-	31016 B	1.00
ToImmutableDictionary	Job-LOTKLJ	main	1000	237,679.59 ns	3,661.106 ns	3,057.189 ns	238,570.74 ns	229,820.64 ns	240,334.00 ns	1.00	Base	0.00	5.6818	0.9470	-	64102 B	1.00
ToImmutableDictionary	Job-QETJTA	PR	1000	230,173.07 ns	4,311.465 ns	3,822.002 ns	231,834.56 ns	219,823.01 ns	233,708.06 ns	0.97	Same	0.02	6.3406	0.9058	-	64102 B	1.00
TryGetValue_True_Dictionary	Job-LOTKLJ	main	1000	15,714.41 ns	299.767 ns	294.411 ns	15,796.10 ns	15,092.75 ns	16,233.26 ns	1.00	Base	0.00	-	-	-	-	NA
TryGetValue_True_Dictionary	Job-QETJTA	PR	1000	16,141.57 ns	315.820 ns	295.418 ns	16,228.09 ns	15,554.74 ns	16,578.18 ns	1.03	Same	0.02	-	-	-	-	NA
TryGetValue_True_ImmutableDictionary	Job-LOTKLJ	main	1000	59,513.28 ns	1,073.929 ns	1,004.554 ns	59,719.73 ns	57,310.95 ns	60,994.35 ns	1.00	Base	0.00	-	-	-	-	NA
TryGetValue_True_ImmutableDictionary	Job-QETJTA	PR	1000	59,640.00 ns	1,154.325 ns	1,079.756 ns	60,059.92 ns	57,502.75 ns	60,990.41 ns	1.00	Same	0.03	-	-	-	2 B	NA
ToDictionary	Job-LOTKLJ	main	10000	244,971.90 ns	4,813.058 ns	5,349.701 ns	245,296.34 ns	235,273.75 ns	253,046.79 ns	1.00	Base	0.00	76.7857	76.7857	76.7857	283073 B	1.00
ToDictionary	Job-QETJTA	PR	10000	246,629.63 ns	7,293.248 ns	8,106.426 ns	243,601.12 ns	233,309.42 ns	263,366.79 ns	1.01	Same	0.03	76.4925	76.4925	76.4925	283068 B	1.00
ToImmutableDictionary	Job-LOTKLJ	main	10000	3,143,437.89 ns	60,920.847 ns	59,832.417 ns	3,164,813.12 ns	3,007,650.00 ns	3,203,625.00 ns	1.00	Base	0.00	62.5000	25.0000	-	640105 B	1.00
ToImmutableDictionary	Job-QETJTA	PR	10000	3,190,842.99 ns	57,976.310 ns	62,034.002 ns	3,207,857.50 ns	3,062,612.50 ns	3,308,832.50 ns	1.01	Same	0.03	62.5000	25.0000	-	640105 B	1.00
TryGetValue_True_Dictionary	Job-LOTKLJ	main	10000	199,541.34 ns	4,524.300 ns	5,028.748 ns	200,181.25 ns	191,146.48 ns	211,170.08 ns	1.00	Base	0.00	-	-	-	1 B	1.00
TryGetValue_True_Dictionary	Job-QETJTA	PR	10000	195,256.67 ns	3,751.698 ns	3,684.669 ns	196,337.70 ns	188,147.78 ns	200,153.24 ns	0.98	Same	0.04	-	-	-	5 B	5.00
TryGetValue_True_ImmutableDictionary	Job-LOTKLJ	main	10000	884,652.17 ns	15,963.048 ns	14,150.826 ns	891,639.79 ns	856,336.67 ns	898,688.75 ns	1.00	Base	0.00	-	-	-	3 B	1.00
TryGetValue_True_ImmutableDictionary	Job-QETJTA	PR	10000	906,593.35 ns	17,063.661 ns	14,248.930 ns	910,996.53 ns	875,974.31 ns	928,415.62 ns	1.03	Same	0.03	-	-	-	3 B	1.00

In certain cases performance becomes up to 2x slower, but this impacts cases where lookup is currently incorrect.

[eiriktsarpalis]

@andrewjsaid there seem to be a few failing unit tests from KeyAnalyzer.

[andrewjsaid]

@eiriktsarpalis there's 2 parts to the reduced performance.

As the bug is causing us to use case sensitive comparison it's artificially faster than any correct version of the code thus it's not a fair comparison.

The second part is the cost of removing this optimization which boils down to case sensitivity of a a partial substring of up to 8 chars. To measure that change we can benchmark with #93986 which has a much larger diff but keeps the optimization.

[eiriktsarpalis]

Thanks

[eiriktsarpalis]

Thank you for the help @andrewjsaid!

[eiriktsarpalis]

/backport to release/8.0

[github-actions]

Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/6856200961

[github-actions]

@eiriktsarpalis backporting to release/8.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Add failing tests
Applying: Fix incorrect case sensitivity in FrozenDictionary and FrozenSet for some cases
Applying: When hashing the entire string, case sensitivity of hash and equals should be the same
Applying: Address code review comments
Applying: Only ignore case insensitivity if entire string is ASCII non-letters
error: sha1 information is lacking or useless (src/libraries/System.Collections.Immutable/src/System.Collections.Immutable.csproj).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0005 Only ignore case insensitivity if entire string is ASCII non-letters
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

[github-actions]

@eiriktsarpalis an error occurred while backporting to release/8.0, please check the run log for details!

Error: git am failed, most likely due to a merge conflict.

[andrewjsaid]

Pleased to be of help!