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

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

[Olga Arkhangelskaia]

Fix box.info:memory() output. Now it has the same output as box.info.memory().
---
Closes 4668
 src/box/lua/info.c                            |  1 +
 test/box-tap/gh-4668-box-info-memory.test.lua | 15 +++++++++++++++
 2 files changed, 16 insertions(+)
 create mode 100755 test/box-tap/gh-4668-box-info-memory.test.lua

diff --git a/src/box/lua/info.c b/src/box/lua/info.c
index d0e553b1d..3d515ae9e 100644
--- a/src/box/lua/info.c
+++ b/src/box/lua/info.c
@@ -322,6 +322,7 @@ lbox_info_memory_call(struct lua_State *L)
 	struct engine_memory_stat stat;
 	engine_memory_stat(&stat);
 
+	lua_newtable(L);
 	lua_pushstring(L, "data");
 	luaL_pushuint64(L, stat.data);
 	lua_settable(L, -3);
diff --git a/test/box-tap/gh-4668-box-info-memory.test.lua b/test/box-tap/gh-4668-box-info-memory.test.lua
new file mode 100755
index 000000000..1a13fa903
--- /dev/null
+++ b/test/box-tap/gh-4668-box-info-memory.test.lua
@@ -0,0 +1,15 @@
+#!/usr/bin/env tarantool
+--
+-- gh-4668: box.info:memory() displayed full content of box.info
+--
+local tap = require('tap')
+local test = tap.test("Tarantool 4668")
+test:plan(1)
+
+box.cfg()
+
+a = box.info.memory()
+b = box.info:memory()
+
+test:is(table.concat(a), table.concat(b), "box.info:memory")
+os.exit(0)
-- 
2.20.1 (Apple Git-117)

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

[Igor Munkin]

Olya,

Thanks for the patch!

On 29.06.20, Olga Arkhangelskaia wrote:
> The patch fixes the output of box.info:memory(). It used to return the same
> table as the box.info().
> 
> box.info.memory() can be written as box.info[“memory”](), that later has the only
> one argument on the stack - box.info.memory table we need to fill:
> box.info.memory() -> getmetatable(box.info.memory).__call(box.info.memory)[1]
> box.info:memory() call is the same as box.info[“memory”](box.info). So, it
> results in extra argument on the stack[1].
> box.info:mem()->getmetatable(box.info.memory).__call(box.info.memory, box.info).
> When function tries to fill box.info table - __call metamethod of box.info is
> trigged - resulting in box.info table returned as the result.

Nice passage! I guess it should be polished an added to the original
commit message.

> 
> There are two options to get rid if extra box.info table:
> 1. Create new table in the beginning of the function(eg. box.info.gc).
> Every time box.info.memory is called it will generate new table with the fresh
> info.
> 2. The second way is to ignore box.info argument on the stack and fill
> directly box.info.memory table, that was passed as an argument.
> 
> I have implemented the first approach because there is box.info.gc works
> the same way and we only need to add one line of code.
> However, I do not know why it was done in such a way on the first place.
> So if you have pros for the second options, please share with me.

I have no idea why it is implemented in such complex way, maybe Sasha
does? Why box.info.memory yields an empty "callable" table on each
lookup? Why it can't just return a function to be called or a table with
memory metrics as a result of the lookup? Unfortunately the latter
approach breaks the backward compatibility but the first one can save
some time on short-term objects creation (I guess no one checks
box.info.memory type). Thoughts? Please also consider the comments I
left for the patch itself.

> 
> [1] https://www.lua.org/manual/5.1/manual.html#2.8
> 
> @Changelog:
> To retrieve information about memory usage box.info:memory() can be used.
> Olga Arkhangelskaia (1):
>   box: fixed box.info:memory()
> 
>  src/box/lua/info.c                            |  1 +
>  test/box-tap/gh-4668-box-info-memory.test.lua | 15 +++++++++++++++
>  2 files changed, 16 insertions(+)
>  create mode 100755 test/box-tap/gh-4668-box-info-memory.test.lua
> 
> -- 
> 2.20.1 (Apple Git-117)
> 

-- 
Best regards,
IM

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

[Igor Munkin]

Olya,

Thanks for the patch! I see the patch subject differs from the
corresponding commit subject you've pushed to the upstream. What is the
final one? Please also consider several nits I left below.

On 29.06.20, Olga Arkhangelskaia wrote:
> Fix box.info:memory() output. Now it has the same output as box.info.memory().
> ---
> Closes 4668
>  src/box/lua/info.c                            |  1 +
>  test/box-tap/gh-4668-box-info-memory.test.lua | 15 +++++++++++++++

Typo: s/gh-4668/gh-4688/.

>  2 files changed, 16 insertions(+)
>  create mode 100755 test/box-tap/gh-4668-box-info-memory.test.lua
> 

<snipped>

> diff --git a/test/box-tap/gh-4668-box-info-memory.test.lua b/test/box-tap/gh-4668-box-info-memory.test.lua
> new file mode 100755
> index 000000000..1a13fa903
> --- /dev/null
> +++ b/test/box-tap/gh-4668-box-info-memory.test.lua
> @@ -0,0 +1,15 @@
> +#!/usr/bin/env tarantool
> +--
> +-- gh-4668: box.info:memory() displayed full content of box.info

Ditto.

> +--
> +local tap = require('tap')
> +local test = tap.test("Tarantool 4668")

Ditto.

> +test:plan(1)
> +
> +box.cfg()
> +
> +a = box.info.memory()
> +b = box.info:memory()
> +
> +test:is(table.concat(a), table.concat(b), "box.info:memory")
> +os.exit(0)
> -- 
> 2.20.1 (Apple Git-117)
> 

-- 
Best regards,
IM

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

[Olga Arkhangelskaia]

Hi Igor!

Thanks for the review!

I am sorry for  the confusion with the subject. It is box: fix 
box.info:memory().

I have fixed issue number all over the patch and added the paragraph  
about arguments to the commit messae/

Now it looks like:

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

Any box.info.xxx() is the same as box.info[“xxx”]().
E.g. box.info.memory() ->
getmetatable(box.info.memory).__call(box.info.memory)[1]
After __index and __call metamethods, the final function that fills 
xxx-table,
has the only argument - empty table to fill.
When box.info:xxx() is invoked it automatically passes one argument:
box.info[“xxx”](box.info). So the resulting call has 2 arguments on the 
stack.
box.info:xxx()->getmetatable(box.info.xxx).__call(box.info.xxx, box.info)
When function tries to fill box.info table - __call metamethod of 
box.info is
trigged.

box.info.gc does not have this problem because of an extra table that is
created in the beginning of the bottom function. box.info.memory follows
the same way.

[1] https://www.lua.org/manual/5.1/manual.html#2.8

Closes 4688

02.07.2020 0:34, Igor Munkin пишет:
> Olya,
>
> Thanks for the patch! I see the patch subject differs from the
> corresponding commit subject you've pushed to the upstream. What is the
> final one? Please also consider several nits I left below.
>
> On 29.06.20, Olga Arkhangelskaia wrote:
>> Fix box.info:memory() output. Now it has the same output as box.info.memory().
>> ---
>> Closes 4668
>>   src/box/lua/info.c                            |  1 +
>>   test/box-tap/gh-4668-box-info-memory.test.lua | 15 +++++++++++++++
> Typo: s/gh-4668/gh-4688/.
>
>>   2 files changed, 16 insertions(+)
>>   create mode 100755 test/box-tap/gh-4668-box-info-memory.test.lua
>>
> <snipped>
>
>> diff --git a/test/box-tap/gh-4668-box-info-memory.test.lua b/test/box-tap/gh-4668-box-info-memory.test.lua
>> new file mode 100755
>> index 000000000..1a13fa903
>> --- /dev/null
>> +++ b/test/box-tap/gh-4668-box-info-memory.test.lua
>> @@ -0,0 +1,15 @@
>> +#!/usr/bin/env tarantool
>> +--
>> +-- gh-4668: box.info:memory() displayed full content of box.info
> Ditto.
>
>> +--
>> +local tap = require('tap')
>> +local test = tap.test("Tarantool 4668")
> Ditto.
>
>> +test:plan(1)
>> +
>> +box.cfg()
>> +
>> +a = box.info.memory()
>> +b = box.info:memory()
>> +
>> +test:is(table.concat(a), table.concat(b), "box.info:memory")
>> +os.exit(0)
>> -- 
>> 2.20.1 (Apple Git-117)
>>

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

[Alexander Turenko]

Pasted the actual patch to comment in-place.

> commit ad00de576ab0300a3e48be2cdda2bef5938eb40e
> Author: Olga Arkhangelskaia <arkholga@tarantool.org>
> Date:   Mon Jun 29 12:14:24 2020 +0300
> 
>     box: fix box.info:memory()
>     
>     Fix the output of box.info:memory(). It used to return the same table as the
>     box.info().

Nit: 72 symbols at max. Here and below.

Nit: I would say 'the return value', because 'the output' looks more

>     
>     Any box.info.xxx() is the same as box.info[“xxx”]().
>     E.g. box.info.memory() ->
>     getmetatable(box.info.memory).__call(box.info.memory)[1]

It is ambiguous: whether [1] is first element or an array or a reference
for the link below.

>     After __index and __call metamethods, the final function that fills xxx-table,
>     has the only argument - empty table to fill.
>     When box.info:xxx() is invoked it automatically passes one argument:
>     box.info[“xxx”](box.info). So the resulting call has 2 arguments on the stack.
>     box.info:xxx()->getmetatable(box.info.xxx).__call(box.info.xxx, box.info)

Nit: I would surround '->' with spaces for readability.

>     When function tries to fill box.info table - __call metamethod of box.info is
>     trigged.

It is not correct. It would be __newindex metamethod, but it is not
defined on the table. I guess you was misguided by a console output,
because of __serialize method. In fact <box.info> table will be filled
with 'cache', 'lua', 'data' and other box.info.memory() fields. You can
verify it youself:

 | tarantool> setmetatable(box.info:memory(), nil)
 | ---
 | - cache: 0
 |   lua: 1076262
 |   data: 37816
 |   index: 1097728
 |   net: 589824
 |   tx: 0
 |   version: 2.5.0-208-gcf6975793
 |   package: Tarantool
 | ...

Despite changes I requested above I appecitate the intention to clarify
the change.

>     
>     box.info.gc does not have this problem because of an extra table that is
>     created in the beginning of the bottom function. box.info.memory follows
>     the same way.
>     
>     [1] https://www.lua.org/manual/5.1/manual.html#2.8

Nit: Markdown provides '[1]: https://' syntax for reference style links,
but mayble there are others markups, where syntax is the same as above.
I don't know for sure. Personally I use markdown (but sometimes with
asciidoc titles) for texts with several simple markup elements like
hyperlinks. Many developers aware of this syntax.

>     
>     Closes 4688

Typo: no hash symbol.

> 
> diff --git a/src/box/lua/info.c b/src/box/lua/info.c
> index d0e553b1d..3d515ae9e 100644
> --- a/src/box/lua/info.c
> +++ b/src/box/lua/info.c
> @@ -322,6 +322,7 @@ lbox_info_memory_call(struct lua_State *L)
>  	struct engine_memory_stat stat;
>  	engine_memory_stat(&stat);
>  
> +	lua_newtable(L);

Nit: Six same structured blocks are below. But after the change the
first one will differs. Please, add an empty line after lua_newtable().

BTW, we can use lua_createtable() to allocate a hashmap of necessary
size before inserting elements. It is to avoid resizing of the table
(don't know whether it is actual for small map sizes like 6).

The change itself is okay for me.

>  	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..63dcdab8f
> --- /dev/null
> +++ b/test/box-tap/gh-4688-box-info-memory.test.lua
> @@ -0,0 +1,15 @@
> +#!/usr/bin/env tarantool
> +--
> +-- gh-4688: box.info:memory() displayed full content of box.info
> +--
> +local tap = require('tap')
> +local test = tap.test("Tarantool 4688")

Nit: Single and double quotes are used without any system.

Nit: See how other top level test cases are named: `grep tap.test
test/*/gh-*`.

> +test:plan(1)
> +
> +box.cfg()
> +
> +a = box.info.memory()
> +b = box.info:memory()
> +
> +test:is(table.concat(a), table.concat(b), "box.info:memory")

First, 'lua' values likely will be different. Second, table.concat()
concatenates elements of an array like {'x', 'y', 'z'}. It is not for
maps (it just gives an empty string). The result is that the test passes
ever without the fix.

Most obvious way would be using of test:is_deeply(), but since 'lua'
field may vary, we can do one of the following ways:

 | a.lua = a.lua and '<stripped>' or nil
 | b.lua = b.lua and '<stripped>' or nil
 |
 | test:is_deeply(a, b, 'box.info:memory() is the same as box.info.memory()')

Or

 | 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, <...>)

Feel free to use any variant or provide your own.

> +os.exit(0)

Please, set exit code appropriately (see [1]).

[1]: https://github.com/tarantool/doc/issues/1004

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

[Alexander Turenko]

> > There are two options to get rid if extra box.info table:
> > 1. Create new table in the beginning of the function(eg. box.info.gc).
> > Every time box.info.memory is called it will generate new table with the fresh
> > info.
> > 2. The second way is to ignore box.info argument on the stack and fill
> > directly box.info.memory table, that was passed as an argument.
> > 
> > I have implemented the first approach because there is box.info.gc works
> > the same way and we only need to add one line of code.
> > However, I do not know why it was done in such a way on the first place.
> > So if you have pros for the second options, please share with me.
> 
> I have no idea why it is implemented in such complex way, maybe Sasha
> does? Why box.info.memory yields an empty "callable" table on each
> lookup? Why it can't just return a function to be called or a table with
> memory metrics as a result of the lookup? Unfortunately the latter
> approach breaks the backward compatibility but the first one can save
> some time on short-term objects creation (I guess no one checks
> box.info.memory type). Thoughts? Please also consider the comments I
> left for the patch itself.

I don't see a reason. The history of src/box/lua/info.c changes shows
that this way was initially implemented for box.info.phia() (which was
renamed later to box.info.vinyl()). Then box.info.memory(),
box.info.gc() and box.info.sql() were added in the same way.
box.info.phia() was moved from box.phia().

I agree with you. We should define a case to estimate impact of
replacing a table + metamethod with a function. Not even to make a
decision whether it worth to change, but to imagine the situation at
whole.

I would consider metrics collection case using tarantool/metrics every
minute when default metrics are enabled. I guess it'll call
box.info.vinyl(), box.info.memory() and box.info.gc() once for each
metrics collection. So the proposed change will safe 3 extra short-term
object creations per minute.

I don't see a case when those functions should be called more often and
become a part of hot path. So I would say that reducing of GC object
allocations here does not look worthful for me considering possible
impact of subtle differences (like serialization of `box.info` or other
differences we can miss) that may fail some scripts or tools.

> 
> > 
> > [1] https://www.lua.org/manual/5.1/manual.html#2.8
> > 
> > @Changelog:
> > To retrieve information about memory usage box.info:memory() can be used.

If you are a user, which read release notes and doesn't aware of the
problem (and don't remember whether box.info.memory() or
box.info:memory() is suggested by the documentation), then it is hard to
understand what was changed. I would explicitly mention
`box.info.memory()` variant of the call: this way the idea of the change
would be more clear.

BTW, don't forget to include an issue number (see examples in existing
release notes on GitHub).

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

[Olga Arkhangelskaia]

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

Hi Alexandr!

Thanks for the review! I have fixed all nits and rewrite the commit message.

Fix the return value of box.info:memory(). It used to return the same 
table as the box.info(). | box.info.memory() = box.info[“memory”]() | 
box.info.memory() -> 
getmetatable(box.info.memory).__call(box.info.memory) | 
box.info:memory() = box.info[“memory”](box.info) | box.info:memory() -> 
getmetatable(box.info...).__call(box.info.., box.info) See 
https://www.lua.org/manual/5.1/manual.html#2.8.An extra argument on Lua 
stack in the second case is box.info table with __serialize metamethod. 
When all internal functions are over, memory table is pushed on the 
stack. When console prints the output (converts lua table to yaml format 
and prints) __serialize metamethod of box.info is called. __serilize 
metamethod of box.info shows only some predefined fields and completely 
ignores memory table. To avoid this call, patch pushes an extra empty 
table on Lua stack. When console calls ymal.serializer to print lua 
table, box.info will be ignored. Closes #4688 
<https://github.com/tarantool/tarantool/issues/4688>

09.07.2020 4:08, Alexander Turenko пишет:
> Pasted the actual patch to comment in-place.
>
>> commit ad00de576ab0300a3e48be2cdda2bef5938eb40e
>> Author: Olga Arkhangelskaia <arkholga@tarantool.org>
>> Date:   Mon Jun 29 12:14:24 2020 +0300
>>
>>      box: fix box.info:memory()
>>      
>>      Fix the output of box.info:memory(). It used to return the same table as the
>>      box.info().
> Nit: 72 symbols at max. Here and below.
>
> Nit: I would say 'the return value', because 'the output' looks more
fixed
>
>>      
>>      Any box.info.xxx() is the same as box.info[“xxx”]().
>>      E.g. box.info.memory() ->
>>      getmetatable(box.info.memory).__call(box.info.memory)[1]
> It is ambiguous: whether [1] is first element or an array or a reference
> for the link below.
fixed
>
>>      After __index and __call metamethods, the final function that fills xxx-table,
>>      has the only argument - empty table to fill.
>>      When box.info:xxx() is invoked it automatically passes one argument:
>>      box.info[“xxx”](box.info). So the resulting call has 2 arguments on the stack.
>>      box.info:xxx()->getmetatable(box.info.xxx).__call(box.info.xxx, box.info)
> Nit: I would surround '->' with spaces for readability.
fixed
>>      When function tries to fill box.info table - __call metamethod of box.info is
>>      trigged.
> It is not correct. It would be __newindex metamethod, but it is not
> defined on the table. I guess you was misguided by a console output,
> because of __serialize method. In fact <box.info> table will be filled
> with 'cache', 'lua', 'data' and other box.info.memory() fields. You can
> verify it youself:
fixed
>
>   | tarantool> setmetatable(box.info:memory(), nil)
>   | ---
>   | - cache: 0
>   |   lua: 1076262
>   |   data: 37816
>   |   index: 1097728
>   |   net: 589824
>   |   tx: 0
>   |   version: 2.5.0-208-gcf6975793
>   |   package: Tarantool
>   | ...
>
> Despite changes I requested above I appecitate the intention to clarify
> the change.
>
>>      
>>      box.info.gc does not have this problem because of an extra table that is
>>      created in the beginning of the bottom function. box.info.memory follows
>>      the same way.
>>      
>>      [1] https://www.lua.org/manual/5.1/manual.html#2.8
Changed
> Nit: Markdown provides '[1]: https://' syntax for reference style links,
> but mayble there are others markups, where syntax is the same as above.
> I don't know for sure. Personally I use markdown (but sometimes with
> asciidoc titles) for texts with several simple markup elements like
> hyperlinks. Many developers aware of this syntax.
>
>>      
>>      Closes 4688
> Typo: no hash symbol.
Fixed
>> diff --git a/src/box/lua/info.c b/src/box/lua/info.c
>> index d0e553b1d..3d515ae9e 100644
>> --- a/src/box/lua/info.c
>> +++ b/src/box/lua/info.c
>> @@ -322,6 +322,7 @@ lbox_info_memory_call(struct lua_State *L)
>>   	struct engine_memory_stat stat;
>>   	engine_memory_stat(&stat);
>>   
>> +	lua_newtable(L);
> Nit: Six same structured blocks are below. But after the change the
> first one will differs. Please, add an empty line after lua_newtable().
added an epmty line.
>
> BTW, we can use lua_createtable() to allocate a hashmap of necessary
> size before inserting elements. It is to avoid resizing of the table
> (don't know whether it is actual for small map sizes like 6).
>
> The change itself is okay for me.
>
>>   	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..63dcdab8f
>> --- /dev/null
>> +++ b/test/box-tap/gh-4688-box-info-memory.test.lua
>> @@ -0,0 +1,15 @@
>> +#!/usr/bin/env tarantool
>> +--
>> +-- gh-4688: box.info:memory() displayed full content of box.info
>> +--
>> +local tap = require('tap')
>> +local test = tap.test("Tarantool 4688")
> Nit: Single and double quotes are used without any system.
>
> Nit: See how other top level test cases are named: `grep tap.test
> test/*/gh-*`.
>> +test:plan(1)
>> +
>> +box.cfg()
>> +
>> +a = box.info.memory()
>> +b = box.info:memory()
>> +
>> +test:is(table.concat(a), table.concat(b), "box.info:memory")
> First, 'lua' values likely will be different. Second, table.concat()
> concatenates elements of an array like {'x', 'y', 'z'}. It is not for
> maps (it just gives an empty string). The result is that the test passes
> ever without the fix.
>
> Most obvious way would be using of test:is_deeply(), but since 'lua'
> field may vary, we can do one of the following ways:
>
>   | a.lua = a.lua and '<stripped>' or nil
>   | b.lua = b.lua and '<stripped>' or nil
>   |
>   | test:is_deeply(a, b, 'box.info:memory() is the same as box.info.memory()')
>
> Or
>
>   | 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, <...>)
New test:
  --
  -- gh-4688: box.info:memory() displayed full content of box.info
  --
  local tap = require('tap')
-local test = tap.test("Tarantool 4688")
-test:plan(1)
~
  box.cfg()
~
-a = box.info.memory()
-b = box.info:memory()
+local test = tap.test('gh-4688-box.info:memory-wrong-result')
+test:plan(1)
+
+local a = box.info.memory()
+local b = box.info:memory()
+
+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")
~
-test:is(table.concat(a), table.concat(b), "box.info:memory")
-os.exit(0)
+os.exit(test:check() and 0 or 1)
>
> Feel free to use any variant or provide your own.
>
>> +os.exit(0)
> Please, set exit code appropriately (see [1]).
>
> [1]: https://github.com/tarantool/doc/issues/1004

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

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

[Olga Arkhangelskaia]

09.07.2020 4:08, Alexander Turenko пишет:
>>> There are two options to get rid if extra box.info table:
>>> 1. Create new table in the beginning of the function(eg. box.info.gc).
>>> Every time box.info.memory is called it will generate new table with the fresh
>>> info.
>>> 2. The second way is to ignore box.info argument on the stack and fill
>>> directly box.info.memory table, that was passed as an argument.
>>>
>>> I have implemented the first approach because there is box.info.gc works
>>> the same way and we only need to add one line of code.
>>> However, I do not know why it was done in such a way on the first place.
>>> So if you have pros for the second options, please share with me.
>> I have no idea why it is implemented in such complex way, maybe Sasha
>> does? Why box.info.memory yields an empty "callable" table on each
>> lookup? Why it can't just return a function to be called or a table with
>> memory metrics as a result of the lookup? Unfortunately the latter
>> approach breaks the backward compatibility but the first one can save
>> some time on short-term objects creation (I guess no one checks
>> box.info.memory type). Thoughts? Please also consider the comments I
>> left for the patch itself.
> I don't see a reason. The history of src/box/lua/info.c changes shows
> that this way was initially implemented for box.info.phia() (which was
> renamed later to box.info.vinyl()). Then box.info.memory(),
> box.info.gc() and box.info.sql() were added in the same way.
> box.info.phia() was moved from box.phia().
>
> I agree with you. We should define a case to estimate impact of
> replacing a table + metamethod with a function. Not even to make a
> decision whether it worth to change, but to imagine the situation at
> whole.
>
> I would consider metrics collection case using tarantool/metrics every
> minute when default metrics are enabled. I guess it'll call
> box.info.vinyl(), box.info.memory() and box.info.gc() once for each
> metrics collection. So the proposed change will safe 3 extra short-term
> object creations per minute.
>
> I don't see a case when those functions should be called more often and
> become a part of hot path. So I would say that reducing of GC object
> allocations here does not look worthful for me considering possible
> impact of subtle differences (like serialization of `box.info` or other
> differences we can miss) that may fail some scripts or tools.
>
>>> [1] https://www.lua.org/manual/5.1/manual.html#2.8
>>>
>>> @Changelog:
>>> To retrieve information about memory usage box.info:memory() can be used.
> If you are a user, which read release notes and doesn't aware of the
> problem (and don't remember whether box.info.memory() or
> box.info:memory() is suggested by the documentation), then it is hard to
> understand what was changed. I would explicitly mention
> `box.info.memory()` variant of the call: this way the idea of the change
> would be more clear.

@Changelog:
box.info:memory() gives the same result as box.info.memory()(gh-4688).

>
> BTW, don't forget to include an issue number (see examples in existing
> release notes on GitHub).

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

[Alexander Turenko]

The code and the test look okay.

The explanation of changes may be misleading. There are no formal rules
here, but I made attempt to comment it from correctness point of view.

WBR, Alexander Turenko.

On Thu, Jul 09, 2020 at 04:57:50PM +0300, Olga Arkhangelskaia wrote:
> Hi Alexandr!
> 
> Thanks for the review! I have fixed all nits and rewrite the commit message.
> 
> Fix the return value of box.info:memory(). It used to return the same table
> as the box.info(). | box.info.memory() = box.info[“memory”]() |
> box.info.memory() -> getmetatable(box.info.memory).__call(box.info.memory) |
> box.info:memory() = box.info[“memory”](box.info) | box.info:memory() ->
> getmetatable(box.info...).__call(box.info.., box.info) See
> https://www.lua.org/manual/5.1/manual.html#2.8.An extra argument on Lua
> stack in the second case is box.info table with __serialize metamethod. When
> all internal functions are over, memory table is pushed on the stack. When
> console prints the output (converts lua table to yaml format and prints)
> __serialize metamethod of box.info is called. __serilize metamethod of
> box.info shows only some predefined fields and completely ignores memory
> table. To avoid this call, patch pushes an extra empty table on Lua stack.
> When console calls ymal.serializer to print lua table, box.info will be
> ignored. Closes #4688 <https://github.com/tarantool/tarantool/issues/4688>

__serilize, ymal. Unicode quotes. Over 72 symbol lines.

The link to the lua manual is written twice: in-place and in markdown
reference link style.

Please, ensure that your client sends correct text/plain layer. See the
cited text above and the entry in tarantool-patches archive [1].

 | An extra argument on Lua stack in the second case is box.info table
 | with __serialize metamethod.

It is correct.

 | When all internal functions are over, memory table is
 | pushed on the stack.

Before the patch box.info() table is changed in the
lbox_info_memory_call() function. Or it is about the state after the
patch? Not clear for me.

 | When console prints the output (converts lua table to yaml format and
 | prints) __serialize metamethod of box.info is called.

It is correct, but it is not about the solved problem. I guess it
appears here due to comment in [2] re __serialize and my suggestions
about clarifying why a behaviour observed in the tarantool console will
look different from a correct description in a commit message ('lua',
'data' and other box.info.memory() fields are not seen in the serialized
box.info:memory() value before the patch). I cited below the suggestions
(in Russian).

 | Alexander Turenko: Без фикса:
 |
 | Индекс -3 указывает на таблицу box.info. В нее докидываются поля
 | (видно в консоли, если убрать метатаблицу с __serialize). Но
 | __serialize эти докинутые поля не показывает, там только конкретные
 | выводятся (см. реализацию).
 |
 | <...>
 |
 | Alexander Turenko: Я бы попробовал описать и посмотрел, как читается
 | для человека вне контекста.
 |
 | Alexander Turenko: Что-то типа «результат в консоли может быть
 | misleading, потому что выводится результат вызова __serialize
 | metamethod, а не raw table».
 |
 | Alexander Turenko: А то действительно в описании будет одно, а если
 | пойти проверять, то можно увидеть другое.

When side notes are mixed into a general problem description, it is hard
to understand the idea.

 | __serilize metamethod of box.info shows only some predefined fields and
 | completely ignores memory table.

Hmm. It is correct, but may be misleading: how (in theory) the
metamethod would consider box.info.memory() table that is created in
some other scope? Or, maybe, you say about 'lua', 'data' and other
box.info.memory() fields (not the 'memory table')? If so, it has more
sense, but anyway: even if they would not be ignored in __serialize, our
expectation is that box.info:memory() will give {lua = <...>, data =
<...>, ...}, not the whole box.info() with 'lua', 'data', ... extra
fields. So it also is not applicable as description of the key problem.

 | To avoid this call, patch pushes an extra empty table on Lua stack.

Not correct. The new table is necessary not due to __serialize
behaviour, but because we want to return {lua = <...>, data = <...>,
...} table, not add those fields into box.info() table and return it.

 | When console calls ymal.serializer to print lua table, box.info will
 | be ignored.

It is correct, but may be misleading. We return another table and so the
serializer will encode it instead of box.info(). So, yep, box.info()
will be ignored, but how it may not be ignored by a console serializer
if we don't return it? And, again, __serialize behaviour is not the key
problem.

[1]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-July/018384.html
[2]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-July/018372.html

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

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

-- 
Best regards,
IM

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

[Igor Munkin]

Sasha,

Thanks for your explanation!

On 09.07.20, Alexander Turenko wrote:

<snipped>

> > 
> > I have no idea why it is implemented in such complex way, maybe Sasha
> > does? Why box.info.memory yields an empty "callable" table on each
> > lookup? Why it can't just return a function to be called or a table with
> > memory metrics as a result of the lookup? Unfortunately the latter
> > approach breaks the backward compatibility but the first one can save
> > some time on short-term objects creation (I guess no one checks
> > box.info.memory type). Thoughts? Please also consider the comments I
> > left for the patch itself.
> 
> I don't see a reason. The history of src/box/lua/info.c changes shows
> that this way was initially implemented for box.info.phia() (which was
> renamed later to box.info.vinyl()). Then box.info.memory(),
> box.info.gc() and box.info.sql() were added in the same way.
> box.info.phia() was moved from box.phia().
> 
> I agree with you. We should define a case to estimate impact of
> replacing a table + metamethod with a function. Not even to make a
> decision whether it worth to change, but to imagine the situation at
> whole.
> 
> I would consider metrics collection case using tarantool/metrics every
> minute when default metrics are enabled. I guess it'll call
> box.info.vinyl(), box.info.memory() and box.info.gc() once for each
> metrics collection. So the proposed change will safe 3 extra short-term
> object creations per minute.
> 
> I don't see a case when those functions should be called more often and
> become a part of hot path. So I would say that reducing of GC object
> allocations here does not look worthful for me considering possible
> impact of subtle differences (like serialization of `box.info` or other
> differences we can miss) that may fail some scripts or tools.

Yes, it's definitely not a part of the hot path, *but* still implicitly
affects the platform performance a little. Maybe we need to file an
issue for such investigation? It would be nice to look on this part
under the particular workload.

> 

<snipped>


-- 
Best regards,
IM

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

[Alexander Turenko]

> > > I have no idea why it is implemented in such complex way, maybe Sasha
> > > does? Why box.info.memory yields an empty "callable" table on each
> > > lookup? Why it can't just return a function to be called or a table with
> > > memory metrics as a result of the lookup? Unfortunately the latter
> > > approach breaks the backward compatibility but the first one can save
> > > some time on short-term objects creation (I guess no one checks
> > > box.info.memory type). Thoughts? Please also consider the comments I
> > > left for the patch itself.
> > 
> > I don't see a reason. The history of src/box/lua/info.c changes shows
> > that this way was initially implemented for box.info.phia() (which was
> > renamed later to box.info.vinyl()). Then box.info.memory(),
> > box.info.gc() and box.info.sql() were added in the same way.
> > box.info.phia() was moved from box.phia().
> > 
> > I agree with you. We should define a case to estimate impact of
> > replacing a table + metamethod with a function. Not even to make a
> > decision whether it worth to change, but to imagine the situation at
> > whole.
> > 
> > I would consider metrics collection case using tarantool/metrics every
> > minute when default metrics are enabled. I guess it'll call
> > box.info.vinyl(), box.info.memory() and box.info.gc() once for each
> > metrics collection. So the proposed change will safe 3 extra short-term
> > object creations per minute.
> > 
> > I don't see a case when those functions should be called more often and
> > become a part of hot path. So I would say that reducing of GC object
> > allocations here does not look worthful for me considering possible
> > impact of subtle differences (like serialization of `box.info` or other
> > differences we can miss) that may fail some scripts or tools.
> 
> Yes, it's definitely not a part of the hot path, *but* still implicitly
> affects the platform performance a little. Maybe we need to file an
> issue for such investigation? It would be nice to look on this part
> under the particular workload.

I would take it with a little priority, but of course I don't have
objections against filing an issue if you see an area for improvement.

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

[Olga Arkhangelskaia]

[-- 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 --]

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

[Alexander Turenko]

LGTM.

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

[Igor Munkin]

Olya,

Thanks for the fixes! I checked the upstream. The patch LGTM except the
tiny nit.


Sasha, please proceed with the patch.

On 16.07.20, Olga Arkhangelskaia wrote:

<snipped>

> When box.info:memory is called, it has box.info table as a first

Typo: s/a first/the 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.

<snipped>

-- 
Best regards,
IM

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

[Kirill Yukhin]

Hello,

On 29 июн 15:11, Olga Arkhangelskaia wrote:
> The patch fixes the output of box.info:memory(). It used to return the same
> table as the box.info().
> 
> box.info.memory() can be written as box.info[“memory”](), that later has the only
> one argument on the stack - box.info.memory table we need to fill:
> box.info.memory() -> getmetatable(box.info.memory).__call(box.info.memory)[1]
> box.info:memory() call is the same as box.info[“memory”](box.info). So, it
> results in extra argument on the stack[1].
> box.info:mem()->getmetatable(box.info.memory).__call(box.info.memory, box.info).
> When function tries to fill box.info table - __call metamethod of box.info is
> trigged - resulting in box.info table returned as the result.
> 
> There are two options to get rid if extra box.info table:
> 1. Create new table in the beginning of the function(eg. box.info.gc).
> Every time box.info.memory is called it will generate new table with the fresh
> info.
> 2. The second way is to ignore box.info argument on the stack and fill
> directly box.info.memory table, that was passed as an argument.
> 
> I have implemented the first approach because there is box.info.gc works
> the same way and we only need to add one line of code.
> However, I do not know why it was done in such a way on the first place.
> So if you have pros for the second options, please share with me.
> 
> [1] https://www.lua.org/manual/5.1/manual.html#2.8
> 
> @Changelog:
> To retrieve information about memory usage box.info:memory() can be used.
> Olga Arkhangelskaia (1):
>   box: fixed box.info:memory()

I've checked your patch into master.

--
Regards, Kirill Yukhin

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

[Olga Arkhangelskaia]

17.07.2020 0:04, Igor Munkin пишет:
> Olya,
>
> Thanks for the fixes! I checked the upstream. The patch LGTM except the
> tiny nit.
>
>
> Sasha, please proceed with the patch.
>
> On 16.07.20, Olga Arkhangelskaia wrote:
>
> <snipped>
>
>> When box.info:memory is called, it has box.info table as a first
> Typo: s/a first/the first/.
Fixed
>
>> 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.
> <snipped>
>