Code

Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#3439 closed (fixed)

django.dispatch.* is grossly slow

Reported by: (removed) Owned by: adrian
Component: Core (Other) Version: master
Severity: Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

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) 7 years ago.
dispatcher refactoring round 1.
dispatch.patch (17.8 KB) - added by (removed) 7 years ago.
import of upstream tests, fix deregistration bug

Download all attachments as: .zip

Change History (8)

Changed 7 years ago by (removed)

dispatcher refactoring round 1.

comment:1 Changed 7 years ago by SmileyChris

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

Changed 7 years ago by (removed)

import of upstream tests, fix deregistration bug

comment:2 Changed 7 years ago by Simon G. <dev@…>

  • Triage Stage changed from Unreviewed to Accepted

comment:3 Changed 7 years ago by (removed)

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 Changed 7 years ago 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?

comment:5 Changed 7 years ago by jacob

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

(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 Changed 7 years ago by (removed)

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 Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.