Asynchronous writes

[BK1603]

Branch for implementing asynchronous writes in neovim.

[BK1603]

@teto, all the commmits from my previous PR were also added, can this be avoided?

EDIT: precious->previous

[teto]

prefix the pr with [WIP]. Yes you can create another branch and git cherry-pick a range of commits.

EDIT: and then force push this same branch with git push -f

[BK1603]

Yes you can create another branch and git cherry-pick a range of commits.

I will fix it and make another PR asap.

EDIT: Also sorry for the mess earlier 😅

[BK1603]

The code is working, I have to add the error display parts and fix the error messages, I can either add it to backup_utils or move those functions back to fileio.c

[BK1603]

There's another thing that needs to be done, since for restoring backups, we would need the backup name, I can make the backup global in the backup_utils.c file and use the restore function over there. That's what I have thought of for now.
What are your opinions on this?

EDIT: But that might end up polluting the global namespace, just like the error variables are doing right now. I think I should do something else for it.

[BK1603]

@teto, Well, I will be working on the error reporting and the restore function now. What would then remain would be the final write function and calling them all asynchronously.

PS: Also this isn't the final code that would be merged, this is just what's working for now, it requires cleanup for the unnecessary headers and the global namespace along with proper error reporting. I just wanted to get your views on the functions created and if something else is required.

And thanks a lot for taking a look! :)

[BK1603]

@teto, A question:

Over here, if we fail copying the original file, we still try to set atime and mtime on the backup file. Is that intentional? What is the point of this if there is no file yet? If this isn't the case then I must have misunderstood something.

[teto]

It does seem weird. I tried a git blame on neovim source code but with little success. Checking against the vim source code (grep for buf_write), it seems to do the same (does not check against vim_rename success). This seems wrong.

[BK1603]

@teto, for the async write, should I use an async flag in the buf_write function and use it to call the functions asynchronously? That could be done either by making more functions for buf_write and queuing them (using uv_work_queue or making a wrapper function that simply calls buf_write asynchronously if async flag is set. What would you suggest?

IMO, just creating a wrapper around buf_write to call it asynchronously would be a bit 'hacky', but doing that is pretty easy and there are very little chances of different behavior. Creating more functions and breaking down buf_write further would help with refactoring, but it would take some time.

[teto]

we can adjust the API later, make sure the behavior is correct first.

[BK1603]

@teto, a status update. I am making functions for asynchronous call right now. I will push as soon as a few of them are callable from a new thread. Also I am looking for better ways of implementing this if possible, since it still isn't too late for that.

Upon trying to implement my approach of libuv work queues I realized that a huge baton would have to be passed around between multiple threads and the entire write function would be executed partly via the functions on a separate thread and partly via the callbacks passed to the work queue.
I currently am working on the callbacks.

Also I think I should use a new loop for this?

EDIT: Breaking down the write function into such chunks is proving to be a rather daunting task, so it might take some more time before I push a commit. I hope that's not a problem. :)

[teto]

what do you mean a huge baton ?

Also I think I should use a new loop for this?

Is the added complexity worth the gain ? we need to make sure what is saved is the content of the buffer when starting to save. Make sure to write some tests if needed and iterate while keeping the tests green.

[BK1603]

what do you mean a huge baton ?

@teto, by the baton I mean the struct that must be maintained for transferring necessary values in
between threads. I think it can be reduced by a bit of careful in place calculation but I am not yet too sure of how much can it be reduced.

Is the added complexity worth the gain ? we need to make sure what is saved is the content of the buffer when starting to save. Make sure to write some tests if needed and iterate while keeping the tests green.

I'll make sure to write a few tests, also for the loop, in which loop should I queue these functions then? I thought having a separate loop for this would work better. I don't think it should be too complex. Just a write_loop containing the work queue for the relevant functions? But I might be misunderstanding something regarding the complexity.

EDIT: I think we should discuss the implementation details once again. Can I DM you? I'd like to discuss the current strategy and what better ways there could be if you're free? I will later comment the approach over here too for anyone else interested in following up. I believe that a discussion would be very helpful for the both of us, as in I'd have a better idea of how to do stuff without increasing the complexity too much, and reviewing this PR would be easier for you. :)

[BK1603]

@teto, here is a broad overview of the current implementation plan:
First, make a backup asynchronously using a new thread. If the backup is successful, initialize write_info and call write. If the backup is not successful, display the error messages and return.
If the backup was successful, write should be called in a new thread. If the write is successful, do whatever we do at the end of write and return. If write isn't successful, restore the backup, display the error messages and return.

This implementation plan requires a struct to pass things around in various threads. Which is what I referred to as the baton. There are some other complications arising in implementing this. The biggest being separating the code into several callable functions that can be called asynchronously. And then there is the question: which loop to use for these functions? I guess we cannot block the main event loop during the I/O, and honestly I am not aware of all the loops being used in neovim. I do know about the existence of the fs_loop, but I think blocking that would mean that we cannot read a new file until the old file is written.

So I am open to any suggestions regarding the loop or even the implementation plan.

EDIT: All the checks regarding the asynchronous calls would have to be done in their respective callbacks. Which mean that the write itself would be split into two parts, the functions executed asynchronously and the callbacks provided to them. So the synchronous write function would be a series of interleaved function and callback calls.