Re: [Tarantool-patches] [PATCH] lua/utils: improve luaT_newthread performance

[Sergey Ostanevich <sergos@tarantool.org>]

Hi!

On 22 Jul 18:12, Igor Munkin wrote:
> Sergos,
> 
> Thanks for your review!
> 
> On 22.07.20, Sergey Ostanevich wrote:
> > Hi!
> > 
> > Thanks for the patch, LGTM.
> 
> Added your tag:
> | Reviewed-by: Sergey Ostanevich <sergos@tarantool.org>
> 
> > 
> > I wonder if it can be done in a more safe way in luaT_newthread() so
> > that if the refernece is not initialized then initialize it, rather 
> > assert in debug and - I suppose - fail with creation in release? I see
> > an overhead for condition here and making even bigger indirect 
> > machinery for the wrappers itself won't bring a lot either.
> 
> Frankly speaking, it looks an overkill to me. What are your exact
> concerns? I added those asserts mostly for self-check. This is a hot
> path in call/eval request handling, fiber creation and stored procedures
> calls, so Debug mode testing fails if the problem occurs. This approach
> is the similar to <box_process_lua> one[1] but there are no such checks.
> This static reference can't be unintentionally changed outside this
> translation unit, so its usage is well-localized. My arguments are not
> about performance but sanity. Do you see any flaws in this design?
> 

The change is not the issue, the missing initialization is. Since
luaT_newthread() is external - there _could_ be some circumstances that
it will be called w/o init done. I believe the lua_rawgeti() should
fail, so my question if it will report a comprehensible error? If it
will - it should be enough, so I enforce my LGTM once more.

Regards,
Sergos