From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 9710A2BE4F for ; Mon, 9 Apr 2018 06:44:59 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id cWjqnK7aCTKD for ; Mon, 9 Apr 2018 06:44:59 -0400 (EDT) Received: from smtp17.mail.ru (smtp17.mail.ru [94.100.176.154]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 567A02BA13 for ; Mon, 9 Apr 2018 06:44:58 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v2 1/1] schema:frommap() to create table or tuple References: <832dae8c-3736-1547-b0a4-75f1c8debc52@tarantool.org> <20180406154401.GA18456@atlas> <14e32326-7bca-5e49-68b7-3341ee7ce58b@tarantool.org> From: Vladislav Shpilevoy Message-ID: <1a5472f7-7183-1849-750c-6f2f7748918d@tarantool.org> Date: Mon, 9 Apr 2018 13:44:55 +0300 MIME-Version: 1.0 In-Reply-To: <14e32326-7bca-5e49-68b7-3341ee7ce58b@tarantool.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: Kirill Shcherbatov , tarantool-patches@freelists.org Hello. See 7 comments below. On 09/04/2018 11:30, Kirill Shcherbatov wrote: > I've accounted Kostya's comments. > Going to measure performance. > > > From ef47d3536b845a84b52be6c84a66aef12fbe8561 Mon Sep 17 00:00:00 2001 > From: Kirill Shcherbatov > Date: Mon, 9 Apr 2018 11:24:38 +0300 > Subject: [PATCH] Moved obtaining cptr to Lua code. > > --- > src/box/lua/schema.lua | 33 +++++++++++++++++++++++++--- > src/box/lua/space.cc | 59 ++++++-------------------------------------------- > 2 files changed, 37 insertions(+), 55 deletions(-) > > diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua > index 323c2d6..a0f2d25 100644 > --- a/src/box/lua/schema.lua > +++ b/src/box/lua/schema.lua > @@ -1072,6 +1072,19 @@ end > > internal.check_iterator_type = check_iterator_type -- export for net.box > > +-- Get a C space structure pointer by space id using cache. > +-- Update cache if necessary. > +-- The _schema_version field is a cache invalidation rule. > +function space_cptr_cached(space) > + local schema_version = builtin.box_schema_version() > + if space._schema_version == schema_version then > + return space._cptr > + end > + space._cptr = builtin.space_by_id(space.id) > + space._schema_version = schema_version > + return space._cptr > +end > + > function box.schema.space.bless(space) > local index_mt = {} > -- __len and __index > @@ -1320,7 +1333,7 @@ function box.schema.space.bless(space) > end > space_mt.bsize = function(space) > check_space_arg(space, 'bsize') > - local s = builtin.space_by_id(space.id) > + local s = space_cptr_cached(space) 1. I know, it is very small one-line fix, but lets do it in a separate patch. > if s == nil then > box.error(box.error.NO_SUCH_SPACE, space.name) > end > @@ -1413,13 +1426,20 @@ function box.schema.space.bless(space) > end > space_mt.run_triggers = function(space, yesno) > check_space_arg(space, 'run_triggers') > - local s = builtin.space_by_id(space.id) > + local s = space_cptr_cached(space) 2. This too. > if s == nil then > box.error(box.error.NO_SUCH_SPACE, space.name) > end > builtin.space_run_triggers(s, yesno) > end > - space_mt.frommap = box.internal.space.frommap > + space_mt.frommap = function(space, map, opts) > + -- update the cache > + local cptr = space_cptr_cached(space) > + if cptr == nil then > + return box.NULL, "Space " .. space.name .. " does not exist" 3. box.NULL == nil, but `if box.NULL` check is true, so box.NULL behaves different from nil. Please, return exactly nil. > + end > + return box.internal.space.frommap(space, map, opts) > + end > space_mt.__index = space_mt > > setmetatable(space, space_mt) > @@ -2234,5 +2254,12 @@ box.internal.schema = {} > box.internal.schema.init = function() > box_sequence_init() > end > +box.internal.space._cptr = function(space) > + local cptr = space_cptr_cached(space) > + if cptr == nil then > + return box.NULL, "Space " .. space.name .. " does not exist" > + end > + return cptr > +end 4. You do not need this function anymore. It is unused. > > box.NULL = msgpack.NULL > diff --git a/src/box/lua/space.cc b/src/box/lua/space.cc > index 2723baf..9383096 100644 > --- a/src/box/lua/space.cc > +++ b/src/box/lua/space.cc > @@ -397,51 +397,6 @@ static struct trigger on_alter_space_in_lua = { > }; > > /** > - * Get a C pointer for space structure. > - * Caches the result. Cleans the user(negative) stack itself. > - * @param Lua space object at top of the Lua stack. > - * @retval not nil C structure pointer. > - * @retval nil, err Can not find underlying space, error > - * description is returned. > - */ > -static int > -lbox_space_ptr_cached(struct lua_State *L) > -{ > - if (lua_gettop(L) < 1 || !lua_istable(L, 1)) > - luaL_error(L, "Usage: space:_cptr()"); > - > - lua_getfield(L, 1, "_schema_version"); > - uint64_t schema_version = luaL_touint64(L, -1); > - lua_pop(L, 1); > - uint64_t global_shema_version = box_schema_version(); > - > - void *space = NULL; > - if (schema_version == global_shema_version) { > - lua_getfield(L, 1, "_cptr"); > - space = (void *)lua_topointer(L, -1); > - } else { > - lua_getfield(L, 1, "id"); > - uint32_t id = luaL_touint64(L, -1); > - space = space_by_id(id); > - if (space == NULL) { > - lua_pushnil(L); > - lua_pushstring(L, tt_sprintf("Space with id '%d' "\ > - "doesn't exist", id)); > - return 2; > - } > - > - /** update the cache */ > - luaL_pushuint64(L, global_shema_version); > - lua_setfield(L, 1, "_schema_version"); > - lua_pushlightuserdata(L, space); > - lua_setfield(L, 1, "_cptr"); > - } > - lua_pop(L, 1); > - lua_pushlightuserdata(L, space); > - return 1; > -} > - > -/** > * Make a tuple or a table Lua object by map. > * @param Lua space object. > * @param Lua map table object. > @@ -456,11 +411,11 @@ lbox_space_frommap(struct lua_State *L) > { > struct tuple_dictionary *dict = NULL; > struct space *space = NULL; > - int rc, argc = lua_gettop(L); > + int argc = lua_gettop(L); > bool table = false; > if (argc < 2 || argc > 3 || !lua_istable(L, 2)) > goto usage_error; > - if (argc == 3) { > + if (argc == 3 && !lua_isnil(L, 3)) { 5. Why do you need this? Error on argc == 3 and argv[3] != nil is the same as error on argc > 2. > if (!lua_istable(L, 3)) > goto usage_error; > lua_getfield(L, 3, "table"); > @@ -469,10 +424,11 @@ lbox_space_frommap(struct lua_State *L) > table = lua_toboolean(L, -1); > } > > - rc = lbox_space_ptr_cached(L); > - if (rc != 1) > - return rc; > - space = (struct space *) lua_topointer(L, -1); > + lua_getfield(L, 1, "_cptr"); > + space = (struct space *)lua_topointer(L, -1); 6. Unnecessary diff. > + /* validation is in the Lua code */ > + assert(space); 7. Please, use explicit != NULL, start a sentence with a capital letter, and put a dot at the end. > + > dict = space->format->dict; > lua_createtable(L, space->def->field_count, 0); > > @@ -582,7 +538,6 @@ box_lua_space_init(struct lua_State *L) > > static const struct luaL_Reg space_internal_lib[] = { > {"frommap", lbox_space_frommap}, > - {"_cptr", lbox_space_ptr_cached}, > {NULL, NULL} > }; > luaL_register(L, "box.internal.space", space_internal_lib); >