Re: [Tarantool-patches] [PATCH 1/1] box: fixed box.info:memory()

[Olga Arkhangelskaia <arkholga@tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 4054 bytes --]

Hi Igor!

I have fixed all issue you have mentioned and rebased and updated branch.

Since there is no much have changed I will write here only new commit 
message:

Fix the return value of box.info:memory(). It used to return the same
table as the box.info().

If one removes metatable of the box.info:memory():
tarantool> setmetatable(box.info:memory(), nil)
---
- cache: 0
   lua: 1855568
   data: 37976
   index: 1196032
   net: 589824
   tx: 0
   version: 2.5.0-158-g667145aff
   package: Tarantool

When box.info:memory is called, it has box.info table as a first
argument (seehttps://www.lua.org/manual/5.1/manual.html#2.8). While
printing the table, lbox_info_memory_call adds box.info.memory's fields
to the box.info table. To avoid this effect and return just
box.info.memory we put an empty table on the Lua stack at the beginning
of the lbox_info_memory_call function. Instead adding fields to box.info
table lbox_info_memory_call fills that empty table.

Closes  #4688  <https://github.com/tarantool/tarantool/issues/4688>

16.07.2020 20:56, Igor Munkin пишет:
> Olya,
>
> Thanks for your updates! The patch is OK, but I still have several
> comments regarding it, so I pasted it below. Besides, the branch is
> extremely outdated and based on 2.2 (it might complicate the merge
> process).
>
> | box: fix box.info:memory()
> |
> | Fix the return value of box.info:memory(). It used to return the same table
> | as the box.info().
> |
> | If one removes metatable of the box.info:memory():
> | tarantool> setmetatable(box.info:memory(), nil)
> | ---
> | - cache: 0
> |   lua: 1855568
> |   data: 37976
> |   index: 1196032
> |   net: 589824
> |   tx: 0
> |   version: 2.5.0-158-g667145aff
> |   package: Tarantool
> |
> | While printing the table, lbox_info_memory_call adds box.info.memory's fields to
> | the box.info table. To avoid this effect and return just box.info.memory we put
> | an empty table on the Lua stack at the beginning of the lbox_info_memory_call
> | function.
> |
> | Closes #4688
>
> At first, the message still exceeds 72 symbols. Sasha has already
> pointed to this several times. Let's make Sasha a bit happier!
>
> Furthermore, you described why the output is the same as the one
> obtained via box.info, but stripped everything regarding why box.info is
> used here. I still guess it's worth to be mentioned, even if it is
> perfectly described in Lua reference manual and PIL.
>
> | diff --git a/src/box/lua/info.c b/src/box/lua/info.c
> | index d0e553b1d..e5b1c32f0 100644
> | --- a/src/box/lua/info.c
> | +++ b/src/box/lua/info.c
> | @@ -322,6 +322,8 @@ lbox_info_memory_call(struct lua_State *L)
> |  	struct engine_memory_stat stat;
> |  	engine_memory_stat(&stat);
> |
> | +	lua_newtable(L);
> | +
>
> I agree with Sasha, regarding <lua_createtable> usage: it can
> preallocate the exact required size memory area, so I see no reasons to
> ignore such benefit.
>
> |  	lua_pushstring(L, "data");
> |  	luaL_pushuint64(L, stat.data);
> |  	lua_settable(L, -3);
> | diff --git a/test/box-tap/gh-4688-box-info-memory.test.lua b/test/box-tap/gh-4688-box-info-memory.test.lua
> | new file mode 100755
> | index 000000000..8d6aa0e00
> | --- /dev/null
> | +++ b/test/box-tap/gh-4688-box-info-memory.test.lua
> | @@ -0,0 +1,28 @@
> | +#!/usr/bin/env tarantool
> | +
> | +--
> | +-- gh-4688: box.info:memory() displayed full content of box.info
> | +--
> | +local tap = require('tap')
> | +
> | +box.cfg()
> | +
> | +local test = tap.test('gh-4688-box.info:memory-wrong-result')
> | +test:plan(1)
> | +
> | +local a = box.info.memory()
> | +local b = box.info:memory()
>
> These variables are excess.
>
> | +
> | +local function get_keys(t)
> | +    local keys = {}
> | +    for k, v in pairs(t) do
> | +        table.insert(keys, k)
> | +    end
> | +    return keys
> | +end
> | +
> | +local keys_1 = get_keys(box.info.memory())
> | +local keys_2 = get_keys(box.info:memory())
> | +test:is_deeply(keys_1, keys_2, "box.info:memory coincide with box.info.memory")
> | +
> | +os.exit(test:check() and 0 or 1)
>

[-- Attachment #2: Type: text/html, Size: 5668 bytes --]