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

[Alexander Turenko <alexander.turenko@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).