Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32.7k
Fixed #36439: Optimizeacheck_password CPU heavy internal hashing process by using a ThreadPoolExecutor#19611
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
3750296 to1bb5279Compareacheck_password CPU heavy internal hashing process by using a ThreadPoolExecutoracheck_password CPU heavy internal hashing process by using a ThreadPoolExecutorb984047 toc9eec27Comparec9eec27 tod1a1ef1Compared1a1ef1 to50a6d87CompareBased on theticket, a test may not be needed for this patch. |
…sword` into a `ThreadPoolExecutor`.
50a6d87 tob5354b5Compareacheck_password CPU heavy internal hashing process by using a ThreadPoolExecutoracheck_password CPU heavy internal hashing process by using a ThreadPoolExecutor| @@ -546,6 +546,8 @@ def gettext_noop(s): | |||
| AUTH_PASSWORD_VALIDATORS = [] | |||
| CHECK_PASSWORD_EXECUTOR_MAX_WORKERS = None | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We shouldn't add a global setting for something niche within a contrib app
In general adding settings is quite controversial and needs to be discussed and agreed with the wider community
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks for checking. If we don't want to control any of theThreadPoolExecutor options, we can also rely on its basic instance or maybe create a ThreadPoolExecutor everyacheck_password call, depending on the performance.
| @@ -282,6 +282,18 @@ The default integer number of seconds to cache a page for the | |||
| See :doc:`/topics/cache`. | |||
| .. setting:: CHECK_PASSWORD_EXECUTOR_MAX_WORKERS | |||
| ``CHECK_PASSWORD_EXECUTOR_MAX_WORKERS`` | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't think we should add a setting, but if we were, this would need the "versionadded" annotation and a release note in 6.0
Uh oh!
There was an error while loading.Please reload this page.
Trac ticket number
ticket-36439
Branch description
Conducted severalexperiments/benchmarks on the performance of the
acheck_passwordthat blocks asyncio's event-loop, which is causing a performance bottleneck. Adding aThreadPoolExecutorsignificantly improved the performance gap compared to the test without using it.Also, introduced the
CHECK_PASSWORD_EXECUTOR_MAX_WORKERSfor allowing developers to provide a configurable setting for theThreadPoolExecutorinstance. See the updated document on this PR.This PR targets a patch for v5.2 as mentioned in the ticket.
Checklist
mainbranch.