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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stop storing transformed element when error is encountered in mapOrAccumulate #3374

Closed
wants to merge 10 commits into from

Conversation

kyay10
Copy link
Collaborator

@kyay10 kyay10 commented Feb 10, 2024

Seemed like an easy win :) If an error is encountered early-on, there's no reason why we should keep storing transformed elements (we just waste memory). Micro-benchmarking maybe is needed to figure out if this actually helps with anything. I think knowing inside forEachAccumulating whether there has been an error or not is likely useful outside of this anyway. Do note that 1.22 is getting finalised, so perhaps the ship has sailed for this change (it does change the API of the currently-unreleased forEachAccumulating)

Copy link
Contributor

github-actions bot commented Feb 10, 2024

Kover Report

File Coverage [71.28%]
arrow-libs/core/arrow-core/src/commonMain/kotlin/arrow/core/raise/RaiseAccumulate.kt 71.28%
Total Project Coverage 53.01%

@kyay10 kyay10 requested a review from nomisRev February 11, 2024 14:56
@serras
Copy link
Member

serras commented Feb 11, 2024

Although I'm not fond of the particular implementation, this seems like an interesting idea. Would it be possible instead to split the work of mapOrAccumulate into two parts? First we run usual map until we find an error, and if we do, then we run with the rest of the iterable using forEachAccumulating, but not saving any values.

In addition, @nomisRev do you remember why we make all of these functions inline? Doing so for raise seems worth it, but these functions seem big enough that inlineing may be worth in terms of binary size.

@nomisRev
Copy link
Member

nomisRev commented Feb 12, 2024

Although I'm not fond of the particular implementation, this seems like an interesting idea. Would it be possible instead to split the work of mapOrAccumulate into two parts? First we run usual map until we find an error, and if we do, then we run with the rest of the iterable using forEachAccumulating, but not saving any values.

I think you're thinking of Iterator instead of Iterable, such that you can maintain the "state" of the iterator after the "early exit". That could indeed be a very interesting alternative. Loop until the first failure, and then continue looping but without storing the results. The success buffer could even be prematurely cleared.

In addition, @nomisRev do you remember why we make all of these functions inline?

We normally make things inline to support suspend in the lambda, since now Raise is meant for both non-suspend and suspend.

Making things inline for other reasons to reduce the binary would be great! 👍

@kyay10
Copy link
Collaborator Author

kyay10 commented Feb 18, 2024

Closed in favour of #3376

@kyay10 kyay10 closed this Feb 18, 2024
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.

3 participants