-
Notifications
You must be signed in to change notification settings - Fork 136
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
perf: Update getCredential to only refresh credential once per request #453
Conversation
Signed-off-by: Russ <[email protected]>
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. |
@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:
|
@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):
Laravel v9 with Auth0:
Laravel v10/v11 with Auth0:
Laravel v11 with Auth0 and this PR Fix:
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. |
…ad of this->findSession
@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:
|
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!) |
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