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

Add checks for if instruments exist before running instrument functions. #628

Merged
merged 2 commits into from
Aug 25, 2018
Merged

Add checks for if instruments exist before running instrument functions. #628

merged 2 commits into from
Aug 25, 2018

Conversation

Fuyukai
Copy link
Member

@Fuyukai Fuyukai commented Aug 23, 2018

This provides a small speed-up.

Microbenchmark script:

import time

import trio


async def main():
    q = trio.Queue(0)

    async def i1():
        await q.put("123")

    async def i2():
        i = await q.get()

    start = time.time()
    print("Start time", start)
    async with trio.open_nursery() as n:
        for x in range(0, 5_000):
            n.start_soon(i1)
            n.start_soon(i2)
            if x % 500 == 0:
                await trio.sleep(0)

    end = time.time()
    print("End time", end)
    print("Elapsed", end - start)


trio.run(main)

Before:

Start time 1535036274.015487
End time 1535036274.5630355
Elapsed 0.5475485324859619

After:

Start time 1535036229.3229249
End time 1535036229.8288205
Elapsed 0.5058956146240234

I generally got a ~40-50ms speedup with this change.

@codecov
Copy link

codecov bot commented Aug 23, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@ac4a028). Click here to learn what that means.
The diff coverage is 90%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #628   +/-   ##
=========================================
  Coverage          ?   99.28%           
=========================================
  Files             ?       91           
  Lines             ?    10763           
  Branches          ?      768           
=========================================
  Hits              ?    10686           
  Misses            ?       58           
  Partials          ?       19
Impacted Files Coverage Δ
trio/_core/_run.py 99.68% <90%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ac4a028...ff7bbc8. Read the comment docs.

@njsmith
Copy link
Member

njsmith commented Aug 25, 2018

eh, in the long run I think we should try to approach these things systematically based on benchmarks and bottlenecks people experience in real life (#604), but this particular change is so trivial and harmless that sure, why not.

@Fuyukai
Copy link
Member Author

Fuyukai commented Aug 25, 2018

I actually discovered this by a profile - a suspicious amount of time was being spent inside instrument() so I added these guards and they seemed to reduce it by a bit.

@njsmith
Copy link
Member

njsmith commented Aug 25, 2018

Oh interesting, what were you profiling?

@njsmith njsmith merged commit b926f5d into python-trio:master Aug 25, 2018
@Fuyukai Fuyukai deleted the instrument-speedup branch August 25, 2018 23:19
@Fuyukai
Copy link
Member Author

Fuyukai commented Aug 25, 2018

Just my Discord bot - I just wanted to see if there was any low hanging fruit in the main trio loop for a real program, saw the instrument calls, and wrote the change and a more pure benchmark to show it off.

@njsmith
Copy link
Member

njsmith commented Aug 26, 2018

Neat. BTW, if it's possible to convert your bot code into a benchmark (like literally: take the existing code and wrap a harness around it), that could be a good way to start at #604 ... the more quirky real-world stuff is in a benchmark, the better it is for guiding these kinds of decisions :-).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants