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

[Oleg Babin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Thanks for your review. See my answers below.

On 25.08.2021 23:34, Vladislav Shpilevoy wrote:
> 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?

It's a good question. I thought thank mempool should be faster than malloc.

We can easily drop this commit. I had some doubts about it.


Benchmark is following:

```

for _ = 1, 10 do
     local start = clock.time()
     for _ = 1, 1e5 do
         fiber.new(function() end)
     end
     fiber.yield()
     print('fiber.new()', clock.time() - start)
end

```

Current result on my machine:

```

fiber.new()    1.7790501117706
fiber.new()    0.23458814620972
fiber.new()    0.2239830493927
fiber.new()    0.25585198402405
fiber.new()    0.26314878463745
fiber.new()    0.17044496536255
fiber.new()    0.23963904380798
fiber.new()    0.2929859161377
fiber.new()    0.16769504547119
fiber.new()    0.26060795783997

```

Previous commit:

```

fiber.new()    1.7500579357147
fiber.new()    0.24660110473633
fiber.new()    0.23380303382874
fiber.new()    0.28128218650818
fiber.new()    0.25019407272339
fiber.new()    0.23627710342407
fiber.new()    0.29107189178467
fiber.new()    0.19264888763428
fiber.new()    0.27617883682251
fiber.new()    0.25651788711548

```


master:

```

fiber.new()    1.6132490634918
fiber.new()    0.23563098907471
fiber.new()    0.23894000053406
fiber.new()    0.25919413566589
fiber.new()    0.29895401000977
fiber.new()    0.24670886993408
fiber.new()    0.26245188713074
fiber.new()    0.21903514862061
fiber.new()    0.19983816146851
fiber.new()    0.23553991317749

```


A bit modified test with fiber.new -> fiber.create

olegrok/fiber-ref HEAD:

```

fiber.create()    0.14023184776306
fiber.create()    0.13392496109009
fiber.create()    0.13862681388855
fiber.create()    0.13287806510925
fiber.create()    0.14014101028442
fiber.create()    0.15336418151855
fiber.create()    0.13403511047363
fiber.create()    0.12675404548645
fiber.create()    0.13705611228943
fiber.create()    0.13896298408508

```

olegrok/fiber-ref HEAD~1:

```

fiber.create()    0.13091707229614
fiber.create()    0.12622594833374
fiber.create()    0.13849806785583
fiber.create()    0.13334107398987
fiber.create()    0.12660312652588
fiber.create()    0.13159799575806
fiber.create()    0.12833094596863
fiber.create()    0.1359851360321
fiber.create()    0.1327109336853
fiber.create()    0.12926387786865

```


master:

```

fiber.create()    0.17535996437073
fiber.create()    0.15730094909668
fiber.create()    0.15560913085938
fiber.create()    0.15467691421509
fiber.create()    0.15472817420959
fiber.create()    0.17342209815979
fiber.create()    0.15398406982422
fiber.create()    0.16387009620667
fiber.create()    0.16766691207886
fiber.create()    0.17136907577515

```



>> 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.


Fixed

>>   #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.

Fixed


>>   		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.


Fixed

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