Django

Code

Ticket #3439 (closed: fixed)

Opened 2 years ago

Last modified 2 years ago

django.dispatch.* is grossly slow

Reported by: Brian Harring <ferringb@gmail.com> Assigned to: adrian
Milestone: Component: Core framework
Version: SVN Keywords:
Cc: Triage Stage: Accepted
Has patch: 1 Needs documentation: 0
Needs tests: 0 Patch needs improvement: 0

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

dispatch-refactoring.patch (6.8 kB) - added by Brian Harring <ferringb@gmail.com> on 02/05/07 19:08:51.
dispatcher refactoring round 1.
dispatch.patch (17.8 kB) - added by Brian Harring <ferringb@gmail.com> on 02/06/07 21:34:42.
import of upstream tests, fix deregistration bug

Change History

02/05/07 19:08:51 changed by Brian Harring <ferringb@gmail.com>

  • attachment dispatch-refactoring.patch added.

dispatcher refactoring round 1.

02/06/07 15:08:04 changed by SmileyChris

  • needs_better_patch changed.
  • needs_tests changed.
  • needs_docs changed.

02/06/07 21:34:42 changed by Brian Harring <ferringb@gmail.com>

  • attachment dispatch.patch added.

import of upstream tests, fix deregistration bug

02/17/07 04:26:29 changed by Simon G. <dev@simon.net.nz>

  • stage changed from Unreviewed to Accepted.

02/17/07 05:11:36 changed by Brian Harring <ferringb@gmail.com>

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

02/25/07 21:12:11 changed by jacob

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?

02/25/07 21:17:04 changed by jacob

  • status changed from new to closed.
  • resolution set to fixed.

(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.

02/25/07 21:42:25 changed by ferringb@gmail.com

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


Add/Change #3439 (django.dispatch.* is grossly slow)




Change Properties
Action