Re: [Tarantool-patches] [PATCH v3 1/3] fiber.top() refactor clock and cpu time calculation

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hi! Thanks for the fixes!

I suggest you to move all functions related to cpu and clock
state to fiber.c and make them static there. In order not to
pollute the global namespace with fiber's private functions.

I did that in a commit above this one. You may squash, or redo,
whatever:

===================================================================

diff --git a/src/lib/core/fiber.c b/src/lib/core/fiber.c
index 1e08d0ec9..3faa198d3 100644
--- a/src/lib/core/fiber.c
+++ b/src/lib/core/fiber.c
@@ -44,6 +44,112 @@
 
 #if ENABLE_FIBER_TOP
 #include <x86intrin.h> /* __rdtscp() */
+
+static inline void
+clock_stat_add_delta(struct clock_stat *stat, uint64_t clock_delta)
+{
+	stat->delta += clock_delta;
+}
+
+/**
+ * Calculate the exponential moving average for the clock deltas
+ * per loop iteration. The coeffitient is 1/16.
+ */
+static inline uint64_t
+clock_diff_accumulate(uint64_t acc, uint64_t delta)
+{
+	if (acc > 0)
+		return delta / 16 + 15 * acc / 16;
+	else
+		return delta;
+}
+
+static inline void
+clock_stat_update(struct clock_stat *stat, double nsec_per_clock)
+{
+	stat->acc = clock_diff_accumulate(stat->acc, stat->delta);
+	stat->prev_delta = stat->delta;
+	stat->cputime += stat->delta * nsec_per_clock;
+	stat->delta = 0;
+}
+
+static inline void
+clock_stat_reset(struct clock_stat *stat)
+{
+	stat->acc = 0;
+	stat->delta = 0;
+	stat->prev_delta = 0;
+	stat->cputime = 0;
+}
+
+static void
+cpu_stat_start(struct cpu_stat *stat)
+{
+	stat->prev_clock = __rdtscp(&stat->prev_cpu_id);
+	stat->cpu_miss_count = 0;
+	/*
+	 * We want to measure thread cpu time here to calculate
+	 * each fiber's cpu time, so don't use libev's ev_now() or
+	 * ev_time() since they use either monotonic or realtime
+	 * system clocks.
+	 */
+	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;
+	}
+	stat->prev_cputime = (uint64_t) ts.tv_sec * FIBER_TIME_RES + ts.tv_nsec;
+}
+
+static inline void
+cpu_stat_reset(struct cpu_stat *stat)
+{
+	stat->prev_cpu_miss_count = 0;
+	cpu_stat_start(stat);
+}
+
+static uint64_t
+cpu_stat_on_csw(struct cpu_stat *stat)
+{
+	uint32_t cpu_id;
+	uint64_t delta, clock = __rdtscp(&cpu_id);
+
+	if (cpu_id == stat->prev_cpu_id) {
+		delta = clock - stat->prev_clock;
+	} else {
+		delta = 0;
+		stat->prev_cpu_id = cpu_id;
+		stat->cpu_miss_count++;
+	}
+	stat->prev_clock = clock;
+
+	return delta;
+}
+
+static double
+cpu_stat_end(struct cpu_stat *stat, struct clock_stat *cord_clock_stat)
+{
+	stat->prev_cpu_miss_count = stat->cpu_miss_count;
+	stat->cpu_miss_count = 0;
+
+	struct timespec ts;
+	uint64_t delta_time;
+	double nsec_per_clock = 0;
+	if (clock_gettime(CLOCK_THREAD_CPUTIME_ID, &ts) != 0) {
+		say_debug("clock_gettime(): failed to get this thread's"
+			  " cpu time.");
+	} else {
+		delta_time = (uint64_t) ts.tv_sec * FIBER_TIME_RES +
+			     ts.tv_nsec;
+		if (delta_time > stat->prev_cputime && cord_clock_stat->delta > 0) {
+			delta_time -= stat->prev_cputime;
+			nsec_per_clock = (double) delta_time / cord()->clock_stat.delta;
+		}
+	}
+	return nsec_per_clock;
+}
+
 #endif /* ENABLE_FIBER_TOP */
 
 #include "third_party/valgrind/memcheck.h"
@@ -88,9 +194,6 @@ static int (*fiber_invoke)(fiber_func f, va_list ap);
 
 #if ENABLE_FIBER_TOP
 static __thread bool fiber_top_enabled = false;
-
-uint64_t
-cpu_stat_on_csw(struct cpu_stat *stat);
 #endif /* ENABLE_FIBER_TOP */
 
 /**
@@ -1091,112 +1194,6 @@ loop_on_iteration_start(ev_loop *loop, ev_check *watcher, int revents)
 	cpu_stat_start(&cord()->cpu_stat);
 }
 
-/**
- * Calculate the exponential moving average for the clock deltas
- * per loop iteration. The coeffitient is 1/16.
- */
-static inline uint64_t
-clock_diff_accumulate(uint64_t acc, uint64_t delta)
-{
-	if (acc > 0) {
-		return delta / 16 + 15 * acc / 16;
-	} else {
-		return delta;
-	}
-}
-
-inline void
-clock_stat_add_delta(struct clock_stat *stat, uint64_t clock_delta)
-{
-	stat->delta += clock_delta;
-}
-
-void
-clock_stat_update(struct clock_stat *stat, double nsec_per_clock)
-{
-	stat->acc = clock_diff_accumulate(stat->acc, stat->delta);
-	stat->prev_delta = stat->delta;
-	stat->cputime += stat->delta * nsec_per_clock;
-	stat->delta = 0;
-}
-
-void
-clock_stat_reset(struct clock_stat *stat)
-{
-	stat->acc = 0;
-	stat->delta = 0;
-	stat->prev_delta = 0;
-	stat->cputime = 0;
-}
-
-void
-cpu_stat_start(struct cpu_stat *stat)
-{
-	stat->prev_clock = __rdtscp(&stat->prev_cpu_id);
-	stat->cpu_miss_count = 0;
-	/*
-	 * We want to measure thread cpu time here to calculate
-	 * each fiber's cpu time, so don't use libev's ev_now() or
-	 * ev_time() since they use either monotonic or realtime
-	 * system clocks.
-	 */
-	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;
-	}
-	stat->prev_cputime = (uint64_t) ts.tv_sec * FIBER_TIME_RES + ts.tv_nsec;
-}
-
-void
-cpu_stat_reset(struct cpu_stat *stat)
-{
-	stat->prev_cpu_miss_count = 0;
-	cpu_stat_start(stat);
-}
-
-uint64_t
-cpu_stat_on_csw(struct cpu_stat *stat)
-{
-	uint32_t cpu_id;
-	uint64_t delta = 0;
-	uint64_t clock = __rdtscp(&cpu_id);
-
-	if (cpu_id == stat->prev_cpu_id) {
-		delta = clock - stat->prev_clock;
-	} else {
-		stat->prev_cpu_id = cpu_id;
-		stat->cpu_miss_count++;
-	}
-	stat->prev_clock = clock;
-
-	return delta;
-}
-
-double
-cpu_stat_end(struct cpu_stat *stat, struct clock_stat *cord_clock_stat)
-{
-	stat->prev_cpu_miss_count = stat->cpu_miss_count;
-	stat->cpu_miss_count = 0;
-
-	struct timespec ts;
-	uint64_t delta_time;
-	double nsec_per_clock = 0;
-	if (clock_gettime(CLOCK_THREAD_CPUTIME_ID, &ts) != 0) {
-		say_debug("clock_gettime(): failed to get this thread's"
-			  " cpu time.");
-	} else {
-		delta_time = (uint64_t) ts.tv_sec * FIBER_TIME_RES +
-			     ts.tv_nsec;
-		if (delta_time > stat->prev_cputime && cord_clock_stat->delta > 0) {
-			delta_time -= stat->prev_cputime;
-			nsec_per_clock = (double) delta_time / cord()->clock_stat.delta;
-		}
-	}
-	return nsec_per_clock;
-}
-
 static void
 loop_on_iteration_end(ev_loop *loop, ev_prepare *watcher, int revents)
 {
diff --git a/src/lib/core/fiber.h b/src/lib/core/fiber.h
index 06ce28bb1..c5b975513 100644
--- a/src/lib/core/fiber.h
+++ b/src/lib/core/fiber.h
@@ -91,15 +91,6 @@ struct clock_stat {
 	uint64_t cputime;
 };
 
-void
-clock_stat_add_delta(struct clock_stat *stat, uint64_t clock_delta);
-
-void
-clock_stat_update(struct clock_stat *stat, double nsec_per_clock);
-
-void
-clock_stat_reset(struct clock_stat *stat);
-
 /**
  * A struct encapsulating all knowledge this cord has about cpu
  * clocks and their state.
@@ -119,18 +110,6 @@ struct cpu_stat {
 	uint32_t prev_cpu_miss_count;
 };
 
-void
-cpu_stat_start(struct cpu_stat *stat);
-
-void
-cpu_stat_reset(struct cpu_stat *stat);
-
-uint64_t
-cpu_stat_on_csw(struct cpu_stat *stat);
-
-double
-cpu_stat_end(struct cpu_stat *stat, struct clock_stat *cord_clock_stat);
-
 #endif /* ENABLE_FIBER_TOP */
 
 enum { FIBER_NAME_MAX = 32 };