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

[low priority] [idea] Match() cache. #33580

Conversation

jayunit100
Copy link
Member

@jayunit100 jayunit100 commented Sep 27, 2016

Problem:

Match is using alot of CPU in issues like #31795 .

Solution:

Disclaimer: I havent investigated wether or not redundant calls to match are happening, but i suspect if you had 1000 nodes, and say 100 pods per node, and were scanning labels, you could have alot of string comparison churn if the pods were identical or similar. I'd need to watch the match ops @ scale to really confirm if theres any redundancy.

Anyway, Here's a possibility for optimizing Match() performance. Could be improved via xref #33572 . I'll write some tests to confirm its cache speeds things up (At least in the obvious case scenarios). The hard part will be determining in the "real world" wether or not we have a high likliehood of bursts of identical Match operations. cc @kubernetes/sig-scheduling


This change is Reviewable

@jayunit100 jayunit100 changed the title [low priority] Labels + selector cache [low priority] [idea] Match() cache. Sep 27, 2016
@jayunit100
Copy link
Member Author

note this is largely untested, will verify it some more later.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCI GKE smoke e2e failed for commit fadf5d0. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@jayunit100
Copy link
Member Author

@k8s-bot gci gke e2e test this issue #33175.

@@ -28,6 +28,8 @@ type Labels interface {

// Get returns the value for the provided label.
Get(label string) (value string)

String() string
Copy link
Member Author

Choose a reason for hiding this comment

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

this is a convenience interface update, that allows lookups in Match cache so that any Labels support hashing. should generally work since there is a String rep, but havent tested it under all conditions

mutex
env var for benchmarking
@jayunit100 jayunit100 force-pushed the sched-checkservice-affinity-match-optimization-1 branch from fadf5d0 to ce499ed Compare September 28, 2016 00:35
@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 28, 2016
@k8s-ci-robot
Copy link
Contributor

Jenkins unit/integration failed for commit ce499ed. Full PR test history.

The magic incantation to run this job again is @k8s-bot unit test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins verification failed for commit ce499ed. Full PR test history.

The magic incantation to run this job again is @k8s-bot verify test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCI GCE e2e failed for commit ce499ed. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GKE smoke e2e failed for commit ce499ed. Full PR test history.

The magic incantation to run this job again is @k8s-bot gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@jayunit100
Copy link
Member Author

jayunit100 commented Sep 28, 2016

I think for now this is a microoptimization: the Has function implementations i looked at i think are constant time , no loops. If we are going to have more complex Has implementations for label sets etc, in the future this type of cache could help. will close [temporarily at least]

@jayunit100 jayunit100 closed this Sep 28, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants