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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 2 commits into from

Conversation

Munzey
Copy link
Contributor

@Munzey Munzey commented May 8, 2020

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: npryce/result4k#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
Copy link
Owner

michaelbull commented May 8, 2020

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.

* @sample com.github.michaelbull.result.bind.ResultBindingTest
*
*/
inline fun <V, E> binding(crossinline block: ResultBinding<E>.() -> V): Result<V, E> {
Copy link
Owner

Choose a reason for hiding this comment

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

I think we should make this function name more specific, for example resultBinding or bindResult, as binding feels too generic. Let me know if you have a good proposal for this.

Copy link
Owner

Choose a reason for hiding this comment

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

Upon further inspection, it looks like a library called bow for swift calls this function "binding", so let's keep it that way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah nice, yep bow is the counter part to arrow so this makes sense!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but i agree, perhaps it is a bit too generic, and I agree resultBinding might be the best way to phrase what is is - my only issue is it becomes quite a long word to use for what is essentially a keyword.

Copy link
Contributor Author

@Munzey Munzey May 9, 2020

Choose a reason for hiding this comment

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

interestingly if you go far back enough in the arrow repo (back to when it was called kategory) they had considered calling bind -> await. but might have been a good choice to not confuse this as being something async or relating to coroutines

@Munzey
Copy link
Contributor Author

Munzey commented May 9, 2020

@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 npryce/result4k#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
Copy link
Contributor Author

Munzey commented May 9, 2020

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
Copy link
Owner

Added in 9bcaa97

@Munzey
Copy link
Contributor Author

Munzey commented May 11, 2020

nice! really like the when pattern matching!

@michaelbull
Copy link
Owner

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
Copy link
Contributor Author

Munzey commented May 11, 2020

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

@Munzey
Copy link
Contributor Author

Munzey commented May 11, 2020

@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
Copy link
Owner

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
Copy link
Contributor Author

Munzey commented May 11, 2020

@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
Copy link
Owner

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
Copy link
Contributor Author

Munzey commented May 11, 2020

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
Copy link
Owner

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

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.

None yet

2 participants