From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp29.i.mail.ru (smtp29.i.mail.ru [94.100.177.89]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 6BF9C445320 for ; Thu, 16 Jul 2020 23:29:58 +0300 (MSK) References: <20200629121118.21596-1-arkholga@tarantool.org> <20200629121118.21596-2-arkholga@tarantool.org> <20200701213447.GE5559@tarantool.org> <16d471e3-d578-7b12-6a4f-08814a2f1b64@tarantool.org> <20200709010804.7kejojfasvu7o7oj@tkn_work_nb> <20200715144023.pll42732ijjw6obo@tkn_work_nb> <20200716175618.GN5559@tarantool.org> From: Olga Arkhangelskaia Message-ID: <1d010980-646d-3c73-bef7-1009fff6609c@tarantool.org> Date: Thu, 16 Jul 2020 23:29:56 +0300 MIME-Version: 1.0 In-Reply-To: <20200716175618.GN5559@tarantool.org> Content-Type: multipart/alternative; boundary="------------070037ACBF91826F41AECEBF" Content-Language: en-GB Subject: Re: [Tarantool-patches] [PATCH 1/1] box: fixed box.info:memory() List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Munkin Cc: tarantool-patches@dev.tarantool.org, Alexander Turenko This is a multi-part message in MIME format. --------------070037ACBF91826F41AECEBF Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit 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 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 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) > --------------070037ACBF91826F41AECEBF Content-Type: text/html; charset="utf-8" Content-Transfer-Encoding: 8bit

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 (see https://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
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)
--------------070037ACBF91826F41AECEBF--