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 105C82A6F6 for ; Mon, 9 Apr 2018 04:30:13 -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 cCYWVG3QC6dx for ; Mon, 9 Apr 2018 04:30:12 -0400 (EDT) Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (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 8AEED29623 for ; Mon, 9 Apr 2018 04:30:12 -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> From: Kirill Shcherbatov Message-ID: <14e32326-7bca-5e49-68b7-3341ee7ce58b@tarantool.org> Date: Mon, 9 Apr 2018 11:30:09 +0300 MIME-Version: 1.0 In-Reply-To: <20180406154401.GA18456@atlas> Content-Type: text/plain; charset=utf-8 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: tarantool-patches@freelists.org Cc: "v.shpilevoy@tarantool.org" 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) 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) 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" + 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 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)) { 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); + /* validation is in the Lua code */ + assert(space); + 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); -- 2.7.4 On 06.04.2018 18:44, Konstantin Osipov wrote: > * Kirill Shcherbatov [18/04/05 10:24]: > >> This feature is designed to build tuple or table object by map. >> Tuple output is default. If you wish to make a table, specify table = true option. >> subscriber:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {table = true}) > > What are the performance results from adding the cache? > > How much do we win? > > It's OK to export _schema_version to Lua as extern int via FFI, > and avoid a function call. > >> +static int >> +lbox_space_ptr_cached(struct lua_State *L) > > The fast path of this function should be in plain Lua. > > You have to re-fetch the space via lookup in a slowpath only, > when schema version has changed. > > > What about using the cptr in all other functions? > > >> +{ >> + 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. >> + * @param Lua opts table object (optional). >> + * @retval not nil A tuple or a table conforming to a space >> + * format. >> + * @retval nil, err Can not built a tuple. A reason is returned in >> + * the second value. >> + */ >> +static int >> +lbox_space_frommap(struct lua_State *L) >> +{ >> + struct tuple_dictionary *dict = NULL; >> + struct space *space = NULL; >> + int rc, argc = lua_gettop(L); >> + bool table = false; >> + if (argc < 2 || argc > 3 || !lua_istable(L, 2)) >> + goto usage_error; >> + if (argc == 3) { >> + if (!lua_istable(L, 3)) >> + goto usage_error; >> + lua_getfield(L, 3, "table"); >> + if (!lua_isboolean(L, -1) && !lua_isnil(L, -1)) >> + goto usage_error; >> + table = lua_toboolean(L, -1); >> + } >> + >> + rc = lbox_space_ptr_cached(L); >> + if (rc != 1) >> + return rc; >> + space = (struct space *) lua_topointer(L, -1); >> + dict = space->format->dict; >> + lua_createtable(L, space->def->field_count, 0); >> + >> + lua_pushnil(L); >> + while (lua_next(L, 2) != 0) { >> + uint32_t fieldno; >> + size_t key_len; >> + const char *key = lua_tolstring(L, -2, &key_len); >> + uint32_t key_hash = lua_hashstring(L, -2); >> + if (tuple_fieldno_by_name(dict, key, key_len, key_hash, >> + &fieldno)) { >> + lua_pushnil(L); >> + lua_pushstring(L, tt_sprintf("Unknown field '%s'", >> + key)); >> + return 2; >> + } >> + lua_rawseti(L, -3, fieldno+1); >> + } >> + if (table) >> + return 1; >> + >> + lua_replace(L, 1); >> + lua_settop(L, 1); >> + return lbox_tuple_new(L); >> +usage_error: >> + return luaL_error(L, "Usage: space:frommap(map, opts)"); >> +} >> + >> void >> box_lua_space_init(struct lua_State *L) >> { >> @@ -464,4 +579,12 @@ box_lua_space_init(struct lua_State *L) >> lua_pushnumber(L, VCLOCK_MAX); >> lua_setfield(L, -2, "REPLICA_MAX"); >> lua_pop(L, 2); /* box, schema */ >> + >> + 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); >> + lua_pop(L, 1); >> } >> diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c >> index 7ca4299..92a7185 100644 >> --- a/src/box/lua/tuple.c >> +++ b/src/box/lua/tuple.c >> @@ -90,7 +90,7 @@ luaT_istuple(struct lua_State *L, int narg) >> return *(struct tuple **) data; >> } >> >> -static int >> +int >> lbox_tuple_new(lua_State *L) >> { >> int argc = lua_gettop(L); >> diff --git a/src/box/lua/tuple.h b/src/box/lua/tuple.h >> index 0965644..5d7062e 100644 >> --- a/src/box/lua/tuple.h >> +++ b/src/box/lua/tuple.h >> @@ -66,6 +66,9 @@ luaT_istuple(struct lua_State *L, int idx); >> >> /** \endcond public */ >> >> +int >> +lbox_tuple_new(struct lua_State *L); >> + >> static inline int >> luaT_pushtupleornil(struct lua_State *L, struct tuple *tuple) >> { >> diff --git a/test/box/tuple.result b/test/box/tuple.result >> index ec5ed90..2dd8f0d 100644 >> --- a/test/box/tuple.result >> +++ b/test/box/tuple.result >> @@ -1060,6 +1060,101 @@ box.tuple.bsize == t.bsize >> --- >> - true >> ... >> +-- >> +-- gh-3282: space:frommap(). >> +-- >> +format = {} >> +--- >> +... >> +format[1] = {name = 'aaa', type = 'unsigned'} >> +--- >> +... >> +format[2] = {name = 'bbb', type = 'unsigned'} >> +--- >> +... >> +format[3] = {name = 'ccc', type = 'unsigned'} >> +--- >> +... >> +format[4] = {name = 'ddd', type = 'unsigned'} >> +--- >> +... >> +s = box.schema.create_space('test', {format = format}) >> +--- >> +... >> +s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}) >> +--- >> +- [2, 4, 3, 1] >> +... >> +s:frommap({ddd = 1, aaa = 2, bbb = 3}) >> +--- >> +- [2, 3, null, 1] >> +... >> +s:frommap({ddd = 1, aaa = 2, ccc = 3, eee = 4}) >> +--- >> +- null >> +- Unknown field 'eee' >> +... >> +s:frommap() >> +--- >> +- error: 'Usage: space:frommap(map, opts)' >> +... >> +s:frommap({}) >> +--- >> +- [] >> +... >> +s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {table = true}) >> +--- >> +- - 2 >> + - 4 >> + - 3 >> + - 1 >> +... >> +s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {table = false}) >> +--- >> +- [2, 4, 3, 1] >> +... >> +s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = box.NULL}) >> +--- >> +- [2, null, 3, 1] >> +... >> +s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {dummy = true}) >> +--- >> +- [2, 4, 3, 1] >> +... >> +_ = s:create_index('primary', {parts = {'aaa'}}) >> +--- >> +... >> +tuple = s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}) >> +--- >> +... >> +s:replace(tuple) >> +--- >> +- [2, 4, 3, 1] >> +... >> +_, err = box.internal.space._cptr(s) >> +--- >> +... >> +assert(not err) >> +--- >> +- true >> +... >> +s:drop() >> +--- >> +... >> +_, err = s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}) >> +--- >> +... >> +assert(err ~= nil) >> +--- >> +- true >> +... >> +_, err = box.internal.space._cptr(s) >> +--- >> +... >> +assert(err ~= nil) >> +--- >> +- true >> +... >> test_run:cmd("clear filter") >> --- >> - true >> diff --git a/test/box/tuple.test.lua b/test/box/tuple.test.lua >> index 0b3392d..03e6194 100644 >> --- a/test/box/tuple.test.lua >> +++ b/test/box/tuple.test.lua >> @@ -349,4 +349,33 @@ box.tuple.update == t.update >> box.tuple.upsert == t.upsert >> box.tuple.bsize == t.bsize >> >> +-- >> +-- gh-3282: space:frommap(). >> +-- >> +format = {} >> +format[1] = {name = 'aaa', type = 'unsigned'} >> +format[2] = {name = 'bbb', type = 'unsigned'} >> +format[3] = {name = 'ccc', type = 'unsigned'} >> +format[4] = {name = 'ddd', type = 'unsigned'} >> +s = box.schema.create_space('test', {format = format}) >> +s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}) >> +s:frommap({ddd = 1, aaa = 2, bbb = 3}) >> +s:frommap({ddd = 1, aaa = 2, ccc = 3, eee = 4}) >> +s:frommap() >> +s:frommap({}) >> +s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {table = true}) >> +s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {table = false}) >> +s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = box.NULL}) >> +s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {dummy = true}) >> +_ = s:create_index('primary', {parts = {'aaa'}}) >> +tuple = s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}) >> +s:replace(tuple) >> +_, err = box.internal.space._cptr(s) >> +assert(not err) >> +s:drop() >> +_, err = s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}) >> +assert(err ~= nil) >> +_, err = box.internal.space._cptr(s) >> +assert(err ~= nil) >> + >> test_run:cmd("clear filter") >> -- >> 2.7.4 >> >> >