Add clock sources to os/clock (:realtime, :monotonic, :cputime)

[zevv]

instead of CLOCK_REALTIME, this matches the description from the documentation of os/clock. Fixes issue #1144

[zevv]

• (os/clock) suggests this function might be related to the POSIX clock() function which measures CPU time used, and is useful for profiling - see below.

• Changing this function to use CLOCK_MONOTONIC will take a way the only method for getting the current clock time at high resolution though, which is a shame.

Possible solution: We could add an optional argument maybe to select the clock source (:monotonic :realtime, :cputime etc), or add a new function for providing high resolution time?

[sogaiu]

Another factor to consider is that os/clock has been used in a number of projects [1] for measuring performance. Perhaps whatever changes are made, the behavior of the no argument version of os/clock should remain the same.

---

[1] I grepped across the Janet code I've collected and I see a fair number of start-clock, start, delta, sorts of results involving os/clock.

[zevv]

for performance measurements, the semantics of os/clock being real-time or monotonic does not change. I do agree that changing the behavior of a function is not without risk, as other peoples code might already depend on the fact that the current implementation returns UNIX epoch time.

[zevv]

Added source argument to (os/clock), and implemented this for POSIX. I'll try to get up with implementations for windows and mac/BSD, but since I don't have any of those running I have limited means for testing.

[sogaiu]

I can do some Windows testing.

[sogaiu]

About to start testing, but wanted to share the build messages on Windows first.

C:\Users\user\Desktop\janet.clock>build_win.bat
abstract.c
array.c
asm.c
buffer.c
bytecode.c
capi.c
cfuns.c
compile.c
corelib.c
debug.c
emit.c
ev.c
ffi.c
fiber.c
gc.c
inttypes.c
io.c
marsh.c
math.c
net.c
src\core\net.c(956): warning C4996: 'inet_addr': Use inet_pton() or InetPton() instead or define _WINSOCK_DEPRECATED_NO_WARNINGS to disable deprecated API warnings
os.c
parse.c
peg.c
pp.c
regalloc.c
run.c
specials.c
state.c
string.c
strtod.c
struct.c
symcache.c
table.c
tuple.c
util.c
src\core\util.c(895): warning C4244: '=': conversion from 'LONGLONG' to 'long', possible loss of data
src\core\util.c(898): warning C4244: '=': conversion from 'float' to 'time_t', possible loss of data
src\core\util.c(899): warning C4244: '=': conversion from 'double' to 'long', possible loss of data
src\core\util.c(897): warning C4244: 'initializing': conversion from 'clock_t' to 'float', possible loss of data
value.c
vector.c
vm.c
wrap.c
array_test.c
boot.c
buffer_test.c
number_test.c
system_test.c
table_test.c
   Creating library build\janet_boot.lib and object build\janet_boot.exp
janet.c
src/core/net.c(956): warning C4996: 'inet_addr': Use inet_pton() or InetPton() instead or define _WINSOCK_DEPRECATED_NO_WARNINGS to disable deprecated API warnings
src/core/util.c(895): warning C4244: '=': conversion from 'LONGLONG' to 'long', possible loss of data
src/core/util.c(898): warning C4244: '=': conversion from 'float' to 'time_t', possible loss of data
src/core/util.c(899): warning C4244: '=': conversion from 'double' to 'long', possible loss of data
src/core/util.c(897): warning C4244: 'initializing': conversion from 'clock_t' to 'float', possible loss of data
shell.c
   Creating library janet.lib and object janet.exp
=== Successfully built janet.exe for Windows ===
=== Run 'build_win test' to run tests. ==
=== Run 'build_win clean' to delete build artifacts. ===

[sogaiu]

The following is very basic testing, but FWIW:

PS C:\Users\user\Desktop\janet.clock> ansicon
Microsoft Windows [Version 10.0.19045.2965]
(c) Microsoft Corporation. All rights reserved.

C:\Users\user\Desktop\janet.clock>.\janet.exe
Janet 1.28.0-dev-local windows/x64/msvc - '(doc)' for help
repl:1:> (os/clock)
1.68458e+09
repl:2:> (os/clock :realtime)
1.68458e+09
repl:3:> (os/clock :monotonic)
2219.12
repl:4:> (os/clock :cputime)
18.979

[zevv]

Added tests validating the different clock times, takes a few hundred ms to run in CI though.

[zevv]

About to start testing, but wanted to share the build messages on Windows first.

Right; if you can make the time, would you be able to make the MSVC compiler shut up about those with some proper casts? mingw is happy about that, and the only way to test this with the MSVC compiler for me is to go through CI every time.

[sogaiu]

I'll take a look.

[zevv]

thanks for that. I'm not really sure though if this is doing the right thing, because now we end up with os/time and os/clock, which do similar-but-not-quite-the-same things...

[sogaiu]

Ah...well, perhaps bakpakin will have an opinion :)

Below is a diff of some tweaks to get the remaining 2 messages to go away when using VS2022's C compiler. Not really sure if the changes are good -- though the tests did pass.

For reference, for the net.c changes, chose InetPton() over silencing the warning as allegedly there are thread-safety issues with inet_addr on Windows -- at least according to a comment here.

diff --git a/src/core/net.c b/src/core/net.c
index e628bce1..3f469bc8 100644
--- a/src/core/net.c
+++ b/src/core/net.c
@@ -953,7 +953,11 @@ JANET_CORE_FN(cfun_net_setsockopt,
         const char *addr = janet_getcstring(argv, 2);
         memset(&val.v_mreq, 0, sizeof val.v_mreq);
         val.v_mreq.imr_interface.s_addr = htonl(INADDR_ANY);
+#ifdef JANET_WINDOWS
+       InetPton(AF_INET, (PCSTR)addr, &val.v_mreq.imr_multiaddr.s_addr);
+#else
         val.v_mreq.imr_multiaddr.s_addr = inet_addr(addr);
+#endif
         optlen = sizeof(val.v_mreq);
     } else if (st->optname == IPV6_JOIN_GROUP || st->optname == IPV6_LEAVE_GROUP) {
         const char *addr = janet_getcstring(argv, 2);
diff --git a/src/core/util.c b/src/core/util.c
index 8d14aeb8..362bc8c7 100644
--- a/src/core/util.c
+++ b/src/core/util.c
@@ -892,7 +892,8 @@ int janet_gettime(struct timespec *spec, enum JanetTimeSource source) {
         QueryPerformanceCounter(&count);
         QueryPerformanceFrequency(&perf_freq);
         spec->tv_sec = count.QuadPart / perf_freq.QuadPart;
-        spec->tv_nsec = (count.QuadPart % perf_freq.QuadPart) * 1000000000 / perf_freq.QuadPart;
+        spec->tv_nsec =
+           (long)((count.QuadPart % perf_freq.QuadPart) * 1000000000 / perf_freq.QuadPart);
     } else if (source == JANET_TIME_CPUTIME) {
         FILETIME creationTime, exitTime, kernelTime, userTime;
         GetProcessTimes(GetCurrentProcess(), &creationTime, &exitTime, &kernelTime, &userTime);

[zevv]

Thanks for that' I'll apply the newly introduced warning, but not the change in net.c, as this is not related to what this change is doing.

[bakpakin]

So the idea behind os/time vs. os/clock is that clock is used for measuring durations, perf, etc. with high precision. os/time is just meant to to give second precision related to calendar time, such that the argument can be passed to os/date, used as an integer timestamp, etc. I have no issue with os/clock having multiple modes.

[bakpakin]

This may need some work for the BSDs, so it is probably best to stick to posix.

[zevv]

I believe the current implementation is ok for Linux, Windows and Mac, the test actually checks if CPU time increases when spinning but not when sleeping.

If you are ok with it, I think this PR should be good to go as it is now.

[zevv]

(You want it squashed before merging?)

[zevv]

This may need some work for the BSDs, so it is probably best to stick to posix.

Not sure if I understand what you mean, the current #else case should be fine for BSDs I believe?

[bakpakin]

LGTM on bsds: https://builds.sr.ht/~bakpakin/job/993449