[tarantool-patches] Re: [PATCH 2/3] Move lua gc to a dedicated module

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Thanks for the patch! See 4 comments below.

On 31/07/2018 19:25, AKhatskevich wrote:
> `vshard.lua_gc.lua` is a new module which helps make gc work more
> intense.
> Before the commit that was a duty of router and storage.
> 
> Reasons to move lua gc to a separate module:
> 1. It is not a duty of vshard to collect garbage, so let gc fiber
>     be as far from vshard as possible.
> 2. Next commits will introduce multiple routers feature, which require
>     gc fiber to be a singleton.
> 
> Closes #138
> ---
>   test/router/garbage_collector.result    | 27 +++++++++++------
>   test/router/garbage_collector.test.lua  | 18 ++++++-----
>   test/storage/garbage_collector.result   | 27 +++++++++--------
>   test/storage/garbage_collector.test.lua | 22 ++++++--------
>   vshard/lua_gc.lua                       | 54 +++++++++++++++++++++++++++++++++
>   vshard/router/init.lua                  | 19 +++---------
>   vshard/storage/init.lua                 | 20 ++++--------
>   7 files changed, 116 insertions(+), 71 deletions(-)
>   create mode 100644 vshard/lua_gc.lua
> 
> diff --git a/test/router/garbage_collector.result b/test/router/garbage_collector.result
> index 3c2a4f1..a7474fc 100644
> --- a/test/router/garbage_collector.result
> +++ b/test/router/garbage_collector.result
> @@ -40,27 +40,30 @@ test_run:switch('router_1')
>   fiber = require('fiber')
>   ---
>   ...
> -cfg.collect_lua_garbage = true
> +lua_gc = require('vshard.lua_gc')
>   ---
>   ...
> -iters = vshard.consts.COLLECT_LUA_GARBAGE_INTERVAL / vshard.consts.DISCOVERY_INTERVAL
> +cfg.collect_lua_garbage = true

1. Now this code tests nothing but just fibers. Below you do wakeup
and check that iteration counter is increased, but it is obvious
thing. Before your patch the test really tested that GC is called
by checking for nullified weak references. Now I can remove collectgarbage()
from the main_loop and nothing would changed. Please, make this test
be a test.

Moreover, the test hangs forever both locally and on Travis.

> diff --git a/test/storage/garbage_collector.result b/test/storage/garbage_collector.result
> index 3588fb4..d94ba24 100644
> --- a/test/storage/garbage_collector.result
> +++ b/test/storage/garbage_collector.result

2. Same. Now the test passes even if I removed collectgarbage() from
the main loop.

> diff --git a/vshard/lua_gc.lua b/vshard/lua_gc.lua
> new file mode 100644
> index 0000000..8d6af3e
> --- /dev/null
> +++ b/vshard/lua_gc.lua
> @@ -0,0 +1,54 @@
> +--
> +-- This module implements background lua GC fiber.
> +-- It's purpose is to make GC more aggressive.
> +--
> +
> +local lfiber = require('fiber')
> +local MODULE_INTERNALS = '__module_vshard_lua_gc'
> +
> +local M = rawget(_G, MODULE_INTERNALS)
> +if not M then
> +    M = {
> +        -- Background fiber.
> +        bg_fiber = nil,
> +        -- GC interval in seconds.
> +        interval = nil,
> +        -- Main loop.
> +        -- Stored here to make the fiber reloadable.
> +        main_loop = nil,
> +        -- Number of `collectgarbage()` calls.
> +        iterations = 0,
> +    }
> +end
> +local DEFALUT_INTERVAL = 100

3. For constants please use vshard.consts.

4. You should not choose interval inside the main_loop.
Please, use 'default' option in cfg.lua.