From 12cecd07628cb28d7bc80c17bfc01ca783b8683c Mon Sep 17 00:00:00 2001 From: Raimo Niskanen Date: Thu, 17 Jan 2019 16:08:01 +0100 Subject: Write some more comments in the engine loop --- lib/stdlib/src/gen_statem.erl | 193 +++++++++++++++++++++++++++++++++--------- 1 file changed, 155 insertions(+), 38 deletions(-) (limited to 'lib/stdlib') diff --git a/lib/stdlib/src/gen_statem.erl b/lib/stdlib/src/gen_statem.erl index beb37e43e0..2c03b4fff6 100644 --- a/lib/stdlib/src/gen_statem.erl +++ b/lib/stdlib/src/gen_statem.erl @@ -815,7 +815,10 @@ format_status( T -> [T] end]. - +%% Update #params.parent only if it differs. This should not +%% be possible today (OTP-22.0), but could happen for example +%% if someone implements changing a server's parent +%% in a new sys call. -compile({inline, update_parent/2}). update_parent(P, Parent) -> case P of @@ -885,17 +888,31 @@ wakeup_from_hibernate(P, Debug, S) -> loop_receive(P, Debug, S). %%%========================================================================== -%%% State Machine engine implementation of proc_lib/gen server +%%% State Machine engine implementation on proc_lib/gen %% Server loop, consists of all loop* functions %% and detours through sys:handle_system_message/7 and proc_lib:hibernate/3 +%% +%% The loop tries to keep all temporary values in arguments +%% and takes shortcuts for ?not_sys_debug, empty lists, etc. +%% The engine state #state{} is picked apart during the loop, +%% new values are kept in arguments, and a new #state{} is +%% composed at the end of the loop. #params{} collect engine +%% state fields that rarely changes. +%% +%% The loop is optimized a bit for staying in the loop, assuming that +%% system events are rare. So a detour to sys requires re-packing +%% of the engine state. %% Entry point for system_continue/3 +%% loop(P, Debug, #state{hibernate = true} = S) -> loop_hibernate(P, Debug, S); loop(P, Debug, S) -> loop_receive(P, Debug, S). +%% Go to hibernation +%% loop_hibernate(P, Debug, S) -> %% %% Does not return but restarts process at @@ -908,6 +925,9 @@ loop_hibernate(P, Debug, S) -> %% Entry point for wakeup_from_hibernate/3 +%% +%% Receive a new process message +%% loop_receive( #params{hibernate_after = HibernateAfterTimeout} = P, Debug, S) -> %% @@ -958,6 +978,8 @@ loop_receive( loop_hibernate(P, Debug, S) end. +%% We have received an event +%% loop_receive_result(P, ?not_sys_debug = Debug, S, Event) -> %% Here is the queue of not yet handled events created Events = [], @@ -976,7 +998,8 @@ loop_receive_result( Events = [], loop_event(P, Debug_1, S, Event, Events). -%% Entry point for handling an event; received or enqueued +%% Handle one event; received or enqueued +%% loop_event( P, Debug, #state{hibernate = true} = S, Event, Events) -> %% @@ -993,8 +1016,9 @@ loop_event( loop_event(P, Debug, S, Event, Events) -> loop_event_handler(P, Debug, S, Event, Events). +%% Call the state function, eventually +%% -compile({inline, [loop_event_handler/5]}). -%% Call the state function loop_event_handler( P, Debug, #state{state_data = State_Data} = S, Event, Events) -> %% @@ -1004,6 +1028,8 @@ loop_event_handler( Q = [Event|Events], loop_callback_mode(P, Debug, S, Q, State_Data, Event). +%% Figure out the callback mode +%% loop_callback_mode( #params{callback_mode = undefined} = P, Debug, S, Q, State_Data, CallbackEvent) -> @@ -1028,6 +1054,8 @@ loop_callback_mode( loop_callback_mode(P, Debug, S, Q, State_Data, CallbackEvent) -> loop_state_callback(P, Debug, S, Q, State_Data, CallbackEvent). +%% Check the result of Module:callback_mode() +%% loop_callback_mode_result( P, Debug, S, Q, State_Data, CallbackEvent, CallbackMode, [H|T], NewCallbackMode, NewStateEnter) -> @@ -1072,7 +1100,9 @@ loop_callback_mode_result( end. -%% Make a state enter call to the state function +%% Make a state enter call to the state function, we loop back here +%% from further down if state enter calls are enabled +%% loop_state_enter( P, Debug, #state{state_data = {PrevState,_PrevData}} = S, Q, NextState_NewData, @@ -1085,6 +1115,8 @@ loop_state_enter( NextEventsR, Hibernate, TimeoutsR, Postpone, StateCall, CallbackEvent). +%% Make a state call (not state enter call) to the state function +%% loop_state_callback(P, Debug, S, Q, State_Data, CallbackEvent) -> NextEventsR = [], Hibernate = false, @@ -1134,9 +1166,7 @@ loop_state_callback( NextEventsR, Hibernate, TimeoutsR, Postpone, CallEnter, StateCall, Actions). -%% Process the result from the state function. -%% When TransOpts =:= trans_opts it was a state function call, -%% otherwise it is an option tuple and it was a state enter call. +%% Process the result from the state function %% loop_state_callback_result( P, Debug, S, Q, {State,_Data} = State_Data, @@ -1278,6 +1308,7 @@ loop_state_callback_result( end. %% Ensure that Actions are a list +%% loop_actions( P, Debug, S, Q, NextState_NewData, NextEventsR, Hibernate, TimeoutsR, Postpone, @@ -1315,6 +1346,8 @@ loop_actions( NextEventsR, Hibernate, TimeoutsR, Postpone) end. +%% Process the returned actions +%% loop_actions_list( P, Debug, S, Q, NextState_NewData, NextEventsR, Hibernate, TimeoutsR, Postpone, @@ -1389,6 +1422,8 @@ loop_actions_list( CallEnter, StateCall, Actions, Timeout) end. +%% Process a reply action +%% loop_actions_reply( P, Debug, S, Q, NextState_NewData, NextEventsR, Hibernate, TimeoutsR, Postpone, @@ -1417,6 +1452,8 @@ loop_actions_reply( Q) end. +%% Process a next_event action +%% loop_actions_next_event( P, Debug, S, Q, NextState_NewData, NextEventsR, Hibernate, TimeoutsR, Postpone, @@ -1445,7 +1482,12 @@ loop_actions_next_event( _ -> terminate( error, - {bad_state_enter_action_from_state_function, + {if + StateCall -> + bad_action_from_state_function; + true -> + bad_state_enter_action_from_state_function + end, {next_event,Type,Content}}, ?STACKTRACE(), P, Debug, S#state{ @@ -1454,6 +1496,8 @@ loop_actions_next_event( Q) end. +%% Process a timeout action, or also any unrecognized action +%% loop_actions_timeout( P, Debug, S, Q, NextState_NewData, NextEventsR, Hibernate, TimeoutsR, Postpone, @@ -1630,6 +1674,8 @@ loop_state_transition( end end. +%% State transition to the same state +%% loop_keep_state( P, Debug, #state{timers = {TimerRefs,TimeoutTypes} = Timers} = S, Events, NextState_NewData, @@ -1657,6 +1703,8 @@ loop_keep_state( Timers) end. +%% State transition to a different state +%% loop_state_change( P, Debug, S, Events, NextState_NewData, NextEventsR, Hibernate, TimeoutsR, Postponed) -> @@ -1723,6 +1771,9 @@ loop_state_change( Timers) end. +%% Continue state transition with processing of +%% inserted events and timeout events +%% loop_next_events( P, Debug, S, Events, NextState_NewData, @@ -1754,6 +1805,8 @@ loop_next_events( NextEventsR, Hibernate, TimeoutsR, Postponed, Timers, Seen, TimeoutEvents). +%% Continue state transition with processing of timeout events +%% loop_timeouts( P, Debug, S, Events, NextState_NewData, @@ -1838,48 +1891,29 @@ loop_timeouts( P, Debug, S, Events, NextState_NewData, NextEventsR, Hibernate, TimeoutsR, Postponed, - {TimerRefs,TimeoutTypes} = Timers, Seen, TimeoutEvents, + Timers, Seen, TimeoutEvents, TimeoutType, Time, TimeoutMsg, TimeoutOpts) -> %% case Time of infinity -> %% Cancel any running timer - Timers_1 = cancel_timer_by_type(TimeoutType, Timers), - loop_timeouts( + loop_timeouts_cancel( P, Debug, S, Events, NextState_NewData, NextEventsR, Hibernate, TimeoutsR, Postponed, - Timers_1, Seen#{TimeoutType => true}, TimeoutEvents); + Timers, Seen, TimeoutEvents, + TimeoutType); 0 when TimeoutOpts =:= [] -> %% Relative timeout zero %% - cancel any running timer %% handle timeout zero events later %% - %% Instead of using cancel_timer_by_type/2 here, duplicate - %% the code, so we can make the function exit differ, - %% so the common subexpression optimization does not - %% kick in and accidentally force spilling all live - %% registers for the no TimeoutType match path - case TimeoutTypes of - #{TimeoutType := TimerRef} -> - Timers_1 = - cancel_timer_by_ref_and_type( - TimerRef, TimeoutType, TimerRefs, TimeoutTypes), - loop_timeouts( - P, Debug, S, - Events, NextState_NewData, - NextEventsR, Hibernate, TimeoutsR, Postponed, - Timers_1, Seen, - [{TimeoutType,TimeoutMsg}|TimeoutEvents], - TimeoutType); - #{} -> - loop_timeouts( - P, Debug, S, - Events, NextState_NewData, - NextEventsR, Hibernate, TimeoutsR, Postponed, - Timers, Seen#{TimeoutType => true}, - [{TimeoutType,TimeoutMsg}|TimeoutEvents]) - end; + loop_timeouts_cancel( + P, Debug, S, + Events, NextState_NewData, + NextEventsR, Hibernate, TimeoutsR, Postponed, + Timers, Seen, [{TimeoutType,TimeoutMsg}|TimeoutEvents], + TimeoutType); _ -> %% (Re)start the timer TimerRef = @@ -1915,6 +1949,11 @@ loop_timeouts( end end. %% +%% Helper function for the ugly mis-optimization workaround +%% loop_timeouts_cancel below +%% +%% Do not inline! +%% loop_timeouts( P, Debug, S, Events, NextState_NewData, @@ -1928,11 +1967,83 @@ loop_timeouts( NextEventsR, Hibernate, TimeoutsR, Postponed, Timers, Seen#{TimeoutType => true}, TimeoutEvents). +%% Loop helper to cancel a timeout +%% +loop_timeouts_cancel( + P, Debug, S, + Events, NextState_NewData, + NextEventsR, Hibernate, TimeoutsR, Postponed, + {TimerRefs,TimeoutTypes} = Timers, Seen, TimeoutEvents, + TimeoutType) -> + %% Ugly mis-optimization workaround + %% + %% This function body should have been: + %% Timers_1 = cancel_timer_by_type(TimeoutType, Timers), + %% loop_timeouts( + %% P, Debug, S, + %% Events, NextState_NewData, + %% NextEventsR, Hibernate, TimeoutsR, Postponed, + %% Timers_1, Seen#{TimeoutType => true}, TimeoutEvents). + %% + %% Since cancel_timer_by_type is inlined there is a code path + %% that checks if TimeoutType is a key in the map TimeoutTypes + %% and if not simply makes Timers_1 = Timers and then does + %% the map update of Seen and loops back to loop_timeouts/12. + %% This code path does not contain any external call and the + %% map update is an instruction, so that should be a simple + %% and fast path. + %% + %% The other code path when TimeoutType is a key in the map + %% TimeoutTypes calls erlang:cancel_timer/1 which forces + %% all live registers (about 13 of them) out on the stack + %% and back again afterwards. + %% + %% Unfortunately the optimization of common subexpressions + %% sees that both these function exits are identical and + %% joins them. Then during register allocation the common + %% function exit is adapted for the code path that spills + %% all live registers to the stack. So also the simple + %% and fast path spills all live registers around its + %% map update... Bummer! + %% + %% So this workaround duplicates cancel_timer_by_type/2 here, + %% and makes the function exits differ - the slow case + %% calls a helper function above to update Seen, and + %% the fast case updates Seen in this function. This tricks + %% the common subexpression optimizer into not joining + %% these two code paths. + %% + %% So the helper function above must not be inlined, please! + %% + case TimeoutTypes of + #{TimeoutType := TimerRef} -> + Timers_1 = + cancel_timer_by_ref_and_type( + TimerRef, TimeoutType, TimerRefs, TimeoutTypes), + loop_timeouts( + P, Debug, S, + Events, NextState_NewData, + NextEventsR, Hibernate, TimeoutsR, Postponed, + Timers_1, Seen, TimeoutEvents, + TimeoutType); + #{} -> + loop_timeouts( + P, Debug, S, + Events, NextState_NewData, + NextEventsR, Hibernate, TimeoutsR, Postponed, + Timers, Seen#{TimeoutType => true}, TimeoutEvents) + end. + +%% Continue state transition with prepending timeout zero events +%% before event queue reversal i.e appending timeout zero events +%% loop_prepend_timeout_events(P, Debug, S, TimeoutEvents, EventsR) -> {Debug_1,Events_1R} = prepend_timeout_events(P, Debug, S, TimeoutEvents, EventsR), loop_done(P, Debug_1, S, Events_1R, []). +%% Place inserted events first in the event queue +%% loop_done(P, Debug, S, NextEventsR, Events) -> case NextEventsR of [] -> @@ -1945,6 +2056,9 @@ loop_done(P, Debug, S, NextEventsR, Events) -> loop_done(P, Debug, S, lists:reverse(NextEventsR, Events)) end. %% +%% State transition is done, keep looping if there are +%% enqueued events, otherwise get a new event +%% loop_done(P, Debug, S, Q) -> %%% io:format( %%% "loop_done: state_data = ~p,~n" @@ -1963,6 +2077,9 @@ loop_done(P, Debug, S, Q) -> %%--------------------------------------------------------------------------- %% Server loop helpers +%% Parse an option list for erlang:start_timer/4 to figure out +%% if the timeout will be absolute or relative +%% parse_timeout_opts_abs(Opts) -> parse_timeout_opts_abs(Opts, false). %% -- cgit v1.2.3