michaelbull / kotlin-result

A multiplatform Result monad for modelling success or failure operations.
ISC License
1.02k stars 57 forks source link

Add monad comprehensions via binding block with bind extension function #13

Closed Munzey closed 4 years ago

Munzey commented 4 years ago

Hello!

Thought I would open a pull request to add monad comprehension like syntax for this result type. Full disclosure: before i knew about this repo I had looked to adding this feature for result4k: https://github.com/npryce/result4k/pull/3 Wasn't getting a response there so I've moved on 😅 that's when I started looking around and found this very cool repo!

For that pr I did quite a bit of micro benchmarking with jmh. The main reason for not moving forward with arrow's either type for me is how poor performing it is. The apis are also very unstable (still not at a 1.0 release). So it was important for me to choose another lib that was performant.

With my (possibly contrived) test I have benchmarked and compared flatmapping versus using my proposed bind syntax for this repo. It came out a little slower:

Lib Success Flow Failure Flow
ResultBinding 187250 ops/ms 178704 ops/ms
flatmap 280932 ops/ms 258156 ops/ms

One thing to point out is that comparing this library to Result4k, the ops/ms is significantly slower in this repo for flatmapping (i get around 250,000 - 280,000 ops/ms in this repo, compared to almost 450,000 ops/ms in result4k on my machine). I suspect its related to the use of contracts and more object creation in this repo. Regardless, its still pretty fast! just thought its interesting.

Anyway, hope your open to the proposal and keeping well in these strange times!

michaelbull commented 4 years ago

Wow this is a very interesting concept. Is this inspired from an existing Result/Either impl? If so I'd like to look over how it compares to an established implementation.

One thing to point out is that comparing this library to Result4k, the ops/ms is significantly slower in this repo for flatmapping

This is particularly interesting. I wasn't aware that the compiler contracts would have any effect on performance?

Looking at result4k's flatMap, they are doing the same thing as in this library and not applying any fewer object allocations than I do. So I can't see any other explanation other than the inclusion of contracts.

result4k

inline fun <T, Tʹ, E> Result<T, E>.flatMap(f: (T) -> Result<Tʹ, E>): Result<Tʹ, E> =
    when (this) {
        is Success<T> -> f(value)
        is Failure<E> -> this
    }

kotlin-result

inline infix fun <V, E, U> Result<V, E>.andThen(transform: (V) -> Result<U, E>): Result<U, E> {
    contract {
        callsInPlace(transform, InvocationKind.AT_MOST_ONCE)
    }

    return when (this) {
        is Ok -> transform(value)
        is Err -> this
    }
}

On a wider note, if you have done some benchmarking with JMH or similar then I am very open to adding a benchmark framework to this repository. Let me know if that's something you'd like to contribute to.

Munzey commented 4 years ago

@michaelbull thanks for fast and detailed response! 🥰

Is this inspired from an existing Result/Either impl? If so I'd like to look over how it compares to an established implementation.

So this is the output of having investigated using arrow, then having gone to see this talk at kotlin conf last year: https://www.youtube.com/watch?v=pvYAQNT4o0I I came from writing scala before moving to kotlin and i really missed some of the language ability such as for comprehensions

Arrow added this functionality via Monad Comprehensions. Because they are emulating higher kinded types, the code is fairly complex in how they achieve this syntax. They also use coroutines to do this. If you were interested in their implementation I would take a look at the following files: https://github.com/arrow-kt/arrow-core/blob/master/arrow-core/src/main/kotlin/arrow/core/extensions/either.kt#L256 https://github.com/arrow-kt/arrow-core/blob/master/arrow-core-data/src/main/kotlin/arrow/typeclasses/MonadContinuations.kt#L31 https://github.com/arrow-kt/arrow-core/blob/master/arrow-core-data/src/main/kotlin/arrow/typeclasses/suspended/BindSyntax.kt

My intial attempt was to basically strip back their coroutine code to work strictly for result type (i mention the benchmark results of that in the description of https://github.com/npryce/result4k/pull/3). Basically while it was a huge improvement over arrows implementation that covers all of their monadic types, I realised I didnt really need coroutines at all, so really all i ended up keeping was the clever scoped lambda type signature and the syntax convention. So without coroutines suddenly we see speeds comparable to just using the normal flatmap provided in these result libraries. So to answer the question in a long winded way 😅 no there isnt really a comparable implementation that I know of!

Edit: Something you may notice reading arrows monad comprehension documentation. They show in their examples of using fx bind that they override operator .component1() so that you don't need to use .bind() and instead just destructure. They never updated the documentation but youll see in BindSyntax that they ended up deprecating it because of a weird compiler optimisation that can cause failed binds not to run if you do something like:

(_) = resultThatWIllFail() // skipped by compiler
Munzey commented 4 years ago

also regarding

On a wider note, if you have done some benchmarking with JMH or similar then I am very open to adding a benchmark framework to this repository. Let me know if that's something you'd like to contribute to.

sure I can open a pull request with benchmarking added. SOmething I never tried to do actually was compare any of these libraries to the internal kotlin result type. Might be interesting! Should mention that I'm no expert on micro benchmarking - so maybe when i put up the pr someone can chime in on whether there are better ways to measure.

michaelbull commented 4 years ago

Added in 9bcaa974ca03b9f518a1e84717f79272f4d090c2

Munzey commented 4 years ago

nice! really like the when pattern matching!

michaelbull commented 4 years ago

I replaced the this.getOrElse with the when purely to avoid allocating an extra Err - instead we use the Err that it already is.

Munzey commented 4 years ago

also I can rebase #14 now to point to new master

Munzey commented 4 years ago

@michaelbull do you think it would make sense to add this functionality to the readme? I can open a proposed change if you'd like? Up to you 😄

michaelbull commented 4 years ago

Yeah, I think a section titled "monad comprehensions" or something would be good. If you could also add the links to the example implementations (e.g. swift's bow lib) to emphasise where the inspiration comes from that would be helpful.

Is there a way to get the benchmarking to output the results in a markdown formatted table? I would like to add benchmark results to the README (ran on my desktop) but don't see an easy way of displaying the data.

Munzey commented 4 years ago

@michaelbull hmm the only output I know of is in build/reports/benchmarks/main/[timestamp]/benchmark.json

might need to write a script to generate some pretty output of that

michaelbull commented 4 years ago

Okay, I found an online visualiser but it required me to put all the benchmarks in one class for them to be compared properly:

image

Munzey commented 4 years ago

pretty cool! find that graphic to be fairly biased though - while bindingSuccess is significantly less ops/ms, the x axis distances seem very strange 😅

michaelbull commented 4 years ago

It's a logarithmic scale, you can still compare the raw numbers appropriately