-
Notifications
You must be signed in to change notification settings - Fork 715
feat: monadic interface for asynchronous operations in Std #8003
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
Conversation
|
Mathlib CI status (docs):
|
hargoniX
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which types in this PR are the intended surface level API? Generally people seem to expect one type that can be awaited (that's presumably Async?) and one type they can use for concurrent units of work (that's presumably AsyncTask?). I guess we should probably make this sort of intent clear? Though it should hopefully become clear based on tests/examples that end up being written later on.
Based on this I do wonder how far we want to align the rest of the async API that we have built so far with this Async type. Do you think e.g. the functions we've built so far in TCP/UDP/Timers should be Async instead of AsyncTask?
Beyond that as discussed with the user feedback on discord renaming parallel is probably a good idea but that's already on the TODO list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would also be great if there was some sort of documentation, either in the code or this PR body, that actually explains the intentions behind all of these classes etc. So far with mini-redis I've mostly been guessing based on the test files here and conversations with you.
I also wonder about the planned integration with the existing async IO things, are we going to migrate those to Async? Are they going to stay AsyncTask?
As a last technical remark: I've been wondering in mini-redis how to do the following: When I'm in a monad stack that contains Async at the bottom and I end up producing a raw Async, how can I run that as a background computation? Running async on it doesn't seem to work and neither does discard.
Style wise the file looks "quite Haskell" which is not something we usually do but I'm not generally opposed to it, maybe other people have strong opinions here.
src/Std/Internal/Async/Basic.lean
Outdated
| instance [inst : MonadAsync t m] [Monad m] : MonadAsync t (ReaderT n m) where | ||
| async p := inst.async ∘ p | ||
|
|
||
| instance [inst : MonadAsync t m] [Monad m] : MonadAsync t (StateRefT' s n m) where |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed in DMs I'm not 100% sure about this instance, if we spawn a couple of computations in a StateRefT with this they are all going to be referring to the same IO.Ref and thus race all the times no? Also why does StateRefT have an instance but not StateT and the others above that have instances?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if we should provide StateRefT and StateT instances. These instances behave weirdly with Async, so I think we should only provide an instance for ReaderT, just to make it easier to have some kind of context with Mutexes.
tydeu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! I have a few minor suggestions and one major one, but I am otherwise quite happy with how this is turning out. anxiously await being able to use this API in Lake to improve the codebase's readability!
tydeu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks even better! 😄
Co-authored-by: Mac Malone <[email protected]>
Co-authored-by: Mac Malone <[email protected]>
TwoFX
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Feel free to merge after adressing the style nit.
This PR adds a new monadic interface for
Asyncoperations.This is the design for the
Asyncmonad that I liked the most. The idea was refined with the help of @tydeu. Before that, I had some prerequisites in mind:yieldpoints, so we could avoid usingbindTaskfor every lifted IO operationTasks during recursionThe 2 and 3 points are not covered in this PR, I wish I had a good solution but right now only a few sketches of this.
Explicit
yieldpointsI thought this would be easy at first, but it actually turned out kinda tricky. I ended up creating the
suspendsyntax, which is just a small modification of the lift method (<- ...) syntax. It desugars toSuspend.suspend task fun _ => .... So something like:Would become:
This makes things a bit more efficient. When using
bind, we would try to avoid creating aTaskchain, and thesuspendwould be the only place we useTask.bind. But there's a problem if we usebindwith something that needssuspend, it’ll block the whole task. Blocking is the only way to prevent task accumulation when using plainbindinside a structure like that:Because we simply need to remove the
ofTaskand transform it into anok.Infinite chain of Tasks
If you create an infinite recursive function using
Task(which is super common in servers like HTTP ones), it can lead to a lot of memory usage. Because those tasks get chained forever and won't be freed until the function returns.To get around that, I used CPS and instead of just calling
Task.bind, I’d spawn a new task and return an "empty" one like:fun k => Task.bind (...) fun value => do k value; pure emptyTaskThis works great with a CPS-style monad, but it generates a huge IR by itself.
Just doing CPS alone was too much, though, because every lifted operation created a new continuation and a
Task.bind. So, I used it withsuspendand got a better performance, but the usage is not good withsuspend.The current monad
Right now, the monad I’m using is super simple. It doesn't solve the earlier problems, but the API is clean, and the generated IR is small enough. An example of how we should use it is: