Skip to content
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

[SR-6197] Swift subclass of NSTextStorage is slow because of Swift bridging #48749

Open
swift-ci opened this issue Oct 21, 2017 · 12 comments
Open

[SR-6197] Swift subclass of NSTextStorage is slow because of Swift bridging #48749

swift-ci opened this issue Oct 21, 2017 · 12 comments

Comments

@swift-ci
Copy link
Collaborator

@swift-ci swift-ci commented Oct 21, 2017

Previous ID SR-6197
Radar None
Original Reporter fumoboy007 (JIRA User)
Type Bug

Attachment: Download

Environment

Apple Swift version 4.0 (swiftlang-900.0.65.2 clang-900.0.37)

Target: x86_64-apple-macosx10.9

macOS 10.13

Additional Detail from JIRA
Votes 2
Component/s Compiler, Standard Library
Labels Bug
Assignee None
Priority Medium

md5: d13ea9787b0d44984c429de46b2d6547

Issue Description:

`NSTextStorage` vs. `TextStorageSwiftSubclass`

Calling `-[NSTextStorage attributesAtIndex:effectiveRange:]` is nearly 3 times as slow for `TextStorageSwiftSubclass` compared to `NSTextStorage`. Time profiling shows that this is caused by the bridging of the Objective-C dictionary return value to Swift. Effectively, this means we cannot use Swift if we want to subclass `NSTextStorage`.

Sample project and time profiler trace are attached.

@belkadan
Copy link
Contributor

@belkadan belkadan commented Oct 23, 2017

@rjmccall
Copy link
Member

@rjmccall rjmccall commented Oct 24, 2017

Hmm. The bridging peephole addresses some instances of this class of problem, but not ObjC subclassing or protocol implementation, and it won't be available until Swift 4.1 anyway. So this has to stay open for now.

@belkadan, we don't allow methods to be implemented/overridden with their original types, right?

@belkadan
Copy link
Contributor

@belkadan belkadan commented Oct 24, 2017

No, we don't. I suppose we could (by checking bridging conversions, not by checking the original type).

@belkadan
Copy link
Contributor

@belkadan belkadan commented Oct 24, 2017

That gets tricky for further Swift overrides, though.

@rjmccall
Copy link
Member

@rjmccall rjmccall commented Oct 24, 2017

I think it would basically require Swift overrides to stick with the original types, which is an unfortunate leak of what's really an implementation detail.

I don't know how to do this without substantially changing the code-generation model for Swift overrides/implementations of ObjC methods, though.

@swift-ci
Copy link
Collaborator Author

@swift-ci swift-ci commented Feb 8, 2018

Comment by Ian McDowell (JIRA)

Hi guys. Any progress or ideas here?

I am also trying to implement a NSTextStorage subclass in an all-swift app, and seeing it taking around 1.5 seconds to update for a new page of text. The vast majority of that time is spent bridging to and from NSDictionary.

If I implement the following method in Objective-C, and the rest in a Swift subclass, the execution time for the same operation is under 200 milliseconds.

- (NSDictionary<NSAttributedStringKey,id> *)attributesAtIndex:(NSUInteger)location effectiveRange:(NSRangePointer)range;

@rjmccall
Copy link
Member

@rjmccall rjmccall commented Feb 26, 2018

We don't have a short-term solution for this, and unfortunately we're likely to stay busy on other things until at least after the fall release. I don't have a better suggestion for you than just implementing the one method in Objective-C.

@jpsim
Copy link
Contributor

@jpsim jpsim commented Nov 14, 2018

There’s a reference to this perhaps being possible to improve once Swift 4.1 arrived. Would now be a good time to revisit this?

@rjmccall
Copy link
Member

@rjmccall rjmccall commented Nov 14, 2018

The only reasonable solution to this that I can see is to allow Swift subclasses to define overrides using the original, unbridged types. Unfortunately, it's way too late to be adding significant new generalizations like that in 5.0 — it's very likely that we'll end up chasing the long tail of unanticipated interaction problems in the new feature.

@Catfish-Man
Copy link
Member

@Catfish-Man Catfish-Man commented Jan 5, 2019

I think this should be considerably better on current trunk. Not "different big O" better like would be needed to really fix it, but I expect a nice incremental improvement.

@bobergj
Copy link

@bobergj bobergj commented Jan 23, 2019

The only reasonable solution to this that I can see is to allow Swift subclasses to define overrides using the original, unbridged types.

Note that this is not just an issue with subclasses, but also delegate methods, such as NSLayoutManagerDelegate shouldUseTemporaryAttributes.

What about the following:

  • Add a new macro NS_SWIFT_UNBRIDGED and corresponding clang attribute that disables automatic bridging (NSString, NSDictionary etc) of an objc parameter or return type. Make the swift ClangImporter respect this attribute.

With "just" that I believe the issue can be worked around by developers, for the aforementioned NSTextStorage as follows:

Define a protocol and forwarding base class in objc:

@interface MyTextStorageImplProtocol
- (NSDictionary<NSAttributedStringKey,id> *)impl_attributesAtIndex:(NSUInteger)location effectiveRange:(NSRangePointer)range NS_SWIFT_UNBRIDGED;
@end

@interface MyTextStorageBase : NSTextStorage
@property (nonatomic, weak) id<MyTextStorageImplProtocol> myImpl;
- (NSDictionary<NSAttributedStringKey,id> *)attributesAtIndex:(NSUInteger)location effectiveRange:(NSRangePointer)range;  // @implementation not showed here calls corresponding myImpl method
@end

In swift:

class MyTextStorage: MyTextStorageBase <MyTextStorageImplProtocol> {
   
   init() {
       super.myImpl = self
   }

   func impl_attributes(at location: Int, effectiveRange range: NSRangePointer?) -> NSDictionary {
     // Do your thing
   } 
   ...
} 

How is this better than just defining the full subclass in Obiective-C? Well, assuming the rest of the app is written in Swift, your attributesAtIndex method can now access all your non-objc bridgeable Swift types.

While of course not ideal, this provides a escape hatch that is better than waiting for a possible fix in Swift 6.
Also NS_SWIFT_UNBRIDGED would be useful for annotating performance sensitive APIs in third party libraries. Or even in Apples TextKit! 🙂

Edit: I realised that a poor-mans version of NS_SWIFT_UNBRIDGED, called NS_NON_BRIDGED already exists in https://github.com/apple/swift/blob/master/stdlib/public/SwiftShims/FoundationShimSupport.h:

#ifndef NS_NON_BRIDGED
#define NS_NON_BRIDGED(type) NSObject *
#endif

@rjmccall
Copy link
Member

@rjmccall rjmccall commented Jan 23, 2019

Sure, if you're willing to write some ObjC stubs there's definitely a continuum of workarounds. I don't think that changes what the right language direction here is, though.

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants