From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp63.i.mail.ru (smtp63.i.mail.ru [217.69.128.43]) (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 70C99445320 for ; Thu, 9 Jul 2020 04:09:14 +0300 (MSK) Date: Thu, 9 Jul 2020 04:08:28 +0300 From: Alexander Turenko Message-ID: <20200709010828.nlfq6sbavluwu6wf@tkn_work_nb> References: <20200629121118.21596-1-arkholga@tarantool.org> <20200701213441.GD5559@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200701213441.GD5559@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: Igor Munkin Cc: tarantool-patches@dev.tarantool.org > > 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).