Combining Q objects with | is inefficient; lots of deep cloning

[Jeff Balogh]

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.

[Jeff Balogh]

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

[Alex Gaynor]

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.

[Jeff Balogh]

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.

[Alex Gaynor]

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.

[Russell Keith-Magee]

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.

[James Bennett]

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.

[Carl Meyer]

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

[Jacob]

Milestone 1.3 deleted