* [tarantool-patches] [PATCH v1 1/1] box: disable sparse optimization in box.tuple.new() @ 2019-02-11 10:43 Kirill Shcherbatov 2019-02-12 11:18 ` [tarantool-patches] " Kirill Shcherbatov 2019-02-18 8:45 ` [tarantool-patches] Re: [PATCH v1 1/1] " Kirill Yukhin 0 siblings, 2 replies; 8+ messages in thread From: Kirill Shcherbatov @ 2019-02-11 10:43 UTC (permalink / raw) To: tarantool-patches, v.shpilevoy; +Cc: Kirill Shcherbatov 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 | 5 ++++- test/box/tuple.result | 50 +++++++++++++++++++++++++++++++++++++++++ test/box/tuple.test.lua | 26 +++++++++++++++++++++ 3 files changed, 80 insertions(+), 1 deletion(-) diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c index 756856f4e..3bd4ab006 100644 --- a/src/box/lua/tuple.c +++ b/src/box/lua/tuple.c @@ -107,9 +107,12 @@ lbox_tuple_new(lua_State *L) mpstream_init(&stream, buf, ibuf_reserve_cb, ibuf_alloc_cb, luamp_error, 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); + struct luaL_serializer cfg = *luaL_msgpack_default; + cfg.encode_sparse_ratio = 0; + luamp_encode_tuple(L, &cfg, &stream, 1); } else { /* Backward-compatible format: box.tuple.new(1, 2, 3). */ mpstream_encode_array(&stream, argc); 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [tarantool-patches] box: disable sparse optimization in box.tuple.new() 2019-02-11 10:43 [tarantool-patches] [PATCH v1 1/1] box: disable sparse optimization in box.tuple.new() Kirill Shcherbatov @ 2019-02-12 11:18 ` Kirill Shcherbatov 2019-02-12 11:33 ` [tarantool-patches] " Konstantin Osipov 2019-02-12 17:50 ` Vladislav Shpilevoy 2019-02-18 8:45 ` [tarantool-patches] Re: [PATCH v1 1/1] " Kirill Yukhin 1 sibling, 2 replies; 8+ messages in thread From: Kirill Shcherbatov @ 2019-02-12 11:18 UTC (permalink / raw) To: tarantool-patches, Vladislav Shpilevoy 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 | 9 ++++++++ test/box/tuple.result | 50 +++++++++++++++++++++++++++++++++++++++++ test/box/tuple.test.lua | 26 +++++++++++++++++++++ 3 files changed, 85 insertions(+) diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c index 756856f4e..52fb79d5b 100644 --- a/src/box/lua/tuple.c +++ b/src/box/lua/tuple.c @@ -108,8 +108,17 @@ lbox_tuple_new(lua_State *L) luamp_error, L); if (argc == 1 && (lua_istable(L, 1) || luaT_istuple(L, 1))) { + /** + * Disable storage optimization for excessively + * sparse arrays as a tuple always must be regular + * MP_ARRAY. + */ + int encode_sparse_ratio = + luaL_msgpack_default->encode_sparse_ratio; + luaL_msgpack_default->encode_sparse_ratio = 0; /* New format: box.tuple.new({1, 2, 3}) */ luamp_encode_tuple(L, luaL_msgpack_default, &stream, 1); + luaL_msgpack_default->encode_sparse_ratio = encode_sparse_ratio; } else { /* Backward-compatible format: box.tuple.new(1, 2, 3). */ mpstream_encode_array(&stream, argc); 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [tarantool-patches] Re: box: disable sparse optimization in box.tuple.new() 2019-02-12 11:18 ` [tarantool-patches] " Kirill Shcherbatov @ 2019-02-12 11:33 ` Konstantin Osipov 2019-02-12 17:50 ` Vladislav Shpilevoy 1 sibling, 0 replies; 8+ messages in thread From: Konstantin Osipov @ 2019-02-12 11:33 UTC (permalink / raw) To: tarantool-patches; +Cc: Vladislav Shpilevoy * Kirill Shcherbatov <kshcherbatov@tarantool.org> [19/02/12 14:23]: > 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. Why not keep a separate config around instead of tweaking the default config each time? -- Konstantin Osipov, Moscow, Russia, +7 903 626 22 32 http://tarantool.io - www.twitter.com/kostja_osipov ^ permalink raw reply [flat|nested] 8+ messages in thread
* [tarantool-patches] Re: box: disable sparse optimization in box.tuple.new() 2019-02-12 11:18 ` [tarantool-patches] " Kirill Shcherbatov 2019-02-12 11:33 ` [tarantool-patches] " Konstantin Osipov @ 2019-02-12 17:50 ` Vladislav Shpilevoy 2019-02-13 12:19 ` Konstantin Osipov 2019-02-15 15:17 ` Kirill Shcherbatov 1 sibling, 2 replies; 8+ messages in thread From: Vladislav Shpilevoy @ 2019-02-12 17:50 UTC (permalink / raw) To: tarantool-patches, Kirill Shcherbatov Hi! Thanks for the patch! On 12/02/2019 14:18, Kirill Shcherbatov wrote: > 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 > --- Please, next time put here links to the branch and issue. The patch is ok for me. But I see, that Kostja complaints, that you could create a special serializer for it. I agree, but I see a drawback about it. Separate serializer would be unconfigurable. Now a user can change serializer by making require('msgpack').cfg(...). So the issue can be fixed manually like this even on 2.1: [002] Test failed! Result content mismatch: [002] --- box/tuple.result Tue Feb 12 18:26:58 2019 [002] +++ box/tuple.reject Tue Feb 12 18:42:10 2019 [002] @@ -1168,6 +1168,12 @@ [002] -- gh-3882: Inappropriate storage optimization for sparse arrays [002] -- in box.tuple.new. [002] -- [002] +m = require('msgpack') [002] +--- [002] +... [002] +m.cfg{encode_sparse_ratio = 0} [002] +--- [002] +... I added these two lines without a patch, and the test passed. With Kostja's way of implementation, tuple encoding would be impossible to configure. If Kostja thinks, that your way does not work, then do whatever he wants, please. Ask him for clarification. > src/box/lua/tuple.c | 9 ++++++++ > test/box/tuple.result | 50 +++++++++++++++++++++++++++++++++++++++++ > test/box/tuple.test.lua | 26 +++++++++++++++++++++ > 3 files changed, 85 insertions(+) > ^ permalink raw reply [flat|nested] 8+ messages in thread
* [tarantool-patches] Re: box: disable sparse optimization in box.tuple.new() 2019-02-12 17:50 ` Vladislav Shpilevoy @ 2019-02-13 12:19 ` Konstantin Osipov 2019-02-15 15:17 ` Kirill Shcherbatov 1 sibling, 0 replies; 8+ messages in thread From: Konstantin Osipov @ 2019-02-13 12:19 UTC (permalink / raw) To: tarantool-patches; +Cc: Kirill Shcherbatov * Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [19/02/12 20:54]: Please use a separate serializer configuration. Both solutions are bad, but at least I see no forward compatibility issues with a separate configuration. If we break any existing code by changing the behavior, we'll expose the config as a separate one for box.tuple.new() or frommap(). > Hi! Thanks for the patch! > > On 12/02/2019 14:18, Kirill Shcherbatov wrote: > > 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 > > --- > > Please, next time put here links to the branch and issue. > > The patch is ok for me. But I see, that Kostja complaints, that > you could create a special serializer for it. I agree, but I see > a drawback about it. Separate serializer would be unconfigurable. > Now a user can change serializer by making require('msgpack').cfg(...). > So the issue can be fixed manually like this even on 2.1: > > [002] Test failed! Result content mismatch: > [002] --- box/tuple.result Tue Feb 12 18:26:58 2019 > [002] +++ box/tuple.reject Tue Feb 12 18:42:10 2019 > [002] @@ -1168,6 +1168,12 @@ > [002] -- gh-3882: Inappropriate storage optimization for sparse arrays > [002] -- in box.tuple.new. > [002] -- > [002] +m = require('msgpack') > [002] +--- > [002] +... > [002] +m.cfg{encode_sparse_ratio = 0} > [002] +--- > [002] +... > > I added these two lines without a patch, and the test passed. > > With Kostja's way of implementation, tuple encoding would be > impossible to configure. > > If Kostja thinks, that your way does not work, then do whatever > he wants, please. Ask him for clarification. > > > src/box/lua/tuple.c | 9 ++++++++ > > test/box/tuple.result | 50 +++++++++++++++++++++++++++++++++++++++++ > > test/box/tuple.test.lua | 26 +++++++++++++++++++++ > > 3 files changed, 85 insertions(+) > > -- Konstantin Osipov, Moscow, Russia, +7 903 626 22 32 http://tarantool.io - www.twitter.com/kostja_osipov ^ permalink raw reply [flat|nested] 8+ messages in thread
* [tarantool-patches] Re: box: disable sparse optimization in box.tuple.new() 2019-02-12 17:50 ` Vladislav Shpilevoy 2019-02-13 12:19 ` Konstantin Osipov @ 2019-02-15 15:17 ` Kirill Shcherbatov 2019-02-15 21:31 ` Vladislav Shpilevoy 1 sibling, 1 reply; 8+ messages in thread From: Kirill Shcherbatov @ 2019-02-15 15:17 UTC (permalink / raw) To: tarantool-patches, Vladislav Shpilevoy; +Cc: Kostya Osipov 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [tarantool-patches] Re: box: disable sparse optimization in box.tuple.new() 2019-02-15 15:17 ` Kirill Shcherbatov @ 2019-02-15 21:31 ` Vladislav Shpilevoy 0 siblings, 0 replies; 8+ messages in thread From: Vladislav Shpilevoy @ 2019-02-15 21:31 UTC (permalink / raw) To: Kirill Shcherbatov, tarantool-patches, Kirill Yukhin; +Cc: Kostya Osipov 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 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* [tarantool-patches] Re: [PATCH v1 1/1] box: disable sparse optimization in box.tuple.new() 2019-02-11 10:43 [tarantool-patches] [PATCH v1 1/1] box: disable sparse optimization in box.tuple.new() Kirill Shcherbatov 2019-02-12 11:18 ` [tarantool-patches] " Kirill Shcherbatov @ 2019-02-18 8:45 ` Kirill Yukhin 1 sibling, 0 replies; 8+ messages in thread From: Kirill Yukhin @ 2019-02-18 8:45 UTC (permalink / raw) To: tarantool-patches; +Cc: v.shpilevoy, Kirill Shcherbatov Hello, On 11 Feb 13:43, Kirill Shcherbatov wrote: > 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 I've checked your patch into 1.10 and 2.1 branches. -- Regards, Kirill Yukhin ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-02-18 8:45 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-02-11 10:43 [tarantool-patches] [PATCH v1 1/1] box: disable sparse optimization in box.tuple.new() Kirill Shcherbatov 2019-02-12 11:18 ` [tarantool-patches] " Kirill Shcherbatov 2019-02-12 11:33 ` [tarantool-patches] " Konstantin Osipov 2019-02-12 17:50 ` Vladislav Shpilevoy 2019-02-13 12:19 ` Konstantin Osipov 2019-02-15 15:17 ` Kirill Shcherbatov 2019-02-15 21:31 ` Vladislav Shpilevoy 2019-02-18 8:45 ` [tarantool-patches] Re: [PATCH v1 1/1] " Kirill Yukhin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox