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

Some simplifications #784

Merged
merged 2 commits into from
Aug 2, 2020
Merged

Conversation

raphael-proust
Copy link
Collaborator

Two relatively simple modifications. (Albeit one is simple but not trivial.)

The first commit implements Lwt_main.yield as a call to Lwt.pause and removes all the support code associated with the previous implementation. This is not trivial and may have subtle effects on the scheduling performed in Lwt_main.run.

The second commit simply avoids some List.rev calls when traversing lists (iter_p, iteri_p, etc.).

@aantron
Copy link
Collaborator

aantron commented May 21, 2020

The second commit looks good. Thanks!

I want to note for future reference, that it changes the order in which join and internal collect functions visit promises. However, this order was never specified, or, in the case of join, explicitly defined as arbitrary.

The first commit should be fine, but I hesitate to merge it, because it may have some subtle implications. I suggest replacing it with a comment explaining that yield is really the same as pause, but exposed and implemented separately for historical reasons. Perhaps the comment should link to this PR and/or the earlier discussion.

@raphael-proust
Copy link
Collaborator Author

The second commit looks good. Thanks!

I want to note for future reference, that it changes the order in which join and internal collect functions visit promises. However, this order was never specified, or, in the case of join, explicitly defined as arbitrary.

I added a note in the CHANGES file, not sure if that's how you want to keep track of it. If not let me know and I can remove that part.

The first commit should be fine, but I hesitate to merge it, because it may have some subtle implications. I suggest replacing it with a comment explaining that yield is really the same as pause, but exposed and implemented separately for historical reasons. Perhaps the comment should link to this PR and/or the earlier discussion.

I'll make that change. How strongly should I encourage people to use pause rather than yield? Do we want to deprecate yield altogether or just point at pause?

It could be good to ask people to try an run their big project on the yield-is-pause variant and see if anything breaks. A bit like the opam switch thing on the compiler variants.

@raphael-proust raphael-proust force-pushed the some-simplifications branch 2 times, most recently from 9eda7ee to 2b67f2d Compare May 25, 2020 08:44
@raphael-proust
Copy link
Collaborator Author

I just thought of one possible way to keep a fully backwards-compatible semantics for parallel iterators: provide an alternatvie to join that captures the last exception rather than the first. This new version is very similar except for:

-        match !join_result with
-        | Fulfilled () -> join_result := new_result
-        | Rejected _ -> ()
+        join_result := new_result

I don't know if it is worth adding a new function that behaves so similarly to join. In fact, the documentation of join does say that it selects an arbitrary rejections amongst all the available one. So the intended semantics of this new function, its documentation even, would be the same as with vanilla join.

IMO it's not worth it. (IMO we should randomly select either of the join variants at compile time just to actually be arbitrary, but that's a bit mean…)

@aantron aantron merged commit 860c7e0 into ocsigen:master Aug 2, 2020
@aantron
Copy link
Collaborator

aantron commented Aug 2, 2020

Thanks!

IMO it's not worth it.

[On adding a new function] I agree.

I added a note in the CHANGES file, not sure if that's how you want to keep track of it. If not let me know and I can remove that part.

I usually write/rewrite the changelog right before release, so this is fine :)

How strongly should I encourage people to use pause rather than yield? Do we want to deprecate yield altogether or just point at pause?

It would probably be too noisy to attach a deprecation annotation to it, so I think the way you have the docs recommend pause in this PR is 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