[tarantool-patches] Re: [PATCH 1/1] netbox: optimize body decoding

[Konstantin Osipov <kostja@tarantool.org>]

* Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [18/05/14 14:44]:
> Part of #3333
> ---
> Branch: https://github.com/tarantool/tarantool/tree/gh-3333-netbox-high-cpu
> Issue: https://github.com/tarantool/tarantool/issues/3333
> 
>  src/box/lua/net_box.c   | 42 ++++++++++++++++++++++++++++++++++++++++
>  src/box/lua/net_box.lua | 51 +++++++++++++++++++++----------------------------
>  2 files changed, 64 insertions(+), 29 deletions(-)
> 
> diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
> index 04fe70b03..3d9479283 100644
> --- a/src/box/lua/net_box.c
> +++ b/src/box/lua/net_box.c
> @@ -38,6 +38,7 @@
>  #include "box/iproto_constants.h"
>  #include "box/lua/tuple.h" /* luamp_convert_tuple() / luamp_convert_key() */
>  #include "box/xrow.h"
> +#include "box/tuple.h"
>  
>  #include "lua/msgpack.h"
>  #include "third_party/base64.h"
> @@ -555,6 +556,46 @@ handle_error:
>  	return 2;
>  }
>  
> +/**
> + * Decode Tarantool response body.
> + * @param Lua stack[1] Raw MessagePack pointer.
> + * @param Lua stack[2] End of body pointer.
> + * @retval Decoded body.
> + */
> +static int
> +netbox_decode_body(struct lua_State *L)
> +{
> +	uint32_t ctypeid;
> +	const char *data = *(const char **)luaL_checkcdata(L, 1, &ctypeid);
> +	const char *end = *(const char **)luaL_checkcdata(L, 2, &ctypeid);
> +	assert(mp_typeof(*data) == MP_MAP);
> +	uint32_t map_size = mp_decode_map(&data);
> +	/* Until 2.0 body has no keys except DATA. */
> +	assert(map_size == 1);
> +	for (uint32_t i = 0; i < map_size; ++i) {
> +		uint32_t key = mp_decode_uint(&data);
> +		if (key == IPROTO_DATA) {
> +			uint32_t count = mp_decode_array(&data);
> +			lua_createtable(L, count, 0);
> +			struct tuple_format *format =
> +				box_tuple_format_default();
> +			for (uint32_t j = 0; j < count; ++j) {
> +				const char *begin = data;
> +				mp_next(&data);
> +				struct tuple *tuple =
> +					box_tuple_new(format, begin, data);
> +				if (tuple == NULL)
> +					luaT_error(L);
> +				luaT_pushtuple(L, tuple);
> +				lua_rawseti(L, -2, j + 1);
> +			}
> +		}
> +	}
> +	if (data != end)
> +		return luaL_error(L, "invalid xrow length");

The message is a bit misleading in net.box context. "Invalid
packet length" perhaps?

Besides, here you use luaL_error(), in other places in the code
you use assert(). Can we be consistent, and use Lua errors
instead, as long as you insist on keeping these checks? The
performance impact will be the same but we'll have a chance to
see/handle this message in production deployments.

Please add a comment why you insist on having this check.

Technically, this condition should never be true. net.box works
with tarantool, and tarantool always returns correct messagepack
(unless there is a bug of course). If the message pack is broken,
we can be PWN!ed, as the comment in decode() indicates. 

If you still insist on having these checks, please add a comment:
-- Keep the check for debug purposes: the condition may be true
-- only in case of Tarantool bug


> +	return 1;
> +}

Please let's have a consistent signature for decode() functions
and decode_body() has the same signature as decode()


The patch looks good overall besides these two issues.

Did you benchmark it? I'd expect a pretty hefty improvement, esp.
in case of large result sets. Care to run a test?

> +
>  int
>  luaopen_net_box(struct lua_State *L)
>  {
> @@ -572,6 +613,7 @@ luaopen_net_box(struct lua_State *L)
>  		{ "encode_auth",    netbox_encode_auth },
>  		{ "decode_greeting",netbox_decode_greeting },
>  		{ "communicate",    netbox_communicate },
> +		{ "decode_body",    netbox_decode_body },
>  		{ NULL, NULL}
>  	};
>  	/* luaL_register_module polutes _G */
> diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
> index 46382129f..194a1c86b 100644
> --- a/src/box/lua/net_box.lua
> +++ b/src/box/lua/net_box.lua
> @@ -27,7 +27,6 @@ local encode_auth     = internal.encode_auth
>  local encode_select   = internal.encode_select
>  local decode_greeting = internal.decode_greeting
>  
> -local sequence_mt      = { __serialize = 'sequence' }
>  local TIMEOUT_INFINITY = 500 * 365 * 86400
>  local VSPACE_ID        = 281
>  local VINDEX_ID        = 289
> @@ -51,30 +50,28 @@ local E_PROC_LUA             = box.error.PROC_LUA
>  local is_final_state         = {closed = 1, error = 1}
>  
>  local function decode_nil(...) end
> -local function decode_nop(...) return ... end
> -local function decode_tuple(response)
> -    if response[1] then
> -        return box.tuple.new(response[1])
> -    end
> +local function decode_data(raw_data, raw_data_end)
> +    local response, real_end = decode(raw_data)
> +    assert(real_end == raw_data_end, "invalid xrow length")
> +    return response[IPROTO_DATA_KEY]
> +end
> +local function decode_tuple(raw_data, raw_data_end)
> +    return internal.decode_body(raw_data, raw_data_end)[1]
>  end
> -local function decode_get(response)
> -    if response[2] then
> +local function decode_get(raw_data, raw_data_end)
> +    local body = internal.decode_body(raw_data, raw_data_end)
> +    if body[2] then
>          return nil, box.error.MORE_THAN_ONE_TUPLE
>      end
> -    if response[1] then
> -        return box.tuple.new(response[1])
> -    end
> +    return body[1]
>  end
> -local function decode_select(response)
> -    setmetatable(response, sequence_mt)
> -    local tnew = box.tuple.new
> -    for i, v in pairs(response) do
> -        response[i] = tnew(v)
> -    end
> -    return response
> +local function decode_select(raw_data, raw_data_end)
> +    return internal.decode_body(raw_data, raw_data_end)
>  end
> -local function decode_count(response)
> -    return response[1]
> +local function decode_count(raw_data, raw_data_end)
> +    local response, real_end = decode(raw_data)
> +    assert(real_end == raw_data_end, "invalid xrow length")
> +    return response[IPROTO_DATA_KEY][1]
>  end
>  
>  local method_encoder = {
> @@ -103,8 +100,8 @@ local method_encoder = {
>  local method_decoder = {
>      ping    = decode_nil,
>      call_16 = decode_select,
> -    call_17 = decode_nop,
> -    eval    = decode_nop,
> +    call_17 = decode_data,
> +    eval    = decode_data,
>      insert  = decode_tuple,
>      replace = decode_tuple,
>      delete  = decode_tuple,
> @@ -115,7 +112,7 @@ local method_decoder = {
>      min     = decode_get,
>      max     = decode_get,
>      count   = decode_count,
> -    inject  = decode_nop,
> +    inject  = decode_data,
>  }
>  
>  local function next_id(id) return band(id + 1, 0x7FFFFFFF) end
> @@ -470,12 +467,8 @@ local function create_transport(host, port, user, password, callback,
>          end
>  
>          -- Decode xrow.body[DATA] to Lua objects
> -        body, body_end_check = decode(body_rpos)
> -        assert(body_end == body_end_check, "invalid xrow length")
> -        if body and body[IPROTO_DATA_KEY] then
> -            request.response, request.errno =
> -                method_decoder[request.method](body[IPROTO_DATA_KEY])
> -        end
> +        request.response, request.errno =
> +            method_decoder[request.method](body_rpos, body_end)
>          request.cond:broadcast()
>      end
>  
> -- 
> 2.15.1 (Apple Git-101)

-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov