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

Better timestamp logic #3031

Merged
merged 8 commits into from
Feb 19, 2021
Merged

Better timestamp logic #3031

merged 8 commits into from
Feb 19, 2021

Conversation

NelsonVides
Copy link
Collaborator

@NelsonVides NelsonVides commented Feb 10, 2021

Just converting some of those ugly triplets-of-deathtimestamp things erlang used to do. Just generate everything with smaller and faster system_time/0,1 calls.

Just converting some of those ugly triplets-of-deathtimestamp things erlang used to do. Just generate everything with smaller and faster system_time/0,1 calls.

TL;DR: everything is extensively explained here: https://erlang.org/doc/apps/erts/time_correction.html

TL;DR;2:

  • erlang:monotonic_time/0,1 for measuring time differences. It's what receive ... after and all the timer bifs are using for example. Scalable, always monotonic, non-locking, self-adjusting, etc. Better precision by using /0, that is, in native units, whatever that means for the current erlang runtime, but fine to specify a human-friendly unit if it makes code simpler.
  • erlang:system_time/0,1 for timestamps that don't warp or warp gently according to the runtime's configuration.
  • os:system_time/0,1 for timestamps that are going somewhere outside of this system, like sending them to a client. The OS has a better view of hardware clocks or NTP syncs. There's no control on warping though.

What's the point: system_time and monotonic_time are two completely different units of measure. Most operating systems have some kind of monotonic clock, either in software or by having a (not as precise as a super-expensive atomic clock) hardware device. Then, periodically, the OS syncs over the internet with the Network Time Protocol, hence reflecting the real system-time. In practice, this means than the system time can change in unpredictable ways. A POSIX day is always 86400 seconds long, but UTC days can be 86400, or 86401, or 86399: this knowledge is adjusted through NTP. So a view of the system time can in theory arbitrarily jump back and forth for adjustments, and two measures done exactly one second apart can report any positive or negative number for their difference.

What are all the time functions? Basically, the most important is erlang:monotonic_time() :: integer(), it is the "time engine" for the whole BEAM. All the receive ... after, the BIF timers, and all other timeouts, are actually measured using erlang:monotonic_time(). It is internal to the current runtime (two different runtimes will have completely different start times). It is always monotonic and unshared across threads, and it periodically corrects (unless disabled, which you should basically never do these days). It's all you need, and your best bet, for measuring time differences: the fastest to query, and to compute with, as it is just a 64bits int, and not requiring slow system calls. It is periodically adjusted by the runtime system with regards to the system time, OS time, NTP, and everything else, in very complex ways.

Then there's erlang:system_time, which basically means erlang's point of view of POSIX, with all its internal adjustments, but periodically adjusted to track the OS system time, as the OS has a better view of hardware clocks and NTP. Has correction and warp configurations to adjust that, and adjusts gently to the OS time. Good enough for measuring time diffs and setting up timestamps.

Then even more precise (but not necessarily with more resolution), there's the equivalent OS monotonic time and system time, which are system calls, and most importantly, the erlang runtime has no control on how these warp. They can arbitrarily jump back and forth, or only forth in the case of the monotonic variant, according to the OS adjustments, in very imprecise intervals. Very bad for measuring time diffs.

Corrections and time warps also nicely explained here: https://learnyousomeerlang.com/time (maybe we should add the multi-warp flag to our vm.config?)

@mongoose-im

This comment has been minimized.

Copy link
Member

@chrzaszcz chrzaszcz left a comment

Choose a reason for hiding this comment

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

My feelings are mixed about this changeset.
Firstly, it's great to get rid of erlang:timestamp().

But... after the changes I am seeing the following:
erlang:monotonic_time(microsecond)
erlang:system_time(microsecond)
erlang:system_time()

I would like to use only one whenever possible with clear rule when to use the monotonic variant. Otherwise it looks like a bunch of adhoc changes.

@NelsonVides
Copy link
Collaborator Author

My feelings are mixed about this changeset.
Firstly, it's great to get rid of erlang:timestamp().

But... after the changes I am seeing the following:
erlang:monotonic_time(microsecond)
erlang:system_time(microsecond)
erlang:system_time()

I would like to use only one whenever possible with clear rule when to use the monotonic variant. Otherwise it looks like a bunch of adhoc changes.

Seems legit, I'll have a look and explain why each is used and if some could be unified 👍🏽

@chrzaszcz
Copy link
Member

chrzaszcz commented Feb 11, 2021

Seems legit, I'll have a look and explain why each is used and if some could be unified 👍🏽

If you already have a set of your own rules for doing the changes, share them as it is difficult to figure out the motivation of the changes, e.g.

  • Change os:timestamp to erlang:system_time(...) in this and that situation - just a few bullet points might be enough, like when to use monotonic and when not.

So... I just want to make it clear that I wanted to say it's difficult to figure out the motivation and reasoning - not that there was no motivation or reasoning behind it.

I think it would be good to use the same reasoning for unifying all timestamp logic, as I am assuming there is much more to cover? maybe we could have a story with the motivation described in a bit more detail?

@NelsonVides
Copy link
Collaborator Author

Seems legit, I'll have a look and explain why each is used and if some could be unified 👍🏽

If you already have a set of your own rules for doing the changes, share them as it is difficult to figure out the motivation of the changes, e.g.

* Change `os:timestamp` to `erlang:system_time(...)` in this and that situation - just a few bullet points might be enough, like when to use monotonic and when not.

So... I just want to make it clear that I wanted to say it's difficult to figure out the motivation and reasoning - not that there was no motivation or reasoning behind it.

I think it would be good to use the same reasoning for unifying all timestamp logic, as I am assuming there is much more to cover? maybe we could have a story with the motivation described in a bit more detail?

Will surely prepare all of that when I have a bit of time, it's actually an interesting exercise to write it down 🙂 👍🏽

@NelsonVides
Copy link
Collaborator Author

Seems legit, I'll have a look and explain why each is used and if some could be unified 👍🏽

If you already have a set of your own rules for doing the changes, share them as it is difficult to figure out the motivation of the changes, e.g.

* Change `os:timestamp` to `erlang:system_time(...)` in this and that situation - just a few bullet points might be enough, like when to use monotonic and when not.

So... I just want to make it clear that I wanted to say it's difficult to figure out the motivation and reasoning - not that there was no motivation or reasoning behind it.

I think it would be good to use the same reasoning for unifying all timestamp logic, as I am assuming there is much more to cover? maybe we could have a story with the motivation described in a bit more detail?

@chrzaszcz Description of the PR has been updated, when merging all that big message can be added to the merge commit, so that it's part of the git history.

When it comes to a general unification of all timestamp logic, hmm... it's a bit of an investigation to find out what other parts of MIM need to improve their timestamps, I wouldn't like this PR to be blocked by that 😕

@esl esl deleted a comment from mongoose-im Feb 16, 2021
@NelsonVides
Copy link
Collaborator Author

Nevertheless, maybe I'll just try to get rid of usec, there's a ticket for that in our Jira 🤔

@mongoose-im

This comment has been minimized.

@NelsonVides
Copy link
Collaborator Author

Damn, the PR message messed up. Now it's done, and CI is passing, and usec is out.
@chrzaszcz

Copy link
Member

@chrzaszcz chrzaszcz left a comment

Choose a reason for hiding this comment

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

Great work, just a small comment from me.

src/pubsub/mod_pubsub.erl Outdated Show resolved Hide resolved
@mongoose-im
Copy link
Collaborator

9099.1 / Erlang 23.0.3 / small_tests / 0b87486
Reports root / small

@chrzaszcz chrzaszcz merged commit c1f658b into master Feb 19, 2021
@chrzaszcz chrzaszcz deleted the better_timestamp_logic branch February 19, 2021 16:49
@leszke leszke added this to the 4.2.0 milestone Apr 16, 2021
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

Successfully merging this pull request may close these issues.

4 participants