Conversation
reconcile_remote_registry_snapshot/3 tested every existing entry of the remote node against the snapshot names with ordsets:is_element/2, which is a linear scan, so the reconciliation was quadratic. It shows when an ack_sync arrives from a node whose entries are already in the table, for example when a node is discovered twice at scope start. The names now go into a map and membership is tested with is_map_key/2. The map is built with maps:from_keys/2 from OTP 24 and with maps:from_list/1 below it. The new test case asserts the cost of reconciling a 10 000 entry snapshot in reductions.
Author
|
@ostinelli may I ask you to look if this PR makes sense for you? We are switching to syn, and in some scenarios it helps quite much. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
reconcile_remote_registry_snapshot/3builds the snapshot names withordsets:from_list/1and callsordsets:is_element/2once per entry the node already holds for that remote node, so the reconciliation is quadratic.After an ordinary join or reconnection the receiver holds no entries for that node, because they were purged when the remote scope went down, so the cost stays hidden. It shows when an
ack_syncarrives from a node whose entries are already in the table. One way to get there:init/1callsnet_kernel:monitor_nodes(true)beforehandle_continue(after_init)sendsdiscovertonodes(), so a node that connects in between getsdiscovertwice and answers with two full snapshots. The second one is compared against everything the first one inserted, inside the scope process, and everyregisteron that node waits behind it.Membership test alone, N snapshot names against N existing entries, OTP 27.3.4, median of three runs:
The fix uses a map and
is_map_key/2. The map is built withmaps:from_keys/2from OTP 24 and withmaps:from_list/1below it. The suite cannot cover that fallback, because it needspeerand so OTP 25, so I checked it by hand on OTP 23:srccompiles and the fallback is the branch compiled in.The new test case fails on the current code with 50 308 359 reductions for a 10 000 entry snapshot and passes with the fix at about 281 000. Tests pass on OTP 25 through 29 and dialyzer is clean.