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 for pynamodb #968

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Asyncio for pynamodb #968

wants to merge 12 commits into from

Conversation

tasn
Copy link

@tasn tasn commented Aug 10, 2021

Adding asyncio support to pynamodb.
This makes #853 redundant.

I haven't properly tested everything, but my code seems to work. Notes:

  1. For whatever reason my code's performance hasn't improved (requests per second). I wonder if there are any sync/locky things lurking anywhere. I suspect it may just be the fact I'm testing against a local dynamodb rather than a "real" one, so I'm stuck at its capacity.
  2. The model and index functions only have async variants at the moment, I haven't given them the same _async suffix I gave others. This is easy to fix.
  3. I made iterators async only (should be easy to make them also support the classic iterator interface).

Would love to get your thoughts on why things may not be as performant as I thought they would/should be.

CC @svix-yair, @ikonst, @garrettheel

@garrettheel
Copy link
Member

Thanks for picking this up!

I think the only way this can be mergeable is to separate out the async functionality as an optional extra. This would require some refactoring of connection-related parts to separate the current botocore sync implementation from the new aiobotocore async one.

Would love to get your thoughts on why things may not be as performant as I thought they would/should be.

I wouldn't expect asyncio to be inherently faster than another form of cooperative multitasking. That said, can you share the code you're using to benchmark? There are many variables such as the python version, event loop implementation, etc. to account for. I might guess that aiobotocore's implementation is heavier than the urllib3-based one in botocore too, so it wouldn't surprise me if a naive version of this performed worse than sync.

@tasn
Copy link
Author

tasn commented Aug 10, 2021

Update: I tried with a remote dynamodb and it doesn't look like it's a matter of the the local dynamodb being overloaded.
I also tried updating the max_pool_connections to something that's larger than 10, thinking that may be it, but it's not the case.

@tasn
Copy link
Author

tasn commented Aug 10, 2021

I don't have a good benchmark unfortunately, I'm just trying to hammer my FastAPI server with requests. One version is using this async version, and another is using a sync implementation (and normal pynamo).

I'd have expected the number of concurrent requests it can handle to increase dramatically due to the networking code happening in parallel. Though I'm getting roughly the same results.

@tasn
Copy link
Author

tasn commented Aug 10, 2021

OK, one last update: it looks like it's CPU bound indeed. Most of the time is spent between _convert_to_request_dict and _create_prepared_request (namely _sign_request).

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.

None yet

2 participants