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 D066E6EC58; Wed, 4 Aug 2021 15:30:05 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org D066E6EC58 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1628080205; bh=01SKBGBa5sO+1nVHlx/aSpLaJpehA/HvVK/TaZZgGWs=; 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=Gl975DkJmUipwRFzoDPErr9afyNeY/vpIn1Nz0NEqgbJQJhIFuyqHsIZypSGxj+Pj KVtcp9pmmTb3b/5eSA6XKNct2sXu58995zMpR3TtvtQJ/ELAULX3qZoo0CRwyP62U8 HRKdlJ4S1Ogm0UTMNCFtH0NfhgH0IG3lm5Jahi2o= 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 0915F6EC58 for ; Wed, 4 Aug 2021 15:30:03 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 0915F6EC58 Received: by smtpng1.m.smailru.net with esmtpa (envelope-from ) id 1mBG26-0002PX-T5; Wed, 04 Aug 2021 15:30:03 +0300 Date: Wed, 4 Aug 2021 15:30:01 +0300 To: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org Message-ID: <20210804123001.3cwudybvkw6dds74@esperanza> References: <903f6291-bab5-985e-f922-042453e4d146@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <903f6291-bab5-985e-f922-042453e4d146@tarantool.org> X-4EC0790: 10 X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD941C43E597735A9C30A5AB0699C09BB51E5FD76225F0C99C3182A05F53808504075965997A329CD3E6D12B4957C6285A96095AC32A25E7637C8B852B68A46605A X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7466896EF24E80F12EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637F08BD7C3AB07DC7F8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D80E2CBBA578A7AD91C446D42B89C178D2117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCF1175FABE1C0F9B6A471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352033AC447995A7AD18CB629EEF1311BF91D2E47CDBA5A96583BA9C0B312567BB231DD303D21008E29813377AFFFEAFD269A417C69337E82CC2E827F84554CEF50127C277FBC8AE2E8BA83251EDC214901ED5E8D9A59859A8B65D56369A3576CBA5089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-C1DE0DAB: 0D63561A33F958A5803970732C21CAC83C0825E5E8E61320BAD7BE8DD057F7F3D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA7501A9DF589746230F410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34D2AC226BFD455724AF0B8DB926105B2ABB2D3566778AC766653E084990F8C8474FAA75D862ED5C621D7E09C32AA3244CBBF6DCB559B1025AAC8D377250CFB23905AB220A9D022EBCFACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojeDyvyeZJDJGy1RCUWZmkyg== X-Mailru-Sender: 689FA8AB762F7393C37E3C1AEC41BA5D1E463F061C4D6FEEECE877834D6AE0C2274CEFED1673C562683ABF942079399BFB559BB5D741EB966A65DFF43FF7BE03240331F90058701C67EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH 15/20] net.box: rewrite request implementation in C 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 Mon, Aug 02, 2021 at 11:54:43PM +0200, Vladislav Shpilevoy wrote: > In an early version I saw on the branch you tried to keep the > discarded requests in the hash. It is not so anymore, correct? > Now discarded requests are GCed even their response didn't arrive > yet? (It should be so, but I decided to clarify just in case.) Both original version and the current version pushed to the branch remove discarded requests from the hash (both luaT_netbox_request_gc and luaT_netbox_request_discard call netbox_request_unregister, which removes the request from the hash table). > > +struct netbox_registry { > > + /* sync -> netbox_request */ > > 1. For out of function comments we usually use /**. This includes the > comments for structs and their members. Fixed here, there, and everywhere. > > +struct netbox_request { > > + /* > > + * Reference to the request result or LUA_NOREF if the response hasn't > > + * been received yet. If the response was decoded to a user-provided > > + * buffer, the result stores the length of the decoded data. > > 2. Do you mean result_ref stores the length? I can't find it in the code. > It always stores a real Lua ref from what I see. Yes, it always stores a Lua ref. In case the result is written to a user-provided buffer, it stores a reference to the result lenght. Updated the comment to clarify that. > > + */ > > + int result_ref; > > + /* > > + * Error if the request failed (ref incremented). NULL on success or if > > + * the response hasn't been received yet. > > + */ > > + struct error *error; > > + > > 3. Extra empty line. Removed. > > +static bool > > +netbox_request_is_ready(struct netbox_request *request) > > 4. The request can be made 'const'. Added. > > +static bool > > +netbox_request_wait(struct netbox_request *request, double *timeout) > > +{ > > + double ts = ev_monotonic_now(loop()); > > 5. fiber_clock() might be a little shorter (does the same). The same below. > Although it is not inline. Up to you. Also see a related comment below. We use ev_monotonic_now in more places in the code so I'm going to stick to it: ~/src/tarantool$ git grep fiber_clock src | grep -c -v -e '\.lua:' 19 ~/src/tarantool$ git grep ev_monotonic_now src | grep -c -v -e '\.lua:' 60 > > 6. Have you tried making these 1-5 line functions explicitly inline? > I remember with Mergen we saw some actual perf difference in SQL code > when did so. Although that time the functions were in the header vs in > a .c file. > > I see you used inline for some functions in this commit. Why not for > the ones like these? Marked all short, frequently called functions 'inline'. It didn't affect the test results though. > > +static int > > +luaT_netbox_request_wait_result(struct lua_State *L) > > +{ > > + struct netbox_request *request = luaT_check_netbox_request(L, 1); > > + double timeout = TIMEOUT_INFINITY; > > + if (!lua_isnoneornil(L, 2)) { > > + if (lua_type(L, 2) != LUA_TNUMBER || > > + (timeout = lua_tonumber(L, 2)) < 0) > > + luaL_error(L, "Usage: future:wait_result(timeout)"); > > + } > > + while (!netbox_request_is_ready(request)) { > > + if (!netbox_request_wait(request, &timeout)) { > > + luaL_testcancel(L); > > + diag_set(ClientError, ER_TIMEOUT); > > 7. In some places you use box_error_raise(), in others the > explicit diag_set(ClientError). Why? For instance: > > box_error_raise(ER_NO_CONNECTION, "%s", strerror(errno)); > box_error_raise(ER_NO_CONNECTION, "Peer closed"); > > In others the raise is justified because you do not know the > error code and its message at compile time there. In these 2 > I probably do not know something? box_error_raise() allows you to set an arbitrary error message while diag_set() uses error messages corresponding to error codes (src/box/errcode.h). E.g. for ER_NO_CONNECTION, it's always "Connection is not established". I use box_error_raise() here in order to avoid changing error messages which were reported by Lua code originally. > > +static int > > +luaT_netbox_request_pairs(struct lua_State *L) > > +{ > > + if (!lua_isnoneornil(L, 2)) { > > + if (lua_type(L, 2) != LUA_TNUMBER || lua_tonumber(L, 2) < 0) > > + luaL_error(L, "Usage: future:pairs(timeout)"); > > + } else { > > + if (lua_isnil(L, 2)) > > + lua_pop(L, 1); > > + lua_pushnumber(L, TIMEOUT_INFINITY); > > + } > > + lua_pushcclosure(L, luaT_netbox_request_iterator_next, 2); > > 8. Push of cfunctions, especially closures, on a regular basis might > be expensive. Could you please try to make it a non-closure function > and cache its reference like I showed in the proposal about request __gc? My test doesn't check performance of the request iterator so I need to come up with a new test first to check this hypothesis. I'll reply to this email with the results once they're ready. > > +static int > > +netbox_new_request(struct lua_State *L) > > +{ > > + struct netbox_request *request = lua_newuserdata(L, sizeof(*request)); > > 9. Does it help perf if the requests are allocated on mempool? I tried to allocate requests on mempool. To achieve that, we have to turn them into C data. It resulted in a performance degradation, see the parallel thread: https://lists.tarantool.org/pipermail/tarantool-patches/2021-July/025039.html The code is here (last two commits): https://github.com/tarantool/tarantool/tree/vdavydov/net-box-optimization-cdata-request Lua does a lot of malloc calls under the hood. Eliminating just this one newuserdata doesn't seem to make any difference. > > + netbox_request_create(request); > > 10. Below you override a lot of fields initialized in > netbox_request_create(). Do you really need this create(), can you > inline it without the unnecessary assignments? Right. Removed netbox_request_create. > > + luaL_getmetatable(L, netbox_request_typename); > > + lua_setmetatable(L, -2); > > + struct netbox_registry *registry = luaT_check_netbox_registry(L, 1); > > + request->sync = luaL_touint64(L, 2); > > + request->buffer = (struct ibuf *) lua_topointer(L, 3); > > + lua_pushvalue(L, 3); > > + request->buffer_ref = luaL_ref(L, LUA_REGISTRYINDEX); > > + request->skip_header = lua_toboolean(L, 4); > > + request->method = lua_tointeger(L, 5); > > + assert(request->method < netbox_method_MAX); > > + lua_pushvalue(L, 6); > > + request->on_push_ref = luaL_ref(L, LUA_REGISTRYINDEX); > > + lua_pushvalue(L, 7); > > + request->on_push_ctx_ref = luaL_ref(L, LUA_REGISTRYINDEX); > > + if (!lua_isnil(L, 8)) > > + request->format = lbox_check_tuple_format(L, 8); > > + else > > + request->format = tuple_format_runtime; > > + tuple_format_ref(request->format); > > + if (netbox_request_register(request, registry) != 0) > > + luaT_error(L); > > + return 1; > > +} > > diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua > > index 0ad6cac022f2..4bc66940ea2a 100644 > > --- a/src/box/lua/net_box.lua > > +++ b/src/box/lua/net_box.lua > > @@ -468,20 +304,8 @@ local function create_transport(host, port, user, password, callback, > > local id = next_request_id > > encode_method(method, send_buf, id, ...) > > next_request_id = next_id(id) > > - -- Request in most cases has maximum 10 members: > > - -- method, buffer, skip_header, id, cond, errno, response, > > - -- on_push, on_push_ctx and format. > > - local request = setmetatable(table_new(0, 10), request_mt) > > - request.method = method > > - request.buffer = buffer > > - request.skip_header = skip_header > > - request.id = id > > - request.cond = fiber.cond() > > - requests[id] = request > > - request.on_push = on_push > > - request.on_push_ctx = on_push_ctx > > - request.format = format > > - return request > > + return internal.new_request(requests, id, buffer, skip_header, method, > > + on_push, on_push_ctx, format) > > 11. You might want to try to cache internal.new_request globally, > use it without '.' operator, and re-check the benches. Might be nothing > or not, but '.' is definitely not free. The same for > dispatch_response_iproto, dispatch_response_console. Replied in the parallel thread: https://lists.tarantool.org/pipermail/tarantool-patches/2021-August/025137.html > > - msg, real_end, request.errno = decode_push(body_rpos, body_end) > > 12. decode_push is not unused. Deleted it. The incremental diff is below. -- diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c index 6859e955ac6e..e7cb5b3d810f 100644 --- a/src/box/lua/net_box.c +++ b/src/box/lua/net_box.c @@ -79,79 +79,63 @@ enum netbox_method { }; struct netbox_registry { - /* sync -> netbox_request */ + /** sync -> netbox_request */ struct mh_i64ptr_t *requests; }; struct netbox_request { enum netbox_method method; - /* + /** * Unique identifier needed for matching the request with its response. * Used as a key in the registry. */ uint64_t sync; - /* + /** * The registry this request belongs to or NULL if the request has been * completed. */ struct netbox_registry *registry; - /* Format used for decoding the response (ref incremented). */ + /** Format used for decoding the response (ref incremented). */ struct tuple_format *format; - /* Signaled when the response is received. */ + /** Signaled when the response is received. */ struct fiber_cond cond; - /* + /** * A user-provided buffer to which the response body should be copied. * If NULL, the response will be decoded to Lua stack. */ struct ibuf *buffer; - /* + /** * Lua reference to the buffer. Used to prevent garbage collection in * case the user discards the request. */ int buffer_ref; - /* + /** * Whether to skip MessagePack map header and IPROTO_DATA key when * copying the response body to a user-provided buffer. Ignored if * buffer is not set. */ bool skip_header; - /* Lua references to on_push trigger and its context. */ + /** Lua references to on_push trigger and its context. */ int on_push_ref; int on_push_ctx_ref; - /* - * Reference to the request result or LUA_NOREF if the response hasn't - * been received yet. If the response was decoded to a user-provided - * buffer, the result stores the length of the decoded data. + /** + * Lua reference to the request result or LUA_NOREF if the response + * hasn't been received yet. If the response was decoded to a + * user-provided buffer (see buffer_ref), result_ref stores a Lua + * reference to an integer value that contains the length of the + * decoded data. */ int result_ref; - /* + /** * Error if the request failed (ref incremented). NULL on success or if * the response hasn't been received yet. */ struct error *error; - }; static const char netbox_registry_typename[] = "net.box.registry"; static const char netbox_request_typename[] = "net.box.request"; -static void -netbox_request_create(struct netbox_request *request) -{ - request->method = netbox_method_MAX; - request->sync = -1; - request->registry = NULL; - request->format = NULL; - fiber_cond_create(&request->cond); - request->buffer = NULL; - request->buffer_ref = LUA_REFNIL; - request->skip_header = false; - request->on_push_ref = LUA_REFNIL; - request->on_push_ctx_ref = LUA_REFNIL; - request->result_ref = LUA_NOREF; - request->error = NULL; -} - static void netbox_request_destroy(struct netbox_request *request) { @@ -206,19 +190,19 @@ netbox_request_unregister(struct netbox_request *request) mh_i64ptr_del(h, k, NULL); } -static bool -netbox_request_is_ready(struct netbox_request *request) +static inline bool +netbox_request_is_ready(const struct netbox_request *request) { return request->registry == NULL; } -static void +static inline void netbox_request_signal(struct netbox_request *request) { fiber_cond_broadcast(&request->cond); } -static void +static inline void netbox_request_complete(struct netbox_request *request) { netbox_request_unregister(request); @@ -229,7 +213,7 @@ netbox_request_complete(struct netbox_request *request) * Waits on netbox_request::cond. Subtracts the wait time from the timeout. * Returns false on timeout or if the fiber was cancelled. */ -static bool +static inline bool netbox_request_wait(struct netbox_request *request, double *timeout) { double ts = ev_monotonic_now(loop()); @@ -238,14 +222,14 @@ netbox_request_wait(struct netbox_request *request, double *timeout) return rc == 0; } -static void +static inline void netbox_request_set_result(struct netbox_request *request, int result_ref) { assert(request->result_ref == LUA_NOREF); request->result_ref = result_ref; } -static void +static inline void netbox_request_set_error(struct netbox_request *request, struct error *error) { assert(request->error == NULL); @@ -297,7 +281,7 @@ netbox_registry_destroy(struct netbox_registry *registry) /** * Looks up a request by id (sync). Returns NULL if not found. */ -static struct netbox_request * +static inline struct netbox_request * netbox_registry_lookup(struct netbox_registry *registry, uint64_t sync) { struct mh_i64ptr_t *h = registry->requests; @@ -1637,9 +1621,6 @@ static int netbox_new_request(struct lua_State *L) { struct netbox_request *request = lua_newuserdata(L, sizeof(*request)); - netbox_request_create(request); - luaL_getmetatable(L, netbox_request_typename); - lua_setmetatable(L, -2); struct netbox_registry *registry = luaT_check_netbox_registry(L, 1); request->sync = luaL_touint64(L, 2); request->buffer = (struct ibuf *)lua_topointer(L, 3); @@ -1657,8 +1638,15 @@ netbox_new_request(struct lua_State *L) else request->format = tuple_format_runtime; tuple_format_ref(request->format); - if (netbox_request_register(request, registry) != 0) + fiber_cond_create(&request->cond); + request->result_ref = LUA_NOREF; + request->error = NULL; + if (netbox_request_register(request, registry) != 0) { + netbox_request_destroy(request); luaT_error(L); + } + luaL_getmetatable(L, netbox_request_typename); + lua_setmetatable(L, -2); return 1; } diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua index 1379d04268d9..20dec5648926 100644 --- a/src/box/lua/net_box.lua +++ b/src/box/lua/net_box.lua @@ -68,11 +68,6 @@ local M_INJECT = 17 -- utility tables local is_final_state = {closed = 1, error = 1} -local function decode_push(raw_data) - local response, raw_end = decode(raw_data) - return response[IPROTO_DATA_KEY][1], raw_end -end - local function version_id(major, minor, patch) return bit.bor(bit.lshift(major, 16), bit.lshift(minor, 8), patch) end