-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Add Registry partition_by option for duplicate registries #14654
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
base: main
Are you sure you want to change the base?
Add Registry partition_by option for duplicate registries #14654
Conversation
lib/elixir/lib/registry.ex
Outdated
@@ -255,7 +256,7 @@ defmodule Registry do | |||
|
|||
defp whereis_name(registry, key) do | |||
case key_info!(registry) do | |||
{:unique, partitions, key_ets} -> | |||
{:unique, partitions, key_ets, _} -> |
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.
Instead of adding a new element, I wonder if we should introduce a new internal type. So we have :unique
, :duplicate
and :duplicate_by_key
.
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.
We could even have :duplicate_by_pid
and :duplicate_by_key
. Or, alternatively, have the type be {:duplicate, :pid | :key}
. But basically there is no need to store more information that only applies to duplicates and not unique.
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 this makes sense, considering that the new key doesn't make sense for a unique
registry. Just wondering if :duplicate
should also have an explicit alias, something like :duplicate_by_pid
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.
Yeah, I think we commented at the same time. Let's go with either:duplicate_by_pid
and :duplicate_by_key
or {:duplicate, partition_by}
. Your call. The second one will allow us to partially match when we don't care about the partition.
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.
Sounds good. I'll also make sure that :duplicate
keeps existing behaviour of :duplicate_by_pid
(or {:duplicate, :pid}
})
…t of {:duplicate, startegy}
@josevalim I updated it to use Will update the PR description shortly. |
@@ -467,7 +511,7 @@ defmodule Registry do | |||
:error | |||
end | |||
|
|||
{kind, _, _} -> | |||
{kind, _, _, _} -> |
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 claude needs to be reverted but we need to be careful with #{kind}
below, that will no longer work.
|
||
arg = | ||
{kind, registry, i, partitions, key_partition, pid_partition, listeners, compressed} |
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.
Reverting:
arg = | |
{kind, registry, i, partitions, key_partition, pid_partition, listeners, compressed} | |
arg = {kind, registry, i, partitions, key_partition, pid_partition, listeners, compressed} |
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 have dropped a few comments.
I also think we need to better parameterize the test:
- Break the current RegistryTest into Registry.UniqueTest and Registry.DuplicateTest
- Make sure the DuplicateTest is parameterized by partition count AND partition by
The new tests you added can be part of the new DuplicateTest.
Summary
This PR adds key-based partitioning for duplicate Registry entries, allowing optimization for workloads with many keys and few entries per key (e.g., many topics with few subscribers each).
Background
This feature addresses performance concerns first noticed in phoenixframework/phoenix_pubsub#198, where Registry's PID-based partitioning wasn't optimal for workloads with many topics and relatively few subscribers per topic.
Solution
Enhanced Keys API
:unique
: Traditional unique registry behavior:duplicate
: Traditional duplicate registry with PID-based partitioning (default){:duplicate, :pid}
: Explicit PID-based partitioning for duplicate registries{:duplicate, :key}
: New key-based partitioning for duplicate registriesPerformance Optimization
{:duplicate, :key}
now only need to check a single partitionChanges
lib/elixir/lib/registry.ex:
keys
typespec to include{:duplicate, :key}
and{:duplicate, :pid}
lookup/2
,values/3
,match/3
,count/1
,count_match/3
,select/2
and other functions to support both partitioning strategieslib/elixir/test/elixir/registry_test.exs:
{:duplicate, :key}
behaviorAPI Changes
New Keys Format
Usage Guidelines
:duplicate
or{:duplicate, :pid}
: When you have few keys with many entries (e.g., one topic with many subscribers){:duplicate, :key}
: When you have many keys with few entries each (e.g., many topics with few subscribers)Backward Compatibility
This change is fully backward compatible:
:duplicate
still uses PID partitioning){:duplicate, :key}
partitioningRelated
🤖 Generated with Claude Code