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

Fix quadratic complexity in Reconciler.Reconcile(). #10989

Merged
merged 5 commits into from
Mar 10, 2022

Conversation

Tener
Copy link
Contributor

@Tener Tener commented Mar 9, 2022

Reconciler.Reconcile() is "accidentally quadratic".
For large number of resources N, the running time of this function blows up:

    reconciler_test.go:299: [N=10000] Run time 710.941583ms, time for element 71.094µs
    reconciler_test.go:299: [N=20000] Run time 1.99310675s, time for element 99.655µs
    reconciler_test.go:299: [N=30000] Run time 4.042424417s, time for element 134.747µs
    reconciler_test.go:299: [N=40000] Run time 8.069562916s, time for element 201.739µs
    reconciler_test.go:299: [N=50000] Run time 13.143397s, time for element 262.867µs
    reconciler_test.go:299: [N=100000] Run time 1m13.327041833s, time for element 733.27µs 

This complexity arises because Reconciler.Reconcile() calls both processNewResource and processRegisteredResource N times, and each of those calls currentResources.Find() which also has N complexity. Combined we arrive at NxN=N^2.

After simple fix (switching to map lookup instead of linear list scan):

    reconciler_test.go:299: [N=10000] Run time 296.669375ms, time for element 29.666µs
    reconciler_test.go:299: [N=20000] Run time 575.412375ms, time for element 28.77µs
    reconciler_test.go:299: [N=30000] Run time 840.50825ms, time for element 28.016µs
    reconciler_test.go:299: [N=40000] Run time 1.104382709s, time for element 27.609µs
    reconciler_test.go:299: [N=50000] Run time 1.369955208s, time for element 27.399µs
    reconciler_test.go:299: [N=100000] Run time 2.700222125s, time for element 27.002µs

For increased efficiency and to make it clear reconciler operates on a list of unique resources, I've changed the respective signatures to the new type types.ResourcesWithLabelsMap.

Additionally, two (unused) functions called Find() were removed due to the danger of accidentally repeating the problem in question. collection.ToMap()["foo"] should be used instead.

@Tener Tener requested review from espadolini and rosstimothy March 9, 2022 13:18
@github-actions github-actions bot added application-access database-access Database access related issues and PRs desktop-access labels Mar 9, 2022
@Tener Tener changed the title Fix quadratic Reconciler.Reconcile() complexity. Fix quadratic complexity in Reconciler.Reconcile(). Mar 9, 2022
Copy link
Contributor

@codingllama codingllama left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice.

lib/srv/db/watcher.go Outdated Show resolved Hide resolved
Copy link
Contributor

@espadolini espadolini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what @codingllama said

@@ -90,14 +90,21 @@ type ResourceWithLabels interface {
// ResourcesWithLabels is a list of labeled resources.
type ResourcesWithLabels []ResourceWithLabels
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we still need this type?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we do: it is rather popular in the codebase. I could work to replace it, but not everywhere ResourcesWithLabelsMap makes sense.

@Tener Tener merged commit 0e5e9bf into master Mar 10, 2022
@Tener Tener deleted the tener/reconciler_reconcile_complexity branch March 10, 2022 11:06
Tener added a commit that referenced this pull request Mar 10, 2022
* Fix quadratic Reconciler.Reconcile() complexity.

Co-authored-by: Alan Parra <[email protected]>
Tener added a commit that referenced this pull request Mar 10, 2022
* Fix quadratic Reconciler.Reconcile() complexity.

Co-authored-by: Alan Parra <[email protected]>
@webvictim webvictim mentioned this pull request Apr 19, 2022
@webvictim webvictim mentioned this pull request Jun 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
application-access database-access Database access related issues and PRs desktop-access
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants