Skip to content

Adjust String bridging machinery to reduce overhead #60235

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
Closed
wants to merge 4 commits into from
Closed

Adjust String bridging machinery to reduce overhead #60235

wants to merge 4 commits into from

Conversation

@Catfish-Man
Copy link
Contributor

No description provided.

@Catfish-Man Catfish-Man self-assigned this Jul 26, 2022
@Catfish-Man
Copy link
Contributor Author

@swift-ci please test

@Catfish-Man
Copy link
Contributor Author

@swift-ci please benchmark

Catfish-Man
Catfish-Man commented Jul 26, 2022
View reviewed changes
stdlib/public/stubs/FoundationHelpers.mm Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This could slightly slow down back deployed apps, since they may get NULL for this

stdlib/public/stubs/FoundationHelpers.mm Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using this instead of _CFGetTypeID + _CFStringTypeID lets us keep our dirty data footprint even despite looking up _CFStringCreateTaggedPointerString

stdlib/public/core/StringBridge.swift Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

1.33x speedup on my machine

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apparently 1.76x on x86, nice!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Up to 1.90x on one benchmark after the most recent improvement :)

@Catfish-Man
Copy link
Contributor Author

Benchmark results mostly look good but I should double check what's going on with NSStringConversion.MutableCopy.*

@Catfish-Man
Copy link
Contributor Author

@swift-ci please benchmark

@Catfish-Man
Copy link
Contributor Author

@swift-ci please test

@Catfish-Man Catfish-Man marked this pull request as ready for review July 26, 2022 22:19
@Catfish-Man
Copy link
Contributor Author

@swift-ci please smoke test

@Catfish-Man
Copy link
Contributor Author

@swift-ci please benchmark

@Catfish-Man
Copy link
Contributor Author

LLDB failure looks unrelated to me, asking about it

@Catfish-Man
Copy link
Contributor Author

Might not be unrelated, interesting

@Catfish-Man
Copy link
Contributor Author

@swift-ci please benchmark

@Catfish-Man
Copy link
Contributor Author

@swift-ci Please test stdlib with toolchain

@phausler
Copy link
Contributor

So I think I follow what is going on here; overall it seems ok, there are some open questions per maintenance and the commented out code but all in all I think I understand the perf side of it.

phausler
phausler approved these changes Jul 29, 2022
View reviewed changes
Copy link
Contributor

@phausler phausler left a comment

Choose a reason for hiding this comment

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

The alterations don't have anything immediate that are leaping out to me. The one quibble (granted I understand the reason why to keep sensitive code for refactoring in...) is that commented out code is immediately out of date; no one ever expects to maintain that.

@Catfish-Man
Copy link
Contributor Author

I think I figured out the failure. It's not actually caused by this if I'm right; going to talk to the lldb folks about what to do.

@Catfish-Man
Copy link
Contributor Author

@swift-ci please test

@Catfish-Man
Copy link
Contributor Author

@swift-ci please smoke benchmark

@Catfish-Man
Copy link
Contributor Author

Catfish-Man commented Jan 31, 2023 •
edited
Loading

Benchmark results still look good after 6 months, let's see if the test situation we were waiting on has changed…

------- Performance (x86_64): -Osize -------

REGRESSION                                       OLD        NEW        DELTA    RATIO    
NSStringConversion.MutableCopy.Rebridge.Medium   372.333    449.333    +20.7%   **0.83x (?)**
NSStringConversion.MutableCopy.Medium            469.75     548.0      +16.7%   **0.86x (?)**
ObjectiveCBridgeStringIsEqualAllSwift            48.919     53.028     +8.4%    **0.92x (?)**

IMPROVEMENT                                      OLD        NEW        DELTA    RATIO    
ObjectiveCBridgeToNSString                       265.143    138.615    -47.7%   **1.91x**
ObjectiveCBridgeStubToArrayOfNSString2           2553.333   1901.25    -25.5%   **1.34x**
ObjectiveCBridgeStubToNSString                   1691.818   1293.846   -23.5%   **1.31x (?)**
EqualStringSubstring                             31.033     23.733     -23.5%   **1.31x**
EqualSubstringSubstring                          30.933     23.75      -23.2%   **1.30x (?)**
ObjectiveCBridgeToNSArray                        6976.923   5363.333   -23.1%   **1.30x (?)**
EqualSubstringSubstringGenericEquatable          31.027     24.03      -22.6%   **1.29x (?)**
LessSubstringSubstring                           30.895     23.977     -22.4%   **1.29x (?)**
EqualSubstringString                             30.92      24.065     -22.2%   **1.28x (?)**
LessSubstringSubstringGenericComparable          30.914     24.063     -22.2%   **1.28x (?)**
ObjectiveCBridgeToNSSet                          9083.333   7685.0     -15.4%   **1.18x (?)**
StringComparison_longSharedPrefix                242.1      209.4      -13.5%   **1.16x (?)**
SevenBoom                                        535.5      478.0      -10.7%   **1.12x (?)**
NSError                                          127.083    115.0      -9.5%    **1.11x (?)**
NSStringConversion.UTF8                          627.333    580.75     -7.4%    **1.08x (?)**
Chars2                                           3444.286   3212.821   -6.7%    **1.07x (?)**

@Catfish-Man
Copy link
Contributor Author

@swift-ci please test

2 similar comments
@Catfish-Man
Copy link
Contributor Author

@swift-ci please test

@Catfish-Man
Copy link
Contributor Author

@swift-ci please test

@Catfish-Man
Copy link
Contributor Author

This all got fixed up later in other ways

@Catfish-Man Catfish-Man deleted the dlslim branch May 30, 2025 22:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cooperp cooperp cooperp left review comments

@phausler phausler phausler approved these changes

Assignees

@Catfish-Man Catfish-Man

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Catfish-Man @phausler @cooperp