Optimize random string generator to avoid multiple locks & use bit-masking

[shyamjvs]

Ref #53327

We recently started seeing a 50% decrease in scheduling throughput (for e.g in kubemark-500 scale job) and turns out #53135 introduced it.
The reason is this call to create a random 32-length string.
From the code of the rand utility (which is being heavily used throughout the system for randomizing object names), I noticed following performance issues:

• to create an n-length string, we are making n calls to rand.Intn() each of which does a lock+unlock operation on the RNG.. while just 1 lock+unlock operation is enough for all
• we're choosing one character (from an alphabet of 27 chars) per each random integer.. while we can select 10 characters using a single int63 (by masking and bit-shifting) as 1 character uses just 5 bits of randomness
• the character set is defined as a global slice (mutable), so the compiler needs to fetch length of the slice on each invocation to len() (we're making n of those).. while we can just use a const string (immutable) which will make len directly available as a cached constant (yes, go does it!)

This PR is making the above fixes. I'll try to add some benchmarking to measure the difference (as @wojtek-t suggested).

/cc @kubernetes/sig-scalability-misc @kubernetes/sig-scheduling-bugs @kubernetes/sig-api-machinery-misc @wojtek-t @smarterclayton

[shyamjvs]

/test pull-kubernetes-kubemark-e2e-gce-big

[shyamjvs]

/test pull-kubernetes-kubemark-e2e-gce-big

[shyamjvs]

/test pull-kubernetes-kubemark-e2e-gce-big

[shyamjvs]

/test pull-kubernetes-kubemark-e2e-gce-big

[shyamjvs]

/test pull-kubernetes-kubemark-e2e-gce-big

[shyamjvs]

/test pull-kubernetes-kubemark-e2e-gce-big

[shyamjvs]

/test pull-kubernetes-kubemark-e2e-gce-big

[shyamjvs]

@jsafrane would be able to answer.

[smarterclayton]

Why are we using a global static utility function that needs a global static lock? That seems... bad.

We should be initializing a random source for whatever components need it and let them do their locking. The volume controller should only be contending with itself, in which case just give it a seed per worker (which should be easy).

[shyamjvs]

@smarterclayton I filed #53888 for the issue you stated. I'd prefer having that change in a separate PR as there are multiple places where the change needs to be made (i.e replace global RNG references with newRNG() calls).

[wojtek-t]

@smarterclayton I filed #53888 for the issue you stated. I'd prefer having that change in a separate PR as there are multiple places where the change needs to be made (i.e replace global RNG references with newRNG() calls).

I agree - let's not try to solve all the problems of random library as a resolving scalability regression.

/lgtm

[wojtek-t]

/approve

[shyamjvs]

@wojtek-t Also added the benchmark (though I'm still trying to make it run locally). Could you re-lgtm?

[wojtek-t]

/lgtm

[k8s-github-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: shyamjvs, wojtek-t

Associated issue: 53327

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

Needs approval from an approver in each of these OWNERS Files:

• pkg/controller/serviceaccount/OWNERS [wojtek-t]
• staging/src/k8s.io/apimachinery/pkg/OWNERS [wojtek-t]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

[shyamjvs]

Results of benchmark:

without this PR:

BenchmarkRandomStringGeneration-12    	  300000	      4129 ns/op

with this PR:

BenchmarkRandomStringGeneration-12    	 5000000	       337 ns/op

[wojtek-t]

Cool, but that's something I expected.
Looking forward to see the impact on kubemark test jobs.

[k8s-github-robot]

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

[k8s-github-robot]

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

[smarterclayton]

Changing it is fine, I just want to make sure we don't continue the pattern (i.e. if we cut ourselves on a sharp corner, we should fix the corner, not put a sign up that says "don't touch"). This should be the last time someone regresses on this by accidentally using this package.

…

On Fri, Oct 13, 2017 at 9:11 AM, Kubernetes Submit Queue < ***@***.***> wrote: Merged #53720 <#53720>. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#53720 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_p4JF90soU7iaTu7vWfegXOXIr9Nuks5sr2GSgaJpZM4P1ems> .

[wojtek-t]

Changing it is fine, I just want to make sure we don't continue the pattern
(i.e. if we cut ourselves on a sharp corner, we should fix the corner, not
put a sign up that says "don't touch"). This should be the last time
someone regresses on this by accidentally using this package.

Agree. I just wanted to fix regression ASAP and this seemed to fix it (and already existed).
But I completely agree that we should get rid of that package (or rework it somehow to at least not be global).