[RDY] atomic multi request for async remote plugins #4568

bfredl · 2016-04-13T15:13:31Z

Based on @ZyX-I:s suggestion #4411 (comment) this allows a rpc client to send a list of requests for evaluation at once. This allows "atomic" execution from an external async context (of course, any request that can lead to the execution of vimscript can and will have side-effects in the middle of executing the multi request). Even if set_var and del_var is changed to not return the oldvalue as suggested in #4411 the old behavior can be kept by bundling a "get_var" and "set_var" request

To support simple test-and-set behavior, it is possible to assert the return value of a request and abort the execution. For anything more complicated it is better to inject some vimscript (or lua) and let it do it.

The argument is an array of [methodname, args, sendreturn{, assertreturn}] entries.
simple example:

vim.api.multi_request([['vim_eval', ['3+3'], False, 6],
                       ['vim_get_current_line', [], True]])

It could be possible to do something like first

get_lines [buffer, -1]
get_linecount [buffer]  
get_var [buffer, "changedtick"]

and then

get_var [buffer, "changedtick"] False oldchangedtick
set_lines [buffer, linenr, oldtext+newtext]
add_highlight [buffer, linenr, ...]

So that the update to the buffer is only done if the buffer was unchanged. The first return value is the number of successful request to this condition can be detected.

Bikeshed of the api and return format welcome. It is quite-low level, but the intention is mainly for client/hosts implementations that can export nicer APIs like list.pop and dict.pop in the case of python, to plugins.

justinmk · 2016-05-28T01:42:13Z

src/nvim/api/vim.c

@@ -607,6 +607,96 @@ Array vim_get_api_info(uint64_t channel_id)
  return rv;
 }

+Array vim_multi_request(uint64_t channel_id, Array calls, Error *err)


vim_batch_call ?

justinmk · 2016-05-28T01:53:26Z

Bikeshed of the api and return format welcome

What's the return format? The general shape looks reasonable. Granular error reporting (eg of the failed assertionwill) help a lot.

equalsraf · 2016-05-28T08:13:07Z

Have not checked thoroughly, but this makes me wonder if anybody out there hasn't looked at msgpack-rpc "transactions" by placing multiple rpc messages in a new message type.

bfredl · 2016-09-14T09:32:00Z

Updated. I removed the preassert for now, this already useful in various cases without it, (such as updating multiple bufhl in an async context without causing excessive redraws, or emulating the old behavior of del_var after #5336) and that could be added in a separate PR without breaking existing usages.

bfredl · 2016-09-24T09:15:12Z

@justinmk I'd like to nominate this for 0.1.6, as said this would be able to fix real performance issues in bufhl (ref arakashic/chromatica.nvim#18), and, say implement vim.dictonary.pop() which is missing in the python-client for some reason, without using deprecated methods. (Though both are in principle possible with vimL helpers, it surely would seem strange if consumers of the new API would need to rely on viml code to be performant)

oni-link · 2016-09-24T12:07:31Z

src/nvim/api/vim.c

+
+    MsgpackRpcRequestHandler handler = msgpack_rpc_get_handler_for(name.data,
+                                                                   name.size);
+    Object result = handler.fn(channel_id, UINT64_MAX, args, &nested_error);


UINT64_MAX -> NO_RESPONSE

I don't like that really, no API function does it yet, but they could use NO_RESPONSE for the optimization "no response exected, don't bother calculating the return value". A separate flag here is probably the best (to use by eval.c:api_wrapper also)

So INTERNAL_RESPONSE is a future advisory flag, it's not used any where yet.

oni-link · 2016-09-24T12:15:29Z

src/nvim/api/vim.c

+    Object result = handler.fn(channel_id, UINT64_MAX, args, &nested_error);
+    if (nested_error.set) {
+      // error handled after loop
+      break;


Is on error result already freed or NIL? Otherwise missing clean up.

it should be an invariant, whenever an api function sets an error, the return value should be NIL (or at least non-allocating).

oni-link · 2016-09-24T12:20:10Z

src/nvim/api/vim.c

+/// with at least three elements: the request name, an array of arguments, and
+/// a boolean indicating if the return value should be sent back.
+///
+/// @return an array with three elements. The first is the number of calls that


In case of a validation error an empty array is returned.

Do you mean validation of the multi_request array itself? Then an error is sent back, nothing can be returned.

oni-link · 2016-09-24T12:22:15Z

src/nvim/api/vim.c

+/// call resulting in an error. The second is an array of return values, for
+/// calls when the third boolean flag was set to true. The third is NIL if all
+/// calls succeeded. If a call resulted in an error, it is a two-element array
+/// with the error code and message.


code -> type ?

oni-link · 2016-09-24T12:33:12Z

src/nvim/api/vim.c

+    }
+
+    if (retval) {
+      ADD(results, result);


If a call fails but a result should be returned, nothing is put in results. Is this expected?

yes, the comlete multi_request is then interrupted, so there is no ambiguity what calls the present elements in results come from.

justinmk · 2016-10-12T21:44:40Z

call_atomic seems best. Though it's inconsistent with the existing call (which only calls VimL functions, not request tuples), there's really no perfect approach here because of the unusual nature of this (we don't have any other API functions that take a request tuple).

bfredl · 2016-10-13T07:46:39Z

Updated. An alternative could be call_methods_atomic if we use the distinction of API methods from vimL functions.

bfredl · 2016-10-15T07:49:07Z

I think it is RDY, if no one objects against call_methods_atomic

justinmk · 2016-10-15T10:05:52Z

I would prefer call_atomic because, I think we should eliminate the arbitrary jargon distinction between "methods " and "functions". It's confusing that we assigned special meaning there. Not all API functions are acting on an object in a meaningful sense, so it's more inside knowledge.

(In the tests I have been meaning to rename "meths" to "api" or something like that)

bfredl · 2016-10-15T10:10:36Z

Well, the object in that case is the nvim instance, but I agree it is a bit internal jargon, so changing back to call_atomic.

justinmk · 2016-10-15T23:56:54Z

src/nvim/api/vim.c

+/// be indicated in the return value, see below.
+///
+/// @return an array with three elements. The first is the number of calls that
+/// where performed without errors. If it is the same as the length of "calls",


Wouldn't need the first item if we instead always return the results.

My other suggestion also does this, but in either case I would then include the index in the error tuple because explicit is better than implicit.

justinmk · 2016-10-15T23:59:27Z

src/nvim/api/vim.c

+///
+/// @param calls an array of calls, where each call is described by an array
+/// with at least three elements: the request name, an array of arguments, and
+/// a boolean indicating if the return value should be sent back.


Could the third item (retval boolean) be a premature optimization? It would greatly simplify explanation/interface of call_atomic if we did not make results optional.

Well, we allready allow invidual calls to drop the return value, seems strange to lose this just because one uses multi-request. But to make bookkeeping simpler we could replace them with NIL so array indexing is consistent.

justinmk · 2016-10-16T00:01:47Z

src/nvim/api/vim.c

+///
+/// This has two main usages: Firstly, to perform several requests from an
+/// async context atomically, i.e. without processing requests from other rpc
+/// clients or redrawing or allowing user interaction in between. Note that api


avoiding redraws is a really nice feature

bfredl · 2016-10-16T05:43:59Z

(In the tests I have been meaning to rename "meths" to "api" or something like that)

👍 I also think we should merge curbufmeths into curbuf so one could curbuf.set_lines

bfredl · 2016-10-17T09:37:26Z

Ok, updated. It is probably simpler to make sure that that all methods where the return value is not essential, will not have a heavy return value, like we did for the set_var methods.

remove unused response_id parameter of handle_nvim_... helpers

bfredl · 2016-10-22T08:53:23Z

Did some more doc adjustments, if no more comments I will merge after tests.

FEATURES: 0b5a7e4 neovim#4432 API: external UIs can render custom popupmenu c6ac4f8 neovim#4934 API: call any API method from vimscript 31df051 neovim#4568 API: nvim_call_atomic(): multiple calls in a single request b268ba3 neovim#5424 API: nvim_win_get_number(), nvim_tabpage_get_number() e7e2844 has("nvim-1.2.3") checks for a specific Nvim version 522b885 neovim#5295, neovim#5493 `:CheckHealth` checks tmux, terminfo, performance 719dae2 neovim#5384 events: allow event processing in getchar() f25797f neovim#5386 API: metadata: Nvim version & API level 22dfe69 neovim#5389 API: metadata: "since", "deprecated_since" 605e743 Added QuickFixLine highlight group CHANGES: 4af6ec7 neovim#5253 perf: Disable clipboard in do_cmdline() 6e9f329 neovim#5299 perf: Skip foldUpdate() in insert-mode. 9d4fcec neovim#5426 perf: Do not auto-update folds for some foldmethods. eeec0ca neovim#5419 tui: Default to normal-mode cursor shape. FIXES: e838452 neovim#5436 tui: Fix "weird characters" / "bleeding termcodes" 10a54ad neovim#5243 signal_init: Always unblock SIGCHLD. bccb49b neovim#5316 eval.c: Fix memory leak for detached pty job 626065d neovim#5227 tchdir: New tab should inherit CWD. cd321b7 neovim#5292 getcwd(): Return empty string if CWD is invalid. 6127eae shada: Fix non-writeable ShaDa directory handling ca65514 neovim#2789 system(): Respect shellxescape, shellxquote 2daf54e neovim#4874 Restore vim-like tab dragging 0c536b5 neovim#5319 syntax.c: Support bg/fg special color-names. 3c53371 neovim#4972 from justinmk/schedule-ui_refresh 68bcb32 neovim#4789 tui.c: Do not wait for tui loop on teardown. c8b6ec2 neovim#5409 v:count broken in command-line window 6bc3bce neovim#5461 fix emoji display 51937e1 neovim#5470 fix :terminal with :argadd, :argu 79d77da neovim#5481 external UIs: opening multiple files from command-line 657ba62 neovim#5501 rplugin: resolve paths in manifest file 6a6f188 neovim#5502 system('foo &', 'bar'): Show error, don't crash. 1ff162c neovim#5515 os_nodetype: open fd with O_NONBLOCK 2a6c5bb neovim#5450 modeline: Handle version number overflow. 0ade1bb neovim#5225 CI tests now run against Windows!

marvim added the WIP label Apr 13, 2016

justinmk reviewed May 28, 2016
View reviewed changes

bfredl mentioned this pull request Aug 16, 2016

Performance improvement arakashic/chromatica.nvim#18

Closed

bfredl force-pushed the multirequest branch from 2c15806 to 1504ebc Compare August 20, 2016 15:19

bfredl mentioned this pull request Aug 29, 2016

[RFC] functions to notify a co-process when the current buffer is modified #5269

Closed

bfredl force-pushed the multirequest branch from 1504ebc to b9aeab9 Compare September 4, 2016 19:20

bfredl mentioned this pull request Sep 14, 2016

[RFC] make del_var and set_var not return the old value. #5336

Merged

bfredl force-pushed the multirequest branch 2 times, most recently from 8ca88b3 to f5ee768 Compare September 14, 2016 09:29

bfredl force-pushed the multirequest branch from f5ee768 to f88350e Compare September 14, 2016 16:31

bfredl changed the title ~~[WIP] atomic multi request (with precondition asserts) for async remote plugins~~ [WIP] atomic multi request for async remote plugins Sep 19, 2016

bfredl changed the title ~~[WIP] atomic multi request for async remote plugins~~ [RFC] atomic multi request for async remote plugins Sep 19, 2016

bfredl force-pushed the multirequest branch 3 times, most recently from e7371f7 to 36caf43 Compare September 19, 2016 16:31

marvim added RFC and removed WIP labels Sep 19, 2016

bfredl force-pushed the multirequest branch from 36caf43 to 67d8737 Compare September 24, 2016 09:07

bfredl force-pushed the multirequest branch 2 times, most recently from bd2e3f3 to 3b03a31 Compare September 24, 2016 10:06

oni-link reviewed Sep 24, 2016

View reviewed changes

bfredl force-pushed the multirequest branch from d211bfb to 7f2163d Compare October 13, 2016 07:45

bfredl force-pushed the multirequest branch from 7f2163d to d17c137 Compare October 14, 2016 14:44

bfredl changed the title ~~[RFC] atomic multi request for async remote plugins~~ [RDY] atomic multi request for async remote plugins Oct 15, 2016

marvim added RDY and removed RFC labels Oct 15, 2016

bfredl force-pushed the multirequest branch from d17c137 to 3ec2bc0 Compare October 15, 2016 10:11

justinmk reviewed Oct 15, 2016

View reviewed changes

justinmk reviewed Oct 16, 2016

View reviewed changes

bfredl force-pushed the multirequest branch 2 times, most recently from d113523 to a7d842a Compare October 17, 2016 09:36

ghost mentioned this pull request Oct 17, 2016

Add support for nvim_call_atomic neovim/go-client#4

Closed

bfredl force-pushed the multirequest branch 3 times, most recently from 2391f3d to f6968dc Compare October 22, 2016 08:51

api: call multiple methods atomically (useful in async contexts)

f6968dc

remove unused response_id parameter of handle_nvim_... helpers

justinmk approved these changes Oct 22, 2016

View reviewed changes

bfredl merged commit 31df051 into neovim:master Oct 22, 2016

jszakmeister removed the RDY label Oct 22, 2016

bfredl mentioned this pull request Oct 23, 2016

RPC/API: Use Cap'n Proto instead of MessagePack #5526

Open

Uh oh!

[RDY] atomic multi request for async remote plugins #4568

[RDY] atomic multi request for async remote plugins #4568

Uh oh!

Conversation

bfredl commented Apr 13, 2016 • edited by justinmk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinmk commented May 28, 2016

Uh oh!

equalsraf commented May 28, 2016

Uh oh!

bfredl commented Sep 14, 2016

Uh oh!

bfredl commented Sep 24, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinmk commented Oct 12, 2016

Uh oh!

bfredl commented Oct 13, 2016

Uh oh!

bfredl commented Oct 15, 2016

Uh oh!

justinmk commented Oct 15, 2016

Uh oh!

bfredl commented Oct 15, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bfredl commented Oct 16, 2016

Uh oh!

bfredl commented Oct 17, 2016

Uh oh!

bfredl commented Oct 22, 2016

Uh oh!

Uh oh!

bfredl commented Apr 13, 2016 •

edited by justinmk

Loading