Switch grapheme break property searching to Eytzinger binary search #71668

Catfish-Man · 2024-02-15T23:40:27Z

Fixes rdar://123055338

Catfish-Man · 2024-02-15T23:40:36Z

@swift-ci please test

Catfish-Man · 2024-02-15T23:41:05Z

@swift-ci Please Apple Silicon benchmark

Catfish-Man · 2024-02-16T00:15:11Z

@swift-ci please test

Catfish-Man · 2024-02-16T00:15:19Z

@swift-ci Please Apple Silicon benchmark

Catfish-Man · 2024-02-16T01:36:14Z

Preliminary results look nice

------- Performance (arm64): -O -------

IMPROVEMENT                               OLD         NEW         DELTA    RATIO    
StringWithCString2                        0.001       0.0         -50.0%   **2.00x (?)**
EqualSubstringSubstring                   34.328      19.344      -43.6%   **1.77x**
EqualSubstringString                      34.319      19.344      -43.6%   **1.77x**
LessSubstringSubstringGenericComparable   34.319      19.344      -43.6%   **1.77x**
EqualSubstringSubstringGenericEquatable   34.319      19.344      -43.6%   **1.77x**
LessSubstringSubstring                    34.319      19.344      -43.6%   **1.77x**
EqualStringSubstring                      34.321      19.346      -43.6%   **1.77x**
StringDistance.characters.mixed           4555.0      2887.0      -36.6%   **1.58x**
CharacterRecognizer.mixed                 24.94       16.344      -34.5%   **1.53x**
StringComparison_longSharedPrefix         253.889     173.571     -31.6%   **1.46x**
StringHasPrefixUnicode                    33584.615   27386.667   -18.5%   **1.23x**
SortStringsUnicode                        1780.0      1488.571    -16.4%   **1.20x**
Set.isDisjoint.Seq.Empty.Box              45.25       39.0        -13.8%   **1.16x**
StringUTF16SubstringBuilder               964.762     837.692     -13.2%   **1.15x**
StringHasSuffixUnicode                    48333.333   42566.038   -11.9%   **1.14x**
Breadcrumbs.IdxToUTF16Range.longASCII     10.308      9.37        -9.1%    **1.10x**
Set.isDisjoint.Seq.Box.Empty              70.2        63.974      -8.9%    **1.10x**
StringFromLongWholeSubstring              2.18        2.026       -7.1%    **1.08x (?)**

------- Performance (arm64): -Osize -------

IMPROVEMENT                               OLD         NEW         DELTA    RATIO    
EqualSubstringString                      34.478      19.032      -44.8%   **1.81x**
EqualStringSubstring                      34.473      19.035      -44.8%   **1.81x**
LessSubstringSubstring                    34.642      19.188      -44.6%   **1.81x**
EqualSubstringSubstring                   34.641      19.192      -44.6%   **1.80x**
LessSubstringSubstringGenericComparable   34.638      19.192      -44.6%   **1.80x**
EqualSubstringSubstringGenericEquatable   34.638      19.192      -44.6%   **1.80x**
StringDistance.characters.mixed           4554.0      2887.0      -36.6%   **1.58x**
CharacterRecognizer.mixed                 24.916      16.32       -34.5%   **1.53x**
StringComparison_longSharedPrefix         267.556     187.692     -29.8%   **1.43x**
StringHasPrefixUnicode                    33461.538   27576.923   -17.6%   **1.21x**
SortStringsUnicode                        1795.833    1520.714    -15.3%   **1.18x**
StringHasSuffixUnicode                    48425.532   42666.667   -11.9%   **1.13x (?)**
StringUTF16SubstringBuilder               1025.909    938.333     -8.5%    **1.09x (?)**
StringFromLongWholeSubstring              2.183       2.028       -7.1%    **1.08x (?)**

Catfish-Man · 2024-02-16T06:59:11Z

128-byte aligning the array is ever so slightly slower, so no need to change that

Catfish-Man · 2024-02-16T07:15:57Z

@swift-ci Please Apple Silicon benchmark

Catfish-Man · 2024-02-16T07:16:06Z

@swift-ci please test

Catfish-Man · 2024-02-16T08:33:23Z

Now THIS is pod racing

------- Performance (arm64): -O -------

REGRESSION                        OLD         NEW         DELTA    RATIO    
DataAppendArray                   1768.908    1919.0      +8.5%    **0.92x (?)**

IMPROVEMENT                       OLD         NEW         DELTA    RATIO    
StringDistance.characters.mixed   4554.0      1577.0      -65.4%   **2.89x**
CharacterRecognizer.mixed         24.93       10.298      -58.7%   **2.42x**
StringHasPrefixUnicode            33600.0     24815.385   -26.1%   **1.35x**
StringHasSuffixUnicode            48382.353   39800.0     -17.7%   **1.22x**
StringUTF16SubstringBuilder       964.167     837.407     -13.1%   **1.15x (?)**
CSVParsing.Char                   94.833      87.346      -7.9%    **1.09x**
StringFromLongWholeSubstring      2.183       2.026       -7.2%    **1.08x (?)**

------- Performance (arm64): -Osize -------

REGRESSION                                OLD         NEW         DELTA    RATIO    
DataCreateEmptyArray                      686.538     811.232     +18.2%   **0.85x**
BufferFillFromSlice                       10.579      12.094      +14.3%   **0.87x (?)**
CharIteration_punctuated_unicodeScalars   652.101     731.681     +12.2%   **0.89x (?)**
DataCreateSmallArray                      1279.213    1404.217    +9.8%    **0.91x**

IMPROVEMENT                               OLD         NEW         DELTA    RATIO    
StringDistance.characters.mixed           4554.0      1576.0      -65.4%   **2.89x**
CharacterRecognizer.mixed                 24.916      10.545      -57.7%   **2.36x**
StringHasPrefixUnicode                    33464.286   24821.918   -25.8%   **1.35x**
StringHasSuffixUnicode                    48520.833   39896.552   -17.8%   **1.22x**
StringUTF16SubstringBuilder               1018.636    902.4       -11.4%   **1.13x**
CSVParsing.Char                           98.739      91.68       -7.1%    **1.08x (?)**
StringFromLongWholeSubstring              2.184       2.028       -7.1%    **1.08x**

Catfish-Man · 2024-02-16T17:25:56Z

@swift-ci Please Apple Silicon benchmark

Catfish-Man · 2024-02-16T17:26:06Z

@swift-ci please test

Catfish-Man · 2024-02-16T18:56:21Z

stdlib/public/core/UnicodeBreakProperty.swift

    case v
    case zwj

+    @inline(__always)


The purpose of making sure this gets inlined is to allow the compiler enough visibility to combine the switch statement here with the switch statement in the caller, rather than going switch -> flag -> switch

Catfish-Man · 2024-02-16T18:57:26Z

stdlib/public/stubs/Unicode/UnicodeGrapheme.cpp

+
+    //If we want the left child of the current node in our virtual tree,
+    //that's at index * 2, if we want the right child it's at (index * 2) + 1
    if (scalar < lower) {


This branch could be hoisted above fetching upper and enumValue, but the optimizer successfully does that for us, and it reads better this way

milseman

Overall LGTM but I'd like @Azoy to review as well

milseman · 2024-02-16T18:55:56Z

stdlib/public/stubs/Unicode/UnicodeGrapheme.cpp

 #include "swift/shims/UnicodeData.h"
 #include <limits>

+


@Azoy could you look over the changes to the C++ code and the strategy used?

stdlib/public/core/StringGraphemeBreaking.swift

utils/gen-unicode-data/Sources/GenUtils/Emit.swift

stephentyrone · 2024-02-16T18:58:21Z

stdlib/public/stubs/Unicode/Common/GraphemeData.h

-  0x8061F23C, 0xB621F249, 0x2081F3FB, 0xA7A1F400, 0xA121F546, 0x8FE1F680, 0x8161F774, 0x8541F7D5,
-  0x8061F80C, 0x80E1F848, 0x80A1F85A, 0x80E1F888, 0x8A21F8AE, 0x85C1F90C, 0x8121F93C, 0xB701F947,
-  0x3EE0000, 0x2BEE0020, 0xFEE0080, 0x3DEE0100,
+static const __swift_uint32_t _swift_stdlib_graphemeBreakProperties[638] = {


You don't really need the dummy 0th element, but also it doesn't really matter =)

But the 4 bytes of binary size Steve!

Co-authored-by: Michael Ilseman <[email protected]>

Azoy

This is wonderful! Apart from what Michael has already mentioned, everything here looks good to me!

Catfish-Man · 2024-02-16T19:10:51Z

@swift-ci please test

Catfish-Man · 2024-02-16T19:13:04Z

@swift-ci please test

Catfish-Man self-assigned this Feb 15, 2024

Switch grapheme break property searching to Eytzinger binary search

d8f47a5

Catfish-Man force-pushed the ancient-austrian-techniques branch from 3123ce4 to d8f47a5 Compare February 16, 2024 00:15

Catfish-Man marked this pull request as ready for review February 16, 2024 06:19

Catfish-Man requested a review from a team as a code owner February 16, 2024 06:19

Combine conditionals

ce4bd2e

A few more microoptimizations

5c8ea26

Catfish-Man mentioned this pull request Feb 16, 2024

Optimize grapheme breaking #71595

Closed

Catfish-Man commented Feb 16, 2024

View reviewed changes

milseman approved these changes Feb 16, 2024

View reviewed changes

stephentyrone reviewed Feb 16, 2024

View reviewed changes

Apply suggestions from code review

2ade969

Co-authored-by: Michael Ilseman <[email protected]>

Azoy approved these changes Feb 16, 2024

View reviewed changes

Catfish-Man enabled auto-merge (squash) February 16, 2024 19:10

Add a comment describing what I'm doing

b206358

Catfish-Man disabled auto-merge February 16, 2024 19:13

Catfish-Man enabled auto-merge (squash) February 16, 2024 19:14

Catfish-Man merged commit ea7d077 into swiftlang:main Feb 17, 2024

Switch grapheme break property searching to Eytzinger binary search #71668

Switch grapheme break property searching to Eytzinger binary search #71668

Uh oh!

Conversation

Catfish-Man commented Feb 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Catfish-Man commented Feb 15, 2024

Uh oh!

Catfish-Man commented Feb 15, 2024

Uh oh!

Catfish-Man commented Feb 16, 2024

Uh oh!

Catfish-Man commented Feb 16, 2024

Uh oh!

Catfish-Man commented Feb 16, 2024

Uh oh!

Catfish-Man commented Feb 16, 2024

Uh oh!

Catfish-Man commented Feb 16, 2024

Uh oh!

Catfish-Man commented Feb 16, 2024

Uh oh!

Catfish-Man commented Feb 16, 2024

Uh oh!

Catfish-Man commented Feb 16, 2024

Uh oh!

Catfish-Man commented Feb 16, 2024

Uh oh!

Catfish-Man Feb 16, 2024

Choose a reason for hiding this comment

Uh oh!

Catfish-Man Feb 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

milseman left a comment

Choose a reason for hiding this comment

Uh oh!

milseman Feb 16, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stephentyrone Feb 16, 2024

Choose a reason for hiding this comment

Uh oh!

Azoy Feb 16, 2024

Choose a reason for hiding this comment

Uh oh!

Azoy left a comment

Choose a reason for hiding this comment

Uh oh!

Catfish-Man commented Feb 16, 2024

Uh oh!

Catfish-Man commented Feb 16, 2024

Uh oh!

Uh oh!

Catfish-Man commented Feb 15, 2024 •

edited

Loading

Catfish-Man Feb 16, 2024 •

edited

Loading