uwiger / gproc

Extended process registry for Erlang
Apache License 2.0
1.07k stars 232 forks source link

fix(gproc_pool): fix `rand:uniform/1` range #193

Closed thalesmg closed 1 year ago

thalesmg commented 1 year ago

Since rand:uniform/1's return value is a number in a closed interval (0 =< N =< Sz), we shouldn't call it with the pool size + 1, otherwise we will get a non-uniform distribution of results where one of the workers receive substantially more work than all others.

To verify this, I've started a pool with 8 workers using the random load-balancing algorithm and measured the frequencies of each PID in gproc_pool.

PoolName = my_pool.
PoolSize = 8.
ok = gproc_pool:new(PoolName, random, [{size, PoolSize}]).
lists:foreach(fun(I) ->
  gproc_pool:add_worker(PoolName, {PoolName, I}, I),
  spawn(fun() ->
    true = gproc_pool:connect_worker(PoolName, {PoolName, I}),
    receive
        stop -> ok
    end
  end),
  ok
end, lists:seq(1, PoolSize)).
MeasureFreqs = fun(Pool, NumPoints) ->
  Pids = lists:map(fun(_) -> gproc_pool:pick_worker(Pool) end, lists:seq(1, NumPoints)),
  lists:foldl(
    fun(Pid, Acc) ->
      maps:update_with(Pid, fun(N) -> N + 1 end, 1, Acc)
    end,
    #{},
    Pids
   )
end.
MeasureFreqs(PoolName, 100_000).

Results prior to the fix (note how <0.172.0> has almost double the frequency of other PIDs):

 #{<0.172.0> => 22222,<0.173.0> => 11138,<0.174.0> => 11000,
  <0.175.0> => 11046,<0.176.0> => 11141,<0.177.0> => 11165,
  <0.178.0> => 11166,<0.179.0> => 11122}

After the fix:

 #{<0.172.0> => 12460,<0.173.0> => 12367,<0.174.0> => 12541,
  <0.175.0> => 12494,<0.176.0> => 12649,<0.177.0> => 12399,
  <0.178.0> => 12439,<0.179.0> => 12651}
uwiger commented 1 year ago

Thanks!

thalesmg commented 1 year ago

Hi @uwiger , do you have plans to tag a new version that would contain this fix?

We are interested in using the upstream version if possible.

Thanks! :smile_cat:

uwiger commented 1 year ago

Published version 0.9.1

thalesmg commented 1 year ago

Thank you! :beers: