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

Remove obsolete second wakeup_paused #917

Merged
merged 8 commits into from
Jul 31, 2023
Merged

Conversation

madroach
Copy link
Contributor

@madroach madroach commented Jan 4, 2022

It was introduced in d5822af.
If I understand correctly this was done to prevent calling select
without timeout while there were pending paused promises.

Since now we explicitely check for this condition and call select with
zero timeout (should_block_waiting_for_io). This extra wakeup_paused
seems to be obsolete.

Maybe in Lwt.pause it should also be documented that Lwt.pause () needs to be used 2 / 3 times to enforce completion of a whole main loop iteration. The current documentation talks about the 'next "tick"'? #916

@raphael-proust
Copy link
Collaborator

If I remember correctly, the backend-agnostic/unix-free pause mechanism was added in order to support js_of_ocaml. Does anyone know if the double waking of paused promise is necessary for compatibility (as in, to produce a more similar scheduling) as on js-of-ocaml? I doubt it because I doubt we have enough control about the scheduling of promises on different js backends that we can even dream about scheduling compatibility.

Ping @vouillon @hhugo

@raphael-proust
Copy link
Collaborator

With this change, the behaviour of pause and yield becomes identical. I think that we should remove the code for yield and simply define yield as an alias for pause.

We could even remove yield altogether because it has been deprecated. But maybe it's worth waiting one or two releases before we remove it to smooth out the transition.

@raphael-proust
Copy link
Collaborator

There needs to be an entry in the CHANGES file. I can add it later before merging if you prefer.

Copy link
Collaborator

@raphael-proust raphael-proust left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I don't know how to use github very well and I mostly had comments not related to the chunks anyway, so I left comments but apparently they don't count as a review but I think this does?)

@madroach
Copy link
Contributor Author

madroach commented Jan 6, 2022

While preparing a CHANGES entry I found this:

Lwt_main.yield and Lwt_unix.yield are deprecated in favor of the generic Lwt.pause, and Lwt_unix.auto_yield is deprecated in favor of the new Lwt_unix.auto_pause. Currently, Lwt_main.run resolves paused promises more frequently than yielded promises; the difference is unintended but existing applications could unintentionally depend on it. (#855, #858, Favonia)

Now I'm wondering in what way existing applications could depend on this behaviour?
I suspect they can't.

@raphael-proust
Copy link
Collaborator

Now I'm wondering in what way existing applications could depend on this behaviour?
I suspect they can't.

Well for example, in your case (#916) you had to use three calls to pause where one call to yield sufficed. There could be an application that depends on this the same way that you do, but maybe does not know about it. As you pointed out it's not well documented/specified.

@madroach
Copy link
Contributor Author

madroach commented Jan 6, 2022

Yes, resolving paused promises more often may break stuff intending to wait for one whole iteration. But resolving less often? No I don't think so.

@madroach
Copy link
Contributor Author

madroach commented Jan 6, 2022

added a CHANGES entry

src/core/lwt.mli Outdated Show resolved Hide resolved
@raphael-proust
Copy link
Collaborator

Hi,

I've added a commit to your branch. We can roll it back or amend it or something. It's more of a proposal/suggestion than anything.

The idea behind it is that pause is now equivalent to yield (modulo the fact that yielded promises are resolved after which paused promises are. (So choose [yield () >|= incr a; pause () >|= incr b; yield () >|= a;] will actually have an intermediate state where a=2 and b=1.) But I think this difference is

  • undocumented
  • not something anyone should rely on in any way
  • not something anyone is likely to rely on in any way

So the commit just makes Lwt_unix.yield = Lwt.pause. And then it can eliminate some unneeded code.

Let me know what you think.

@madroach
Copy link
Contributor Author

I think this is sensible. There is indeed no relevant difference between yield and pause.

@madroach madroach force-pushed the master branch 2 times, most recently from 83ddb0b to d835010 Compare March 2, 2022 18:16
@raphael-proust raphael-proust dismissed their stale review July 17, 2023 15:16

github's UI is confisuin and Idk how to use reviews so I'm just dismissing this

src/unix/lwt_main.mli Outdated Show resolved Hide resolved
src/unix/lwt_main.ml Outdated Show resolved Hide resolved
Christopher Zimmermann and others added 6 commits July 28, 2023 11:20
It was introduced in d5822af.
If I understand it correctly this was done to prevent calling select
without timeout while there were pending paused promises.

Since now we explicitely check for this condition and call select with
zero timeout (`should_block_waiting_for_io`). This extra `wakeup_paused`
seems to be obsolete.
@raphael-proust raphael-proust merged commit 792ab06 into ocsigen:master Jul 31, 2023
22 of 23 checks passed
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.

None yet

3 participants