-
Notifications
You must be signed in to change notification settings - Fork 10.5k
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
Avoid protocol conformance lookups in more NSString bridging scenarios #39378
Conversation
@swift-ci please test |
@swift-ci please benchmark |
There was a problem hiding this 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() { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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()) { |
There was a problem hiding this comment.
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.
Build failed |
Performance (x86_64): -O
Code size: -OPerformance (x86_64): -Osize
Code size: -OsizePerformance (x86_64): -Onone
Code size: -swiftlibsHow to read the dataThe 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 Noise: Sometimes the performance results (not code size!) contain false Hardware Overview
|
@swift-ci please test |
@swift-ci please benchmark |
Build failed |
Build failed before running benchmark. |
Build failed |
@swift-ci please smoke test |
@swift-ci please smoke benchmark |
Build failed before running benchmark. |
@swift-ci please smoke benchmark |
@swift-ci please smoke test |
Performance (x86_64): -O
Code size: -OPerformance (x86_64): -Osize
Code size: -OsizePerformance (x86_64): -Onone
Code size: -swiftlibsHow to read the dataThe 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 Noise: Sometimes the performance results (not code size!) contain false Hardware Overview
|
Huh. Interesting. I wonder why we're not seeing more of a win from the second change. |
Oh, it being incorrect probably has something to do with that, heh |
@swift-ci please smoke test |
3c8ab8a
to
98796f1
Compare
@swift-ci please benchmark |
@swift-ci please smoke test |
Performance (x86_64): -O
Code size: -OPerformance (x86_64): -Osize
Code size: -OsizePerformance (x86_64): -Onone
Code size: -swiftlibsHow to read the dataThe 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 Noise: Sometimes the performance results (not code size!) contain false Hardware Overview
|
Now it has wins but I don't believe them. That benchmark is finicky and shouldn't touch this code. |
98796f1
to
ab78777
Compare
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 |
@swift-ci please smoke test |
@swift-ci please smoke benchmark |
Performance (x86_64): -O
Code size: -OPerformance (x86_64): -Osize
Code size: -OsizePerformance (x86_64): -Onone
Code size: -swiftlibsHow to read the dataThe 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 Noise: Sometimes the performance results (not code size!) contain false Hardware Overview
|
ab78777
to
12166a9
Compare
@swift-ci please smoke test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
No description provided.