Implemented erlang:bump_reductions nif. #1813
Conversation
There was a problem hiding this comment.
Thank you for this!
I wonder if we could take the opposite direction. In AtomVM, the reduction count is decremented and not incremented as on BEAM (or at least as bump_reductions/1 and process_info/2 process it). The first function could decrement the internal reduction count while the second could return the max - the current value. What do you think?
| @@ -272,7 +273,8 @@ send_after(Time, Dest, Msg) -> | |||
| (Pid :: pid(), message_queue_len) -> {message_queue_len, non_neg_integer()}; | |||
| (Pid :: pid(), memory) -> {memory, non_neg_integer()}; | |||
| (Pid :: pid(), links) -> {links, [pid()]}; | |||
| (Pid :: pid(), monitored_by) -> {monitored_by, [pid() | resource() | port()]}. | |||
| (Pid :: pid(), monitored_by) -> {monitored_by, [pid() | resource() | port()]}; | |||
| (Pid :: pid(), reductions) -> {reductions, [pos_integer()]}. | |||
There was a problem hiding this comment.
Are you sure about the spec? Reading your code it seems the function returns {reductions, non_neg_integer()}, following Erlang/OTP, instead of {reductions, [pos_integer()]}.
There was a problem hiding this comment.
Yes, I guess there should be non_neg_integer() just like in the OTP.
| %% @end | ||
| %%----------------------------------------------------------------------------- | ||
| -spec bump_reductions(pos_integer()) -> true. | ||
| bump_reductions(Reductions) when is_integer(Reductions), Reductions > 1 -> |
There was a problem hiding this comment.
Erlang/OTP seems to allow bump_reductions(1)
There was a problem hiding this comment.
Yes - I will look into it.
| @@ -50,6 +50,7 @@ X(PORT_COUNT_ATOM, "\xA", "port_count") | |||
| X(ATOM_COUNT_ATOM, "\xA", "atom_count") | |||
| X(SYSTEM_ARCHITECTURE_ATOM, "\x13", "system_architecture") | |||
| X(WORDSIZE_ATOM, "\x8", "wordsize") | |||
| X(REDUCTIONS_ATOM, "\xA", "reductions") | |||
There was a problem hiding this comment.
Usually we add new atoms at the end, but I'm not sure we really have a convention here, adding in the middle reduces merge conflicts. However, the jit compiler depends on some values and it may break it.
There was a problem hiding this comment.
I think if it reduces merge conflicts I would leave it as it is.
| { | ||
| UNUSED(argc); | ||
| VALIDATE_VALUE(argv[0], term_is_integer); | ||
| int64_t reductions_to_bump = term_to_int(argv[0]) - 1; |
There was a problem hiding this comment.
I'm not sure why we do -1 here.
There was a problem hiding this comment.
I think its because bump_reductions is creating 1 reduction by itself just by being called.
| @@ -1767,7 +1767,7 @@ HOT_FUNC int scheduler_entry_point(GlobalContext *glb) | |||
| Module *prev_mod; | |||
| term *x_regs; | |||
| const uint8_t *pc; | |||
| int remaining_reductions; | |||
| uint64_t max_reductions; | |||
There was a problem hiding this comment.
On 32 bits platforms it's going to be a problem. Why this change?
There was a problem hiding this comment.
I see, will be reverted to int.
| UNUSED(argc); | ||
| VALIDATE_VALUE(argv[0], term_is_integer); | ||
| int64_t reductions_to_bump = term_to_int(argv[0]) - 1; | ||
| ctx->reductions += reductions_to_bump; |
There was a problem hiding this comment.
We should ensure we don't overflow, whatever the width of this field is.
| @@ -1864,8 +1864,8 @@ HOT_FUNC int scheduler_entry_point(GlobalContext *glb) | |||
| #ifdef IMPL_EXECUTE_LOOP | |||
| ctx->cp = module_address(mod->module_index, pc - code); | |||
|
|
|||
| remaining_reductions--; | |||
| if (LIKELY(remaining_reductions)) { | |||
| ++ctx->reductions; | |||
There was a problem hiding this comment.
This is going to slow us down as opposed to keeping the value in a local variable.
There was a problem hiding this comment.
Local variable from opcodesswitch is unobtainable in nif. Also every context should have its own reductions counter if we want the reductions to work exactly like the OTP version.
These changes are made under both the "Apache 2.0" and the "GNU Lesser General
Public License 2.1 or later" license terms (dual license).
SPDX-License-Identifier: Apache-2.0 OR LGPL-2.1-or-later