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: Update getCredential to only refresh credential once per request #453

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

ComputerTinker
Copy link

Changes

The code currently performs full credential refreshes several times per request, which has a negative performance impact, especially when complex queries are involved. This change updates the SDK to refresh the credential once per request, and serve up a cached version of the credential for subsequent calls to getCredential within the same request thereafter.

Please see associated issue for more detailed information.

References

#452

Contributor Checklist

Signed-off-by: Russ <ComputerTinker@users.noreply.github.com>
@evansims
Copy link
Member

evansims commented Apr 9, 2024

Hey @ComputerTinker, thanks again for working on a solution here. This appears to fail in a number of scenarios we check for with our unit tests. Safe to disregard the linter errors for now, those are easily addressed later.

@ComputerTinker
Copy link
Author

@evansims, yeah I saw the failed PEST tests and started working on a solution. It really seems to come down to two scenarios, from what I can tell:

  1. I forgot to clear the didCredRefresh flag in the logout function and a few other places. Duh, but it's an easy fix, and once I made that change locally it resolved two of the four issues
  2. The other two failed tests involve modifying the session data outside of the API, and when that happens the cached credential data is no longer accurate. I started thinking about a way to do an MD5-like thumbprint of the key session data and only go through the full credential refresh if we detect that's changed, but I'm struggling with some of the context of how the SDK works. Hoping you'll be able to bring your experience to bear here. 🙏

@ComputerTinker
Copy link
Author

ComputerTinker commented Apr 10, 2024

@evansims just updated the PR with code that passes all tests and should give you an idea of what I was talking about with thumbprinting the credential. Here are the performance tests from a few heavy-lifting screens in our app:

Laravel v9 without Auth0 (baseline):

  • Screen 1 (28k recs): 51s, 50s, 50s
  • Screen 2 (12k recs): 17s, 18s, 17s
  • Screen 3 (29k recs): 1:17, 1:17
  • Screen 4 (62k recs): 23s, 23s

Laravel v9 with Auth0:

  • Screen 1 (28k recs): 2:22, 2:25, 2:16
  • Screen 2 (12k recs): 40s, 39s, 39s
  • Screen 3 (29k recs): 4:00 Timeout Error
  • Screen 4 (62k recs): 1:45, 1:48

Laravel v10/v11 with Auth0:

  • Screen 1 (28k recs): 4:00 Timeout Error
  • Screen 2 (12k recs): 2:39, 2:39
  • Screen 3 (29k recs): 4:00 Timeout Error
  • Screen 4 (62k recs): 4:00 Timeout Error

Laravel v11 with Auth0 and this PR Fix:

  • Screen 1 (28k recs): 1:20, 1:20
  • Screen 2 (12k recs): 31s, 32s
  • Screen 3 (29k recs): 4:00 Timeout Error
  • Screen 4 (62k recs): 1:10, 1:10

Inside of getCredential, the findSession logic is still decently slow all by itself. Perhaps we can find a more performant way to test whether the credential has changed to optimize this further.

image

@ComputerTinker
Copy link
Author

@evansims I just updated the PR with a commit which allows our app to function at near-baseline speeds again, while still passing all PEST and lint tests. I think that's as much as I can noodle out on my end. Anxious to know what you think. 🤞

Laravel v11 with Auth0 and updated PR Fix:

  • Screen 1 (28k recs): 54s
  • Screen 2 (12k recs): 18s
  • Screen 3 (29k recs): 1:26
  • Screen 4 (62k recs): 25s

@evansims
Copy link
Member

Awesome work on this @ComputerTinker, I think you've come up with a solid solution here! Let me run it through its paces on my end (through some example app suites I have set up, just to double-check there are no unintended side effects I can identify,) but I think this looks really great.

(Thanks also for that great benchmarking; it's tremendously helpful!)

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