[Tarantool-patches] [PATCH 0/2] fiber.top(): minor fixup

[Tarantool-patches] [PATCH 0/2] fiber.top(): minor fixup

[Serge Petrenko]

The first patch fixes variable initialization after fiber.top_enable()
so that cord clocks don't contain garbage on first event loop iteration after
enable.

The second patch alters fiber.top() test to wait for correct output before
testing it.

Follow-up https://github.com/tarantool/tarantool/issues/2694
Branch https://github.com/tarantool/tarantool/tree/sp/gh-2694-test-fixup

Serge Petrenko (2):
  fiber: reset clock stats on fiber.top_enable()
  app/fiber: wait till a full event loop iteration ends.

 src/lib/core/fiber.c    |  3 +++
 test/app/fiber.result   | 35 ++++++++++++++++++++++++++---------
 test/app/fiber.test.lua | 31 +++++++++++++++++++++++--------
 3 files changed, 52 insertions(+), 17 deletions(-)

-- 
2.21.0 (Apple Git-122)

[Tarantool-patches] [PATCH 1/2] fiber: reset clock stats on fiber.top_enable()

[Serge Petrenko]

Follow-up #2694
---
 src/lib/core/fiber.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/lib/core/fiber.c b/src/lib/core/fiber.c
index aebaba7f0..cad1e21b0 100644
--- a/src/lib/core/fiber.c
+++ b/src/lib/core/fiber.c
@@ -1206,12 +1206,15 @@ fiber_top_enable()
 		cord()->clock_acc = 0;
 		cord()->cpu_miss_count_last = 0;
 		cord()->clock_delta_last = 0;
+		cord()->clock_delta = 0;
 		struct timespec ts;
 		if (clock_gettime(CLOCK_THREAD_CPUTIME_ID, &ts) != 0) {
 			say_debug("clock_gettime(): failed to get this"
 				  "thread's cpu time.");
 			return;
 		}
+		cord()->clock_last = __rdtscp(&cord()->cpu_id_last);
+		cord()->cpu_miss_count = 0;
 		cord()->cputime_last = (uint64_t) ts.tv_sec * FIBER_TIME_RES +
 						  ts.tv_nsec;
 	}
-- 
2.21.0 (Apple Git-122)

[Tarantool-patches] [PATCH 2/2] app/fiber: wait till a full event loop iteration ends.

[Serge Petrenko]

fiber.top() fills in statistics every event loop iteration,
so if it was just enabled, fiber.top() may contain 'inf's and
'nan's in fiber cpu usage averages because total time consumed by
the main thread was not yet accounted for.
Same stands for viewing top() results for a freshly created fiber:
its metrics will be zero since it hasn't lived a full ev loop iteration
yet.
Fix this by delaying the test till top() results are meaningful and add
minor refactoring.

Follow-up #2694
---
 test/app/fiber.result   | 35 ++++++++++++++++++++++++++---------
 test/app/fiber.test.lua | 31 +++++++++++++++++++++++--------
 2 files changed, 49 insertions(+), 17 deletions(-)

diff --git a/test/app/fiber.result b/test/app/fiber.result
index 4a094939f..d447a36fc 100644
--- a/test/app/fiber.result
+++ b/test/app/fiber.result
@@ -1469,6 +1469,19 @@ sum = 0
 fiber.top_enable()
 ---
 ...
+-- Check that a number is finite.
+-- Lua doesn't have this, sorry.
+function finite(num)\
+    return type(num) == 'number' and num < math.huge and\
+           num > -math.huge and tostring(num) ~= 'nan'\
+end
+---
+...
+-- Wait till a full event loop iteration passes, so that
+-- top() contains meaningful results.
+while not finite(fiber.top().cpu["1/sched"].instant) do  fiber.yield() end
+---
+...
 a = fiber.top()
 ---
 ...
@@ -1504,9 +1517,9 @@ for k, v in pairs(a) do\
 end
 ---
 ...
-sum_inst
+sum_inst > 99 and sum_inst < 101 or sum_inst
 ---
-- 100
+- true
 ...
 -- not exact due to accumulated integer division errors
 sum_avg > 99 and sum_avg < 101 or sum_avg
@@ -1517,24 +1530,28 @@ tbl = nil
 ---
 ...
 f = fiber.new(function()\
-    for i = 1,1000 do end\
-    fiber.yield()\
-    tbl = fiber.top().cpu[fiber.self().id()..'/'..fiber.self().name()]\
+    local fiber_key = fiber.self().id()..'/'..fiber.self().name()\
+    tbl = fiber.top().cpu[fiber_key]\
+    while tbl.time == 0 do\
+        for i = 1,1000 do end\
+        fiber.yield()\
+        tbl = fiber.top().cpu[fiber_key]\
+    end\
 end)
 ---
 ...
-while f:status() ~= 'dead' do fiber.sleep(0.01) end
+while f:status() ~= 'dead' do fiber.yield() end
 ---
 ...
-tbl["average"] > 0
+tbl.average > 0
 ---
 - true
 ...
-tbl["instant"] > 0
+tbl.instant > 0
 ---
 - true
 ...
-tbl["time"] > 0
+tbl.time > 0
 ---
 - true
 ...
diff --git a/test/app/fiber.test.lua b/test/app/fiber.test.lua
index 38b85d554..33ebe063f 100644
--- a/test/app/fiber.test.lua
+++ b/test/app/fiber.test.lua
@@ -634,6 +634,17 @@ sum = 0
 -- gh-2694 fiber.top()
 fiber.top_enable()
 
+-- Check that a number is finite.
+-- Lua doesn't have this, sorry.
+function finite(num)\
+    return type(num) == 'number' and num < math.huge and\
+           num > -math.huge and tostring(num) ~= 'nan'\
+end
+
+-- Wait till a full event loop iteration passes, so that
+-- top() contains meaningful results.
+while not finite(fiber.top().cpu["1/sched"].instant) do  fiber.yield() end
+
 a = fiber.top()
 type(a)
 -- scheduler is present in fiber.top()
@@ -652,19 +663,23 @@ for k, v in pairs(a) do\
     sum_avg = sum_avg + v["average"]\
 end
 
-sum_inst
+sum_inst > 99 and sum_inst < 101 or sum_inst
 -- not exact due to accumulated integer division errors
 sum_avg > 99 and sum_avg < 101 or sum_avg
 tbl = nil
 f = fiber.new(function()\
-    for i = 1,1000 do end\
-    fiber.yield()\
-    tbl = fiber.top().cpu[fiber.self().id()..'/'..fiber.self().name()]\
+    local fiber_key = fiber.self().id()..'/'..fiber.self().name()\
+    tbl = fiber.top().cpu[fiber_key]\
+    while tbl.time == 0 do\
+        for i = 1,1000 do end\
+        fiber.yield()\
+        tbl = fiber.top().cpu[fiber_key]\
+    end\
 end)
-while f:status() ~= 'dead' do fiber.sleep(0.01) end
-tbl["average"] > 0
-tbl["instant"] > 0
-tbl["time"] > 0
+while f:status() ~= 'dead' do fiber.yield() end
+tbl.average > 0
+tbl.instant > 0
+tbl.time > 0
 
 fiber.top_disable()
 fiber.top()
-- 
2.21.0 (Apple Git-122)

Re: [Tarantool-patches] [PATCH 2/2] app/fiber: wait till a full event loop iteration ends.

[Vladislav Shpilevoy]

Hi! Thanks for the path!

We don't use dots in commit title. I guess this is a typo.

On 13/11/2019 19:04, Serge Petrenko wrote:
> fiber.top() fills in statistics every event loop iteration,
> so if it was just enabled, fiber.top() may contain 'inf's and
> 'nan's in fiber cpu usage averages because total time consumed by
> the main thread was not yet accounted for.
> Same stands for viewing top() results for a freshly created fiber:
> its metrics will be zero since it hasn't lived a full ev loop iteration
> yet.
> Fix this by delaying the test till top() results are meaningful and add
> minor refactoring.
> 
> Follow-up #2694
> ---
>  test/app/fiber.result   | 35 ++++++++++++++++++++++++++---------
>  test/app/fiber.test.lua | 31 +++++++++++++++++++++++--------
>  2 files changed, 49 insertions(+), 17 deletions(-)
> 
> diff --git a/test/app/fiber.result b/test/app/fiber.result
> index 4a094939f..d447a36fc 100644
> --- a/test/app/fiber.result
> +++ b/test/app/fiber.result
> @@ -1469,6 +1469,19 @@ sum = 0
>  fiber.top_enable()
>  ---
>  ...
> +-- Check that a number is finite.
> +-- Lua doesn't have this, sorry.
> +function finite(num)\
> +    return type(num) == 'number' and num < math.huge and\
> +           num > -math.huge and tostring(num) ~= 'nan'\

1. A canonical way to check for NaN is compare a value with
itself. If they are not equal, then it is NaN.

But more important questions are:
- How can a number from top() have a not 'number' type?
- How can top() contain a NaN, and an infinite value?

> +end
> +---
> +...
> +-- Wait till a full event loop iteration passes, so that
> +-- top() contains meaningful results.
> +while not finite(fiber.top().cpu["1/sched"].instant) do  fiber.yield() end

2. Double whitespace after 'do'.

Re: [Tarantool-patches] [PATCH 2/2] app/fiber: wait till a full event loop iteration ends.

[Sergey Petrenko]

Hi! Thank you for review!

>Четверг, 14 ноября 2019, 1:45 +03:00 от Vladislav Shpilevoy <v.shpilevoy@tarantool.org>:
>
>Hi! Thanks for the path!
>
>We don't use dots in commit title. I guess this is a typo. 

Fixed.

>
>
>On 13/11/2019 19:04, Serge Petrenko wrote:
>> fiber.top() fills in statistics every event loop iteration,
>> so if it was just enabled, fiber.top() may contain 'inf's and
>> 'nan's in fiber cpu usage averages because total time consumed by
>> the main thread was not yet accounted for.
>> Same stands for viewing top() results for a freshly created fiber:
>> its metrics will be zero since it hasn't lived a full ev loop iteration
>> yet.
>> Fix this by delaying the test till top() results are meaningful and add
>> minor refactoring.
>> 
>> Follow-up #2694
>> ---
>>  test/app/fiber.result   | 35 ++++++++++++++++++++++++++---------
>>  test/app/fiber.test.lua | 31 +++++++++++++++++++++++--------
>>  2 files changed, 49 insertions(+), 17 deletions(-)
>> 
>> diff --git a/test/app/fiber.result b/test/app/fiber.result
>> index 4a094939f..d447a36fc 100644
>> --- a/test/app/fiber.result
>> +++ b/test/app/fiber.result
>> @@ -1469,6 +1469,19 @@ sum = 0
>>  fiber.top_enable()
>>  ---
>>  ...
>> +-- Check that a number is finite.
>> +-- Lua doesn't have this, sorry.
>> +function finite(num)\
>> +    return type(num) == 'number' and num < math.huge and\
>> +           num > -math.huge and tostring(num) ~= 'nan'\
>
>1. A canonical way to check for NaN is compare a value with
>itself. If they are not equal, then it is NaN.

Fixed, thanks!

>
>
>But more important questions are:
>- How can a number from top() have a not 'number' type? 

It can't. I just wanted to implement a caconical is_finite check.
I can remove it, if you want me to.

>
>- How can top() contain a NaN, and an infinite value? 

NaN: you issue fiber.top() on the same iteration you called
fiber.top_enable(). cord()->clock_delta_last and fiber()->clock_delta_last
both are 0, because clock_delta_last contains data from a previous ev loop
iteration. Division gives you NaN.

>
>
>> +end
>> +---
>> +...
>> +-- Wait till a full event loop iteration passes, so that
>> +-- top() contains meaningful results.
>> +while not finite(fiber.top().cpu["1/sched"].instant) do  fiber.yield() end
>
>2. Double whitespace after 'do'.

Fixed, thanks.

Incremental diff follows.

diff --git a/test/app/fiber.result b/test/app/fiber.result
index d447a36fc..8eb506bd5 100644
--- a/test/app/fiber.result
+++ b/test/app/fiber.result
@@ -1469,17 +1469,14 @@ sum = 0
 fiber.top_enable()
 ---
 ...
--- Check that a number is finite.
--- Lua doesn't have this, sorry.
-function finite(num)\
-    return type(num) == 'number' and num < math.huge and\
-           num > -math.huge and tostring(num) ~= 'nan'\
+function isnan(num)\
+    return num ~= num\
 end
 ---
 ...
 -- Wait till a full event loop iteration passes, so that
 -- top() contains meaningful results.
-while not finite(fiber.top().cpu["1/sched"].instant) do  fiber.yield() end
+while isnan(fiber.top().cpu["1/sched"].instant) do fiber.yield() end
 ---
 ...
 a = fiber.top()
diff --git a/test/app/fiber.test.lua b/test/app/fiber.test.lua
index 33ebe063f..24cb8b492 100644
--- a/test/app/fiber.test.lua
+++ b/test/app/fiber.test.lua
@@ -634,16 +634,13 @@ sum = 0
 -- gh-2694 fiber.top()
 fiber.top_enable()
 
--- Check that a number is finite.
--- Lua doesn't have this, sorry.
-function finite(num)\
-    return type(num) == 'number' and num < math.huge and\
-           num > -math.huge and tostring(num) ~= 'nan'\
+function isnan(num)\
+    return num ~= num\
 end
 
 -- Wait till a full event loop iteration passes, so that
 -- top() contains meaningful results.
-while not finite(fiber.top().cpu["1/sched"].instant) do  fiber.yield() end
+while isnan(fiber.top().cpu["1/sched"].instant) do fiber.yield() end
 
 a = fiber.top()
 type(a)
 


Regards,
Sergey Petrenko

Re: [Tarantool-patches] [PATCH 2/2] app/fiber: wait till a full event loop iteration ends.

[Vladislav Shpilevoy]

Hi! Thanks for the fixes!

>> But more important questions are:
>> - How can a number from top() have a not 'number' type? 
> 
> It can't. I just wanted to implement a caconical is_finite check.
> I can remove it, if you want me to.
> 
>>
>> - How can top() contain a NaN, and an infinite value? 
> 
> NaN: you issue fiber.top() on the same iteration you called
> fiber.top_enable(). cord()->clock_delta_last and fiber()->clock_delta_last
> both are 0, because clock_delta_last contains data from a previous ev loop
> iteration. Division gives you NaN.

Hm, so a user should be ready that top() can return invalid values?
I think that it may be better to return 0, when cord()->clock_delta_last
is 0.

Re: [Tarantool-patches] [PATCH 2/2] app/fiber: wait till a full event loop iteration ends.

[Serge Petrenko]

Hi! Thank you for you reply!

> 15 нояб. 2019 г., в 0:44, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> написал(а):
> 
> Hi! Thanks for the fixes!
> 
>>> But more important questions are:
>>> - How can a number from top() have a not 'number' type? 
>> 
>> It can't. I just wanted to implement a caconical is_finite check.
>> I can remove it, if you want me to.
>> 
>>> 
>>> - How can top() contain a NaN, and an infinite value? 
>> 
>> NaN: you issue fiber.top() on the same iteration you called
>> fiber.top_enable(). cord()->clock_delta_last and fiber()->clock_delta_last
>> both are 0, because clock_delta_last contains data from a previous ev loop
>> iteration. Division gives you NaN.
> 
> Hm, so a user should be ready that top() can return invalid values?
> I think that it may be better to return 0, when cord()->clock_delta_last
> is 0.

Seems reasonable. I fixed it in the first patch, added some more cleanups
there and resent the whole series as v2.

--
Serge Petrenko
sergepetrenko@tarantool.org