Re: [Tarantool-patches] [PATCH 4/4] fiber: allocate on_stop triggers using mempool

[Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Thanks for the patch!

See 4 comments below.

On 11.08.2021 22:33, Oleg Babin via Tarantool-patches wrote:
> From: Oleg Babin <babinoleg@mail.ru>
> 
> Previously we use malloc for such purpose however we created
> on_stop triggers only if fiber storage was touched. Currently we
> create on_stop trigger for each lua fiber. So it seems reasonable
> to use mempool for such frequent allocations/deallocations.

1. Does it give any perf win? This commit specifically. Seems like an
overkill. A trigger allocation might happen only once per fiber's
lifetime, and the fiber's creation is supposed to be much longer than
the trigger's malloc. The commit probably gives a tiny fraction of
percent improvement or does not change anything. Do you have results?

> diff --git a/src/lua/fiber.c b/src/lua/fiber.c
> index 268ddf9cc..f7c0ca63f 100644
> --- a/src/lua/fiber.c
> +++ b/src/lua/fiber.c
> @@ -36,10 +36,19 @@
>  #include "backtrace.h"
>  #include "tt_static.h"
>  
> +#include <small/mempool.h>

2. Please, use "" for non-system headers.

>  #include <lua.h>
>  #include <lauxlib.h>
>  #include <lualib.h>
>  
> @@ -122,12 +131,13 @@ lbox_pushfiber(struct lua_State *L, struct fiber *f)
>  {
>  	int ref = f->storage.lua.ref;
>  	if (ref <= 0) {
> -		struct trigger *t = (struct trigger *)malloc(sizeof(*t));
> +		struct trigger *t = mempool_alloc(&on_stop_triggers_pool);;
>  		if (t == NULL) {
>  			diag_set(OutOfMemory, sizeof(*t), "malloc", "t");
>  			luaT_error(L);
>  		}
> -		trigger_create(t, lbox_fiber_on_stop, NULL, (trigger_f0) free);
> +		trigger_create(t, lbox_fiber_on_stop, NULL,
> +				 on_stop_trigger_free);

3. Misalignment.

>  		trigger_add(&f->on_stop, t);
>  
>  		uint64_t fid = f->fid;
> @@ -921,6 +931,9 @@ static const struct luaL_Reg fiberlib[] = {
>  void
>  tarantool_lua_fiber_init(struct lua_State *L)
>  {
> +	mempool_create(&on_stop_triggers_pool, &cord()->slabc,
> +				   sizeof(struct trigger));

4. Ditto.

> +
>  	luaL_register_module(L, fiberlib_name, fiberlib);
>  	lua_pop(L, 1);
>  	luaL_register_type(L, fiberlib_name, lbox_fiber_meta);
>