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

Improve Performance for Async Pagination #1414

Merged
merged 5 commits into from
Mar 22, 2025

Conversation

tim-hub
Copy link
Contributor

@tim-hub tim-hub commented Feb 25, 2025

django ninja is a good framework, when I work on the project, I found the pagination on async view is much lower than sync view for performance wise, like row 1 and row 3, so I have made some update for the pagination class.

Type Name # Requests # Fails Average (ms) Min (ms) Max (ms) Average size (bytes) RPS Failures/s
async with origin pagination /api/bff/trade 108 12 11294.07 5764 23045 38551.11 0.86 0.1
async with new pagination change /api/bff/trade/async 108 0 75.9 25 475 43370 0.86 0
sync pagination /api/bff/trade/sync 108 0 193.67 24 2175 43370 0.86 0

In another word, if use the origin pagination on async view, it is actually does not bring any befenits for performance but slow it down. This PR is to resolve this.

@vitalik
Copy link
Owner

vitalik commented Mar 9, 2025

aget_list_or_404 seems aded only in 5.x django

maybe it just worth adding some await

[obj async for obj in queryset[offset: offset + self.page_size]]

throwing 404 if empty here also does not make sense actually

@vitalik
Copy link
Owner

vitalik commented Mar 9, 2025

hi @tim-hub
could you run your test with async for and latest version from master ?

@tim-hub
Copy link
Contributor Author

tim-hub commented Mar 14, 2025

aget_list_or_404 seems aded only in 5.x django

maybe it just worth adding some await

[obj async for obj in queryset[offset: offset + self.page_size]]

throwing 404 if empty here also does not make sense actually

This makes sense, I have updated it.

@tim-hub
Copy link
Contributor Author

tim-hub commented Mar 14, 2025

hi @tim-hub could you run your test with async for and latest version from master ?

I have run it again,

image

It basically kept the same pattern, that new async pagination( and sync pagination) performance better than origin async pagination without the changes.

not surprisingly [obj async for] and aget_list_or_404 do not performance much different, because as we nowaget is just a wrapper + 404. anyway, I agree 404 does not make sense here.

@tim-hub
Copy link
Contributor Author

tim-hub commented Mar 15, 2025

latest commit to fix the failing test

@vitalik
Copy link
Owner

vitalik commented Mar 15, 2025

@tim-hub

i think if-statement should be other way around

if isinstance(queryset, list):

to

if isinstance(queryset, QuerySet)
   do await
else: # can be list, tuple, string etc - that does not have alist
   ...

@tim-hub
Copy link
Contributor Author

tim-hub commented Mar 15, 2025

@tim-hub

i think if-statement should be other way around

if isinstance(queryset, list):

to

if isinstance(queryset, QuerySet)
   do await
else: # can be list, tuple, string etc - that does not have alist
   ...

I see, make sense

@vitalik vitalik merged commit 35873f1 into vitalik:master Mar 22, 2025
36 of 37 checks passed
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