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

Add monotonic_time, system_time and unique_integer to System #4024

Merged
merged 2 commits into from
Dec 1, 2015
Merged

Add monotonic_time, system_time and unique_integer to System #4024

merged 2 commits into from
Dec 1, 2015

Conversation

whatyouhide
Copy link
Member

Implements #4018.

The implementation is pretty straightforward, I'm just not sure about the docs: should they be more specific? Should they just point to the docs of the functions in the :erlang module?

(also, is :erlang.time_unit ok in the @spec or should I replicate the type in the System module?)

end

test "system_time/0" do
assert System.system_time() > 0
Copy link
Member

Choose a reason for hiding this comment

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

The return of this function is spec'ed as integer, not pos_integer.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, right. Good catch!

@fishcakez
Copy link
Member

When using the time API I have found convert_time_unit/3 very useful, using it almost every time I've used time. The main reason is that its ideal to use native throughout readings/calculations and then convert to the desired unit at the end. Similarly to convert from a desired unit to native when configuration is read.

correction](http://www.erlang.org/doc/apps/erts/time_correction.html) in the
Erlang docs.

Inlined by the compiler into `:erlang.system_time/0`.
Copy link
Member

Choose a reason for hiding this comment

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

Don't forget to actually inline all of them. :bowtie:

@whatyouhide
Copy link
Member Author

Actually inlined all the functions. We can't use default args with system_time and monotonic_time like @fishcakez suggested because Erlang throws ArgumentErrors for those:

iex> :erlang.system_time(:native)
** (ArgumentError) ...

As for convert_time_unit/3, I think it makes sense, should I go on and add it?

@@ -148,6 +148,13 @@ inline(?system, stacktrace, 0) -> {erlang, get_stacktrace};
inline(?tuple, to_list, 1) -> {erlang, tuple_to_list};
inline(?tuple, append, 2) -> {erlang, append_element};

inline(?system, monotonic_time, 0) -> {erlang, monotonic_time};
Copy link
Member

Choose a reason for hiding this comment

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

I think you could move get_stacktrace inlining into this group as well.

Copy link
Member

Choose a reason for hiding this comment

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

It is ordered alphabetically, so it should be before tuple. :)

Copy link
Member

Choose a reason for hiding this comment

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

Then this group move there. 😄

@josevalim
Copy link
Member

+1 for convert_time_unit

@whatyouhide
Copy link
Member Author

Ok, pushed all the mentioned changes. I removed the explict "see [function] for more information" in the docs as when I say "Inlined by the compiler into fun", fun is linked automatically.

We should be good to go!

@lexmag
Copy link
Member

lexmag commented Dec 1, 2015

I'm wondering, shall we have erlang:time_offset/0 as well?
It's recommended for use when "Seed Random Number Generation With a Unique Value".

@lexmag
Copy link
Member

lexmag commented Dec 1, 2015

And more important "Determine Order of Events With Time of the Event".
It might be the only way to do so.

josevalim added a commit that referenced this pull request Dec 1, 2015
Add monotonic_time, system_time and unique_integer to System
@josevalim josevalim merged commit d314230 into elixir-lang:master Dec 1, 2015
@fishcakez
Copy link
Member

@lexmag when would use time_offset and not system_time in those cases?

@lexmag
Copy link
Member

lexmag commented Dec 1, 2015

@fishcakez to identify the order of events for example, from new time API guide:

Time = erlang:monotonic_time(),
UMI = erlang:unique_integer([monotonic]),
EventTag = {Time, UMI}

And if you are interested in the Erlang system time at the time when the event occurred you can also save the time offset before or after saving the events using erlang:time_offset/0. Erlang monotonic time added with the time offset corresponds to Erlang system time.
If you are executing in a mode where time offset may change and you want to be able to get the actual Erlang system time when the event occurred you can save the time offset as a third element in the tuple (the least significant element when comparing 3-tuples).

Thus it's needed to make {Time, UMI, Offset}.

@lexmag
Copy link
Member

lexmag commented Dec 1, 2015

You can't use system_time in that case, as offset can "travel" to any "side".

@josevalim
Copy link
Member

Let's add time_offset as well as it is related to system_time

@fishcakez
Copy link
Member

You can't use system_time in that case, as offset can "travel" to any "side".

I don't follow this bit, can you expand?

@lexmag
Copy link
Member

lexmag commented Dec 1, 2015

Having this snippet we have proper Order of Events:

Time = erlang:monotonic_time(),
UMI = erlang:unique_integer([monotonic]),
EventTag = {Time, UMI}

but it doesn't really represent time when the event occurred and thus one can use erlang:system_time() instead of erlang:monotonic_time() what is truly wrong because this system_time can perform a time warp.
The proper way is to form {erlang:monotonic_time(), UMI, erlang:system_time()}.
Indeed, in that case system_time should work, but it seems a bit redundant (and I'm afraid error-prone if people confuse the order), the third element could just be an erlang:time_offset().
It looks more explicit to me, and we have one less erlang:monotonic_time() call underneath. :bowtie:

@fishcakez
Copy link
Member

Oh sorry, I thought you were saying can not use system_time as third element but you weren't at all.

@fishcakez
Copy link
Member

It looks more explicit to me, and we have one less erlang:monotonic_time() call underneath.

I think its less explicit as reason that term is there is so we know the system time and not the offset. Though of course the optimisation is nice and makes good sense. I think we need to work on the docs to explain under each function when it is suitable to use it and with examples showing how to combine with other functions.

@whatyouhide whatyouhide deleted the time-functions-in-system branch December 1, 2015 17:33
@whatyouhide whatyouhide restored the time-functions-in-system branch December 1, 2015 17:50
@whatyouhide whatyouhide deleted the time-functions-in-system branch December 1, 2015 17:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants