From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 4975023BB4 for ; Tue, 15 May 2018 14:38:34 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id J9tsluF95JTW for ; Tue, 15 May 2018 14:38:34 -0400 (EDT) Received: from smtp52.i.mail.ru (smtp52.i.mail.ru [94.100.177.112]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 02E8F23B46 for ; Tue, 15 May 2018 14:38:33 -0400 (EDT) Date: Tue, 15 May 2018 21:38:31 +0300 From: Konstantin Osipov Subject: [tarantool-patches] Re: [PATCH 1/1] netbox: optimize body decoding Message-ID: <20180515183831.GA11426@atlas> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: Vladislav Shpilevoy Cc: tarantool-patches@freelists.org * Vladislav Shpilevoy [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