Add extension methods for combining multiple results into one #18
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@PressXtoChris Can you allow me to make edits to the pull request branch? |
|
Nevermind, it's my git that's being wonky. |
PressXtoChris
commented
Jul 5, 2024
tests/LightResults.Extensions.Operations.Tests/LightResults.Extensions.Operations.Tests.csproj
Outdated
Show resolved
Hide resolved
|
This is for 10, 100, 1,000, and 10,000 results respectively. When I started, benchmarks looked like this: Now they look like this: |
|
Don't know if you want to take another crack at it now? |
|
I've added the comparisons benchmarks project. That's an impressive jump you got |
|
I improved performance and reduced memory allocations. I doubt we'll be able to do much better than this. So I'm pretty confident that this could be merged now, unless you've got more ideas. Before: After: Before: After: Before: After: Before: After: |
PressXtoChris
commented
Jul 6, 2024
|
You're right, I missed that. Thanks. |
|
In the end, there's a clear reduction in memory allocations, most significatingly in happy paths. It's a trade off by an increase in execution time. But I'd rather sacrifice execution over memory allocations.
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes jscarle/LightResults#32
What's the change?
Introducing extension methods to combine multiple results into one
Why is it wanted?
There are some situations where a developer might be concurrently calling multiple methods that return results (e.g.
Task.WhenAll), or have a method that calls an operation multiple times (e.g.yield returning into anIEnumerable). Rather than require the developer to inspect each result individually, we can combine them into a single result object for easy processing.How is it implemented?
Add two new extension methods:
Result Collect(this IEnumerable<Result> results)andResult<IReadOnlyList<T>> Collect(this IEnumerable<Result<T>> results). These will turn the enumerable into an array (if it wasn't already an array), then turn that array into aReadOnlySpanbecause they're faster to iterate over. We build up a list of errors that all the results had, and return them in a failed result. If there were no failures, we return an OK result (with a ReadOnlyList containing all the values if it wasResult<T>).Added a new LightResults.Extensions.Operations project to the solution to house these new extension methods.
Benchmarking
I've experimented with a couple of different implementations, and from my benchmarks this seems to perform pretty well. I was using an array of 512 results, half of them successful and half of them failed.
Linqones were done using similar logic to what I've got in the OP of Extension method to collectIEnumerable<Result<T>>intoResult<IEnumerable<T>>LightResults#32ForvsForSizedcomparisons are using a for loop, and comparing pre-sizing the error/value lists.Spanones are converting array to aReadOnlySpanlike I have in this PR. TheUnsafeversions useMemoryMarshal.GetReference()andUnsafe.Add()to traverse the memory the results are at. Negligible speed differences, and probably not worth the risk even if they were faster.Comparing to other libraries that offer similar functionality:
This implementation is almost on par with FluentResults. Rascal's not really a fair comparison because
Sequence()returns early with the first error it comes across, rather than getting all of them.Let me know if you would like to add a Benchmarks project to this solution too; and I can commit the code for them
Naming
Not sure if you've got a preferred name for this operation. I borrowed
Collectfrom Rust since that's what they use to combineVec<Result<T, E>>intoResult<Vec<T>, E>. I thinkMergeorCombinecould also work (and might be a little more intuitive).