[Tarantool-patches] [PATCH v4 2/2] misc: add C and Lua API for platform metrics

Sergey Kaplun skaplun at tarantool.org
Wed Oct 7 17:35:00 MSK 2020


Hi Igor! Thanks for the review!

On 07.10.20, Igor Munkin wrote:
> Sergey,
> 
> Thanks for the patch! Please consider my comments below.
> 

<snipped>

> 
> > +#define LJLIB_MODULE_misc
> > +
> > +LJLIB_CF(misc_getmetrics)
> > +{
> > +  struct luam_Metrics metrics;
> > +  lua_createtable(L, 0, 22);
> 
> 19 slots are definitely enough.

Nice catch! Forgot to change this line when was removing colors counting.

> 
> > +  GCtab *m = tabV(L->top - 1);
> 
> Once more: please, declare the variables in the beginning of the block.

Ooops. My bad. Fixed.

> 
> > +
> > +  luaM_metrics(L, &metrics);
> > +
> 
> <snipped>
> 
> > diff --git a/src/lj_mapi.c b/src/lj_mapi.c
> > new file mode 100644
> > index 0000000..7645a44
> > --- /dev/null
> > +++ b/src/lj_mapi.c
> > @@ -0,0 +1,61 @@
> 
> <snipped>
> 
> > +
> > +LUAMISC_API void luaM_metrics(lua_State *L, struct luam_Metrics *metrics)
> > +{
> > +  lua_assert(metrics != NULL);
> 
> Ditto.

And this too.

> 
> > +  global_State *g = G(L);
> > +  GCState *gc = &g->gc;
> > +#if LJ_HASJIT
> > +  jit_State *J = G2J(g);
> > +#endif
> > +

<snipped>

> > +struct luam_Metrics {
> > +  /* Strings amount found in string hash instead of allocation of new one. */
> 
> If I don't know how this metric is collected, I'll never guess what this
> comment mean. Let's reword it the following way:
> | /*
> | ** Number of strings being interned (i.e. the string with the same
> | ** payload is found, so a new one is not created/allocated)
> | */
> 

Thanks! Fixed.

> > +  size_t strhash_hit;
> > +  /* Strings amount allocated and put into string hash. */
> 
> Well, does it mean this metric has the same value as gc_strnum?
> Not indeed. This is a number of the strings

Ok. I rewrote this in the following way:
| /* Total number of strings allocations during the platform lifetime. */

> 
> > +  size_t strhash_miss;

<snipped>

> > +
> > +  /*
> > +  ** Overall number of snap restores (amount of guard assertions
> > +  ** leading to stopping trace executions and trace exits,
> > +  ** that are not stitching with other traces).
> 
> Minor: Strictly saying, it's not a stitching in LuaJIT terms.

Removed misleading comments.

> 
> > +  */
> > +  size_t jit_snap_restore;
> > +  /* Overall number of abort traces. */
> > +  size_t jit_trace_abort;
> > +  /* Total size of all allocated machine code areas. */
> > +  size_t jit_mcode_size;
> > +  /* Amount of JIT traces. */
> > +  unsigned int jit_trace_num;
> > +};
> 
> <snipped>
> 
> > diff --git a/test/clib-misclib-getmetrics.test.lua b/test/clib-misclib-getmetrics.test.lua
> 
> Add Github issue to the file name.

Here and below:
I thought we use gh-xxxx style for bugfix tests. Tests for new
features are named the same as a feature.
Is `misclib-getmetrics-{capi,lapi}.test.lua` good enough?

> 
> > new file mode 100755
> > index 0000000..34adaba
> > --- /dev/null
> > +++ b/test/clib-misclib-getmetrics.test.lua
> > @@ -0,0 +1,174 @@
> > +#!/usr/bin/env tarantool
> > +
> > +local file = debug.getinfo(1, "S").source:sub(2)
> > +local filepath = file:match("(.*/)")
> > +local soext = jit.os == "OSX" and "dylib" or "so"
> > +package.cpath = filepath..'clib-misclib-getmetrics/?.'..soext..";"
> > +                ..package.cpath
> 
> I've already solved this issue, so this part can be easily borrowed from
> utils.lua[1]. Consider the following:
> | local path = arg[0]:gsub('%.test%.lua', '')
> | local suffix = package.cpath:match('?.(%a+);')
> | package.cpath = ('%s/?.%s;%s'):format(path, suffix, package.cpath)
> 

Yep. See now. Thanks! We should add one small function for this
to reuse in other tests later.

> > +
> > +local jit_opt_default_hotloop = 56
> > +local jit_opt_default_hotexit = 10
> > +local jit_opt_default_level = 3
> > +local jit_opt_default_minstitch = 0
> 
> Well, this can be done in a much more elegant way:
> | local jit_opt_default = {
> |   3, -- level
> |   "hotloop=56",
> |   "hotexit=10",
> |   "minstitch=0",
> | }
> |
> | <snipped>
> |
> | jit.opt.start(unpack(jit_opt_default))
> 

Yep. Really looks better.

> > +

<snipped>

> > +    for i = 1, 1000 do
> > +        table.insert(placeholder.tab, table_new(i, 0))
> 
> This can be done via creating { i }, so table.new is excess here.

Yes! Thanks! Added here and below.

> 
> > +    end
> 
> <snipped>
> 
> > +-- Compiled loop with a direct exit to the interpreter.
> > +test:ok(testgetmetrics.snap_restores(function()
> > +    jit.opt.start(2, "hotloop=2")
> 
> Why hotloop is 2?

It has no any sacral significance. To avoid confusion, I've changed the
value to 1 here and below.

> 

<snipped>

> 
> > diff --git a/test/clib-misclib-getmetrics/testgetmetrics.c b/test/clib-misclib-getmetrics/testgetmetrics.c
> 

<snipped>

> 
> > +static int base(lua_State *L)
> > +{
> > +	struct luam_Metrics metrics;
> > +	luaM_metrics(L, &metrics);
> > +
> > +	/* Just check API. */
> 
> Minor: it worths to drop a comment why there is a <ssize_t> here. It's
> quite obvious, but has come to my mind only in a few seconds.

Sure! No problem!

> 
> > +	assert((ssize_t)metrics.strhash_hit >= 0);
> 
> <snipped>
> 
> > +}
> > +
> > +static int gc_allocated_freed(lua_State *L)
> > +{
> > +	struct luam_Metrics oldm, newm;
> > +	/* Force up garbage collect all dead objects. */
> > +	lua_gc(L, LUA_GCCOLLECT, 0);
> > +
> > +	luaM_metrics(L, &oldm);
> > +	/* Simple garbage generation. */
> > +	if (luaL_dostring(L, "local i = 0 for j = 1, 10 do i = i + j end"))
> > +		luaL_error(L, "failed to translate Lua code snippet");
> 
> Side note: OK, this is a funny example: the code to be executed doesn't
> generate the garbage per se, but the machinery running it does. This is

Yes, that was the idea.

> nice, since AFAICS you can calculate the exact amount to allocated and
> freed, can't you? Feel free to ignore this comment or consider it as
> an extracurricular activity.

I didn't set the exact value on purpose. It seemed to me that we should
not be tied to the behavior of the corresponding machinery, that can to be
changed in future.

> 
> > +	lua_gc(L, LUA_GCCOLLECT, 0);
> > +	luaM_metrics(L, &newm);
> > +	assert(newm.gc_allocated - oldm.gc_allocated > 0);
> > +	assert(newm.gc_freed - oldm.gc_freed > 0);
> > +
> > +	lua_pushboolean(L, 1);
> > +	return 1;
> > +}
> > +
> > +static int gc_steps(lua_State *L)
> > +{
> > +	struct luam_Metrics oldm, newm;
> > +	/*
> > +	 * Some garbage has already happened before the next line,
> > +	 * i.e. during fronted processing lua test chunk.
> 
> Typo: s/fronted/frontend/.
> Typo: s/lua/Lua/.

Fixed here and below.

> 
> > +	 * Let's put a full garbage collection cycle on top
> > +	 * of that, and confirm that non-null values are reported
> > +	 * (we are not yet interested in actual numbers):
> > +	 */
> > +	luaM_metrics(L, &oldm);
> > +	lua_gc(L, LUA_GCCOLLECT, 0);
> 
> OK, this is a misguiding part. At first I was confused with the comment
> below that *new values are the same*. Why the difference doesn't equals
> to zero if they are the same? Later I found the <lua_gc> call after
> <oldm> initialization *but* prior to these asserts. I guess the next
> reordering definitely make the code clearer:
> | luaM_metrics(L, &oldm);
> | assert(oldm.gc_steps_pause > 0);
> | <other asserts>
> | lua_gc(L, LUA_GCCOLLECT, 0);
> 
> > +	assert(oldm.gc_steps_pause > 0);
> 
> <snipped>
> 
> > +	/*
> > +	 * As long as we don't create new Lua objects
> > +	 * consequent call should return the same values:
> 
> How come? They *are not* the same considering the asserts below (except
> <gc_steps_finalize>). The comment is misguiding.
> 

This is bad mistake of mine. Here I want to check that results are the
same after two consecutive calls. Fixed. Thanks for your question!

> > +	 */
> > +	luaM_metrics(L, &newm);
> > +	assert(newm.gc_steps_pause - oldm.gc_steps_pause > 0);
> 
> <snipped>
> 
> > +}
> > +
> > +static int objcount(lua_State *L)
> > +{
> > +	struct luam_Metrics oldm, newm;
> > +	int n = lua_gettop(L);
> > +	if (n != 1 || !lua_isfunction(L, 1))
> > +		luaL_error(L, "incorrect argument: 1 function is required");
> > +
> > +	/* Force up garbage collect all dead objects. */
> > +	lua_gc(L, LUA_GCCOLLECT, 0);
> > +
> > +	luaM_metrics(L, &oldm);
> > +	/* Generate garbage. */
> > +	lua_call(L, 0, 0);
> 
> If loop optimization is disabled, then you can pass the number of
> iterations as an argument to the function and check the metrics values
> against it. Thoughts?

But there are more objects besides base generated one (for example
strings like "uint64_t", "ffi").
But we can steal set number of iterations here this makes test more flexible.
I've added it to patch.

> 
> > +	lua_gc(L, LUA_GCCOLLECT, 0);
> > +	luaM_metrics(L, &newm);
> 
> <snipped>
> 
> > +}
> 
> > +static int tracenum_base(lua_State *L)
> > +{
> > +	struct luam_Metrics metrics;
> > +	int n = lua_gettop(L);
> > +	if (n != 1 || !lua_isfunction(L, 1))
> > +		luaL_error(L, "incorrect arguments: 1 function is required");
> > +
> > +	luaJIT_setmode(L, 0, LUAJIT_MODE_OFF);
> 
> This line looks excess.

Removed here and below.

> 
> > +	luaJIT_setmode(L, 0, LUAJIT_MODE_FLUSH);
> > +	/* Force up garbage collect all dead objects. */
> > +	lua_gc(L, LUA_GCCOLLECT, 0);
> > +
> > +	luaM_metrics(L, &metrics);
> > +	assert(metrics.jit_trace_num == 0);
> > +
> > +	luaJIT_setmode(L, 0, LUAJIT_MODE_ON);
> 
> <snipped>
> 
> > +
> > +	luaJIT_setmode(L, 0, LUAJIT_MODE_FLUSH);
> > +	/* Force up garbage collect all dead objects. */
> > +	lua_gc(L, LUA_GCCOLLECT, 0);
> > +	luaM_metrics(L, &metrics);
> > +	assert(metrics.jit_trace_num == 0);
> > +
> > +	luaJIT_setmode(L, 0, LUAJIT_MODE_ON);
> 
> This line looks excess.

Removed here and below.

> 
> > +	lua_pushboolean(L, 1);
> > +	return 1;
> > +}
> 
> <snipped>
> 
> > diff --git a/test/lib-misc-getmetrics.test.lua b/test/lib-misc-getmetrics.test.lua
> 
> Add Github issue to the file name.
> 
> > new file mode 100755
> > index 0000000..2859e26
> > --- /dev/null
> > +++ b/test/lib-misc-getmetrics.test.lua
> > @@ -0,0 +1,418 @@
> > +#!/usr/bin/env tarantool
> > +
> > +-- This is a part of tarantool/luajit testing suite.
> > +-- Major portions taken verbatim or adapted from the LuaVela testing suit.
> 
> Typo: s/suit/suite/.

Thanks! Fixed.

> 
> > +-- Copyright (C) 2015-2019 IPONWEB Ltd.
> 
> <snipped>
> 
> > +test:test("gc-allocated-freed", function(subtest)
> > +    subtest:plan(1)
> > +
> > +    -- Force up garbage collect all dead objects.
> > +    collectgarbage("collect")
> > +
> > +    -- Bump getmetrincs table and string keys allocation.
> 
> Typo: s/getmetrincs/getmetrics/.

Thanks! Fixed!

> 
> > +    local old_metrics = misc.getmetrics()
> > +
> > +    -- Remember allocated size for getmetrics table.
> > +    old_metrics = misc.getmetrics()
> > +
> > +    collectgarbage("collect")
> > +
> > +    local new_metrics = misc.getmetrics()
> > +
> > +    local diff_alloc = new_metrics.gc_allocated - old_metrics.gc_allocated
> > +    local getmetrics_alloc = diff_alloc
> 
> Why the difference can't be assigned right to <getmetrics_alloc>?

Yep, it will be more clear. Thanks!

> 

<snipped>

> 
> > +    -- Bump strings and table creation.
> > +    local old_metrics = misc.getmetrics()
> 
> OK, I finally got the math you explained to me offline, but it's too
> complex for such simple case. Let's consider the newly allocated table
> right in the test assertion.
> 

Yep, this looks confusing. Fixed.

> > +    old_metrics = misc.getmetrics()

<snipped>

> 
> > +test:test("tracenum-base", function(subtest)
> > +    subtest:plan(3)
> > +
> > +    jit.off()
> 
> This line looks excess.

Yep. Removed.

> 
> > +    jit.flush()
> > +    collectgarbage("collect")
> > +    local metrics = misc.getmetrics()
> > +    subtest:is(metrics.jit_trace_num, 0)
> > +
> > +    jit.on()

And this too.

> > +    local sum = 0
> > +    for i = 1, 100 do
> > +        sum = sum + i
> > +    end
> > +
> > +    metrics = misc.getmetrics()
> > +    subtest:is(metrics.jit_trace_num, 1)
> > +
> > +    jit.off()
> 
> This line looks excess.

Yep. Removed.

> 
> > +    jit.flush()
> > +    collectgarbage("collect")
> > +
> > +    metrics = misc.getmetrics()
> > +    subtest:is(metrics.jit_trace_num, 0)
> > +
> > +    jit.on()

And this too.

> > +end)
> > +
> > +os.exit(test:check() and 0 or 1)
> > -- 
> > 2.28.0
> > 
> 
> [1]: https://github.com/tarantool/luajit/blob/tarantool/test/utils.lua
> 
> -- 
> Best regards,
> IM

See iterative patch in the bottom. Branch force-pushed.

===================================================================
diff --git a/src/lib_misc.c b/src/lib_misc.c
index ef11237..6f7b9a9 100644
--- a/src/lib_misc.c
+++ b/src/lib_misc.c
@@ -29,8 +29,10 @@ static LJ_AINLINE void setnumfield(struct lua_State *L, GCtab *t,
 LJLIB_CF(misc_getmetrics)
 {
   struct luam_Metrics metrics;
-  lua_createtable(L, 0, 22);
-  GCtab *m = tabV(L->top - 1);
+  GCtab *m;
+
+  lua_createtable(L, 0, 19);
+  m = tabV(L->top - 1);
 
   luaM_metrics(L, &metrics);
 
diff --git a/src/lj_mapi.c b/src/lj_mapi.c
index 7645a44..13737e0 100644
--- a/src/lj_mapi.c
+++ b/src/lj_mapi.c
@@ -17,13 +17,14 @@
 
 LUAMISC_API void luaM_metrics(lua_State *L, struct luam_Metrics *metrics)
 {
-  lua_assert(metrics != NULL);
   global_State *g = G(L);
   GCState *gc = &g->gc;
 #if LJ_HASJIT
   jit_State *J = G2J(g);
 #endif
 
+  lua_assert(metrics != NULL);
+
   metrics->strhash_hit = g->strhash_hit;
   metrics->strhash_miss = g->strhash_miss;
 
diff --git a/src/lmisclib.h b/src/lmisclib.h
index e2d1909..0c07707 100644
--- a/src/lmisclib.h
+++ b/src/lmisclib.h
@@ -13,9 +13,12 @@
 /* API for obtaining various platform metrics. */
 
 struct luam_Metrics {
-  /* Strings amount found in string hash instead of allocation of new one. */
+  /*
+  ** Number of strings being interned (i.e. the string with the
+  ** same payload is found, so a new one is not created/allocated).
+  */
   size_t strhash_hit;
-  /* Strings amount allocated and put into string hash. */
+  /* Total number of strings allocations during the platform lifetime. */
   size_t strhash_miss;
 
   /* Amount of allocated string objects. */
@@ -44,8 +47,7 @@ struct luam_Metrics {
 
   /*
   ** Overall number of snap restores (amount of guard assertions
-  ** leading to stopping trace executions and trace exits,
-  ** that are not stitching with other traces).
+  ** leading to stopping trace executions).
   */
   size_t jit_snap_restore;
   /* Overall number of abort traces. */
diff --git a/test/misclib-getmetrics-capi.test.lua b/test/misclib-getmetrics-capi.test.lua
index 34adaba..f133274 100755
--- a/test/misclib-getmetrics-capi.test.lua
+++ b/test/misclib-getmetrics-capi.test.lua
@@ -1,15 +1,15 @@
 #!/usr/bin/env tarantool
 
-local file = debug.getinfo(1, "S").source:sub(2)
-local filepath = file:match("(.*/)")
-local soext = jit.os == "OSX" and "dylib" or "so"
-package.cpath = filepath..'clib-misclib-getmetrics/?.'..soext..";"
-                ..package.cpath
+local path = arg[0]:gsub('%.test%.lua', '')
+local suffix = package.cpath:match('?.(%a+);')
+package.cpath = ('%s/?.%s'):format(path, suffix)
 
-local jit_opt_default_hotloop = 56
-local jit_opt_default_hotexit = 10
-local jit_opt_default_level = 3
-local jit_opt_default_minstitch = 0
+local jit_opt_default = {
+    3, -- level
+    "hotloop=56",
+    "hotexit=10",
+    "minstitch=0",
+}
 
 local tap = require('tap')
 
@@ -22,8 +22,7 @@ test:ok(testgetmetrics.base())
 test:ok(testgetmetrics.gc_allocated_freed())
 test:ok(testgetmetrics.gc_steps())
 
-test:ok(testgetmetrics.objcount(function()
-    local table_new = require("table.new")
+test:ok(testgetmetrics.objcount(function(iterations)
     local ffi = require("ffi")
 
     jit.opt.start(0)
@@ -36,36 +35,36 @@ test:ok(testgetmetrics.objcount(function()
     }
 
     -- Separate objects creations to separate jit traces.
-    for i = 1, 1000 do
+    for i = 1, iterations do
         table.insert(placeholder.str, tostring(i))
     end
 
-    for i = 1, 1000 do
-        table.insert(placeholder.tab, table_new(i, 0))
+    for i = 1, iterations do
+        table.insert(placeholder.tab, {i})
     end
 
-    for i = 1, 1000 do
+    for i = 1, iterations do
         table.insert(placeholder.udata, newproxy())
     end
 
-    for i = 1, 1000 do
+    for i = 1, iterations do
         -- Check counting of VLA/VLS/aligned cdata.
         table.insert(placeholder.cdata, ffi.new("char[?]", 4))
     end
 
-    for i = 1, 1000 do
+    for i = 1, iterations do
         -- Check counting of non-VLA/VLS/aligned cdata.
         table.insert(placeholder.cdata, ffi.new("uint64_t", i))
     end
 
     placeholder = nil
     -- Restore default jit settings.
-    jit.opt.start(jit_opt_default_level)
+    jit.opt.start(unpack(jit_opt_default))
 end))
 
 -- Compiled loop with a direct exit to the interpreter.
 test:ok(testgetmetrics.snap_restores(function()
-    jit.opt.start(0, "hotloop=2")
+    jit.opt.start(0, "hotloop=1")
 
     local old_metrics = misc.getmetrics()
 
@@ -77,7 +76,7 @@ test:ok(testgetmetrics.snap_restores(function()
     local new_metrics = misc.getmetrics()
 
     -- Restore default jit settings.
-    jit.opt.start(jit_opt_default_level, "hotloop="..jit_opt_default_hotloop)
+    jit.opt.start(unpack(jit_opt_default))
 
     -- A single snapshot restoration happened on loop finish.
     return 1
@@ -85,7 +84,7 @@ end))
 
 -- Compiled loop with a side exit which does not get compiled.
 test:ok(testgetmetrics.snap_restores(function()
-    jit.opt.start(0, "hotloop=2", "hotexit=2", "minstitch=15")
+    jit.opt.start(0, "hotloop=1", "hotexit=2", "minstitch=15")
 
     local function foo(i)
         -- math.fmod is not yet compiled!
@@ -98,9 +97,7 @@ test:ok(testgetmetrics.snap_restores(function()
     end
 
     -- Restore default jit settings.
-    jit.opt.start(jit_opt_default_level, "hotloop="..jit_opt_default_hotloop,
-                  "hotexit="..jit_opt_default_hotexit,
-                  "minstitch="..jit_opt_default_minstitch)
+    jit.opt.start(unpack(jit_opt_default))
 
     -- Side exits from the root trace could not get compiled.
     return 5
@@ -110,7 +107,7 @@ end))
 test:ok(testgetmetrics.snap_restores(function()
     -- Optimization level is important here as `loop` optimization
     -- may unroll the loop body and insert +1 side exit.
-    jit.opt.start(0, "hotloop=5", "hotexit=5")
+    jit.opt.start(0, "hotloop=1", "hotexit=5")
 
     local function foo(i)
         return i <= 10 and i or tostring(i)
@@ -122,8 +119,7 @@ test:ok(testgetmetrics.snap_restores(function()
     end
 
     -- Restore default jit settings.
-    jit.opt.start(jit_opt_default_level, "hotloop="..jit_opt_default_hotloop,
-                  "hotexit="..jit_opt_default_hotexit)
+    jit.opt.start(unpack(jit_opt_default))
 
     -- 5 side exits to the interpreter before trace gets hot
     -- and compiled
@@ -157,8 +153,8 @@ test:ok(testgetmetrics.snap_restores(function()
     foo(21)
 
     -- Restore default jit settings.
-    jit.opt.start(jit_opt_default_level, "hotloop="..jit_opt_default_hotloop,
-                  "hotexit="..jit_opt_default_hotexit)
+    jit.opt.start(unpack(jit_opt_default))
+
     return 2
 end))
 
diff --git a/test/misclib-getmetrics-capi/testgetmetrics.c b/test/misclib-getmetrics-capi/testgetmetrics.c
index 32802d2..4a7355a 100644
--- a/test/misclib-getmetrics-capi/testgetmetrics.c
+++ b/test/misclib-getmetrics-capi/testgetmetrics.c
@@ -11,7 +11,10 @@ static int base(lua_State *L)
 	struct luam_Metrics metrics;
 	luaM_metrics(L, &metrics);
 
-	/* Just check API. */
+	/*
+	 * Just check API.
+	 * (ssize_t) cast need for not always-true unsigned >= 0 check.
+	 */
 	assert((ssize_t)metrics.strhash_hit >= 0);
 	assert((ssize_t)metrics.strhash_miss >= 0);
 
@@ -64,13 +67,14 @@ static int gc_steps(lua_State *L)
 	struct luam_Metrics oldm, newm;
 	/*
 	 * Some garbage has already happened before the next line,
-	 * i.e. during fronted processing lua test chunk.
+	 * i.e. during frontend processing Lua test chunk.
 	 * Let's put a full garbage collection cycle on top
 	 * of that, and confirm that non-null values are reported
 	 * (we are not yet interested in actual numbers):
 	 */
-	luaM_metrics(L, &oldm);
 	lua_gc(L, LUA_GCCOLLECT, 0);
+
+	luaM_metrics(L, &oldm);
 	assert(oldm.gc_steps_pause > 0);
 	assert(oldm.gc_steps_propagate > 0);
 	assert(oldm.gc_steps_atomic > 0);
@@ -84,11 +88,11 @@ static int gc_steps(lua_State *L)
 	 * consequent call should return the same values:
 	 */
 	luaM_metrics(L, &newm);
-	assert(newm.gc_steps_pause - oldm.gc_steps_pause > 0);
-	assert(newm.gc_steps_propagate - oldm.gc_steps_propagate > 0);
-	assert(newm.gc_steps_atomic - oldm.gc_steps_atomic > 0);
-	assert(newm.gc_steps_sweepstring - oldm.gc_steps_sweepstring > 0);
-	assert(newm.gc_steps_sweep - oldm.gc_steps_sweep > 0);
+	assert(newm.gc_steps_pause - oldm.gc_steps_pause == 0);
+	assert(newm.gc_steps_propagate - oldm.gc_steps_propagate == 0);
+	assert(newm.gc_steps_atomic - oldm.gc_steps_atomic == 0);
+	assert(newm.gc_steps_sweepstring - oldm.gc_steps_sweepstring == 0);
+	assert(newm.gc_steps_sweep - oldm.gc_steps_sweep == 0);
 	/* Nothing to finalize, skipped. */
 	assert(newm.gc_steps_finalize == 0);
 	oldm = newm;
@@ -110,7 +114,7 @@ static int gc_steps(lua_State *L)
 
 	/*
 	 * Now let's run three GC cycles to ensure that
-	 * zero-to-one transition was not a lucky coincidence.
+	 * increment was not a lucky coincidence.
 	 */
 	lua_gc(L, LUA_GCCOLLECT, 0);
 	lua_gc(L, LUA_GCCOLLECT, 0);
@@ -139,8 +143,9 @@ static int objcount(lua_State *L)
 	lua_gc(L, LUA_GCCOLLECT, 0);
 
 	luaM_metrics(L, &oldm);
-	/* Generate garbage. */
-	lua_call(L, 0, 0);
+	/* Generate garbage. Argument is iterations amount. */
+	lua_pushnumber(L, 1000);
+	lua_call(L, 1, 0);
 	lua_gc(L, LUA_GCCOLLECT, 0);
 	luaM_metrics(L, &newm);
 	assert(newm.gc_strnum - oldm.gc_strnum == 0);
@@ -195,7 +200,6 @@ static int tracenum_base(lua_State *L)
 	if (n != 1 || !lua_isfunction(L, 1))
 		luaL_error(L, "incorrect arguments: 1 function is required");
 
-	luaJIT_setmode(L, 0, LUAJIT_MODE_OFF);
 	luaJIT_setmode(L, 0, LUAJIT_MODE_FLUSH);
 	/* Force up garbage collect all dead objects. */
 	lua_gc(L, LUA_GCCOLLECT, 0);
@@ -203,7 +207,6 @@ static int tracenum_base(lua_State *L)
 	luaM_metrics(L, &metrics);
 	assert(metrics.jit_trace_num == 0);
 
-	luaJIT_setmode(L, 0, LUAJIT_MODE_ON);
 	/* Generate traces. */
 	lua_call(L, 0, 1);
 	n = lua_gettop(L);
diff --git a/test/misclib-getmetrics-lapi.test.lua b/test/misclib-getmetrics-lapi.test.lua
index 2859e26..3b3d1f8 100755
--- a/test/misclib-getmetrics-lapi.test.lua
+++ b/test/misclib-getmetrics-lapi.test.lua
@@ -1,7 +1,7 @@
 #!/usr/bin/env tarantool
 
 -- This is a part of tarantool/luajit testing suite.
--- Major portions taken verbatim or adapted from the LuaVela testing suit.
+-- Major portions taken verbatim or adapted from the LuaVela testing suite.
 -- Copyright (C) 2015-2019 IPONWEB Ltd.
 
 local tap = require('tap')
@@ -9,10 +9,12 @@ local tap = require('tap')
 local test = tap.test("lib-misc-getmetrics")
 test:plan(10)
 
-local jit_opt_default_hotloop = 56
-local jit_opt_default_hotexit = 10
-local jit_opt_default_level = 3
-local jit_opt_default_minstitch = 0
+local jit_opt_default = {
+    3, -- level
+    "hotloop=56",
+    "hotexit=10",
+    "minstitch=0",
+}
 
 -- Test Lua API.
 test:test("base", function(subtest)
@@ -49,7 +51,7 @@ test:test("gc-allocated-freed", function(subtest)
     -- Force up garbage collect all dead objects.
     collectgarbage("collect")
 
-    -- Bump getmetrincs table and string keys allocation.
+    -- Bump getmetrics table and string keys allocation.
     local old_metrics = misc.getmetrics()
 
     -- Remember allocated size for getmetrics table.
@@ -59,8 +61,7 @@ test:test("gc-allocated-freed", function(subtest)
 
     local new_metrics = misc.getmetrics()
 
-    local diff_alloc = new_metrics.gc_allocated - old_metrics.gc_allocated
-    local getmetrics_alloc = diff_alloc
+    local getmetrics_alloc = new_metrics.gc_allocated - old_metrics.gc_allocated
 
     -- Do not use test:ok to avoid extra allocated/freed objects.
     assert(getmetrics_alloc > 0, "count allocated table for getmetrics")
@@ -75,7 +76,7 @@ test:test("gc-allocated-freed", function(subtest)
 
     new_metrics = misc.getmetrics()
 
-    diff_alloc = new_metrics.gc_allocated - old_metrics.gc_allocated
+    local diff_alloc = new_metrics.gc_allocated - old_metrics.gc_allocated
     local diff_freed = new_metrics.gc_freed - old_metrics.gc_freed
 
     assert(diff_alloc > getmetrics_alloc,
@@ -113,7 +114,7 @@ test:test("gc-steps", function(subtest)
     subtest:plan(24)
 
     -- Some garbage has already created before the next line,
-    -- i.e. during fronted processing this chunk.
+    -- i.e. during frontend processing this chunk.
     -- Let's put a full garbage collection cycle on top of that,
     -- and confirm that non-null values are reported (we are not
     -- yet interested in actual numbers):
@@ -154,8 +155,8 @@ test:test("gc-steps", function(subtest)
     subtest:is(newm.gc_steps_finalize, 0)
     oldm = newm
 
-    -- Now let's run three GC cycles to ensure that zero-to-one
-    -- transition was not a lucky coincidence.
+    -- Now let's run three GC cycles to ensure that increment
+    -- was not a lucky coincidence.
     collectgarbage("collect")
     collectgarbage("collect")
     collectgarbage("collect")
@@ -172,7 +173,6 @@ end)
 
 test:test("objcount", function(subtest)
     subtest:plan(4)
-    local table_new = require("table.new")
     local ffi = require("ffi")
 
     jit.opt.start(0)
@@ -180,9 +180,7 @@ test:test("objcount", function(subtest)
     -- Remove all dead objects.
     collectgarbage("collect")
 
-    -- Bump strings and table creation.
     local old_metrics = misc.getmetrics()
-    old_metrics = misc.getmetrics()
 
     local placeholder = {
         str = {},
@@ -197,7 +195,7 @@ test:test("objcount", function(subtest)
     end
 
     for i = 1, 1000 do
-        table.insert(placeholder.tab, table_new(i, 0))
+        table.insert(placeholder.tab, {i})
     end
 
     for i = 1, 1000 do
@@ -221,7 +219,12 @@ test:test("objcount", function(subtest)
     -- Check that amount of objects not increased.
     subtest:is(new_metrics.gc_strnum, old_metrics.gc_strnum,
                "strnum don't change")
-    subtest:is(new_metrics.gc_tabnum, old_metrics.gc_tabnum,
+    -- When we call getmetrics, we create table for metrics first.
+    -- So, when we save old_metrics there are x + 1 tables,
+    -- when we save new_metrics there are x + 2 tables, because
+    -- old table hasn't been collected yet (it is still
+    -- reachable).
+    subtest:is(new_metrics.gc_tabnum - old_metrics.gc_tabnum, 1,
                "tabnum don't change")
     subtest:is(new_metrics.gc_udatanum, old_metrics.gc_udatanum,
                "udatanum don't change")
@@ -229,14 +232,14 @@ test:test("objcount", function(subtest)
                "cdatanum don't change")
 
     -- Restore default jit settings.
-    jit.opt.start(jit_opt_default_level)
+    jit.opt.start(unpack(jit_opt_default))
 end)
 
 test:test("snap-restores-direct-loop", function(subtest)
     -- Compiled loop with a direct exit to the interpreter.
     subtest:plan(1)
 
-    jit.opt.start(0, "hotloop=2")
+    jit.opt.start(0, "hotloop=1")
 
     local old_metrics = misc.getmetrics()
 
@@ -251,14 +254,14 @@ test:test("snap-restores-direct-loop", function(subtest)
     subtest:is(new_metrics.jit_snap_restore - old_metrics.jit_snap_restore, 1)
 
     -- Restore default jit settings.
-    jit.opt.start(jit_opt_default_level, "hotloop="..jit_opt_default_hotloop)
+    jit.opt.start(unpack(jit_opt_default))
 end)
 
 test:test("snap-restores-loop-side-exit-non-compiled", function(subtest)
     -- Compiled loop with a side exit which does not get compiled.
     subtest:plan(1)
 
-    jit.opt.start(0, "hotloop=2", "hotexit=2", "minstitch=15")
+    jit.opt.start(0, "hotloop=1", "hotexit=2", "minstitch=15")
 
     local function foo(i)
         -- math.fmod is not yet compiled!
@@ -277,9 +280,7 @@ test:test("snap-restores-loop-side-exit-non-compiled", function(subtest)
     subtest:is(new_metrics.jit_snap_restore - old_metrics.jit_snap_restore, 5)
 
     -- Restore default jit settings.
-    jit.opt.start(jit_opt_default_level, "hotloop="..jit_opt_default_hotloop,
-                  "hotexit="..jit_opt_default_hotexit,
-                  "minstitch="..jit_opt_default_minstitch)
+    jit.opt.start(unpack(jit_opt_default))
 end)
 
 test:test("snap-restores-loop-side-exit", function(subtest)
@@ -288,7 +289,7 @@ test:test("snap-restores-loop-side-exit", function(subtest)
 
     -- Optimization level is important here as `loop` optimization
     -- may unroll the loop body and insert +1 side exit.
-    jit.opt.start(0, "hotloop=5", "hotexit=5")
+    jit.opt.start(0, "hotloop=1", "hotexit=5")
 
     local function foo(i)
         return i <= 10 and i or tostring(i)
@@ -308,8 +309,7 @@ test:test("snap-restores-loop-side-exit", function(subtest)
     subtest:is(new_metrics.jit_snap_restore - old_metrics.jit_snap_restore, 6)
 
     -- Restore default jit settings.
-    jit.opt.start(jit_opt_default_level, "hotloop="..jit_opt_default_hotloop,
-                  "hotexit="..jit_opt_default_hotexit)
+    jit.opt.start(unpack(jit_opt_default))
 end)
 
 test:test("snap-restores-scalar", function(subtest)
@@ -352,8 +352,7 @@ test:test("snap-restores-scalar", function(subtest)
     subtest:is(new_metrics.jit_snap_restore - old_metrics.jit_snap_restore, 2)
 
     -- Restore default jit settings.
-    jit.opt.start(jit_opt_default_level, "hotloop="..jit_opt_default_hotloop,
-                  "hotexit="..jit_opt_default_hotexit)
+    jit.opt.start(unpack(jit_opt_default))
 end)
 
 test:test("strhash", function(subtest)
@@ -390,13 +389,11 @@ end)
 test:test("tracenum-base", function(subtest)
     subtest:plan(3)
 
-    jit.off()
     jit.flush()
     collectgarbage("collect")
     local metrics = misc.getmetrics()
     subtest:is(metrics.jit_trace_num, 0)
 
-    jit.on()
     local sum = 0
     for i = 1, 100 do
         sum = sum + i
@@ -405,14 +402,11 @@ test:test("tracenum-base", function(subtest)
     metrics = misc.getmetrics()
     subtest:is(metrics.jit_trace_num, 1)
 
-    jit.off()
     jit.flush()
     collectgarbage("collect")
 
     metrics = misc.getmetrics()
     subtest:is(metrics.jit_trace_num, 0)
-
-    jit.on()
 end)
 
 os.exit(test:check() and 0 or 1)
===================================================================

-- 
Best regards,
Sergey Kaplun


More information about the Tarantool-patches mailing list