From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id BDA7A70310; Thu, 5 Aug 2021 15:23:06 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org BDA7A70310 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1628166186; bh=Yuupzdmj17o4LERlbjDg0AL9Etx1vkH2mB239je6B4E=; h=Date:To:Cc:References:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=VsNfZ84l0xhHSeoRw2uopAJmGLAK2H09gRcPTmEs7UaAXnLs/nlD8R1GWzpe3/cyS rhQsf+A40jpxm5SKzb4QfAEkF7dhW6ZodW1aBXQH/BEw6iwikklZ079tQv6LPZXk1F W2nqHHxr9Akqtn/jiesjtPCB3PrHva9tZZuTw4Vw= Received: from smtpng1.i.mail.ru (smtpng1.i.mail.ru [94.100.181.251]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 1402A70310 for ; Thu, 5 Aug 2021 15:23:06 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 1402A70310 Received: by smtpng1.m.smailru.net with esmtpa (envelope-from ) id 1mBcOv-0008Mf-4j; Thu, 05 Aug 2021 15:23:05 +0300 Date: Thu, 5 Aug 2021 15:23:03 +0300 To: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org Message-ID: <20210805122303.owbpluoy6sxmglxi@esperanza> References: <8bd6bfd92d7c62f03f9f4d84a65c5727712c428c.1627024646.git.vdavydov@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD92087353F0EC44DD906AB4890CDABF0C5CB76CEE71D3E4007182A05F538085040507CA72BD465D9E0A422BEE0FF0831AAB055B670288ADDFC0E701FE2514BB270 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE771540F9ECFC94C4BEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006374B788E32FE20A07D8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D80EBAF6289780A78895C4769619C6AFD1117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCF1175FABE1C0F9B6A471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352033AC447995A7AD18C26CFBAC0749D213D2E47CDBA5A96583BA9C0B312567BB231DD303D21008E29813377AFFFEAFD269A417C69337E82CC2E827F84554CEF50127C277FBC8AE2E8BA83251EDC214901ED5E8D9A59859A8B6A45692FFBBD75A6A089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-C1DE0DAB: 0D63561A33F958A5471B979BEDD55C3B626753581F405F2D4F9E3253939BD891D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA7501A9DF589746230F410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34047E322BFAFD69BDABAE9DBAC9B46CC46C49DE8736FF4FDDB21EC957D14164A61D227F192681393C1D7E09C32AA3244C204E0088DA5BA7902529DAE3892D75FE1E098CBE561D6343FACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojh4v93/7HD3V0HRkAyvUvnw== X-Mailru-Sender: 689FA8AB762F7393C37E3C1AEC41BA5DB676C15EA3552A6A0DE2476F0DEE35B0274CEFED1673C562683ABF942079399BFB559BB5D741EB966A65DFF43FF7BE03240331F90058701C67EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH 20/20] net.box: do not create request object in Lua for sync requests X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Vladimir Davydov via Tarantool-patches Reply-To: Vladimir Davydov Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 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, ...)