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

Add SequenceType methods to AnySequence #818

Closed

Conversation

therealbnut
Copy link
Contributor

Implements other methods of SequenceType in AnySequence so that overrides work correctly.

Currently AnySequence(0...1000000000).suffix(2) takes a long time to run, but (0...1000000000).suffix(2) does not.

@therealbnut
Copy link
Contributor Author

See https://bugs.swift.org/browse/SR-413 for more details.

@@ -132,6 +132,28 @@ internal class _AnySequenceBox<Element> {

internal func _underestimateCount() -> Int { _abstract() }

internal func _map<T>(@noescape transform: Element throws -> T) rethrows -> [T] { _abstract(); }
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove semicolons and wrap to 80 columns (here and everywhere in the patch).

@therealbnut
Copy link
Contributor Author

Thanks @gribozavr, I've applied your feedback, just verifying the tests pass locally.

}

override func _dropFirst(n: Int) -> AnySequence<Element> {
return _base.dropFirst(n)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this actually forwards to the underlying sequence. It just forces the overload resolution to pick up the default implementation.

@gribozavr
Copy link
Contributor

@therealbnut Thank you. I don't think this change actually achieves what you wanted. Could you take a look at #220?

Another thing that is missing is the same optimization for Any*Collection.

And of course, tests are missing. The tests should use LoggingRangeReplaceableCollection wrapped in AnySequence and Any*Collection and verify that the implementations actually forward to the underlying collection.

@gribozavr gribozavr self-assigned this Dec 30, 2015
@therealbnut
Copy link
Contributor Author

Thanks @gribozavr, I've had a look at #220, good to know. I don't think it completely overlaps with this though.

I'm fairly sure this will work, but I will implement the tests and verify. I'm finding my dev environment for swift's libraries a little cumbersome at the moment.

Should I be putting tests into CheckSequenceType.swift? Also do you know a fast way from the command-line to just build what has changed (and run that test)?

@gribozavr
Copy link
Contributor

@therealbnut The reason why this won't work is because at the point where you call, say, base.prefix() you are using the static return type AnySequence -- that is only the case in the default implementation. The custom default implementation uses Base.SubSequence as the return type. If you use that, you need wrapping into another box, and then the rest of #220.

The forwarding tests should go into ./test/1_stdlib/ExistentialCollection.swift and should use expectCustomizable.

If you want to add more in-depth tests, take a look at the validation-test/stdlib/Slice/ directory. Those tests create a bunch of different collection arrangements, and call one of the check*Collection functions, that call every possible method on the collection and verify behavior correctness. This won't show that the optimization is in the effect, since the default implementation is conservatively correct, but it will show that the type overall behaves correctly.

@therealbnut
Copy link
Contributor Author

Ah right, I understand what you meant now - I agree. I'll add the tests and see if I can get it working, and/or put this on hold until #220.

@gribozavr
Copy link
Contributor

@therealbnut JFYI: #895 has landed.

@therealbnut
Copy link
Contributor Author

Thanks @gribozavr, and thank you for your help :) I'll see what I can get done, although I'm back at work with no free time for about 3 months.

I will probably do SequenceType first as that can potentially land separately.

On another note, I feel like all of this should be able to be automated by the compiler. I think this effort would be better spent with a more generic solution. Although, I'm unsure of the scale of the work to get this done. Also it would require a proposal. What do you think about the short-term/long-term win work tradeoff?

  • It would get us AnySequence, AnyForwardCollection, AnyForwardIndex, AnyGenerator, etc . for free.
  • It would prevent this sort of bug from occurring again.
  • It would put control of the implementation in the hands of the compiler (do compilers have hands?).
  • It would allow future optimisations to be applied uniformly and with a consistent implementation.
  • It removes a lot of error-prone repetitive boilerplate code.

Something like this could work:

struct AnySequence<T>: @any SequenceType {
    public typealias Element = T
    public typealias SubSequence = AnySequence<T>
}

The compiler could generate those method definitions. These are two downsides:

  • It likely won't copy method comments from the protocol. I think that comments should also be addressed, but as a separate issue that improves all protocols.
  • It isn't explicit that it's providing a method, so would you expect SequenceType's extension or AnySequence to provide the implementation.

Alternatively it could be clearer if you explicitly state the methods, with comments. I can't think of a good way to annotate that though, and it also adds a lot of boilerplate.

@gribozavr
Copy link
Contributor

@therealbnut Automatically generating an implementation like this with type erasure might be possible, but it would be a massively large compiler feature, because of the complexity of the problem, and because there are so many choices in the way you can actually do the implementation details of AnySequence. I don't think it would be implemented any time soon.

What is interesting (and practical) in this area is the interface forwarding problem, its application area is much wider than type erasure, and I think it would be more interesting to implement in the language.

@therealbnut
Copy link
Contributor Author

@gribozavr hmm, that's a good point. I think there was proposals that talked about implementation forwarding, but if I recall correctly it was jumbled in with a lot of other features.

Having a play around with how it could be expressed it seems to require a few capabilities I don't remember those proposals discussing, namely override, public, private, computed and/or abstract.

private class _SequenceBase<E>: SequenceType {
    private var _neverUsed: AnySequence<E> { return _abstract() }
    forward SequenceType = _neverUsed
}
private final class _Sequence<S: SequenceType>: _SequenceBase<S.Generator.Element> {
    private var _implementation: S
    override forward SequenceType = _implementation
}
struct AnySequence<T>: SequenceType {
    public typealias Element = T
    public typealias SubSequence = AnySequence<T>

    private var base: _SequenceBase<T>

    public forward SequenceType = base
}

Anyway, I'll proceed with this PR, it's at least something that would be desirable in the interim.

@therealbnut therealbnut force-pushed the andrew-anysequence-methods branch from ee4398b to f830b19 Compare January 24, 2016 02:54
@therealbnut
Copy link
Contributor Author

FYI I believe the tests to be valid, but they will be failing until the AnySequence implementation is updated.

@swift-ci

@DougGregor
Copy link
Member

This has been dormant for a long time. @dabrahams , is this a direction we want to go?

@therealbnut
Copy link
Contributor Author

@DougGregor Sorry about that. I reported the bug and had a go at adding tests and fixing it, I got stuck and had work commitments, it was also pre Swift 3 and iirc. dependent on changes out of scope of that release.

The bug (if unresolved) is still a big Swift performance issue though, and perhaps can be fixed with some of the recent improvements to generics and extensions. I think you've been doing work around that.

https://bugs.swift.org/browse/SR-413

@dabrahams
Copy link
Contributor

@DougGregor at least until such time as we have generalized existentials, yes, we'd like this to happen.

@therealbnut
Copy link
Contributor Author

therealbnut commented May 21, 2017

I'm closing this, the work is still necessary, but this branch is well and truly stale. Thanks for the reviews that were made on it, while the changes weren't included they helped me learn and contribute in other ways.

@therealbnut therealbnut deleted the andrew-anysequence-methods branch May 21, 2017 23:18
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.

4 participants