Refactor supervisor, and store children in a map instead of a list #1602
Conversation
sirihansen
force-pushed
the
siri/supervisor/store-children-in-map
branch
from
3162477
to
8f03f08
Compare
sirihansen
added
enhancement
team:MW
testing
lib/stdlib/src/supervisor.erl
Outdated
| {abort,{failed_to_start_child,Id,Reason}} | ||
| end | ||
| end, | ||
| ch_map(Fun,Children). |
There was a problem hiding this comment.
I think it would be easier to understand if you changed Fun to Start and ch_map to children_map so that last
line would read children_map(Start, Children).
lib/stdlib/src/supervisor.erl
Outdated
| {update,Child#child{pid=undefined}} | ||
| end, | ||
| {ok,NChildren} = ch_map(Fun, Children), | ||
| NChildren. |
There was a problem hiding this comment.
I think it would be easier to understand if you changed Fun to Terminate and ch_map to children_map so that next to last line would read {ok, Nchildren} = children_map(Terminate, Children).
lib/stdlib/src/supervisor.erl
Outdated
| child_type = ChildType, modules = Mods}) -> | ||
| {Id, Pid, ChildType, Mods} | ||
| end, | ||
| State#state.children), |
There was a problem hiding this comment.
I think list_children could be a better name.
There was a problem hiding this comment.
Or children_to_list?? To comply with children_map|fold ...
There was a problem hiding this comment.
Why not consistency is normally a good thing :)
lib/stdlib/src/supervisor.erl
Outdated
| end, {0,0,0,0}, State#state.children), | ||
| ch_fold(fun(_Id, Child, Counts) -> | ||
| count_child(Child, Counts) | ||
| end, {0,0,0,0}, State#state.children), |
There was a problem hiding this comment.
ch_foldl -> children_fold
I think it is not unbearable long and clearer.
sirihansen
added
testing
sirihansen
changed the title
| start_children(Children, SupName) -> | ||
| Start = | ||
| fun(Id,Child) -> | ||
| case do_start_child(SupName, Child) of |
There was a problem hiding this comment.
Apologies for a comment on the style but I believe this is indented too much.
There was a problem hiding this comment.
That's actually a bit strange - it's the indentation done by the erlang emacs mode on funs... I will check why...
There was a problem hiding this comment.
Apparently, this is a correct indentation - it is like this to cover for a combination of multiple fun clauses in combination with newline before guards.. e.g.
fun(X)
when X > 0 ->
positive;
(X)
when X < 0 ->
negative;
(0) ->
zero
end
... and the main problem lies in the second clause, where it would be very strange if one indentation level was removed:
fun(X)
when X > 0 ->
positive;
(X)
when X < 0 ->
negative;
(0) ->
zero
end
| start_children([], NChildren, _SupName) -> | ||
| {ok, NChildren}. | ||
| start_children(Children, SupName) -> | ||
| Start = |
There was a problem hiding this comment.
Is there a good reason why this function is inline instead of being its own named function? It would help make stacktraces more clear, imo, if the Start function was named something like attempt_start_child/2 or something.
There was a problem hiding this comment.
No, there is no particular reason for this. I'll make it a separate function. Thanks!
There was a problem hiding this comment.
Sorry, I answered this one a bit too quickly. The reason for it being inline is that it uses the bound variable SupName. Moving it out would make the code less clean, and I don't believe that it is really justified by the gain. As is, the stack trace of a crashed child start function is as follows:
** exception exit: {shutdown,
{failed_to_start_child,1,
{'EXIT',
{undef,
[{test_gen_server,start_link,[],[]},
{supervisor,do_start_child_i,3,[{file,"supervisor.erl"},{line,365}]},
{supervisor,do_start_child,2,[{file,"supervisor.erl"},{line,351}]},
{supervisor,'-start_children/2-fun-0-',3,[{file,"supervisor.erl"},{line,335}]},
{supervisor,ch_map,4,[{file,"supervisor.erl"},{line,1148}]},
{supervisor,init_children,2,[{file,"supervisor.erl"},{line,301}]},
{gen_server,init_it,2,[{file,"gen_server.erl"},{line,369}]},
{gen_server,init_it,6,[{file,"gen_server.erl"},{line,337}]}]}}}}
which I believe is clear enough.
| {abort,{failed_to_start_child,Id,Reason}} | ||
| end | ||
| end, | ||
| children_map(Start,Children). |
There was a problem hiding this comment.
This could be children_map(fun attempt_start_child/2, Children).
There was a problem hiding this comment.
See this comment.
There was a problem hiding this comment.
Yeah I see your function closes over SupName. Thanks for considering it.
sirihansen
force-pushed
the
siri/supervisor/store-children-in-map
branch
from
53296db
to
9cdceb1
Compare
sirihansen
force-pushed
the
siri/supervisor/store-children-in-map
branch
from
9cdceb1
to
18bddf3
Compare
sverker
removed
the
testing
sirihansen
added
the
testing
sirihansen
force-pushed
the
siri/supervisor/store-children-in-map
branch
from
18bddf3
to
a03895d
Compare
sirihansen
removed
the
testing
This is based on PR-1467, but re-implemented upon a refactored version of
supervisor. The refactoring is included in this PR.For the time being, I have chosen not to remove the child ID from the child record (as suggested in the original PR), since that would only save one word per child and the current solutions gives a bit cleaner code.