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

Fix elytron properties perf issues #6358

Merged
merged 2 commits into from
Jan 2, 2020

Conversation

loicmathieu
Copy link
Contributor

During some performance testing, I discover, thanks to profiler CPU flamegraph, that the Elytron Properties implementation uses a lot of CPU cycles to create the WilldFlyElytronProvider.

It apears that the security provider supplier is called on each request calls, and triggers the creation of a new WilldFlyElytronProvider.

Creating a unique WilldFlyElytronProvider and returing it from the supplier instead of creating it each time fix the issue.

Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. I added a comment to further improve the situation.

@Override
public Provider[] get() {
return new Provider[] { new WildFlyElytronProvider() };
return providers;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you can make the whole supplier static and reuse it at both locations?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@gsmet
Copy link
Member

gsmet commented Dec 30, 2019

Also, can you check in the other security implementations that we don't have this pattern?

@loicmathieu
Copy link
Contributor Author

@gsmet I add the same performance optimization to the JDBC implementation.

OAuth2 and OIDC didn't suffers from the same issue.

@loicmathieu
Copy link
Contributor Author

@gsmet I also review the LDAP and JPA security extensions currently under review, LDAP has the same issue (I add a review comment), JPA doesn't seems to have it (not 100% sure as it's a complex extension and I just have a quick look).

@geoand geoand requested a review from gsmet January 2, 2020 08:13
@gsmet gsmet merged commit ef84e93 into quarkusio:master Jan 2, 2020
@gsmet
Copy link
Member

gsmet commented Jan 2, 2020

Merged, thanks!

@gsmet gsmet added this to the 1.1.1 milestone Jan 2, 2020
@loicmathieu loicmathieu deleted the fix/perf-issues branch January 30, 2020 18:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants