#12725 closed (fixed)
Combining Q objects with | is inefficient; lots of deep cloning
Reported by: | Jeff Balogh | Owned by: | nobody |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | 1.2-alpha |
Severity: | Keywords: | ||
Cc: | Wil Clouser | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description
I have a view that was pegging the CPU for way longer than it should, so I went to do some profiling. It turns out that the view was spending 1.4 seconds in various parts of deepcopy, and I tracked that down to ORing a bunch of Q objects. Whenever two Q objects are combined with |, the lhs is cloned so that a new, combined Q object is returned. The view was combining 50+ Q objects (lame, I know) and was bogged down in cloning.
The attached patch adds a Q.or_
class method that takes any number of Q objects and keyword arguments and ORs them together without any cloning.
I thought about adding Q.and_
for symmetry, but that's the normal operating of Q and filter to begin with, so I didn't want to add another way to do it.
Sad note: for whatever reason, the view in question is only 120ms slower than the optimized Q.or_
version today. But 120ms is still a lot.
Attachments (1)
Change History (8)
by , 15 years ago
Attachment: | Q.or_.diff added |
---|
follow-up: 2 comment:1 by , 15 years ago
ISTM if this optimization is really worth it (and I don't know what your usecase is), it should be done in _combine at the base class level in django.utils.tree. Specifically if the RHS has the same combinator type as the operator being preformed than the new node can be created by just doing a list addition on the lists of children.
comment:2 by , 15 years ago
Replying to Alex:
Specifically if the RHS has the same combinator type as the operator being preformed than the new node can be created by just doing a list addition on the lists of children.
I assumed that the clones were being done to preserve some immutability guarantee in the LHS.
comment:3 by , 15 years ago
a) Most of the technical details of my post were terribly off, please don't take them literally (I really need to double check myself before referring to methods).
b) Yes, clones are made, but the issue I thought was being discussed here was their nesting, which isn't necessary. If the comparator is the same than they can be kept in a flat list, which avoids the deep recursion in deepcopy calls.
comment:4 by , 15 years ago
milestone: | → 1.2 |
---|---|
Patch needs improvement: | set |
Triage Stage: | Unreviewed → Accepted |
I'm not sure I'm wild about the idea of having a second way of doing or; if there's a problem with using a pipe, then it should be fixed there if at all possible.
comment:5 by , 15 years ago
milestone: | 1.2 → 1.3 |
---|
Since this isn't an actual bug but rather a possible optimization (and since we can't seem to agree on exactly how to apply it), I'm punting it from 1.2.
comment:6 by , 14 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
This was fixed as a side effect of r12865 - there is no longer any deepcopy performed when combining Q objects.
The Q.or_ class method combines a bunch of Q objects and keyword arguments without all the cloning that occurs using |.