[Django] #32782: Optimize _get_user_permissions by using set comprehensions
Django
-------------------------------------+-------------------------------------
Reporter: Abhyudai | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: | Version: 3.2
Uncategorized |
Severity: Normal | Keywords: permissions, auth
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 1
UI/UX: 0 |
-------------------------------------+-------------------------------------
The intended patch
{{{
diff --git a/django/contrib/auth/models.py b/django/contrib/auth/models.py
index 5f092f0ae8..8f7b65322c 100644
--- a/django/contrib/auth/models.py
+++ b/django/contrib/auth/models.py
@@ -191,13 +191,13 @@ class UserManager(BaseUserManager):
# A few helper functions for common logic between User and AnonymousUser.
def _user_get_permissions(user, obj, from_name):
- permissions = set()
name = 'get_%s_permissions' % from_name
- for backend in auth.get_backends():
- if hasattr(backend, name):
- permissions.update(getattr(backend, name)(user, obj))
- return permissions
-
+ return {
+ permission
+ for backend in auth.get_backends()
+ if hasattr(backend, name)
+ for permission in getattr(backend, name)(user, obj)
+ }
def _user_has_perm(user, perm, obj):
"""
}}}
Since the function `_get_user_permissions` is used by almost all
permission related methods, this change could potentially reduce the time
taken for it.
If it seems acceptable, I would love to send a pull request.
--
Ticket URL: <https://code.djangoproject.com/ticket/32782>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: Abhyudai | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Severity: Normal | Resolution:
Keywords: permissions, auth | Triage Stage:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by David Smith):
> could potentially reduce the time taken for it.
Could you provide a benchmark to evidence the performance gains from this
optimisation?
--
Ticket URL: <https://code.djangoproject.com/ticket/32782#comment:1>
Django
-------------------------------------+-------------------------------------
Reporter: Abhyudai | Owner: nobody
Cleanup/optimization |
Component: Uncategorized | Version: 3.2
Keywords: permissions, auth | Triage Stage:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => needsinfo
Comment:
Yep, happy to look at this if it's quicker.
There's
[https://github.com/django/django/pull/12133#issuecomment-763034865 an
example of a simple benchmark script using pyperf here].
You'd need to install pyperf for that approach, but it's straight-forward
enough. [https://pyperf.readthedocs.io/en/latest/ pyperf docs].
--
Ticket URL: <https://code.djangoproject.com/ticket/32782#comment:2>
Django
-------------------------------------+-------------------------------------
Reporter: Abhyudai | Owner: nobody
Cleanup/optimization |
Component: Uncategorized | Version: 3.2
Severity: Normal | Resolution: needsinfo
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Nick Pope):
I thought an alternative implementation could be the following to avoid
needing `hasattr()` and `getattr()`:
{{{#!python
def _user_get_permissions(user, obj, name):
name = f'get_{name}_permissions'
return {
permission
for backend in auth.get_backends()
if (method := getattr(backend, name, None))
for permission in method(user, obj)
}
}}}
I think this is more readable (personally), but there is no speed up:
{{{
Before: Mean +- std dev: 252 us +- 16 us
After: Mean +- std dev: 253 us +- 10 us
}}}
Maybe I got something wrong in my script. Here it is if anyone was
interested in how to do this:
{{{#!python
import time
import django
import pyperf
from django.apps import apps
from django.conf import settings
from django.core.management import call_command
settings.configure(
# Duplicate default backend 10 times to add more looping:
AUTHENTICATION_BACKENDS=['django.contrib.auth.backends.ModelBackend']
* 10,
DATABASES={'default': {'ENGINE': 'django.db.backends.sqlite3', 'NAME':
':memory:'}},
INSTALLED_APPS=['django.contrib.auth', 'django.contrib.contenttypes',
'__main__'],
)
django.setup()
# Run the migrations to create the require models:
call_command('migrate', verbosity=0)
# Look up the required models:
User = apps.get_model('auth.User')
Group = apps.get_model('auth.Group')
Permission = apps.get_model('auth.Permission')
# Create a user with a group and permissions assigned to both the user and
group:
user = User.objects.create_user('test')
group = Group.objects.create(name='test')
permissions = Permission.objects.all()
user.groups.add(group)
user.user_permissions.add(*permissions)
group.permissions.add(*permissions)
# Call permission methods to pre-cache results to reduce overhead from
database queries:
user.get_all_permissions()
user.get_user_permissions()
user.get_group_permissions()
def test(loops, user):
range_it = range(loops)
t0 = time.perf_counter()
for loop in range_it:
user.get_all_permissions()
user.get_user_permissions()
user.get_group_permissions()
return time.perf_counter() - t0
runner = pyperf.Runner()
runner.bench_time_func('Fetch Permissions', test, user)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32782#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: Abhyudai | Owner: nobody
Cleanup/optimization |
Component: Uncategorized | Version: 3.2
Severity: Normal | Resolution: needsinfo
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Abhyudai):
I also didn't notice any speed up even without the `:=` walrus operator
and `f-strings`(using my original patch). My results were pretty similar
to the ones observed by Nick.
Although, the non-scientific benchmarking was encouraging (using `time`
command), the more scientific one wasn't.
For what it is worth, the script user for benchmarking was
{{{#!python
import os
from django.conf.global_settings import INSTALLED_APPS
import django
from django.conf import settings
from django.core.management import call_command
import pyperf
settings.configure(
INSTALLED_APPS= (
'django.contrib.auth',
'django.contrib.contenttypes',
),
DATABASES = {
'default': {
'ENGINE': 'django.db.backends.sqlite3',
'NAME': os.path.join(
os.path.dirname(os.path.dirname(os.path.abspath(__file__))),
'db.sqlite3'
),
}
},
)
django.setup()
call_command('migrate', verbosity=0)
from django.contrib.auth.models import User, _user_get_permissions
user, _ = User.objects.get_or_create(username='a', password='a')
def test(loops):
t0 = pyperf.perf_counter()
# repeat to reduce impact of for loop
for loop in range(loops):
_user_get_permissions(user, None, 'all')
_user_get_permissions(user, None, 'all')
_user_get_permissions(user, None, 'all')
_user_get_permissions(user, None, 'all')
_user_get_permissions(user, None, 'all')
_user_get_permissions(user, None, 'all')
_user_get_permissions(user, None, 'all')
_user_get_permissions(user, None, 'all')
_user_get_permissions(user, None, 'all')
_user_get_permissions(user, None, 'all')
_user_get_permissions(user, None, 'all')
return pyperf.perf_counter() - t0
runner = pyperf.Runner()
runner.bench_time_func('User permissions', test)
}}}
{{{
Before: Mean +- std dev: 220 us +- 11 us
After: User permissions: Mean +- std dev: 223 us +- 11 us
}}}
This appeared a lit weird to me because comprehensions tend to be
computationally faster than the traditional `for` loops. At least, that is
how they are marketed, and were sold to me :-p
The script used for profiling was,(this was saved as `profiler.py`)
{{{#!python
import os
from django.conf.global_settings import INSTALLED_APPS
import django
from django.conf import settings
from django.core.management import call_command
settings.configure(
INSTALLED_APPS= (
'django.contrib.auth',
'django.contrib.contenttypes',
),
DATABASES = {
'default': {
'ENGINE': 'django.db.backends.sqlite3',
'NAME': os.path.join(
os.path.dirname(os.path.dirname(os.path.abspath(__file__))),
'db.sqlite3'
),
}
},
)
django.setup()
from django.contrib.auth.models import User, _user_get_permissions
user, _ = User.objects.get_or_create(username='a', password='a')
def main():
user.get_all_permissions()
if __name__ == '__main__':
main()
}}}
I saw a tiny increase in the percentage of time spent in the
`_user_get_permissions` function. The earlier one took `1.29%`, while the
newer one spent `1.31%`, although pretty insignificant but the traditional
`for` loops one spends lesser time !
I can't seem to find a way to attach the `pstats` files here.
--
Ticket URL: <https://code.djangoproject.com/ticket/32782#comment:4>