Opened 17 years ago

Closed 17 years ago

Last modified 17 years ago

#3439 closed (fixed)

django.dispatch.* is grossly slow

Reported by: (removed) Owned by: Adrian Holovaty
Component: Core (Other) Version: dev
Severity: 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

Attached is a refactoring chopping off a good chunk of dispatch's overhead; hard to test it standalone, so testing was done for simple iteration over records and isolating the diff in the profiling for the refactoring.

Short version, attached patch shows 5x boost in dispatch speed; for iteration (just that, no rendering) over 53k records, time drops from ~15.5s to ~11.6s just by cleaning up dispatch.*

The internal data structs dispatch is using are still fairly innefficient- scaling will be an issue the more receivers there are for dispatched signals. So... can clean that up further, but there isn't any documentation re: the expectations of dispatcher. For example- order of connection to a signal, must the order be maintained for notifications at a specific sender/signal level? If I connect (in this order) func f1, func f2, both to object("foo") signal Any, must they be notified in order f1,f2?

I realize pydispatcher is a seperate project- that said, they've already moved onto 2.0, and there are several 1.0.x's for pydispatcher released already- djangos' bundled copy is still at 1.0.0. Whats the intention? Intending on keeping it in sync, or is it just a fork point?

Either way, the attached refactoring is not covered by tests at the moment- mainly since there are no tests in django for dispatcher. Would like to get feedback on the expectations of dispatcher (f1/f2 from above for example) prior to writing those tests, since the implementation (and google for that matter) lack any material on it.

Finally, due to the massive number of calls to send (thus to getAllReceivers and friends), normal seperation (getAllReceivers using getReceivers for example) was intentionally broken down a bit- in other words, inlined a few functions. The codes invoked enough that the overheads seperation really isn't worth it, especially if you're not actually using signals for the most part (thus dispatcher is pretty much pure time waste).

Attachments (2)

dispatch-refactoring.patch (6.8 KB ) - added by (removed) 17 years ago.
dispatcher refactoring round 1.
dispatch.patch (17.8 KB ) - added by (removed) 17 years ago.
import of upstream tests, fix deregistration bug

Download all attachments as: .zip

Change History (8)

by (removed), 17 years ago

Attachment: dispatch-refactoring.patch added

dispatcher refactoring round 1.

by (removed), 17 years ago

Attachment: dispatch.patch added

import of upstream tests, fix deregistration bug

comment:2 by Simon G. <dev@…>, 17 years ago

Triage Stage: UnreviewedAccepted

comment:3 by (removed), 17 years ago

Minor optimization I missed, although it's quadratic worst case correction; in _killBackref

while senderkey in receivers_list:
  try:
    receivers_list.remove(senderkey)
  except:
    break

is... certifiably insane. Utterly.

No idea how I missed that one...

receivers_list = [x for x in receivers_list if senderkey != x]

avoids quadratic worst case for if receivers_list is all senderkey; python lists are arrays, popleft forces everything beyond to be moved one to the left (worth noting context, and resolve_variable have the same whackyness).

comment:4 by Jacob, 17 years ago

This patch works quite well for me -- major props for tests, too :)

However, for some reason I can't understand at all, it seems to be failing under sqlite. I'm going to check the patch in anyway since the performance gains are measurable, but Brian, if you're still following this thread, can you *please* take a look at it under sqlite and try to figure out what's going on?

comment:5 by Jacob, 17 years ago

Resolution: fixed
Status: newclosed

(In [4588]) Fixed #3439: vastly improved the performance of django.dispatch (and added tests!). Thanks to Brian Harring for the patch. Note that one of the new tests fails under sqlite currently; it's not clear if this is a sqlite problem or a problem with the tests, but it appears not to be a problem with the dispatcher itself.

comment:6 by (removed), 17 years ago

Well... I guess since you said please. ;)

Which test is failing? The code *should* be totally unaware of sqlite also- quick grep of backends namespace, nothing in there seems to know of dispatch, so I'm semi at a loss why you're running into issues. Additionally... my testing was strictly against sqlite, so I'm a fair bit curious ;)

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