From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp41.i.mail.ru (smtp41.i.mail.ru [94.100.177.101]) (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 87C11469710 for ; Tue, 19 May 2020 00:52:24 +0300 (MSK) References: <20200518093748.16825-1-skaplun@tarantool.org> From: Vladislav Shpilevoy Message-ID: <74b93f1b-5515-6f8a-32d6-4672e312eb51@tarantool.org> Date: Mon, 18 May 2020 23:52:22 +0200 MIME-Version: 1.0 In-Reply-To: <20200518093748.16825-1-skaplun@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH] lua: lua_field_inspect_table without pushcfunction List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Sergey Kaplun , tarantool-patches@dev.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'