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

[Serge Petrenko <sergepetrenko@tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 3836 bytes --]

Hi! Thankyou for reviewing this.

I’ve managed to finally find the cause of test failures.
It is fixed in the second patch of the newly sent series.
I addressed your comments and resent v3.

--
Serge Petrenko
sergepetrenko@tarantool.org




> 16 нояб. 2019 г., в 0:39, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> написал(а):
> 
> Hi! Thanks for the fixes!
> 
> See 2 comments below.
> 
> On 15/11/2019 15:58, Serge Petrenko wrote:
>> We didn't refresh last remembered clock on fiber.top_enable()
>> This means that the fiber issuing fiber.top_enable() would get a huge
>> chunk of cpu time on its first yield. Fix this.
>> Also reset clock_delta and cpu_miss_count.
>> 
>> If fiber.top() is issued on the same ev loop iteration as
>> fiber.top_enable(), clock_delta_last is 0 for both cord and all the
>> fibers, so report "instant" and "average" stats per last iteration as 0
>> instead of NaN.
>> 
>> Follow-up #2694
>> ---
>> src/lib/core/fiber.c | 21 +++++++++++++++++++++
>> src/lua/fiber.c      | 12 ++++++++++--
>> 2 files changed, 31 insertions(+), 2 deletions(-)
>> 
>> diff --git a/src/lib/core/fiber.c b/src/lib/core/fiber.c
>> index aebaba7f0..258c094f5 100644
>> --- a/src/lib/core/fiber.c
>> +++ b/src/lib/core/fiber.c
>> @@ -1203,9 +1203,30 @@ fiber_top_enable()
>> 		ev_check_start(cord()->loop, &cord()->check_event);
>> 		fiber_top_enabled = true;
>> 
>> +		/*
>> +		 * Reset cord and fiber clock stats in order to
>> +		 * count from zero even on reenable.
>> +		 */
>> 		cord()->clock_acc = 0;
>> 		cord()->cpu_miss_count_last = 0;
>> 		cord()->clock_delta_last = 0;
>> +		cord()->clock_delta = 0;
>> +
>> +		struct fiber * fiber;
>> +		rlist_foreach_entry(fiber, &cord()->alive, link) {
>> +			fiber->clock_acc = 0;
>> +			fiber->clock_delta_last = 0;
>> +			fiber->clock_delta = 0;
>> +			fiber->cputime = 0;
> 
> 1. fiber_reset() should do exactly the same, right?
> Then why doesn't it nullify clock_delta_last?

I just didn’t notice that, sorry.

> 
> This mess with numerous time fields, and their reset,
> looks like a necessity to create a new structure,
> which would be included into cord and fiber structures.
> And which would provide methods for cleaning and
> updating the members.

Done.

> 
>> +		}
>> +
>> +		cord()->sched.clock_acc = 0;
>> +		cord()->sched.clock_delta_last = 0;
>> +		cord()->sched.clock_delta = 0;
>> +		cord()->sched.cputime = 0;
>> +
>> +		cord()->clock_last = __rdtscp(&cord()->cpu_id_last);
>> +		cord()->cpu_miss_count = 0;
>> 		struct timespec ts;
>> 		if (clock_gettime(CLOCK_THREAD_CPUTIME_ID, &ts) != 0) {
>> 			say_debug("clock_gettime(): failed to get this"
>> diff --git a/src/lua/fiber.c b/src/lua/fiber.c
>> index 8b3b22e55..647505643 100644
>> --- a/src/lua/fiber.c
>> +++ b/src/lua/fiber.c
>> @@ -335,11 +335,19 @@ lbox_fiber_top_entry(struct fiber *f, void *cb_ctx)
>> 	lua_newtable(L);
>> 
>> 	lua_pushliteral(L, "average");
>> -	lua_pushnumber(L, f->clock_acc / (double)cord()->clock_acc * 100);
>> +	if (cord()->clock_acc)
> 
> 2. Sorry for a nit, we usually compare with 0
> explicitly, '!= 0', and avoid implicit casts.

No problem. Fixed.

> 
>> +		lua_pushnumber(L, f->clock_acc / (double)cord()->clock_acc * 100);
>> +	else
>> +		lua_pushnumber(L, 0);
>> 	lua_settable(L, -3);
>> +
>> 	lua_pushliteral(L, "instant");
>> -	lua_pushnumber(L, f->clock_delta_last / (double)cord()->clock_delta_last * 100);
>> +	if (cord()->clock_delta_last)
>> +		lua_pushnumber(L, f->clock_delta_last / (double)cord()->clock_delta_last * 100);
>> +	else
>> +		lua_pushnumber(L, 0);
>> 	lua_settable(L, -3);
>> +
>> 	lua_pushliteral(L, "time");
>> 	lua_pushnumber(L, f->cputime / (double) FIBER_TIME_RES);
>> 	lua_settable(L, -3);


[-- Attachment #2: Type: text/html, Size: 23177 bytes --]