Opened 14 years ago

Closed 14 years ago

Last modified 12 years ago

#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)

Q.or_.diff (2.8 KB ) - added by Jeff Balogh 14 years ago.
The Q.or_ class method combines a bunch of Q objects and keyword arguments without all the cloning that occurs using |.

Download all attachments as: .zip

Change History (8)

by Jeff Balogh, 14 years ago

Attachment: Q.or_.diff added

The Q.or_ class method combines a bunch of Q objects and keyword arguments without all the cloning that occurs using |.

comment:1 by Alex Gaynor, 14 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.

in reply to:  1 comment:2 by Jeff Balogh, 14 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 Alex Gaynor, 14 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 Russell Keith-Magee, 14 years ago

milestone: 1.2
Patch needs improvement: set
Triage Stage: UnreviewedAccepted

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 James Bennett, 14 years ago

milestone: 1.21.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 Carl Meyer, 14 years ago

Resolution: fixed
Status: newclosed

This was fixed as a side effect of r12865 - there is no longer any deepcopy performed when combining Q objects.

comment:7 by Jacob, 12 years ago

milestone: 1.3

Milestone 1.3 deleted

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