classification
Title: improvements to sched.py
Type: enhancement Stage: resolved
Components: Library (Lib) Versions: Python 3.3
process
Status: closed Resolution: fixed
Dependencies: 8687 Superseder:
Assigned To: giampaolo.rodola Nosy List: giampaolo.rodola, josiahcarlson, mark.dickinson, neologix, pitrou, python-dev, rhettinger
Priority: low Keywords: needs review, patch

Created on 2010-05-11 04:27 by josiahcarlson, last changed 2011-12-19 18:13 by giampaolo.rodola. This issue is now closed.

Files
File name Uploaded Description Edit
sched.patch josiahcarlson, 2010-05-11 04:27 some improvements to the scheduler library
sched-thread-safe.patch giampaolo.rodola, 2011-12-10 21:10 review
sched-thread-safe.patch giampaolo.rodola, 2011-12-12 11:41 review
Messages (19)
msg105483 - (view) Author: Josiah Carlson (josiahcarlson) * Date: 2010-05-11 04:27
This patch is against Python trunk, but it could be easily targeted for Python 3.2 .  It is meant to extract the scheduler updates from issue1641 without mucking with asyncore.  It's reach is reduced and simplified, which should make maintenance a bit easier.
msg105497 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-05-11 11:12
This must be retargeted to 3.2. Also, the patch lacks some tests.
It seems PEP 8 compliance could be better: function names shouldn't be CamelCased.
Is LocalSynchronize() an implementation detail rather than a public API? If so, I think it would be better if it started with an underscore.
msg105499 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-05-11 11:15
By the way, the patch lacks docs for new public APIs.
msg105502 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2010-05-11 12:21
What are the enhancements introduced by this patch?
A faster cancel() implementation?
If so some simple benchmarks showing the speed improvement of cancel() and eventually also other methods like enterabs() and run() would be nice to have.

I've noticed there are no existing tests for the sched module.
This is very bad. I think the first thing to do is adding tests, make sure they pass with the current implementation, then apply the patch and make sure they keep passing.

Obviously it is also crucial that this patch is fully backward compatible with the current implementation.
msg105504 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2010-05-11 12:31
Created issue 8687 to address the test suite problem.
msg112778 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2010-08-04 09:32
sched.py tests have been checked in.
msg115697 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2010-09-06 11:03
Josiah are you still interested in this?
msg148409 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2011-11-26 14:40
Looking back at this patch, I think we can extract the thread-synchronization parts and the peek() method, as they're both valuable additions, especially the first one.

The very sched doc says:

> In multi-threaded environments, the scheduler class has limitations 
> with respect to thread-safety, inability to insert a new task before 
> the one currently pending in a running scheduler, and holding up the 
> main thread until the event queue is empty. Instead, the preferred 
> approach is to use the threading.Timer class instead.
msg149190 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2011-12-10 21:10
Updated patch adding a synchronized argument to scheduler class and updating doc is in attachment.
msg149281 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-12-12 09:52
I'm not convinced by the decorator approach. Why not simply add "with self.lock" at the beginning of each protected method? It would actually save a function call indirection.
msg149283 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2011-12-12 10:09
Are you suggesting to enable thread-synchronization by default and get rid of explicit "synchronized" argument?
msg149284 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-12-12 10:15
> Are you suggesting to enable thread-synchronization by default and get
> rid of explicit "synchronized" argument?

That would be even better indeed, if you find out that there's no
significant performance degradation.
msg149287 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2011-12-12 11:13
This is what I get by using bench.py script attached to issue13451:

CURRENT VERSION (NO LOCK)

test_cancel                    : time=0.67457 : calls=1 : stdev=0.00000
test_empty                     : time=0.00025 : calls=1 : stdev=0.00000
test_enter                     : time=0.00302 : calls=1 : stdev=0.00000
test_queue                     : time=6.31787 : calls=1 : stdev=0.00000
test_run                       : time=0.00741 : calls=1 : stdev=0.00000


LOCK WITH DECORATOR (no synchronization)

test_cancel                    : time=0.70455 : calls=1 : stdev=0.00000
test_empty                     : time=0.00050 : calls=1 : stdev=0.00000
test_enter                     : time=0.00405 : calls=1 : stdev=0.00000
test_queue                     : time=6.23341 : calls=1 : stdev=0.00000
test_run                       : time=0.00776 : calls=1 : stdev=0.00000


LOCK WITHOUT DECORATOR (always synchronized)

test_cancel                    : time=0.69625 : calls=1 : stdev=0.00000
test_empty                     : time=0.00053 : calls=1 : stdev=0.00000
test_enter                     : time=0.00397 : calls=1 : stdev=0.00000
test_queue                     : time=6.36999 : calls=1 : stdev=0.00000
test_run                       : time=0.00783 : calls=1 : stdev=0.00000


Versions #2 and #3 have the same cost, so it's better to get rid of the explicit argument and always use the lock (version #3).
Differences between #1 and #3 suggest that introducing the lock obviously have a cost though.
It's not too high IMO, but I couldn't say whether it's acceptable or not.
Maybe it makes sense to provide it as a separate class (SynchronizedScheduler).
msg149288 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-12-12 11:17
> Versions #2 and #3 have the same cost, so it's better to get rid of
> the explicit argument and always use the lock (version #3).
> Differences between #1 and #3 suggest that introducing the lock
> obviously have a cost though.
> It's not too high IMO, but I couldn't say whether it's acceptable or
> not.

It looks quite negligible to me. Nobody should be affected in practice.
msg149290 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2011-12-12 11:41
New patch in attachment. I'll commit it later today.
msg149442 - (view) Author: Roundup Robot (python-dev) Date: 2011-12-14 12:34
New changeset f5aed0dba844 by Giampaolo Rodola' in branch 'default':
Fix #8684: make sched.scheduler class thread-safe
http://hg.python.org/cpython/rev/f5aed0dba844
msg149741 - (view) Author: Charles-Fran├žois Natali (neologix) * (Python committer) Date: 2011-12-18 09:52
This broken the "Fedora without thread buildbot", since sched now requires the threading module:
http://www.python.org/dev/buildbot/all/builders/AMD64 Fedora without threads 3.x/builds/1250/steps/test/logs/stdio
msg149883 - (view) Author: Roundup Robot (python-dev) Date: 2011-12-19 18:12
New changeset 50267d2bb320 by Giampaolo Rodola' in branch 'default':
(bug #8684) fix 'fedora without thread buildbot' as per http://bugs.python.org/issue8684
http://hg.python.org/cpython/rev/50267d2bb320
msg149884 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2011-12-19 18:13
This should now be fixed. Thanks for signaling.
History
Date User Action Args
2011-12-19 18:13:26giampaolo.rodolasetstatus: open -> closed

messages: + msg149884
2011-12-19 18:12:08python-devsetmessages: + msg149883
2011-12-18 09:52:05neologixsetstatus: closed -> open
nosy: + neologix
messages: + msg149741

2011-12-14 12:36:09giampaolo.rodolasetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2011-12-14 12:34:41python-devsetnosy: + python-dev
messages: + msg149442
2011-12-12 11:41:05giampaolo.rodolasetfiles: + sched-thread-safe.patch
nosy: + rhettinger
messages: + msg149290

2011-12-12 11:17:16pitrousetmessages: + msg149288
2011-12-12 11:13:32giampaolo.rodolasetmessages: + msg149287
2011-12-12 10:15:47pitrousetmessages: + msg149284
2011-12-12 10:09:24giampaolo.rodolasetmessages: + msg149283
2011-12-12 09:52:18pitrousetstage: patch review
messages: + msg149281
versions: + Python 3.3, - Python 3.2
2011-12-10 21:10:10giampaolo.rodolasetfiles: + sched-thread-safe.patch

messages: + msg149190
2011-11-26 14:40:12giampaolo.rodolasetmessages: + msg148409
2011-01-07 14:49:41mark.dickinsonsetnosy: + mark.dickinson
2010-09-06 11:03:12giampaolo.rodolasetkeywords: patch, patch, needs review

messages: + msg115697
2010-08-04 09:32:28giampaolo.rodolasetkeywords: patch, patch, needs review

messages: + msg112778
2010-05-11 13:07:31r.david.murraysetkeywords: patch, patch, needs review
dependencies: + sched.py module doesn't have a test suite
superseder: sched.py module doesn't have a test suite ->
2010-05-11 12:31:52giampaolo.rodolasetkeywords: patch, patch, needs review
superseder: sched.py module doesn't have a test suite
messages: + msg105504
2010-05-11 12:21:52giampaolo.rodolasetkeywords: patch, patch, needs review

messages: + msg105502
2010-05-11 11:15:18pitrousetkeywords: patch, patch, needs review

messages: + msg105499
2010-05-11 11:12:02pitrousetversions: + Python 3.2
nosy: + pitrou

messages: + msg105497

keywords: patch, patch, needs review
2010-05-11 04:27:48josiahcarlsoncreate