Code

Opened 6 years ago

Closed 6 years ago

Last modified 3 years ago

#6814 closed (fixed)

Signals are slow.

Reported by: keithb Owned by: jdunck
Component: Core (Other) Version: master
Severity: Keywords: sprint signals performance
Cc: jdunck@…, kbussell@…, leo.soto@…, till@…, fijall@…, holger@…, anto.cuni@…, pedronis@…, gabor@…, flosch@…, marinho, i_i Triage Stage: Design decision needed
Has patch: yes Needs documentation: yes
Needs tests: yes Patch needs improvement: yes
Easy pickings: UI/UX:

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 keithb 6 years ago.
Initial attempt at the signal refactor
faster_signals-2.diff (53.4 KB) - added by jdunck 6 years ago.
tentative implementation: "strong_list" in timings.
faster_signals_bench.py (5.9 KB) - added by jdunck 6 years ago.
ugly code to do benchmarking
signals_8156-2.diff (68.0 KB) - added by jacob 6 years ago.
Jeremy's latest patch
signals_8220.diff (67.7 KB) - added by keithb 6 years ago.
dispatch_uid as a parameter to connect/disconnect

Download all attachments as: .zip

Change History (26)

Changed 6 years ago by keithb

Initial attempt at the signal refactor

comment:1 Changed 6 years ago by keithb

  • Needs documentation set
  • Needs tests set
  • Patch needs improvement unset
  • Status changed from new to assigned

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 Changed 6 years ago by jdunck

  • Owner changed from keithb to jdunck
  • Status changed from assigned to new

comment:3 Changed 6 years ago by leosoto

  • Cc leo.soto@… added

comment:4 Changed 6 years ago by jdunck

  • Status changed from new to assigned

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.

Changed 6 years ago by jdunck

tentative implementation: "strong_list" in timings.

Changed 6 years ago by jdunck

ugly code to do benchmarking

comment:5 Changed 6 years ago by jdunck

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 Changed 6 years ago by jdunck

  • Cc jdunck@… added

comment:7 Changed 6 years ago by anonymous

  • Cc till@… added

comment:8 Changed 6 years ago by Simon Greenhill

  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Design decision needed

comment:9 Changed 6 years ago by fijal

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

comment:10 Changed 6 years ago by jdunck

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

I'll make an updated patch ASAP.

comment:11 Changed 6 years ago by jdunck

*when I attached*, not checked in.

comment:12 Changed 6 years ago by fijal

  • Cc fijall@…, holger@…, anto.cuni@…, pedronis@… added

comment:13 Changed 6 years ago by anonymous

  • Cc gabor@… added

comment:14 Changed 6 years ago by anonymous

  • Cc flosch@… added

comment:15 Changed 6 years ago by jdunck

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

Changed 6 years ago by jacob

Jeremy's latest patch

comment:16 Changed 6 years ago by marinho

  • Cc marinho added

Changed 6 years ago by keithb

dispatch_uid as a parameter to connect/disconnect

comment:17 Changed 6 years ago by keithb

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 Changed 6 years ago by i_i

  • Cc ii added

comment:19 Changed 6 years ago by i_i

  • Cc i_i added; ii removed

comment:20 Changed 6 years ago by jacob

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

(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 Changed 3 years ago by jacob

  • milestone 1.0 beta deleted

Milestone 1.0 beta deleted

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.