From: Sergey Kaplun <skaplun@tarantool.org>
To: Igor Munkin <imun@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH v4 2/2] misc: add C and Lua API for platform metrics
Date: Wed, 7 Oct 2020 17:35:00 +0300 [thread overview]
Message-ID: <20201007143500.GA27749@root> (raw)
In-Reply-To: <20201006221721.GN18920@tarantool.org>
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
next prev parent reply other threads:[~2020-10-07 14:35 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-10-05 6:30 [Tarantool-patches] [PATCH v4 0/2] Implement LuaJIT " Sergey Kaplun
2020-10-05 6:30 ` [Tarantool-patches] [PATCH v4 1/2] core: introduce various " Sergey Kaplun
2020-10-07 14:11 ` Igor Munkin
2020-10-07 19:55 ` Sergey Kaplun
2020-10-07 20:16 ` Igor Munkin
2020-10-08 9:28 ` Igor Munkin
2020-10-08 10:11 ` Sergey Kaplun
2020-10-08 12:44 ` Igor Munkin
2020-10-09 14:39 ` Sergey Ostanevich
2020-10-05 6:30 ` [Tarantool-patches] [PATCH v4 2/2] misc: add C and Lua API for " Sergey Kaplun
2020-10-06 22:17 ` Igor Munkin
2020-10-07 5:57 ` Igor Munkin
2020-10-07 14:35 ` Sergey Kaplun [this message]
2020-10-07 18:23 ` Igor Munkin
2020-10-07 20:09 ` Sergey Kaplun
2020-10-09 14:45 ` Sergey Ostanevich
2020-10-13 6:01 ` Sergey Kaplun
2020-10-05 6:30 ` [Tarantool-patches] [RFC v4] rfc: luajit metrics Sergey Kaplun
2020-10-07 14:46 ` Sergey Kaplun
2020-10-08 17:25 ` Igor Munkin
2020-10-08 19:29 ` Sergey Kaplun
2020-10-08 20:26 ` Igor Munkin
2020-10-09 6:06 ` Sergey Kaplun
2020-12-22 9:07 ` Kirill Yukhin
2020-10-08 17:33 ` [Tarantool-patches] [PATCH v4 0/2] Implement LuaJIT platform metrics Igor Munkin
2020-10-13 13:17 ` Kirill Yukhin
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=20201007143500.GA27749@root \
--to=skaplun@tarantool.org \
--cc=imun@tarantool.org \
--cc=tarantool-patches@dev.tarantool.org \
--subject='Re: [Tarantool-patches] [PATCH v4 2/2] misc: add C and Lua API for platform metrics' \
/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