-
Notifications
You must be signed in to change notification settings - Fork 22
Use ThreadLocal caching for ExtendedRandom PRNG contexts #1255
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,7 +16,13 @@ public final class ExtendedRandom { | |
|
|
||
| private OpenJCEPlusProvider provider; | ||
| private NativeInterface nativeInterface; | ||
| final long ockPRNGContextId; | ||
| private final String algName; | ||
|
|
||
| long ockPRNGContextId; | ||
| private boolean usingSharedContext = true; | ||
|
|
||
| private static final ThreadLocal<PRNGContextPointer> prngContextBufferSha256 = new ThreadLocal<PRNGContextPointer>(); | ||
| private static final ThreadLocal<PRNGContextPointer> prngContextBufferSha512 = new ThreadLocal<PRNGContextPointer>(); | ||
|
|
||
| public static ExtendedRandom getInstance(String algName, OpenJCEPlusProvider provider) | ||
| throws OCKException { | ||
|
|
@@ -32,11 +38,35 @@ public static ExtendedRandom getInstance(String algName, OpenJCEPlusProvider pro | |
| } | ||
|
|
||
| private ExtendedRandom(String algName, OpenJCEPlusProvider provider) throws OCKException { | ||
| this.algName = algName; | ||
| this.provider = provider; | ||
| this.nativeInterface = provider.isFIPS() ? NativeOCKAdapterFIPS.getInstance() : NativeOCKAdapterNonFIPS.getInstance(); | ||
| this.ockPRNGContextId = this.nativeInterface.EXTRAND_create(algName); | ||
| this.ockPRNGContextId = getPRNGContext(algName); | ||
| } | ||
|
|
||
| private long getPRNGContext(String algName) throws OCKException { | ||
| PRNGContextPointer prngCtx = null; | ||
| ThreadLocal<PRNGContextPointer> prngCtxBuffer = null; | ||
|
|
||
| switch (algName) { | ||
| case "SHA256": | ||
| prngCtxBuffer = prngContextBufferSha256; | ||
| break; | ||
| case "SHA512": | ||
| prngCtxBuffer = prngContextBufferSha512; | ||
| break; | ||
| default: | ||
| throw new IllegalArgumentException( | ||
| "Unsupported HASHDRBG algorithm: " + algName); | ||
| } | ||
|
|
||
| this.provider.registerCleanable(this, cleanOCKResources(ockPRNGContextId, nativeInterface)); | ||
| prngCtx = prngCtxBuffer.get(); | ||
| if (prngCtx == null) { | ||
| prngCtx = new PRNGContextPointer(algName, this.nativeInterface, this.provider); | ||
| prngCtxBuffer.set(prngCtx); | ||
| } | ||
|
|
||
| return prngCtx.getCtx(); | ||
| } | ||
|
|
||
| public synchronized void nextBytes(byte[] bytes) throws OCKException { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can understand that since the test is crashed so we have to add the However, i feel there is a contradiction between using the
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For the synchronized issue we discussed last Friday, the failure does not occur in the newly added test I moved the payload initialization into the benchmark method, as shown below, but it still crashes. I think it is not only the shared payload, but also the shared With
So either we remove |
||
|
|
@@ -55,22 +85,63 @@ public synchronized void setSeed(byte[] seed) throws OCKException { | |
| } | ||
|
|
||
| if (seed.length > 0) { | ||
| createNonSharedContextForReSeed(); | ||
| this.nativeInterface.EXTRAND_setSeed(ockPRNGContextId, seed); | ||
| } | ||
| } | ||
|
|
||
| private Runnable cleanOCKResources(long ockPRNGContextId, NativeInterface nativeInterface) { | ||
| private void createNonSharedContextForReSeed() throws OCKException { | ||
| if (!usingSharedContext) { | ||
| return; | ||
| } | ||
|
|
||
| long privateCtx = this.nativeInterface.EXTRAND_create(algName); | ||
| this.ockPRNGContextId = privateCtx; | ||
| this.usingSharedContext = false; | ||
|
|
||
| this.provider.registerCleanable(this, cleanNonSharedOCKResources(privateCtx, nativeInterface)); | ||
| } | ||
|
|
||
| private static Runnable cleanNonSharedOCKResources(long ockPRNGContextId, NativeInterface nativeInterface) { | ||
| return () -> { | ||
| try { | ||
| if (ockPRNGContextId != 0) { | ||
| nativeInterface.EXTRAND_delete(ockPRNGContextId); | ||
| } | ||
| } catch (Exception e) { | ||
| if (OpenJCEPlusProvider.getDebug() != null) { | ||
| OpenJCEPlusProvider.getDebug().println("An error occurred while cleaning : " + e.getMessage()); | ||
| OpenJCEPlusProvider.getDebug().println("An error occurred while cleaning: " + e.getMessage()); | ||
| e.printStackTrace(); | ||
| } | ||
| } | ||
| }; | ||
| } | ||
|
|
||
| private static final class PRNGContextPointer { | ||
| final long prngCtx; | ||
|
|
||
| PRNGContextPointer(String algName, NativeInterface nativeInterface, OpenJCEPlusProvider provider) throws OCKException { | ||
| this.prngCtx = nativeInterface.EXTRAND_create(algName); | ||
| provider.registerCleanable(this, cleanOCKResources(this.prngCtx, nativeInterface)); | ||
| } | ||
|
|
||
| long getCtx() { | ||
| return this.prngCtx; | ||
| } | ||
|
|
||
| private static Runnable cleanOCKResources(long ockPRNGContextId, NativeInterface nativeInterface) { | ||
| return () -> { | ||
| try { | ||
| if (ockPRNGContextId != 0) { | ||
| nativeInterface.EXTRAND_delete(ockPRNGContextId); | ||
| } | ||
| } catch (Exception e) { | ||
| if (OpenJCEPlusProvider.getDebug() != null) { | ||
| OpenJCEPlusProvider.getDebug().println("An error occurred while cleaning : " + e.getMessage()); | ||
| e.printStackTrace(); | ||
| } | ||
| } | ||
| }; | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,67 @@ | ||
| /* | ||
| * Copyright IBM Corp. 2026, 2026 | ||
| * | ||
| * This code is free software; you can redistribute it and/or modify it | ||
| * under the terms provided by IBM in the LICENSE file that accompanied | ||
| * this code, including the "Classpath" Exception described therein. | ||
| */ | ||
|
|
||
| package ibm.jceplus.jmh; | ||
|
|
||
| import java.security.SecureRandom; | ||
| import java.util.concurrent.TimeUnit; | ||
| import org.openjdk.jmh.annotations.Benchmark; | ||
| import org.openjdk.jmh.annotations.BenchmarkMode; | ||
| import org.openjdk.jmh.annotations.Measurement; | ||
| import org.openjdk.jmh.annotations.Mode; | ||
| import org.openjdk.jmh.annotations.OutputTimeUnit; | ||
| import org.openjdk.jmh.annotations.Param; | ||
| import org.openjdk.jmh.annotations.Scope; | ||
| import org.openjdk.jmh.annotations.Setup; | ||
| import org.openjdk.jmh.annotations.State; | ||
| import org.openjdk.jmh.annotations.Warmup; | ||
| import org.openjdk.jmh.runner.Runner; | ||
| import org.openjdk.jmh.runner.RunnerException; | ||
| import org.openjdk.jmh.runner.options.Options; | ||
|
|
||
| @BenchmarkMode(Mode.Throughput) | ||
| @OutputTimeUnit(TimeUnit.SECONDS) | ||
| @State(Scope.Benchmark) | ||
| @Warmup(iterations = 3, time = 10, timeUnit = TimeUnit.SECONDS) | ||
| @Measurement(iterations = 4, time = 30, timeUnit = TimeUnit.SECONDS) | ||
| public class RandomNewInstanceBenchmark extends JMHBase { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since this is a new test it would need to be added to the JenkinsfilePerformance file such that is available for users to select.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, added into JenkinsfilePerformance. |
||
|
|
||
| @Param({"16", "2048", "32768"}) | ||
| private int payloadSize; | ||
|
|
||
| private byte[] payload; | ||
|
|
||
| private String algorithm; | ||
| private String provider; | ||
|
|
||
| @Param({"SHA256DRBG|OpenJCEPlus", "SHA512DRBG|OpenJCEPlus", "SHA1PRNG|SUN", "DRBG|SUN"}) | ||
| private String randomToTest; | ||
|
|
||
| @Setup | ||
| public void setup() throws Exception { | ||
| String[] algAndProvider = randomToTest.split("\\|"); | ||
| algorithm = algAndProvider[0]; | ||
| provider = algAndProvider[1]; | ||
| super.setup(provider); | ||
| payload = new byte[payloadSize]; | ||
| } | ||
|
|
||
| @Benchmark | ||
| public byte[] runSecureRandom() throws Exception { | ||
| SecureRandom random = SecureRandom.getInstance(algorithm, provider); | ||
| random.nextBytes(payload); | ||
| return payload; | ||
| } | ||
|
|
||
| public static void main(String[] args) throws RunnerException { | ||
| String testSimpleName = RandomNewInstanceBenchmark.class.getSimpleName(); | ||
| Options opt = optionsBuild(testSimpleName, testSimpleName); | ||
|
|
||
| new Runner(opt).run(); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we still have outstanding questions here if we can share contexts in the same thread. In the scenario we are worried about we would have two instances of
SecureRandomand the state data associated with one instance will be influenced by the state data in the context by the other instance ofSecureRandom. Im not sure if this is a problem or not in OCKC while generating randoms. It might be fine to share state between two streams of random like this but not sure.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From this code change,
ThreadLocalprevents sharing across threads, but yes, it still allows sharing within the same thread between differentSecureRandominstances that use the same algorithm. And, we keepsynchronizedonnextBytes(byte[] bytes)andsetSeed(byte[] seed), so access to the native context is serialized.From
NativeInterface_EXTRAND_create, the native context is created based on the algorithm and then returned. I do not see any instance-specific state passed in during creation, so from this code, it looks possible that the same context could be reused within the same thread.The
GSKituser guide states thatRNG_CTXs are not intrinsically thread-safe, but it does not specify whether reusing the same context sequentially within a single thread across multiple instances is supported/allowed or not.From the benchmark tests, we have tests: reusing one
SecureRandominstance repeatedly, and creating a new instance each time before callingnextBytes(). I ran the JMH tests with both 1 thread and 16 threads, and I did not see any failures or issues.So for the question of whether contexts can be shared within the same thread by multiple instances, I don't think we have documentation that clearly proves it. Just based on the tests, I did not see any issue.
And one additional is, in the existing benchmark, we already reuse the same context repeatedly through a single
SecureRandominstance on the same thread. That is not exactly the same scenario as here, where multipleSecureRandominstances with the same algorithm may sharing one context on the same thread. However, since I don't see any instance-specific state in context creation, and the benchmark tests did not show any issue, I don't see that same-thread multiple instances sharing is causing a problem.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably need to test this on all the platforms. Threading is different on the different platforms and there maybe a probably on some that do not happen on others.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So far, since Fyre VM does not have many platform options, I have run it on both x and z Linux and did not find any threading issues. Is there any particular platform you would like me to check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AIX and Windows
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I ran the functional tests on these platforms and they look good. Right now, I’m running the JMH performance tests on these platforms and will share the results once the runs are complete.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the late update. I ran all the benchmark tests in the
SecurityPerformancePipelineon bothppc64_aixandx86-64_windows, and the results look good. I did not see any issues.