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

[Vladislav Shpilevoy <v.shpilevoy@tarantool.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 <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)

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);
>