-
Notifications
You must be signed in to change notification settings - Fork 108
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
Use asyncio
synchronization instead of anyio
#922
base: master
Are you sure you want to change the base?
Use asyncio
synchronization instead of anyio
#922
Conversation
@MarkusSintonen Thanks so much for your work on this. Could we isolate the benchmarking into a separate PR? |
9064a6c
to
0239c8c
Compare
httpcore/_synchronization.py
Outdated
# await AsyncShieldCancellation.shield(cleanup) | ||
|
||
@staticmethod | ||
async def shield(shielded: Callable[[], Coroutine[Any, Any, None]]) -> None: |
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.
Need to convert into shielding of a coroutine as there is no shielding context manager variant in asyncio
Added here #923 |
f4968e1
to
6ef797e
Compare
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.
Great,
Also, is this mean we no more need anyio
dependency for httpx[asyncio]
installation on Python>=3.11
?
httpcore/_synchronization.py
Outdated
if (lock := self._lock) is None: | ||
lock = self.setup() | ||
await lock.acquire() |
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.
I prefer setup
method returns None at here.
if (lock := self._lock) is None: | |
lock = self.setup() | |
await lock.acquire() | |
if self._lock is None: | |
self.setup() | |
await self._lock.acquire() |
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.
Hmm yeah, I used that although all the mypy ignores this requires looks a bit uglier to me. So from types perspective the original was somewhat "better"
Yes thats right, once we also bring back the native asyncio based network to Actually I fixed the code now so that |
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.
LGTM Thank You @MarkusSintonen for this.
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.
Actually I fixed the code now so that anyio is not required either in Python<3.11
Then, you could also cleanup unused variable anyio
on this file.
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.
Its still needed there for the check in the file as anyio
needs to be present due to the networking backend. Later we can probably drop it
I'll wait you to create a PR from your commit to track and review it separately. |
if not closing: | ||
return |
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.
Could you explain this change to me?
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.
If its an empty list then we dont need to go through await async_cancel_shield
below, right? If that was the question
try: | ||
await asyncio.shield(inner_task) | ||
break | ||
except asyncio.CancelledError: |
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.
Why are we not just using a standard shield here?
inner_task = asyncio.create_task(shielded())
await asyncio.shield(inner_task)
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.
Its to wait for the inner_task
to finish (unless something is repeatedly trying to cancel this). But if we dont need to wait for the given task to finish then it is not needed and we can use just the simple shield (ie it runs in background to completion when there is a cancellation)
This PR highlights to me that the approach to shielding here is wrong.
And then...
This is a bit fiddly to get right, needs some careful attention to detail. |
@tomchristie do you mean the previous contextmanager based shielding also had issues? Requiring additional reordering of closing handlers? If it didnt have issues then we could still use the anyio CancelScope just for shielding? But it means the anyio dependency cant be fully removed. |
Or yeah did you mean the non-IO related state management should be extracted out from the async closing when shielding with asyncio.shield. That's probably right |
@MarkusSintonen I've attempted to answer my intent here through implementation. See #927. (Currently draft, and approach would need to be applied also to the HTTP/2 implementation, and the closing behaviour in the connection pool) |
Big thanks! Now I understand, so yeah it was indeed about "separating" the sync state management and the async IO part. Ie the streams aclose is the cancellable part and the state management being separate from async flow can not be cancelled. |
333c20c
to
eb72670
Compare
eb72670
to
96858c4
Compare
Would you mind testing my latest PR branch (agronholm/anyio#761)? In my own testing, it about halved the performance overhead of |
Nice, thank you! I can try sometime next week. |
@agronholm I run the tests and it is indeed better with the fix. But seems there is still some overhead with the implementation when comparing to asyncio: Anyio PR: (Above tests were run with httpcore optimizations from other pending PRs to not count in the issues from httpcore itself) |
Have you identified where this overhead is coming from? Can we eliminate synchronization as the cause after that PR? |
If you run the benchmarks after replacing the locks and semaphores in the |
It seems to be the |
In the AnyIO variant, the acquisition of an unowned lock would be slower because this operation contains a yield point unlike in the asyncio variant. Would you mind testing against AnyIO lock after commenting out the checkpoint calls around lines 1690-1697 in |
Indeed, it is happening because of the |
Yeah, so the problem here is that this yield point is needed to maintain the same semantics as with Trio. This is Trio's @enable_ki_protection
async def acquire(self) -> None:
"""Acquire the lock, blocking if necessary."""
await trio.lowlevel.checkpoint_if_cancelled()
try:
self.acquire_nowait()
except trio.WouldBlock:
# NOTE: it's important that the contended acquire path is just
# "_lot.park()", because that's how Condition.wait() acquires the
# lock as well.
await self._lot.park()
else:
await trio.lowlevel.cancel_shielded_checkpoint() If there wasn't a yield point, then this could become a busy-wait loop: async def loop(lock):
while True:
async with lock:
print("I'm holding the lock") |
So this is needed for the |
No, the |
To summarize, asyncio's incorrect design makes it faster here. |
Ok I see thanks for the explanation! So in essence |
Only in the |
Asyncio has so many design issues (the ill-fated uncancellation mechanism being that latest example) that at this point it's beyond saving. Even GvR (who's the original author of asyncio) told me to my face that he'd prefer people start using something else. |
What is the status of this PR? Should it be reviewed+merged, closed, or something else? |
Perhaps we should see how agronholm/anyio#761 affects the real-world performance before jumping the gun? |
Also posting here, I rerun the benchmarks with above fix from AnyIO using 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 other issues). |
Should we keep AnyIO but instead use the |
FYI, AnyIO 4.5.0 is out now with the |
Exercise caution. What does the pull request for that look like? That'll be a useful highlighter. |
Here is the PR https://github.com/encode/httpcore/pull/953/files |
I see a slight issue there, not among the changes but still: AnyIO 4.5.0 requires trio >= 0.26.1 in its |
Summary
First PR in series of optimizations to improve performance of
httpcore
and even reach performance levels ofaiohttp
.Related discussion encode/httpx#3215 (comment)
Previously:
With PR:
Request latency is more stable and the overall duration of the benchmark improves by 3.5x.
The difference diminishes when server latency increases over 100ms or so.
Checklist