Skip to content

Avoid protocol conformance lookups in more NSString bridging scenarios #39378

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

Merged

Conversation

Catfish-Man
Copy link
Contributor

No description provided.

@Catfish-Man Catfish-Man requested a review from tbkka September 21, 2021 00:58
@Catfish-Man Catfish-Man self-assigned this Sep 21, 2021
@Catfish-Man
Copy link
Contributor Author

@swift-ci please test

@Catfish-Man
Copy link
Contributor Author

@swift-ci please benchmark

Copy link
Contributor

@tbkka tbkka left a comment

Choose a reason for hiding this comment

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

Few suggestions for you.

@@ -752,6 +752,12 @@ struct ObjCBridgeMemo {
};
#endif

const Metadata *getNSStringMetadata() {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should go in stdlib/public/runtime/SwiftObject.mm with the other getNSXyzMetadata() methods.

Copy link
Contributor

Choose a reason for hiding this comment

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

In fact, I would suggest also putting a getNSStringHashableConformance() there as well.

@@ -764,14 +770,18 @@ tryCastToAnyHashable(
assert(cast<StructMetadata>(destType)->Description
== &STRUCT_TYPE_DESCR_SYM(s11AnyHashable));

bool useCachedNSStringConformance = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not a fan of setting a boolean to test later on in the same code.
I suggest refactoring this with

  const HashableWitnessTable *hashableConformance = nullptr;

here, then set it along different paths.

// Until this is implemented, fall through to the general case
auto cls = srcType;
do {
if (cls == getNSStringMetadata()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

 auto nsString = getNSStringMetadata();
 do {
    if (cls == nsString) {
        hashableConformance = getNSStringHashableConformance();
        break;
    }
    cls = _swift_class_getSuperclass(cls);
  } while (cls != nullptr);
  break;

This hoists the getNSStringMetadata() call out of the loop and does the hashable conformance fetch directly instead of setting a boolean to test later.

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 05d45fac75b066bf87896fd8ef0ab5b6ead84dda

@swift-ci
Copy link
Contributor

Performance (x86_64): -O

Regression OLD NEW DELTA RATIO
UTF8Decode_InitFromCustom_contiguous_ascii_as_ascii 356 393 +10.4% 0.91x (?)
UTF8Decode_InitFromCustom_noncontiguous_ascii_as_ascii 1066 1155 +8.3% 0.92x (?)
FlattenListFlatMap 3912 4219 +7.8% 0.93x (?)
 
Improvement OLD NEW DELTA RATIO
DictionaryOfAnyHashableStrings_lookup 4248 3240 -23.7% 1.31x
DictionaryBridgeToObjC_Access 1085 894 -17.6% 1.21x (?)
ObjectiveCBridgeStubFromNSDateRef 5060 4210 -16.8% 1.20x (?)
DictionaryOfAnyHashableStrings_insert 5642 4774 -15.4% 1.18x (?)
String.data.Medium 104 97 -6.7% 1.07x (?)
RandomShuffleLCG2 480 448 -6.7% 1.07x (?)
Array2D 7216 6736 -6.7% 1.07x (?)

Code size: -O

Performance (x86_64): -Osize

Regression OLD NEW DELTA RATIO
DataAppendSequence 9600 10500 +9.4% 0.91x (?)
Data.append.Sequence.809B.Count.RE 97 106 +9.3% 0.92x (?)
 
Improvement OLD NEW DELTA RATIO
DictionaryOfAnyHashableStrings_lookup 4248 3216 -24.3% 1.32x
DictionaryOfAnyHashableStrings_insert 6272 5376 -14.3% 1.17x (?)
Array2D 7520 6992 -7.0% 1.08x (?)

Code size: -Osize

Performance (x86_64): -Onone

Regression OLD NEW DELTA RATIO
DictionaryRemove 19520 27240 +39.5% 0.72x (?)
DictionarySubscriptDefaultMutation 4160 4769 +14.6% 0.87x (?)
ArrayAppendLazyMap 303660 340500 +12.1% 0.89x (?)
DictionarySubscriptDefaultMutationArray 4524 5016 +10.9% 0.90x (?)
SevenBoom 1357 1460 +7.6% 0.93x (?)
 
Improvement OLD NEW DELTA RATIO
DictionaryOfAnyHashableStrings_lookup 5136 3792 -26.2% 1.35x
DictionaryOfAnyHashableStrings_insert 6944 6062 -12.7% 1.15x (?)
String.replaceSubrange.String 27 24 -11.1% 1.12x (?)

Code size: -swiftlibs

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

@swift-ci please test

@Catfish-Man
Copy link
Contributor Author

@swift-ci please benchmark

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 452da19ddb080a6b161d7736771aeaa479d47f94

@swift-ci
Copy link
Contributor

Build failed before running benchmark.

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 452da19ddb080a6b161d7736771aeaa479d47f94

@Catfish-Man
Copy link
Contributor Author

@swift-ci please smoke test

@Catfish-Man
Copy link
Contributor Author

@swift-ci please smoke benchmark

@Catfish-Man Catfish-Man changed the title Optimize NSString->AnyHashable and String->AnyHashable Avoid protocol conformance lookups in more NSString bridging scenarios Sep 21, 2021
@swift-ci
Copy link
Contributor

Build failed before running benchmark.

@Catfish-Man
Copy link
Contributor Author

@swift-ci please smoke benchmark

@Catfish-Man
Copy link
Contributor Author

@swift-ci please smoke test

@swift-ci
Copy link
Contributor

Performance (x86_64): -O

Regression OLD NEW DELTA RATIO
StringFromLongWholeSubstring 4 5 +25.0% 0.80x
FlattenListFlatMap 6243 7131 +14.2% 0.88x (?)
DictionaryBridgeToObjC_Access 955 1056 +10.6% 0.90x (?)
UTF8Decode_InitFromCustom_contiguous_ascii_as_ascii 361 398 +10.2% 0.91x (?)
LessSubstringSubstring 39 42 +7.7% 0.93x (?)
EqualStringSubstring 39 42 +7.7% 0.93x (?)
EqualSubstringSubstringGenericEquatable 39 42 +7.7% 0.93x
EqualSubstringString 39 42 +7.7% 0.93x (?)
LessSubstringSubstringGenericComparable 39 42 +7.7% 0.93x
SortStringsUnicode 2890 3110 +7.6% 0.93x
 
Improvement OLD NEW DELTA RATIO
Data.init.Sequence.64kB.Count.RE.I 4 3 -25.0% 1.33x (?)
DictionaryOfAnyHashableStrings_lookup 4248 3216 -24.3% 1.32x
DictionaryOfAnyHashableStrings_insert 5642 4746 -15.9% 1.19x (?)
ObjectiveCBridgeStubFromArrayOfNSString2 2920 2700 -7.5% 1.08x (?)
UTF8Decode_InitFromBytes_ascii_as_ascii 563 522 -7.3% 1.08x (?)
Data.hash.Medium 42 39 -7.1% 1.08x (?)

Code size: -O

Performance (x86_64): -Osize

Regression OLD NEW DELTA RATIO
FlattenListLoop 1630 2542 +56.0% 0.64x (?)
StringFromLongWholeSubstring 4 5 +25.0% 0.80x
DictionaryKeysContainsNative 27 30 +11.1% 0.90x (?)
LessSubstringSubstring 39 43 +10.3% 0.91x (?)
EqualSubstringSubstringGenericEquatable 39 43 +10.3% 0.91x (?)
EqualSubstringSubstring 39 42 +7.7% 0.93x (?)
EqualStringSubstring 39 42 +7.7% 0.93x (?)
EqualSubstringString 39 42 +7.7% 0.93x (?)
LessSubstringSubstringGenericComparable 39 42 +7.7% 0.93x (?)
 
Improvement OLD NEW DELTA RATIO
DictionaryOfAnyHashableStrings_lookup 4248 3216 -24.3% 1.32x
FlattenListFlatMap 5301 4156 -21.6% 1.28x (?)
ArrayAppendGenericStructs 2480 2070 -16.5% 1.20x (?)
DictionaryOfAnyHashableStrings_insert 6286 5390 -14.3% 1.17x (?)
ParseFloat.Float.Exp 14 13 -7.1% 1.08x (?)

Code size: -Osize

Performance (x86_64): -Onone

Regression OLD NEW DELTA RATIO
DictionaryRemove 19550 27230 +39.3% 0.72x (?)
RemoveWhereFilterInts 2799 3195 +14.1% 0.88x (?)
RemoveWhereFilterStrings 3401 3785 +11.3% 0.90x (?)
UTF8Decode_InitFromCustom_contiguous_ascii 323 354 +9.6% 0.91x (?)
DataToStringMedium 6350 6950 +9.4% 0.91x (?)
 
Improvement OLD NEW DELTA RATIO
DictionaryOfAnyHashableStrings_lookup 5064 4152 -18.0% 1.22x (?)
Breadcrumbs.MutatedUTF16ToIdx.ASCII 8 7 -12.5% 1.14x (?)
MapReduceLazyCollectionShort 44701 39265 -12.2% 1.14x (?)
DictionaryOfAnyHashableStrings_insert 6972 6132 -12.0% 1.14x (?)
String.replaceSubrange.String 27 24 -11.1% 1.12x (?)
ArrayAppendLazyMap 305250 271590 -11.0% 1.12x (?)
MapReduceLazyCollection 39129 35067 -10.4% 1.12x (?)
StringBuilder 2792 2597 -7.0% 1.08x (?)

Code size: -swiftlibs

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

Huh. Interesting. I wonder why we're not seeing more of a win from the second change.

@Catfish-Man
Copy link
Contributor Author

Oh, it being incorrect probably has something to do with that, heh

@Catfish-Man
Copy link
Contributor Author

@swift-ci please smoke test

@Catfish-Man Catfish-Man force-pushed the any-hashable-in-this-economy branch from 3c8ab8a to 98796f1 Compare December 6, 2021 23:23
@Catfish-Man
Copy link
Contributor Author

@swift-ci please benchmark

@Catfish-Man
Copy link
Contributor Author

@swift-ci please smoke test

@swift-ci
Copy link
Contributor

swift-ci commented Dec 7, 2021

Performance (x86_64): -O

Regression OLD NEW DELTA RATIO
ClassArrayGetter2 50 60 +20.0% 0.83x (?)
Breadcrumbs.MutatedIdxToUTF16.Mixed 196 226 +15.3% 0.87x (?)
Breadcrumbs.MutatedUTF16ToIdx.Mixed 194 222 +14.4% 0.87x (?)
StringBuilderSmallReservingCapacity 190 214 +12.6% 0.89x (?)
StringHasSuffixAscii 1480 1630 +10.1% 0.91x (?)
StringBuilder 184 202 +9.8% 0.91x (?)
StringAdder 249 271 +8.8% 0.92x (?)
Set.isStrictSubset.Int.Empty 38 41 +7.9% 0.93x (?)
 
Improvement OLD NEW DELTA RATIO
LessSubstringSubstring 29 22 -24.1% 1.32x
EqualSubstringSubstring 29 22 -24.1% 1.32x
EqualStringSubstring 29 22 -24.1% 1.32x (?)
EqualSubstringString 29 22 -24.1% 1.32x
LessSubstringSubstringGenericComparable 29 22 -24.1% 1.32x
DictionaryOfAnyHashableStrings_lookup 2880 2208 -23.3% 1.30x (?)
EqualSubstringSubstringGenericEquatable 29 23 -20.7% 1.26x
DictionaryOfAnyHashableStrings_insert 3892 3332 -14.4% 1.17x (?)
StringComparison_longSharedPrefix 333 295 -11.4% 1.13x (?)
RandomShuffleLCG2 320 288 -10.0% 1.11x (?)
FlattenListLoop 1031 934 -9.4% 1.10x (?)
Calculator 160 146 -8.7% 1.10x (?)
RemoveWhereMoveInts 23 21 -8.7% 1.10x (?)
SortStringsUnicode 2265 2080 -8.2% 1.09x (?)
RandomDoubleOpaqueLCG 423 392 -7.3% 1.08x (?)
PrefixWhileAnySequence 202 188 -6.9% 1.07x (?)
SequenceAlgosContiguousArray 610 570 -6.6% 1.07x (?)

Code size: -O

Performance (x86_64): -Osize

Regression OLD NEW DELTA RATIO
Breadcrumbs.MutatedIdxToUTF16.Mixed 197 225 +14.2% 0.88x (?)
Breadcrumbs.MutatedUTF16ToIdx.Mixed 195 222 +13.8% 0.88x (?)
Dictionary4OfObjects 290 321 +10.7% 0.90x (?)
Array2D 4256 4688 +10.2% 0.91x (?)
UTF8Decode_InitDecoding 126 136 +7.9% 0.93x (?)
 
Improvement OLD NEW DELTA RATIO
EqualStringSubstring 30 22 -26.7% 1.36x
EqualSubstringSubstringGenericEquatable 30 22 -26.7% 1.36x
FlattenListFlatMap 3516 2650 -24.6% 1.33x (?)
LessSubstringSubstring 29 22 -24.1% 1.32x
EqualSubstringString 29 22 -24.1% 1.32x
LessSubstringSubstringGenericComparable 29 22 -24.1% 1.32x
DictionaryOfAnyHashableStrings_lookup 2856 2208 -22.7% 1.29x
EqualSubstringSubstring 29 23 -20.7% 1.26x
DictionaryOfAnyHashableStrings_insert 4382 3780 -13.7% 1.16x (?)
StringComparison_longSharedPrefix 332 294 -11.4% 1.13x (?)
SortStringsUnicode 2275 2065 -9.2% 1.10x (?)

Code size: -Osize

Performance (x86_64): -Onone

Regression OLD NEW DELTA RATIO
ArrayAppendGenericStructs 600 1530 +155.0% 0.39x (?)
Breadcrumbs.MutatedUTF16ToIdx.Mixed 198 226 +14.1% 0.88x (?)
Breadcrumbs.MutatedIdxToUTF16.Mixed 207 234 +13.0% 0.88x (?)
SIMDReduce.Int8 6177 6764 +9.5% 0.91x (?)
DataReplaceLargeBuffer 11 12 +9.1% 0.92x (?)
UTF8Decode_InitDecoding 132 143 +8.3% 0.92x (?)
StringToDataLargeUnicode 4250 4600 +8.2% 0.92x (?)
UTF8Decode_InitFromCustom_contiguous 135 146 +8.1% 0.92x (?)
 
Improvement OLD NEW DELTA RATIO
DictionaryOfAnyHashableStrings_lookup 3288 2616 -20.4% 1.26x (?)
ArrayOfGenericPOD2 1055 886 -16.0% 1.19x (?)
MapReduceLazySequence 18838 16106 -14.5% 1.17x (?)
LessSubstringSubstringGenericComparable 57 49 -14.0% 1.16x (?)
LessSubstringSubstring 59 52 -11.9% 1.13x (?)
DictionaryOfAnyHashableStrings_insert 4662 4116 -11.7% 1.13x (?)

Code size: -swiftlibs

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 mini
  Model Identifier: Macmini8,1
  Processor Name: 6-Core Intel Core i7
  Processor Speed: 3.2 GHz
  Number of Processors: 1
  Total Number of Cores: 6
  L2 Cache (per Core): 256 KB
  L3 Cache: 12 MB
  Memory: 64 GB

@Catfish-Man
Copy link
Contributor Author

Now it has wins but I don't believe them. That benchmark is finicky and shouldn't touch this code.

@Catfish-Man Catfish-Man marked this pull request as ready for review December 7, 2021 23:19
@Catfish-Man Catfish-Man force-pushed the any-hashable-in-this-economy branch from 98796f1 to ab78777 Compare January 7, 2022 23:50
@Catfish-Man
Copy link
Contributor Author

Decided to bail on the more complex and less obviously good part of this change and just focus on the bit we're pretty sure is solid

@Catfish-Man
Copy link
Contributor Author

@swift-ci please smoke test

@Catfish-Man
Copy link
Contributor Author

@swift-ci please smoke benchmark

@swift-ci
Copy link
Contributor

swift-ci commented Jan 8, 2022

Performance (x86_64): -O

Regression OLD NEW DELTA RATIO
FlattenListFlatMap 4177 6086 +45.7% 0.69x (?)
ObjectiveCBridgeStubFromNSDateRef 4210 5030 +19.5% 0.84x (?)
String.replaceSubrange.RepChar.Small 478 515 +7.7% 0.93x (?)
 
Improvement OLD NEW DELTA RATIO
DictionaryOfAnyHashableStrings_lookup 4728 3432 -27.4% 1.38x
DictionaryOfAnyHashableStrings_insert 6048 5110 -15.5% 1.18x
Array2D 7504 6912 -7.9% 1.09x (?)
InsertCharacterEndIndex 184 170 -7.6% 1.08x (?)
Set.subtracting.Int.Empty 40 37 -7.5% 1.08x
RandomTree.insert.Unmanaged.fast 213 198 -7.0% 1.08x (?)
RandomShuffleLCG2 480 448 -6.7% 1.07x
String.replaceSubrange.ArrChar 61 57 -6.6% 1.07x (?)

Code size: -O

Performance (x86_64): -Osize

Regression OLD NEW DELTA RATIO
FlattenListFlatMap 4296 6605 +53.7% 0.65x (?)
DictionaryKeysContainsNative 25 29 +16.0% 0.86x (?)
String.replaceSubrange.RepChar.Small 476 523 +9.9% 0.91x (?)
 
Improvement OLD NEW DELTA RATIO
DictionaryOfAnyHashableStrings_lookup 4656 3432 -26.3% 1.36x
DictionaryOfAnyHashableStrings_insert 6748 5768 -14.5% 1.17x (?)
StringBuilderWithLongSubstring 1630 1480 -9.2% 1.10x (?)
RemoveWhereMoveInts 37 34 -8.1% 1.09x (?)
Set.subtracting.Seq.Empty.Int 226 208 -8.0% 1.09x (?)
DictionaryBridgeToObjC_BulkAccess 161 149 -7.5% 1.08x (?)
Array2D 7504 6960 -7.2% 1.08x (?)
String.replaceSubrange.ArrChar 61 57 -6.6% 1.07x (?)

Code size: -Osize

Performance (x86_64): -Onone

Regression OLD NEW DELTA RATIO
RandomDoubleLCG 44035 49793 +13.1% 0.88x (?)
RandomDoubleOpaqueLCG 44419 49984 +12.5% 0.89x (?)
StringWordBuilder 2610 2920 +11.9% 0.89x
StringWordBuilderReservingCapacity 2570 2850 +10.9% 0.90x (?)
RandomDoubleDef 51600 57000 +10.5% 0.91x (?)
CStringLongAscii 326 358 +9.8% 0.91x (?)
RandomDoubleOpaqueDef 52000 56900 +9.4% 0.91x (?)
String.replaceSubrange.RepChar.Small 493 536 +8.7% 0.92x (?)
 
Improvement OLD NEW DELTA RATIO
DictionaryOfAnyHashableStrings_lookup 5400 4080 -24.4% 1.32x (?)
DictionaryOfAnyHashableStrings_insert 7336 6300 -14.1% 1.16x (?)
String.replaceSubrange.String 27 24 -11.1% 1.12x (?)
ObjectiveCBridgeFromNSDictionaryAnyObjectForced 13300 12250 -7.9% 1.09x (?)

Code size: -swiftlibs

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 Catfish-Man force-pushed the any-hashable-in-this-economy branch from ab78777 to 12166a9 Compare January 8, 2022 10:34
@Catfish-Man
Copy link
Contributor Author

@swift-ci please smoke test

Copy link
Contributor

@tbkka tbkka left a comment

Choose a reason for hiding this comment

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

Looks good!

@Catfish-Man Catfish-Man merged commit 3d03925 into swiftlang:main Jan 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants