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

perf: Backport of #3889 to v12 #3905

Open
wants to merge 1 commit into
base: v12
Choose a base branch
from

Conversation

mkleczek
Copy link
Contributor

@mkleczek mkleczek commented Feb 5, 2025

See #3889

@steve-chavez @taimoorzaeem
I've prepared a backport of asynchronous JWT cache purge to 12.
Looks like there is consistent (small but noticable) performance gain.

Done this as it is not clear when next major is going to be released (and if any JWT caching improvements are going to be included).

@wolfgangwalther
Copy link
Member

Looks like there is consistent (small but noticable) performance gain.

How did you test that?

@mkleczek
Copy link
Contributor Author

mkleczek commented Feb 8, 2025

Looks like there is consistent (small but noticable) performance gain.

How did you test that?

https://github.com/PostgREST/postgrest/actions/runs/13158254898

@wolfgangwalther
Copy link
Member

I see. The loadtest results have no meaning for this kind of PR:

  • First of all the main column obviously must be ignored.
  • The difference between v12.2.7 and HEAD is not different from the noise we usually have when running this on the same commit. Example: https://github.com/PostgREST/postgrest/actions/runs/13140690883 - the latest commit on main, which is a simple refactor. 403 -> 404 there, 465 -> 467 here. Not significant.
  • The loadtest runs all requests with the exact same JWT.
  • Since those JWT are hardcoded - they are surely set up so that they never expire.

Thus, the loadtest is testing none of what would be relevant for this PR.

@mkleczek
Copy link
Contributor Author

mkleczek commented Feb 8, 2025

It is difficult for me to judge the effectiveness of existing load tests.
The results are consistent with tests performed earlier on main:
https://github.com/PostgREST/postgrest/actions/runs/13151068303

So it seems that even in case of small JWT cache with fresh tokens taking purgeExpired from the hot path provides response time gains.

@steve-chavez
Copy link
Member

steve-chavez commented Feb 9, 2025

I don't think we should backport this. The MVar/locking logic looks risky, it could cause breakage.

Additionally a perf commit is not critical to backpatch. The gains don't look clear as well.

@mkleczek
Copy link
Contributor Author

mkleczek commented Feb 9, 2025

@steve-chavez
not questioning your decision but could you elaborate a little on the risks of MVar locking logic? I find it pretty straightforward.

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

Successfully merging this pull request may close these issues.

3 participants