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

Timestamps should use currentTimeMillis #822

Merged
merged 1 commit into from Jul 10, 2019

Conversation

@tgodzik
Copy link
Collaborator

commented Jul 9, 2019

nanoTime is not actually time in nanos from 1970, but is used to measure difference. So all timestamps would be quite random since they can repeat themselves. We can't use them as timestamps for sure.

Also add pom.xml to be watched, since I must have forgot about it previously.

/**
* Wrapper around `System.nanoTime` to allow easier testing.
*/
trait Time {
def nanos(): Long
final def millis(): Long = TimeUnit.NANOSECONDS.toMillis(nanos())
final def millis(): Long = System.currentTimeMillis()

This comment has been minimized.

Copy link
@gabro

gabro Jul 10, 2019

Member

Beware: this is not related to the nanos method anymore, which I think it was the intention

This comment has been minimized.

Copy link
@ivanopagano

ivanopagano Jul 10, 2019

You should check how the Time trait is used for testing, since that seems to be the original intent.
As @gabro points out, the idea might be that millis() is coupled to nanos() for testing purposes, and the Time is just a commodity util over nanotimes...

If that's the case, I'd suggest renaming millis() to something more explicit like asMillis() and document that.
Or even better, create an additional millisTimestamp() that doesn't relates to nanos() in any way but explicitly states it's getting some form of system timestamp...
On top of that you could rename nanos -> monotonicNanos, millis -> monotonicMillis?

Just wild ideas

This comment has been minimized.

Copy link
@tgodzik

tgodzik Jul 10, 2019

Author Collaborator

Millis were always used as a timestamp and nanos for timing, which is correct. The only thing is that there is no actual relation between nanoTime and current time.

Fixed it in the tests.

This comment has been minimized.

Copy link
@marek1840

marek1840 Jul 10, 2019

Collaborator

To be honest, I don't see the benefit of using nanos here.
There is one place where nanos are used directly: to calculate the StatusBar priority needed for sorting the statuses. I would argue that using milliseconds here should not be a noticeable change.

Therefore I would like to propose dropping the support for nanoseconds and using milliseconds

This comment has been minimized.

Copy link
@marek1840

marek1840 Jul 10, 2019

Collaborator

Of course there is another solution - never ever using this method for creating timestamps, but then that is an additional complexity

This comment has been minimized.

Copy link
@tgodzik

tgodzik Jul 10, 2019

Author Collaborator

I think we can leave nanos for StatusBar priority. I will rename millis to currentMillis to indicate the difference.

@tgodzik tgodzik force-pushed the tgodzik:fix-timestamps branch from 8b65134 to 3e7ebb4 Jul 10, 2019
*/
trait Time {
def nanos(): Long
final def millis(): Long = TimeUnit.NANOSECONDS.toMillis(nanos())
def millis(): Long
}

object Time {
object system extends Time {
def nanos(): Long = System.nanoTime()

This comment has been minimized.

Copy link
@marek1840

marek1840 Jul 10, 2019

Collaborator

We can also include the "absolute time base" for proper calculation of the elapsed nanoseconds:

Suggested change
def nanos(): Long = System.nanoTime()
private val absoluteTimeBaseNanos: Long = {
val now = Instant.now()
val seconds = TimeUnit.SECONDS.toNanos(now.getEpochSecond)
val nanos = now.getNano
seconds + nanos
}
private val relativeTimeBaseNanos = System.nanoTime()
def nanos(): Long = {
absoluteTimeBaseNanos + elapsed
}
private def elapsed: Long = System.nanoTime() - relativeTimeBaseNanos

Note, that this does not handle the time zones at all. Probably it is safe to assume the time of the server as the valid one. At least for now

This comment has been minimized.

Copy link
@tgodzik

tgodzik Jul 10, 2019

Author Collaborator

I think it might be too much complexity for now.

…not give use an actual time

Also add pom.xml to be watched.
@tgodzik tgodzik force-pushed the tgodzik:fix-timestamps branch from 3e7ebb4 to f20994d Jul 10, 2019
@tgodzik tgodzik requested a review from gabro Jul 10, 2019
@gabro
gabro approved these changes Jul 10, 2019
Copy link
Member

left a comment

👍 LGTM

@tgodzik tgodzik merged commit eba18cd into scalameta:master Jul 10, 2019
2 of 3 checks passed
2 of 3 checks passed
scalameta.metals Build #20190710.3 failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@tgodzik tgodzik deleted the tgodzik:fix-timestamps branch Jul 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.