Optimize the happy path in parXOrAccumulate

[kyay10]

This is done by wrapping raised values in a FailureValue, and values of that class are never leaked outside, hence it always denotes that a value was raised.

[github-actions]

Kover Report

File	Coverage [58.75%]
arrow-libs/fx/arrow-fx-coroutines/src/commonMain/kotlin/arrow/fx/coroutines/ParMap.kt	97.56%
arrow-libs/fx/arrow-fx-coroutines/src/commonMain/kotlin/arrow/fx/coroutines/ParZipOrAccumulate.kt	0.00%
arrow-libs/fx/arrow-fx-coroutines/src/commonMain/kotlin/arrow/fx/coroutines/predef.kt	100.00%

Total Project Coverage	52.67%

[serras]

This is all a bit magical to me, but let's start with some concrete questions: is it possible to make mightFail not inline? That way FailureValue wouldn't have to be exposed (even though it's internal, we still need to worry about future ABI compatibility).

[kyay10]

Is this better? I hid Failure as a class entirely, and instead use a function that returns an Any?.
I know this is magical on the surface, but it's a very similar encoding to stdlib's Result, if you're familiar with that. This is also inspired by EmptyValue from arrow-core.

The idea is to use a union type A | Failure<Error>, and because Failure is our own private type, this is isomorphic to an Either<Error, A>. The only issue is that we lose our typing because Kotlin doesn't have unions (and hence we end up using Any?), so care is needed to ensure we don't get CCEs (or even worse, failing tests).

[nomisRev]

I'm a bit confused on the optimisation 🤔 To me initially it even seems that this is actually resulting in more allocations, or other performance implications.

I'm fine with a Result like encoding, we purposefully never choose it because it eliminates exhaustive uses of when which are a very popular usage in Kotlin.

I'm always a bit hesitant about performance improvements, unless it's extremely obvious like an isEmpty check beforehand, or an isComplete check to avoid a suspension point. Otherwise I feel it's a bit indecisive, or premature optimisation, etc without benchmarks.

I need to take a proper look at the code though, could you elaborate a bit your train of thought of the optimisation @kyay10? I think if we do decide to merge these, or similar changes, we need to document the optimisation similar as I read when studying the KotlinX Coroutines documentation.

[kyay10]

For sure, let me clarify. When using our either builder, we end up allocating a wrapper regardless of whether the function completes successfully or raises a value. We also end up doing an is check when binding that value. In this PR, we use a mightFail builder that only allocates in the raise case. We still have an is check when binding, of course. The reason that this is safe to do is because our Failure class is never used or leaked elsewhere, and so checking for is Failure guarantees that we truly have a value that came from something that was raised. This is inspired by how Raise encodes its success and failure values. We end up basically with the same allocations in the raise case, but with less allocations in the normal-returning case.
In other words, what we have is that for some block Raise<E>.() -> A, we first assume that A == A & !Failure, which is true as long as the Arrow codebase itself doesn't call this function with such a value. Then, we construct an untagged Union A | Failure<Nel<E>>, which by the assumption should be equivalent to the same tagged union encoding of it (i.e. should be equivalent to an Either<A, Nel<E>>). Finally, we just bind it as you'd expect. The only tricky thing here is that Kotlin doesn't have unions, and so we have to use Any?, which means that we must be careful with our types.

[nomisRev]

@kyay10 I labeled this as 2.0.0. We should move 2.0.0 into main asap, such that we can do a Beta/RC1 release.

[kyay10]

Yep, that's fine! I will rebase/merge right when main becomes arrow2

[kyay10]

Closed in favour of #3408 or perhaps a RaisingDeferred type.