CP-314126: Replace O(V²) list lookups with Map/Set in compute_restart_plan - #7233
Open
GabrielBuica wants to merge 8 commits into
Open
CP-314126: Replace O(V²) list lookups with Map/Set in compute_restart_plan#7233GabrielBuica wants to merge 8 commits into
GabrielBuica wants to merge 8 commits into
Conversation
added 8 commits
August 19, 2026 10:23
compute_restart_plan uses List.assoc and List.mem over association lists in inner loops, making the HA plan recompute O(V²). Prepare for fixing this by defining HostMap, HostSet, VMRefOrd, VMRefSet, and VMMap modules alongside the existing HostKey module. Move the existing VMRefOrd and VMMap definitions from the bottom of the file to this location to co-locate all ref-keyed container modules. No functional change. Signed-off-by: Gabriel Buica <danutgabriel.buica@citrix.com>
string_of_host calls List.assoc over all_hosts_and_snapshots on every invocation, which is O(H) per call. Build a HostMap once after sorting and use HostMap.find for O(log H) lookups. string_of_host is called inside string_of_plan which formats every entry in the restart plan, so the cumulative cost was O(V * H). Signed-off-by: Gabriel Buica <danutgabriel.buica@citrix.com>
vm_accounted_to_host calls List.mem over live_hosts to check whether a VM's resident_on or scheduled_to_be_resident_on host is live. This is O(H) per call and vm_accounted_to_host is called per VM, making the total cost O(V * H). Build a HostSet once from live_hosts and use HostSet.mem for O(log H) membership tests. Signed-off-by: Gabriel Buica <danutgabriel.buica@citrix.com>
vm_accounted_to_host and string_of_vm both call List.assoc over vms_to_ensure_running, which is O(V) per call. These are called per VM in the restart plan computation, making the total cost O(V²). Build a VMMap once after sorting and use VMMap.find for O(log V) lookups. Signed-off-by: Gabriel Buica <danutgabriel.buica@citrix.com>
agile_vms_and_memory is built by calling List.assoc over vms_and_memory for each agile VM, which is O(V) per lookup and O(V²) total. Build a VMMap from vms_and_memory and use VMMap.find for O(log V) lookups. Signed-off-by: Gabriel Buica <danutgabriel.buica@citrix.com>
vms_not_restarted filters all protected VMs using List.mem over the vms_restarted list, which is O(V) per check and O(V²) total. Build a VMRefSet from the restart plan and use VMRefSet.mem for O(log V) membership tests. Signed-off-by: Gabriel Buica <danutgabriel.buica@citrix.com>
The "Protected VMs" debug line builds an O(V) string by mapping string_of_vm over every protected VM. Printf.ksprintf always evaluates its arguments before checking whether the log level is enabled, so this work is done unconditionally. Guard it with Debug.is_disabled to skip the formatting when debug logging is off. Signed-off-by: Gabriel Buica <danutgabriel.buica@citrix.com>
…start_plan Add a benchmark that exercises plan_for_n_failures (which calls compute_restart_plan) across different pool sizes: 10 to 500 protected VMs on 3 to 8 hosts. This allows measuring the impact of the O(V²) to O(V log V) lookup optimisations in the preceding commits. Signed-off-by: Gabriel Buica <danutgabriel.buica@citrix.com>
last-genius
approved these changes
Aug 20, 2026
Contributor
There was a problem hiding this comment.
Was this a bottleneck in some large-scale testing? How frequently does this function run - does the call it's a part of have requirements on how long it takes?
In my testing, 10000 VMs took 3.17s after the optimizations (11.6s before), while 1000 VMs took just 0.09s (0.1s before), so this is definitely worth tackling if such scale is the goal - I'm not sure if the rest of the HA stack holds up then though
minglumlu
approved these changes
Aug 20, 2026
Contributor
Author
|
@last-genius it's not related with any large scale bottlenecks directly. it's part of a series of small improvements that we've noticed. |
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
3 participants
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.
compute_restart_plan used List.assoc and List.mem over association lists in inner loops, making the HA plan recomputation O(V²) where V is the number of protected VMs. This replaces
those linear scans with immutable Map and Set structures for O(V log V) overall.
Benchmark results:
│ Scenario │ Before (ns/run) │ After (ns/run) │ Speedup │
│ 3 hosts, 10 VMs │ 312,064 │ 316,296 │ ~1.0x │
│ 3 hosts, 50 VMs │ 1,830,234 │ 1,609,937 │ 1.14x │
│ 3 hosts, 100 VMs │ 4,389,669 │ 3,702,982 │ 1.19x │
│ 3 hosts, 500 VMs │ 52,277,926 │ 34,665,446 │ 1.51x │
│ 8 hosts, 100 VMs │ 4,696,515 │ 3,968,948 │ 1.18x │
│ 8 hosts, 500 VMs │ 54,391,061 │ 34,974,569 │ 1.56x │
Easier to review commit by commit.