Skip to content
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

CP-52880: Avoid O(N^2) behaviour in Xapi_vdi.update_allowed_operations #6183

Draft
wants to merge 1 commit into
base: feature/perf
Choose a base branch
from

Conversation

edwintorok
Copy link
Contributor

Activate old xapi_vdi.update_allowed_operations optimization: get_internal_records_where does a linear scan currently, so operating on N VDIs is O(N^2).

Look at the VBD records directly, like before this 2013 commit which regressed it: 5097475

(We are going to optimize get_record separately so it doesn't go through serialization)

This still requires validation and measurements (e.g. using @contificate 's benchmark)

Activate old xapi_vdi.update_allowed_operations optimization: 
get_internal_records_where does a linear scan currently, so operating on N VDIs is O(N^2).

Look at the VBD records directly, like before this 2013 commit which regressed it:
5097475

(We are going to optimize get_record separately so it doesn't go through serialization)

Signed-off-by: Edwin Török <[email protected]>
@psafont
Copy link
Member

psafont commented Dec 13, 2024

So the change here is avoiding the None value? Why can't the function's signature be change to remove the optional codepath to explicitly remove the n^2 behaviour?

@robhoes
Copy link
Member

robhoes commented Dec 13, 2024

I agree with Pau: the get_internal_records_where branch is still there, but effectively made dead code?

Also, the trade-off here, which triggered the original change 11 years ago, is that if you are not no the coordinator, the get_internal_records_where call is a single round-trip over the network, where now you have a round-trip for each VBD. The network overhead may outweigh the DB handling speedup in that case. It depends whether this function may run on any host or just on the coordinator.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants