-
Notifications
You must be signed in to change notification settings - Fork 10.6k
Re-shim tagged NSString creation #26588
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
Conversation
|
@swift-ci please smoke benchmark |
|
This will almost certainly be a (hopefully smallish) perf regression. I have a plan for fixing it once other pieces are in place. |
Performance: -O
Code size: -OPerformance: -Osize
Code size: -OsizePerformance: -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 |
|
Nooooooo :( :( |
|
@swift-ci please smoke benchmark |
Performance: -O
Code size: -OPerformance: -Osize
Code size: -OsizePerformance: -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 |
|
ok I profiled this and apparently the protocol ends up getting boxed or something wacky, so the work being done is totally unrelated to the objc overhead. That's… somewhat promising I guess. Means I don't have to throw out the whole approach. |
|
@swift-ci please smoke test |
1 similar comment
|
@swift-ci please smoke test |
|
@swift-ci please smoke benchmark |
Performance: -O
Code size: -OPerformance: -Osize
Code size: -OsizePerformance: -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 |
|
BOOM |
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 selector has to start with "new" to get swiftc to not autorelease the return value
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.
Are the trailing underscores intended to convey privateness without screwing up the new prefix?
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.
yup
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.
Casting directly to AnyObject here causes swiftc to emit a bridging call to bridge the optional out to objc; using an objc type explicitly avoids that
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'd suggest translating these PR comments to source code comments to lessen the pain of future generations. This all seems like valuable info.
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.
Unclear what exactly I'm claiming is not being released here, but it does cause something to not be retain-released
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.
Using '__StringStorage.self as AnyClass' instead of my original '_StringSelectorHolder.self as AnyObject' avoids a bunch of machinery that appeared to be boxing something.
Unfortunately it still emits a metadata accessor call that costs about 2/3 as much as objc_msgSend, but I haven't found a good way to avoid that yet.
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.
When we switch to _CFStringCreateTaggedPointerString this will start returning nil more
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'd suggest translating these PR comments to source code comments to lessen the pain of future generations. This all seems like valuable info.
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.
Are the trailing underscores intended to convey privateness without screwing up the new prefix?
0c5d8f4 to
f559a94
Compare
|
@swift-ci please smoke test and merge |
1 similar comment
|
@swift-ci please smoke test and merge |
|
Ugh, it claims it failed but I can't find the failure. Gonna re-run it |
|
@swift-ci please smoke test |
No description provided.