Skip to content
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.

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

Length-based switch dispatch #66081

Merged
merged 15 commits into from
Jan 28, 2023
Merged

Length-based switch dispatch #66081

merged 15 commits into from
Jan 28, 2023

Conversation

jcouv
Copy link
Member

@jcouv jcouv commented Dec 20, 2022

Closes #56374

Design

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

  1. a linear sequence of string comparisons (when fewer than 7 cases)
  2. 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):

  1. 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 jcouv added this to the 17.5 milestone Dec 21, 2022
@jcouv jcouv marked this pull request as ready for review December 21, 2022 18:14
@jcouv jcouv requested a review from a team as a code owner December 21, 2022 18:14
@jcouv
Copy link
Member Author

jcouv commented Dec 21, 2022

    };

📝 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)

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

Choose a reason for hiding this comment

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

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

@iSazonov
Copy link

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
Copy link

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
Copy link
Member

geeknoid commented Jan 3, 2023

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 jcouv modified the milestones: 17.5, 17.6 Jan 5, 2023
@jcouv
Copy link
Member Author

jcouv commented Jan 10, 2023

@dotnet/roslyn-compiler for review. Thanks

1 similar comment
@jcouv
Copy link
Member Author

jcouv commented Jan 12, 2023

@dotnet/roslyn-compiler for review. Thanks

@@ -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>
Copy link
Member

@jjonescz jjonescz Jan 16, 2023

Choose a reason for hiding this comment

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

disable-length-based

Space instead of the first hyphen? #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

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


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

@jjonescz jjonescz Jan 16, 2023

Choose a reason for hiding this comment

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

stringCaseLabels.Length > 0

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

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

@jjonescz jjonescz Jan 16, 2023

Choose a reason for hiding this comment

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

++;

= currentCount + 1; to avoid duplicate read #Closed

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

@jjonescz jjonescz Jan 16, 2023

Choose a reason for hiding this comment

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

.Select(c => c)

This seems unnecessary #Closed

{
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);
Copy link
Member

@jjonescz jjonescz Jan 16, 2023

Choose a reason for hiding this comment

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

Debug.Assert(switchCaseLabels != null);

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


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

@jjonescz jjonescz Jan 16, 2023

Choose a reason for hiding this comment

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

null

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@jcouv jcouv requested a review from jjonescz January 18, 2023 19:35
@jcouv jcouv requested a review from cston January 25, 2023 07:54
IL_0036: bne.un.s IL_00ad
IL_0038: ldarg.0
IL_0039: ldc.i4.0
IL_003a: call "char string.this[int].get"
Copy link
Member Author

Choose a reason for hiding this comment

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

📝 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
Copy link
Member Author

jcouv commented Jan 25, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@jcouv
Copy link
Member Author

jcouv commented Jan 25, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@jcouv
Copy link
Member Author

jcouv commented Jan 26, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@jcouv
Copy link
Member Author

jcouv commented Jan 27, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@jcouv
Copy link
Member Author

jcouv commented Jan 27, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@jcouv
Copy link
Member Author

jcouv commented Jan 28, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@jcouv jcouv merged commit 8aefb7c into dotnet:main Jan 28, 2023
@ghost ghost modified the milestones: 17.6, Next Jan 28, 2023
@jcouv jcouv deleted the switch-string branch January 28, 2023 23:28
@Cosifne Cosifne modified the milestones: Next, 17.6 P1 Jan 31, 2023
@EgorBo
Copy link
Member

EgorBo commented Feb 22, 2023

@jcouv can it possibly be the reason of increased build times in dotnet/runtime? dotnet/runtime#82583 (that Roslyn update contained this PR)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Performance: faster switch over string objects
7 participants