From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp60.i.mail.ru (smtp60.i.mail.ru [217.69.128.40]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 944FF469719 for ; Sun, 11 Oct 2020 18:25:06 +0300 (MSK) References: From: Vladislav Shpilevoy Message-ID: <33039bab-b31e-0da7-359d-dbbc8464aa55@tarantool.org> Date: Sun, 11 Oct 2020 17:25:04 +0200 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH v2 05/15] lua: don't raise a Lua error from luaT_tuple_new() List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Turenko Cc: tarantool-patches@dev.tarantool.org Hi! Thanks for the patch! See 5 comments below. On 11.10.2020 14:57, Alexander Turenko wrote: > This change fixes incorrect behaviour at tuple serialization error in > several places: :frommap(), :compare(), > :select(). See more in #5382. > > Disallow creating a tuple from objects on the Lua stack (idx == 0) in > luaT_tuple_new() for simplicity. There are no such usages in tarantool. > The function is not exposed yet to the module API. This is only > necessary in box.tuple.new(), which anyway raises Lua errors by its > contract. 1. But why? The case of creating a tuple from values may be much faster, when there is a lot of values not wrapped into a table. Table wrap is costly. Could you just merge luaT_tuple_encode_values and luaT_tuple_encode_table into one function, withuout splitting them? > diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c > index ed97c85e4..18cfef979 100644 > --- a/src/box/lua/tuple.c > +++ b/src/box/lua/tuple.c > @@ -69,6 +69,8 @@ extern char tuple_lua[]; /* Lua source */ > > uint32_t CTID_STRUCT_TUPLE_REF; > > +int luaT_tuple_encode_table_ref = LUA_NOREF; 2. Better make it static. It is not used outside of this file. > + > box_tuple_t * > luaT_checktuple(struct lua_State *L, int idx) > { > @@ -98,39 +100,79 @@ luaT_istuple(struct lua_State *L, int narg) > return *(struct tuple **) data; > } > > +/** > + * Encode a Lua values on a Lua stack as an MsgPack array. > + * > + * Raise a Lua error when encoding fails. > + */ > +static int > +luaT_tuple_encode_values(struct lua_State *L) > +{ > + struct ibuf *buf = tarantool_lua_ibuf; > + ibuf_reset(buf); > + struct mpstream stream; > + mpstream_init(&stream, buf, ibuf_reserve_cb, ibuf_alloc_cb, luamp_error, > + L); > + int argc = lua_gettop(L); > + mpstream_encode_array(&stream, argc); > + for (int k = 1; k <= argc; ++k) { > + luamp_encode(L, luaL_msgpack_default, NULL, &stream, k); > + } > + mpstream_flush(&stream); > + return 0; > +} > + > +/** > + * Encode a Lua table or a tuple as MsgPack. > + * > + * Raise a Lua error when encoding fails. > + * > + * It is a kind of critical section to be run under luaT_call(). > + */ > +static int > +luaT_tuple_encode_table(struct lua_State *L) > +{ > + struct ibuf *buf = tarantool_lua_ibuf; > + ibuf_reset(buf); > + struct mpstream stream; > + mpstream_init(&stream, buf, ibuf_reserve_cb, ibuf_alloc_cb, luamp_error, > + L); > + luamp_encode_tuple(L, &tuple_serializer, &stream, 1); > + mpstream_flush(&stream); > + return 0; > +} > + > static char * > luaT_tuple_encode_on_lua_ibuf(struct lua_State *L, int idx, > size_t *tuple_len_ptr) > { > - if (idx != 0 && !lua_istable(L, idx) && !luaT_istuple(L, idx)) { > + assert(idx != 0); > + if (!lua_istable(L, idx) && !luaT_istuple(L, idx)) { > diag_set(IllegalParams, "A tuple or a table expected, got %s", > lua_typename(L, lua_type(L, idx))); > return NULL; > } > > - struct ibuf *buf = tarantool_lua_ibuf; > - ibuf_reset(buf); > - struct mpstream stream; > - mpstream_init(&stream, buf, ibuf_reserve_cb, ibuf_alloc_cb, > - luamp_error, L); > - if (idx == 0) { > - /* > - * Create the tuple from lua stack > - * objects. > - */ > - int argc = lua_gettop(L); > - mpstream_encode_array(&stream, argc); > - for (int k = 1; k <= argc; ++k) { > - luamp_encode(L, luaL_msgpack_default, NULL, &stream, k); > - } > - } else { > - /* Create the tuple from a Lua table. */ > - luamp_encode_tuple(L, &tuple_serializer, &stream, idx); > - } > - mpstream_flush(&stream); > + int top = lua_gettop(L); > + > + /* Calculate absolute value in the stack. */ > + if (idx < 0) > + idx = top + idx + 1;> + > + assert(luaT_tuple_encode_table_ref != LUA_NOREF); > + lua_rawgeti(L, LUA_REGISTRYINDEX, luaT_tuple_encode_table_ref); > + assert(lua_isfunction(L, -1)); > + > + lua_pushvalue(L, idx); > + > + int rc = luaT_call(L, 1, 0); > + lua_settop(L, top); > + if (rc != 0) > + return NULL; > + > if (tuple_len_ptr != NULL) > - *tuple_len_ptr = ibuf_used(buf); > - return buf->buf; > + *tuple_len_ptr = ibuf_used(tarantool_lua_ibuf); 3. Since you assume the function at luaT_tuple_encode_table_ref alwaus uses ibuf, maybe it would be better to add 'on_lua_ibuf' to its name. Or move the length calculation out of the current function, because this one already has 'on_lua_ibuf', so it wouldn't look so strange to get size of tarantool_lua_ibuf after it. > + return tarantool_lua_ibuf->buf; > } > > struct tuple * > @@ -156,15 +198,30 @@ lbox_tuple_new(lua_State *L) > lua_newtable(L); /* create an empty tuple */ > ++argc; > } > + > + box_tuple_format_t *fmt = box_tuple_format_default(); > + 4. You could keep it after the comment below to reduce the diff. > /* > * Use backward-compatible parameters format: > - * box.tuple.new(1, 2, 3) (idx == 0), or the new one: > - * box.tuple.new({1, 2, 3}) (idx == 1). > + * box.tuple.new(1, 2, 3). > */ > - int idx = argc == 1 && (lua_istable(L, 1) || > - luaT_istuple(L, 1)); > - box_tuple_format_t *fmt = box_tuple_format_default(); > - struct tuple *tuple = luaT_tuple_new(L, idx, fmt); > + if (argc != 1 || (!lua_istable(L, 1) && !luaT_istuple(L, 1))) { > + struct ibuf *buf = tarantool_lua_ibuf; > + luaT_tuple_encode_values(L); /* may raise */ 5. We usually put comments on separate line. > + struct tuple *tuple = box_tuple_new(fmt, buf->buf, > + buf->buf + ibuf_used(buf)); > + ibuf_reinit(buf); > + if (tuple == NULL) > + return luaT_error(L); > + luaT_pushtuple(L, tuple); > + return 1; > + }