Re: [Tarantool-patches] [PATCH 20/20] net.box: do not create request object in Lua for sync requests

[Vladimir Davydov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

On Wed, Aug 04, 2021 at 01:09:52AM +0200, Vladislav Shpilevoy wrote:
> > -static int
> > -netbox_perform_async_request(struct lua_State *L)
> > +static void
> > +netbox_perform_async_request_impl(struct lua_State *L, int idx,
> > +				  struct netbox_request *request)
> 
> 1. Impl is a confusing name here. Not clear what it does
> exactly. I propose netbox_make_request() or netbox_send_request()
> or netbox_push_request() or something likewise.

netbox_make_request it is then

> 
> >  {
> > -	struct netbox_request *request = lua_newuserdata(L, sizeof(*request));
> > -	netbox_request_create(request);
> > -	luaL_getmetatable(L, netbox_request_typename);
> > -	lua_setmetatable(L, -2);
> > -
> >  	/* Encode and write the request to the send buffer. */
> > -	struct netbox_registry *registry = luaT_check_netbox_registry(L, 1);
> > -	struct ibuf *send_buf = (struct ibuf *) lua_topointer(L, 2);
> > -	enum netbox_method method = lua_tointeger(L, 5);
> > +	struct netbox_registry *registry = luaT_check_netbox_registry(L, idx);
> > +	struct ibuf *send_buf = (struct ibuf *) lua_topointer(L, idx + 1);
> > +	enum netbox_method method = lua_tointeger(L, idx + 4);
> >  	assert(method < netbox_method_MAX);
> >  	uint64_t sync = registry->next_sync++;
> > -	netbox_encode_method(L, 9, method, send_buf, sync);
> > +	netbox_encode_method(L, idx + 8, method, send_buf, sync);
> >  
> >  	/* Initialize and register the request object. */
> >  	request->method = method;
> >  	request->sync = sync;
> > -	request->buffer = (struct ibuf *) lua_topointer(L, 3);
> > -	lua_pushvalue(L, 3);
> > +	request->buffer = (struct ibuf *) lua_topointer(L, idx + 2);
> > +	lua_pushvalue(L, idx + 2);
> >  	request->buffer_ref = luaL_ref(L, LUA_REGISTRYINDEX);
> > -	request->skip_header = lua_toboolean(L, 4);
> > -	lua_pushvalue(L, 6);
> > +	request->skip_header = lua_toboolean(L, idx + 3);
> > +	lua_pushvalue(L, idx + 5);
> >  	request->on_push_ref = luaL_ref(L, LUA_REGISTRYINDEX);
> > -	lua_pushvalue(L, 7);
> > +	lua_pushvalue(L, idx + 6);
> >  	request->on_push_ctx_ref = luaL_ref(L, LUA_REGISTRYINDEX);
> > -	if (!lua_isnil(L, 8))
> > -		request->format = lbox_check_tuple_format(L, 8);
> > +	if (!lua_isnil(L, idx + 7))
> > +		request->format = lbox_check_tuple_format(L, idx + 7);
> 
> 2. To avoid most of that diff could you make the index relative from
> the beginning? Before this commit you could make 'int idx = 1;' and
> then for each next parameter use 'idx++'. Then this commit wouldn't
> change much here.

The arguments are accessed non-sequentially so idx++ won't work.
(To encode a request we need arguments 1, 2, 5, and 9... and encoding
should be called before creating a request handle, because it may
fail. The argument order comes from Lua net.box functions. I don't want
to reorder arguments in Lua at this point.)

Anyway, I don't think we should care much about intermediate diffs so
let's please leave it as is.

> > +static int
> > +netbox_perform_request(struct lua_State *L)
> > +{
> > +	double timeout = (!lua_isnil(L, 1) ?
> > +			  lua_tonumber(L, 1) : TIMEOUT_INFINITY);
> 
> 2. This piece of code is repeated 6 times in the file. Maybe
> time to make it a function?

Not anymore. I removed timeouts from all IO functions.

> > -    local function perform_async_request(buffer, skip_header, method, on_push,
> > -                                         on_push_ctx, format, ...)
> > +    local function check_active()
> >          if state ~= 'active' and state ~= 'fetch_schema' then
> >              local code = last_errno or E_NO_CONNECTION
> >              local msg = last_error or
> >                  string.format('Connection is not established, state is "%s"',
> >                                state)
> > -            return nil, box.error.new({code = code, reason = msg})
> > +            return box.error.new({code = code, reason = msg})
> > +        end
> > +        return nil
> 
> 3. You can drop this return, it does not do anything (given how
> the function is used).

Dropped.

> > +    end
> > @@ -284,13 +292,18 @@ local function create_transport(host, port, user, password, callback,
> >      --
> >      local function perform_request(timeout, buffer, skip_header, method,
> >                                     on_push, on_push_ctx, format, ...)
> > -        local request, err =
> > -            perform_async_request(buffer, skip_header, method, on_push,
> > -                                  on_push_ctx, format, ...)
> > -        if not request then
> > +        local err = check_active()
> > +        if err then
> >              return nil, err
> >          end
> > -        return request:wait_result(timeout)
> > +        -- alert worker to notify it of the queued outgoing data;
> > +        -- if the buffer wasn't empty, assume the worker was already alerted
> > +        if send_buf:size() == 0 then
> > +            worker_fiber:wakeup()
> > +        end
> 
> 4. Why not extract it all into a function along with check_active?
> Such as 'prepare_request()' or something. These blocks until their
> 'return's are exactly the same.

Sure, done.

Incremental diff:
--
diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
index 282e23716905..06e574cdf746 100644
--- a/src/box/lua/net_box.c
+++ b/src/box/lua/net_box.c
@@ -1572,8 +1572,8 @@ netbox_new_registry(struct lua_State *L)
  *  - ...: method-specific arguments passed to the encoder
  */
 static void
-netbox_perform_async_request_impl(struct lua_State *L, int idx,
-				  struct netbox_request *request)
+netbox_make_request(struct lua_State *L, int idx,
+		    struct netbox_request *request)
 {
 	/* Encode and write the request to the send buffer. */
 	struct netbox_registry *registry = luaT_check_netbox_registry(L, idx);
@@ -1612,7 +1612,7 @@ static int
 netbox_perform_async_request(struct lua_State *L)
 {
 	struct netbox_request *request = lua_newuserdata(L, sizeof(*request));
-	netbox_perform_async_request_impl(L, 1, request);
+	netbox_make_request(L, 1, request);
 	luaL_getmetatable(L, netbox_request_typename);
 	lua_setmetatable(L, -2);
 	return 1;
@@ -1624,7 +1624,7 @@ netbox_perform_request(struct lua_State *L)
 	double timeout = (!lua_isnil(L, 1) ?
 			  lua_tonumber(L, 1) : TIMEOUT_INFINITY);
 	struct netbox_request request;
-	netbox_perform_async_request_impl(L, 2, &request);
+	netbox_make_request(L, 2, &request);
 	while (!netbox_request_is_ready(&request)) {
 		if (!netbox_request_wait(&request, &timeout)) {
 			netbox_request_unregister(&request);
diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
index 9da900933fbc..0bb7ff3ba3a7 100644
--- a/src/box/lua/net_box.lua
+++ b/src/box/lua/net_box.lua
@@ -253,7 +253,7 @@ local function create_transport(host, port, user, password, callback,
         end
     end
 
-    local function check_active()
+    local function prepare_perform_request()
         if state ~= 'active' and state ~= 'fetch_schema' then
             local code = last_errno or E_NO_CONNECTION
             local msg = last_error or
@@ -261,7 +261,11 @@ local function create_transport(host, port, user, password, callback,
                               state)
             return box.error.new({code = code, reason = msg})
         end
-        return nil
+        -- alert worker to notify it of the queued outgoing data;
+        -- if the buffer wasn't empty, assume the worker was already alerted
+        if send_buf:size() == 0 then
+            worker_fiber:wakeup()
+        end
     end
 
     --
@@ -271,15 +275,10 @@ local function create_transport(host, port, user, password, callback,
     --
     local function perform_async_request(buffer, skip_header, method, on_push,
                                          on_push_ctx, format, ...)
-        local err = check_active()
+        local err = prepare_perform_request()
         if err then
             return nil, err
         end
-        -- alert worker to notify it of the queued outgoing data;
-        -- if the buffer wasn't empty, assume the worker was already alerted
-        if send_buf:size() == 0 then
-            worker_fiber:wakeup()
-        end
         return internal.perform_async_request(requests, send_buf, buffer,
                                               skip_header, method, on_push,
                                               on_push_ctx, format, ...)
@@ -292,15 +291,10 @@ local function create_transport(host, port, user, password, callback,
     --
     local function perform_request(timeout, buffer, skip_header, method,
                                    on_push, on_push_ctx, format, ...)
-        local err = check_active()
+        local err = prepare_perform_request()
         if err then
             return nil, err
         end
-        -- alert worker to notify it of the queued outgoing data;
-        -- if the buffer wasn't empty, assume the worker was already alerted
-        if send_buf:size() == 0 then
-            worker_fiber:wakeup()
-        end
         return internal.perform_request(timeout, requests, send_buf, buffer,
                                         skip_header, method, on_push,
                                         on_push_ctx, format, ...)