Improve performance of related manager attribute access

[Keryn Knight]

Currently, getting related managers from an instance is slower than I think it ought to be. Some prelude indicating a relatively expected baseline:

In [1]: from django.contrib.auth.models import User
In [2]: %timeit User.objects
1.26 µs ± 25.2 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)
In [3]: x = User.objects.first()
In [4]: %timeit x.username
34.2 ns ± 0.574 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)
In [5]: %timeit x.get_short_name()
110 ns ± 2.02 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)
In [7]: %timeit x.pk
201 ns ± 4.68 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)

So far, all making sense mostly, but then:

In [8]: %timeit x.groups
7.94 µs ± 40.3 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)
In [9]: %timeit x.user_permissions
8.16 µs ± 182 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)

Crikey that's a big difference. What's going on here?

In [10]: x.groups
Out[10]: <django.db.models.fields.related_descriptors.create_forward_many_to_many_manager.<locals>.ManyRelatedManager at 0x10dc58d60>
In [11]: x.groups
Out[11]: <django.db.models.fields.related_descriptors.create_forward_many_to_many_manager.<locals>.ManyRelatedManager at 0x10da8b490>
In [12]: x.groups is x.groups
Out[12]: False
In [13]: x.user_permissions is x.user_permissions
Out[13]: False

Aha, it's one of those rare points where seeing the 0x notation in a repr is useful! The manager is being re-created on every access, so even though repeated uses might look innocuous because they're "the same" attribute (eg: x.groups.count() and then x.groups.filter(...)), they're much slower to work with.

In [13]: %%timeit -n1000
    ...: users = User.objects.prefetch_related('groups')
    ...: for user in users:
    ...:     user.groups
    ...:
8.03 ms ± 108 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)

If we instead cache the returned manager (as is done for the related_manager_cls) somewhere, we can amortize the cost of creating a per-instance manager on repeated access:

In [1]: from django.contrib.auth.models import User
In [2]: x = User.objects.first()
In [4]: %timeit x.groups
554 ns ± 9.86 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)
In [5]: %timeit x.user_permissions
534 ns ± 8.09 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)
In [6]: x.groups is x.groups
Out[6]: True
In [7]: x.user_permissions is x.user_permissions
Out[7]: True
In [8]: %%timeit -n1000
   ...: users = User.objects.prefetch_related('groups')
   ...: for user in users:
   ...:     user.groups
   ...:
6.93 ms ± 207 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)

Because prefetch_one_level will use getattr to get the manager for every instance seen, the cost is already paid by the time the user wants to then use the fetched data - even though the manager's underlying queryset will just proxy straight to the prefetched objects cache, it is currently paying a high price to do so.

The how and where and the final implementation (if accepted) are still a bit up for grabs; we cannot cache it on the descriptor as that would outlive the instance and grow unbounded as instances are seen, similarly using a weakref dict on the descriptor would stop the cached manager outliving the instance but because it's weakly held, it disappears immediately, so doesn't effectively work as a cache.
That effectively leaves somewhere as "on the instance" as either a private item pushed into instance.__dict__ or re-using the field_cache on the instance's ModelState. I have a branch which I'll push shortly showing a proof of concept of how it could be done, having taken some vague guesses on bits - I guarantee it's not good enough right now, but I think it 'could be.

I think this generally only applies to reverse many-to-one and many-to-many managers because the one-to-one and forward many-to-one descriptors are already making use of the field_cache in their __get__ implementation. So I've only concerned myself with those where I've noted that's not the case...

[Keryn Knight]

PR is ​https://github.com/django/django/pull/14724 ... so far tests are still running, so who knows if it even works :)
Pre-emptively marked as patch needs improvement, and I guarantee there'll be discussion on the PoC if the principle idea is accepted.

[Carlton Gibson]

OK, sounds plausible. Let's accept for discussion/review.

[Keryn Knight]

Refactored the PoC into something tidier, without having to hard-code strings and sprinkle those through-out various places, so now the focus can solely be on things like:

• am I using the right attributes/methods?
• are there test cases missing for completeness?

[Keryn Knight]

Marking as ready to have eyes on it once again, naturally can revert back to needs improvement.

A question I've raised on the ticket, which I reproduce for historical purposes here, is:

There's one thing that isn't handled, because I'm not sure if/how it can actually happen, and that's the case of a ManyToManyField which is symmetrical and the branch entered is self.reverse is True. As far as I know, it's impossible to get a reverse manager for symmetrical m2ms, but if it's possible, it would currently pose a problem because get_cache_name defers to get_accessor_name and that returns None for symmetries, so there would be the possibility of a collision.

[Mariusz Felisiak <felisiak.mariusz@…>]

In 4f8c7fd9:

Fixed #32980 -- Made models cache related managers.

[GitHub <noreply@…>]

In 5e0aa362:

Fixed #33984 -- Reverted "Fixed #32980 -- Made models cache related managers."

This reverts 4f8c7fd9d91b35e2c2922de4bb50c8c8066cbbc6 and adds
two regression tests:

• test_related_manager_refresh(), and
• test_create_copy_with_m2m().

Thanks joeli for the report.

[Mariusz Felisiak <felisiak.mariusz@…>]

In 7a167580:

[4.1.x] Fixed #33984 -- Reverted "Fixed #32980 -- Made models cache related managers."

This reverts 4f8c7fd9d91b35e2c2922de4bb50c8c8066cbbc6 and adds
two regression tests:

• test_related_manager_refresh(), and
• test_create_copy_with_m2m().

Thanks joeli for the report.
Backport of 5e0aa362d91d000984995ce374c2d7547d8d107f from main

[Mariusz Felisiak]

4f8c7fd9d91b35e2c2922de4bb50c8c8066cbbc6 was reverted (see #33984).