Re: [Tarantool-patches] [PATCH 11/20] net.box: rewrite response decoder in C

[Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Thanks for the patch!

See 3 comments below.

On 23.07.2021 13:07, Vladimir Davydov via Tarantool-patches wrote:
> This patch moves method_decoder table from Lua to C. This is a step
> towards rewriting performance-critical parts of net.box in C.
> ---
>  src/box/lua/net_box.c   | 235 +++++++++++++++++++++++++++++++---------
>  src/box/lua/net_box.lua |  43 +-------
>  2 files changed, 189 insertions(+), 89 deletions(-)
> 
> diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
> index 49030aabea69..c0c3725e5350 100644
> --- a/src/box/lua/net_box.c
> +++ b/src/box/lua/net_box.c
> @@ -678,6 +678,79 @@ netbox_encode_method(struct lua_State *L)

<...>

> +
> +/**
> + * Decode Tarantool response body consisting of single
> + * IPROTO_DATA key into Lua table.
> + * @param L Lua stack to push result on.
> + * @param data MessagePack.
> + * @retval Lua table

1. It is not necessary to write these doxygen comments for each
param really if you think they are obvious. The only 'rule' is to
comment the function on its first declaration. Up to you of
course. Not a rule.

> + */
> +static void
> +netbox_decode_table(struct lua_State *L, const char **data,
> +		    const char *data_end, struct tuple_format *format)
> +{
> +	(void) data_end;
> +	(void) format;
> +	netbox_skip_to_data(data);
> +	luamp_decode(L, cfg, data);
> +}
> +
> +/**
> + * Same as netbox_decode_table, but only decodes the first element of the
> + * table, skipping the rest. Used to decode index.count() call result.
> + * @param L Lua stack to push result on.
> + * @param data MessagePack.
> + * @retval count or nil.
> + */
> +static void
> +netbox_decode_value(struct lua_State *L, const char **data,
> +		    const char *data_end, struct tuple_format *format)
> +{
> +	(void) data_end;
> +	(void) format;
> +	netbox_skip_to_data(data);
> +	uint32_t count = mp_decode_array(data);
> +	for (uint32_t i = 0; i < count; ++i) {
> +		if (i == 0)
> +			luamp_decode(L, cfg, data);
> +		else
> +			mp_next(data);

2. Is the compiler able to turn this into?:

		...
		if (count == 0)
			return lua_pushnil(L);

		luamp_decode(L, cfg, data);
		for (uint32_t i = 1; i < count; ++i)
			mp_next(data);
	}

And why not do it right away? It looks shorter and a bit
simpler IMO. Up to you.

> +	}
> +	if (count == 0)
> +		lua_pushnil(L);
> +}
> +
>  /**
>   * Decode IPROTO_DATA into tuples array.
>   * @param L Lua stack to push result on.
> @@ -704,31 +777,45 @@ netbox_decode_data(struct lua_State *L, const char **data,

<...>

> +
> +/**
> + * Same as netbox_decode_select, but only decodes the first tuple of the array,
> + * skipping the rest.
> + * @param L Lua stack to push result on.
> + * @param data MessagePack.
> + * @retval Tuple or nil.
> + */
> +static void
> +netbox_decode_tuple(struct lua_State *L, const char **data,
> +		    const char *data_end, struct tuple_format *format)
> +{
> +	(void) data_end;
> +	netbox_skip_to_data(data);
> +	uint32_t count = mp_decode_array(data);
> +	for (uint32_t i = 0; i < count; ++i) {
> +		const char *begin = *data;
> +		mp_next(data);
> +		if (i > 0)
> +			continue;
> +		struct tuple *tuple = box_tuple_new(format, begin, *data);
> +		if (tuple == NULL)
> +			luaT_error(L);
> +		luaT_pushtuple(L, tuple);
> +	}
> +	if (count == 0)
> +		lua_pushnil(L);

3. Ditto. Wouldn't it be shorter and a bit easier to read by doing this?:

		...
		if (count == 0)
			return lua_pushnil(L);

		struct tuple *tuple = box_tuple_new(format, begin, *data);
		if (tuple == NULL)
			luaT_error(L);
		luaT_pushtuple(L, tuple);
		for (uint32_t i = 1; i < count; ++i)
			mp_next(data);
	}

Up to you.