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

async await using libuv #83

Merged
merged 61 commits into from
Apr 1, 2021
Merged

Conversation

oberblastmeister
Copy link
Collaborator

This pr adds async await from this library that I made here. Take a look at the README to see the enormous ergonomic improvement. It is inspired from neovim-async-tutorial and by some parts of neogit.

Features

  • try to have a async await wrapper for every libuv async function that takes a callback (need to wrap more functions)
  • utilities such as sleep (better than vim.wait, does not block interface)
  • condvar (block until a notification is received)
  • channels (block until a value is received)
    • mspc
    • oneshot
    • broadcast (wip)
  • idle abstraction(wip)
  • async await abstraction for jobs (wip)
  • events (such as nvim_buf_attach, filesystem events, timer events) (wip)
    • call await on an event to get it, no need for callbacks
  • more coming

Let me know what you think. This can be thought as a continuation of my previous pr. This will allow much better async abstractions and prevent from going into callback hell. I saw a pr where Tj wanted to use more callbacks and I think that this is much better and will allow to make every part of telescope async. This pr also abstracts over async blocking from things such as condvar and channels which is much more ergonimic using async await than callbacks.

@oberblastmeister oberblastmeister marked this pull request as ready for review February 24, 2021 02:10
@oberblastmeister oberblastmeister changed the title [WIP] async await using libuv async await using libuv Feb 24, 2021
@bfredl
Copy link
Contributor

bfredl commented Feb 25, 2021

In general I like the design, but I see one perf/complexity oversight. Consider the following code from the test:

local read_file = async(function(path)
  local err, fd = await(a.uv.fs_open(path, "r", 438))
  assert(not err, err)

  local err, stat = await(a.uv.fs_fstat(fd))
  assert(not err, err)

  ...

  return data
end)

local first_bench = async(function()
  local contents = await(read_file(the_path))

  ....
end)

When you run(first_bench) this will create the necessary co-routine for first_bench. however await(async(function (path) ... end)(the_path)) will end up allocating a second co-routine for the inner function which is unnecessary as co-routines in luajit are stackful (without using the C stack). I think this situation should be recognized and then implemented as just calling the equivalent of (function(path) ... end)(path) directly.

@oberblastmeister
Copy link
Collaborator Author

@bfredl I am not sure I understand you. async wrapping the function has to create the coroutine because the inner function could yield which is basically await. run(first_bench) and (function(path) .. end)(path) both do not do anything because async functions return a future so you must do run(first_bench()) and (function(path) .. end)(path)(). However doing that will still create the coroutine because async returns a function that returns another function that will execute the function (by creating a coroutine). I am not sure what you mean by when you say that it is creating an unnecessary coroutine. If the function returns a leaf future (for example when we do a.wrap(some libuv async callback function) we already have that optimization. It will not create a coroutine because the libuv function cannot yield or use await and will run the callback when it is done, resuming the parent coroutine.

@bfredl
Copy link
Contributor

bfredl commented Feb 26, 2021

and (function(path) .. end)(path) both do not do anything

No, i meant the inner function without async which even is what you quoted here. This will run inside the co-routine and still be able to await "leaf" futures (or nest even deeper)

@oberblastmeister
Copy link
Collaborator Author

Sorry, I still do not understand. It has to allocate another coroutine or it cannot yield or await

@bfredl
Copy link
Contributor

bfredl commented Feb 26, 2021

Ok, I can show you some actual code example tomorrow :)

@bfredl
Copy link
Contributor

bfredl commented Feb 27, 2021

Ok here is a hack demonstration of the equivalent control flow the optimization should result in:

diff --git a/tests/plenary/async_lib/work_bench.lua b/tests/plenary/async_lib/work_bench.lua
index 7411ccb..76f8365 100644
--- a/tests/plenary/async_lib/work_bench.lua
+++ b/tests/plenary/async_lib/work_bench.lua
@@ -9,7 +9,7 @@ local plenary_init = vim.api.nvim_get_runtime_file('lua/plenary/init.lua', false
 local plenary_dir = vim.fn.fnamemodify(plenary_init, ":h:h:h")
 local assets_dir = plenary_dir .. '/' .. 'tests/plenary/async_lib/assets/'
 
-local read_file = async(function(path)
+local read_file_inner = function(path)
   local err, fd = await(a.uv.fs_open(path, "r", 438))
   assert(not err, err)
 
@@ -23,7 +23,8 @@ local read_file = async(function(path)
   assert(not err, err)
 
   return data
-end)
+end
+local read_file = async(read_file_inner)
 
 local test = async(function()
   local res = await(work.string.match('abcdefg', 'b..'))
@@ -51,7 +52,8 @@ end)
 
 --- readfile asynchronously, string process not async
 local second_bench = async(function()
-  local contents = await(read_file(assets_dir .. 'README.md'))
+  --local contents = await(read_file(assets_dir .. 'README.md'))
+  --the above should be made equivalent to:
+  local contents = read_file_inner(assets_dir .. 'README.md')
 
   local lines = vim.split(contents, '\n')

This executes just fine as you can yield/await a cross a "normal" function call inside a co-routine, and is cheaper than allocating a second co-routine. Essentially, a co-routine should only executed per async "task", not per each async function running inside the same task ( await_all still needs to do the current behavior obviously, as it structurally forks the current task into several when invoked with several async functions)

The problem here is async() hardcodes the use of execute() which is only needed spawn a new task (possibly from another async task), but not just for awaiting a async function implemented using inner yield/await. Instead, I think, this decision should be delayed to when await() or run() is invoked on the async function (while still handling CPS futures efficiently). If you like I could try to sketch this with concrete code.

@oberblastmeister
Copy link
Collaborator Author

@bfredl I think understand what you mean now. So just to make sure the root future will allocate a coroutine. await-ing a non-leaf future will not yield and just call the inner function and not allocate another coroutine. await-ing a leaf future will actually yield. Do not hardcode execute/creating a coroutine and instead leave that up to the run function or join, because we know that whatever future will be passed will be the root future and be a separate task. Some concrete code would be nice.

@oberblastmeister
Copy link
Collaborator Author

@bfredl I implemented your idea. Futures are now tables with a field called leaf, args, and func. Let me know what you think

@runiq
Copy link

runiq commented Mar 25, 2021

Does this PR make #46 obsolete?

@oberblastmeister
Copy link
Collaborator Author

it does

@tjdevries tjdevries merged commit af5356b into nvim-lua:master Apr 1, 2021
@lewis6991
Copy link
Contributor

🎉🎉🎉

@wbthomason
Copy link

This would be a great thing to upstream, if @tjdevries or anyone else is amenable. I'd love to replace packer's bespoke async/await implementation with something like this - standard across Neovim plugins, and built in to the stdlib (since I don't want to give packer any external dependencies).

@tjdevries
Copy link
Member

Yes, the plan is to test drive the library here and bike shed for a bit. Then we can merge to core.

@wbthomason
Copy link

Looking forward to it, then!

delphinus pushed a commit to delphinus/plenary.nvim that referenced this pull request Apr 26, 2021
* started branch

* added bench

* added plenary. ahead

* changed naming

* added work future and test

* fixed await_all, added more benches and tests

* ntoes

* more notes

* added doc

* added M

* added some more uv functions

* start of counting semaphore

* more docs

* use join in run_all

* started branch

* fixed tests

* removed unneeded

* small changes

* async: refactor futures without object

* maded naming more consistent

* added argc

* added argc for wrap

* added argc for all functions

* put in main loop

* made timeout work

* added runned

* removed convert

* added nvim future to be able to call api

* added select

* fixed wrong argc in select function

* added block on

* updated waiting time for blockon

* added protect and block_on

* added api helper

* updated benchs for api

* fixed protected

* validate sender

* add in_fast_event check

* removed unneeded asset file

* removed comment

* change name to scheduler

* removed idle and work related stuff for now

* removed work tests and changed name to util

* added scope and void

* added check to condvar

* removed unnecesary concats

* removed long bench file

* added better errors

* added many docs

* moved block_on and fixed oneshot channel

* added async tests

* updated tests and added describe it

* fixed channel and added more tests

* more tests

* added counter channel

* changed counter api and added tests

* added more deque methods and tests

* added mspc channel

* woops forgot to commit

* remove runned

Co-authored-by: Björn Linse <[email protected]>
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.

8 participants