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-security performance issue due to missing bean scope #8780

Merged
merged 1 commit into from
Apr 24, 2020

Conversation

loicmathieu
Copy link
Contributor

The DefaultRoleDecoder has no CDI scope so it is insantiated for each request.
As it reads a propety at initialization time, this is very costly.

Adding @ApplicationScoped annotation to it lower the cost of security in one of my profile from 5.7% of frames to 1.3%

All other elytron security related beans are @ApplicationScoped and reading the code this should be safe.

@loicmathieu
Copy link
Contributor Author

/cc @gsmet @sberyozkin @stuartwdouglas

@gsmet
Copy link
Member

gsmet commented Apr 23, 2020

@loicmathieu could you share a profile after your change? 1.3% is still a lot IMHO.

@loicmathieu
Copy link
Contributor Author

@gsmet 1.3% on 20ms response time is 260µs which is pretty low ;)
Profile show that all security related cost is inside elytron SecurityDomain class so out of Quarkus.
I'll send the new profile to the mailing list

@loicmathieu loicmathieu force-pushed the elytron-security-perf branch from da2e704 to e6882f4 Compare April 23, 2020 10:44
@gsmet
Copy link
Member

gsmet commented Apr 23, 2020

We have a test failure in the security native tests. Not sure if it's related.

@loicmathieu loicmathieu force-pushed the elytron-security-perf branch from e6882f4 to 0e054f4 Compare April 24, 2020 07:19
@loicmathieu
Copy link
Contributor Author

@gsmet I just launch the failing test locally on my branch and it passes.

I rebased on master to trigger an other CI.

Anyway, I would like @stuartwdouglas review on this one.

@gsmet gsmet added this to the 1.5.0 milestone Apr 24, 2020
@loicmathieu
Copy link
Contributor Author

@gsmet CI failed again in another test (on Java 8 only).
The previous failing native test passed!

@gsmet gsmet merged commit 771ce5e into quarkusio:master Apr 24, 2020
@gsmet gsmet modified the milestones: 1.5.0, 1.4.1.Final Apr 24, 2020
@loicmathieu loicmathieu deleted the elytron-security-perf branch April 24, 2020 09:39
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.

3 participants