[tarantool-patches] Re: box: disable sparse optimization in box.tuple.new()

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

LGTM.

On 15/02/2019 16:17, Kirill Shcherbatov wrote:
> Hi! I've accounted Kostya's proposal to implement separate serializer.
> 
> ===================================================
> 
> http://github.com/tarantool/tarantool/tree/kshch/gh-3882-disable-sparse-optimization
> https://github.com/tarantool/tarantool/issues/3882
> 
> v1:
> https://www.freelists.org/post/tarantool-patches/box-disable-sparse-optimization-in-boxtuplenew
> 
> The box.tuple.new() used to call luamp_encode_tuple with
> default LUA serializer config 'luaL_msgpack_default'. This
> routine may consider an array to be excessively sparse when
>    + encode_sparse_ratio > 0
>    + max(table) > encode_sparse_safe
>    + max(table) > count(table) * encode_sparse_ratio.
> Sparse optimization save memory via representing excessively
> sparse tuple as MP_MAP. But Tarantool tuple always must be
> MP_ARRAY so it is not relevant for box.tuple.new semantics.
> So it is disabled with encode_sparse_ratio = 0 in a new local
> serializer config.
> 
> Closes #3882
> ---
>   src/box/lua/tuple.c     | 12 +++++++++-
>   src/lua/utils.c         |  9 ++++++++
>   src/lua/utils.h         |  7 ++++++
>   test/box/tuple.result   | 50 +++++++++++++++++++++++++++++++++++++++++
>   test/box/tuple.test.lua | 26 +++++++++++++++++++++
>   5 files changed, 103 insertions(+), 1 deletion(-)
> 
> diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c
> index 756856f4e..5f2bfc807 100644
> --- a/src/box/lua/tuple.c
> +++ b/src/box/lua/tuple.c
> @@ -58,6 +58,7 @@
>   
>   static const char *tuplelib_name = "box.tuple";
>   static const char *tuple_iteratorlib_name = "box.tuple.iterator";
> +static struct luaL_serializer tuple_serializer;
>   
>   extern char tuple_lua[]; /* Lua source */
>   
> @@ -109,7 +110,7 @@ lbox_tuple_new(lua_State *L)
>   
>   	if (argc == 1 && (lua_istable(L, 1) || luaT_istuple(L, 1))) {
>   		/* New format: box.tuple.new({1, 2, 3}) */
> -		luamp_encode_tuple(L, luaL_msgpack_default, &stream, 1);
> +		luamp_encode_tuple(L, &tuple_serializer, &stream, 1);
>   	} else {
>   		/* Backward-compatible format: box.tuple.new(1, 2, 3). */
>   		mpstream_encode_array(&stream, argc);
> @@ -539,6 +540,15 @@ box_lua_tuple_init(struct lua_State *L)
>   
>   	luamp_set_encode_extension(luamp_encode_extension_box);
>   
> +	/*
> +	 * Create special serializer for box.tuple.new().
> +	 * Disable storage optimization for excessively
> +	 * sparse arrays as a tuple always must be regular
> +	 * MP_ARRAY.
> +	 */
> +	luaL_serializer_create(&tuple_serializer);
> +	tuple_serializer.encode_sparse_ratio = 0;
> +
>   	/* Get CTypeID for `struct tuple' */
>   	int rc = luaL_cdef(L, "struct tuple;");
>   	assert(rc == 0);
> diff --git a/src/lua/utils.c b/src/lua/utils.c
> index 978fe61f1..a418b9586 100644
> --- a/src/lua/utils.c
> +++ b/src/lua/utils.c
> @@ -213,6 +213,15 @@ static struct {
>   	{ NULL, 0, 0, 0},
>   };
>   
> +void
> +luaL_serializer_create(struct luaL_serializer *cfg)
> +{
> +	for (int i = 0; OPTIONS[i].name != NULL; i++) {
> +		int *pval = (int *) ((char *) cfg + OPTIONS[i].offset);
> +		*pval = OPTIONS[i].defvalue;
> +	}
> +}
> +
>   /**
>    * Configure one field in @a cfg.
>    * @param L Lua stack.
> diff --git a/src/lua/utils.h b/src/lua/utils.h
> index a47e3d2b4..96311b75c 100644
> --- a/src/lua/utils.h
> +++ b/src/lua/utils.h
> @@ -241,6 +241,13 @@ luaL_checkserializer(struct lua_State *L) {
>   		luaL_checkudata(L, lua_upvalueindex(1), LUAL_SERIALIZER);
>   }
>   
> +/**
> + * Initialize serializer with default parameters.
> + * @param cfg Serializer to inherit configuration.
> + */
> +void
> +luaL_serializer_create(struct luaL_serializer *cfg);
> +
>   /**
>    * Parse configuration table into @a cfg.
>    * @param L Lua stack.
> diff --git a/test/box/tuple.result b/test/box/tuple.result
> index b42012485..16aa66b1a 100644
> --- a/test/box/tuple.result
> +++ b/test/box/tuple.result
> @@ -1164,3 +1164,53 @@ test_run:cmd("clear filter")
>   ---
>   - true
>   ...
> +--
> +-- gh-3882: Inappropriate storage optimization for sparse arrays
> +--          in box.tuple.new.
> +--
> +t = {}
> +---
> +...
> +t[1] = 1
> +---
> +...
> +t[2] = 2
> +---
> +...
> +t[11] = 11
> +---
> +...
> +box.tuple.new(t)
> +---
> +- [1, 2, null, null, null, null, null, null, null, null, 11]
> +...
> +s2 = box.schema.space.create('test')
> +---
> +...
> +test_run:cmd("setopt delimiter ';'")
> +---
> +- true
> +...
> +s2:format({{name="a", type="str"}, {name="b", type="str", is_nullable=true},
> +           {name="c", type="str", is_nullable=true},
> +           {name="d", type="str", is_nullable=true},
> +           {name="e", type="str", is_nullable=true},
> +           {name="f", type="str", is_nullable=true},
> +           {name="g", type="str", is_nullable=true},
> +           {name="h", type="str", is_nullable=true},
> +           {name="i", type="str", is_nullable=true},
> +           {name="j", type="str", is_nullable=true},
> +           {name="k", type="str", is_nullable=true}});
> +---
> +...
> +test_run:cmd("setopt delimiter ''");
> +---
> +- true
> +...
> +s2:frommap({a="1", k="11"})
> +---
> +- ['1', null, null, null, null, null, null, null, null, null, '11']
> +...
> +s2:drop()
> +---
> +...
> diff --git a/test/box/tuple.test.lua b/test/box/tuple.test.lua
> index 276bb0f67..0c89feace 100644
> --- a/test/box/tuple.test.lua
> +++ b/test/box/tuple.test.lua
> @@ -384,3 +384,29 @@ t2 = box.tuple.new(2)
>   t1 = t1:update{{'+', 1, 1}}
>   
>   test_run:cmd("clear filter")
> +
> +--
> +-- gh-3882: Inappropriate storage optimization for sparse arrays
> +--          in box.tuple.new.
> +--
> +t = {}
> +t[1] = 1
> +t[2] = 2
> +t[11] = 11
> +box.tuple.new(t)
> +
> +s2 = box.schema.space.create('test')
> +test_run:cmd("setopt delimiter ';'")
> +s2:format({{name="a", type="str"}, {name="b", type="str", is_nullable=true},
> +           {name="c", type="str", is_nullable=true},
> +           {name="d", type="str", is_nullable=true},
> +           {name="e", type="str", is_nullable=true},
> +           {name="f", type="str", is_nullable=true},
> +           {name="g", type="str", is_nullable=true},
> +           {name="h", type="str", is_nullable=true},
> +           {name="i", type="str", is_nullable=true},
> +           {name="j", type="str", is_nullable=true},
> +           {name="k", type="str", is_nullable=true}});
> +test_run:cmd("setopt delimiter ''");
> +s2:frommap({a="1", k="11"})
> +s2:drop()
> -- 
> 2.20.1
>