Re: [Tarantool-patches] [PATCH v1 luajit 02/41] perf: introduce clock module

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi, Sergey!
Thanks for the review!
Please consider my answers below.

On 11.11.25, Sergey Bronnikov wrote:
> Hi, Sergey!
> 
> thanks for the patch! Please see my comments.
> 
> Sergey
> 
> On 10/24/25 13:50, Sergey Kaplun wrote:
> > This module contains 2 functions:
> > - `realtime()` -- returns the time represented by the wall clock.
> > - `process_cputime()` -- returns the time consumed by all threads of
> >    the process.
> I would rephrase second bullet: "to measure CPU time instead of elapsed 
> time"

Rephrased.

> Also, I would add this description to the Lua module as well.

It is mentioned in the corresponding comment. Or I don't get what you
mean.

> >
> > Both functions are implemented via FFI call to the `clock_gettime()`.
> > ---
> >   perf/utils/clock.lua | 35 +++++++++++++++++++++++++++++++++++
> >   1 file changed, 35 insertions(+)
> >   create mode 100644 perf/utils/clock.lua
> >
> > diff --git a/perf/utils/clock.lua b/perf/utils/clock.lua
> > new file mode 100644
> > index 00000000..57385967
> > --- /dev/null
> > +++ b/perf/utils/clock.lua
> > @@ -0,0 +1,35 @@
> > +local ffi = require('ffi')
> > +
> > +ffi.cdef[[
> > +struct timespec {
> > +  long tv_sec; /* Seconds. */
> > +  long tv_nsec; /* Nanoseconds. */
> > +};
> > +
> > +int clock_gettime(int clockid, struct timespec *tp);
> > +]]
> > +
> > +local C = ffi.C
> > +
> > +-- Wall clock.
> > +local CLOCK_REALTIME = 0
> 
>   This clock is not a reliable source of the time. This clock can be 
> adjusted by
> 
> NTP or manually or by timezones. It is better to use CLOCK_MONOTONIC or
> 
> even CLOCK_MONOTONIC_RAW (not portable, Linux-specific), it is more reliable
> 
> and does not depend on things listed above.

I have no strict opinion on this. On the first hand, monotonic time may
be more reliable. OTOH, Google Benchmark uses realtime. AFAIU, the main
point of using "real-time" is to reflect overall execution duration and
adjust it for the minimum benchmark duration.

Do we need to compare results with MONOTONIC time?
I suppose that there is no way that wall clock will be adjusted during
benchmarks somehow.

> 
> > +-- CPU time consumed by the process.
> > +local CLOCK_PROCESS_CPUTIME_ID = 2
> > +
> > +-- All functions below returns the corresponding `clock_gettime()`
> s/`clock_gettime()`/elapsed time/

Fixed. See the iterative patch below:

===================================================================
diff --git a/perf/utils/clock.lua b/perf/utils/clock.lua
index 57385967..cf708194 100644
--- a/perf/utils/clock.lua
+++ b/perf/utils/clock.lua
@@ -16,8 +16,8 @@ local CLOCK_REALTIME = 0
 -- CPU time consumed by the process.
 local CLOCK_PROCESS_CPUTIME_ID = 2

--- All functions below returns the corresponding `clock_gettime()`
--- in seconds.
+-- All functions below returns the corresponding elapsed time in
+-- seconds.
 local M = {}

 local timespec = ffi.new('struct timespec[1]')
===================================================================

> > +-- in seconds.
> > +local M = {}
> > +
> > +local timespec = ffi.new('struct timespec[1]')
> > +
> > +function M.realtime()
> > +  C.clock_gettime(CLOCK_REALTIME, timespec)
> > +  return tonumber(timespec[0].tv_sec) + tonumber(timespec[0].tv_nsec) / 1e9
> > +end
> > +
> 
> may be it is better to make conversion only once?
> 
> @@ -24,7 +24,7 @@ local timespec = ffi.new('struct timespec[1]')
> 
>   function M.realtime()
>     C.clock_gettime(CLOCK_REALTIME, timespec)
> -  return tonumber(timespec[0].tv_sec) + tonumber(timespec[0].tv_nsec) / 1e9
> +  return tonumber(timespec[0].tv_sec + timespec[0].tv_nsec / 1e9)

In that way we lose the precision of `tv_nsec`. Ignoring.
| luajit -e 'print(400000LL / 1e9)'
| 0LL

>   end
> 
> the same below
> 
> > +function M.process_cputime()
> > +  C.clock_gettime(CLOCK_PROCESS_CPUTIME_ID, timespec)
> > +  return tonumber(timespec[0].tv_sec) + tonumber(timespec[0].tv_nsec) / 1e9
> > +end
> > +
> > +return M

-- 
Best regards,
Sergey Kaplun