Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

improvements to sched.py #52930

Closed
josiahcarlson mannequin opened this issue May 11, 2010 · 19 comments
Closed

improvements to sched.py #52930

josiahcarlson mannequin opened this issue May 11, 2010 · 19 comments
Assignees
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@josiahcarlson
Copy link
Mannequin

josiahcarlson mannequin commented May 11, 2010

BPO 8684
Nosy @rhettinger, @josiahcarlson, @mdickinson, @pitrou, @giampaolo
Dependencies
  • bpo-8687: sched.py module doesn't have a test suite
  • Files
  • sched.patch: some improvements to the scheduler library
  • sched-thread-safe.patch
  • sched-thread-safe.patch
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/giampaolo'
    closed_at = <Date 2011-12-19.18:13:26.078>
    created_at = <Date 2010-05-11.04:27:47.594>
    labels = ['type-feature', 'library']
    title = 'improvements to sched.py'
    updated_at = <Date 2011-12-19.18:13:26.077>
    user = 'https://github.com/josiahcarlson'

    bugs.python.org fields:

    activity = <Date 2011-12-19.18:13:26.077>
    actor = 'giampaolo.rodola'
    assignee = 'giampaolo.rodola'
    closed = True
    closed_date = <Date 2011-12-19.18:13:26.078>
    closer = 'giampaolo.rodola'
    components = ['Library (Lib)']
    creation = <Date 2010-05-11.04:27:47.594>
    creator = 'josiahcarlson'
    dependencies = ['8687']
    files = ['17289', '23903', '23925']
    hgrepos = []
    issue_num = 8684
    keywords = ['patch', 'needs review']
    message_count = 19.0
    messages = ['105483', '105497', '105499', '105502', '105504', '112778', '115697', '148409', '149190', '149281', '149283', '149284', '149287', '149288', '149290', '149442', '149741', '149883', '149884']
    nosy_count = 7.0
    nosy_names = ['rhettinger', 'josiahcarlson', 'mark.dickinson', 'pitrou', 'giampaolo.rodola', 'neologix', 'python-dev']
    pr_nums = []
    priority = 'low'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue8684'
    versions = ['Python 3.3']

    @josiahcarlson
    Copy link
    Mannequin Author

    josiahcarlson mannequin commented May 11, 2010

    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 bpo-1641 without mucking with asyncore. It's reach is reduced and simplified, which should make maintenance a bit easier.

    @josiahcarlson josiahcarlson mannequin assigned giampaolo May 11, 2010
    @josiahcarlson josiahcarlson mannequin added stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels May 11, 2010
    @pitrou
    Copy link
    Member

    pitrou commented May 11, 2010

    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.

    @pitrou
    Copy link
    Member

    pitrou commented May 11, 2010

    By the way, the patch lacks docs for new public APIs.

    @giampaolo
    Copy link
    Contributor

    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.

    @giampaolo
    Copy link
    Contributor

    Created bpo-8687 to address the test suite problem.

    @giampaolo
    Copy link
    Contributor

    sched.py tests have been checked in.

    @giampaolo
    Copy link
    Contributor

    Josiah are you still interested in this?

    @giampaolo
    Copy link
    Contributor

    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.

    @giampaolo
    Copy link
    Contributor

    Updated patch adding a synchronized argument to scheduler class and updating doc is in attachment.

    @pitrou
    Copy link
    Member

    pitrou commented Dec 12, 2011

    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.

    @giampaolo
    Copy link
    Contributor

    Are you suggesting to enable thread-synchronization by default and get rid of explicit "synchronized" argument?

    @pitrou
    Copy link
    Member

    pitrou commented Dec 12, 2011

    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.

    @giampaolo
    Copy link
    Contributor

    This is what I get by using bench.py script attached to bpo-13451:

    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).

    @pitrou
    Copy link
    Member

    pitrou commented Dec 12, 2011

    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.

    @giampaolo
    Copy link
    Contributor

    New patch in attachment. I'll commit it later today.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Dec 14, 2011

    New changeset f5aed0dba844 by Giampaolo Rodola' in branch 'default':
    Fix bpo-8684: make sched.scheduler class thread-safe
    http://hg.python.org/cpython/rev/f5aed0dba844

    @neologix
    Copy link
    Mannequin

    neologix mannequin commented Dec 18, 2011

    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

    @neologix neologix mannequin reopened this Dec 18, 2011
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Dec 19, 2011

    New changeset 50267d2bb320 by Giampaolo Rodola' in branch 'default':
    (bug bpo-8684) fix 'fedora without thread buildbot' as per http://bugs.python.org/issue8684
    http://hg.python.org/cpython/rev/50267d2bb320

    @giampaolo
    Copy link
    Contributor

    This should now be fixed. Thanks for signaling.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants