Opened 5 years ago

Closed 5 years ago

Last modified 4 years ago

#12725 closed (fixed)

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

Reported by: jbalogh Owned by: nobody
Component: Database layer (models, ORM) Version: 1.2-alpha
Severity: Keywords:
Cc: clouserw Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: UI/UX:

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 jbalogh 5 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)

Changed 5 years ago by jbalogh

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

comment:1 follow-up: Changed 5 years ago by Alex

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

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 in reply to: ↑ 1 Changed 5 years ago by jbalogh

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 Changed 5 years ago by Alex

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 Changed 5 years ago by russellm

  • milestone set to 1.2
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to 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 Changed 5 years ago by ubernostrum

  • milestone changed from 1.2 to 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 Changed 5 years ago by carljm

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

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

comment:7 Changed 4 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

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