[tarantool-patches] Re: [PATCH v2 1/1] schema:frommap() to create table or tuple
Kirill Shcherbatov
kshcherbatov at tarantool.org
Mon Apr 9 11:30:09 MSK 2018
I've accounted Kostya's comments.
Going to measure performance.
>From ef47d3536b845a84b52be6c84a66aef12fbe8561 Mon Sep 17 00:00:00 2001
From: Kirill Shcherbatov <kshcherbatov at tarantool.org>
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 <kshcherbatov at tarantool.org> [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
>>
>>
>
More information about the Tarantool-patches
mailing list