Hi Levente,

On Wed, May 06, 2020 at 02:39:28AM +0200, Levente Uzonyi wrote:
> Hi Dave,
> 
> On Tue, 5 May 2020, David T. Lewis wrote:
> 
> >On Sun, May 03, 2020 at 06:45:52PM +0000, commits at source.squeak.org wrote:
> >>Levente Uzonyi uploaded a new version of Chronology-Core to project The 
> >>Inbox:
> >>http://source.squeak.org/inbox/Chronology-Core-ul.54.mcz
> >>
> >>==================== Summary ====================
> >>
> >>Name: Chronology-Core-ul.54
> >>Author: ul
> >>Time: 3 May 2020, 8:45:27.099465 pm
> >>UUID: 770bbc91-fb33-46c2-a283-57f7163b672e
> >>Ancestors: Chronology-Core-nice.52
> >>
> >>Automatically update the time zone cached by the VM every 30 minutes:
> >
> >
> >I am concerned that we are adding kludges on top of kludges, while
> >overlooking a very simple underlying problem.
> >
> >The intent of primitiveUtcWithOffset is to answer, in a single atomic
> >operation, the current time UTC along with the current GMT offset. It
> >should be reported directly from the operating system, and not otherwise
> >cached or manipulated in the VM. The intended use case is for getting the
> >current system time and offset to be used in the creation of DateAndTime 
> >now,
> >nothing else.
> >
> >The original and still correct implementation of ioUtcWithOffset for
> >Unix is a direct call to gettimeofday(). On any reasonably modern Unix
> >system it provides the two values directly in a single OS system call.
> >
> >/* implementation of ioLocalSecondsOffset(), defined in config.h to
> >* override default definition in src/vm/interp.h
> >*/
> >sqInt sqUnixUtcWithOffset(sqLong *microSeconds, int *offset)
> >{
> > struct timeval timeval;
> > if (gettimeofday(&timeval, NULL) == -1) return -1;
> > time_t seconds= timeval.tv_sec;
> > suseconds_t usec= timeval.tv_usec;
> > *microSeconds= seconds * 1000000 + usec;
> >#if defined(HAVE_TM_GMTOFF)
> > *offset= localtime(&seconds)->tm_gmtoff;
> >#else
> > {
> >   struct tm *local= localtime(&seconds);
> >   struct tm *gmt= gmtime(&seconds);
> >   int d= local->tm_yday - gmt->tm_yday;
> >   int h= ((d < -1 ? 24 : 1 < d ? -24 : d * 24) + local->tm_hour - 
> >   gmt->tm_hour);
> >   int m= h * 60 + local->tm_min - gmt->tm_min;
> >   *offset= m * 60;
> > }
> >#endif
> > return 0;
> >}
> >
> >The oscog code base does not have ioUtcWithOffset in 
> >platforms/Cross/vm/sq.h,
> >and the actual implementation of the primitive uses a workaround involving
> >ioUTCMicroseconds and ioLocalSecondsOffset rather than the single call to
> >ioLocalSecondsOffset.
> >
> >I am personally responsible for putting that kludge into Cog in earlier 
> >days,
> >and I apologize for my lapse in judgement. It is horrible, and that's my 
> >fault.
> >
> >It now looks to me like we are trying to have the image keep track of 
> >current
> >GMT offset, and then inform the VM when it changes so that the VM can 
> >update
> >its cached offset value, which it then turns around and uses for my kludgy
> >workaround for the lack of an ioUtcWithOffset implementation.
> >
> >Maybe it is time to add a proper ioUtcWithOffset, and get rid of the 
> >various
> >workarounds in both the VM and the image?
> 
> Eliot always says that asking the OS about the current time zone is too 
> costly for something like DateAndTime now.
> I haven't benchmarked the C code, but I have a Smalltalk benchmark to 
> demonstrate Eliot is right:
> 
> 
> "primitiveUtcWithOffset uses the cached time zone offset"
> a := Array new: 2.
> [ Time primPosixMicrosecondClockWithOffset: a ] benchFor: 1 seconds. 
> '28,200,000 per second. 35.5 nanoseconds per run. 0 % GC time.'.
> 
> "LocalePlugin's primitiveTimezoneOffset asks the OS about the offset via 
> localtime()
> https://github.com/OpenSmalltalk/opensmalltalk-vm/blob/Cog/platforms/unix/plugins/LocalePlugin/sqUnixLocale.c#L675 "
> 
> l := Locale current.
> [ l primTimezone ] benchFor: 1 seconds.
> '773,000 per second. 1.29 microseconds per run. 0 % GC time.'
> 
> 
> The latter takes 363 times longer according to this benchmark.
>

It is OK to cheat if you don't get caught.

On a slow interpreter VM, with the IMHO correct primitive:

   oc := OrderedCollection new.
   stopTime := DateAndTime now + 1 second.
   [ (oc add: (Time primPosixMicrosecondClockWithOffset: DateAndTime basicNew)) < stopTime]
   	whileTrue.
   oc size.  "==> 242827"
   oc asSet size. "==> 242827"

On a fast Spur 64 VM:

   oc := OrderedCollection new.
   stopTime := DateAndTime now + 1 second.
   [ (oc add: (Time primPosixMicrosecondClockWithOffset: DateAndTime basicNew)) < stopTime]
   	whileTrue.
   oc size.  "==> 3098713"
   oc asSet size. "==> 106"

So yes it is fast if you cache the values, but the results are wrong.
That in turn leads to the perceived need for the logic in
Time class>>posixMicrosecondClockWithOffset: to artificially generate
unique time values. Those values are even more incorrect, and the overall
process is almost certainly slower than just doing it right in the first place.

Dave