Performance improvement for CryptoPrimitives #207
Conversation
| } | ||
|
|
||
| Class<?> aClass = Class.forName(securityProviderClassName); | ||
| if (null == aClass) { |
There was a problem hiding this comment.
This could never happen. aClass could not be null.
| throw new InstantiationException(format("Class for security provider %s is not a Java security provider", aClass.getName())); | ||
| } | ||
| Provider securityProvider = (Provider) aClass.newInstance(); | ||
| if (securityProvider == null) { |
There was a problem hiding this comment.
This could never happen. A new instance cannot be null.
| CryptoException rete = null; | ||
|
|
||
| List<Provider> providerList = new LinkedList<>(Arrays.asList(Security.getProviders())); | ||
| if (SECURITY_PROVIDER != null) { //Add if overridden |
There was a problem hiding this comment.
Clarified that adding a security provider may not do much. If you add it you probably want it preferred which means adding it to the start of the list, not the end.
| if (SECURITY_PROVIDER != null) { | ||
| // Add if overridden. Note it is added to the end of the provider list so is only invoked if all other providers fail. | ||
| providerList.add(SECURITY_PROVIDER); | ||
| } |
There was a problem hiding this comment.
Do not create new instance of the BC provider
| continue; | ||
| continue; | ||
| } | ||
| CertificateFactory certFactory = CertificateFactory.getInstance(CERTIFICATE_FORMAT, provider); |
There was a problem hiding this comment.
The factory will never be null.
| return ret; | ||
| } | ||
|
|
||
| private X509Certificate getX509Certificate(byte[] pemCertificate) throws CryptoException { |
There was a problem hiding this comment.
This method now uses cached results.
| * @return | ||
| */ | ||
|
|
||
| private X509Certificate getX509Certificate(byte[] pemCertificate) throws CryptoException { |
There was a problem hiding this comment.
This method is now invoked by the cache loader rather than on every request.
| } | ||
| } | ||
|
|
||
| try { |
There was a problem hiding this comment.
Logic simplified as we can now easily check if the certificate is valid
Signed-off-by: Simon Greatrix <simon.greatrix@setl.io>
Signed-off-by: Simon Greatrix <simon.greatrix@setl.io>
bestbeforetoday
left a comment
bestbeforetoday
left a comment
There was a problem hiding this comment.
Unless there is a compelling reason to implement a custom hash algorithm -- please just say if there is -- I think using the standard library implementation is preferable. Otherwise looks good to me.
src/main/java/org/hyperledger/fabric/sdk/security/CryptoPrimitives.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
|
|
||
| private final LoadingCache<CertKey, CertValue> x509Cache = CacheBuilder.newBuilder().maximumSize(1000).build(new CacheLoader<CertKey, CertValue>() { |
There was a problem hiding this comment.
I wonder if an expireAfterAccess setting would also be beneficial to ensure the cache does't collect stale or very rarely used values over long execution times. What do you think?
There was a problem hiding this comment.
The expireAfterAccess setting doesn't do what one imagines, unless you also have a timer thread periodically invoking the cache's cleanUp method. Entries that are past their expiry time are kept in memory until some process touches them. There is no automatic clean up of Guava caches.
Using expireAfterWrite could have been used to simplify the implementation at the cost of re-parsing and re-checking the certificate periodically where currently we just need to do the re-check to spot certificates reaching their end of life. Less code, but also less performance, and I've written the code now :)
Is there a mechanism in the SDK for specifying low level configuration? A configurable cache size instead of a fixed one would be a nice-to-have in some circumstances.
For the time being, I would suggest just leaving this as it is.
There was a problem hiding this comment.
There are a few values obtained from config at the top of CryptoPrimitives. You could use the same pattern quite easily, just adding another property with suitable default to org.hyperledger.fabric.sdk.helper.Config.
…ives.java Co-authored-by: Mark S. Lewis <Mark.S.Lewis@outlook.com> Signed-off-by: Simon Greatrix <simon@pippsford.com>
|
@simon-greatrix Do you want to make any more changes to expose config for cache size and/or certificate re-check time, or do you want to get this PR in as-is and consider configuration in another PR? |
|
I would like to get this PR in as-is. In my environment the changes improved a two second block chain query to a 20 millisecond query, so I strongly believe this will have widespread benefit. The configuration is a nice-to-have but not crucial. |
|
GitHub is flagging that the PR is out of date with the base branch. Could you rebase on main and re-push to make it ready to merge? |
|
If you get a chance to try the new Fabric Gateway client API with Fabric v2.4+, I would be very interested in any feedback on comparative performance in your environment. The handling of certificates and keys there are intended to be lighter weight, but I haven’t done any comparative benchmarking. |
|
Thank you for the contribution! |
|
@Mergifyio backport release-2.2 |
✅ Backports have been created
|
Signed-off-by: Simon Greatrix <simon.greatrix@setl.io> Co-authored-by: Simon Greatrix <simon@pippsford.com> (cherry picked from commit 560aa5b)
2 participants
Every request required a creation of a new BouncyCastleProvider which is a very expensive operation.
Furthermore, certificates were re-parsed and their chains re-validated on every reques. Again this is a very expensive operation.
Taken together these factors caused excessive delays in interacting with the Fabric block chain.