On Wed, 24 Jan 2024 15:25:28 GMT, Oli Gillespie <ogilles...@openjdk.org> wrote:
> Avoid expensive `Class.forName` call when constructing Providers such as > `SecureRandom` which take constructor parameters. This can easily be cached > in EngineDescription (this cache already existed before, it was removed in > [JDK-8280970](https://bugs.openjdk.org/browse/JDK-8280970) as unused, I'm > bringing it back unchanged to support this new usage). > > Benchmark results on my Linux x86 host show around a 20% reduction in time to > create a new `SecureRandom` instance. Most of the remaining overhead is due > to a failing constructor lookup - see > [JDK-8324648](https://bugs.openjdk.org/browse/JDK-8324648). > > > Before > newSecureRandom avgt 2930 ± 50 ns/op > > After > newSecureRandom avgt 2400 ± 33 ns/op > > > I have seen multiple real-world applications which call `new SecureRandom()` > on the hot path, so I believe efficiency here is important. This looks promising to me! But we need to polish this a bit: src/java.base/share/classes/java/security/Provider.java line 1560: > 1558: final boolean supportsParameter; > 1559: final String constructorParameterClassName; > 1560: private volatile Class<?> constructorParameterClass; Style: no need for `private` here, match what other fields are doing. src/java.base/share/classes/java/security/Provider.java line 1568: > 1566: } > 1567: > 1568: Class<?> getConstructorParameterClass() throws > ClassNotFoundException { I think we want to do what `Service.getImplClass` is doing, for extra safety: retain cached class on a `WeakReference`. This would avoid accidental class leakage in this code. src/java.base/share/classes/java/security/Provider.java line 1909: > 1907: } else { > 1908: ctrParamClz = cap.constructorParameterClassName == > null? > 1909: null : cap.getConstructorParameterClass(); Maybe we should move the ternary for `cap.constructorParameterClassName == null` into new method to begin with. test/micro/org/openjdk/bench/java/security/SecureRandomBench.java line 1: > 1: package org.openjdk.bench.java.security; No copyright header? test/micro/org/openjdk/bench/java/security/SecureRandomBench.java line 16: > 14: > 15: @Benchmark > 16: public SecureRandom newSecureRandom() throws Exception { Just "create()", I think. No need to repeat it with `SecureRandomBench.newSecureRandom`. ------------- PR Review: https://git.openjdk.org/jdk/pull/17559#pullrequestreview-1842025562 PR Review Comment: https://git.openjdk.org/jdk/pull/17559#discussion_r1465331538 PR Review Comment: https://git.openjdk.org/jdk/pull/17559#discussion_r1465330794 PR Review Comment: https://git.openjdk.org/jdk/pull/17559#discussion_r1465332784 PR Review Comment: https://git.openjdk.org/jdk/pull/17559#discussion_r1465332949 PR Review Comment: https://git.openjdk.org/jdk/pull/17559#discussion_r1465334379
