Optimize _get_user_permissions by using set comprehensions

[Abhyudai]

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.

[David Smith]

could potentially reduce the time taken for it.

Could you provide a benchmark to evidence the performance gains from this optimisation?

[Carlton Gibson]

Yep, happy to look at this if it's quicker.

There's ​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. ​pyperf docs.

[Nick Pope]

I thought an alternative implementation could be the following to avoid needing hasattr() and getattr():

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:

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)

[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

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)

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.