Opened 17 years ago
Closed 17 years ago
#5895 closed (fixed)
inline-edit performance problem after changeset 6655
Reported by: | Owned by: | jkocherhans | |
---|---|---|---|
Component: | Forms | Version: | newforms-admin |
Severity: | Keywords: | ||
Cc: | Triage Stage: | Accepted | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
I noticed a massive performance problem today after updating to the latest newforms-admin. Attempting to load a change page for one of my models that has another model edited inline pegged the CPU and started eating memory. I tracked it down to this code in /django/newforms/models.py, in the init for BaseModelFormSet:
if self.queryset: kwargs['initial'] = [initial_data(obj) for obj in self.get_queryset()]
Problem is self.queryset is an unfiltered queryset for the entire inline-edited model, it is not filtered by the pk of the parent instance until the self.get_queryset() call in the 2nd line, and "if self.queryset:" actually (apparently) calls __len__
on the queryset. __len__
in turn calls _get_data(), which in my case meant going and retrieving over 800,000 rows from the database, bringing my machine to its knees. Fix (attached) is simple:
if self.queryset is not None: kwargs['initial'] = [initial_data(obj) for obj in self.get_queryset()]
Attachments (1)
Change History (9)
by , 17 years ago
Attachment: | perf_fix.diff added |
---|
comment:1 by , 17 years ago
comment:2 by , 17 years ago
Yeah, I have also come across this in my newforms-admin branch. I originally did exactly what Karen did in the patch, but Malcolm brings up a good point and that is the correct fix. However, the correct workaround is to do self.get_queryset().count()
. It also needs to be applied to
save_existing_objects
where it does a check for no objects in the database.
comment:3 by , 17 years ago
Ah, kwargs['initial']
should only be set to something if the filtered queryset has something in it? Sounds reasonable, but I couldn't really tell the intent from what was there in my brief look through.
Is there some reason the unfiltered queryset needs to be kept around? It makes tracing through the code with a debugger (at least Eclipse/PyDev, and in my DB's case) a royal pain since the debugger goes out to lunch trying to display local variables whenever one of them is this huge unfiltered queryset....
comment:4 by , 17 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
Sorry about this. I'm working on a proper fix, but it isn't nearly as simple as the ones proposed, though they *do* fix the symptoms. There's a bit of an impedance mismatch between what I was doing before (passing in instances as an argument to the formset constructor) and doing things form_for_instance style. That mismatch still exists in the InlineFormset
class. Making formset_for_model use model._default_manager.none()
for the queryset is a start, but that breaks inline_formset
.
comment:5 by , 17 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:6 by , 17 years ago
Also broken by changeset 6655: bringing up an add page for a model that has inlines specified. Traceback is:
Traceback (most recent call last): File "/tmp/django/newforms-admin/django/core/handlers/base.py" in _real_get_response 81. response = callback(request, *callback_args, **callback_kwargs) File "/tmp/django/newforms-admin/django/contrib/admin/sites.py" in root 136. return self.model_page(request, *url.split('/', 2)) File "/tmp/django/newforms-admin/django/contrib/admin/sites.py" in model_page 153. return admin_obj(request, rest_of_url) File "/tmp/django/newforms-admin/django/contrib/admin/options.py" in __call__ 251. return self.add_view(request) File "/tmp/django/newforms-admin/django/contrib/admin/options.py" in add_view 504. inline_formset = FormSet() TypeError at /admin/crossword/puzzles/add/ __init__() takes at least 2 arguments (1 given)
I verified this worked before changeset 6655. Since it looks to be related to the same set of changes I figured I would just note it here vs. opening a different ticket. (If it belongs in its own ticket, let me know, and I'll open one.)
comment:8 by , 17 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
The problematic code no longer exists as of [6839].
The problem you're fixing here is best fixed by making
__nonzero__
on QuerySets not have to read more than just the first line of the result set. However, that won't land on trunk until queryset-refactor is done, so this is probably a reasonable temporary workaround. Fortunately, it won't be needed everywhere we do "if queryset" for ever and a day because we are fixing the root problem.