Support microsecond precision in realTime and realTimeInstant on the JVM#2416
Conversation
|
I guess there needs to be a change made in SyncIO also but I'm not sure what that is. |
|
|
||
| def nowMillis(): Long | ||
|
|
||
| def nowMicros(): Long |
There was a problem hiding this comment.
This will need a default implementation similar to the one for Scala.js, otherwise this is a binary incompatible change.
There was a problem hiding this comment.
Just for my understanding would you mind pointing to the default implementation for Scala.js?
There was a problem hiding this comment.
You wrote it yourself, nowMillis() * 1000. 😁
There was a problem hiding this comment.
Ah, I see what you mean now 😅 It needs a default implementation to ensure backwards compatibility with existing implementations. Thanks
There was a problem hiding this comment.
Exactly, and because of this we end up with a method that can only make a best-effort to provide microsecond precision with no real guarantee.
| self.applicative.map(self.realTime)(d => Instant.ofEpochMilli(d.toMillis)) | ||
| } | ||
| def realTimeInstant: F[Instant] = | ||
| self.applicative.map(self.realTime)(d => Instant.EPOCH.plusNanos(d.toNanos)) |
There was a problem hiding this comment.
Nanos since the epoch, seems like a tall ask, but I'm not really familiar with time APIs, so I might be wrong.
|
For JS side of things, just want to point out this:
Besides @vasilmkd's comments above, here is my concern/confusion: IIUC we can't change the semantics of It seems to me a better way to accomplish this change would be to introduce a new typeclass e.g. |
|
Thanks for the comments @vasilmkd & @armanbilge My intention with this PR was to port the changes that were made in CE2 (#1461). As it seems this change will require more planning I'm happy if you would like to close this PR and start a discussion in the issues? |
dbd1de7 to
38b2f53
Compare
|
If you dig through the sources for We should provide this functionality in Edit: @brendanmaguire If you're interested in pushing on this, I would recommend starting from the following signatures (which can be added to // should return the delta between nanoTime and currentTimeMillis
// held out as a separate function so implementations can override it with a more accurate/faster version
def realNanoOffset: F[FiniteDuration] = ???
def fastRealTime: F[F[FiniteDuration]] = ???Bikeshed the names. I think you'll need to add the latter to |
|
@djspiewak Thanks for the detailed comment 🙂 I could be misunderstanding here but from looking at the Edit: Ah, unless your suggestion is to do this natively in both JS and the JVM? |
|
Interested to hear your thoughts on the above @djspiewak . I'm happy to take on this task but I'm not sure what is required at the moment. |
You might be right about this. We need to investigate a bit more. |
|
This came up again in typelevel/natchez#427 (comment), again to do with tracing. Any ideas to unblock this? What about my |
|
To aid discussion, I'm pretty sure this is what @djspiewak was proposing above in #2416 (comment). Unless I completely misunderstood something, actually this can be implemented etnirely with /**
* Calculates the offset between the real time and the monotonic nanotime, i.e.:
* realNanoOffset = realTime - monotonic.
*/
def realNanoOffset: F[FiniteDuration] =
applicative.map2(realTime, monotonic)(_ - _)
/**
* A representation of the current system time with up to nanosecond accuracy.
*/
def fastRealTime: F[F[FiniteDuration]] =
applicative.map(realNanoOffset)(offset => applicative.map(monotonic)(_ + offset))Personally, I'm not crazy about this approach (tl;dr I think it's annoying to implement and annoying to use), and I just proposed a different strategy in #2529. Edit: just to expand my tl;dr a bit (all IMHO, and of course open to be convinced otherwise):
|
|
One more additional followup question. Daniel mentioned that his offset strategy is
Like, why is that? At least according to this So it's not obvious to me what we'd gain with |
|
@brendanmaguire Okay so looping back to this! Finally! :-P After a lot of thought and things, new plan. Let me know if you'd be interested in implementing the following:
That last one could just be a separate PR. Anyway, wdyt? Wanted to give you first crack at it since you've been patiently waiting for a review for more than six months. |
|
Hey @djspiewak . Thanks for getting back to this 🙂 The approach sounds good 👍 Yes, I'd be happy to give this a go over the coming week, time permitting. If you would like it sooner then please feel free to hand it off to someone else. |
|
@brendanmaguire Go for it! |
…the JVM * Use `java.time.Instant` to provide microsecond precision for `nowMicros` * Use `nowMicros` instead of `nowMillis` to determine the real time * Provide default implementation of `Scheduler#nowMicros` This is a follow up to typelevel#1461
38b2f53 to
b58ae18
Compare
|
Just getting to look at this now. I think my original PR pretty much implements what you're suggesting in your first two bullet points @djspiewak 🤔 I've rebased on top of the latest |
djspiewak
left a comment
There was a problem hiding this comment.
This looks correct to me! Thank you! So we can merge this and then a second PR can take on the FiniteDuration => F[F[FiniteDuration]] function.
Thanks @djspiewak 🙂
I'm afraid I don't fully understand what's required for this. I'm happy to try it if you could give further guidance on what's involved, or if someone else wants to take it on that's cool with me either. |
So here's the idea… The advantage to calling We can leverage this to make a much, much faster version of The only thing tricky here is that there might be an autoyield between calls, which could mean the offset is wildly off. #2633 will fix this, but for the time being you can work around it by simply computing the offset twice consecutively and taking the Anyway, the signature of this function should be something like |
|
Thanks for the detailed explanation @djspiewak . Makes sense to me now. I will give it a go during the week. |
|
Hey @djspiewak . I took a look at the I'm not sure how to go about updating the Is there an approach here I'm missing? Thanks! |
|
@brendanmaguire |
Great! Thanks @armanbilge 🙂 |
java.time.Instantto provide microsecond precision fornowMicrosnowMicrosinstead ofnowMillisto determine the real timeThis is a follow up to #1461