Kotlin / kotlinx-datetime

KotlinX multiplatform date/time library
Apache License 2.0
2.39k stars 99 forks source link

FR: Use inline class for Instant #121

Open bubenheimer opened 3 years ago

bubenheimer commented 3 years ago

When initially looking at this library I was hoping to find an implementation of Instant that just wraps a Long or Double with an inline class, similar to the implementation of Duration. I would like to replace plain Long millisecond values in web server code where I cannot compromise on the memory footprint needed for these values. My API needs are basic, essentially "ms since epoch" (or possibly: ms since an arbitrary more recent reference point), adding/subtracting Durations, getting time deltas as Duration, plus basic logging for debug & monitoring purposes.

dkhalanskyjb commented 3 years ago

ms since an arbitrary more recent reference point

"Arbitrary" as in chosen by us, the library writers, or by you? If it's the latter, then Duration is a much better fit for your use case than Instant, which can be thought of as a Duration whose beginning is fixed to be the start of the epoch.

Also, Duration values can be added to and subtracted from each other, so it seems like all the needs you listed (aside from logging) are met by Duration.

bubenheimer commented 3 years ago

Thanks, those are valid points regarding the more hypothetical "relative time" use case, I did mean arbitrary reference point chosen by my own code. On that note, if I did decide to go down this route at some point, I would want to use Int as the underlying base type for millisecond-precision durations up to about 49 days, which is sufficient for my use case. I cannot do this with the Duration type.

My primary use case at this time remains a Long/Double-wrapping inline Instant class; the alternative relative time option is a somewhat radical optimization to my present codebase, so I'm not planning to pursue it any time soon, if ever.

As a stopgap I've created my own CompactInstant inline wrapper class around a msSinceEpoch Long for now, but I'm still pulling in kotlinx-datetime's Instant class for a human-readable toString() in a multiplatform environment. I'm still interested in a genuine kotlinx-datetime CompactInstant implementation.

dkhalanskyjb commented 3 years ago

[...] I would want to use Int as the underlying base type for millisecond-precision durations up to about 49 days, which is sufficient for my use case. I cannot do this with the Duration type.

Could you explain why not? Duration wraps a Long and supports far more than 49 days in millisecond precision.

I've created my own CompactInstant inline wrapper class around a msSinceEpoch Long for now

But how is it different from a Duration, except for toString() implementation which could be added under a different name as an extension function for Duration?

bubenheimer commented 3 years ago

Long vs. Int: the motivation for this feature request is memory footprint. Int has half the memory footprint of Duration's Long (on many platforms). My use case is limited to durations of no more than one or two days, and I do not need nanosecond precision, so Int would be a good choice.

Using a CompactInstant type to represent absolute points in time is much easier to work with than Duration; the whole programming world revolves around msSinceEpoch, the effort of departing from this model on my own is difficult to contain in a solo project - it would create significant additional code complexity plus maintenance burdens and make code less reusable. In particular, all relative timepoints now need a reference point to be valid, and this reference point cannot be stored in or referenced from a duration inline class. The reference point marks the start of a user session and remains valid for the duration of the session, but each session has a different reference point, so I'd have to carry around the reference point to wherever I need to transform the relative duration to an absolute timepoint.

ilya-g commented 3 years ago

We'd rather avoid introducing another type for representing instants into kotlinx-datetime because doing so would not only inflate the API surface significantly, but I believe would also make using the library harder: the users would constantly have to decide whether they should use Instant or CompactInstant in their code.

bubenheimer commented 3 years ago

Could it make sense to provide some of the general-purpose date/time APIs for usage with epoch-based milliseconds without the extra step of a throwaway CompactInstant conversion? And keep those APIs in some separate "core" library artifact, so it wouldn't be necessary to introduce the full datetime dependencies when almost none of it is used?

In my KMP project I have to pull in kotlinx-datetime just to log some human-readable debug timestamps from a ms-since-epoch value; alternatively I could introduce platform-specific dependencies, but that seems even worse.

OrhanTozan commented 2 years ago

@bubenheimer https://github.com/korlibs/klock has what you are looking for.

dkhalanskyjb commented 2 years ago

@OrhanTozan, are you talking about the DateTime class there https://github.com/korlibs/klock/blob/master/klock/src/commonMain/kotlin/com/soywiz/klock/DateTime.kt? Doesn't look like it supports working with Duration, which is more or less the only feature requested here.

dkhalanskyjb commented 2 years ago

@bubenheimer, it looks like your needs can be easily satisfied with just a tiny wrapper around Duration that only calls our library for pretty printing. Allocating an Instant for printing should not be a big deal, given how the resulting string takes up even more bytes.

@JvmInline
public value class CompactInstant(public val sinceEpoch: Duration): Comparable<CompactInstant> {

    public companion object {
        public fun fromEpochMilliseconds(epochMilliseconds: Long): CompactInstant =
            CompactInstant(epochMilliseconds.milliseconds)
    }

    private val kotlinxInstant: Instant
        get() = Instant.fromEpochMilliseconds(0) + sinceEpoch

    override fun toString(): String = kotlinxInstant.toString()

    public operator fun plus(other: Duration): CompactInstant = CompactInstant(sinceEpoch.plus(other))

    public operator fun minus(other: Duration): CompactInstant = CompactInstant(sinceEpoch.minus(other))

    public operator fun minus(other: CompactInstant): Duration = sinceEpoch.minus(other.sinceEpoch)

    override fun compareTo(other: CompactInstant): Int = sinceEpoch.compareTo(other.sinceEpoch)
}

Tested like this:

        val instant1 = CompactInstant.fromEpochMilliseconds(1637843185123L)
        val instant2 = CompactInstant.fromEpochMilliseconds(1444443185123L)
        assertEquals("2021-11-25T12:26:25.123Z", instant1.toString())
        assertEquals("2015-10-10T02:13:05.123Z", instant2.toString())
        assertTrue(instant2 < instant1)
        assertFalse(instant2 > instant1)
        assertNotEquals(instant2, instant1)
        assertTrue(instant2 != instant1)
        assertEquals(instant2, instant2)

In my opinion, if something in its entirety takes about 20 straightforward lines of code, it's better to just copy-paste the definition into the project and not include any library: this way, adding new operations, removing old ones, and in general iterating on the thing is more straightforward. EDIT: for example, for eventually replacing the Long duration with an Int. :)

What do you think?

dkhalanskyjb commented 2 years ago

Missed another portion of your request.

Could it make sense to provide some of the general-purpose date/time APIs for usage with epoch-based milliseconds without the extra step of a throwaway CompactInstant conversion?

Do you mean, just a separate library artifact that only contained a function to convert the number of epoch milliseconds into an ISO-8601 string? All the other things you've requested are more or less just + and - operations.

For formatting, Instant internally uses LocalDateTime, which, in turn, uses LocalDate. So, in fact, a good chunk of the library is being used just for this string conversion.

bubenheimer commented 2 years ago

@OrhanTozan thank you for the pointer.

@dkhalanskyjb:

Do you mean, just a separate library artifact that only contained a function to convert the number of epoch milliseconds into an ISO-8601 string? All the other things you've requested are more or less just + and - operations.

Yes, I was thinking of it as foundational functionality (a core artifact or utility artifact), that other datetime artifacts would use. But from what you're saying that's not how the code is structured, unless it would be appropriate to move LocalDateTime & LocalDate into this core artifact. All I want here is write a human-readable date/time string to Android logcat and/or a bug tracker for troubleshooting; it's hard to justify pulling in the entire datetime library only for this, but I can't easily and automatically post-process those types of logs, either.

Also, thank you for coming up with that code gist, it is interesting to see the Duration-based wrapper, I may switch to it at some point.

bubenheimer commented 2 years ago

My needs for Java-agnostic datetime handling may expand over time, that would provide reasonable justification for incorporating the full datetime artificat.