Opened 8 years ago
Closed 8 years ago
#27002 closed Cleanup/optimization (fixed)
Redundant database query rendering a ModelChoiceField with RadioSelect or CheckboxSelectMultiple
Reported by: | Alex Hill | Owned by: | nobody |
---|---|---|---|
Component: | Forms | Version: | dev |
Severity: | Normal | 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 (last modified by )
Followup to #27001:
Rendering a ModelChoiceField with RadioSelect and CheckboxSelectMultiple, there's a redundant iteration that triggers an unnecessary database call.
Here's the relevant code from ForNode.render (GitHub link):
if not hasattr(values, '__len__'): # line 164 values = list(values) len_values = len(values) ... for i, item in enumerate(values): # line 177 ...
In this code, values
is a BoundField. BoundField defines __len__
like this:
def __len__(self): return len(list(self.__iter__()))
So len(values)
on line 164 triggers an iteration over values
(and thus a database query), then the for loop at line 177 triggers another.
The options as I see them are
- Remove
BoundField.__len__
so thatlist(values)
is called inForNode.render
. This fixes the problem without breaking any tests. It is backwards-incompatible (e.g. with an existing template passing a boundfield to thelength
template filter), but not with anything documented. - Cache iterator results somewhere internally so that the actual query is only executed once. This is easy to do on BoundField with a cached property. No backward compatibility concerns here.
- Unconditionally call
list(values)
in ForNode. Also works, but a bit heavy-handed and not necessary if one of the first two options are acceptable.
I think (2) is the best choice. It's pretty clean, and it means the fix can be implemented in 1.10 (and 1.8 LTS?) as a bug fix without backward compatibility concerns.
Change History (8)
comment:1 by , 8 years ago
Description: | modified (diff) |
---|
comment:2 by , 8 years ago
Has patch: | set |
---|
comment:3 by , 8 years ago
We're targeting template-based widget rendering, which removes ChoiceFieldRenderer
, for 1.11 (PR). Is this issue still relevant after that patch? Unless this is a regression in Django 1.10, we don't need to fix it there.
comment:4 by , 8 years ago
Yep this is still relevant. The test included in this patch fails in the 15667 branch. This patch only concerns BoundField
– the ChoiceFieldRenderer
changes were all in #27001.
Now that #27001 is merged I've rebased on master and opened a pull request.
An unnecessary duplicated query is just as much a bug in 1.10 as it will be in 1.11. Why not fix it in 1.10?
comment:5 by , 8 years ago
An unnecessary duplicated query is just as much a bug in 1.10 as it will be in 1.11. Why not fix it in 1.10?
Per the documented backport policy:
Patches applied to the master branch must also be applied to the last feature release branch, to be released in the next patch release of that feature series, when they fix critical problems:
- Security issues.
- Data loss bugs.
- Crashing bugs.
- Major functionality bugs in newly-introduced features.
- Regressions from older versions of Django.
The rule of thumb is that fixes will be backported to the last feature release for bugs that would have prevented a release in the first place (release blockers).
comment:6 by , 8 years ago
Triage Stage: | Unreviewed → Accepted |
---|---|
Type: | Bug → Cleanup/optimization |
Version: | 1.10 → master |
Implementation of (2) at https://github.com/django/django/compare/master...AlexHill:ticket_27002
That patch builds in #27001, so I haven't opened a PR for it yet.