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

asyncio: Optimize get_event_loop and _get_running_loop #75531

Closed
jimmylai mannequin opened this issue Sep 5, 2017 · 11 comments
Closed

asyncio: Optimize get_event_loop and _get_running_loop #75531

jimmylai mannequin opened this issue Sep 5, 2017 · 11 comments
Labels
3.7 (EOL) end of life performance Performance or resource usage topic-asyncio

Comments

@jimmylai
Copy link
Mannequin

jimmylai mannequin commented Sep 5, 2017

BPO 31350
Nosy @vstinner, @1st1, @Mariatta, @jimmylai
PRs
  • bpo-31350: Optimize get_event_loop and _get_running_loop #3347
  • [3.6] bpo-31350: Optimize get_event_loop and _get_running_loop (GH-3347) #3373
  • Files
  • bench_asyncio.py
  • bench_get_event_loop.py
  • 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 = None
    closed_at = <Date 2017-09-06.03:06:58.314>
    created_at = <Date 2017-09-05.17:19:35.454>
    labels = ['expert-asyncio', '3.7', 'performance']
    title = 'asyncio: Optimize get_event_loop and _get_running_loop'
    updated_at = <Date 2017-09-06.03:06:58.313>
    user = 'https://github.com/jimmylai'

    bugs.python.org fields:

    activity = <Date 2017-09-06.03:06:58.313>
    actor = 'Mariatta'
    assignee = 'none'
    closed = True
    closed_date = <Date 2017-09-06.03:06:58.314>
    closer = 'Mariatta'
    components = ['asyncio']
    creation = <Date 2017-09-05.17:19:35.454>
    creator = 'jimmylai'
    dependencies = []
    files = ['47120', '47122']
    hgrepos = []
    issue_num = 31350
    keywords = []
    message_count = 11.0
    messages = ['301342', '301343', '301344', '301348', '301356', '301357', '301358', '301359', '301416', '301433', '301434']
    nosy_count = 4.0
    nosy_names = ['vstinner', 'yselivanov', 'Mariatta', 'jimmylai']
    pr_nums = ['3347', '3373']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'performance'
    url = 'https://bugs.python.org/issue31350'
    versions = ['Python 3.6', 'Python 3.7']

    @jimmylai
    Copy link
    Mannequin Author

    jimmylai mannequin commented Sep 5, 2017

    get_event_loop() and _get_running_loop() can be faster.

    Case Time Mean Improve
    No Change 7.323 +- 0.172 7.323 0.00%
    Remove class _RunningLoop 6.513 +- 0.115 6.513 -11.06%
    Expand _get_running_loop() inside get_event_loop() 5.851 +- 0.160 5.851 -20.10%
    Use Tuple instead of two attributes 6.179 +- 0.099 6.179 -15.62%
    Tuple + Remove _RunningLoop 6.026 +- 0.123 6.026 -17.71%
    Tuple + return ternary + Remove _RunningLoop 6.060 +- 0.111 6.06 -17.25%
    Combine all four optimizations 4.735 +- 0.111 4.735 -35.34%
    Remove class _RunningLoop + Use Tuple instead of two attributes 6.241 +- 0.097 6.241 -14.78%

    Experimenting with different techniques to optimize get_event_loop and _get_running_loop.

    After discuss with Yuri, decide not to expand _get_running_loop inside get_event_loop.
    Combine tuple in _running_loop and Remove _RunningLoop (just use threading.local) can achieve the best improvement: 17.71% faster.

    @jimmylai jimmylai mannequin added 3.7 (EOL) end of life topic-asyncio performance Performance or resource usage labels Sep 5, 2017
    @vstinner
    Copy link
    Member

    vstinner commented Sep 5, 2017

    Can you please provide the code of your benchmark?

    @vstinner vstinner changed the title Optimize get_event_loop and _get_running_loop asyncio: Optimize get_event_loop and _get_running_loop Sep 5, 2017
    @jimmylai
    Copy link
    Mannequin Author

    jimmylai mannequin commented Sep 5, 2017

    Benchmark script: Run 10 times to get mean and stdev

    import asyncio
    import time
    
    async def async_get_loop():
        start_time = time.time()
        for _ in range(5000000):
            asyncio.get_event_loop()
        return time.time() - start_time
    
    loop = asyncio.get_event_loop()
    results = []
    for _ in range(10):
        start_time = time.time()
        result = loop.run_until_complete(async_get_loop())
        results.append(result)
    
    import statistics
    print("elapse time: %.3lf +- %.3lf secs" % (statistics.mean(results), statistics.stdev(results)))

    @vstinner
    Copy link
    Member

    vstinner commented Sep 5, 2017

    I suggest to use my perf module to run benchmark, especially if the tested function takes less than 1 ms, which is the case here.

    Attached benchmark script calls asyncio.get_event_loop(). Result on the master branch with PR 3347:

    haypo@selma$ ./python ~/bench_asyncio.py --inherit=PYTHONPATH -o patch.json
    haypo@selma$ ./python ~/bench_asyncio.py --inherit=PYTHONPATH -o ref.json
    haypo@selma$ ./python -m perf compare_to ref.json patch.json

    Mean +- std dev: [ref] 881 ns +- 42 ns -> [patch] 859 ns +- 14 ns: 1.03x faster (-3%)

    I'm not convinced that the PR is worth it. 3% is not interesting on a micro benchmark.

    Or is there an issue in my benchmark?

    @1st1
    Copy link
    Member

    1st1 commented Sep 5, 2017

    I'm not convinced that the PR is worth it. 3% is not interesting on a micro benchmark.

    I found a small issue in the PR (left a comment in the PR).

    I think using a tuple is still a good idea (even if the speedup is tiny) because logically, both attributes on that threading.local() object are always set and read at the same time. Essentially, it's a pair of (loop, pid), so using a tuple here makes the code easier to reason about.

    @vstinner
    Copy link
    Member

    vstinner commented Sep 5, 2017

    If the motivation is correctness and not performance, please adjust the issue and PR description :-)

    @1st1
    Copy link
    Member

    1st1 commented Sep 5, 2017

    Or is there an issue in my benchmark?

    Yes. The correct benchmark would be to measure get_event_loop performance from within a running event loop.

    @vstinner
    Copy link
    Member

    vstinner commented Sep 5, 2017

    According to Jimmy, asyncio.get_event_loop() behaves differently if it's called while an event loop is running. So my first benchmark was wrong.

    Attached bench_get_event_loop.py measures asyncio.get_event_loop() performance when an event loop is running. I get a different result:

    haypo@selma$ ./python -m perf compare_to ref.json patch.json
    Mean +- std dev: [ref] 555 ns +- 11 ns -> [patch] 498 ns +- 11 ns: 1.11x faster (-10%)

    Ok, now it's 10% faster :-)

    @1st1
    Copy link
    Member

    1st1 commented Sep 6, 2017

    New changeset 80bbe6a by Yury Selivanov (jimmylai) in branch 'master':
    bpo-31350: Optimize get_event_loop and _get_running_loop (bpo-3347)
    80bbe6a

    @Mariatta
    Copy link
    Member

    Mariatta commented Sep 6, 2017

    New changeset ff125e1 by Mariatta (Miss Islington (bot)) in branch '3.6':
    bpo-31350: Optimize get_event_loop and _get_running_loop (GH-3347) (GH-3373)
    ff125e1

    @Mariatta
    Copy link
    Member

    Mariatta commented Sep 6, 2017

    This has been backported.
    Thanks all :)

    @Mariatta Mariatta closed this as completed Sep 6, 2017
    @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
    3.7 (EOL) end of life performance Performance or resource usage topic-asyncio
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants