From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <sergos@tarantool.org>
Received: from smtp5.mail.ru (smtp5.mail.ru [94.100.179.24])
 (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 0C996445320
 for <tarantool-patches@dev.tarantool.org>;
 Fri, 24 Jul 2020 19:15:35 +0300 (MSK)
Date: Fri, 24 Jul 2020 19:15:34 +0300
From: Sergey Ostanevich <sergos@tarantool.org>
Message-ID: <20200724161534.GE894@tarantool.org>
References: <ec0eb12f402d5ea2a37b50212caa66b5808375a6.1595238969.git.imun@tarantool.org>
 <20200722113055.GB894@tarantool.org>
 <20200722151209.GP18920@tarantool.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20200722151209.GP18920@tarantool.org>
Subject: Re: [Tarantool-patches] [PATCH] lua/utils: improve luaT_newthread
	performance
List-Id: Tarantool development patches <tarantool-patches.dev.tarantool.org>
List-Unsubscribe: <https://lists.tarantool.org/mailman/options/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=unsubscribe>
List-Archive: <https://lists.tarantool.org/pipermail/tarantool-patches/>
List-Post: <mailto:tarantool-patches@dev.tarantool.org>
List-Help: <mailto:tarantool-patches-request@dev.tarantool.org?subject=help>
List-Subscribe: <https://lists.tarantool.org/mailman/listinfo/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=subscribe>
To: Igor Munkin <imun@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org, Vladislav Shpilevoy <v.shpilevoy@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