Re: [Tarantool-patches] [PATCH v2 05/15] lua: don't raise a Lua error from luaT_tuple_new()

[Vladislav Shpilevoy <v.shpilevoy@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: <space_object>:frommap(), <key_def_object>:compare(),
> <merge_source>: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;
> +	}