[PATCH] Deprecate current-milliseconds in favor of current-process-milliseconds and improve behaviour a bit [was: Re: Exposing subsecond precision in current-seconds]

2020-05-06 Thread felix . winkelmann
> On Thu, Apr 30, 2020 at 12:11:31PM +0200, felix.winkelm...@bevuta.com wrote:
> > > Yeah, like I said in my other message, we could use the name
> > > current-process-milliseconds, like Racket;
> > > https://docs.racket-lang.org/reference/time.html
> > >
> > > I like the name, because it's pretty clear what it does; it yields the
> > > number of milliseconds the process has been running for.
> > >
> > > We can make it a procedure without arguments, because that part of the
> > > Racket interface looks messy and confusing to me.
> >
> > Yes, makes sense.
>
> Attached is a set of two patches.  The first one simply adds a
> deprecation notice to current-milliseconds and adds the new procedure
> current-process-milliseconds.
>

Thanks - pushed. I have taken the freedom to add comments in 
chicken.time.import.scm
and library.scm indicating listed identifiers are deprecated.


felix





[PATCH] Deprecate current-milliseconds in favor of current-process-milliseconds and improve behaviour a bit [was: Re: Exposing subsecond precision in current-seconds]

2020-05-03 Thread Peter Bex
On Thu, Apr 30, 2020 at 12:11:31PM +0200, felix.winkelm...@bevuta.com wrote:
> > Yeah, like I said in my other message, we could use the name
> > current-process-milliseconds, like Racket;
> > https://docs.racket-lang.org/reference/time.html
> >
> > I like the name, because it's pretty clear what it does; it yields the
> > number of milliseconds the process has been running for.
> >
> > We can make it a procedure without arguments, because that part of the
> > Racket interface looks messy and confusing to me.
> 
> Yes, makes sense.

Attached is a set of two patches.  The first one simply adds a
deprecation notice to current-milliseconds and adds the new procedure
current-process-milliseconds.

While I was looking into this, I've noticed a few strange things.
First off, on Unix, C_startup_time_seconds uses just the second since
startup, ignoring the microseconds.  This leads to erratic behaviour:

$ csi -e -R chicken.time -p '(current-process-milliseconds)'
301
$ csi -e -R chicken.time -p '(current-process-milliseconds)'
910
$ csi -e -R chicken.time -p '(current-process-milliseconds)'
82
$ csi -e -R chicken.time -p '(current-process-milliseconds)'
269
$ csi -e -R chicken.time -p '(current-process-milliseconds)'
905
$ csi -e -R chicken.time -p '(current-process-milliseconds)'
15

With the patch, we get sane behaviour:
$ csi -e -R chicken.time -p '(current-process-milliseconds)'
23
$ csi -e -R chicken.time -p '(current-process-milliseconds)'
24
$ csi -e -R chicken.time -p '(current-process-milliseconds)'
26
$ csi -e -R chicken.time -p '(current-process-milliseconds)'
24

On Windows, we used to use clock(), but according to
https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/clock?view=vs-2019
that function stops behaving sanely after 24.8 days of uptime,
at which point it starts returning -1.  This shouldn't usually
be a problem, but on a server running some long-running CHICKEN
process this could be an issue.

Besides, that also says CLOCKS_PER_SEC is defined as 1000 by
Microsoft.  So the logic in there doesn't make much sense since
it's defined as such, and C_NONUNIX is only true on MINGW32.

And if that code were triggered on non-Windows non-UNIX systems,
that would result in this undefined issue where the time since
system startup might be used.  To me it makes a lot more sense
to define current-process-milliseconds as time since process
startup, and drop the wishy-washy documentation that it's
time "since process or system startup".

Cheers,
Peter
From a53e6a707e34fa26f42a110b8948ef9710c0d86e Mon Sep 17 00:00:00 2001
From: Peter Bex 
Date: Sun, 3 May 2020 12:15:08 +0200
Subject: [PATCH 1/2] Deprecate current-milliseconds in favor of
 current-process-milliseconds

For consistency, a similar deprecation is also made for the underlying
C API.
---
 DEPRECATED   |  6 ++
 NEWS | 11 +--
 batch-driver.scm |  2 +-
 chicken.h|  5 -
 chicken.time.import.scm  |  1 +
 library.scm  |  6 +-
 manual/Module (chicken time) |  4 ++--
 runtime.c|  6 ++
 scheduler.scm|  6 +++---
 tcp.scm  | 10 +-
 tests/loopy-test.scm |  6 +++---
 tests/test.scm   | 10 +-
 types.db |  3 ++-
 13 files changed, 52 insertions(+), 24 deletions(-)

diff --git a/DEPRECATED b/DEPRECATED
index 40756ce2..8f25faa8 100644
--- a/DEPRECATED
+++ b/DEPRECATED
@@ -1,6 +1,12 @@
 Deprecated functions and variables
 ==
 
+5.2.1
+- current-milliseconds and its C implementations C_milliseconds and
+  C_a_i_current_milliseconds have been deprecated in favor of
+  current-process_milliseconds, C_current_process_milliseconds and
+  C_a_i_current_process_milliseconds
+
 5.1.1
 
 - ##sys#check-exact and its C implementations C_i_check_exact and
diff --git a/NEWS b/NEWS
index 825fa8ab..67cc9f55 100644
--- a/NEWS
+++ b/NEWS
@@ -4,13 +4,20 @@
   - Fixed a bug where optimisations for `irregex-match?` would cause
 runtime errors due to the inlined specialisations not being
 fully-expanded (see #1690).
+  - current-milliseconds has been deprecated in favor of the name
+current-process-milliseconds, to avoid confusion due to naming
+of current-milliseconds versus current-seconds, which do something
+quite different.
 
 - Runtime system
   - Sleeping primordial thread doesn't forget mutations made to
 parameters in interrupt handlers anymore. (See #1638. Fix
 contributed by Sebastien Marie)
-- A feature corresponding to the word size is available
-   regardless of the word size (#1693)
+  - A feature corresponding to the word size is available
+regardless of the word size (#1693)
+  - Deprecated C_(a_i_current_)milliseconds in favor of
+C_(a_i_)current_process_milliseconds to match the Scheme-level
+deprecation of current-milliseconds.
 
 - Build system
   -