Re: [Tarantool-patches] [PATCH] lua: lua_field_inspect_table without pushcfunction

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hi! Thanks for the patch!

Igor, please, join the review.

Sergey, see 4 comments below.

On 18/05/2020 11:37, Sergey Kaplun wrote:
> Currently on encoding table we push cfunction (lua_field_try_serialize)
> to lua stack with additional lightuserdata and table value and after
> pcall that function to avoid a raise of error.
> 
> In this case LuaJIT creates new object which will not live long time,
> so it increase amount of dead object and also increase time and
> frequency of garbage collection inside LuaJIT.
> Also this pcall is necessary only in case when metafield __serialize
> of serilizable object has LUA_TFUNCTION type.
> 
> So instead pushcfunction with pcall we can directly call the function
> trying to serialize an object.
> ---

1. Is there a measurement, how much does this patch help and when,
exactly?

> branch: https://github.com/tarantool/tarantool/tree/skaplun/no-ticket-lua-inspect-table-refactoring
> 
>  src/lua/utils.c | 132 ++++++++++++++++--------------------------------
>  1 file changed, 44 insertions(+), 88 deletions(-)
> 
> diff --git a/src/lua/utils.c b/src/lua/utils.c
> index d410a3d03..58715a55e 100644
> --- a/src/lua/utils.c
> +++ b/src/lua/utils.c
> @@ -461,91 +461,69 @@ lua_field_inspect_ucdata(struct lua_State *L, struct luaL_serializer *cfg,
>  }
>  
>  /**
> - * A helper structure to simplify safe call of __serialize method.
> - * It passes some arguments into the serializer called via pcall,
> - * and carries out some results.
> + * Call __serialize method of a table object by index if the former exists.
> + *
> + * If __serialize not exists function does nothing and the function returns 1;

2. 'not exists' -> 'does not exist'.

> + *
> + * If __serialize exists, is correct and is a function then
> + * a result of serialization replaces old value by the index and
> + * the function returns 0;
> + *
> + * If the serialization hints like 'array' or 'map', then field->type,
> + * field->syze and field->compact sets if necessary
> + * and the function returns 0;
> + *
> + * Otherwise it is an error, set diag and the funciton returns -1;
> + *
> + * Return values:
> + * -1 - error, set diag
> + *  0 - has serialize, success replace and have to finish
> + *  1 - hasn't serialize, need to process
>   */
> -struct lua_serialize_status {
> -	/**
> -	 * True if an attempt to call __serialize has failed. A
> -	 * diag message is set.
> -	 */
> -	bool is_error;
> -	/**
> -	 * True, if __serialize exists. Otherwise an ordinary
> -	 * default serialization is used.
> -	 */
> -	bool is_serialize_used;
> -	/**
> -	 * True, if __serialize not only exists, but also returned
> -	 * a new value which should replace the original one. On
> -	 * the contrary __serialize could be a string like 'array'
> -	 * or 'map' and do not push anything but rather say how to
> -	 * interpret the target table. In such a case there is
> -	 * nothing to replace with.
> -	 */
> -	bool is_value_returned;
> -	/** Parameters, passed originally to luaL_tofield. */
> -	struct luaL_serializer *cfg;
> -	struct luaL_field *field;
> -};
>  
> -/**
> - * Call __serialize method of a table object if the former exists.
> - * The function expects 2 values pushed onto the Lua stack: a
> - * value to serialize, and a pointer at a struct
> - * lua_serialize_status object. If __serialize exists, is correct,
> - * and is a function then one value is pushed as a result of
> - * serialization. If it is correct, but just a serialization hint
> - * like 'array' or 'map', then nothing is pushed. Otherwise it is
> - * an error. All the described outcomes can be distinguished via
> - * lua_serialize_status attributes.
> - */
>  static int
> -lua_field_try_serialize(struct lua_State *L)
> +lua_field_try_serialize(struct lua_State *L, int idx,
> +			struct luaL_serializer *cfg, struct luaL_field *field)
>  {
> -	struct lua_serialize_status *s =
> -		(struct lua_serialize_status *) lua_touserdata(L, 2);
> -	s->is_serialize_used = (luaL_getmetafield(L, 1, LUAL_SERIALIZE) != 0);
> -	s->is_error = false;
> -	s->is_value_returned = false;
> -	if (! s->is_serialize_used)
> -		return 0;
> -	struct luaL_serializer *cfg = s->cfg;
> -	struct luaL_field *field = s->field;
> +	bool is_serialize_used = (luaL_getmetafield(L, idx,
> +						    LUAL_SERIALIZE) != 0);
> +	if (!is_serialize_used)
> +		return 1;

3. Seems like you don't need to save the check result into a variable.
You can just inline is_serialize_used assignment into the condition.

>  	if (lua_isfunction(L, -1)) {
>  		/* copy object itself */
> -		lua_pushvalue(L, 1);
> -		lua_call(L, 1, 1);
> -		s->is_error = (luaL_tofield(L, cfg, NULL, -1, field) != 0);
> -		s->is_value_returned = true;
> -		return 1;
> +		lua_pushvalue(L, idx);
> +		if (lua_pcall(L, 1, 1, 0) != 0) {
> +			diag_set(LuajitError, lua_tostring(L, -1));
> +			return -1;
> +		}
> +		if (luaL_tofield(L, cfg, NULL, -1, field) != 0)
> +			return -1;
> +		lua_replace(L, idx);
> +		return 0;
>  	}
>  	if (!lua_isstring(L, -1)) {
>  		diag_set(LuajitError, "invalid " LUAL_SERIALIZE " value");
> -		s->is_error = true;
> -		lua_pop(L, 1);
> -		return 0;
> +		return -1;
>  	}
>  	const char *type = lua_tostring(L, -1);
>  	if (strcmp(type, "array") == 0 || strcmp(type, "seq") == 0 ||
>  	    strcmp(type, "sequence") == 0) {
>  		field->type = MP_ARRAY; /* Override type */
> -		field->size = luaL_arrlen(L, 1);
> +		field->size = luaL_arrlen(L, idx);
>  		/* YAML: use flow mode if __serialize == 'seq' */
>  		if (cfg->has_compact && type[3] == '\0')
>  			field->compact = true;
>  	} else if (strcmp(type, "map") == 0 || strcmp(type, "mapping") == 0) {
>  		field->type = MP_MAP;   /* Override type */
> -		field->size = luaL_maplen(L, 1);
> +		field->size = luaL_maplen(L, idx);
>  		/* YAML: use flow mode if __serialize == 'map' */
>  		if (cfg->has_compact && type[3] == '\0')
>  			field->compact = true;
>  	} else {
>  		diag_set(LuajitError, "invalid " LUAL_SERIALIZE " value");
> -		s->is_error = true;
> +		return -1;
>  	}
> -	lua_pop(L, 1);
> +	lua_pop(L, 1); /* remove value was setted by luaL_getmetafield */

4. Please, don't write comments on the same line as code. Also start
sentences with a capital letter and end with a dot.

'was setted' -> 'set'