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 32ED628179 for ; Mon, 6 Aug 2018 13:03:29 -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 TAuEbgPRUv2b for ; Mon, 6 Aug 2018 13:03:29 -0400 (EDT) Received: from smtp37.i.mail.ru (smtp37.i.mail.ru [94.100.177.97]) (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 BAF6D28176 for ; Mon, 6 Aug 2018 13:03:28 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH 2/3] Move lua gc to a dedicated module References: <6127e37a-f35b-0ec9-4592-463f0fe61fc5@tarantool.org> <9b838580-c1f0-7065-310b-2f51adb9c7f4@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Mon, 6 Aug 2018 20:03:24 +0300 MIME-Version: 1.0 In-Reply-To: <9b838580-c1f0-7065-310b-2f51adb9c7f4@tarantool.org> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Language: en-US Content-Transfer-Encoding: 8bit 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, Alex Khatskevich Thanks for the patch! It is LGTM but can not push since it depends on the previous one (cherry-pick shows conflicts). On 03/08/2018 23:04, Alex Khatskevich wrote: > > On 01.08.2018 21:43, Vladislav Shpilevoy wrote: >> 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. > GC test returned back. >> >> Moreover, the test hangs forever both locally and on Travis. > Fixed >> >>> 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. > returned. >> >>> 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. > DEFAULT_INTERVAL is removed at all. > Interval value is became required. > > > > full diff > > > > commit ec221bd060f46e4dc009eaab1c6c1bd1cf5a4150 > Author: AKhatskevich > Date:   Thu Jul 26 01:17:00 2018 +0300 > >     Move lua gc to a dedicated module > >     `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 > > diff --git a/test/router/garbage_collector.result b/test/router/garbage_collector.result > index 3c2a4f1..7780046 100644 > --- a/test/router/garbage_collector.result > +++ b/test/router/garbage_collector.result > @@ -40,41 +40,59 @@ 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 >  --- >  ... >  vshard.router.cfg(cfg) >  --- >  ... > +lua_gc.internal.bg_fiber ~= nil > +--- > +- true > +... > +-- Check that `collectgarbage()` was really called. >  a = setmetatable({}, {__mode = 'v'}) >  --- >  ... >  a.k = {b = 100} >  --- >  ... > -for i = 1, iters + 1 do vshard.router.discovery_wakeup() fiber.sleep(0.01) end > +iterations = lua_gc.internal.iterations > +--- > +... > +lua_gc.internal.bg_fiber:wakeup() > +--- > +... > +while lua_gc.internal.iterations < iterations + 1 do fiber.sleep(0.01) end >  --- >  ... >  a.k >  --- >  - null >  ... > +lua_gc.internal.interval = 0.001 > +--- > +... >  cfg.collect_lua_garbage = false >  --- >  ... >  vshard.router.cfg(cfg) >  --- >  ... > -a.k = {b = 100} > +lua_gc.internal.bg_fiber == nil > +--- > +- true > +... > +iterations = lua_gc.internal.iterations >  --- >  ... > -for i = 1, iters + 1 do vshard.router.discovery_wakeup() fiber.sleep(0.01) end > +fiber.sleep(0.01) >  --- >  ... > -a.k ~= nil > +iterations == lua_gc.internal.iterations >  --- >  - true >  ... > diff --git a/test/router/garbage_collector.test.lua b/test/router/garbage_collector.test.lua > index b3411cd..e8d0876 100644 > --- a/test/router/garbage_collector.test.lua > +++ b/test/router/garbage_collector.test.lua > @@ -13,18 +13,24 @@ test_run:cmd("start server router_1") >  -- >  test_run:switch('router_1') >  fiber = require('fiber') > +lua_gc = require('vshard.lua_gc') >  cfg.collect_lua_garbage = true > -iters = vshard.consts.COLLECT_LUA_GARBAGE_INTERVAL / vshard.consts.DISCOVERY_INTERVAL >  vshard.router.cfg(cfg) > +lua_gc.internal.bg_fiber ~= nil > +-- Check that `collectgarbage()` was really called. >  a = setmetatable({}, {__mode = 'v'}) >  a.k = {b = 100} > -for i = 1, iters + 1 do vshard.router.discovery_wakeup() fiber.sleep(0.01) end > +iterations = lua_gc.internal.iterations > +lua_gc.internal.bg_fiber:wakeup() > +while lua_gc.internal.iterations < iterations + 1 do fiber.sleep(0.01) end >  a.k > +lua_gc.internal.interval = 0.001 >  cfg.collect_lua_garbage = false >  vshard.router.cfg(cfg) > -a.k = {b = 100} > -for i = 1, iters + 1 do vshard.router.discovery_wakeup() fiber.sleep(0.01) end > -a.k ~= nil > +lua_gc.internal.bg_fiber == nil > +iterations = lua_gc.internal.iterations > +fiber.sleep(0.01) > +iterations == lua_gc.internal.iterations > >  test_run:switch("default") >  test_run:cmd("stop server router_1") > diff --git a/test/storage/garbage_collector.result b/test/storage/garbage_collector.result > index 3588fb4..6bec2db 100644 > --- a/test/storage/garbage_collector.result > +++ b/test/storage/garbage_collector.result > @@ -120,7 +120,7 @@ test_run:switch('storage_1_a') >  fiber = require('fiber') >  --- >  ... > -log = require('log') > +lua_gc = require('vshard.lua_gc') >  --- >  ... >  cfg.collect_lua_garbage = true > @@ -129,38 +129,50 @@ cfg.collect_lua_garbage = true >  vshard.storage.cfg(cfg, names.storage_1_a) >  --- >  ... > --- Create a weak reference to a able {b = 100} - it must be > --- deleted on the next GC. > +lua_gc.internal.bg_fiber ~= nil > +--- > +- true > +... > +-- Check that `collectgarbage()` was really called. >  a = setmetatable({}, {__mode = 'v'}) >  --- >  ... >  a.k = {b = 100} >  --- >  ... > -iters = vshard.consts.COLLECT_LUA_GARBAGE_INTERVAL / vshard.consts.DEFAULT_COLLECT_BUCKET_GARBAGE_INTERVAL > +iterations = lua_gc.internal.iterations >  --- >  ... > --- Wait until Lua GC deletes a.k. > -for i = 1, iters + 1 do vshard.storage.garbage_collector_wakeup() fiber.sleep(0.01) end > +lua_gc.internal.bg_fiber:wakeup() > +--- > +... > +while lua_gc.internal.iterations < iterations + 1 do fiber.sleep(0.01) end >  --- >  ... >  a.k >  --- >  - null >  ... > +lua_gc.internal.interval = 0.001 > +--- > +... >  cfg.collect_lua_garbage = false >  --- >  ... >  vshard.storage.cfg(cfg, names.storage_1_a) >  --- >  ... > -a.k = {b = 100} > +lua_gc.internal.bg_fiber == nil > +--- > +- true > +... > +iterations = lua_gc.internal.iterations >  --- >  ... > -for i = 1, iters + 1 do vshard.storage.garbage_collector_wakeup() fiber.sleep(0.01) end > +fiber.sleep(0.01) >  --- >  ... > -a.k ~= nil > +iterations == lua_gc.internal.iterations >  --- >  - true >  ... > diff --git a/test/storage/garbage_collector.test.lua b/test/storage/garbage_collector.test.lua > index 79e76d8..407b8a1 100644 > --- a/test/storage/garbage_collector.test.lua > +++ b/test/storage/garbage_collector.test.lua > @@ -46,22 +46,24 @@ customer:select{} >  -- >  test_run:switch('storage_1_a') >  fiber = require('fiber') > -log = require('log') > +lua_gc = require('vshard.lua_gc') >  cfg.collect_lua_garbage = true >  vshard.storage.cfg(cfg, names.storage_1_a) > --- Create a weak reference to a able {b = 100} - it must be > --- deleted on the next GC. > +lua_gc.internal.bg_fiber ~= nil > +-- Check that `collectgarbage()` was really called. >  a = setmetatable({}, {__mode = 'v'}) >  a.k = {b = 100} > -iters = vshard.consts.COLLECT_LUA_GARBAGE_INTERVAL / vshard.consts.DEFAULT_COLLECT_BUCKET_GARBAGE_INTERVAL > --- Wait until Lua GC deletes a.k. > -for i = 1, iters + 1 do vshard.storage.garbage_collector_wakeup() fiber.sleep(0.01) end > +iterations = lua_gc.internal.iterations > +lua_gc.internal.bg_fiber:wakeup() > +while lua_gc.internal.iterations < iterations + 1 do fiber.sleep(0.01) end >  a.k > +lua_gc.internal.interval = 0.001 >  cfg.collect_lua_garbage = false >  vshard.storage.cfg(cfg, names.storage_1_a) > -a.k = {b = 100} > -for i = 1, iters + 1 do vshard.storage.garbage_collector_wakeup() fiber.sleep(0.01) end > -a.k ~= nil > +lua_gc.internal.bg_fiber == nil > +iterations = lua_gc.internal.iterations > +fiber.sleep(0.01) > +iterations == lua_gc.internal.iterations > >  test_run:switch('default') >  test_run:drop_cluster(REPLICASET_2) > diff --git a/vshard/lua_gc.lua b/vshard/lua_gc.lua > new file mode 100644 > index 0000000..c6c5cd3 > --- /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 > + > +M.main_loop = function() > +    lfiber.sleep(M.interval) > +    collectgarbage() > +    M.iterations = M.iterations + 1 > +    return M.main_loop() > +end > + > +local function set_state(active, interval) > +    assert(type(interval) == 'number') > +    M.interval = interval > +    if active and not M.bg_fiber then > +        M.bg_fiber = lfiber.create(M.main_loop) > +        M.bg_fiber:name('vshard.lua_gc') > +    end > +    if not active and M.bg_fiber then > +        M.bg_fiber:cancel() > +        M.bg_fiber = nil > +    end > +    if active then > +        M.bg_fiber:wakeup() > +    end > +end > + > +if not rawget(_G, MODULE_INTERNALS) then > +    rawset(_G, MODULE_INTERNALS, M) > +end > + > +return { > +    set_state = set_state, > +    internal = M, > +} > diff --git a/vshard/router/init.lua b/vshard/router/init.lua > index e2b2b22..3e127cb 100644 > --- a/vshard/router/init.lua > +++ b/vshard/router/init.lua > @@ -7,6 +7,7 @@ if rawget(_G, MODULE_INTERNALS) then >      local vshard_modules = { >          'vshard.consts', 'vshard.error', 'vshard.cfg', >          'vshard.hash', 'vshard.replicaset', 'vshard.util', > +        'vshard.lua_gc', >      } >      for _, module in pairs(vshard_modules) do >          package.loaded[module] = nil > @@ -18,6 +19,7 @@ local lcfg = require('vshard.cfg') >  local lhash = require('vshard.hash') >  local lreplicaset = require('vshard.replicaset') >  local util = require('vshard.util') > +local lua_gc = require('vshard.lua_gc') > >  local M = rawget(_G, MODULE_INTERNALS) >  if not M then > @@ -43,8 +45,7 @@ if not M then >          discovery_fiber = nil, >          -- Bucket count stored on all replicasets. >          total_bucket_count = 0, > -        -- If true, then discovery fiber starts to call > -        -- collectgarbage() periodically. > +        -- Boolean lua_gc state (create periodic gc task). >          collect_lua_garbage = nil, >          -- This counter is used to restart background fibers with >          -- new reloaded code. > @@ -151,8 +152,6 @@ end >  -- >  local function discovery_f() >      local module_version = M.module_version > -    local iterations_until_lua_gc = > -        consts.COLLECT_LUA_GARBAGE_INTERVAL / consts.DISCOVERY_INTERVAL >      while module_version == M.module_version do >          while not next(M.replicasets) do >              lfiber.sleep(consts.DISCOVERY_INTERVAL) > @@ -188,12 +187,6 @@ local function discovery_f() >                      M.route_map[bucket_id] = replicaset >                  end >              end > -            iterations_until_lua_gc = iterations_until_lua_gc - 1 > -            if M.collect_lua_garbage and iterations_until_lua_gc == 0 then > -                iterations_until_lua_gc = > -                    consts.COLLECT_LUA_GARBAGE_INTERVAL / consts.DISCOVERY_INTERVAL > -                collectgarbage() > -            end >              lfiber.sleep(consts.DISCOVERY_INTERVAL) >          end >      end > @@ -504,7 +497,6 @@ local function router_cfg(cfg) >      end >      local new_replicasets = lreplicaset.buildall(vshard_cfg) >      local total_bucket_count = vshard_cfg.bucket_count > -    local collect_lua_garbage = vshard_cfg.collect_lua_garbage >      log.info("Calling box.cfg()...") >      for k, v in pairs(box_cfg) do >          log.info({[k] = v}) > @@ -531,7 +523,7 @@ local function router_cfg(cfg) > vshard_cfg.connection_outdate_delay) >      M.connection_outdate_delay = vshard_cfg.connection_outdate_delay >      M.total_bucket_count = total_bucket_count > -    M.collect_lua_garbage = collect_lua_garbage > +    M.collect_lua_garbage = vshard_cfg.collect_lua_garbage >      M.current_cfg = vshard_cfg >      M.replicasets = new_replicasets >      -- Update existing route map in-place. > @@ -548,8 +540,7 @@ local function router_cfg(cfg) >          M.discovery_fiber = util.reloadable_fiber_create( >              'vshard.discovery', M, 'discovery_f') >      end > -    -- Destroy connections, not used in a new configuration. > -    collectgarbage() > +    lua_gc.set_state(M.collect_lua_garbage, consts.COLLECT_LUA_GARBAGE_INTERVAL) >  end > >  -------------------------------------------------------------------------------- > diff --git a/vshard/storage/init.lua b/vshard/storage/init.lua > index 40216ea..3e29e9d 100644 > --- a/vshard/storage/init.lua > +++ b/vshard/storage/init.lua > @@ -10,7 +10,8 @@ if rawget(_G, MODULE_INTERNALS) then >      local vshard_modules = { >          'vshard.consts', 'vshard.error', 'vshard.cfg', >          'vshard.replicaset', 'vshard.util', > -        'vshard.storage.reload_evolution' > +        'vshard.storage.reload_evolution', > +        'vshard.lua_gc', >      } >      for _, module in pairs(vshard_modules) do >          package.loaded[module] = nil > @@ -21,6 +22,7 @@ local lerror = require('vshard.error') >  local lcfg = require('vshard.cfg') >  local lreplicaset = require('vshard.replicaset') >  local util = require('vshard.util') > +local lua_gc = require('vshard.lua_gc') >  local reload_evolution = require('vshard.storage.reload_evolution') > >  local M = rawget(_G, MODULE_INTERNALS) > @@ -75,8 +77,7 @@ if not M then >          collect_bucket_garbage_fiber = nil, >          -- Do buckets garbage collection once per this time. >          collect_bucket_garbage_interval = nil, > -        -- If true, then bucket garbage collection fiber starts to > -        -- call collectgarbage() periodically. > +        -- Boolean lua_gc state (create periodic gc task). >          collect_lua_garbage = nil, > >          -------------------- Bucket recovery --------------------- > @@ -1063,9 +1064,6 @@ function collect_garbage_f() >      -- buckets_for_redirect is deleted, it gets empty_sent_buckets >      -- for next deletion. >      local empty_sent_buckets = {} > -    local iterations_until_lua_gc = > -        consts.COLLECT_LUA_GARBAGE_INTERVAL / M.collect_bucket_garbage_interval > - >      while M.module_version == module_version do >          -- Check if no changes in buckets configuration. >          if control.bucket_generation_collected ~= control.bucket_generation then > @@ -1106,12 +1104,6 @@ function collect_garbage_f() >              end >          end >  ::continue:: > -        iterations_until_lua_gc = iterations_until_lua_gc - 1 > -        if iterations_until_lua_gc == 0 and M.collect_lua_garbage then > -            iterations_until_lua_gc = consts.COLLECT_LUA_GARBAGE_INTERVAL / > - M.collect_bucket_garbage_interval > -            collectgarbage() > -        end >          lfiber.sleep(M.collect_bucket_garbage_interval) >      end >  end > @@ -1586,7 +1578,6 @@ local function storage_cfg(cfg, this_replica_uuid, is_reload) >      local shard_index = vshard_cfg.shard_index >      local collect_bucket_garbage_interval = >          vshard_cfg.collect_bucket_garbage_interval > -    local collect_lua_garbage = vshard_cfg.collect_lua_garbage > >      -- It is considered that all possible errors during cfg >      -- process occur only before this place. > @@ -1641,7 +1632,7 @@ local function storage_cfg(cfg, this_replica_uuid, is_reload) >      M.rebalancer_max_receiving = rebalancer_max_receiving >      M.shard_index = shard_index >      M.collect_bucket_garbage_interval = collect_bucket_garbage_interval > -    M.collect_lua_garbage = collect_lua_garbage > +    M.collect_lua_garbage = vshard_cfg.collect_lua_garbage >      M.current_cfg = vshard_cfg > >      if was_master and not is_master then > @@ -1666,6 +1657,7 @@ local function storage_cfg(cfg, this_replica_uuid, is_reload) >          M.rebalancer_fiber:cancel() >          M.rebalancer_fiber = nil >      end > +    lua_gc.set_state(M.collect_lua_garbage, consts.COLLECT_LUA_GARBAGE_INTERVAL) >      -- Destroy connections, not used in a new configuration. >      collectgarbage() >  end > >