[tarantool-patches] Re: [PATCH v2 1/1] schema:frommap() to create table or tuple

[Kirill Shcherbatov <kshcherbatov@tarantool.org>]

I've accounted Kostya's comments. 
Going to measure performance.


From ef47d3536b845a84b52be6c84a66aef12fbe8561 Mon Sep 17 00:00:00 2001
From: Kirill Shcherbatov <kshcherbatov@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@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
>>
>>
>