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

auth: dramatically improve checkPassword performance #11735

Merged
merged 1 commit into from
Apr 1, 2020

Conversation

wswcfan
Copy link
Contributor

@wswcfan wswcfan commented Mar 30, 2020

Optimize lock scope for CheckPassword, to improve authentication performance in concurrent scenarios 
when enable auth and using authentication based password.

etcd version: 3.4.3

When enable auth and using authentication based password, we found that etcd's read and write performance dropped sharply, and even a large number of timeouts occurred. After our investigation, we found that time is spent on Authenticate.
image

Furtherly, we found that the CompareHashAndPassword function will take tens of milliseconds. This function will block the lock of CheckPassword, which can cause certain requests to block for tens of seconds when there is thousounds of concurrent connection.
(We have add some debug log in etcd. As shown below, the time unit is millisecond)
image
image

Moving CompareHashAndPassword out of the critical section of the lock can greatly improve performance. After we do this, we no longer find timeout requests.

And we developed a tool based on etcd benchmark CLI to test the performance of the Authenticate interface. The performance data is as follows:

We consider a single node etcd cluster with the following hardware configuration:

  • 1 machines(etcd server) of v16 CPUs + 32GB Memory + 50GB SSD
  • 1 machine(client) of 8 vCPUs + 16GB Memory + 50GB SSD
  • Ubuntu 16.04
  • etcd Version: 3.5.0-pre, Go Version: go1.13

Before optimization:

benchmark_auth auth --total 3000 --clients 100 --conns 100

image

benchmark_auth auth --total 3000 --clients 200 --conns 200

image
With 200 concurrency, a large number of requests time out.

After optimization:

benchmark_auth auth --total 3000 --clients 100 --conns 100

image

benchmark_auth auth --total 3000 --clients 200 --conns 200

image

We can see that the performance is improved more than 10 times after optimization. And with the increase of concurrencys, the performance improvement is more and more obvious

@wswcfan wswcfan changed the title auth: optimize lock scope in CheckPassword, to improve performance when auth enabled auth: dramatically improve checkPassword performance Mar 30, 2020
Copy link
Contributor

@mitake mitake left a comment

Choose a reason for hiding this comment

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

Thanks a lot! But could you add a comment about this change to the comment? Not using defer unlock is a little bit unusual so people who read and modify the code should be warned about this (e.g. forgetting about unlocking in error paths).

@wswcfan wswcfan force-pushed the optimize-auth-lock-performance branch from b7853d9 to 3f419f9 Compare March 31, 2020 17:56
@wswcfan
Copy link
Contributor Author

wswcfan commented Mar 31, 2020

Thanks for your suggestion! I refactored a version that uses closures, so we can still use the defer statement, it may look better than the previous implementation, what do you think? @mitake

to improve authentication performance in concurrent scenarios when enable auth and using authentication based password
@wswcfan wswcfan force-pushed the optimize-auth-lock-performance branch from 3f419f9 to 9cf3162 Compare March 31, 2020 18:22
Copy link
Contributor

@mitake mitake left a comment

Choose a reason for hiding this comment

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

@wswcfan LGTM! Yeah your approach based on closure looks great :) Defer to @jingyih

Copy link
Contributor

@jingyih jingyih left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@jingyih jingyih merged commit 054b0e5 into etcd-io:master Apr 1, 2020
tangcong added a commit to tangcong/etcd that referenced this pull request Apr 5, 2020
tangcong added a commit to tangcong/etcd that referenced this pull request Apr 5, 2020
tangcong added a commit to tangcong/etcd that referenced this pull request Apr 5, 2020
jingyih added a commit that referenced this pull request Apr 10, 2020
gyuho added a commit that referenced this pull request May 20, 2020
…5-origin-release-3.4

Automated cherry pick of #11735 on release-3.4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants