[Django] #23623: ModelChoiceField generates unused temporary cache
Django
--------------------------------------+------------------------------------
Reporter: tchaumeny | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Forms | Version: master
Severity: Normal | Keywords: ModelChoiceField,
Triage Stage: Unreviewed | admin
Easy pickings: 0 | Has patch: 1
| UI/UX: 0
--------------------------------------+------------------------------------
There is a huge memory consumption when using some admin pages with big
production datasets (see also https://groups.google.com/forum/#!topic
/django-users/YYlLWyBH_go).
I looked into the `ModelChoiceIterator` used to generate the choices for a
`ModelChoiceField`, and noticed that a temporary queryset cache was
generated.
With the proposed PR, this cache is no longer generated. After applying
the patch, I noticed that the maximum memory usage dropped from ~200MB to
~150MB for a model with about 30 000 instances.
--
Ticket URL: <https://code.djangoproject.com/ticket/23623>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: tchaumeny | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version: master
Component: Forms | Resolution:
Severity: Normal | Triage Stage:
Keywords: ModelChoiceField, | Unreviewed
admin | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by tchaumeny):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Old description:
> There is a huge memory consumption when using some admin pages with big
> production datasets (see also https://groups.google.com/forum/#!topic
> /django-users/YYlLWyBH_go).
>
> I looked into the `ModelChoiceIterator` used to generate the choices for
> a `ModelChoiceField`, and noticed that a temporary queryset cache was
> generated.
>
> With the proposed PR, this cache is no longer generated. After applying
> the patch, I noticed that the maximum memory usage dropped from ~200MB to
> ~150MB for a model with about 30 000 instances.
New description:
There is a huge memory consumption when using some admin pages with big
production datasets (see also https://groups.google.com/forum/#!topic
/django-users/YYlLWyBH_go).
I looked into the `ModelChoiceIterator` used to generate the choices for a
`ModelChoiceField`, and noticed that an unused temporary queryset cache
was generated due to the use of `.all()`, but unused (there is no variable
pointing to the cloned queryset after the iteration).
With the proposed PR, this cache is no longer generated. After applying
the patch, I noticed that the maximum memory usage dropped from ~200MB to
~150MB for a model with about 30 000 instances.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/23623#comment:1>
Django
Reporter: tchaumeny | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version: master
Component: Forms | Resolution:
Severity: Normal | Triage Stage:
Keywords: ModelChoiceField, | Unreviewed
admin | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Old description:
> There is a huge memory consumption when using some admin pages with big
> production datasets (see also https://groups.google.com/forum/#!topic
> /django-users/YYlLWyBH_go).
>
> I looked into the `ModelChoiceIterator` used to generate the choices for
> a `ModelChoiceField`, and noticed that an unused temporary queryset cache
> was generated due to the use of `.all()`, but unused (there is no
> variable pointing to the cloned queryset after the iteration).
>
> With the proposed PR, this cache is no longer generated. After applying
> the patch, I noticed that the maximum memory usage dropped from ~200MB to
> ~150MB for a model with about 30 000 instances.
New description:
There is a huge memory consumption when using some admin pages with big
production datasets (see also https://groups.google.com/forum/#!topic
/django-users/YYlLWyBH_go).
I looked into the `ModelChoiceIterator` used to generate the choices for a
`ModelChoiceField`, and noticed that an unused temporary queryset cache
was generated due to the use of `.all()` (there is no variable pointing to
the cloned queryset after the iteration).
With the proposed PR, this cache is no longer generated. After applying
the patch, I noticed that the maximum memory usage dropped from ~200MB to
~150MB for a model with about 30 000 instances.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/23623#comment:2>
Django
Reporter: tchaumeny | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version: master
Component: Forms | Resolution:
Severity: Normal | Triage Stage:
Keywords: ModelChoiceField, | Unreviewed
admin | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by timgraham):
Have you seen #22841? I think that is related to this.
--
Ticket URL: <https://code.djangoproject.com/ticket/23623#comment:3>
Django
Reporter: tchaumeny | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version: master
Component: Forms | Resolution:
Severity: Normal | Triage Stage:
Keywords: ModelChoiceField, | Unreviewed
admin | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by tchaumeny):
I missed that ticket, thanks. My understanding of #22841 is that it
proposes a way to "tell" the `ModelChoiceField` to reuse a `queryset`
instead of reevaluating it at each time.
I think that my PR is related but compatible with that proposed change as
it handles the case where we don't want to reuse the `queryset`. That case
is currently handled by iterating on `queryset.all()` — which creates a
cloned unevaluated queryset — to construct the choices. My proposition is
to use `iterator()` instead as this is designed precisely for those cases
where you don't want to use cache nor to populate it.
--
Ticket URL: <https://code.djangoproject.com/ticket/23623#comment:4>
Django
Reporter: tchaumeny | Owner: nobody
Type: | Status: closed
Cleanup/optimization | Version: master
Component: Forms | Resolution: fixed
Severity: Normal | Triage Stage:
Keywords: ModelChoiceField, | Unreviewed
admin | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"fa534b92dda0771661a98a1ca302ced264d0a6da"]:
{{{
#!CommitTicketReference repository=""
revision="fa534b92dda0771661a98a1ca302ced264d0a6da"
Fixed #23623 -- Reduced memory consumption when generating
ModelChoiceField choices
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23623#comment:5>
Django
Reporter: tchaumeny | Owner: nobody
Type: | Status: closed
Cleanup/optimization |
Component: Forms | Version: master
Severity: Normal | Resolution: fixed
Keywords: ModelChoiceField, | Triage Stage:
admin | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by charettes):
It looks like this introduced a regression when passing a queryset using
the `prefetch_related()` which is not compatible with `iterator()` by
design #25496.
I would suggest we conditionally use `iterator()` if no related data
should be fetched instead of simply reverting this optimization.
Unfortunately there's no public API to determine whether or not a
`QuerySet` is prefetching data and we would have to rely on the
`qs._prefetch_related_lookups` property.
--
Ticket URL: <https://code.djangoproject.com/ticket/23623#comment:6>