Set the TZ env to get the UTC-ms since epoch.

Set the TZ env to get the UTC-ms since epoch.

Previously we tried to simply subtract the timezone difference from
the local value. Apparently this doesn't work...

Fixes issue 1268.

Committed: https://code.google.com/p/dart/source/detail?r=7285

[floitsch, 2012-04-03 15:00:55]

Apparently there is no thread-safe function to compute the ms since epoch for a
specific timezone (here UTC or local). I have therefore added a static mutex.

[Ivan Posva, 2012-04-03 23:03:52]

Adding a Mutex here does not help you with thread-safety. Adding Carl as he has
opinions on which APIs to use.

-Ivan

https://chromiumcodereview.appspot.com/9969098/diff/3001/runtime/vm/os_linux.cc
File runtime/vm/os_linux.cc (right):

https://chromiumcodereview.appspot.com/9969098/diff/3001/runtime/vm/os_linux....
runtime/vm/os_linux.cc:52: MutexLocker ml(&timezone_mutex);
You might be protecting against other Dart VM based code killing your TZ
environment variable, but how to you intend to protect against the embedder
calling these functions at the same time? You only think you are safe here, but
are not.

[floitsch, 2012-04-04 08:27:43]

https://chromiumcodereview.appspot.com/9969098/diff/3001/runtime/vm/os_linux.cc
File runtime/vm/os_linux.cc (right):

https://chromiumcodereview.appspot.com/9969098/diff/3001/runtime/vm/os_linux....
runtime/vm/os_linux.cc:52: MutexLocker ml(&timezone_mutex);
On 2012/04/03 23:03:52, Ivan Posva wrote:
> You might be protecting against other Dart VM based code killing your TZ
> environment variable, but how to you intend to protect against the embedder
> calling these functions at the same time? You only think you are safe here,
but
> are not.
I never thought I would be safe against the embedder. Afaik you cannot be safe.
If the embedder is changing the TZ variable then localtime_r, and mktime will
return the values for the embedder's TZ.
However, it is true that we screw up the embedders TZ env. That is, even if the
embedder is nice and doesn't change the TZ, *we* do it behind its back. So the
embedder might get into troubles if it relies on localtime_r and mktime to
return local times.
I just looked at V8's code, and I will try to adapt their approach, so that we
don't change the env (for the embedder's sake).

[floitsch, 2012-04-11 11:48:02]

ping.

[Ivan Posva, 2012-04-11 12:01:23]

Mostly cleanup comments. I hope that Carl can take can properly assess the time
API usage.

-Ivan

https://chromiumcodereview.appspot.com/9969098/diff/4003/runtime/vm/os_linux.cc
File runtime/vm/os_linux.cc (right):

https://chromiumcodereview.appspot.com/9969098/diff/4003/runtime/vm/os_linux....
runtime/vm/os_linux.cc:17: #include "vm/thread.h"
Please only include what you need.

https://chromiumcodereview.appspot.com/9969098/diff/4003/runtime/vm/os_linux....
runtime/vm/os_linux.cc:42: // TODO(floitsch): cache utc offset.
If you cache how long are you willing to cache the value? If the UTC offset of
the machine changes, should this keep track or not?

https://chromiumcodereview.appspot.com/9969098/diff/4003/runtime/vm/os_macos.cc
File runtime/vm/os_macos.cc (right):

https://chromiumcodereview.appspot.com/9969098/diff/4003/runtime/vm/os_macos....
runtime/vm/os_macos.cc:18: #include "vm/thread.h"
ditto

https://chromiumcodereview.appspot.com/9969098/diff/4003/runtime/vm/os_win.cc
File runtime/vm/os_win.cc (right):

https://chromiumcodereview.appspot.com/9969098/diff/4003/runtime/vm/os_win.cc...
runtime/vm/os_win.cc:10: #include "vm/thread.h"
ditto

[floitsch, 2012-04-11 12:10:21]

https://chromiumcodereview.appspot.com/9969098/diff/4003/runtime/vm/os_linux.cc
File runtime/vm/os_linux.cc (right):

https://chromiumcodereview.appspot.com/9969098/diff/4003/runtime/vm/os_linux....
runtime/vm/os_linux.cc:17: #include "vm/thread.h"
On 2012/04/11 12:01:24, Ivan Posva wrote:
> Please only include what you need.

Done.

https://chromiumcodereview.appspot.com/9969098/diff/4003/runtime/vm/os_linux....
runtime/vm/os_linux.cc:42: // TODO(floitsch): cache utc offset.
On 2012/04/11 12:01:24, Ivan Posva wrote:
> If you cache how long are you willing to cache the value? If the UTC offset of
> the machine changes, should this keep track or not?

I would do the same as V8. Changed comment to include a reference to V8.

https://chromiumcodereview.appspot.com/9969098/diff/4003/runtime/vm/os_macos.cc
File runtime/vm/os_macos.cc (right):

https://chromiumcodereview.appspot.com/9969098/diff/4003/runtime/vm/os_macos....
runtime/vm/os_macos.cc:18: #include "vm/thread.h"
On 2012/04/11 12:01:24, Ivan Posva wrote:
> ditto

Done.

https://chromiumcodereview.appspot.com/9969098/diff/4003/runtime/vm/os_win.cc
File runtime/vm/os_win.cc (right):

https://chromiumcodereview.appspot.com/9969098/diff/4003/runtime/vm/os_win.cc...
runtime/vm/os_win.cc:10: #include "vm/thread.h"
On 2012/04/11 12:01:24, Ivan Posva wrote:
> ditto

Done.

[cshapiro, 2012-04-19 23:52:12]

Hi Florian,

I am not so sure about the invocations of tzset.  It does not seem to be
consistently invoked.  We need to straighten that out.

As an aside, I am confused about the choice of OS routines.  It would seem that
we need a wrapper for localtime, mktime, and a "mkgmtime".  All of these
functions traffic in time_t and struct tm, both standard C library types.  In
its current form there seems to be a lot of code duplication of OS-independent
code in the OS-dependent modules and it is hard to tell if the same modification
was done in all 3 places.

Thanks,

Carl

https://chromiumcodereview.appspot.com/9969098/diff/9001/runtime/vm/os_linux.cc
File runtime/vm/os_linux.cc (right):

https://chromiumcodereview.appspot.com/9969098/diff/9001/runtime/vm/os_linux....
runtime/vm/os_linux.cc:42: static int UtcOffsetInSeconds(time_t
seconds_since_epoch) {
This seems unnecessary, see below.

https://chromiumcodereview.appspot.com/9969098/diff/9001/runtime/vm/os_linux....
runtime/vm/os_linux.cc:73: *result += UtcOffsetInSeconds(*result);
See comment in os_macos.cc.

https://chromiumcodereview.appspot.com/9969098/diff/9001/runtime/vm/os_macos.cc
File runtime/vm/os_macos.cc (left):

https://chromiumcodereview.appspot.com/9969098/diff/9001/runtime/vm/os_macos....
runtime/vm/os_macos.cc:31: tzset();  // Make sure the libc knows about the local
zone.
I am confused by this call being removed for Win32 and MacOS but not for Linux.

https://chromiumcodereview.appspot.com/9969098/diff/9001/runtime/vm/os_macos.cc
File runtime/vm/os_macos.cc (right):

https://chromiumcodereview.appspot.com/9969098/diff/9001/runtime/vm/os_macos....
runtime/vm/os_macos.cc:41: // TODO(floitsch): cache utc offset.
This seems unnecessary.  See below.

https://chromiumcodereview.appspot.com/9969098/diff/9001/runtime/vm/os_macos....
runtime/vm/os_macos.cc:72: *result += UtcOffsetInSeconds(*result);
use timegm instead of mktime and correcting with UtcOffsetInSeconds. This
function is available on Linux and MacOS X.

https://chromiumcodereview.appspot.com/9969098/diff/9001/runtime/vm/os_win.cc
File runtime/vm/os_win.cc (left):

https://chromiumcodereview.appspot.com/9969098/diff/9001/runtime/vm/os_win.cc...
runtime/vm/os_win.cc:23: tzset();  // Make sure the libc knows about the local
zone.
Why was this removed?  The documentation states that localtime consults the TZ
environment variable

http://msdn.microsoft.com/en-us/library/a442x3ye(v=vs.80).aspx

Either this call is needed for all conversions using the C library routines, or
it is not.

https://chromiumcodereview.appspot.com/9969098/diff/9001/runtime/vm/os_win.cc
File runtime/vm/os_win.cc (right):

https://chromiumcodereview.appspot.com/9969098/diff/9001/runtime/vm/os_win.cc...
runtime/vm/os_win.cc:48: tzset();
Why is this being called before a conversion for a timezone independent value? 
This looks like it should precede the mktime call, only.

https://chromiumcodereview.appspot.com/9969098/diff/9001/runtime/vm/os_win.cc...
runtime/vm/os_win.cc:58: if ((*result == -1 || *result == 1) &&
(tm_broken_down.tm_wday == -1)) {
How is returning 1 an error?

[ahe, 2012-04-20 13:12:54]

FYI
This passes on Windows:
Release_ia32\dart.exe tests\corelib\src\DateTimeTest.dart

[floitsch, 2012-04-20 16:09:57]

PTAL.

https://chromiumcodereview.appspot.com/9969098/diff/9001/runtime/vm/os_macos.cc
File runtime/vm/os_macos.cc (left):

https://chromiumcodereview.appspot.com/9969098/diff/9001/runtime/vm/os_macos....
runtime/vm/os_macos.cc:31: tzset();  // Make sure the libc knows about the local
zone.
On 2012/04/19 23:52:12, cshapiro wrote:
> I am confused by this call being removed for Win32 and MacOS but not for
Linux.

removed.

https://chromiumcodereview.appspot.com/9969098/diff/9001/runtime/vm/os_macos.cc
File runtime/vm/os_macos.cc (right):

https://chromiumcodereview.appspot.com/9969098/diff/9001/runtime/vm/os_macos....
runtime/vm/os_macos.cc:72: *result += UtcOffsetInSeconds(*result);
On 2012/04/19 23:52:12, cshapiro wrote:
> use timegm instead of mktime and correcting with UtcOffsetInSeconds. This
> function is available on Linux and MacOS X.

The manpage of timegm explicitly states that it is deprecated and should never
be used.

https://chromiumcodereview.appspot.com/9969098/diff/9001/runtime/vm/os_win.cc
File runtime/vm/os_win.cc (left):

https://chromiumcodereview.appspot.com/9969098/diff/9001/runtime/vm/os_win.cc...
runtime/vm/os_win.cc:23: tzset();  // Make sure the libc knows about the local
zone.
On 2012/04/19 23:52:12, cshapiro wrote:
> Why was this removed?  The documentation states that localtime consults the TZ
> environment variable
> 
> http://msdn.microsoft.com/en-us/library/a442x3ye%28v=vs.80%29.aspx
> 
> Either this call is needed for all conversions using the C library routines,
or
> it is not.

localtime_s only looks at the env variable. tzset *uses* the env variable but
doesn't set it. Actually I think that localtime_s invokes tzset itself (quoting
the doc):
When TZ is set, three other environment variables (_timezone, _daylight, and
_tzname) are automatically set as well.

https://chromiumcodereview.appspot.com/9969098/diff/9001/runtime/vm/os_win.cc
File runtime/vm/os_win.cc (right):

https://chromiumcodereview.appspot.com/9969098/diff/9001/runtime/vm/os_win.cc...
runtime/vm/os_win.cc:48: tzset();
On 2012/04/19 23:52:12, cshapiro wrote:
> Why is this being called before a conversion for a timezone independent value?

> This looks like it should precede the mktime call, only.

Looking through the docs it looks as calling tzset should never be necessary.
Removing.

https://chromiumcodereview.appspot.com/9969098/diff/9001/runtime/vm/os_win.cc...
runtime/vm/os_win.cc:58: if ((*result == -1 || *result == 1) &&
(tm_broken_down.tm_wday == -1)) {
On 2012/04/19 23:52:12, cshapiro wrote:
> How is returning 1 an error?

Couldn't find any reference to it either. Probably added before I switched to
_mkgmtime. I will remove it.

[cshapiro, 2012-04-20 20:47:49]

https://chromiumcodereview.appspot.com/9969098/diff/9001/runtime/vm/os_macos.cc
File runtime/vm/os_macos.cc (right):

https://chromiumcodereview.appspot.com/9969098/diff/9001/runtime/vm/os_macos....
runtime/vm/os_macos.cc:72: *result += UtcOffsetInSeconds(*result);
I see no mention of timegm being deprecated or that it should never be used in
any of the documentation.  Relevant references

https://developer.apple.com/library/mac/#documentation/darwin/reference/manpa...

http://man7.org/linux/man-pages/man3/timegm.3.html

http://www.gnu.org/software/libc/manual/html_node/Broken_002ddown-Time.html#i...

[floitsch, 2012-04-23 08:37:09]

https://chromiumcodereview.appspot.com/9969098/diff/9001/runtime/vm/os_macos.cc
File runtime/vm/os_macos.cc (right):

https://chromiumcodereview.appspot.com/9969098/diff/9001/runtime/vm/os_macos....
runtime/vm/os_macos.cc:72: *result += UtcOffsetInSeconds(*result);
On 2012/04/20 20:47:49, cshapiro wrote:
> I see no mention of timegm being deprecated or that it should never be used in
> any of the documentation.  Relevant references
> 
>
https://developer.apple.com/library/mac/#documentation/darwin/reference/manpa...
> 
> http://man7.org/linux/man-pages/man3/timegm.3.html
> 
>
http://www.gnu.org/software/libc/manual/html_node/Broken_002ddown-Time.html#i...

Ok. "deprecated" was too strong. But they explicitly state that usage should be
avoided. They furthermore don't give any guarantees on multi-threading. That
said, I just went through the sources of the libc and they don't change the env
and also use a lock.
Since this is not guaranteed I would still go with v8's approach, but it's your
call.

[cshapiro, 2012-04-27 03:34:02]

I looked at the glibc sources and, just for reference, the freebsd libc sources
and did not see a lock being acquired in the timegm or its callees.  Perhaps I
have missed something.  Nevertheless, given that this code can be embedded
within a larger application, side effecting global state is an opportunity for a
race, something we should avoid.  

My primary concern at this point is that we eliminate the unsafe assignments to
TZ.  I think calls to timegm on MacOS and Linux solve this.  I do not know what
the V8 approach is (I did notice that V8 calls _tzset on Win32, which confused
me), but up-thread you suggested this avoided the issue with the setenv of TZ.

I certainly do not want this review to go on forever.  I am fine with any
approach that does not make race-y assignments to setenv.  I think using timegm
is the shortest way to get there.  If you think something V8 does is better (and
if it is not have any races), and can implement that quickly, I am okay with
that too.

[floitsch, 2012-04-27 08:24:43]

On 2012/04/27 03:34:02, cshapiro wrote:
> I looked at the glibc sources and, just for reference, the freebsd libc
sources
> and did not see a lock being acquired in the timegm or its callees.  Perhaps I
> have missed something.  Nevertheless, given that this code can be embedded
> within a larger application, side effecting global state is an opportunity for
a
> race, something we should avoid.
The GNU version takes a lock before internally changing the TZ (line 619).
 
http://sourceware.org/git/?p=glibc.git;a=blob;f=time/tzset.c;h=b87578fe48a768...

> 
> My primary concern at this point is that we eliminate the unsafe assignments
to
> TZ.  I think calls to timegm on MacOS and Linux solve this.
My point is, that there is no guarantee it won't. The documentation on this
function is sparse and does not mention multithreading. The GNU version does
internally take a lock, but afaics nothing guarantees that.

>  I do not know what
> the V8 approach is (I did notice that V8 calls _tzset on Win32, which confused
> me), but up-thread you suggested this avoided the issue with the setenv of TZ.
The current version does not use tzset or setenv anymore. So this should be
safe.

> 
> I certainly do not want this review to go on forever.  I am fine with any
> approach that does not make race-y assignments to setenv.  I think using
timegm
> is the shortest way to get there.  If you think something V8 does is better
(and
> if it is not have any races), and can implement that quickly, I am okay with
> that too.
The current version should be race-free.
Afaics it's either using a function that should be avoided (according to its
doc) and does not (explicitly) guarantee thread-safety versus using a field that
is not documented.

I consider both non-optimal but since V8 uses the field too I'm slightly more
comfortable going that route. In any case it is now *much* easier to change if
we want to.

I read your last paragraph as LGTM, but to be sure please give me an explicit
ok, if that's what you meant. If you still think that timegm is the better
approach that's fine with me too. It would probably not take more than 10-15
minutes to do the change.

[cshapiro, 2012-05-01 20:25:53]

On 2012/04/27 08:24:43, floitsch wrote:
>
http://sourceware.org/git/?p=glibc.git;a=blob;f=time/tzset.c;h=b87578fe48a768...

Got it.  Thanks.

> My point is, that there is no guarantee it won't. The documentation on this
> function is sparse and does not mention multithreading. The GNU version does
> internally take a lock, but afaics nothing guarantees that.

I wrote a micro-benchmark that times the invocation cost of timegm, mktime, and
mktime followed by localtime_r.  Configurations of 1, 2, 4, 8, and 16 threads
are evaluated.  The micro-benchmark harness repeats the invocation of these
routines until the timing stabilizes.  Here are the results

CPU: Intel Nehalem with HyperThreading (8 cores) dL1:32KB dL2:256KB
Benchmark                          Time(ns)    CPU(ns) Iterations
-----------------------------------------------------------------
BM_timegm/threads:1                     104        102    6363636  
BM_timegm/threads:2                     385        709     875000  
BM_timegm/threads:4                     585       2121     311112  
BM_timegm/threads:8                     569       4250     160000  
BM_timegm/threads:16                    657       9286      70000  
BM_mktime/threads:1                     969        964     777778  
BM_mktime/threads:2                    3102       4714     140000  
BM_mktime/threads:4                    4335       7794     103924  
BM_mktime/threads:8                    4920      21714      35000  
BM_mktime/threads:16                   5768      76673       8608  
BM_mktime_localtime_r/threads:1        1107       1100     636364  
BM_mktime_localtime_r/threads:2        3331       4133     125802  
BM_mktime_localtime_r/threads:4        5487      10115      58332  
BM_mktime_localtime_r/threads:8        6478      35211      16472  
BM_mktime_localtime_r/threads:16       6520      92345       7472  

These numbers show that timegm is much less costly than mktime.  Also, it looks
like the overhead of calling localtime_r after mktime is relatively small.

> I read your last paragraph as LGTM, but to be sure please give me an explicit
> ok, if that's what you meant. If you still think that timegm is the better
> approach that's fine with me too. It would probably not take more than 10-15
> minutes to do the change.

In light of the benchmark data I think that using timegm is the right way to
proceed.

[floitsch, 2012-05-02 10:54:40]

PTA (hopefully final) L.

[cshapiro, 2012-05-02 22:04:44]

lgtm

https://chromiumcodereview.appspot.com/9969098/diff/29007/runtime/vm/os.h
File runtime/vm/os.h (right):

https://chromiumcodereview.appspot.com/9969098/diff/29007/runtime/vm/os.h#new...
runtime/vm/os.h:25: static bool Gmtime(int64_t seconds_since_epoch, tm*
tm_result);
If you are going to capitalize the T in MkTime you probably want to do the same
here and elsewhere: "GmTime", "LocalTime", "MkGmTime".

[floitsch, 2012-05-03 14:46:04]

https://chromiumcodereview.appspot.com/9969098/diff/29007/runtime/vm/os.h
File runtime/vm/os.h (right):

https://chromiumcodereview.appspot.com/9969098/diff/29007/runtime/vm/os.h#new...
runtime/vm/os.h:25: static bool Gmtime(int64_t seconds_since_epoch, tm*
tm_result);
On 2012/05/02 22:04:44, cshapiro wrote:
> If you are going to capitalize the T in MkTime you probably want to do the
same
> here and elsewhere: "GmTime", "LocalTime", "MkGmTime".

Done.