Avoid storing more transformed elements than necessary #3376
Conversation
Kover Report
|
arrow-libs/core/arrow-core/src/commonMain/kotlin/arrow/core/raise/RaiseAccumulate.kt
Outdated
Show resolved
Hide resolved
| @BuilderInference blockUntilError: RaiseAccumulate<Error>.(A) -> Unit, | ||
| @BuilderInference blockAfterError: RaiseAccumulate<Error>.(A) -> Unit |
There was a problem hiding this comment.
Nit: Do we really need two lambdas here? 🤔
Or was that for JvmName? Otherwise I am also fine using forEachAccumulatingImpl or something. I've started doing that in my own projects, similar how to KotlinX Coroutines does this with produceImpl. flattenMergeImpl, etc.
There was a problem hiding this comment.
We do, because we still need to call transform on each value, just not add the result to the list.
There was a problem hiding this comment.
Why not have:
@BuilderInference block: RaiseAccumulate<Error>.(A) -> B,
add: (A, B) -> Unit
and simply we only call add if we don't have any errors.
We could also just have a Boolean parameter instead, with similar effects.
I really also dislike the binary-size blowup that can occur because mapOrAccumulate inlines its block in 2 different places.
arrow-libs/core/arrow-core/src/commonMain/kotlin/arrow/core/raise/RaiseAccumulate.kt
Outdated
Show resolved
Hide resolved
| ) { | ||
| val error: MutableList<Error> = mutableListOf() | ||
| var errorArose = false |
There was a problem hiding this comment.
Nit: same as above, but with error.isEmpty()
arrow-libs/core/arrow-core/src/commonMain/kotlin/arrow/core/raise/RaiseAccumulate.kt
Outdated
Show resolved
Hide resolved
|
Indeed, @kyay10's comment about size explosion was on point. I've reworked the implementation to use a version with a single function, with a Boolean with errors. (Note however that this function is only exposed for |
| @@ -521,10 +521,19 @@ public inline fun <Error, A> Raise<Error>.forEachAccumulating( | |||
| iterator: Iterator<A>, | |||
| combine: (Error, Error) -> Error, | |||
| @BuilderInference block: RaiseAccumulate<Error>.(A) -> Unit | |||
| ): Unit = forEachAccumulatingImpl(iterator, combine) { item, _ -> block(item) } | |||
|
|
|||
| @PublishedApi @JvmSynthetic | |||
There was a problem hiding this comment.
Synthetic is a nice touch here! Note that this technically doesn't 100% absolve us of ABI compatibility issues according to the discussion on Kotlin/binary-compatibility-validator#165. I wish we had access to @InlineOnly, then maybe BCV would consider those methods to not be part of our ABI, but alas.
This is a counter-proposal for #3374 @kyay10
This introduces a new (internal) version of
forEachAccumulatingwhich works differently before and after the error. That way we can simply stopadding things after the first error.