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-6826] [StringGuts] Side-table deallocation #49375

Closed
milseman mannequin opened this issue Jan 24, 2018 · 12 comments
Closed

[SR-6826] [StringGuts] Side-table deallocation #49375

milseman mannequin opened this issue Jan 24, 2018 · 12 comments
Assignees
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. standard library Area: Standard library umbrella

Comments

@milseman
Copy link
Mannequin

milseman mannequin commented Jan 24, 2018

Previous ID SR-6826
Radar rdar://problem/36825362
Original Reporter @milseman
Type Bug
Status Closed
Resolution Done
Additional Detail from JIRA
Votes 1
Component/s Standard Library
Labels Bug
Assignee @Catfish-Man
Priority Medium

md5: 7066f482bb79190efe2bba3a5936697a

Issue Description:

Regression from #14046

This might be ABI affecting.

When we destroy our storage classes, we spend time consulting side-tables in the objc runtime. This is likely because we want to bridge to NSString fast and our classes derive from NSString at runtime. However, we shouldn’t’ have to pay this cost.

Applies to all StringBuilder benchmarks,
ErrorHandling

This might not be the only culprit of slowdown here. Append may just be slower, and there might be something to explore there. If net regressions persist, please file a new JIRA for them.

@milseman
Copy link
Mannequin Author

milseman mannequin commented Jan 24, 2018

This lies at the intersections of ObjC, Swift, and the ObjC runtime.

/// A shadow for the "core operations" of NSString.
///
/// Covers a set of operations everyone needs to implement in order to
/// be a useful `NSString` subclass.
@objc
public protocol _NSStringCore : _NSCopying /* _NSFastEnumeration */ { ... }
// At runtime, this class is derived from `_SwiftNativeNSStringBase`,
// which is derived from `NSString`.
//
// The @_swift_native_objc_runtime_base attribute
// This allows us to subclass an Objective-C class and use the fast Swift
// memory allocator.
@_fixed_layout // FIXME(sil-serialize-all)
@objc @_swift_native_objc_runtime_base(_SwiftNativeNSStringBase)
public class _SwiftNativeNSString {
  @_versioned // FIXME(sil-serialize-all)
  @objc
  internal init() {}
  deinit {}
}

And our storage classes inherit (through 1 intermediary) from _SwiftNativeNSString and conform to _NSStringCore.

swift_deallocClassInstance calls objc_destructInstance which calls objc_object::sidetable_clearDeallocating().

@belkadan, @gparker42, or @DougGregor Any ideas how we can inherit from NSString for fast bridging, but not go down this deallocation slow path?

@milseman
Copy link
Mannequin Author

milseman mannequin commented Jan 24, 2018

@swift-ci create

@belkadan
Copy link
Contributor

Nothing from me. I figure this is required if someone's done something terrible like stuck an associated object or a weak reference to the string backing, but I would hope we could avoid it in the common case. This is ultimately a Greg question, though—it might mean conditionally avoiding calling objc_destructInstance altogether.

@milseman
Copy link
Mannequin Author

milseman mannequin commented Apr 10, 2019

@Catfish-Man or @mikeash, either of you interested in holding on to this? My understanding is that this can be improved with OS integration.

@Catfish-Man
Copy link
Member

Possibly yes! It's tricky though. Mike has been more directly involved, but if he doesn't want it I'll take it.

@mikeash
Copy link
Contributor

mikeash commented Apr 11, 2019

I think the right solution here will be for the ObjC runtime to track has-sidetableness (there has to be a better term for that) on a class-by-class basis for classes that don't use non-pointer ISA. (That's tracked at rdar://problem/45661834 for those of you who can view it.) Right now, we track this at the class level on platforms that don't support non-pointer ISA, and at the object level on platforms that do. Tracking it at the object level is a lot better, but doesn't work for classes that opt out of non-pointer ISA, which includes all Swift classes. This scheme doesn't currently fall back to tracking at the class level, but it probably should.

David, please feel free to take a shot at this. I'd like to do it but I have a lot of other stuff that needs doing first.

@Catfish-Man
Copy link
Member

Thoughts after brainstorming with Karoy and Michael:

* String backing stores are probably safe to set the existing forbid-assoc-objects bit on: evidence being that anyone relying on being able to set associated objects on arbitrary NSStrings has already likely had their code broken by tagged pointer strings, and having to hit such a code path with a bridged Swift String just makes it all the less likely

* Non-verbatim-bridged collection backing stores are definitely safe to set the existing forbid-assoc-objects bit on, but unfortunately they currently share a class with verbatim-bridged ones. We can likely split those cases up, but will have to be careful about back deployment.

The nice thing about these two cases is that, assuming we're correct, they work without requiring another bit in the class. We do still need objc runtime changes to actually check that bit in the dealloc path though, and we need to be careful not to slow down the other cases too much.

@belkadan
Copy link
Contributor

Hm. Why is the latter okay? Can't people rely on the identity of an NSArray once they have it?

@Catfish-Man
Copy link
Member

Non-verbatim-bridged Array buffers are not accessible from ObjC (we make a new object when bridging).

That said, I may have found a better approach that sidesteps this.

@Catfish-Man
Copy link
Member

(Specifically: we have half a bit left in the refcount)

@Catfish-Man
Copy link
Member

#24005

@Catfish-Man
Copy link
Member

Fix landed. Requires changes to libobjc to take effect.

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. standard library Area: Standard library umbrella
Projects
None yet
Development

No branches or pull requests

3 participants