Add EventLoopFuture reduction primitives #240

karim-elngr · 2018-03-26T19:31:21Z

Implement EventLoopFuture reduce function [#184]

Motivation:

Provide a primitive for reducing an array of EventLoopFuture.

Modifications:

Added new function
EventLoopFuture<T>.reduce(initialResult:futures:eventLoop:reducer)

Recomposed EventLoopFuture<Void> to use the new reduce function.

Result:

A nice API to work with multiple futures.

swift-nio-bot · 2018-03-26T19:31:23Z

Can one of the admins verify this patch?

karim-elngr · 2018-03-26T19:32:28Z

@Lukasa What do you think about this?

weissi

thanks, this looks good to me. Just a small nit.

weissi · 2018-03-27T14:32:02Z

Tests/NIOTests/EventLoopFutureTest.swift

+        let futures = promises.map { $0.futureResult }
+
+        let fN: EventLoopFuture<[Int]> = EventLoopFuture<[Int]>.reduce([], futures, eventLoop: eventLoop, reducer: {$0 + [$1]})
+        _ = promises.map { $0.succeed(result: (1)) }


would you mind making this succeed the promises with different values (maybe 1...5) so we can see that the associativity is correct?

vlm · 2018-03-27T22:59:19Z

Tests/NIOTests/EventLoopFutureTest.swift

+        let promises: [EventLoopPromise<Int>] = (0..<5).map { (_: Int) in eventLoop.newPromise() }
+        let futures = promises.map { $0.futureResult }
+
+        let fN: EventLoopFuture<[Int]> = EventLoopFuture<[Int]>.reduce([], futures, eventLoop: eventLoop, reducer: {$0 + [$1]})


This + does look like quadratic operation. Does it make sense to create some blueprint or comment or a specialized function for such kind of array-based accumulator that makes assembling the result linear?

@vlm That is a perfect use-case for reduce(into:), that I mentioned here.

moiseev · 2018-03-27T23:01:36Z

Just a thought... Maybe it would make sense to have another overload of reduce analogous to the standard library reduce(into:). (See SE-0171).

moiseev · 2018-03-27T23:05:17Z

And another thought. Having reduce be a static method on the EventLoopFuture only makes the call-site unnecessarily verbose. Swift (unlike Java as I remember it) allows free functions, so it should be very much possible to just make reduce a free function and avoid an unnecessary EventLoopFuture<ConcreteType>. prefix.

karim-elngr · 2018-03-28T05:54:14Z

@moiseev I think something like this might work for reduce(into:), what do you think?

func reduce<U>(into initialResult: T, _ futures: [EventLoopFuture<U>], eventLoop: EventLoop, combiner: @escaping (inout T, U) -> ()) -> EventLoopFuture<T> {
    let p0: EventLoopPromise<T> = eventLoop.newPromise()
    var result: T = initialResult
        
    let future = EventLoopFuture<Void>.reduce((), futures, eventLoop: eventLoop) { (_, value) in
        combiner(&result, value)
    }
    future.whenSuccess {
        p0.succeed(result: result)
    }
    future.whenFailure { (error) in
        p0.fail(error: error)
    }
        
    return p0.futureResult
}

Lukasa · 2018-03-28T08:39:27Z

@moiseev I was under the impression the Swift team discouraged the use of free functions? I seem to recall you telling me to get rid of all the ones I put in. 😉

Lukasa

Cool, this looks really good. Just a few minor notes.

Lukasa · 2018-03-28T08:40:03Z

Sources/NIO/EventLoopFuture.swift

+    /// The new `EventLoopFuture` contains the result of reducing the `initialResult` with the
+    /// values of the `[EventLoopFuture<U>]`.
+    ///
+    /// returned `EventLoopFuture` will fail as soon as any of the futures fails: otherwise, it will succeed


Nit: This should begin with "The returned", not "returned"

Lukasa · 2018-03-28T08:40:16Z

Sources/NIO/EventLoopFuture.swift

+    /// only when all of them do.
+    ///
+    /// - parameters:
+    ///     - initialResult: An initial result to being the reduction.


s/being/begin/

Lukasa · 2018-03-28T08:41:49Z

Sources/NIO/EventLoopFuture.swift

@@ -828,14 +828,41 @@ extension EventLoopFuture {
    ///     - eventLoop: The `EventLoop` on which the new `EventLoopFuture` callbacks will fire.
    /// - returns: A new `EventLoopFuture`.
    public static func andAll(_ futures: [EventLoopFuture<Void>], eventLoop: EventLoop) -> EventLoopFuture<Void> {
-        let p0: EventLoopPromise<Void> = eventLoop.newPromise()
+        let body = EventLoopFuture<Void>.reduce((), futures, eventLoop: eventLoop) {
+            (_, _) in


Can we add the types that we're explicitly suppressing here? We've found that it reduces the number of bugs we hit by doing so. That'd be (_: Void, _: Void) in.

Lukasa · 2018-03-28T08:46:27Z

Sources/NIO/EventLoopFuture.swift

+        let body = futures.reduce(p0.futureResult) { (f1: EventLoopFuture<T>, f2: EventLoopFuture<U>) -> EventLoopFuture<T> in
+            f1.and(f2).map(nextPartialResult)
+        }
+        p0.succeed(result: initialResult)


Do we want a thenReduce function that is very similar but where the reducer itself returns an EventLoopFuture? Not sure if we're wildly over-engineering with that case or not.

The problem with the current approach of and followed by map is that and allocates EventLoopPromise and map allocates EventLoopPromise and EventLoopFuture. If we implement a thenReduce(otherFuture:bifunction) we could avoid the overhead of these allocations.

Lukasa · 2018-03-28T08:47:29Z

Tests/NIOTests/EventLoopFutureTest.swift

+        } catch let e {
+            XCTFail("error of wrong type \(e)")
+        }
+    }


Can we have one extra test that validates that the failure is fast? It should be easy enough: fail the first promise and confirm that the result future has completed immediately.

karim-elngr · 2018-03-28T16:06:35Z

@Lukasa @weissi Can you verify the changes?

weissi · 2018-03-28T16:08:22Z

@swift-nio-bot please test

moiseev · 2018-03-28T17:46:35Z

@Lukasa if the free functions were discouraged they wouldn't exist, I think. In this particular case one of the type arguments for a function gets inherited from the type it's defined on, which makes invocation rather ugly. It is also quite possible to group overloads of reduce and potentially other utilities under a 'namespace' (as I did with System). But naming that would be hard ;-)

Lukasa · 2018-03-29T09:40:36Z

@weissi Are you open to having a thenReduce? If so, should it have a better name than that?

weissi · 2018-03-29T19:14:20Z

@Lukasa yes, I am. Not in love with the name though but haven't really got a better suggestion. In Haskell it's foldM

Lukasa · 2018-04-03T08:06:24Z

We could just call it fold, I suppose. foldM is a bad name because, as we have discussed before, while Futures are abstractly monads, ours is not a monad, so we shouldn't really pretend that it is. 😉

I think I prefer fold to thenReduce, tbh. @karim-elngr If you open a PR that provides fold we can use that to rebuild this one.

weissi · 2018-04-03T09:18:44Z

@Lukasa just to point it out though: It's slightly odd that the fold for pure values will then be called reduce and the one for futuristic values will be fold.

Lukasa · 2018-04-03T09:28:46Z

I think it's fine. map -> reduce, then -> fold. Does the symmetry not hold there? (Again, leaving aside "not actually a monad").

karim-elngr · 2018-04-03T10:33:41Z

@Lukasa Yes, I agree foldM is not a good name. Just to be sure the implementation is going to be very similar to this:

foldl f z []     = z                  
foldl f z (x:xs) = foldl f (f z x) xs

where z and xs are all futures right?

Lukasa · 2018-04-03T12:21:21Z

@karim-elngr Looks about right to me.

karim-elngr · 2018-04-05T15:33:47Z

@Lukasa I rebuilt this PR, would you mind checking it out

Lukasa

One minor note here, but I'd like to tag @weissi in for review as well.

Lukasa · 2018-04-05T15:36:33Z

Sources/NIO/EventLoopFuture.swift

-            return p0.futureResult
+        let body: EventLoopFuture<Void> = EventLoopFuture<Void>.reduce((), futures, eventLoop: eventLoop) {
+            (_: Void, _: Void) in
+            return ()


The second line here isn't necessary.

I'm a little disappointed that even the first one is ;-) It should just be { _ in () }.

@moiseev we had bugs just blanket { _ in ... }s because people thought it's some uninteresting type () and just ignored important things. In synchronous programming you also don't write _ = foo() all that often as most of the time you shouldn't ignore what's returned.
Therefore we mandated to explicitly write the types of the stuff you ignore just to make it obvious. In the Void case it should however be (). That makes it totally obvious that something unimportant is ignored and if that should change we'll get a compile-time error.

@karim-elngr maybe just change this to

let body = EventLoopFuture<Void>.reduce((), futures, eventLoop: eventLoop) { ((), ()) in () }

@weissi oh, so it's not that the compiler can't infer the type? I was under the impression that was the case. I don't mind explicit typing at all, but only for the right reasons.

This doesn't compile in swift-4.0.3 with the following error "Closure tuple parameter does not support destructuring"

let body = EventLoopFuture<Void>.reduce((), futures, eventLoop: eventLoop) { ((), ()) in () }

Xcode even suggests the following fix:

let body = EventLoopFuture<Void>.reduce((), futures, eventLoop: eventLoop) { (arg0, arg1) in let () = arg1 let () = arg0 () }

Even this

{ _ in () }

Gives the following error "Contextual closure type '(Void, _) -> Void' expects 2 arguments, but 1 was used in closure body"

Doesn't compile unless we add an additional _ because of SE-0110:

{ _, _ in () }

weissi

thanks very much. Looks good to me. @moiseev mind taking a look too?

moiseev · 2018-04-05T17:37:11Z

Sources/NIO/EventLoopFuture.swift

-            return p0.futureResult
+        let body: EventLoopFuture<Void> = EventLoopFuture<Void>.reduce((), futures, eventLoop: eventLoop) {
+            (_: Void, _: Void) in
+            return ()


I'm a little disappointed that even the first one is ;-) It should just be { _ in () }.

moiseev · 2018-04-05T17:46:51Z

Sources/NIO/EventLoopFuture.swift

+    ///     - eventLoop: The `EventLoop` on which the new `EventLoopFuture` callbacks will fire.
+    ///     - combiner: The bifunction used to combine partialResults with new elements.
+    /// - returns: A new `EventLoopFuture` with the combined value.
+    public static func reduce<U>(into initialResult: T, _ futures: [EventLoopFuture<U>], eventLoop: EventLoop, combiner: @escaping (inout T, U) -> Void) -> EventLoopFuture<T> {


Looking into this a little more, I disagree with the choice of argument labels, especially eventLoop and combiner/reducer. It is fine most of the time, when using trailing closure syntax, but even in the body of reduce(into:) it looks unnatural: combiner(&result, value). Function name should be a verb. But that's an implementation details. I would vote for removing the label for the closure parameter completely, and since we're at it, eventLoop label seems to be repeating type information, so why not drop as well?

That makes sense, how about using names similar to the standard library's reduce (nextPartialResult, updateAccumulatingResult)?
The current andAll implementation in master uses eventLoop label, so I was trying to follow the same convention to be consistent. If we want to drop it, I suggest we also remove it from andAll(_:eventLoop). But I am not sure

I think let's keep the eventLoop label for now. I accept it's a wart, but we'd want to remove it from andAll and that's a nice semver major, so let's leave it for now.

moiseev · 2018-04-05T17:54:47Z

Sources/NIO/EventLoopFuture.swift

+        let p0: EventLoopPromise<T> = eventLoop.newPromise()
+        var result: T = initialResult
+
+        let future = EventLoopFuture<Void>.reduce((), futures, eventLoop: eventLoop) { (_: Void, value: U) in


It is worth investigating the performance of this implementation. It is technically correct, but might not be as performant as a direct call to future.fold (as done in the non-mutaing reduce).

swift-nio-bot · 2018-04-07T00:04:20Z

Can one of the admins verify this patch?

normanmaurer · 2018-04-07T08:17:46Z

@swift-nio-bot test this please

Lukasa · 2018-04-09T11:06:25Z

@swift-nio-bot test this please

Lukasa · 2018-04-09T11:24:10Z

@weissi Mind re-reviewing this?

weissi

thanks, that looks pretty good to me. Just documentation parameter names need to be changed.

weissi · 2018-04-09T11:27:06Z

Sources/NIO/EventLoopFuture.swift

+    ///     - initialResult: An initial result to begin the reduction.
+    ///     - futures: An array of `EventLoopFuture` to wait for.
+    ///     - eventLoop: The `EventLoop` on which the new `EventLoopFuture` callbacks will fire.
+    ///     - combiner: The bifunction used to combine partialResults with new elements.


this parameter got renamed, please change the docs accordingly.

ah yes I am stupid, I forgot to do so.

weissi · 2018-04-09T11:27:14Z

Sources/NIO/EventLoopFuture.swift

+    ///     - initialResult: An initial result to begin the reduction.
+    ///     - futures: An array of `EventLoopFuture` to wait for.
+    ///     - eventLoop: The `EventLoop` on which the new `EventLoopFuture` callbacks will fire.
+    ///     - reducer: The bifunction used to produce partial results.


Lukasa · 2018-04-09T11:27:42Z

Sources/NIO/EventLoopFuture.swift

+    /// values of the `[EventLoopFuture<U>]`.
+    ///
+    /// This function makes copies of the result for each EventLoopFuture, for a version which avoids
+    /// making copies, checkout reduce<U>(into:).


Nits: "check out", and please put backticks around reduce(into:)

Lukasa · 2018-04-09T11:33:45Z

Sources/NIO/EventLoopFuture.swift

+    /// `futures`. However, the failure will not occur until all preceding
+    /// `EventLoopFutures` have completed. At the point the failure is encountered, all subsequent
+    /// `EventLoopFuture` objects will no longer be waited for. This function therefore fails fast: once
+    /// a failure is encountered, it will immediately fail the overall EventLoopFuture.


Nit: backticks around EventLoopFuture.

Lukasa · 2018-04-09T11:33:59Z

Sources/NIO/EventLoopFuture.swift

+    /// Returns a new `EventLoopFuture` that fires only when all the provided futures complete.
+    /// The new `EventLoopFuture` contains the result of combining the `initialResult` with the
+    /// values of the `[EventLoopFuture<U>]`. This funciton is analogous to the standard library's
+    /// reduce(into:), which does not make copies of the result type for each EventLoopFuture.


Nit: backticks around reduce(into:)

Lukasa · 2018-04-09T11:35:04Z

Sources/NIO/EventLoopFuture.swift

+    /// `futures`. However, the failure will not occur until all preceding
+    /// `EventLoopFutures` have completed. At the point the failure is encountered, all subsequent
+    /// `EventLoopFuture` objects will no longer be waited for. This function therefore fails fast: once
+    /// a failure is encountered, it will immediately fail the overall EventLoopFuture.


Nit: backticks for EventLoopFuture.

Motivation: Add a new function to allow us to work with multiple EventLoopFutures, that return values, by combining their results and wrapping it in a new EventLoopFuture, using a specified reducer function. Modifications: Add a new function EventLoopFuture<T>.reduce(initialResult:futures:eventLoop:nextPartialResult) Result: Can reduce multiple EventLoopFutures without having to deal with creating EventLoopFutures.

Motivation: andAll<Void> can be refactored using the new EventLoopFuture reduce(initialResult) function, which should simplify the logic behind andAll<Void>. Modifications: Changed the implementation of andAll<Void> to use EventLoopFuture<Void>.reduce(initialResult) Result: Simpler andAll<Void> implementation.

Motivation: The version of reduce(initialResult) always copies the partial results for each eventLoopFuture in the futures array. This overload of reduce avoid copying by introducing and inout parameter used to combine the results. Modifications: Add a new function EventLoopFuture<T>.reduce(into:) Result: An API which can be used to construct collections from future values in a linear time.

Lukasa

Ok, I'm happy, let's see what @weissi thinks.

Lukasa · 2018-04-09T16:00:53Z

@swift-nio-bot test this please

weissi

thanks very much @karim-elngr , really cool!

weissi requested changes Mar 27, 2018

View reviewed changes

weissi added the 🆕 semver/minor Adds new public API. label Mar 27, 2018

vlm reviewed Mar 27, 2018

View reviewed changes

Lukasa requested changes Mar 28, 2018

View reviewed changes

karim-elngr changed the title ~~Implement `EventLoopFuture<T>.reduce~~ Implement `EventLoopFuture<T>.reduce [#184] Mar 28, 2018

karim-elngr changed the title ~~Implement `EventLoopFuture<T>.reduce [#184]~~ Implement `EventLoopFuture<T>.reduce Mar 28, 2018

karim-elngr changed the title ~~Implement `EventLoopFuture<T>.reduce~~ Add EventLoopFuture reduction primitives Apr 5, 2018

Lukasa requested changes Apr 5, 2018

View reviewed changes

weissi reviewed Apr 5, 2018

View reviewed changes

moiseev reviewed Apr 5, 2018

View reviewed changes

weissi requested changes Apr 9, 2018

View reviewed changes

Lukasa requested changes Apr 9, 2018

View reviewed changes

karim-elngr added 3 commits April 9, 2018 16:21

Lukasa approved these changes Apr 9, 2018

View reviewed changes

weissi approved these changes Apr 9, 2018

View reviewed changes

Lukasa merged commit 39b9a3d into apple:master Apr 9, 2018

Lukasa added this to the 1.4.0 milestone Apr 9, 2018

Add EventLoopFuture reduction primitives #240

Add EventLoopFuture reduction primitives #240

Conversation

karim-elngr commented Mar 26, 2018

Motivation:

Modifications:

Result:

swift-nio-bot commented Mar 26, 2018

karim-elngr commented Mar 26, 2018

weissi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moiseev commented Mar 27, 2018

moiseev commented Mar 27, 2018

karim-elngr commented Mar 28, 2018

Lukasa commented Mar 28, 2018

Lukasa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karim-elngr commented Mar 28, 2018

weissi commented Mar 28, 2018

moiseev commented Mar 28, 2018

Lukasa commented Mar 29, 2018

weissi commented Mar 29, 2018

Lukasa commented Apr 3, 2018

weissi commented Apr 3, 2018 • edited Loading

Lukasa commented Apr 3, 2018

karim-elngr commented Apr 3, 2018 • edited Loading

Lukasa commented Apr 3, 2018

karim-elngr commented Apr 5, 2018

Lukasa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weissi Apr 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weissi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swift-nio-bot commented Apr 7, 2018

normanmaurer commented Apr 7, 2018

Lukasa commented Apr 9, 2018

Lukasa commented Apr 9, 2018

weissi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lukasa left a comment

Choose a reason for hiding this comment

Lukasa commented Apr 9, 2018

weissi left a comment

Choose a reason for hiding this comment

weissi commented Apr 3, 2018 •

edited

Loading

karim-elngr commented Apr 3, 2018 •

edited

Loading

weissi Apr 5, 2018 •

edited

Loading