Tarantool development patches archive
 help / color / mirror / Atom feed
From: Alexander Turenko <alexander.turenko@tarantool.org>
To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH 2.X 5/7] module api: luaT_temp_luastate & luaT_release_temp_luastate
Date: Wed, 30 Sep 2020 13:09:28 +0300	[thread overview]
Message-ID: <20200930100928.ybprk7zniggipe45@tkn_work_nb> (raw)
In-Reply-To: <b6344ba7-f5fd-8913-3833-17ede31b2752@tarantool.org>

On Wed, Sep 30, 2020 at 01:23:17AM +0200, Vladislav Shpilevoy wrote:
> On 29.09.2020 23:03, Alexander Turenko wrote:
> > On Tue, Sep 29, 2020 at 06:10:02PM +0300, Igor Munkin wrote:
> >> Vlad,
> >>
> >> I guess there should be a question to be asked in scope of the original
> >> series: what performance enhancement can we obtain with such hacky
> >> optimization? Do we have any benchmarks? If the performance impact is
> >> negligible I believe these interfaces should be left internal.
> > 
> > Why hacky? I'm a module developer and I ask Tarantool: Hey, I know you
> > have a cache of Lua states. Give me one? Can I yield while the state is
> > in use? I can, nice. Can I share it between fibers? No, okay.
> 
> It is not really a cache of lua states. Maybe of a single lua state, and
> working only in Lua fibers (in C fibers you will create/delete the state
> constantly), but not of states.

I was about to say that background fibers also can reuse a Lua state
after ec9b544a123f3e0614ab94b83a7ac4b395326f34 ('lua: expose temporary
Lua state for iproto calls'), but it is not so in 1.10. And you're about
C fibers, so, yep, I agree: it is not as simple as the cache pattern.

The near goal of exposing those functions would be offer an
easy way to save some allocations in the external merger module (and
maybe others in future). Since the module is mainly needed for 1.10 and
the functions will not save allocations on 1.10 -- so the practial
reason is negligible.

The performance difference also looks small (I shared benchmarking results
in this mailing thread). Since there are questions about design of those
functions, I would use lua_newthread() (or wrapper around it to handle
OOM -- written in the module). We can return back to this API later.

> 
> A proper Lua state cache/pool can be implemented easily without fiber at
> all. Just have a pool with these states, where you either take a free one,
> or allocate a new when the pool is empty. And when you don't need a state,
> put it back into the pool. Then you will have at most the same number of
> states in this pool, as the number of fibers, using this pool. Does not
> seem to be a complex thing, will fit in 50-70 lines on C I think, top.
> Neither seem to be too heavy (I didn't check), since all these states will
> have empty stack, cleared before putting them back to the pool.
> 
> > That's all. Quite simple.
> 
> It is not that simple. Whatever you export now, we will need to support
> potentially forever. So exporting not vital things increases support costs
> for us for a very long time by saving a few time to you and Timur so you
> don't need to change merger code and can move it as is. I would better
> one time made merger less depending on Tarantool internals, then do long
> support of the new pile of internal methods exposed in a hurry.

I agree, but also I consider this activity as good opportunity to
improve the module API. Of course, when we have good design.

  reply	other threads:[~2020-09-30 10:09 UTC|newest]

Thread overview: 64+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-24 17:00 [Tarantool-patches] [PATCH 2.X 0/7] RFC: module api: extend for external merger Lua module Timur Safin
2020-09-24 17:00 ` [Tarantool-patches] [PATCH 2.X 1/7] module api: export box_tuple_validate Timur Safin
2020-09-28 22:20   ` Vladislav Shpilevoy
2020-10-02 12:24     ` Timur Safin
2020-10-09  1:11     ` Alexander Turenko
2020-10-09 20:11       ` Vladislav Shpilevoy
2020-10-10  1:19         ` Alexander Turenko
2020-09-29  5:25   ` Alexander Turenko
2020-10-05  7:35     ` Alexander Turenko
2020-09-24 17:00 ` [Tarantool-patches] [PATCH 2.X 2/7] module api: export box_key_def_dup Timur Safin
2020-09-28 22:21   ` Vladislav Shpilevoy
2020-09-29  5:03     ` Alexander Turenko
2020-09-29 23:19       ` Vladislav Shpilevoy
2020-10-01  3:05         ` Alexander Turenko
2020-10-02 12:25           ` Timur Safin
2020-10-02 12:26     ` Timur Safin
2020-09-24 17:00 ` [Tarantool-patches] [PATCH 2.X 3/7] module api: luaT_newthread Timur Safin
2020-09-28 22:21   ` Vladislav Shpilevoy
2020-10-02 12:27     ` Timur Safin
2020-10-02 21:48       ` Vladislav Shpilevoy
2020-09-29  6:25   ` Alexander Turenko
2020-10-02 12:26     ` Timur Safin
2020-10-02 12:53       ` Alexander Turenko
2020-09-29 15:19   ` Igor Munkin
2020-10-02 16:12     ` Timur Safin
2020-10-03 16:57       ` Igor Munkin
2020-09-24 17:00 ` [Tarantool-patches] [PATCH 2.X 4/7] module api: luaL_register_module & luaL_register_type Timur Safin
2020-09-28 22:21   ` Vladislav Shpilevoy
2020-09-29  5:09     ` Alexander Turenko
2020-09-29 23:20       ` Vladislav Shpilevoy
2020-09-30  6:31         ` Alexander Turenko
2020-09-30  6:33           ` Alexander Turenko
2020-10-02 16:14       ` Timur Safin
2020-09-29  8:03     ` Igor Munkin
2020-09-29 23:21       ` Vladislav Shpilevoy
2020-10-02 16:14       ` Timur Safin
2020-10-03  3:24         ` Alexander Turenko
2020-09-24 17:00 ` [Tarantool-patches] [PATCH 2.X 5/7] module api: luaT_temp_luastate & luaT_release_temp_luastate Timur Safin
2020-09-28 22:21   ` Vladislav Shpilevoy
2020-09-29  5:17     ` Alexander Turenko
2020-09-29 23:21       ` Vladislav Shpilevoy
2020-10-01  3:35         ` Alexander Turenko
2020-09-29 15:10     ` Igor Munkin
2020-09-29 21:03       ` Alexander Turenko
2020-09-29 23:23         ` Vladislav Shpilevoy
2020-09-30 10:09           ` Alexander Turenko [this message]
2020-10-01 15:06             ` Igor Munkin
2020-10-03  2:16         ` Alexander Turenko
2020-10-02 12:24       ` Timur Safin
2020-09-24 17:00 ` [Tarantool-patches] [PATCH 2.X 6/7] module api: luaL_checkibuf & luaL_checkconstchar Timur Safin
2020-09-28 22:21   ` Vladislav Shpilevoy
2020-09-29  5:53     ` Alexander Turenko
2020-09-29 23:25       ` Vladislav Shpilevoy
2020-10-01  3:00         ` Alexander Turenko
2020-10-02 16:14           ` Timur Safin
2020-10-02 21:48             ` Vladislav Shpilevoy
2020-10-08 13:50           ` Timur Safin
2020-09-24 17:00 ` [Tarantool-patches] [PATCH 2.X 7/7] module api: luaL_cdata_iscallable Timur Safin
2020-09-28 22:21   ` Vladislav Shpilevoy
2020-09-29  5:19     ` Alexander Turenko
2020-10-02 16:14     ` Timur Safin
2020-10-02 12:23 ` [Tarantool-patches] [PATCH 2.X 0/7] RFC: module api: extend for external merger Lua module Timur Safin
2020-10-02 21:49   ` Vladislav Shpilevoy
2020-10-03  2:54   ` Alexander Turenko

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200930100928.ybprk7zniggipe45@tkn_work_nb \
    --to=alexander.turenko@tarantool.org \
    --cc=tarantool-patches@dev.tarantool.org \
    --cc=v.shpilevoy@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH 2.X 5/7] module api: luaT_temp_luastate & luaT_release_temp_luastate' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox