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

Make Lwt_seq.of_list lazier, Lwt_seq.to_list tail recursive #1019

Merged
merged 1 commit into from
May 14, 2024

Conversation

samer--
Copy link
Contributor

@samer-- samer-- commented May 9, 2024

This PR makes a couple of small changes to make Lwt_seq.to_list and Lwt_seq.of_list work better with large lists.

of_list: current version builds a large term eagerly and can fail with a stack overflow or segmentation fault for large lists. This lazier version defers the creation of Lwt_seq.node terms until they are needed.

to_list: current version is not tail recursive and can fail with long sequences. This tail recursive version (using a difference list accumulator) seems able to handle longer sequences.

@raphael-proust
Copy link
Collaborator

Thanks!

I think there's one regression: if the first call to seq fails with an exception, then the call to to_list raises an exception (where it used to return a failed promise).

In practice this is probably not too big a deal because the the exception is caught if the call is on the right-on-side of a bind (or inside another closure/lambda). But for now I'd rather we keep this.

Can you add a call to Lwt.apply on the first call to the auxiliary function?

@samer--
Copy link
Contributor Author

samer-- commented May 14, 2024

Thanks!

I think there's one regression: if the first call to seq fails with an exception, then the call to to_list raises an exception (where it used to return a failed promise).

In practice this is probably not too big a deal because the the exception is caught if the call is on the right-on-side of a bind (or inside another closure/lambda). But for now I'd rather we keep this.

Can you add a call to Lwt.apply on the first call to the auxiliary function?

Hi, thanks for reviewing my PR!
I did try to include a call to Lwt.apply when generating the first element of the sequence by using the partially applied Lwt.apply seq as the first argument to aux on line 295. Will this not have the desired effect?

@raphael-proust
Copy link
Collaborator

I did try […]

Ah you're right! I missed this because it's not in the same "shape" as some of the other functions where the recursive function is inlined and the first call has an extra apply.

I think it's all good to go then.

@raphael-proust
Copy link
Collaborator

Thanks for the contribution!

@raphael-proust raphael-proust merged commit a9dcafc into ocsigen:master May 14, 2024
26 checks passed
@samer--
Copy link
Contributor Author

samer-- commented May 14, 2024

Thanks for the contribution!

Thank you for your time reviewing it and for Lwt! All the best.

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.

2 participants