arrow-kt / arrow-fx

Λrrow Fx is part of Λrrow, a functional companion to Kotlin's Standard Library
http://arrow-kt.io
Other
64 stars 15 forks source link

Add arity 4,5,6 to parMapN #316

Closed unaryops closed 3 years ago

unaryops commented 3 years ago

I had a use case for using parMapN 4. I've added support for arity 4, 5, 6.

nomisRev commented 3 years ago

Hey @unaryops, Thanks for your contribution!

We've recently merged a PR to make KotlinX Coroutines a base dependency for Arrow Fx Coroutines to get rid of the friction there was today in terms of cancellation interop etc

This would require a couple changes in the parMapN implementation, it should be easy to write concrete implementation using coroutineScope and async like in parMapN 2-3 which prevents additional schedules/dispatching.

https://github.com/arrow-kt/arrow-fx/pull/317 https://github.com/arrow-kt/arrow-fx/blob/master/arrow-fx-coroutines/src/main/kotlin/arrow/fx/coroutines/ParMapN.kt#L78

unaryops commented 3 years ago

@nomisRev got it. I will update my PR.

unaryops commented 3 years ago

@nomisRev I've noticed the following TODO comment https://github.com/arrow-kt/arrow-fx/blob/aff2bec2c4d67cb5dcbae819c098a9076fb103b4/arrow-fx-coroutines/src/main/kotlin/arrow/fx/coroutines/ParMapN.kt#L151

Is that something I should be taking into consideration?

nomisRev commented 3 years ago

@unaryops yes, in the existing IO implementation and the implementations you added we derived implementations like below. This however requires scheduling 3 parMapN inside parMapN which means that it requires 3 additional schedules/dispatches.

 parMapN(
    ctx,
    suspend { parMapN(ctx, fa, fb, ::Pair) },
    suspend { parMapN(ctx, fc, fd, ::Pair) },
    suspend { parMapN(ctx, fe, ff, ::Pair) }
  ) { ab, cd, ef ->
    val (a, b) = ab
    val (c, d) = cd
    val (e, f) = ef
    f(a, b, c, d, e, f)
  }

With the simpler implementation that delegates to KotlinX we can easily avoid that without requiring large implementations like was required before for parMap2 and parMap3. Since the desired behavior is already ensured by coroutineScope.

unaryops commented 3 years ago

I'm not sure why Check Previous Build Integration is failing.

nomisRev commented 3 years ago

Thanks @unaryops! 🎉