[Tarantool-patches] [PATCH 20/20] net.box: do not create request object in Lua for sync requests
Vladimir Davydov
vdavydov at tarantool.org
Thu Aug 5 15:23:03 MSK 2021
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, ...)
More information about the Tarantool-patches
mailing list