From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (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 EAF0C42EF5C for ; Thu, 2 Jul 2020 00:44:55 +0300 (MSK) Date: Thu, 2 Jul 2020 00:34:41 +0300 From: Igor Munkin Message-ID: <20200701213441.GD5559@tarantool.org> References: <20200629121118.21596-1-arkholga@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20200629121118.21596-1-arkholga@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH 0/1] fix box.info:memory() List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Olga Arkhangelskaia , s@tarantool.org Cc: tarantool-patches@dev.tarantool.org, alexander.turenko@tarantool.org 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