-
Notifications
You must be signed in to change notification settings - Fork 118
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
Round robin pool selection #45
Conversation
lib/finch/pool/strategy.ex
Outdated
@@ -0,0 +1,27 @@ | |||
defmodule Finch.Pool.Strategy do |
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.
This module should probably be private/@moduledoc false, but i felt the documentation could be useful when implementing additional strategies
lib/finch/pool.ex
Outdated
{:ok, _} = Registry.register(registry, shp, []) | ||
{:ok, {shp, opts}} | ||
def init_pool({registry, state}) do | ||
{:ok, _} = Registry.register(registry, state.shp, state.registry_value) |
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.
I realize that we will have this registry value duplicated for every single pool, but I think this is the best way to quickly look it up at the same time as the pool pid.
lib/finch/pool/round_robin.ex
Outdated
|
||
@impl true | ||
def choose_pool([{_, %{counter: counter, count: count}} | _] = pids) do | ||
index = :counters.get(counter, 1) |
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.
Unfortunately I think this does not really guarantee round robin, because you can have handle_enqueue being called multiple times before you get to choose the pool. Or you choose the pool multiple times before handle_enqueue is called and bumped. You think you would need to use ets:update_counter if you want an actual round robin.
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.
Ahh right of course :( Ok, I'll switch to the ets counter. Thanks for pointing that out!
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.
You can probably use the atomics
module that has a add_get
function
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.
That's right! I try that out
46ccef8
to
e806473
Compare
|
||
@impl true | ||
def registry_value(%{count: count}) do | ||
atomics = :atomics.new(1, []) |
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.
I think you might want this to be unsigned. I think you're relying on overflow behavior and this is going into undefined behavior as it's eventually a signed-int overflow. I could be wrong.
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.
Great catch! I am relying on the overflow behaviour, but you're right that if the value becomes negative, things will break. Thank you!
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.
AFAIK atomics
use intrinsics that guarantee proper wraparound for signed integers as well, but yeah you need to be prepared for negative numbers. Unsigned is probably simpler 👍
Change `node.pid` into `node.vm_pid` in MetadataFormatters.ELK.
I did not see any noticeable improvement from these changes in my benchmarks so I left this PR sitting around for that reason. I am definitely still interested in looking in to alternative pool selection strategies, but I will close this PR because it is so outdated. |
I used the
:atomics
module, so that means we would no longer support OTP versions < 21.2. We could fallback to an :ets counter if we feel that supporting older OTP versions is necessary. Or we could possibly just not allow this strategy in older versions