From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 2D9222877D for ; Wed, 1 Aug 2018 14:43:42 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id ZtSqE1QdYx1C for ; Wed, 1 Aug 2018 14:43:42 -0400 (EDT) Received: from smtp3.mail.ru (smtp3.mail.ru [94.100.179.58]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id CFDCF28742 for ; Wed, 1 Aug 2018 14:43:41 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH 2/3] Move lua gc to a dedicated module References: From: Vladislav Shpilevoy Message-ID: <6127e37a-f35b-0ec9-4592-463f0fe61fc5@tarantool.org> Date: Wed, 1 Aug 2018 21:43:39 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org, AKhatskevich 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.