<HTML><BODY><div class="js-helper js-readmsg-msg"><div id="style_16686014901337055000"><div id="style_16686014901337055000_BODY"><div class="cl_619968"><div>Hi, Sergey!</div><div>Thanks for the patch!</div><div>LGTM</div><blockquote style="border-left:1px solid #0857A6;margin:10px;padding:0 0 0 10px;"><div> <blockquote style="border-left:1px solid #0857A6;margin:10px;padding:0 0 0 10px;"><div id=""><div class="js-helper_mr_css_attr js-readmsg-msg_mr_css_attr"><div><div id="style_16681723661646084920_BODY_mr_css_attr">Hi, again!<br><br>Added minor fixes.<br><br>On 11.11.22, Sergey Kaplun via Tarantool-patches wrote:<br><br><snipped><br><br>> ===================================================================<br>> diff --git a/test/tarantool-tests/lj-906-fix-err-mem.test.lua b/test/tarantool-tests/lj-906-fix-err-mem.test.lua<br>> index f512e802..0ec1b30e 100644<br>> --- a/test/tarantool-tests/lj-906-fix-err-mem.test.lua<br>> +++ b/test/tarantool-tests/lj-906-fix-err-mem.test.lua<br>> @@ -12,7 +12,7 @@ local KB = 1024<br>> local MB = 1024 * KB<br>><br>> -- The maximum available table size, taking into account created<br>> --- constants for one function.<br>> +-- constants for one function and nested level.<br>> local TNEW_SIZE = 511<br>><br>> local gc_anchor = {}<br>> @@ -29,9 +29,8 @@ local function eat_chunks(size)<br>> end<br>><br>> -- Function to format inner tab leading to TDUP emitting.<br>> -local function format_inner_tab()<br>> +local function make_flat_table(inner_depth)<br><br>Should be deep_table, obviously.<br><br>===================================================================<br>diff --git a/test/tarantool-tests/lj-906-fix-err-mem.test.lua b/test/tarantool-tests/lj-906-fix-err-mem.test.lua<br>index 7dbc1614..69f74893 100644<br>--- a/test/tarantool-tests/lj-906-fix-err-mem.test.lua<br>+++ b/test/tarantool-tests/lj-906-fix-err-mem.test.lua<br>@@ -29,7 +29,7 @@ local function eat_chunks(size)<br> end<br> <br> -- Function to format inner tab leading to TDUP emitting.<br>-local function make_flat_table(inner_depth)<br>+local function make_deep_table(inner_depth)<br> local inner_tab = ''<br> -- Repeate table template for TDUP.<br> for _ = 1, inner_depth do<br>@@ -49,7 +49,7 @@ local function format_TDUP_chunk()<br> local big_tab = 'local _ = {\n'<br> -- The maximum available table size, taking into account created<br> -- constants for one function and nested level.<br>- local inner_tab = make_flat_table(128)<br>+ local inner_tab = make_deep_table(128)<br> for _ = 1, TNEW_SIZE do<br> big_tab = big_tab .. inner_tab .. '\n'<br> end<br>===================================================================<br><br>> local inner_tab = ''<br>> - local inner_depth = 128<br>> -- Repeate table template for TDUP.<br>> for _ = 1, inner_depth do<br>> inner_tab = inner_tab .. '{a ='<br>> @@ -43,9 +42,14 @@ local function format_inner_tab()<br>> return inner_tab<br>> end<br>><br>> +-- The `lj_err_mem()` doesn't fix `L->top`, when called from<br>> +-- helper function (like `lj_tab_dup()`) before the patch.<br>> +-- This function creates a chunk with many BC_TDUP inside.<br>> local function format_TDUP_chunk()<br>> local big_tab = 'local _ = {\n'<br>> - local inner_tab = format_inner_tab()<br>> + -- The maximum available table size, taking into account created<br>> + -- constants for one function and nested level.<br>> + local inner_tab = make_flat_table(128)<br>> for _ = 1, TNEW_SIZE do<br>> big_tab = big_tab .. inner_tab .. '\n'<br>> end<br>> @@ -56,12 +60,10 @@ end<br>> local TDUP, err = loadstring(format_TDUP_chunk())<br>> assert(TDUP, err)<br>><br>> +-- Function to create the additional frame after to be rewrited<br>> +-- one in case of `lj_err_mem()` misbehaviour.<br><br>Also, minor comments fixes and renaming.<br>===================================================================<br>diff --git a/test/tarantool-tests/lj-906-fix-err-mem.test.lua b/test/tarantool-tests/lj-906-fix-err-mem.test.lua<br>index 69f74893..f4ec6620 100644<br>--- a/test/tarantool-tests/lj-906-fix-err-mem.test.lua<br>+++ b/test/tarantool-tests/lj-906-fix-err-mem.test.lua<br>@@ -45,7 +45,7 @@ end<br> -- The `lj_err_mem()` doesn't fix `L->top`, when called from<br> -- helper function (like `lj_tab_dup()`) before the patch.<br> -- This function creates a chunk with many BC_TDUP inside.<br>-local function format_TDUP_chunk()<br>+local function make_TDUP_chunk()<br> local big_tab = 'local _ = {\n'<br> -- The maximum available table size, taking into account created<br> -- constants for one function and nested level.<br>@@ -57,11 +57,11 @@ local function format_TDUP_chunk()<br> return big_tab<br> end<br> <br>-local TDUP, err = loadstring(format_TDUP_chunk())<br>+local TDUP, err = loadstring(make_TDUP_chunk())<br> assert(TDUP, err)<br> <br>--- Function to create the additional frame after to be rewrited<br>--- one in case of `lj_err_mem()` misbehaviour.<br>+-- Function to create the additional frame to be rewritten later<br>+-- in case of `lj_err_mem()` misbehaviour.<br> local function frame_before_TDUP()<br> TDUP()<br> end<br>===================================================================<br><br>Branch is force-pushed.<br>> local function frame_before_TDUP()<br>> - -- Stack slots are needed for coredump in case of misbehaviour.<br>> - -- luacheck: no unused<br>> - local frame_slot1, frame_slot2<br>> TDUP()<br>> - return frame_slot1, frame_slot2<br>> end<br>><br>> collectgarbage()<br>> ===================================================================<br>><br>><br>> ><br>> > > + -- luacheck: no unused<br>> > > + local frame_slot1, frame_slot2<br>> > > + TDUP()<br>> > > + return frame_slot1, frame_slot2<br>> > > +end<br>> > > +<br>> ><br>> > Minor: I believe you can pack these two lines into something one can<br>> > call <janitor>:<br>> > | local function janitor()<br>> > | collectgarbage('collect')<br>> > | collectgarbage('stop')<br>> > | end<br>> ><br>> > Anyway, you can leave this as is.<br>><br>> Added, thanks!<br>> See the iterative patch below.<br>> ===================================================================<br>> diff --git a/test/tarantool-tests/lj-906-fix-err-mem.test.lua b/test/tarantool-tests/lj-906-fix-err-mem.test.lua<br>> index 519e6bf6..7dbc1614 100644<br>> --- a/test/tarantool-tests/lj-906-fix-err-mem.test.lua<br>> +++ b/test/tarantool-tests/lj-906-fix-err-mem.test.lua<br>> @@ -66,8 +66,12 @@ local function frame_before_TDUP()<br>> TDUP()<br>> end<br>><br>> -collectgarbage()<br>> -collectgarbage('stop')<br>> +local function janitor()<br>> + collectgarbage('collect')<br>> + collectgarbage('stop')<br>> +end<br>> +<br>> +janitor()<br>><br>> -- Avoid OOM on traces.<br>> jit.off()<br>> @@ -75,14 +79,11 @@ jit.off()<br>> -- Stack slots are needed for coredump in case of misbehaviour.<br>> -- luacheck: no unused<br>> local r, e = pcall(eat_chunks, 8 * MB)<br>> -collectgarbage()<br>> -collectgarbage('stop')<br>> +janitor()<br>> pcall(eat_chunks, 8 * KB)<br>> -collectgarbage()<br>> -collectgarbage('stop')<br>> +janitor()<br>> pcall(eat_chunks, 8)<br>> -collectgarbage()<br>> -collectgarbage('stop')<br>> +janitor()<br>><br>> pcall(frame_before_TDUP)<br>> ===================================================================<br>><br>><br>> ><br>> > > +collectgarbage()<br>> > > +collectgarbage('stop')<br>> > > +<br>> > > +-- Avoid OOM on traces.<br>> > > +jit.off()<br>> > > +<br>> > > +-- Stack slots are needed for coredump in case of misbehaviour.<br>> > > +-- luacheck: no unused<br>> > > +local r, e = pcall(eat_chunks, 8 * MB)<br>> > > +collectgarbage()<br>> > > +pcall(eat_chunks, 8 * KB)<br>> > > +collectgarbage()<br>> > > +pcall(eat_chunks, 8)<br>> > > +collectgarbage()<br>> > > +<br>> > > +pcall(frame_before_TDUP)<br>> ><br>> > Do we need to check the status of the <pcall> above?<br>><br>> I suppose no. As we disscussed offline the test may be flaky (false<br>> positive, when we can alloc all tables) due to memory allocations.<br>> Also, this part is very fragile, and we don't want to fix test later by<br>> removing this check the status of the `pcall()`.<br>><br>> ><br>> > > +<br>> > > +-- Release memory for `tap` functions.<br>> > > +gc_anchor = nil<br>> > > +collectgarbage()<br>> ><br>> > Is this step required? I doubt.<br>><br>> Yes it is. Without it we may get the OOM error inside `test:ok()` or<br>> `test:check()` functions (I faced it on my local machine, when rewrite<br>> this test).<br>><br>> ><br>> > > +<br>> > > +test:ok(true, 'correctly throw memory error')<br>> > > +<br>> > > +os.exit(test:check() and 0 or 1)<br>> > > --<br>> > > 2.34.1<br>> > ><br>> ><br>> > --<br>> > Best regards,<br>> > IM<br>><br>> --<br>> Best regards,<br>> Sergey Kaplun<br><br>--<br>Best regards,<br>Sergey Kaplun</div></div></div></div></blockquote><div> </div></div></blockquote></div></div></div></div><div>Best regards,</div><div>Maxim Kokryashkin</div></BODY></HTML>