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
erts: use get_sys_now() rather than get_now() #664
Conversation
erlang:now/0 has known performance problems due to its semantics. In Erlang code one can use e.g. os:timestamp/0 instead, but the VM itself unfortunately calls get_now(), the core implementation of erlang:now/0, in a number of places. This updates three of those places to call the lighter-weight get_sys_now(), the VM equivalent of os:timestamp(), instead: - erl_db.c:fix_table_locked(): the time stamp retrieved is only used for informational/debugging features, and ideally it should not be retrieved at all by default, but there are test suites that depend on this "feature" - erl_gc.c:erts_garbage_collect() when 'long_gc' system monitoring is enabled - erl_trace.c:GET_NOW() when high-precision timestamping is not available or selected There is a fourth usage in io.c:driver_get_now(), which is called from timeout-related code in inet_drv.c, and possibly also from out-of-tree drivers. I'm not sure if this too can be changed to get_sys_now() so I left it unchanged for now. Tested with OTP 18-rc1 and the emulator test suite, no new regressions.
|
Patch has passed first testings and has been assigned to be reviewed I am a script, I am not human |
| &(tb->common.microsec)); | ||
| get_sys_now(&(tb->common.megasec), | ||
| &(tb->common.sec), | ||
| &(tb->common.microsec)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your editor probably displays tabs as 4 spaces. That's wrong for OTP.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does not, and I don't see why you would think that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh right, it's the diff that is wrong because leading tabs misaligns code by one. Nevermind. These mixed tabs and spaces still are a PITA though. :(
|
Patch has passed first testings and has been assigned to be reviewed I am a script, I am not human |
|
We are changing the time API and some of this will be obsolete. We will request additional changes once we have merged these changes to master. |
|
Anything happening on your side with this? |
Conflicts: erts/emulator/beam/erl_gc.c
|
Branch updated from current master. Dropped the changes to erl_gc.c as that code has been rewritten to use a different timing API -- I'm assuming that API doesn't have the problems get_now() has. Updated PR description accordingly. I would like to know if driver_get_now() can be changed to use get_sys_now() instead (or some other newer timing API). |
|
Patch has passed first testings and has been assigned to be reviewed I am a script, I am not human |
|
Thanks for the patch, and sorry for the late response. Replacing erlang:now() timestamps with os:timestamp() timestamps does however change the guarantees that the API gives. Since os:timestamp() isn't strictly monotonically increasing you cannot order events based on these timestamps. I'm therefore rejecting this patch. Keeping erlang:now() does however not solve the problem since it is not time warp safe. I've fixed all issues that this patch identifies by extending the APIs with optional use of monotonic time and in the case of tracing also the optional use of a strict monotonic time based on monotonic time and a unique monotonic integer. This has not been merged yet but will soon be. |
|
@rickard-green @psyeugenic Looking at OTP master now, the only remaining call to get_now seems to be the one in erl_trace.c (line 172). You might want to try to get rid of that too. Also, there's a seemingly obsolete definition of a macro GET_NOW at line 649 of erl_trace.c. |
|
The The |
erlang:now/0 has known performance problems due to its semantics.
In Erlang code one can use e.g. os:timestamp/0 instead, but the
VM itself unfortunately calls get_now(), the core implementation
of erlang:now/0, in a number of places. This updates two of
those places to call the lighter-weight get_sys_now(), the VM
equivalent of os:timestamp(), instead:
used for informational/debugging features, and ideally it should
not be retrieved at all by default, but there are test suites
that depend on this "feature"
available or selected
There is a third usage in io.c:driver_get_now(), which is called
from timeout-related code in inet_drv.c, and possibly also from
out-of-tree drivers. I'm not sure if this too can be changed to
get_sys_now() so I left it unchanged for now.
Tested with OTP 18-rc1 and the emulator test suite, no new regressions.