From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Subject: Re: [tarantool-patches] [PATCH v2 1/3] netbox: store method_encoder args in request References: <665fc3a19c40b64da44824a814087c57b3f9ee1b.1561130584.git.imeevma@gmail.com> From: Vladislav Shpilevoy Message-ID: Date: Fri, 21 Jun 2019 22:39:52 +0200 MIME-Version: 1.0 In-Reply-To: <665fc3a19c40b64da44824a814087c57b3f9ee1b.1561130584.git.imeevma@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit To: tarantool-patches@freelists.org, imeevma@tarantool.org Cc: vdavydov.dev@gmail.com List-ID: Hi! Thanks for the patchset! First of all I would like to say, that I impressed by its simplicity, good work. I have only some minor comments except one concerning internal netbox API and perf. See 3 comments below. On 21/06/2019 17:25, imeevma@tarantool.org wrote: > This patch changes the way arguments are passed to functions in > the array method_encoder, and saves these arguments in REQUEST. > This is necessary to establish a connection between netbox space > objects and the tuples obtained through the netbox > > Needed for #2978 > --- > src/box/lua/net_box.lua | 181 ++++++++++++++++++++++++++++++---------------- > test/box/access.result | 15 +++- > test/box/access.test.lua | 9 ++- > test/box/net.box.result | 6 +- > test/box/net.box.test.lua | 6 +- > 5 files changed, 142 insertions(+), 75 deletions(-) > > diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua > index 251ad40..d9838f8 100644 > --- a/src/box/lua/net_box.lua > +++ b/src/box/lua/net_box.lua > @@ -25,7 +25,6 @@ local check_primary_index = box.internal.check_primary_index > > local communicate = internal.communicate > local encode_auth = internal.encode_auth > -local encode_select = internal.encode_select > local decode_greeting = internal.decode_greeting > > local TIMEOUT_INFINITY = 500 * 365 * 86400 > @@ -84,22 +83,63 @@ local function decode_push(raw_data) > return response[IPROTO_DATA_KEY][1], raw_end > end > > +local function encode_call(send_buf, id, args) 1. It would be great to see here a comment on why does all these methods need request args saved. Secondly, why do you need the args for call/eval/ping/execute? These function does not return tuples from a concrete space. For DML/DQL you need format ptr only, so why do you store the whole set of parameters? You could store just one scalar value. See below how. > + return internal.encode_call(send_buf, id, args.func_name, args.args) > +end > +local function encode_call_16(send_buf, id, args) > + return internal.encode_call_16(send_buf, id, args.func_name, args.args) > +end > +local function encode_ping(send_buf, id, ...) > + return internal.encode_ping(send_buf, id, ...) > +end > +local function encode_eval(send_buf, id, args) > + return internal.encode_eval(send_buf, id, args.code, args.args) > +end > +local function encode_insert(send_buf, id, args) > + return internal.encode_insert(send_buf, id, args.space_id, args.tuple) > +end > +local function encode_replace(send_buf, id, args) > + return internal.encode_replace(send_buf, id, args.space_id, args.tuple) > +end > +local function encode_delete(send_buf, id, args) > + return internal.encode_delete(send_buf, id, args.space_id, args.index_id, > + args.key) > +end > +local function encode_update(send_buf, id, args) > + return internal.encode_update(send_buf, id, args.space_id, args.index_id, > + args.key, args.oplist) > +end > +local function encode_upsert(send_buf, id, args) > + return internal.encode_upsert(send_buf, id, args.space_id, args.key, > + args.oplist) > +end > +local function encode_select(send_buf, id, args) > + return internal.encode_select(send_buf, id, args.space_id, args.index_id, > + args.iterator, args.offset, args.limit, > + args.key) > +end > +local function encode_execute(send_buf, id, args) > + return internal.encode_execute(send_buf, id, args.query, args.parameters, > + args.sql_opts) > +end > + > + > local method_encoder = { > - ping = internal.encode_ping, > - call_16 = internal.encode_call_16, > - call_17 = internal.encode_call, > - eval = internal.encode_eval, 2. You could keep these functions as is, couldn't you? The problem with current version is that before your patch method_encoder.call would invoke C function directly. After your patch each 'call' firstly lookups 'internal' table. This is why me and Nick were trying to avoid calling any 'internal' functions via accessing 'internal' table. > - insert = internal.encode_insert, > - replace = internal.encode_replace, > - delete = internal.encode_delete, > - update = internal.encode_update, > - upsert = internal.encode_upsert, > - select = internal.encode_select, > - execute = internal.encode_execute, > - get = internal.encode_select, > - min = internal.encode_select, > - max = internal.encode_select, > - count = internal.encode_call, > + ping = encode_ping, > + call_16 = encode_call_16, > + call_17 = encode_call, > + eval = encode_eval, > + insert = encode_insert, > + replace = encode_replace, > + delete = encode_delete, > + update = encode_update, > + upsert = encode_upsert, > + select = encode_select, > + execute = encode_execute, > + get = encode_select, > + min = encode_select, > + max = encode_select, > + count = encode_call, > -- inject raw data into connection, used by console and tests > inject = function(buf, id, bytes) > local ptr = buf:reserve(#bytes) > @@ -486,7 +526,7 @@ local function create_transport(host, port, user, password, callback, > -- @retval not nil Future object. > -- > local function perform_async_request(buffer, skip_header, method, on_push, > - on_push_ctx, ...) > + on_push_ctx, args) 3. Problem with wrapping each time the parameters into a table is that table wrap is usually orders of magnitude slower than variable args pass via stack. It was benched recently. If the patch had not been pushed, my suggestion would be to make request() method as perform_async_request(buffer, skip_header, method, on_push, on_push_ctx, *request_ctx*, ...) This function internally would do request.ctx = request_ctx That 'ctx' would be ignored by all decoders except DML/DQL. The latter will store format pointer here. For other requests it would be nil, and costs nothing. 'nil' on Lua stack is stored as a special value and does not occupy memory except stack cell. Motivation of my proposal is that netbox *is used* in highload, despite we sometimes forget about it. In vshard netbox is used extremely hard. Each new request table member costs perf and memory. For example, if we have 300k RPS from netbox connections on one instance (it is a real number), and args table pack costs us +1us (in fact even more), then it is not hard to calculate, that each second we will spend nearly 3ms on packing args. Taking into account the fact, that network delays are 'ms' too, it looks quite expensive. https://github.com/tarantool/vshard/issues/186#issuecomment-500380508 > if state ~= 'active' and state ~= 'fetch_schema' then > return nil, box.error.new({code = last_errno or E_NO_CONNECTION, > reason = last_error})