From: Alex Khatskevich <avkhatskevich@tarantool.org> To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>, tarantool-patches@freelists.org Subject: [tarantool-patches] Re: [PATCH 2/3] Move lua gc to a dedicated module Date: Fri, 3 Aug 2018 23:04:34 +0300 [thread overview] Message-ID: <9b838580-c1f0-7065-310b-2f51adb9c7f4@tarantool.org> (raw) In-Reply-To: <6127e37a-f35b-0ec9-4592-463f0fe61fc5@tarantool.org> 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 <avkhatskevich@tarantool.org> 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
next prev parent reply other threads:[~2018-08-03 20:04 UTC|newest] Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-07-31 16:25 [tarantool-patches] [PATCH 0/3] multiple routers AKhatskevich 2018-07-31 16:25 ` [tarantool-patches] [PATCH 1/3] Update only vshard part of a cfg on reload AKhatskevich 2018-08-01 18:43 ` [tarantool-patches] " Vladislav Shpilevoy 2018-08-03 20:03 ` Alex Khatskevich 2018-08-06 17:03 ` Vladislav Shpilevoy 2018-08-07 13:19 ` Alex Khatskevich 2018-08-08 11:17 ` Vladislav Shpilevoy 2018-07-31 16:25 ` [tarantool-patches] [PATCH 2/3] Move lua gc to a dedicated module AKhatskevich 2018-08-01 18:43 ` [tarantool-patches] " Vladislav Shpilevoy 2018-08-03 20:04 ` Alex Khatskevich [this message] 2018-08-06 17:03 ` Vladislav Shpilevoy 2018-08-08 11:17 ` Vladislav Shpilevoy 2018-07-31 16:25 ` [tarantool-patches] [PATCH 3/3] Introduce multiple routers feature AKhatskevich 2018-08-01 18:43 ` [tarantool-patches] " Vladislav Shpilevoy 2018-08-03 20:05 ` Alex Khatskevich 2018-08-06 17:03 ` Vladislav Shpilevoy 2018-08-07 13:18 ` Alex Khatskevich 2018-08-08 12:28 ` Vladislav Shpilevoy 2018-08-08 14:04 ` Alex Khatskevich 2018-08-08 15:37 ` Vladislav Shpilevoy 2018-08-01 14:30 ` [tarantool-patches] [PATCH] Check self arg passed for router objects AKhatskevich 2018-08-03 20:07 ` [tarantool-patches] [PATCH] Refactor config templates AKhatskevich 2018-08-06 15:49 ` [tarantool-patches] " Vladislav Shpilevoy
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=9b838580-c1f0-7065-310b-2f51adb9c7f4@tarantool.org \ --to=avkhatskevich@tarantool.org \ --cc=tarantool-patches@freelists.org \ --cc=v.shpilevoy@tarantool.org \ --subject='[tarantool-patches] Re: [PATCH 2/3] Move lua gc to a dedicated module' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox