Add time zones support

[straight-shoota]

Until now, instances of Time had a kind member which indicated, whether this instance holds a time in UTC, an unspecified "local" timezone or is entirely unspecified.

This PR adds a Time::Location class which represents a specific time zone. Each Time instance references a Location (i.e where this time applies). From this location's transition rules, the applicable offset at each point in time can be derived, including daylight savings time, time zone changes etc.

Storing only an offset from UTC is not enough, because from that you can't infer what timezone transition rules to apply. And offset changes inside a time zone happen quite frequently (for example daylight savings).

Time

The kind ivar in Time is replaced by location which is a pointer to a Location instance.

struct Time
  @seconds : Int64                    # 8 Byte
  @nanoseconds : Int32                # 4 Byte
  @kind : Time::Kind                  # 4 Byte
end

# Becomes:
struct Time
  @seconds : Int64                    # 8 Byte
  @nanoseconds : Int32                # 4 Byte
  @location : Pointer(Time::Location) # 8 Byte (64 bit architecture)
end

This means, each Time struct will require 24 bytes instead of the currently used 16 bytes (on 64-bit). This increases the size of a time struct slightly but it is inevitable to embed a reference to a time zone if proper time zone handling should be supported.

For incremental time (a reference to a a specific point in time), seconds and nanoseconds always represent time in UTC. The offset will only be added when needed (for example to access one of the date/time fields individually). This makes it easier than if it was stored with offset as that might lead to invalid or ambiguous instances (for example when an hour is skipped or happens twice at a DST switch). Another benefit is, that retrieving the offset (zone transitions are looked up by unix epoch) and comparing two instances requires no conversion. Converting a time between zones means just exchanging the location.

Time::Location

Each location can be one of the following

• a timezone in the IANA time zone database read from a local copy of this database which is provided for by the operating system (available in most unix-like OS's) or a custom distribution (can be configured throught ZONEINFO environment variable)
• A zone with a fixed-offset
• Coordinated Universal Time, available as Time::Location::UTC
• an unspecified time zone, indicating a floating time which is not tied to a specific time zone.

The concept for loading location data from system sources is inspired by Go's implementation which seems very reasonable for this.

Of course, it can't be expected to find that zoneinfo data available on every platform (hey, Windows!). Other systems will probably require some additional effort, but Go has implementations for a variety of platforms, so it can be done one way or another. As far as I understand, under Windows, timezone information is available (needs some translation and stuff, but it's usable) but only limited to the current applicable time zone data. Application that need to cover wider time frames will probably have to distribute their own copy of the timezone database. But in general, it is certainly best if we can rely on the operating system to provide that data.
For now, it should work for all currently supported Crystal targets.

Floating time

A floating time which is not tied to a specific time zone is currently indicated by the special location Location::UNSPECIFIED. Time instances created with Time.new and no location argument are by default floating times. This means, they don't represent a specific point in time but are only aplicable in a specific context.
A floating time 08:30 can for example represent each point in time, where the local time shows 08:30, independent of local time's timezone. If I set an alarm at 08:30, I want that alarm to trigger when it is 8:30 wherever I am at that point.
But it doesn't have to be that, it can also have a different meaning. Floating times have no semantic context.

Comparing or operating on floating and incremental times causes problems:

For example, my computer time says 2017-11-24 19:37, we'll use that as a floating time.
Another source tells me, the current time in UTC is 2017-11-24 18:37 UTC or in my local timezone: 2017-11-24 19:37 +01:00.
Let's compare that with the floating time:

floating = 2017-11-24 19:37
utc      = 2017-11-24 18:37 UTC
local    = 2017-11-24 19:37 +01:00

utc      == local # => true, both represent the same point in time
floating == utc   # => ?
floating == local # => ?

We know that all three readings have been observed a the same point in time, so the last two comparisons should both be true, but they are mutually exclusive.

These issues with floating time are present all over the place (what should the format %z (time zone) print?). I've tried to implement this as sane as possible, but it's still a conceptual problem.
To solve this, I've already brought up the proposal to add a dedicated type for floating time, but this is another issue.

Time representation

This PR also modifies the string representation of a Time instance (regarding #inspect, #to_s):

#  Floating times with unspecified location have no time zone attached and won't print any.
2017-11-25 22:06:17

# UTC times print time zone `UTC`
2017-11-25 22:06:17 UTC

# Times with a fixed-offset location print the timezone offset (seconds are truncated unless relevant)
2017-11-25 22:06:17 +01:00
2017-11-25 22:06:17 +01:00:37

# Times with a non-fixed-offset location print the timezone offset plus zone name
2017-11-25 22:06:17 +01:00 Europe/Berlin

Requires #5321
Fixes #2490
/cc #2481 #4556

[RX14]

I don't think that times should have an unspecified timezone by default, in fact I think we should reconsider the existence of an unspecified time zone: does it have a valid usecase. Having the possibility of not being able to convert a given time to an instant in UTC adds a lot of error cases.

[straight-shoota]

Time.new could probably use the local timezone by default, but I'm not entirely sure about this. For now I've kept it as it has been before (only that until now it didn't have much effect anyway).
It could also make sense to promote the more descriptive constructor methods supporting date field arguments - namely Time.utc, Time.local (maybe Time.floating or similar) - and maybe make Time.now to require location instead of providing a default. This would be super expressive but I guess it's not as developer-friendly and easy-to-understand for beginners.

I'd favor to keep this PR focused on the implementation of time zones and have the general discussion about floating time in a separate issue. For now, I've tried to adjust the existing structure to the new concept, but this can be changed depending on the outcome of the floating time issue.

[straight-shoota]

I've added a section about Time representation in the OP

[Sija]

" " could be a Char.

[straight-shoota]

Thanks, I intended to change that.
However, I'd rather welcome feedback on the general concept instead of such minor details ;)

[luislavena]

Thank you @straight-shoota for this PR, your approach looks very interesting and will love to take a look and see this implementation against some apps I have in the following days.

Will be possible for you to include some examples on creation of time instances on fixed locations? For example, how will be possible to recreate certain date/time on EST/EDT or PST/PDT (perhaps adding that to the Time.new constructor documentation).

Also notice that Time.new with no argument will default to Time::Location.local while Time.new with arguments will default to unspecified (floating). It will be great to include more examples on that usage here in the PR or the code itself (I prefer the later).

Once again, thank you for your contribution and looking forward your responses!
❤️ ❤️ ❤️

[straight-shoota]

@luislavena Some typical usage should be documented in the specs - but of course the documentation still needs to be improved further.

Some examples:

# using a fixed offset of 3:00:
location = Time::Location.fixed(3 * 3600)
time =  Time.new 2014, 10, 30, 21, 18, 13, location: location # => 2014-10-30 21:18:13 +03:00
# using a tz database location:
location = Time::Location.load("Europe/Berlin")
time = Time.new 2014, 10, 30, 21, 18, 13, location: location # => 2014-10-30 21:18:13 +01:00 Europe/Berlin
location = Time::Location.load("EST5EDT")
time = Time.new 2014, 10, 30, 21, 18, 13, location: location # => 2014-10-30 21:18:13 -04:00 EST5EDT
location = Time::Location.load("PST8PDT")
time = Time.new 2014, 10, 30, 21, 18, 13, location: location # => 2014-10-30 21:18:13 -07:00 PST8PDT

The use of Time::Location.load/Time::Location.fixed might be a bit tedious, but I'm not sure if and how this could be improved. It would be possible to allow location to receive a string or int and create the corresponding location accordingly. But I'm not sure if this would be a good idea as it would hide the fact that each call creates a new instance of Time::Location.

Time.new is essentially the same as Time.now, and for the current time it makes sense to be in the local timezone by default. Time.new with individual date/time fields is something different, and as long as there is no location provided, it makes sense to recognize this as "floating" time. It usually makes little sense to create a reference to an absolute wall time reading with a completely unknown zone offset - for all we know, Location.local could be anything at runtime, so it is essentially a floating date, not a specific instance in incremental time.
If you want to create a reference to an absolute point in time, you need to specific an explicit time zone reference, either by using Time.utc or specifying a concrete location (either fixed offset or named location).

So, clearly, the naming can be a bit confusing and should certainly be improved, but that will probably depend on what we decide to do about floating time. One solution could be to make Time.new always require a location and introduce a different means to create floating time (like Time.floating) which won't accept a different location.

[bew]

Is it possible/want-able to create a Location object from a Time::Span object ?

[straight-shoota]

@bew Do you have some usecase in mind?
It should be as easy as Location.fixed(span.total_seconds), so I don't think it would be worth adding a specific API for this.

[bew]

No, no use-case in mind, I was just reading your example Time::Location.fixed(3 * 3600) and was thinking "hmm could we do Time::Location.fixed(3.hours) instead?"

[straight-shoota]

Yeah, I think it should not be too common to use this constructor at all, and even less with a static value.
Creating fixed-offset locations should be avoided as much as possible anyway, unless there is no other way, for example when the time is read from an external source with only an offset and no time zone location attached.

Such absolute timepoints in machine-readable data don't need any offset at all and should better work directly in UTC.
Offsets are only relevant for human interaction, but then you need a proper timezone location anyway to accurately map time and offsets.

[luislavena]

@straight-shoota will be possible for you to rebase against master now that #5321 has been merged?

Thank you.
❤️ ❤️ ❤️

[straight-shoota]

Rebased.

[luislavena]

Thank you @straight-shoota for the explanation and sorry for the delay on getting back to you on your comments.

re: floating time

From the example:

floating = 2017-11-24 19:37
utc      = 2017-11-24 18:37 UTC
local    = 2017-11-24 19:37 +01:00

utc      == local # => true, both represent the same point in time
floating == utc   # => ?
floating == local # => ?

I don't think floating == utc should return true. As described, the scenario of least surprise will be that floating compares the time and date elements of both dates disregard of the time location of each other.

That will allow me ask the question tell me when is 7pm independently of the time location the other time instance I'm comparing against it.

But I'm not sure if this would be a good idea as it would hide the fact that each call creates a new instance of Time::Location.

This will definitely need to be addressed via a cache per-process. Looked at this through the eyes of someone that had to load 6 different timezones and process 500K date records. Haven't converted the script to test this (is a Ruby one) but might do over the weekend for further comments.

Last but not least, really love the flexibility to move time instances across timezones:

ny = Time::Location.load("America/New_York")
ar = Time::Location.load("America/Buenos_Aires")

time1 = Time.now(location: ny)
pp time1 # => 2017-11-28 10:34:23 -05:00 America/New_York

time2 = time1.in(location: ar)
pp time2 # => 2017-11-28 12:34:23 -03:00 America/Buenos_Aires

❤️ ❤️ ❤️

[straight-shoota]

Well, utc and local describe the same point in time, so why floating should really be equal utc when both are observed in the same time zone. This can't be decided conclusively. There will always be some kind of contradiction.
But I'm working on opening a separate issue about floating time, so we can focus here on the location implementation.

Caching location data is not as straightforward as it might seem, because the time zone database could get updated every now and then. You can imagine for example a server-process running for several years and the tz database had 10 releases alone in 2016. As we all know, cache-invalidation is one of the most difficult things in computer science.

The best way to handle this depend highly on the application. I like the idea to make (optional) caching of location data a job of the application or a 3rd party library. The current solution works well enough for stdlib and should be sufficient for many simple use cases. And it's easy to build upon.

[RX14]

Floating doesn't have enough information to be compared, so it shouldn't. Always return false unless it's floating too.

[larubujo]

this change makes all code (like an empty file or a hello world) require zip, zlib, flate, crc32. doesn't sound like a nice thing...

[straight-shoota]

@larubujo Yes, thanks for noticing. This is only temporary to provide a prototype. These dependencies will be removed.
I'm planning on including a custom reader, like it is implemented in Go. This is pretty straightforward for uncompressed ZIP files, I just didn't want to spend the time on that when a working example can rely on the existing implementation.

[Sija]

"0" -> '0'

[straight-shoota]

These lines were not modified by this PR (only whitespace change), so I think this could be fixed independently.

[larubujo]

also char is slower than string. the people that thought that char is more efficient got it wrong. same when you index a string with a char instead of a string. or starts_with. char is slower. so string is preferred.

[straight-shoota]

@larubujo Sure about this? My benchmark says otherwise. However, this is completely off-topic here. So, if you have some evidence to support your claim, please post a new issue about that. This would be quite unexpected.

[Sija]

@larubujo it's true indeed for starts_with?(Char) overload—its performance was severely degraded since v0.23.1 btw:

Benchmark.ips do |x|
  x.report("starts_with? Char") { "foo".starts_with? 'f' }
  x.report("starts_with? String") { "foo".starts_with? "f" }
end

v0.23.1:

  starts_with? Char  457.3M (  2.19ns) (±19.03%)  1.02× slower
starts_with? String 468.03M (  2.14ns) (±17.47%)       fastest

master:

  starts_with? Char  207.7M (  4.81ns) (±13.71%)  2.23× slower
starts_with? String 463.22M (  2.16ns) (±13.89%)       fastest

Yet, passing a Char to IO is still faster:

Benchmark.ips do |x|
  io = IO::Memory.new

  x.report("io << Char") { io << 'f' }
  x.report("io << String") { io << "f" }
end

v0.23.1:

  io << Char 139.61M (  7.16ns) (±19.08%)       fastest
io << String  82.44M ( 12.13ns) (±20.64%)  1.69× slower

master:

  io << Char 142.37M (  7.02ns) (±17.83%)       fastest
io << String  83.37M ( 11.99ns) (±13.60%)  1.71× slower

[larubujo]

it seems i was partially wrong. but it still slower for non-ascii:

require "benchmark"

haystack = "がこう"

needle_string = "う"
needle_char = 'う'

a = 0
Benchmark.ips do |x|
  x.report("ends char") do
    a += haystack.ends_with?(needle_char) ? 1 : 0
  end
  x.report("ends string") do
    a += haystack.ends_with?(needle_string) ? 1 : 0
  end
end

Benchmark.ips do |x|
  x.report("starts char") do
    a += haystack.starts_with?(needle_char) ? 1 : 0
  end
  x.report("starts string") do
    a += haystack.starts_with?(needle_string) ? 1 : 0
  end
end

puts a

output

  ends char  97.43M ( 10.26ns) (±10.90%)  1.40× slower
ends string 136.87M (  7.31ns) (±11.64%)       fastest
  starts char  95.46M ( 10.48ns) (±10.51%)  1.40× slower
starts string 133.26M (   7.5ns) (±10.70%)       fastest

in any case times around nanosecond, so corrections like use '0' instead of "0" dont matter much

[RX14]

I agree that it doesn't matter much, but if you consider the future evolution of the standard library then using the char overload gives you much more optimization possibilities, even if the current performance is different.

If you notice a performance problem, make an issue please.

[Sija]

ditto

[ysbaddaden]

Thanks for the work. Directly parsing the tzdata files is a welcomed achievement, and overall the PR is great!

I'd like to raise a few issues, thought:

1. Requiring zlib in all Crystal programs that use Time, in the event that ZONEINFO may target a ZIP file. I'd assume this is a fairly uncommon practice (please correct me) and the Crystal compiler, for example, is expressly compiled without zlib. Maybe we can relax this, and assume zlib to be fairly available, but this adds yet-another-dependency that not be needed otherwise.

2. Parsed locations appear to never be cached? Which means that Time.new will call Location.local which will lookup a zoneinfo (from TZ) or load /etc/localtime each time?

3. Unspecified location and overall floating time support. IMHO this doesn't belong to this pull request, the related issue even states this is "far from ideal" and it raises concerns (floating time isn't a time with offset that happens to not have an offset, actually, but it's not UTC either, and... infinite rant). IMHO this should be dropped from this PR, and maybe reintroduced later (or not) depending on the outcome of [RFC] Supporting floating time #5332.

Once those 3 points and answered or fixed. This will be a great 👍 from me.

[ysbaddaden]

Huh? reference == next_day (same date, same time, same location); shouldn't next day be the 29th? Thus the assertion below should fail.

[straight-shoota]

Yeah, that's a typo. This example is marked as pending, thus it's not executed at all (and the error got unnoticed until now). With the current implemenation of Time::Span it won't work, because 1.day equals 24 hours, but from 2017-10-28 13:37 (CEST +02:00) to 2017-10-29 13:37 (CET +01:00) it is only 23 hours (but still 1 day).

[ysbaddaden]

I understand you're copying the tzset behavior here, yet having TZ=nil mean local but TZ="" mean UTC is a bit confusing. It's also missing a test for an unknown location (eg: Unknown/Nowhere) that should fallback to UTC.

[straight-shoota]

I concur it is confusing, but so would it be if TC="" did not mean UTC in contrast to tzset... I'm not sure what would be better, but in doubt I'd rather mirror the behaviour of tzset instead of introducing new semantics.

[straight-shoota]

I think the semantics of TZ environment variable should stay the same, we could perhaps remove Location.load("") to mean UTC.

[ysbaddaden]

Maybe consider a Location.load_file("/etc/localtime") to avoid instantiating an array just to throw it away.

[straight-shoota]

I guess it could just open the file here and use read_zoneinfo directly, skipping a few unnecessary steps in between.

[ysbaddaden]

Throwing an exception just to silence it is slow. Please introduce a nilable load?(name) method instead.

[straight-shoota]

I think it's sufficient to add load?(name, sources). At least for this usage we don't need all the special case checks.

[ysbaddaden]

The actual documentation is the IANA. For tzdata this is https://data.iana.org/time-zones/tz-link.html with the source reference at https://github.com/eggert/tz (and of course the format spec in the tzfile(5) man page).

I'd say it's more than "fairly standard" but a "de-facto standard", used by most operating systems (even windows) if not all.

[ysbaddaden]

Time.new should already be localtime so it's a duplicate.

[ysbaddaden]

Again: localtime.

[ysbaddaden]

Do we really have to support ZONEINFO being a ZIP file? That requires all programs that use Time to always link against libz, for something that seems fairly uncommon.

Furthermore, we're compiling Crystal itself with -D without_zlib so it should at least be verified here, or just drop support for ZONEINFO altogether.

[straight-shoota]

If I get a thumbs up, I'll happily implement the simple ZIP access.

[ysbaddaden]

Just call Crystal::System::Time.zone_sources directly.

[ysbaddaden]

Ditto: just call Crystal::System::Time.load_localtime directly.

[straight-shoota]

@ysbaddaden Thanks for the in-depth review!

1. Please see my previous comment answering to @larubujo. I'd implement the ZIP reader directly - it doesn't need to support compression, so it's quite easy to do (essentially a port of this Go function). No need to depend on zlib. On most unix systems, time zone data should be readily available. But on other platforms like Windows (which will be supported eventually) there will be a need for applications to have some other source for a tzinfo database, preferably a ZIP file.

2. Yes. This is surely open for debate. I've quickly touched this in a comment before:

Caching location data is not as straightforward as it might seem, because the time zone database could get updated every now and then. You can imagine for example a server-process running for several years and the tz database had 10 releases alone in 2016. As we all know, cache-invalidation is one of the most difficult things in computer science.

I think the best strategy for stdlib is to to provide the basic tools to have proper time zone support, but leave optimization layers like caching and maybe some sort of location registry to the application or 3rd party libraries. That's also the way they do it in Go (though I'm not sure if there are libraries for caching locations - but the general idea that the stdlib implementation always reads from the database directly).
If you just need the current timestamp, you can prevent a lookup of the local timezone by using Time.utc.

3. We had Kind::Unspecified before, that's why I tried to incorporate this concept (even if it did not literally mean "floating time"). But I agree on your reasoning to remove it from this PR. Though I see a few (minor) issues that might need discussion. I will address that in a subsequent comment.

[straight-shoota]

Ad 3. Removing Location::UNSPECIFIED

It is fairly straightforward to remove all the parts relating to floating time. However, there is an issue with the time format parser: The parser can always encounter time format without time zone which are by themselves not related to a location. How this is to be interpreted depends on the context. In YAML for example, a time without offset or time zone name is considered to be in UTC. In other cases it might mean local time (though that is quite imprecise) or just be undefined (=floating time).
Now, even if we don't support floating time explicitly, it should be possible to recognize that a time value without time zone was parsed. I think this can be handled with user code, such as creating a custom "unspecified" location instance and using this as default value. If the parsed time value has this location, it did not contain a time zone. This should be good without any additional help from stdlib.

The question is, should there be a default value for Time::Format::Parser#time(location) and if so, which would it be? I don't think we can just assume any specific default, because this can't be determined generally.
The culprit is that a default location is only needed for the relative uncommon parsing of a time format which does not include a time zone. For most use cases, it makes no sense to require a default location at all because it's simply never needed.

So, we could either select an arbitrary default location if none particular is specified by the caller, which could be completely wrong in some contexts but might go unnoticed, possibly introducing a subtle hidden bug. Or we could remove the requirement to have an implicit default location which would raise an exception if none is specified by the caller. This would also fail but at least it would be noticed.
I'd prefer the latter: if the format does not include time zone information and no default location was explicitly provided, there is no way to know how such a value should be represented as an instance of Time.

[RX14]

Go for nillable default timezone with raise on get.

[straight-shoota]

I've added two commits to get this running on windows. 💯

Only Location::UTC, Location.local and custom locations are supported for now. Loading tzdata files should work once File is properly ported.

[Sija]

redundant return.

[RX14]

Typos in ae7e6b8's description

timei zone

Time::Loication

[RX14]

Can we use named tuples here for descripriveness?

[RX14]

We don't typically doc these...

[RX14]

Please indent these properly.

[straight-shoota]

@RX14 fixed the issues.

[straight-shoota]

Awesome to see this merged! Thanks for all the reviews and suggestions helping to get this done 💯