Opened 16 years ago

Closed 16 years ago

Last modified 12 years ago

#6814 closed (fixed)

Signals are slow.

Reported by: Keith Bussell Owned by: Jeremy Dunck
Component: Core (Other) Version: dev
Severity: Keywords: sprint signals performance
Cc: jdunck@…, kbussell@…, leo.soto@…, till@…, fijall@…, holger@…, anto.cuni@…, pedronis@…, gabor@…, flosch@…, Marinho Brandão, Ivan Illarionov Triage Stage: Design decision needed
Has patch: yes Needs documentation: yes
Needs tests: yes Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

As per discussions with Jacob, the Signals library is unnecessarily heavy-weight. It's currently based on PyDispatcher, which supports a superset of what's needed for Django.

Design Decisions

  • Signals instances should declare what arguments are allowed to be sent
  • get rid of Any, Anonymous
  • get rid of robustApply. It slows down send() calls, and it breaks some implementations of Python

(work in progress. tired. will update this later.)

Attachments (5)

faster_signals.diff (33.1 KB ) - added by Keith Bussell 16 years ago.
Initial attempt at the signal refactor
faster_signals-2.diff (53.4 KB ) - added by Jeremy Dunck 16 years ago.
tentative implementation: "strong_list" in timings.
faster_signals_bench.py (5.9 KB ) - added by Jeremy Dunck 16 years ago.
ugly code to do benchmarking
signals_8156-2.diff (68.0 KB ) - added by Jacob 16 years ago.
Jeremy's latest patch
signals_8220.diff (67.7 KB ) - added by Keith Bussell 16 years ago.
dispatch_uid as a parameter to connect/disconnect

Download all attachments as: .zip

Change History (26)

by Keith Bussell, 16 years ago

Attachment: faster_signals.diff added

Initial attempt at the signal refactor

comment:1 by Keith Bussell, 16 years ago

Needs documentation: set
Needs tests: set
Status: newassigned

The tests need to be updated to conform to the new API (I killed some of them for now), and the docs need a rewrite.

comment:2 by Jeremy Dunck, 16 years ago

Owner: changed from Keith Bussell to Jeremy Dunck
Status: assignednew

comment:3 by Leo Soto M., 16 years ago

Cc: leo.soto@… added

comment:4 by Jeremy Dunck, 16 years ago

Status: newassigned

Feature tradeoffs:

Strong/weak receiver connections - strong-only is a big win-- about 2x performance. Any receivers going out of scope (and not to be called) should be explicitly disconnected if we remove support for weak receivers.

.send API enforcement - about a 20% overhead to ensure signals are sent properly. Worth it?

API whittling for extensibility: Leo has correctly pointed out that kwargs can be used for the same purpose. Functions that accept kwargs are fast-pathed, but maybe we should change to passing all receiver arguments with kwargs to avoid the overhead entirely?

kwargs vs whittling: 20% overhead to match APIs

In general, the latest patch optimizes for .send at the expense of extra overhead in .connect and .disconnect. .connect and .disconnect are O(n) on number of active receivers for that signal; I don't anticipate that being a big problem, though.

One other enhancement I thought might be good is to have a Model.pre_init signal, similar to Model.DoesNotExist so that .connect(class=Model, ...) would allow listening to, say, pre_init only for instantiation of that model. Other similar class-based receiver lists might speed things up in some cases; the down-side is that each receiver list has some overhead, even if empty.

Regardless of the outcome, the patch also adds regression tests for signals which I believe would be useful.

The attached diff uses a list of receivers with strong references; it has best performance of the alternatives.

Timing details:

original, including pyc generation
0 receivers test 2.16365003586
1 receiver test 18.8934309483; no-op overhead for comparison: 0.341604948044
10 receivers test 101.300621986; no-op overhead for comparison: 4.38265109062

weak dict, weak=True, keithb "faster_signals.diff" Mar 18 2008
0 receivers test 4.87185907364
1 receiver test 11.0454790592
10 receivers test 52.751529932

weak dict, weak=False, keithb "faster_signals.diff" Mar 18 2008
0 receivers test 4.82329916954
1 receiver test 11.550688982
10 receivers test 60.9468040466

weak_dict, including pyc generation
0 receivers test 1.70886898041
1 receiver test 11.0185530186
10 receivers test 50.5068600178

strong_dict, including pyc generation
0 receivers test 1.86286401749
1 receiver test 6.80570101738
10 receivers test 23.5818219185

strong_list, including pyc generation
0 receivers test 2.36390304565
1 receiver test 6.74138212204
10 receivers test 18.7774899006

strong_list, including pyc generation; no api enforcement
0 receivers test 2.36659097672
1 receiver test 4.90005588531
10 receivers test 16.6197218895


separately, comparing performance of argument matching:

with arg matching:

0 arg bench 6.56428813934
1 arg bench 7.65555596352
10 arg bench 19.9225678444

without arg matching:

0 arg bench 5.6941409111
1 arg bench 6.54712295532
10 arg bench 16.4016561508

Other notes:

  • It's mildly annoying that a signal doesn't know its own name.
  • An error from any signal handler breaks all signals. It's the way it's always been. Bad?
  • Switching all core and contrib to signal.send rather than dispatcher.send saves half of overhead when 0 receivers present. Assuming the direction is good, an updated patch should make these changes.

by Jeremy Dunck, 16 years ago

Attachment: faster_signals-2.diff added

tentative implementation: "strong_list" in timings.

by Jeremy Dunck, 16 years ago

Attachment: faster_signals_bench.py added

ugly code to do benchmarking

comment:5 by Jeremy Dunck, 16 years ago

Feature tradeoffs:

  • 2x performance if we drop weak receiver connections. Any receivers going out of scope (and not to be called) should be explicitly disconnected if we remove support for weak receivers.
  • 20% savings if we drop .send API enforcement; ensures signals are sent properly. Worth it?
  • 20% savings if we require receivers to take kwargs to avoid API whittling while still supporting extensibility
    • Functions that accept kwargs are already fast-pathed; the 20% overhead is for whittling receivers that don't take kwargs.
  • 2x performance when 0 receivers present: switch all core and contrib to signal.send rather than dispatcher.send
    • Assuming the direction is good, an updated patch should make these changes.

In general, the latest patch optimizes for .send at the expense of extra overhead in .connect and .disconnect. .connect and .disconnect are O(n) on number of active receivers for that signal; I don't anticipate that being a big problem, though.

One other enhancement I (well, David Gouldin) thought might be good is to have a Model.pre_init signal, similar to Model.DoesNotExist so that .connect(class=Model, ...) would allow listening to, say, pre_init only for instantiation of that model. Other similar class-based receiver lists might speed things up in some cases; the down-side is that each receiver list has some overhead, even if empty.

Regardless of the outcome, the patch also adds regression tests for signals which I believe would be useful.

Timing details:

original, including pyc generation
0 receivers test 2.16365003586
1 receiver test 18.8934309483; no-op overhead for comparison:  0.341604948044
10 receivers test 101.300621986; no-op overhead for comparison: 4.38265109062

weak dict, weak=True, keithb "faster_signals.diff" Mar 18 2008
0 receivers test 4.87185907364
1 receiver test 11.0454790592
10 receivers test 52.751529932

weak dict, weak=False, keithb "faster_signals.diff" Mar 18 2008
0 receivers test 4.82329916954
1 receiver test 11.550688982
10 receivers test 60.9468040466

weak_dict, including pyc generation
0 receivers test 1.70886898041
1 receiver test 11.0185530186
10 receivers test 50.5068600178

strong_dict, including pyc generation
0 receivers test 1.86286401749
1 receiver test 6.80570101738
10 receivers test 23.5818219185

strong_list, including pyc generation
0 receivers test 2.36390304565
1 receiver test 6.74138212204
10 receivers test 18.7774899006

strong_list, including pyc generation; no api enforcement
0 receivers test 2.36659097672
1 receiver test 4.90005588531
10 receivers test 16.6197218895 

separately, comparing performance of argument matching:

with arg matching:
    0 arg bench 6.56428813934
    1 arg bench 7.65555596352
    10 arg bench 19.9225678444
without arg matching:    
    0 arg bench 5.6941409111
    1 arg bench 6.54712295532
    10 arg bench 16.4016561508

Other notes:

  • It's mildly annoying that a signal doesn't know its own name.
  • An error from any signal handler breaks all signals. It's the way it's always been. Bad?

comment:6 by Jeremy Dunck, 16 years ago

Cc: jdunck@… added

comment:7 by anonymous, 16 years ago

Cc: till@… added

comment:8 by Simon Greenhill, 16 years ago

Patch needs improvement: set
Triage Stage: UnreviewedDesign decision needed

comment:9 by Maciej Fijalkowski, 16 years ago

Patch (faster-signals2.diff) breakes test suite.

comment:10 by Jeremy Dunck, 16 years ago

Mmmph. All tests passed when I checked in, but bitrot has apparently set in.

I'll make an updated patch ASAP.

comment:11 by Jeremy Dunck, 16 years ago

*when I attached*, not checked in.

comment:12 by Maciej Fijalkowski, 16 years ago

Cc: fijall@… holger@… anto.cuni@… pedronis@… added

comment:13 by anonymous, 16 years ago

Cc: gabor@… added

comment:14 by anonymous, 16 years ago

Cc: flosch@… added

comment:15 by Jeremy Dunck, 16 years ago

For some reason, unable to upload the most recent patch.

Included as an attachment on a django-dev thread:
http://groups.google.com/group/django-developers/browse_thread/thread/815f76ad7e823cbf

by Jacob, 16 years ago

Attachment: signals_8156-2.diff added

Jeremy's latest patch

comment:16 by Marinho Brandão, 16 years ago

Cc: Marinho Brandão added

by Keith Bussell, 16 years ago

Attachment: signals_8220.diff added

dispatch_uid as a parameter to connect/disconnect

comment:17 by Keith Bussell, 16 years ago

signals_8220.diff replaces signals_8156-2.diff by adding the suggestions made by Jacob.

Differences:

  • dispatch_uid was moved from an attribute on the receiver to a parameter passed to connect/disconnect.
  • tests updated

comment:18 by Ivan Illarionov, 16 years ago

Cc: ii added

comment:19 by Ivan Illarionov, 16 years ago

Cc: Ivan Illarionov added; ii removed

comment:20 by Jacob, 16 years ago

Resolution: fixed
Status: assignedclosed

(In [8223]) Major refactoring of django.dispatch with an eye towards speed. The net result is that signals are up to 90% faster.

Though some attempts and backwards-compatibility were made, speed trumped compatibility. Thus, as usual, check BackwardsIncompatibleChanges for the complete list of backwards-incompatible changes.

Thanks to Jeremy Dunck and Keith Busell for the bulk of the work; some ideas from Brian Herring's previous work (refs #4561) were incorporated.

Documentation is, sigh, still forthcoming.

Fixes #6814 and #3951 (with the new dispatch_uid argument to connect).

comment:15 by Jacob, 12 years ago

milestone: 1.0 beta

Milestone 1.0 beta deleted

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