Length-based switch dispatch #66081

jcouv · 2022-12-20T21:29:08Z

Design

There are currently multiple strategies for emitting switch dispatches on strings:

a linear sequence of string comparisons (when fewer than 7 cases)
dispatch on the input's hash code then do a final string comparison

This PR adds a third strategy, which is based on avoiding computing the hash code (relatively expensive):

dispatch on input length, then dispatch on one character/position, then do a final string dispatch (when a few cases remain) or comparison (when a single case remains)

Performance

The strategy was evaluated in terms of performance (typical, average and worst case, based on synthetic and realistic cases) and IL size. The plan is to do further performance evaluation on different runtime environments using the runtime's perf lab. The PR also includes an opt-out flag (that we could remove after some period of time) in case the optimization somehow caused a performance regression.

When the final string dispatch involves more than 7 cases, we expect the new approach to perform worse. In simplified terms, the cost of "Length dispatch + char dispatch + hashcode dispatch" is worse than that of "hashcode dispatch".

But what about "Length dispatch + character dispatch + N string comparisons"?
Based on measurements, this new approach is most clearly beneficial when the number of cases for final string dispatch (N) is 5 or lower.

So the new approach is used when there are more than 7 cases and the final string dispatch buckets have 5 or fewer cases.

Notes:

this implements shortcuts where possible: for example, whenever a Length dispatch leaves only one possible candidate left, we skip the character dispatch and directly go to the final string comparison.
the selection of the most "distinctive" character position for cases of a certain length is a heuristic. We prefer positions that yield most buckets of size 1. As a secondary criteria, we prefer positions that minimize the size of the largest bucket.

Implementation

Most of the optimization is implemented in the same place where the hashcode optimization was implemented, ie in the emit layer for BoundSwitchDispatch. But since the decision to emit private helpers is made in lowering (LowerSwitchDispatchNode), the analysis of the Length+char dispatch is also performed during lowering (and the result stashed until needed for emitting).
I've opted for using LINQ in a few places to favor readability. I'm open to discussing some more efficient ways of doing the bucket analysis.
I'm also open to removing some of the IL verification, to make the tests more compact.

jcouv · 2022-12-21T20:51:36Z

};

📝 Illustration of the tree of cases (Dump() method output) for this test case:

Length dispatch:
Buckets: 1, 1, 1, 1, 1, 1, 3, 1, 1, 1, 1
  case null: <null>
  case default(int): <arm-21>
  case 1: <char-dispatch-40>
  case 2: <string-dispatch-41>
  case 3: <string-dispatch-42>
  case 4: <string-dispatch-43>
  case 5: <char-dispatch-52>

Char dispatches:
Label <char-dispatch-40>:
  Selected char position: 0:
  case a: <arm-22>
  case b: <arm-23>
  case c: <arm-24>
Label <char-dispatch-52>:
  Selected char position: 0:
  case a: <string-dispatch-44>
  case b: <string-dispatch-45>
  case h: <string-dispatch-46>
  case l: <string-dispatch-47>
  case n: <string-dispatch-48>
  case s: <string-dispatch-49>
  case t: <string-dispatch-50>
  case w: <string-dispatch-51>

String dispatches:
Label <string-dispatch-41>:
  case "no": <arm-25>
Label <string-dispatch-42>:
  case "yes": <arm-26>
Label <string-dispatch-43>:
  case "four": <arm-27>
Label <string-dispatch-44>:
  case "alice": <arm-28>
Label <string-dispatch-45>:
  case "blurb": <arm-29>
Label <string-dispatch-46>:
  case "hello": <arm-30>
Label <string-dispatch-47>:
  case "lamps": <arm-31>
  case "lambs": <arm-32>
  case "lower": <arm-33>
Label <string-dispatch-48>:
  case "names": <arm-34>
Label <string-dispatch-49>:
  case "slurp": <arm-35>
Label <string-dispatch-50>:
  case "towed": <arm-36>
Label <string-dispatch-51>:
  case "words": <arm-37>

Refers to: src/Compilers/CSharp/Test/Emit2/CodeGen/CodeGenLengthBasedSwitchTests.cs:72 in 04e329a. [](commit_id = 04e329a, deletion_comment = False)

EgorBo · 2022-12-21T22:06:41Z

src/Compilers/CSharp/Portable/BoundTree/LengthBasedStringSwitchData.cs

+    //
+    //   // strings of length 1 don't need any further validation once we've checked one char
+    //   case 1:
+    //     switch (key[posM])


can be just key[0] here for this case?

iSazonov · 2022-12-22T07:08:33Z

You may already be using this - since the first step uses the string length, the compiler can remove the boundary check in the following steps.

iSazonov · 2022-12-22T07:12:52Z

When the final string dispatch involves more than 7 cases, we expect the new approach to perform worse.

You could switch the check from a single character to a pair (i.e. ushort -> uint). This can noticeably reduce collisions in some cases.

geeknoid · 2023-01-03T15:05:22Z

Hey, just a drive-by comment here from the peanut gallery.

When I implemented the frozen collections, I pulled out all the stops to try and get as much performance possible for read-only dictionaries and sets. This resulted in appreciable gains overall. I think the compiler should be able to adopt nearly all the same techniques to achieve similar benefits.

For example, I implemented support for partial string hashing. Since all the strings are known a priori, you know which part of the strings are different and can thus limit hashing to only the parts of the strings which are different. To make this work better, I added support for both left- and right-justified strings which deals with strings having common prefixes or suffxes cleanly.

Go take a look at https://github.com/dotnet/runtime/blob/main/src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/StringComparers/ComparerPicker.cs for the logic that selects how to compare strings within the frozen collections. I suspect most of the same logic would work within the compiler as well.

jcouv · 2023-01-10T18:18:20Z

@dotnet/roslyn-compiler for review. Thanks

jcouv · 2023-01-12T18:01:30Z

@dotnet/roslyn-compiler for review. Thanks

jjonescz · 2023-01-16T11:09:16Z

src/Compilers/CSharp/Portable/CSharpResources.resx

@@ -7406,4 +7406,7 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
  <data name="WRN_ParamsArrayInLambdaOnly_Title" xml:space="preserve">
    <value>Parameter has params modifier in lambda but not in target delegate type.</value>
  </data>
+  <data name="IDS_DisableLengthBasedSwitch" xml:space="preserve">
+    <value>disable-length-based switch</value>


disable-length-based

Space instead of the first hyphen? #Closed

Actually intended for hyphens throughout. Fixed and added a command-line test.

jjonescz · 2023-01-16T11:09:31Z

src/Compilers/CSharp/Portable/BoundTree/LengthBasedStringSwitchData.cs

+
+            internal StringJumpTable(LabelSymbol label, ImmutableArray<(ConstantValue value, LabelSymbol label)> stringCaseLabels)
+            {
+                Debug.Assert(stringCaseLabels.All(c => c.value.IsString) && stringCaseLabels.Length > 0);


stringCaseLabels.Length > 0

Should similar assert be also in the other two tables above? #Closed

jjonescz · 2023-01-16T11:09:42Z

src/Compilers/CSharp/Portable/BoundTree/LengthBasedStringSwitchData.cs

+                    var currentChar = caseLabel.value.StringValue[position];
+                    if (countPerChar.TryGetValue(currentChar, out var currentCount))
+                    {
+                        countPerChar[currentChar]++;


++;

= currentCount + 1; to avoid duplicate read #Closed

jjonescz · 2023-01-16T11:09:53Z

src/Compilers/CSharp/Portable/BoundTree/LengthBasedStringSwitchData.cs

+            foreach (var group in casesWithGivenLength.GroupBy(c => c.value.StringValue![bestCharacterPosition]))
+            {
+                char character = group.Key;
+                var label = CreateAndRegisterStringJumpTable(group.Select(c => c).ToImmutableArray(), stringJumpTables);


.Select(c => c)

This seems unnecessary #Closed

jjonescz · 2023-01-16T11:14:52Z

src/Compilers/CSharp/Portable/CodeGen/EmitStatement.cs

        {
            Debug.Assert(expression.ConstantValue == null);
            Debug.Assert((object)expression.Type != null &&
                (expression.Type.IsValidV6SwitchGoverningType() || expression.Type.IsSpanOrReadOnlySpanChar()));
            Debug.Assert(switchCaseLabels.Length > 0);

-            Debug.Assert(switchCaseLabels != null);


Debug.Assert(switchCaseLabels != null);

Perhaps assert that either switchCaseLabels or lengthBasedSwitchStringJumpTableOpt is not null? #Closed

jjonescz · 2023-01-16T11:28:47Z

src/Compilers/CSharp/Portable/CodeGen/EmitStatement.cs

+
+                Debug.Assert(spanTLengthMethod != null && !spanTLengthMethod.HasUseSiteError);
+                var spanCharLengthMethod = spanTLengthMethod.AsMember((NamedTypeSymbol)keyType);
+                return _module.Translate(spanCharLengthMethod, null, _diagnostics.DiagnosticBag);


null

Why isn't syntax node passed here, too? #Closed

This is moved code. The null was in the original code. Fixed it now.

jcouv · 2023-01-25T08:10:20Z

src/Compilers/CSharp/Test/Emit/CodeGen/SwitchTests.cs

+  IL_0036:  bne.un.s   IL_00ad
+  IL_0038:  ldarg.0
+  IL_0039:  ldc.i4.0
+  IL_003a:  call       "char string.this[int].get"


📝 we only need to do a character check, no need to follow up with a string check, since we're dealing with strings of length==1.

jcouv · 2023-01-25T15:45:52Z

/azp run

azure-pipelines · 2023-01-25T15:46:15Z