Check for HAM/config provider more efficiently #18399
Conversation
|
#build |
|
Your personal build request is at https://wasrtc.hursley.ibm.com:9443/jazz/resource/itemOid/com.ibm.team.build.BuildResult/_MGTioAsrEeygdL3FgLlrXw Target locations of links might be accessible only to IBM employees. |
|
Your Open Liberty build results are ready for viewing.
|
|
The build jdmcclur-18399-20210901-1453 For help analyzing your personal build, go to https://cognitive.hursley.ibm.com/buildAnalysis.html?uuid=_MGTioAsrEeygdL3FgLlrXw |
|
A lot of FATS failed, so may need to rethink this - haven't had time to investigate yet. |
|
#build |
|
Your personal build request is at https://wasrtc.hursley.ibm.com:9443/jazz/resource/itemOid/com.ibm.team.build.BuildResult/_pD3mcCa8EeyWeMDU9jagxQ Target locations of links might be accessible only to IBM employees. |
|
#libby |
|
The build jdmcclur-18399-20211006-0953 |
There was a problem hiding this comment.
Yeah the code will work this way, but I would suggest getting rid of the returns. Instead of return on the first one, just do an else to the rest of it where it is doing the synchronized. Similarly I would suggest that for the since return statement that you swap the if condition and put the remaining logic in the == null condition instead. The code will be much cleaner.
Can you also do a double check synchronize to avoid the synchronization if it is already set?
AuthConfigProvider authConfigProvider = providerFactory.getConfigProvider(JASPIC_LAYER_HTTP_SERVLET, appContext, (RegistrationListener) null);
if (authConfigProvider == null) {
// Synchronized since checking if there is a provider and registering one need to be done as a single atomic operation.
synchronized (this) {
authConfigProvider = providerFactory.getConfigProvider(JASPIC_LAYER_HTTP_SERVLET, appContext, (RegistrationListener) null);
if (authConfigProvider == null) {
| HttpAuthenticationMechanism ham = getHttpAuthenticationMechanism(false); | ||
| if (ham != null) { | ||
| return true; | ||
| if (isHam == null) { |
There was a problem hiding this comment.
I don't think this will work. The call for getting the http authentication mechanism is on a ThreadLocal so it can be different per thread. Also this class is a singleton. A new instance would not be created when there is a config change that could change the http authentication mechanism.
There was a problem hiding this comment.
OK, bummer. If we can't speed up this check, not sure the other change is really worth it. It would probably still help my scenario, but would slow down a scenario where there is an authConfigProvider I think.
|
@jhanders34 @vijaysun-omr @utle I looked at this some more. It looks like the change helps my case (no jaspic/authconfigprovider) by ~1.5%. However, I think it would regress a scenario where there is an authconfigprovider configured by ~0.5%. So, the question is, do many customers use jaspic? Is this a big deal to regress it? Thoughts? |
|
We are not aware of that many customers using the jaspic provider. Can you give a little detail why we regress for jaspic customers? |
|
@utle - Yes, because of the way I switched things around to avoid the sync lock on the method, it now always does this check first Before, it didn't do this check if an authconfigprovider is already in the cache. This is a pretty minor regression though. Maybe 0.5%. |
I am ok with the minor regression since we do not have that many customer using JASPI provider. |
|
#libby |
|
#build |
Code analysis and actionsDO NOT DELETE THIS COMMENT.
|
|
Your personal build request is at https://wasrtc.hursley.ibm.com:9443/jazz/resource/itemOid/com.ibm.team.build.BuildResult/_UfyIIFawEeyWaZwSyDrk3A Target locations of links might be accessible only to IBM employees. |
|
The build jdmcclur-18399-20211206-1015 |
When enabling mp-4.0 there is a small regression compared to mp-3.3 due to bringing in appsecurity-3.0 and needing to check for jaspic. This is an attempt to address that in BuildBridgeIfNeeded.