Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement Comparable time marks in a time source returned by Clock.asTimeSource() #271

Merged
merged 3 commits into from
May 16, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 39 additions & 6 deletions core/common/src/Clock.kt
Original file line number Diff line number Diff line change
Expand Up @@ -40,17 +40,50 @@ public fun Clock.todayIn(timeZone: TimeZone): LocalDate =
* Returns a [TimeSource] that uses this [Clock] to mark a time instant and to find the amount of time elapsed since that mark.
*/
@ExperimentalTime
public fun Clock.asTimeSource(): TimeSource = object : TimeSource {
override fun markNow(): TimeMark = InstantTimeMark(now(), this@asTimeSource)
public fun Clock.asTimeSource(): TimeSource.WithComparableMarks = object : TimeSource.WithComparableMarks {
override fun markNow(): ComparableTimeMark = InstantTimeMark(now(), this@asTimeSource)
}

@ExperimentalTime
private class InstantTimeMark(private val instant: Instant, private val clock: Clock) : TimeMark {
override fun elapsedNow(): Duration = clock.now() - instant
private class InstantTimeMark(private val instant: Instant, private val clock: Clock) : ComparableTimeMark {
override fun elapsedNow(): Duration = saturatingDiff(clock.now(), instant)

override fun plus(duration: Duration): TimeMark = InstantTimeMark(instant + duration, clock)
override fun plus(duration: Duration): ComparableTimeMark = InstantTimeMark(instant.saturatingAdd(duration), clock)
override fun minus(duration: Duration): ComparableTimeMark = InstantTimeMark(instant.saturatingAdd(-duration), clock)

override fun minus(duration: Duration): TimeMark = InstantTimeMark(instant - duration, clock)
override fun minus(other: ComparableTimeMark): Duration {
if (other !is InstantTimeMark || other.clock != this.clock) {
throw IllegalArgumentException("Subtracting or comparing time marks from different time sources is not possible: $this and $other")
}
return saturatingDiff(this.instant, other.instant)
}

override fun equals(other: Any?): Boolean {
return other is InstantTimeMark && this.clock == other.clock && this.instant == other.instant
}

override fun hashCode(): Int = instant.hashCode()

override fun toString(): String = "InstantTimeMark($instant, $clock)"

private fun Instant.isSaturated() = this == Instant.MAX || this == Instant.MIN
private fun Instant.saturatingAdd(duration: Duration): Instant {
if (isSaturated()) {
if (duration.isInfinite() && duration.isPositive() != this.isDistantFuture) {
throw IllegalArgumentException("Summing infinities of different signs")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But we already have Instant.plus(Duration). Its behavior is slightly different, and I even like saturatingAdd more, but shouldn't saturatingDiff also throw on NaN if we keep saturatingAdd the way it is here?

Copy link
Member Author

@ilya-g ilya-g May 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't saturatingDiff also throw on NaN

No, the operation minus on time marks is defined to return zero when calculating difference between two equal infinitely distant time marks.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it internally inconsistent? This would make sense in the Instant infinity model of "all events in the far future are just one event," but in the Mark infinity model of "infinitely far events are fuzzily distributed along the timeline," this is surprising.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's consistent in time mark infinity model: all the distant events are the same infinitely distant event. It's not consistent with math infinity, but we're ok with that. See the Libraries DM at 2023-04-11.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, ok. This model is so unintuitive to me that I didn't even think about it. Well, since the behavior is already established and you implemented it precisely, I guess there's no need to discuss anything further, as my gripes are with the behavior, not the implementation.

}
return this
}
return this + duration
}
private fun saturatingDiff(instant1: Instant, instant2: Instant): Duration = when {
instant1 == instant2 ->
Duration.ZERO
instant1.isSaturated() || instant2.isSaturated() ->
(instant1 - instant2) * Double.POSITIVE_INFINITY
else ->
instant1 - instant2
}
}

@Deprecated("Use Clock.todayIn instead", ReplaceWith("this.todayIn(timeZone)"), DeprecationLevel.WARNING)
Expand Down
86 changes: 86 additions & 0 deletions core/common/test/ClockTimeSourceTest.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
/*
* Copyright 2019-2023 JetBrains s.r.o. and contributors.
* Use of this source code is governed by the Apache 2.0 License that can be found in the LICENSE.txt file.
*/

package kotlinx.datetime.test

import kotlinx.datetime.*
import kotlin.test.*
import kotlin.time.*
import kotlin.time.Duration.Companion.days
import kotlin.time.Duration.Companion.nanoseconds

@OptIn(ExperimentalTime::class)
class ClockTimeSourceTest {
@Test
fun arithmetic() {
val timeSource = Clock.System.asTimeSource()
val mark0 = timeSource.markNow()

val markPast = mark0 - 1.days
val markFuture = mark0 + 1.days

assertTrue(markPast < mark0)
assertTrue(markFuture > mark0)
assertEquals(mark0, markPast + 1.days)
assertEquals(2.days, markFuture - markPast)
}

@Test
fun elapsed() {
val clock = object : Clock {
var instant = Clock.System.now()
override fun now(): Instant = instant
}
val timeSource = clock.asTimeSource()
val mark = timeSource.markNow()
assertEquals(Duration.ZERO, mark.elapsedNow())

clock.instant += 1.days
assertEquals(1.days, mark.elapsedNow())

clock.instant -= 2.days
assertEquals(-1.days, mark.elapsedNow())

clock.instant = Instant.MAX
assertEquals(Duration.INFINITE, mark.elapsedNow())
}

@Test
fun differentSources() {
val mark1 = Clock.System.asTimeSource().markNow()
val mark2 = object : Clock {
override fun now(): Instant = Instant.DISTANT_FUTURE
}.asTimeSource().markNow()
assertNotEquals(mark1, mark2)
assertFailsWith<IllegalArgumentException> { mark1 - mark2 }
assertFailsWith<IllegalArgumentException> { mark1 compareTo mark2 }
}

@Test
fun saturation() {
val mark0 = Clock.System.asTimeSource().markNow()

val markFuture = mark0 + Duration.INFINITE
val markPast = mark0 - Duration.INFINITE

for (delta in listOf(Duration.ZERO, 1.nanoseconds, 1.days)) {
assertEquals(markFuture, markFuture - delta)
assertEquals(markFuture, markFuture + delta)

assertEquals(markPast, markPast - delta)
assertEquals(markPast, markPast + delta)
}
val infinitePairs = listOf(markFuture to markPast, markFuture to mark0, mark0 to markPast)
for ((later, earlier) in infinitePairs) {
assertEquals(Duration.INFINITE, later - earlier)
assertEquals(-Duration.INFINITE, earlier - later)
}
assertEquals(Duration.ZERO, markFuture - markFuture)
assertEquals(Duration.ZERO, markPast - markPast)
Comment on lines +80 to +81
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This highlights that we have a problem with Instant saturation. If we have something like Instant.MAX + 1.hours - (Instant.MAX - 1.hours), we get something like 1.hours, whereas, due to saturation, it should be a NaN. Lacking NaN, I'd settle for some other nonsensical value like Instant.MAX or Instant.MIN, but certainly not what we have now, which is a plausible but incorrect result.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are not instants, but time marks, they have different saturation rules. Some are infinitely distant, but we defined the difference between them as zero.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are not instants, but time marks, they have different saturation rules

Is there a reason why we can't consider Instant values just a particular sort of time marks where we know exactly which point in real-life time the time mark corresponds to? To me, the distinction is blurry enough that I wouldn't expect their behavior to differ. In fact, even when saturation happens, they behave the same in most cases.

Looks like the difference is just the following:

  • Instant.MAX - Duration.INFINITE is Instant.MIN (so the Duration is "more infinite"), but Mark(Instant.MAX) - Duration.INFINITE throws.
  • Instant.MAX - (Instant.MAX - 1.seconds) is 1.seconds, but Mark(Instant.MAX) - Mark(Instant.MAX - 1.seconds) is infinity.

So, we have two models of infinity:

  • Beyond Instant.MAX, all events happen simultaneously, but Instant.MAX happens immediately after Instant.MAX - epsilon. So, the time "stops" after a point, collapsing into one event. Duration behaves consistently with that: since this event that everything collapsed into is just a specific moment in time, we can work with the moment of that event as usual.
  • Beyond Mark(Instant.MAX), all events happen simultaneously, and Mark(Instant.MAX) is infinitely far from Mark(Instant.MAX - epsilon). So, Mark(Instant.MAX) can be thought of as some undefined moment in the infinitely-far region. Duration behaves consistently: since that moment is undefined but infinitely far, we don't know anything about it, and adding/subtracting non-infinite durations stays in the undefined region, whereas adding conflicting infinities is erroneous.

Personally, after thinking about it, I like the Mark() one better, I think it reflects the intuition more nicely. WDYT about unifying the behaviors of these concepts? If there's any particular reason why they have to have different concepts of infinity, then sure, use cases are the kings, we'll have to live with the discrepancy, but I just couldn't think of any.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In InstantTimeMark, the values Instant.MIN and Instant.MAX are chosen to represent infinitely distant time marks. Thus they are sacrificed for that and we can't have a time mark that corresponds exactly to the Instant.MIN/MAX. Technically we could avoid that by introducing some flag to represent infinite saturation, but I don't see much value in that, given that Instant.MIN/MAX are just our internal constants (different on different platforms) and they would hardly be used in practice for something meaningful.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically we could avoid that by introducing some flag to represent infinite saturation, but I don't see much value in that

Yeah, me neither. What I see value in, is making sure Instant and InstantTimeMark work the same way, probably by implementing the operations on Instant to follow suit. With your clarification that Mark(Instant.MAX) also follows the model of "all events in the far future collapse into one," the difference becomes just that Instant considers Instant.MAX to be that last event, whereas Mark considers some infinitely far-away event to be the last one, even though it's represented as Instant.MAX. I don't understand why this distinction is worth introducing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instant considers Instant.MAX to be that last event, whereas Mark considers some infinitely far-away event to be the last one, even though it's represented as Instant.MAX. I don't understand why this distinction is worth introducing.

It's expected that timeMark + Duration.INFINITE returns -Duration.INFINITE when queried for elapsedNow, which would not be possible if InstantTimeMark saturated to some finite moment, like Instant.MAX. Thus we have to either give the infinite meaning to some values of Instant, or to encode infiniteness with some additional flag.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's expected that timeMark + Duration.INFINITE returns -Duration.INFINITE when queried for elapsedNow

By the same logic, wouldn't we also expect the following to be true?

assertEquals(-Duration.INFINITE, instant - (instant + Duration.INFINITE))

Thus we have to either give the infinite meaning to some values of Instant

Sure, why not consider Instant.MAX a "truly" infinite value?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is out of scope of this PR


assertFailsWith<IllegalArgumentException> { markFuture - Duration.INFINITE }
assertFailsWith<IllegalArgumentException> { markPast + Duration.INFINITE }
}
}