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

Improve scheduler's performance by eliminating sorting of nodes by their score #65396

Merged
merged 1 commit into from Jun 24, 2018

Conversation

@bsalamat
Copy link
Contributor

bsalamat commented Jun 23, 2018

What this PR does / why we need it:
Profiling scheduler, I noticed that scheduler spends a significant amount of time in sorting the nodes after we score them to find nodes with the highest score. Finding nodes with the highest score does not need sorting the array. This PR replaces the sort with a linear scan.

Eliminating the sort results in over 10% improvement in throughput of the scheduler.

Before (3 runs for 5000 nodes, scheduling 1000 pods in a cluster running 2000 pods):
BenchmarkScheduling/5000Nodes/2000Pods-12 1000 20682552 ns/op
BenchmarkScheduling/5000Nodes/2000Pods-12 1000 20464729 ns/op
BenchmarkScheduling/5000Nodes/2000Pods-12 1000 21188906 ns/op

After:
BenchmarkScheduling/5000Nodes/2000Pods-12 1000 18485866 ns/op
BenchmarkScheduling/5000Nodes/2000Pods-12 1000 18457749 ns/op
BenchmarkScheduling/5000Nodes/2000Pods-12 1000 18418200 ns/op

Release note:

Improve scheduler's performance by eliminating sorting of nodes by their score.
@stewart-yu

This comment has been minimized.

Copy link
Contributor

stewart-yu commented Jun 23, 2018

“replaces the sort with a linear scan.”, LGTM

maxScore := priorityList[0].Score
firstAfterMaxScore := sort.Search(len(priorityList), func(i int) bool { return priorityList[i].Score < maxScore })

g.lastNodeIndexLock.Lock()

This comment has been minimized.

@wgliang

wgliang Jun 23, 2018

Member

Why delete the lock action?

This comment has been minimized.

@bsalamat

bsalamat Jun 23, 2018

Author Contributor

This is called only from "ScheduleOne" which is single threaded.

This comment has been minimized.

@misterikkit

misterikkit Jun 25, 2018

Contributor

If we are (safely) removing thread safety from this function, it's worth mentioning in the function comment.

g.lastNodeIndex++
g.lastNodeIndexLock.Unlock()

This comment has been minimized.

@yastij

yastij Jun 23, 2018

Member

@bsalamat - do we still need the lastNodeIndexLock as this removes its only usage.

This comment has been minimized.

@bsalamat

bsalamat Jun 23, 2018

Author Contributor

That's right. We don't need it anymore. Removed.

@bsalamat bsalamat force-pushed the bsalamat:sched_no_sort branch from 61cdc20 to ffc8cc2 Jun 23, 2018

@k8s-ci-robot k8s-ci-robot added size/M and removed size/S labels Jun 23, 2018

@yastij

This comment has been minimized.

Copy link
Member

yastij commented Jun 23, 2018

@bsalamat - thanks, LGTM

@yastij

This comment has been minimized.

Copy link
Member

yastij commented Jun 23, 2018

/retest

@k82cn

This comment has been minimized.

Copy link
Member

k82cn commented Jun 24, 2018

/lgtm

@k82cn

k82cn approved these changes Jun 24, 2018

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jun 24, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bsalamat, k82cn

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Jun 24, 2018

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Jun 24, 2018

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit f0311d8 into kubernetes:master Jun 24, 2018

16 of 17 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-e2e-gce-100-performance
Details
cla/linuxfoundation bsalamat authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
maxScore := priorityList[0].Score
firstAfterMaxScore := sort.Search(len(priorityList), func(i int) bool { return priorityList[i].Score < maxScore })

g.lastNodeIndexLock.Lock()

This comment has been minimized.

@misterikkit

misterikkit Jun 25, 2018

Contributor

If we are (safely) removing thread safety from this function, it's worth mentioning in the function comment.

@@ -176,23 +175,34 @@ func (g *genericScheduler) Predicates() map[string]algorithm.FitPredicate {
return g.predicates
}

// findMaxScores returns the indexes of nodes in the "priorityList" that has the highest "Score".
func findMaxScores(priorityList schedulerapi.HostPriorityList) []int {

This comment has been minimized.

@misterikkit

misterikkit Jun 25, 2018

Contributor

Any reason this returns []int instead of []HostPriority?

@@ -454,7 +454,7 @@ func TestGenericSchedulerWithExtenders(t *testing.T) {
// because of the errors from errorPredicateExtender and/or
// errorPrioritizerExtender.
predicates: map[string]algorithm.FitPredicate{"true": truePredicate},
prioritizers: []algorithm.PriorityConfig{{Map: EqualPriorityMap, Weight: 1}},
prioritizers: []algorithm.PriorityConfig{{Function: machine2Prioritizer, Weight: 1}},

This comment has been minimized.

@misterikkit

misterikkit Jun 25, 2018

Contributor

Where are Function and machine2Prioritizer defined?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.