Re: RFR: 8291897: TerminatingThreadLocal(s) not registered from virtual thread(s)
On Thu, 4 Aug 2022 15:54:29 GMT, Alan Bateman wrote: >> This is an attempt to fix inconsistent behavior of TerminatingThreadLocal(s) >> when used internally in JDK for per-carrier-thread caches of native >> ByteBuffer(s) and NativeBuffer(s). If used from virtual thread, such >> TerminatingThreadLocal(s) are not registered with the carrier thread for the >> cleanup. >> The fix introduces an internal CarrierThreadLocal subclass of ThreadLocal >> which rewires its API methods to use current thread's carrier thread as the >> source of ThreadLocalMap instead of current thread itself (if it is a >> VirtualThread for example). TerminatingThreadLocal is now a subclass of >> CarrierThreadLocal. It seems only per-carrier-thread caching is a usecase >> for it. The uses of JavaLangAccess in various places to access a >> carrier-thread value of given ThreadLocal has been replaced by public API >> calls and the use of CarrierThreadLocal instead of plain ThreadLocal. >> JavaLangAccess is still used to dispatch the CarrierThreadLocal API methods, >> but only for that. Would someone be tempted to use JavaLangAccess methods >> directly, they now require CarrierThreadLocal argument to guard against >> missuses. >> The REGISTRY of TerminatingThreadLocal(s) that tracks which of them have >> values bound to a particular carrier thread is now implemented conveniently >> with a CarrierThreadLocal. >> The test is expanded with a case that demonstrates a situation where a >> carrier thread is terminated. Since it must wait for 30 seconds for that to >> happen, only one of the test cases is performed in this mode. The correct >> logic of TerminatingThreadLocal is still verified with all test-cases using >> platform threads that can be terminated more rapidly. > > src/java.base/share/classes/sun/nio/ch/Util.java line 233: > >> 231: } >> 232: >> 233: BufferCache cache = bufferCache.get(); > > I assume JLA is no longer needed. Right, I forgot to remove this one. - PR: https://git.openjdk.org/jdk/pull/9743
Re: RFR: 8291897: TerminatingThreadLocal(s) not registered from virtual thread(s) [v2]
> This is an attempt to fix inconsistent behavior of TerminatingThreadLocal(s) > when used internally in JDK for per-carrier-thread caches of native > ByteBuffer(s) and NativeBuffer(s). If used from virtual thread, such > TerminatingThreadLocal(s) are not registered with the carrier thread for the > cleanup. > The fix introduces an internal CarrierThreadLocal subclass of ThreadLocal > which rewires its API methods to use current thread's carrier thread as the > source of ThreadLocalMap instead of current thread itself (if it is a > VirtualThread for example). TerminatingThreadLocal is now a subclass of > CarrierThreadLocal. It seems only per-carrier-thread caching is a usecase for > it. The uses of JavaLangAccess in various places to access a carrier-thread > value of given ThreadLocal has been replaced by public API calls and the use > of CarrierThreadLocal instead of plain ThreadLocal. JavaLangAccess is still > used to dispatch the CarrierThreadLocal API methods, but only for that. Would > someone be tempted to use JavaLangAccess methods directly, they now require > CarrierThreadLocal argument to guard against missuses. > The REGISTRY of TerminatingThreadLocal(s) that tracks which of them have > values bound to a particular carrier thread is now implemented conveniently > with a CarrierThreadLocal. > The test is expanded with a case that demonstrates a situation where a > carrier thread is terminated. Since it must wait for 30 seconds for that to > happen, only one of the test cases is performed in this mode. The correct > logic of TerminatingThreadLocal is still verified with all test-cases using > platform threads that can be terminated more rapidly. Peter Levart has updated the pull request incrementally with one additional commit since the last revision: Removed unused JLA, SharedSecrets, added @enablePreview test annotation - Changes: - all: https://git.openjdk.org/jdk/pull/9743/files - new: https://git.openjdk.org/jdk/pull/9743/files/fa2078e4..2339b87f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=9743&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=9743&range=00-01 Stats: 10 lines in 2 files changed: 3 ins; 3 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/9743.diff Fetch: git fetch https://git.openjdk.org/jdk pull/9743/head:pull/9743 PR: https://git.openjdk.org/jdk/pull/9743
Re: RFR: 8291897: TerminatingThreadLocal(s) not registered from virtual thread(s) [v2]
On Thu, 4 Aug 2022 15:55:38 GMT, Alan Bateman wrote:
>> Peter Levart has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Removed unused JLA, SharedSecrets, added @enablePreview test annotation
>
> test/jdk/jdk/internal/misc/TerminatingThreadLocal/TestTerminatingThreadLocal.java
> line 40:
>
>> 38: * @compile --enable-preview -source ${jdk.version}
>> TestTerminatingThreadLocal.java
>> 39: * @run main/othervm --enable-preview
>> -Djdk.virtualThreadScheduler.parallelism=1
>> -Djdk.virtualThreadScheduler.maxPoolSize=2 TestTerminatingThreadLocal
>> 40: */
>
> If you add `@enablePreview` then it will allow you to drop the `@compile`
> line and drop `--enable-preview from the `@run` tag.
>
> You might want to add the bugId to the `@bug` list too.
Done in next commit.
-
PR: https://git.openjdk.org/jdk/pull/9743
Re: RFR: JDK-8290975 Minor cleanup could be done in javax.security [v2]
On Thu, 4 Aug 2022 20:21:44 GMT, Mark Powers wrote: >> First constructor doesn't set defaultName (or inputName), so there will be a >> error "might not have been initialized". > > I verified the error message happens when `defaultName` is final. Yes but `defaultName` (like `prompt`) should never change once the object is constructed. So it seems inconsistent not to also mark it final (and if necessary, set it to a default value (`null`) in the constructors). - PR: https://git.openjdk.org/jdk/pull/9664
Re: RFR: JDK-8290975 Minor cleanup could be done in javax.security [v2]
On Thu, 4 Aug 2022 17:03:37 GMT, Bradford Wetmore wrote: >> src/java.base/share/classes/javax/security/auth/callback/TextInputCallback.java >> line 46: >> >>> 44: * @since 1.4 >>> 45: */ >>> 46: private final String prompt; >> >> I think you can also mark `defaultText` final. > > Same as above. Same comment as above about setting it to a default value in the constructors. - PR: https://git.openjdk.org/jdk/pull/9664
RFR: 8227651: Tests fail with SSLProtocolException: Input record too big
Fix `SSLEngineService` test class to make sure it does not discard any network data between `handshaking` and `receive`. With TLS1.3 the client starts sending application data immediately after sending the Finished message. The server may read some of that data in the `handshaking` method. This patch makes sure that any such data is delivered to the `receive` method for processing. - Commit messages: - Preserve excess network data Changes: https://git.openjdk.org/jdk/pull/9773/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=9773&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8227651 Stats: 51 lines in 8 files changed: 5 ins; 5 del; 41 mod Patch: https://git.openjdk.org/jdk/pull/9773.diff Fetch: git fetch https://git.openjdk.org/jdk pull/9773/head:pull/9773 PR: https://git.openjdk.org/jdk/pull/9773
Re: RFR: JDK-8290975 Minor cleanup could be done in javax.security [v2]
On Fri, 5 Aug 2022 10:04:43 GMT, Sean Mullan wrote: >> I verified the error message happens when `defaultName` is final. > > Yes but `defaultName` (like `prompt`) should never change once the object is > constructed. So it seems inconsistent not to also mark it final (and if > necessary, set it to a default value (`null`) in the constructors). Agreed. I'll make the change. - PR: https://git.openjdk.org/jdk/pull/9664
Re: RFR: 8288568: Reduce runtime of java.security microbenchmarks [v3]
> - Reduce forks, iteration, runtime to reduce runtime while maintaining high > data quality on typical benchmarking hosts. > > Reduces runtime from estimated 10+ hours to 54 minutes. Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Address comments on SSLHandshake - Changes: - all: https://git.openjdk.org/jdk/pull/9189/files - new: https://git.openjdk.org/jdk/pull/9189/files/8330c018..aa843dd7 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=9189&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=9189&range=01-02 Stats: 5 lines in 1 file changed: 0 ins; 3 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/9189.diff Fetch: git fetch https://git.openjdk.org/jdk pull/9189/head:pull/9189 PR: https://git.openjdk.org/jdk/pull/9189
Re: RFR: 8288568: Reduce runtime of java.security microbenchmarks [v2]
On Tue, 19 Jul 2022 20:55:54 GMT, David Schlosnagle wrote: >> Claes Redestad has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Copyrights, apply consistent settings to PermissionsImplies > > test/micro/org/openjdk/bench/java/security/SSLHandshake.java line 56: > >> 54: @Warmup(iterations = 5, time = 1) >> 55: @Measurement(iterations = 5, time = 1) >> 56: @Fork(value = 3) > > should the `@Warmup`, `@Measurement`, and `@Fork` be removed from the > `doHandshake()` benchmark on lines 115-117 below so that these take effect? > > https://github.com/openjdk/jdk/blob/c2cbeb3ee875936c98bb15ec32d692f7d866df76/test/micro/org/openjdk/bench/java/security/SSLHandshake.java#L111-L115 Good observation. This means this benchmark was already tuned, and the pre-existing tuning overrides the setting I thought I was testing with. I've effectively reverted the changes but moved the annotations to class scope for consistency. - PR: https://git.openjdk.org/jdk/pull/9189
Re: RFR: JDK-8290975 Minor cleanup could be done in javax.security [v2]
On Thu, 4 Aug 2022 20:20:35 GMT, Mark Powers wrote: >> src/java.base/share/classes/javax/security/auth/PrivateCredentialPermission.java >> line 130: >> >>> 128: * @serial >>> 129: */ >>> 130: private final boolean testing = false; >> >> This should really use java.security.debug so this tracing can be enabled at >> runtime, but that's probably more of a separate fix - feel free to file a >> separate issue. > > I'll file a bug later today and add to this bug report. You must mean sun.security.util.Debug. - PR: https://git.openjdk.org/jdk/pull/9664
Re: RFR: JDK-8290975 Minor cleanup could be done in javax.security [v2]
On Fri, 5 Aug 2022 14:29:38 GMT, Mark Powers wrote: >> I'll file a bug later today and add to this bug report. > > You must mean sun.security.util.Debug. JDK-8291974 - PR: https://git.openjdk.org/jdk/pull/9664
Re: RFR: JDK-8290975 Minor cleanup could be done in javax.security [v2]
On Fri, 5 Aug 2022 14:09:18 GMT, Mark Powers wrote: >> Yes but `defaultName` (like `prompt`) should never change once the object is >> constructed. So it seems inconsistent not to also mark it final (and if >> necessary, set it to a default value (`null`) in the constructors). > > Agreed. I'll make the change. That works also, as the API allows for these values to be set/reset to null. Can't do the same for inputName, as that can be set later. - PR: https://git.openjdk.org/jdk/pull/9664
Re: RFR: JDK-8290975 Minor cleanup could be done in javax.security [v3]
> https://bugs.openjdk.org/browse/JDK-8290975 Mark Powers has updated the pull request incrementally with one additional commit since the last revision: comment from sean - Changes: - all: https://git.openjdk.org/jdk/pull/9664/files - new: https://git.openjdk.org/jdk/pull/9664/files/70606fd9..2f54c822 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=9664&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=9664&range=01-02 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/9664.diff Fetch: git fetch https://git.openjdk.org/jdk pull/9664/head:pull/9664 PR: https://git.openjdk.org/jdk/pull/9664
Re: RFR: JDK-8290975 Minor cleanup could be done in javax.security [v4]
> https://bugs.openjdk.org/browse/JDK-8290975 Mark Powers has updated the pull request incrementally with one additional commit since the last revision: comment applies to two files - Changes: - all: https://git.openjdk.org/jdk/pull/9664/files - new: https://git.openjdk.org/jdk/pull/9664/files/2f54c822..21355258 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=9664&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=9664&range=02-03 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/9664.diff Fetch: git fetch https://git.openjdk.org/jdk pull/9664/head:pull/9664 PR: https://git.openjdk.org/jdk/pull/9664
Re: RFR: JDK-8290975 Minor cleanup could be done in javax.security [v4]
On Fri, 5 Aug 2022 21:49:01 GMT, Mark Powers wrote: >> https://bugs.openjdk.org/browse/JDK-8290975 > > Mark Powers has updated the pull request incrementally with one additional > commit since the last revision: > > comment applies to two files LGTM. - Marked as reviewed by wetmore (Reviewer). PR: https://git.openjdk.org/jdk/pull/9664
Re: RFR: JDK-8290975 Minor cleanup could be done in javax.security [v4]
On Fri, 5 Aug 2022 21:49:01 GMT, Mark Powers wrote: >> https://bugs.openjdk.org/browse/JDK-8290975 > > Mark Powers has updated the pull request incrementally with one additional > commit since the last revision: > > comment applies to two files Thanks Sean and Brad for the review! - PR: https://git.openjdk.org/jdk/pull/9664
