<HTML><BODY><div id="composeWebView_editable_content" data-mailruapp-compose-id="composeWebView_editable_content" style="text-align: left;"><div>Hi!</div><div><br></div><div>Thank you for the patch, it LGTM.</div><div><br></div><div id="mail-app-auto-signature">
Best regards,<div>Sergos </div>
</div><br><br>Friday, 2 October 2020, 17:49 +0300 from Igor Munkin <imun@tarantool.org>:<br><div id="composeWebView_previouse_content" data-mailruapp-compose-id="composeWebView_previouse_content"><blockquote id="mail-app-auto-quote" style="border-left-width: 1px; border-left-style: solid; border-left-color: rgb(11, 102, 249); margin: 10px 0px 10px 5px; padding: 0px 0px 0px 10px; display: inherit;" data-darkosha-id="1:3"><div class="js-helper js-readmsg-msg" data-darkosha-id="1:4" style="">
<style type="text/css" data-darkosha-id="1:5" style=""></style>
<div data-darkosha-id="1:6" style="">
<base target="_self" href="https://e.mail.ru/" data-darkosha-id="1:7" style="">
<div id="style_16016501930017495364_BODY" data-darkosha-id="1:8" style="">While running GC hook (i.e. __gc metamethod) garbage collector engine<br data-darkosha-id="1:9" style="">
is "stopped": the memory penalty threshold is set to LJ_MAX_MEM and<br data-darkosha-id="1:10" style="">
incremental GC step is not triggered as a result. Ergo, yielding the<br data-darkosha-id="1:11" style="">
execution at the finalizer body leads to further running platform with<br data-darkosha-id="1:12" style="">
disabled LuaJIT GC. It is not re-enabled until the yielded fiber doesn't<br data-darkosha-id="1:13" style="">
get the execution back.<br data-darkosha-id="1:14" style="">
<br data-darkosha-id="1:15" style="">
This changeset extends <cord_on_yield> routine with the check whether GC<br data-darkosha-id="1:16" style="">
hook is active. If the switch-over occurs in scope of __gc metamethod<br data-darkosha-id="1:17" style="">
the platform is forced to stop its execution with EXIT_FAILURE and calls<br data-darkosha-id="1:18" style="">
panic routine before the exit.<br data-darkosha-id="1:19" style="">
<br data-darkosha-id="1:20" style="">
Relates to #4518<br data-darkosha-id="1:21" style="">
Follows up #4727<br data-darkosha-id="1:22" style="">
<br data-darkosha-id="1:23" style="">
Signed-off-by: Igor Munkin <<a href="mailto:imun@tarantool.org" data-darkosha-id="1:24" style="">imun@tarantool.org</a>><br data-darkosha-id="1:25" style="">
---<br data-darkosha-id="1:26" style="">
<br data-darkosha-id="1:27" style="">
Vlad introduced the internal interface and local internal background<br data-darkosha-id="1:28" style="">
fiber in scope of 8443bd9 ("fiber: introduce schedule_task() internal<br data-darkosha-id="1:29" style="">
function") to postpone any yielding finalization (e.g. 3d5b4da ("fio:<br data-darkosha-id="1:30" style="">
close unused descriptors automatically") and f073834 ("swim: use<br data-darkosha-id="1:31" style="">
fiber._internal.schedule_task() for GC")). After this patch is merged we<br data-darkosha-id="1:32" style="">
need to update docs and provide users a correct scenario to detect and<br data-darkosha-id="1:33" style="">
fix yielding finalizers.<br data-darkosha-id="1:34" style="">
<br data-darkosha-id="1:35" style="">
Here are also the benchmark results for the Release build:<br data-darkosha-id="1:36" style="">
* Vanilla (2711797) -> Patched (61072ba) (min, median, mean, max):<br data-darkosha-id="1:37" style="">
| fibers: 10; iters: 100 0% 0% 0% 2%<br data-darkosha-id="1:38" style="">
| fibers: 10; iters: 1000 -3% 0% 0% 1%<br data-darkosha-id="1:39" style="">
| fibers: 10; iters: 10000 -3% 0% -1% -2%<br data-darkosha-id="1:40" style="">
| fibers: 10; iters: 100000 0% 0% 0% 1%<br data-darkosha-id="1:41" style="">
| fibers: 100; iters: 100 -1% -2% -2% -9%<br data-darkosha-id="1:42" style="">
| fibers: 100; iters: 1000 0% 0% 0% 5%<br data-darkosha-id="1:43" style="">
| fibers: 100; iters: 10000 0% 0% 0% 0%<br data-darkosha-id="1:44" style="">
| fibers: 100; iters: 100000 0% 0% 0% -3%<br data-darkosha-id="1:45" style="">
| fibers: 1000; iters: 100 0% 0% 0% 0%<br data-darkosha-id="1:46" style="">
| fibers: 1000; iters: 1000 0% 0% 0% 0%<br data-darkosha-id="1:47" style="">
| fibers: 1000; iters: 10000 0% 0% 0% 1%<br data-darkosha-id="1:48" style="">
| fibers: 1000; iters: 100000 0% 0% 0% 2%<br data-darkosha-id="1:49" style="">
| fibers: 10000; iters: 100 0% -3% -1% -2%<br data-darkosha-id="1:50" style="">
| fibers: 10000; iters: 1000 0% -2% -3% -2%<br data-darkosha-id="1:51" style="">
| fibers: 10000; iters: 10000 0% 0% 0% -2%<br data-darkosha-id="1:52" style="">
| fibers: 10000; iters: 100000 0% -3% -3% -6%<br data-darkosha-id="1:53" style="">
<br data-darkosha-id="1:54" style="">
src/lua/utils.c | 26 ++++++++++++++-<br data-darkosha-id="1:55" style="">
test/app-tap/yield-in-gc-finalizer.test.lua | 36 +++++++++++++++++++++<br data-darkosha-id="1:56" style="">
2 files changed, 61 insertions(+), 1 deletion(-)<br data-darkosha-id="1:57" style="">
create mode 100755 test/app-tap/yield-in-gc-finalizer.test.lua<br data-darkosha-id="1:58" style="">
<br data-darkosha-id="1:59" style="">
diff --git a/src/lua/utils.c b/src/lua/utils.c<br data-darkosha-id="1:60" style="">
index bb2287162..399bec6c6 100644<br data-darkosha-id="1:61" style="">
--- a/src/lua/utils.c<br data-darkosha-id="1:62" style="">
+++ b/src/lua/utils.c<br data-darkosha-id="1:63" style="">
@@ -1324,7 +1324,8 @@ tarantool_lua_utils_init(struct lua_State *L)<br data-darkosha-id="1:64" style="">
* the running fiber yields the execution.<br data-darkosha-id="1:65" style="">
* Since Tarantool fibers don't switch-over the way Lua coroutines<br data-darkosha-id="1:66" style="">
* do the platform ought to notify JIT engine when one lua_State<br data-darkosha-id="1:67" style="">
- * substitutes another one.<br data-darkosha-id="1:68" style="">
+ * substitutes another one. Furthermore fiber switch is forbidden<br data-darkosha-id="1:69" style="">
+ * when GC hook (i.e. __gc metamethod) is running.<br data-darkosha-id="1:70" style="">
*/<br data-darkosha-id="1:71" style="">
void cord_on_yield(void)<br data-darkosha-id="1:72" style="">
{<br data-darkosha-id="1:73" style="">
@@ -1355,4 +1356,27 @@ void cord_on_yield(void)<br data-darkosha-id="1:74" style="">
* lead to a failure on any next compiler phase.<br data-darkosha-id="1:75" style="">
*/<br data-darkosha-id="1:76" style="">
lj_trace_abort(g);<br data-darkosha-id="1:77" style="">
+<br data-darkosha-id="1:78" style="">
+ /*<br data-darkosha-id="1:79" style="">
+ * XXX: While running GC hook (i.e. __gc metamethod)<br data-darkosha-id="1:80" style="">
+ * garbage collector is formally "stopped" since the<br data-darkosha-id="1:81" style="">
+ * memory penalty threshold is set to its maximum value,<br data-darkosha-id="1:82" style="">
+ * ergo incremental GC step is not triggered. Thereby,<br data-darkosha-id="1:83" style="">
+ * yielding the execution at this point leads to further<br data-darkosha-id="1:84" style="">
+ * running platform with disabled LuaJIT GC. The fiber<br data-darkosha-id="1:85" style="">
+ * doesn't get the execution back until it's ready, so<br data-darkosha-id="1:86" style="">
+ * in pessimistic scenario LuaJIT OOM might occur<br data-darkosha-id="1:87" style="">
+ * earlier. As a result fiber switch is prohibited when<br data-darkosha-id="1:88" style="">
+ * GC hook is active and the platform is forced to stop.<br data-darkosha-id="1:89" style="">
+ */<br data-darkosha-id="1:90" style="">
+ if (unlikely(g->hookmask & (HOOK_ACTIVE|HOOK_GC))) {<br data-darkosha-id="1:91" style="">
+ struct lua_State *L = fiber()->storage.lua.stack;<br data-darkosha-id="1:92" style="">
+ assert(L != NULL);<br data-darkosha-id="1:93" style="">
+ lua_pushfstring(L, "fiber %d is switched while running GC"<br data-darkosha-id="1:94" style="">
+ " finalizer (i.e. __gc metamethod)",<br data-darkosha-id="1:95" style="">
+ fiber()->fid);<br data-darkosha-id="1:96" style="">
+ if (g->panic)<br data-darkosha-id="1:97" style="">
+ g->panic(L);<br data-darkosha-id="1:98" style="">
+ exit(EXIT_FAILURE);<br data-darkosha-id="1:99" style="">
+ }<br data-darkosha-id="1:100" style="">
}<br data-darkosha-id="1:101" style="">
diff --git a/test/app-tap/yield-in-gc-finalizer.test.lua b/test/app-tap/yield-in-gc-finalizer.test.lua<br data-darkosha-id="1:102" style="">
new file mode 100755<br data-darkosha-id="1:103" style="">
index 000000000..a7e173721<br data-darkosha-id="1:104" style="">
--- /dev/null<br data-darkosha-id="1:105" style="">
+++ b/test/app-tap/yield-in-gc-finalizer.test.lua<br data-darkosha-id="1:106" style="">
@@ -0,0 +1,36 @@<br data-darkosha-id="1:107" style="">
+#!/usr/bin/env tarantool<br data-darkosha-id="1:108" style="">
+<br data-darkosha-id="1:109" style="">
+if #arg == 0 then<br data-darkosha-id="1:110" style="">
+ local tap = require('tap')<br data-darkosha-id="1:111" style="">
+ local test = tap.test('test')<br data-darkosha-id="1:112" style="">
+<br data-darkosha-id="1:113" style="">
+ test:plan(1)<br data-darkosha-id="1:114" style="">
+<br data-darkosha-id="1:115" style="">
+ -- XXX: Shell argument <test> is necessary to differ test case<br data-darkosha-id="1:116" style="">
+ -- from the test runner.<br data-darkosha-id="1:117" style="">
+ local cmd = string.gsub('<LUABIN> 2>/dev/null <SCRIPT> test', '%<(%w+)>', {<br data-darkosha-id="1:118" style="">
+ LUABIN = arg[-1],<br data-darkosha-id="1:119" style="">
+ SCRIPT = arg[0],<br data-darkosha-id="1:120" style="">
+ })<br data-darkosha-id="1:121" style="">
+ test:isnt(os.execute(cmd), 0, 'fiber.yield is forbidden in __gc')<br data-darkosha-id="1:122" style="">
+<br data-darkosha-id="1:123" style="">
+ os.exit(test:check() and 0 or 1)<br data-darkosha-id="1:124" style="">
+end<br data-darkosha-id="1:125" style="">
+<br data-darkosha-id="1:126" style="">
+<br data-darkosha-id="1:127" style="">
+-- Test body.<br data-darkosha-id="1:128" style="">
+<br data-darkosha-id="1:129" style="">
+local ffi = require('ffi')<br data-darkosha-id="1:130" style="">
+local fiber = require('fiber')<br data-darkosha-id="1:131" style="">
+<br data-darkosha-id="1:132" style="">
+ffi.cdef('struct test { int foo; };')<br data-darkosha-id="1:133" style="">
+<br data-darkosha-id="1:134" style="">
+local test = ffi.metatype('struct test', {<br data-darkosha-id="1:135" style="">
+ __gc = function() fiber.yield() end,<br data-darkosha-id="1:136" style="">
+})<br data-darkosha-id="1:137" style="">
+<br data-darkosha-id="1:138" style="">
+local t = test(9)<br data-darkosha-id="1:139" style="">
+t = nil<br data-darkosha-id="1:140" style="">
+<br data-darkosha-id="1:141" style="">
+-- This call leads to the platform panic.<br data-darkosha-id="1:142" style="">
+collectgarbage('collect')<br data-darkosha-id="1:143" style="">
-- <br data-darkosha-id="1:144" style="">
2.25.0<br data-darkosha-id="1:145" style="">
<br data-darkosha-id="1:146" style="">
</div>
<base target="_self" href="https://e.mail.ru/" data-darkosha-id="1:147" style="">
</div>
</div></blockquote></div></div></BODY></HTML>