Opened 8 years ago

Closed 7 years ago

#5895 closed (fixed)

inline-edit performance problem after changeset 6655

Reported by: Karen Tracey <kmtracey@…> 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: UI/UX:

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)

perf_fix.diff (589 bytes) - added by Karen Tracey <kmtracey@…> 8 years ago.

Download all attachments as: .zip

Change History (9)

Changed 8 years ago by Karen Tracey <kmtracey@…>

comment:1 Changed 8 years ago by mtredinnick

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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.

comment:2 Changed 8 years ago by brosner

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 Changed 8 years ago by Karen Tracey <kmtracey@…>

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 Changed 8 years ago by jkocherhans

  • Owner changed from nobody to jkocherhans
  • Status changed from new to 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 Changed 8 years ago by brosner

  • Triage Stage changed from Unreviewed to Accepted

comment:6 Changed 8 years ago by Karen Tracey <kmtracey@…>

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:7 Changed 8 years ago by LongMan <alx.gsv@…>

oops. created before reading that #5919. but problem is actual

comment:8 Changed 7 years ago by jkocherhans

  • Resolution set to fixed
  • Status changed from assigned to closed

The problematic code no longer exists as of [6839].

Note: See TracTickets for help on using tickets.
Back to Top