* [tarantool-patches] Re: [PATCH 1/1] netbox: optimize body decoding
2018-05-14 11:41 [tarantool-patches] [PATCH 1/1] netbox: optimize body decoding Vladislav Shpilevoy
@ 2018-05-15 18:31 ` Vladislav Shpilevoy
2018-05-15 18:38 ` Konstantin Osipov
1 sibling, 0 replies; 4+ messages in thread
From: Vladislav Shpilevoy @ 2018-05-15 18:31 UTC (permalink / raw)
To: tarantool-patches; +Cc: kostja
Hello.
> Константин Осипов, [15 мая 2018 г., 21:16:08]:
> я всё же считаю что у decode_body должна быть такая же сигнатура как у decode
>
> вот эта проверка должна быть снаружи:
>
> + if (data != end)
> + return luaL_error(L, "invalid xrow length");
>
> и сообщение кривое
>
> это не xrow length
>
> это body length
Below the fix is presented. At the end of the letter the whole patch is
attached.
diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
index 3d9479283..adf5f7603 100644
--- a/src/box/lua/net_box.c
+++ b/src/box/lua/net_box.c
@@ -560,14 +560,13 @@ handle_error:
* Decode Tarantool response body.
* @param Lua stack[1] Raw MessagePack pointer.
* @param Lua stack[2] End of body pointer.
- * @retval Decoded body.
+ * @retval Decoded body and position of the body end.
*/
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. */
@@ -591,9 +590,8 @@ netbox_decode_body(struct lua_State *L)
}
}
}
- if (data != end)
- return luaL_error(L, "invalid xrow length");
- return 1;
+ *(const char **)luaL_pushcdata(L, ctypeid) = data;
+ return 2;
}
int
diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
index 194a1c86b..8fcaf89e8 100644
--- a/src/box/lua/net_box.lua
+++ b/src/box/lua/net_box.lua
@@ -49,29 +49,30 @@ local E_PROC_LUA = box.error.PROC_LUA
-- utility tables
local is_final_state = {closed = 1, error = 1}
-local function decode_nil(...) 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]
+local function decode_nil(raw_data, raw_data_end)
+ return nil, raw_data_end
end
-local function decode_tuple(raw_data, raw_data_end)
- return internal.decode_body(raw_data, raw_data_end)[1]
+local function decode_data(raw_data)
+ local response, raw_end = decode(raw_data)
+ return response[IPROTO_DATA_KEY], raw_end
end
-local function decode_get(raw_data, raw_data_end)
- local body = internal.decode_body(raw_data, raw_data_end)
+local function decode_tuple(raw_data)
+ local response, raw_end = internal.decode_body(raw_data)
+ return response[1], raw_end
+end
+local function decode_get(raw_data)
+ local body, raw_end = internal.decode_body(raw_data)
if body[2] then
- return nil, box.error.MORE_THAN_ONE_TUPLE
+ return nil, raw_end, box.error.MORE_THAN_ONE_TUPLE
end
- return body[1]
+ return body[1], raw_end
end
-local function decode_select(raw_data, raw_data_end)
- return internal.decode_body(raw_data, raw_data_end)
+local function decode_select(raw_data)
+ return internal.decode_body(raw_data)
end
-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]
+local function decode_count(raw_data)
+ local response, raw_end = decode(raw_data)
+ return response[IPROTO_DATA_KEY][1], raw_end
end
local method_encoder = {
@@ -467,8 +468,10 @@ local function create_transport(host, port, user, password, callback,
end
-- Decode xrow.body[DATA] to Lua objects
- request.response, request.errno =
+ local real_end
+ request.response, real_end, request.errno =
method_decoder[request.method](body_rpos, body_end)
+ assert(real_end == body_end, "invalid body length")
request.cond:broadcast()
end
===========================================================================================
diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
index 04fe70b03..adf5f7603 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,44 @@ 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 and position of the body end.
+ */
+static int
+netbox_decode_body(struct lua_State *L)
+{
+ uint32_t ctypeid;
+ const char *data = *(const char **)luaL_checkcdata(L, 1, &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);
+ }
+ }
+ }
+ *(const char **)luaL_pushcdata(L, ctypeid) = data;
+ return 2;
+}
+
int
luaopen_net_box(struct lua_State *L)
{
@@ -572,6 +611,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..8fcaf89e8 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
@@ -50,31 +49,30 @@ local E_PROC_LUA = box.error.PROC_LUA
-- utility tables
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_nil(raw_data, raw_data_end)
+ return nil, raw_data_end
end
-local function decode_get(response)
- if response[2] then
- return nil, box.error.MORE_THAN_ONE_TUPLE
- end
- if response[1] then
- return box.tuple.new(response[1])
- end
+local function decode_data(raw_data)
+ local response, raw_end = decode(raw_data)
+ return response[IPROTO_DATA_KEY], raw_end
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)
+local function decode_tuple(raw_data)
+ local response, raw_end = internal.decode_body(raw_data)
+ return response[1], raw_end
+end
+local function decode_get(raw_data)
+ local body, raw_end = internal.decode_body(raw_data)
+ if body[2] then
+ return nil, raw_end, box.error.MORE_THAN_ONE_TUPLE
end
- return response
+ return body[1], raw_end
+end
+local function decode_select(raw_data)
+ return internal.decode_body(raw_data)
end
-local function decode_count(response)
- return response[1]
+local function decode_count(raw_data)
+ local response, raw_end = decode(raw_data)
+ return response[IPROTO_DATA_KEY][1], raw_end
end
local method_encoder = {
@@ -103,8 +101,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 +113,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 +468,10 @@ 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
+ local real_end
+ request.response, real_end, request.errno =
+ method_decoder[request.method](body_rpos, body_end)
+ assert(real_end == body_end, "invalid body length")
request.cond:broadcast()
end
diff --git a/test/box/call.result b/test/box/call.result
index e27e2127d..40d7ef952 100644
--- a/test/box/call.result
+++ b/test/box/call.result
@@ -632,7 +632,7 @@ conn:eval("return return_sparse1()")
...
conn:call_16("return_sparse1")
---
-- - [{20: 1, 1: 1}]
+- - [{1: 1, 20: 1}]
...
function return_sparse2() return { [1] = 1, [20] = 1} end
---
^ permalink raw reply [flat|nested] 4+ messages in thread* [tarantool-patches] Re: [PATCH 1/1] netbox: optimize body decoding
2018-05-14 11:41 [tarantool-patches] [PATCH 1/1] netbox: optimize body decoding Vladislav Shpilevoy
2018-05-15 18:31 ` [tarantool-patches] " Vladislav Shpilevoy
@ 2018-05-15 18:38 ` Konstantin Osipov
2018-05-15 18:42 ` Vladislav Shpilevoy
1 sibling, 1 reply; 4+ messages in thread
From: Konstantin Osipov @ 2018-05-15 18:38 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
* 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
^ permalink raw reply [flat|nested] 4+ messages in thread