Use bit mask in Regex's IsWordChar

[stephentoub]

Improves throughput of IsWordChar on non-ASCII chars by ~15%.

Roslyn has also fixed the codegen for is/is not patterns, so the switch expressions used here instead have been switched back.

[ghost]

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

Issue Details

---

Improves throughput of IsWordChar on non-ASCII chars by ~15%.

Roslyn has also fixed the codegen for is/is not patterns, so the switch expressions used here instead have been switched back.

Author:	stephentoub
Assignees:	stephentoub
Labels:	area-System.Text.RegularExpressions
Milestone:	-

[IDisposable]

There are three blocks of identical code (not counting the source emitting version nor the test) for the WordCategories constant. Could we pull this out somewhere common so we don't eventually get them out of sync?

[stephentoub]

Could we pull this out somewhere common so we don't eventually get them out of sync?

There are three copies of the const. We could remove one of the three by combining two of them into a field const, but the one in the source generator needs to remain separate. The categories that make up \w are also never going to change.

[IDisposable]

Working on the coalescing of the constant...

Also I noticed a what looks like a typo

 int chDiv8 = ch >> 3;
 return (uint)chDiv8 < (uint)ascii.Length ?
     (ascii[chDiv8] & (1 << (ch & 0x7))) != 0 :
     ((WordCategoriesMask & (1 << (int)CharUnicodeInfo.GetUnicodeCategory(ch))) != 0 ||
      (ch == ZeroWidthJoiner | ch == ZeroWidthNonJoiner));   

That last line shoudn't the | ch == actually be || ch == so we're doing a logical OR not a bitwise OR?

[IDisposable]

@stephentoub is there a performance history that explains the duplicate code in IsWordChar(char ch) and IsBoundaryWordChar(char ch)? Specifically, is there any reason we should not just make IsBoundaryWordChar call into IsWordChar like

  public static bool IsBoundaryWordChar(char ch)
  {
      // According to UTS#18 Unicode Regular Expressions (http://www.unicode.org/reports/tr18/)
      // RL 1.4 Simple Word Boundaries  The class of <word_character> includes all Alphabetic
      // values from the Unicode character database, from UnicodeData.txt [UData], plus the U+200C
      // ZERO WIDTH NON-JOINER and U+200D ZERO WIDTH JOINER.
      const char ZeroWidthNonJoiner = '\u200C', ZeroWidthJoiner = '\u200D';
      return IsWordChar(ch) || ch == ZeroWidthJoiner || ch == ZeroWidthNonJoiner;
  }

For branch prediction and common data, I would expect the ordering shown above to be more valuable than doing the cheaper, but less likely character equality checks first...

[stephentoub]

Specifically, is there any reason we should not just make IsBoundaryWordChar call into IsWordChar like

Because then you'll be doing the extra boundary checks even in the ASCII case where they're unnecessary.

[stephentoub]

That last line shoudn't the | ch == actually be || ch == so we're doing a logical OR not a bitwise OR?

That is on purpose based on perf testing and the elimination of the branch on average being better.

[IDisposable]

Glad I asked! I've got the PR ready then... one other question. Would it be ugly to expose this constant in some way so the Emitter generated code could just reference it? Is there precedent for that sort of construct?

[stephentoub]

Would it be ugly to expose this constant in some way so the Emitter generated code could just reference it?

Yes. We want the emitted code to be as readable as possible, which means it's better for it to read:

           const int WordCategories =
               1 << (int)UnicodeCategory.UppercaseLetter |
               1 << (int)UnicodeCategory.LowercaseLetter |
               1 << (int)UnicodeCategory.TitlecaseLetter |
               1 << (int)UnicodeCategory.ModifierLetter |
               1 << (int)UnicodeCategory.OtherLetter |
               1 << (int)UnicodeCategory.NonSpacingMark |
               1 << (int)UnicodeCategory.DecimalDigitNumber |
               1 << (int)UnicodeCategory.ConnectorPunctuation;

than to read:

           const int WordCategories = 0x4013F;

[EgorBo]

than to read:

Reminds me this

runtime/src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.cs

Lines 90 to 92 in c3983a2

	internal static bool IsPrimitiveType(this CorElementType et)
	// COR_ELEMENT_TYPE_I1,I2,I4,I8,U1,U2,U4,U8,R4,R8,I,U,CHAR,BOOLEAN
	=> ((1 << (int)et) & 0b_0011_0000_0000_0011_1111_1111_1100) != 0;

[IDisposable]

PR submitted, I did make a change in Emitter, but ONLY to change the name to WordCategoriesMask to better match the library code.

My follow-up question for the #84003 (comment) is more along the lines of should we expose a public constant from RegexCharClass that is referenced in the Emitter's output code such that the emitted code looks like

    "    return (uint)chDiv8 < (uint)ascii.Length ?",
    "        (ascii[chDiv8] & (1 << (ch & 0x7))) != 0 :",
    "        (RegexCharClass.WordCategoriesMask & (1 << (int)CharUnicodeInfo.GetUnicodeCategory(ch))) != 0;",
    "}",
});

[stephentoub]

My follow-up question for the #84003 (comment) is more along the lines of should we expose a public constant from RegexCharClass that is referenced in the Emitter's output code such that the emitted code looks like

No, I don't believe there's enough value in that (and RegexCharClass is internal). This is how we currently do the check; that could change in the future, and we'd be exposing public API for what's an implementation detail.