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

[Olga Arkhangelskaia <arkholga@tarantool.org>]

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