[tarantool-patches] Re: [PATCH v2 1/1] schema:frommap() to create table or tuple
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Mon Apr 9 13:44:55 MSK 2018
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 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)
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);
>
More information about the Tarantool-patches
mailing list