Skip to content

Extend the existing memoization of NSString's Hashable conformance to cover AnyObjects that are actually NSStrings as well #82440

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
merged 4 commits into from
Jun 24, 2025

Conversation

Catfish-Man
Copy link
Contributor

Fixes rdar://154123172 (Consider memoizing the lookup for NSString's conformance to Hashable)

… cover AnyObjects that are actually NSStrings as well
@Catfish-Man Catfish-Man requested a review from tbkka June 24, 2025 00:21
@Catfish-Man Catfish-Man self-assigned this Jun 24, 2025
@Catfish-Man
Copy link
Contributor Author

@swift-ci please test

@Catfish-Man
Copy link
Contributor Author

@swift-ci please Apple Silicon benchmark

@Catfish-Man
Copy link
Contributor Author

@swift-ci please benchmark

@Catfish-Man
Copy link
Contributor Author

@swift-ci please test

@Catfish-Man
Copy link
Contributor Author

@swift-ci please benchmark

@Catfish-Man
Copy link
Contributor Author

@swift-ci please benchmark

@Catfish-Man
Copy link
Contributor Author

@swift-ci please 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.

This looks entirely reasonable now. Does this show the performance shift you were looking for?

@Catfish-Man
Copy link
Contributor Author

I've verified in lldb that we no longer hit swift_conformsToProtocolCommon at all when bridging an NSDictionary of NSStrings, I'm going to see what the full benchmark suite run thinks about the impact of that

@Catfish-Man
Copy link
Contributor Author

@swift-ci please Apple Silicon benchmark

@Catfish-Man
Copy link
Contributor Author

Turns out we don't have benchmark coverage for AnyHashable! A local test shows ~18.6% overhead from the conformance lookups

@Catfish-Man
Copy link
Contributor Author

Test failure is clang crashing while trying to compile LLVM, looks like

@Catfish-Man
Copy link
Contributor Author

Verified with a release build locally: 4s -> 3s for 10M iterations

@Catfish-Man Catfish-Man enabled auto-merge (squash) June 24, 2025 03:07
@Catfish-Man
Copy link
Contributor Author

@swift-ci please test

@Catfish-Man Catfish-Man merged commit 08233af into main Jun 24, 2025
6 of 7 checks passed
@Catfish-Man Catfish-Man deleted the memoize-harder branch June 24, 2025 15:21
@swiftlang swiftlang deleted a comment from SaaSCh Jun 24, 2025
@swiftlang swiftlang deleted a comment from SaaSCh Jun 24, 2025
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