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

Provide constructor for Instant class that takes a Double #77

Closed
ta834n opened this issue Dec 1, 2020 · 3 comments
Closed

Provide constructor for Instant class that takes a Double #77

ta834n opened this issue Dec 1, 2020 · 3 comments

Comments

@ta834n
Copy link

ta834n commented Dec 1, 2020

Hello,
I really like using your project. I just miss one little feature:
could you provide a constructor for the Instant class that takes a timestamp of type Double (milliseconds since 1 January 1970 UTC)?

Reason: we try to avoid creating objects in our multi-platform project wherever possible, but using Long for timestamps results in object creation. A Double value can be directly mapped to a JavaScript Number value. It also feels more natural to what the JavaScript Date class does.

Thanks in advance!

@Jadarma
Copy link

Jadarma commented Dec 5, 2020

The Instant class itself is written with discrete units in mind, since they are faster to do calculations with and easier to reason about on platforms that have the concept of longs (all targets except JS). Internally, all instants hold their data as a Long, so even if a constructor would accept a double, in order to adapt it to the common definition, it will still have to do the conversion, introducing the boxing. The only alternative would be to rewrite the JS implementation to not use longs at all, which is not an option since it would make it too different from the others, so this object creation in JavaScript is a necessary evil that shouldn't be circumvented.

I would instead suggest you define your own constructor by extending the companion object, just so at least you don't have to put a .toLong() in every place where it is called:

@Suppress("NOTHING_TO_INLINE")
inline fun Instant.Companion.fromEpochMilliseconds(epochMilliseconds: Double) =
    Instant.fromEpochMilliseconds(epochMilliseconds.toLong())

val time = Instant.fromEpochMilliseconds(12345678.9)

The real question is: have you noticed any significant performance issues in your project while using the library?
If so, are you sure it is related to the boxing?

@ta834n
Copy link
Author

ta834n commented Dec 7, 2020

Hello @Jadarma ,
thank you very much for that answer.
To answer your question: I have not checked whether there are performance issues while using the library.
We have heavily used timestamps of type Long in our project but replaced them all with Double values once we measured an performance impact. As far as I know only Int values are mapped to JavaScript Number values but they cannot be used due to their reduced range.
Since we do not want to go back to Long values I have to reevaluate for which part I will use kotlinx-datetime.

@ta834n ta834n closed this as completed Dec 7, 2020
@Jadarma
Copy link

Jadarma commented Dec 7, 2020

Hello, @ta834n. Glad I could help. Just a small correction: all numeric types except Long are mapped to the JS Number, see here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants