[tarantool-patches] [PATCH v2 1/3] netbox: store method_encoder args in request

Vladislav Shpilevoy v.shpilevoy at tarantool.org
Fri Jun 21 23:39:52 MSK 2019


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 at 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})



More information about the Tarantool-patches mailing list