Skip to content

Eliminate Sequence.SubSequence + Use @_borrowed #20454

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

Closed

Conversation

airspeedswift
Copy link
Member

These affected very similar benchmarks so trying them together.

rjmccall and others added 4 commits November 8, 2018 18:12
Most of the stdlib's properties don't need @_borrowed because they're
@inlinable, but I did find one place in an overlay where it's probably
sensible to make the operation use generalized accessors even if
they're resilient.
This is a performance hack: inlining a coroutine can promote heap
allocations of the frame to stack allocations, which is valuable
out of proportion to the normal weight.  There are surely more
principled ways of getting this effect, though.
Remove split customization point

Eliminate SubSequence from Sequence protocol

Collapse LazyCollection

Collapse LazyMapCollection

Eliminate _SequenceWrapper

Collapse LazyFilterCollection

Collapse LazyDrop/PrefixWhileCollection

Fix tests, ABI stability update

Collapse FlattenSequence
@airspeedswift
Copy link
Member Author

@swift-ci please test

@airspeedswift
Copy link
Member Author

@swift-ci please benchmark

@@ -332,7 +332,7 @@ extension IndexingIterator: IteratorProtocol, Sequence {
/// or bidirectional collection must traverse the entire collection to count
/// the number of contained elements, accessing its `count` property is an
/// O(*n*) operation.
public protocol Collection: Sequence where SubSequence: Collection {
public protocol Collection: Sequence {
// FIXME: ideally this would be in MigrationSupport.swift, but it needs
// to be on the protocol instead of as an extension
@available(*, deprecated/*, obsoleted: 5.0*/, message: "all index distances are now of type Int")
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this still the case?

Copy link
Member Author

Choose a reason for hiding this comment

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

You mean the commented out obsolete part? I need to go through and remove all those – but that's not an ABI thing so not as urgent as some other stuff.

@swift-ci
Copy link
Contributor

swift-ci commented Nov 9, 2018

Build failed
Swift Test OS X Platform
Git Sha - 460bb99

@swift-ci
Copy link
Contributor

swift-ci commented Nov 9, 2018

!!! Couldn't read commit file !!!

@rjmccall
Copy link
Contributor

rjmccall commented Nov 9, 2018

Okay, there are some perf numbers in there if you look at the log, although they don't seem particularly credible. Makes me wonder if the benchmark can be optimized to triviality now or something.

@airspeedswift
Copy link
Member Author

airspeedswift commented Nov 9, 2018

@DougGregor getting this really strange error on the build bot that doesn't reproduce on any other machines:

22:34:43 [ RUN      ] FlattenCollection.FlattenCollection instances (BidirectionalCollection)
22:34:43 stderr>>> Assertion failed: (registered == c && "objc_readClassPair failed to instantiate the class in-place"), function swift_instantiateObjCClass, file /Users/buildnode/jenkins/workspace/swift-PR-osx/branch-master/swift/stdlib/public/runtime/SwiftObject.mm, line 1253.
22:34:43 stderr>>> CRASHED: SIGABRT
22:34:43 the test crashed unexpectedly
22:34:43 [     FAIL ] FlattenCollection.FlattenCollection instances (BidirectionalCollection)

Any ideas what might cause that? It only started happening when I introduced the final bits of conditional conformance to some sequences.

@DougGregor
Copy link
Member

@airspeedswift that can happen if two Swift classes have the same @objc names. Prior to macOS Mojave, objc_readClassPair would return a null if the class was already registered, causing Swift's standard library to assert.

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.

5 participants