Skip to content

Bridged Strings should have some different/additional overrides for performance (<rdar://problem/26236614>) #20383

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Nov 8, 2018
Merged

Bridged Strings should have some different/additional overrides for performance (<rdar://problem/26236614>) #20383

merged 1 commit into from
Nov 8, 2018

Conversation

@Catfish-Man
Copy link
Contributor

@Catfish-Man Catfish-Man commented Nov 7, 2018 •
edited
Loading

This provides specialized overrides of more NSString methods, as well as moving dynamic dispatch boundaries around and sprinkling @_effects in to reduce refcounting overhead.

rdar://problem/26236614
@Catfish-Man Catfish-Man changed the title Bridged Strings should have some different/additional overrides for performance Nov 7, 2018
@Catfish-Man
Copy link
Contributor Author

Catfish-Man commented Nov 7, 2018

@swift-ci please benchmark
@Catfish-Man
Copy link
Contributor Author

Catfish-Man commented Nov 7, 2018

@swift-ci please test
Catfish-Man
Catfish-Man commented Nov 7, 2018
View reviewed changes
Catfish-Man
Catfish-Man commented Nov 7, 2018
View reviewed changes
Catfish-Man
Catfish-Man commented Nov 7, 2018
View reviewed changes
Catfish-Man
Catfish-Man commented Nov 7, 2018
View reviewed changes
Catfish-Man
Catfish-Man commented Nov 7, 2018
View reviewed changes
Catfish-Man
Catfish-Man commented Nov 7, 2018
View reviewed changes
@gottesmm
Copy link
Contributor

gottesmm commented Nov 7, 2018

@Catfish-Man Do you think the comments in the PR that you have put here should be comments in source instead? (Have no opinion, just seems like useful information)
@Catfish-Man
Copy link
Contributor Author

Catfish-Man commented Nov 7, 2018

@gottesmm probably so :) good point
@Catfish-Man Catfish-Man self-assigned this Nov 7, 2018
@swift-ci
Copy link
Contributor

swift-ci commented Nov 7, 2018

Build comment file:

Performance: -O

TEST OLD NEW DELTA RATIO
Improvement
ObjectiveCBridgeStringIsEqualAllSwift 909 62 -93.2% 14.66x
ObjectiveCBridgeStringUTF8String 321 22 -93.1% 14.59x
ObjectiveCBridgeStringIsEqual2 1055 227 -78.5% 4.65x
ObjectiveCBridgeStringHash 141 87 -38.3% 1.62x
ObjectiveCBridgeStringGetUTF8Contents 458 299 -34.7% 1.53x
ObjectiveCBridgeStringGetASCIIContents 568 373 -34.3% 1.52x
ObjectiveCBridgeStringIsEqual 280 197 -29.6% 1.42x
ObjectiveCBridgeStringCompare 1158 921 -20.5% 1.26x
ObjectiveCBridgeStringRangeOfString 1094 898 -17.9% 1.22x
ObjectiveCBridgeStringCompare2 1112 913 -17.9% 1.22x
StringEqualPointerComparison 564 512 -9.2% 1.10x

Performance: -Osize

TEST OLD NEW DELTA RATIO
Improvement
ObjectiveCBridgeStringIsEqualAllSwift 906 61 -93.3% 14.85x
ObjectiveCBridgeStringUTF8String 320 22 -93.1% 14.54x
ObjectiveCBridgeStringIsEqual2 1055 229 -78.3% 4.61x
ObjectiveCBridgeStringHash 140 87 -37.9% 1.61x
ObjectiveCBridgeStringGetUTF8Contents 463 299 -35.4% 1.55x
ObjectiveCBridgeStringGetASCIIContents 574 373 -35.0% 1.54x
ObjectiveCBridgeStringIsEqual 281 191 -32.0% 1.47x
ObjectiveCBridgeStringCompare 1143 917 -19.8% 1.25x
ObjectiveCBridgeStringCompare2 1110 909 -18.1% 1.22x
ObjectiveCBridgeStringRangeOfString 1094 899 -17.8% 1.22x
StringEqualPointerComparison 564 512 -9.2% 1.10x

Performance: -Onone

TEST OLD NEW DELTA RATIO
Regression
ArrayOfGenericPOD2 955 1109 +16.1% 0.86x (?)
ArrayOfPOD 694 764 +10.1% 0.91x
Improvement
ObjectiveCBridgeStringUTF8String 326 22 -93.3% 14.82x
ObjectiveCBridgeStringIsEqualAllSwift 910 62 -93.2% 14.68x
ObjectiveCBridgeStringIsEqual2 1060 227 -78.6% 4.67x
ObjectiveCBridgeStringHash 140 87 -37.9% 1.61x
ObjectiveCBridgeStringGetUTF8Contents 459 299 -34.9% 1.54x
ObjectiveCBridgeStringGetASCIIContents 568 373 -34.3% 1.52x
ObjectiveCBridgeStringIsEqual 281 192 -31.7% 1.46x
DictionaryKeysContainsNative 67 53 -20.9% 1.26x
ObjectiveCBridgeStringCompare 1145 915 -20.1% 1.25x
ObjectiveCBridgeStringCompare2 1116 912 -18.3% 1.22x
ObjectiveCBridgeStringRangeOfString 1095 904 -17.4% 1.21x
CharIteration_korean_unicodeScalars_Backwards 254796 218523 -14.2% 1.17x
FlattenListFlatMap 272116 241809 -11.1% 1.13x
How to read the data The tables contain differences in performance which are larger than 8% and differences in code size which are larger than 1%.

If you see any unexpected regressions, you should consider fixing the regressions before you merge the PR.

Noise: Sometimes the performance results (not code size!) contain false alarms. Unexpected regressions which are marked with '(?)' are probably noise. If you see regressions which you cannot explain you can try to run the benchmarks again. If regressions still show up, please consult with the performance team (@eeckstein).

Hardware Overview 
  Model Name: Mac Pro
  Model Identifier: MacPro6,1
  Processor Name: Quad-Core Intel Xeon E5
  Processor Speed: 3.7 GHz
  Number of Processors: 1
  Total Number of Cores: 4
  L2 Cache (per Core): 256 KB
  L3 Cache: 10 MB
  Memory: 16 GB
@swift-ci
Copy link
Contributor

swift-ci commented Nov 7, 2018

Build failed
Swift Test OS X Platform
Git Sha - 753dc9619fc97f89f6f1d8f6b6281070aea6a870
@Catfish-Man
Copy link
Contributor Author

Catfish-Man commented Nov 7, 2018

Looks like the test failure is just the ABI checker complaining, which is expected. We'll see on Linux, I haven't tested it yet.
@BasThomas BasThomas mentioned this pull request Nov 7, 2018
Closed
@Catfish-Man Catfish-Man requested a review from milseman November 7, 2018 21:30
@Catfish-Man
Copy link
Contributor Author

Catfish-Man commented Nov 7, 2018

@swift-ci please benchmark
1 similar comment
@Catfish-Man
Copy link
Contributor Author

Catfish-Man commented Nov 7, 2018

@swift-ci please benchmark
@Catfish-Man Catfish-Man changed the title [DNM] Bridged Strings should have some different/additional overrides for performance Nov 7, 2018
@Catfish-Man Catfish-Man changed the title [DNM] Bridged Strings should have some different/additional overrides for performance (<rdar://problem/26236614>) Nov 7, 2018
@swift-ci
Copy link
Contributor

swift-ci commented Nov 7, 2018

Build comment file:

Performance: -O

TEST OLD NEW DELTA RATIO
Regression
DictionaryBridgeToObjC_Access 998 1227 +22.9% 0.81x (?)
Improvement
ObjectiveCBridgeStringIsEqualAllSwift 1002 61 -93.9% 16.43x
ObjectiveCBridgeStringUTF8String 357 24 -93.3% 14.87x
ObjectiveCBridgeStringIsEqual2 1132 193 -83.0% 5.87x
ObjectiveCBridgeStringHash 157 76 -51.6% 2.07x
ObjectiveCBridgeStringGetASCIIContents 631 319 -49.4% 1.98x
ObjectiveCBridgeStringGetUTF8Contents 519 267 -48.6% 1.94x
ObjectiveCBridgeStringIsEqual 309 175 -43.4% 1.77x
ObjectiveCBridgeStringCompare2 1243 854 -31.3% 1.46x
ObjectiveCBridgeStringCompare 1278 879 -31.2% 1.45x
ObjectiveCBridgeStringRangeOfString 1221 862 -29.4% 1.42x
StringEqualPointerComparison 628 571 -9.1% 1.10x

Performance: -Osize

TEST OLD NEW DELTA RATIO
Improvement
ObjectiveCBridgeStringIsEqualAllSwift 1001 61 -93.9% 16.41x
ObjectiveCBridgeStringUTF8String 356 24 -93.3% 14.83x
ObjectiveCBridgeStringIsEqual2 1142 194 -83.0% 5.89x
ObjectiveCBridgeStringGetASCIIContents 631 317 -49.8% 1.99x
ObjectiveCBridgeStringHash 157 79 -49.7% 1.99x
ObjectiveCBridgeStringGetUTF8Contents 511 259 -49.3% 1.97x
ObjectiveCBridgeStringIsEqual 310 177 -42.9% 1.75x
ObjectiveCBridgeStringCompare 1269 867 -31.7% 1.46x
ObjectiveCBridgeStringCompare2 1244 852 -31.5% 1.46x
ObjectiveCBridgeStringRangeOfString 1220 862 -29.3% 1.42x
StringEqualPointerComparison 628 571 -9.1% 1.10x
StringUTF16Builder 480 441 -8.1% 1.09x
InsertCharacterEndIndex 161 149 -7.5% 1.08x

Performance: -Onone

TEST OLD NEW DELTA RATIO
Regression
ArrayOfPOD 781 860 +10.1% 0.91x (?)
Improvement
ObjectiveCBridgeStringIsEqualAllSwift 1005 63 -93.7% 15.95x
ObjectiveCBridgeStringUTF8String 363 25 -93.1% 14.52x
ObjectiveCBridgeStringIsEqual2 1136 197 -82.7% 5.77x
ObjectiveCBridgeStringHash 157 77 -51.0% 2.04x
ObjectiveCBridgeStringGetASCIIContents 634 317 -50.0% 2.00x
ObjectiveCBridgeStringGetUTF8Contents 512 258 -49.6% 1.98x
ObjectiveCBridgeStringIsEqual 310 179 -42.3% 1.73x
ObjectiveCBridgeStringCompare2 1246 858 -31.1% 1.45x
ObjectiveCBridgeStringCompare 1271 877 -31.0% 1.45x
ObjectiveCBridgeStringRangeOfString 1232 863 -30.0% 1.43x
SubstringEqualString 1699 1445 -14.9% 1.18x
CharIndexing_ascii_unicodeScalars 362435 322423 -11.0% 1.12x
CharIndexing_tweet_unicodeScalars 717561 639618 -10.9% 1.12x
CharIndexing_chinese_unicodeScalars 276741 246966 -10.8% 1.12x
CharIndexing_punctuated_unicodeScalars 80542 72180 -10.4% 1.12x
CharIndexing_japanese_unicodeScalars 436047 391779 -10.2% 1.11x
RemoveWhereQuadraticString 2759 2513 -8.9% 1.10x
CharIndexing_utf16_unicodeScalars 302214 277128 -8.3% 1.09x
How to read the data The tables contain differences in performance which are larger than 8% and differences in code size which are larger than 1%.

If you see any unexpected regressions, you should consider fixing the regressions before you merge the PR.

Noise: Sometimes the performance results (not code size!) contain false alarms. Unexpected regressions which are marked with '(?)' are probably noise. If you see regressions which you cannot explain you can try to run the benchmarks again. If regressions still show up, please consult with the performance team (@eeckstein).

Hardware Overview 
  Model Name: Mac Pro
  Model Identifier: MacPro6,1
  Processor Name: 12-Core Intel Xeon E5
  Processor Speed: 2.7 GHz
  Number of Processors: 1
  Total Number of Cores: 12
  L2 Cache (per Core): 256 KB
  L3 Cache: 30 MB
  Memory: 64 GB
milseman
milseman approved these changes Nov 7, 2018
View reviewed changes
Copy link
Member

@milseman milseman left a comment

Choose a reason for hiding this comment

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

LGTM
@Catfish-Man
Copy link
Contributor Author

Catfish-Man commented Nov 8, 2018

@swift-ci please benchmark
2 similar comments
@Catfish-Man
Copy link
Contributor Author

Catfish-Man commented Nov 8, 2018

@swift-ci please benchmark
@Catfish-Man
Copy link
Contributor Author

Catfish-Man commented Nov 8, 2018

@swift-ci please benchmark
@milseman
Copy link
Member

milseman commented Nov 8, 2018

Right, the creation and use of the classes should always be behind a resilient function. Good catch
@swift-ci
Copy link
Contributor

swift-ci commented Nov 8, 2018

Build comment file:

Performance: -O

TEST OLD NEW DELTA RATIO
Improvement
ObjectiveCBridgeStringIsEqualAllSwift 985 63 -93.6% 15.63x
ObjectiveCBridgeStringUTF8String 358 24 -93.3% 14.92x
ObjectiveCBridgeStringIsEqual2 1137 197 -82.7% 5.77x
ObjectiveCBridgeStringHash 156 77 -50.6% 2.03x
ObjectiveCBridgeStringGetUTF8Contents 520 260 -50.0% 2.00x
ObjectiveCBridgeStringGetASCIIContents 635 325 -48.8% 1.95x
ObjectiveCBridgeStringIsEqual 308 178 -42.2% 1.73x
ObjectiveCBridgeStringCompare2 1246 846 -32.1% 1.47x
ObjectiveCBridgeStringCompare 1287 883 -31.4% 1.46x
ObjectiveCBridgeStringRangeOfString 1222 860 -29.6% 1.42x
StringEqualPointerComparison 628 571 -9.1% 1.10x

Performance: -Osize

TEST OLD NEW DELTA RATIO
Improvement
ObjectiveCBridgeStringIsEqualAllSwift 985 62 -93.7% 15.89x
ObjectiveCBridgeStringUTF8String 358 24 -93.3% 14.92x
ObjectiveCBridgeStringIsEqual2 1135 196 -82.7% 5.79x
ObjectiveCBridgeStringGetASCIIContents 635 317 -50.1% 2.00x
ObjectiveCBridgeStringHash 157 79 -49.7% 1.99x
ObjectiveCBridgeStringGetUTF8Contents 512 259 -49.4% 1.98x
ObjectiveCBridgeStringIsEqual 314 178 -43.3% 1.76x
ObjectiveCBridgeStringCompare 1277 869 -31.9% 1.47x
ObjectiveCBridgeStringCompare2 1245 850 -31.7% 1.46x
ObjectiveCBridgeStringRangeOfString 1221 862 -29.4% 1.42x
StringEqualPointerComparison 628 571 -9.1% 1.10x

Performance: -Onone

TEST OLD NEW DELTA RATIO
Regression
ArrayOfPOD 781 860 +10.1% 0.91x (?)
Improvement
ObjectiveCBridgeStringIsEqualAllSwift 986 63 -93.6% 15.65x
ObjectiveCBridgeStringUTF8String 358 25 -93.0% 14.32x
ObjectiveCBridgeStringIsEqual2 1140 205 -82.0% 5.56x
ObjectiveCBridgeStringHash 156 77 -50.6% 2.03x
ObjectiveCBridgeStringGetASCIIContents 636 317 -50.2% 2.01x
ObjectiveCBridgeStringGetUTF8Contents 513 259 -49.5% 1.98x
ObjectiveCBridgeStringIsEqual 310 177 -42.9% 1.75x
ObjectiveCBridgeStringCompare 1290 875 -32.2% 1.47x
ObjectiveCBridgeStringCompare2 1246 854 -31.5% 1.46x
ObjectiveCBridgeStringRangeOfString 1220 861 -29.4% 1.42x
PrefixArrayLazy 39251 29996 -23.6% 1.31x
DropLastArrayLazy 12946 10041 -22.4% 1.29x
SuffixArrayLazy 12780 9999 -21.8% 1.28x
DropFirstArrayLazy 38736 30543 -21.2% 1.27x
ArrayAppendLazyMap 190174 160030 -15.9% 1.19x
MapReduceLazyCollection 24792 21141 -14.7% 1.17x
FatCompactMap 320767 279009 -13.0% 1.15x
MapReduceLazyCollectionShort 36076 32468 -10.0% 1.11x
How to read the data The tables contain differences in performance which are larger than 8% and differences in code size which are larger than 1%.

If you see any unexpected regressions, you should consider fixing the
regressions before you merge the PR.

Noise: Sometimes the performance results (not code size!) contain false
alarms. Unexpected regressions which are marked with '(?)' are probably noise.
If you see regressions which you cannot explain you can try to run the
benchmarks again. If regressions still show up, please consult with the
performance team (@eeckstein).

Hardware Overview 
  Model Name: Mac Pro
  Model Identifier: MacPro6,1
  Processor Name: 12-Core Intel Xeon E5
  Processor Speed: 2.7 GHz
  Number of Processors: 1
  Total Number of Cores: 12
  L2 Cache (per Core): 256 KB
  L3 Cache: 30 MB
  Memory: 64 GB
--------------
@Catfish-Man
Copy link
Contributor Author

Catfish-Man commented Nov 8, 2018

Heeeey it worked! I was really expecting that to be a huge regression. Awesome. Ok, gonna get this ready to go. Also, sadly, I have to delete the -UTF8String optimization. I realized that Cocoa jumps through hoops to return non-NULL there, so we need to fall back to the superclass, not return nil, and I haven't figured out how to do that yet.
@jrose-apple
Copy link
Contributor

jrose-apple commented Nov 8, 2018

Also, sadly, I have to delete the -UTF8String optimization. I realized that Cocoa jumps through hoops to return non-NULL there, so we need to fall back to the superclass, not return nil, and I haven't figured out how to do that yet.

_swift_stdlib_getAutoreleasedUTF8String(id)? (But, okay, separate PR.)
@Catfish-Man
Copy link
Contributor Author

Catfish-Man commented Nov 8, 2018

@swift-ci please test and merge
1 similar comment
@Catfish-Man
Copy link
Contributor Author

Catfish-Man commented Nov 8, 2018

@swift-ci please test and merge
@Catfish-Man
Copy link
Contributor Author

Catfish-Man commented Nov 8, 2018

Sigh… now to figure out why that isn't failing locally
@milseman
Copy link
Member

milseman commented Nov 8, 2018

That's a 32-bit build. Try with -i
@Catfish-Man
Copy link
Contributor Author

Catfish-Man commented Nov 8, 2018

Ok yeah, 32 bit failure it is. Fixing this is going to be a lot more challenging sadly, so probably tomorrow.
@Catfish-Man
Copy link
Contributor Author

Catfish-Man commented Nov 8, 2018

@swift-ci please test and merge
5 similar comments
@Catfish-Man
Copy link
Contributor Author

Catfish-Man commented Nov 8, 2018

@swift-ci please test and merge
@Catfish-Man
Copy link
Contributor Author

Catfish-Man commented Nov 8, 2018

@swift-ci please test and merge
@Catfish-Man
Copy link
Contributor Author

Catfish-Man commented Nov 8, 2018

@swift-ci please test and merge
@Catfish-Man
Copy link
Contributor Author

Catfish-Man commented Nov 8, 2018

@swift-ci please test and merge
@Catfish-Man
Copy link
Contributor Author

Catfish-Man commented Nov 8, 2018

@swift-ci please test and merge
@Catfish-Man
Copy link
Contributor Author

Catfish-Man commented Nov 8, 2018

Oh I forgot to pull on the machine I was building on, that's why my results don't match this. About that "going to sleep and doing this tomorrow"…
@milseman
Copy link
Member

milseman commented Nov 8, 2018

If you want to split off the dropping of fixedLayout, into a separate PR, that's fine with me.
@Catfish-Man
Copy link
Contributor Author

Catfish-Man commented Nov 8, 2018

@swift-ci please test and merge
1 similar comment
@Catfish-Man
Copy link
Contributor Author

Catfish-Man commented Nov 8, 2018

@swift-ci please test and merge
@swift-ci swift-ci merged commit 7a68a69 into swiftlang:master Nov 8, 2018
@Catfish-Man Catfish-Man deleted the stringbridgingspeedups branch November 8, 2018 22:28
@xwu xwu mentioned this pull request Dec 2, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

6 participants

@Catfish-Man @gottesmm @swift-ci @milseman @jrose-apple @airspeedswift