| 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 without mucking with asyncore. It's reach is reduced and simplified, which should make maintenance a bit easier. |
|
|
| msg105497 - (view) |
Author: Antoine Pitrou (pitrou) *  |
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) * |
Date: 2010-05-11 11:15 |
| By the way, the patch lacks docs for new public APIs. |
|
|
| msg105502 - (view) |
Author: Giampaolo Rodola' (giampaolo.rodola) * |
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) * |
Date: 2010-05-11 12:31 |
| Created issue 8687 to address the test suite problem. |
|
|
| msg112778 - (view) |
Author: Giampaolo Rodola' (giampaolo.rodola) * |
Date: 2010-08-04 09:32 |
| sched.py tests have been checked in. |
|
|
| msg115697 - (view) |
Author: Giampaolo Rodola' (giampaolo.rodola) * |
Date: 2010-09-06 11:03 |
| Josiah are you still interested in this? |
|
|
| msg148409 - (view) |
Author: Giampaolo Rodola' (giampaolo.rodola) * |
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) * |
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) * |
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) * |
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) * |
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) * |
Date: 2011-12-12 11:13 |
| This is what I get by using bench.py script attached to : 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) * |
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) * |
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) * |
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) * |
Date: 2011-12-19 18:13 |
| This should now be fixed. Thanks for signaling. |
|
|