-
Notifications
You must be signed in to change notification settings - Fork 140
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
Delegated the implementations of Lock and Semaphore to the async backend class #761
Conversation
This is what I used to benchmark import asyncio
import time
import trio
import anyio
async def acquire_release(lock):
async with lock:
await anyio.sleep(0)
async def main(lock_class: type):
lock = lock_class()
async with anyio.create_task_group() as tg:
for _ in range(10000):
tg.start_soon(acquire_release, lock)
def benchmark(lock_class: type, name: str, backend: str) -> None:
start = time.monotonic()
anyio.run(main, lock_class, backend=backend)
print(f"{name}: {time.monotonic() - start}")
benchmark(anyio.Lock, "anyio+asyncio", backend="asyncio")
benchmark(asyncio.Lock, "asyncio", backend="asyncio")
benchmark(anyio.Lock, "anyio+trio", backend="trio")
benchmark(trio.Lock, "trio", backend="trio") |
This is very nice! Could you also benchmark |
I was going to tackle |
As expected, there's a significant performance gap between |
After my revised implementation of
|
I run the above benchmark with following async def main(lock_class: type):
lock = lock_class()
for _ in range(10000):
await acquire_release(lock) Then unfortunately AnyIO still has quite a great overhead with asyncio (as we found also here encode/httpcore#922):
Caused by this line https://github.com/agronholm/anyio/pull/761/files#diff-202c3fc4551e5a287df488a9bd96a25dcc1405ec135d5c7aceb6ef4c02755341R1694 Explanation on why above is required here encode/httpcore#922 (comment) |
I can see why this would be a problem...but removing that call violates the Trio design principles where each coroutine function in the API should always contain a checkpoint. I'm not sure I can do much about this before v5.0. |
Perhaps it could be made opt-out via a parameter? That wouldn't break backwards compat, and would be in line with Trio's principles, in the sense that by default it does the right thing. |
Yeah that sounds pretty reasonable to me! |
EDIT: I must be tired; disregard the above. I did have a
Are you absolutely sure that you ran the benchmark against this PR? On what Python version? Because I'm seeing very different results. Here's an updated benchmark that adds a warm-up round: import asyncio
import time
import trio
import anyio
async def acquire_release(lock):
async with lock:
await anyio.sleep(0)
async def main(lock_class: type):
lock = lock_class()
async with anyio.create_task_group() as tg:
for _ in range(10000):
tg.start_soon(acquire_release, lock)
def benchmark(lock_class: type, name: str, backend: str, warmup: bool) -> None:
start = time.monotonic()
anyio.run(main, lock_class, backend=backend)
if not warmup:
print(f"{name}: {time.monotonic() - start}")
for warmup in (True, False):
benchmark(anyio.Lock, "anyio+asyncio", backend="asyncio", warmup=warmup)
benchmark(asyncio.Lock, "asyncio", backend="asyncio", warmup=warmup)
benchmark(anyio.Lock, "anyio+trio", backend="trio", warmup=warmup)
benchmark(trio.Lock, "trio", backend="trio", warmup=warmup) I'm seeing
Would you mind re-running the benchmark at your end? |
Oh, by the way, maybe you'd like to join the AnyIO room on Gitter to discuss this in (almost) real time? |
Sure, here are the rerun results with the commit 787a454 High contention:
Low contention:
I used the same benchmark as above here (that has the warmup) for the high contention case. The low contention case I again used this in the place of "main": async def main(lock_class: type):
lock = lock_class()
for _ in range(10000):
await acquire_release(lock) So in the high-contention case the results are similar as before but in the low-contention case the In |
For me its about similar. But the low-contention case is much worse. Did you try the low-contention where the lock is not contested? |
That I did not. I will try it your way and see if I can make more sense of the results. |
I added the low contention case as part of full benchmark if its useful: import asyncio
import time
from typing import Awaitable, Callable
import trio
import anyio
async def acquire_release(lock):
async with lock:
await anyio.sleep(0)
async def high_contention(lock_class: type):
lock = lock_class()
async with anyio.create_task_group() as tg:
for _ in range(10000):
tg.start_soon(acquire_release, lock)
async def no_contention(lock_class: type):
lock = lock_class()
for _ in range(10000):
await acquire_release(lock)
def benchmark(main: Callable[[type], Awaitable[None]], lock_class: type, name: str, backend: str, warmup: bool) -> None:
start = time.monotonic()
anyio.run(main, lock_class, backend=backend)
if not warmup:
print(f"{name}: {time.monotonic() - start}")
for warmup in (True, False):
for lock_contention in (True, False):
if not warmup:
case_msg = "high contention" if lock_contention else "no contention"
print(case_msg)
case = high_contention if lock_contention else no_contention
benchmark(case, anyio.Lock, "anyio+asyncio", backend="asyncio", warmup=warmup)
benchmark(case, asyncio.Lock, "asyncio", backend="asyncio", warmup=warmup)
benchmark(case, anyio.Lock, "anyio+trio", backend="trio", warmup=warmup)
benchmark(case, trio.Lock, "trio", backend="trio", warmup=warmup) |
Yeah, so the |
Yep, so it seems. The difference to asyncio is then pretty negligible when removing |
Alright, then I would say that I need to again look at adding the |
Alright, please test against the |
If you find that this new option works as expected, I will polish the branch and merge it to this one. |
It seems to work! I run the benchmark here and this is what I got:
(Unable to run Trio benchmark, it fails with Also run httpcore benchmarks and it also looks very good: First results with all optimization PRs applied (to not count other httpcore issues in). Lastly results without all optimization PRs applied (how it would look against httpcore master, with its issues). |
I've fixed the |
I run with Trio included with
|
Also posted the above results to the related httpcore PR and asked Tom Christie whether to keep AnyIO there for synchronization. Maintainer(s) had some other ideas about removing the hard dependency to that, but not sure what was the idea with that. Hence the plain asyncio network backend PR is also there waiting. But unfortunately httpcore author has disappeared somewhere :/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation here looks good to me, but I'm somewhat concerned about the design: as you note in encode/httpcore#922 (comment) fast_acquire=True
can turn code which is otherwise correct (i.e. is cancellable and subject to timeouts as expected) into an infinite loop via spooky action at a distance. I've had nasty production bugs of this form, which is why https://github.com/python-trio/flake8-async/ has lint rules to warn against such functions.
Performance improvements absolutely are important, but maybe we can support this on asyncio but make fast_acquire=True
a no-op on Trio where cancellation scopes actually do exist?
I'm not sure what the existence of cancellation scopes has to do with this. The point here was to skip the extra event loop cycle in the no-contention scenario. As for the dangers, they're documented and this is strictly opt-in. |
It's locally opt-in, but has non-local effects - e.g. if I'm using some library that uses this, I can no longer rely on the Maybe that's just a lost cause with third-party libraries anyway, but it does make me sad to give up that property. |
I'd say it's always been a lost cause. Until we have some way of enforcing this (and I'm not seeing this in the foreseeable future), it will remain that way. |
Aside from that, is there anything else amiss here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's fair enough. I can't see any other issues, so let's ship it!
Changes
This improves the performance of
anyio.Lock
on asyncio by ~50% by delegating the implementation of the class to the async back-end.Checklist
If this is a user-facing code change, like a bugfix or a new feature, please ensure that
you've fulfilled the following conditions (where applicable):
tests/
) added which would fail without your patchdocs/
, in case of behavior changes or newfeatures)
docs/versionhistory.rst
).If this is a trivial change, like a typo fix or a code reformatting, then you can ignore
these instructions.
Updating the changelog
If there are no entries after the last release, use
**UNRELEASED**
as the version.If, say, your patch fixes issue #123, the entry should look like this:
* Fix big bad boo-boo in task groups (#123 <https://github.com/agronholm/anyio/issues/123>_; PR by @yourgithubaccount)
If there's no issue linked, just link to your pull request instead by updating the
changelog after you've created the PR.