RFR: 8304956: Update KeyStore.getDefaultType() specification to return pkcs12 as fallback
Replaced "jks" with "pkcs12" in both the spec and fallback for `KeyStore.getDefaultType()` - Commit messages: - removed jks as fallback keystore and replaced with pkcs12 Changes: https://git.openjdk.org/jdk/pull/15625/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=15625&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8304956 Stats: 4 lines in 1 file changed: 0 ins; 1 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/15625.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15625/head:pull/15625 PR: https://git.openjdk.org/jdk/pull/15625
Re: RFR: 8304956: Update KeyStore.getDefaultType() specification to return pkcs12 as fallback
On Thu, 7 Sep 2023 18:12:28 GMT, Ben Perez wrote: > Replaced "jks" with "pkcs12" in both the spec and fallback for > `KeyStore.getDefaultType()` Marked as reviewed by findoboutm...@github.com (no known OpenJDK username). - PR Review: https://git.openjdk.org/jdk/pull/15625#pullrequestreview-1616027096
Re: RFR: 8304956: Update KeyStore.getDefaultType() specification to return pkcs12 as fallback
On Thu, 7 Sep 2023 18:12:28 GMT, Ben Perez wrote: > Replaced "jks" with "pkcs12" in both the spec and fallback for > `KeyStore.getDefaultType()` looks fine. Could you add some test code (perhaps to an existing test) for this ? It should be trivial to test. I think you'll need a CSR also - PR Comment: https://git.openjdk.org/jdk/pull/15625#issuecomment-1710650911
Re: RFR: 8304956: Update KeyStore.getDefaultType() specification to return pkcs12 as fallback
On Thu, 7 Sep 2023 18:12:28 GMT, Ben Perez wrote: > Replaced "jks" with "pkcs12" in both the spec and fallback for > `KeyStore.getDefaultType()` > looks fine. Could you add some test code (perhaps to an existing test) for > this ? It should be trivial to test. > > I think you'll need a CSR also > > /csr Yes, I agree a CSR is needed since the specification behavior is being changed. - PR Comment: https://git.openjdk.org/jdk/pull/15625#issuecomment-1710657080
Re: RFR: 8304956: Update KeyStore.getDefaultType() specification to return pkcs12 as fallback
On Thu, 7 Sep 2023 18:12:28 GMT, Ben Perez wrote: > Replaced "jks" with "pkcs12" in both the spec and fallback for > `KeyStore.getDefaultType()` looks like this comment in the same file could be updated also: (jks -> pkcs12) /* * Constant to lookup in the Security properties file to determine * the default keystore type. * In the Security properties file, the default keystore type is given as: * * keystore.type=jks * */ - PR Comment: https://git.openjdk.org/jdk/pull/15625#issuecomment-1710658508
Re: RFR: 8308995: Update Network IO JFR events to be static mirror events [v5]
> The socket read/write JFR events currently use instrumentation of java.base > code using templates in the jdk.jfr modules. This results in some java.base > code residing in the jdk.jfr module which is undesirable. > > JDK19 added static support for event classes. The old instrumentor classes > should be replaced with mirror events using the static support. > > In the java.base module: > Added two new events, jdk.internal.event.SocketReadEvent and > jdk.internal.event.SocketWriteEvent. > java.net.Socket and sun.nio.ch.SocketChannelImpl were changed to make use of > the new events. > > In the jdk.jfr module: > jdk.jfr.events.SocketReadEvent and jdk.jfr.events.SocketWriteEvent were > changed to be mirror events. > In the package jdk.jfr.internal.instrument, the classes > SocketChannelImplInstrumentor, SocketInputStreamInstrumentor, and > SocketOutputStreamInstrumentor were removed. The JDKEvents class was updated > to reflect all of those changes. > > The existing tests in test/jdk/jdk/jfr/event/io continue to pass with the new > implementation: > Passed: jdk/jfr/event/io/TestSocketChannelEvents.java > Passed: jdk/jfr/event/io/TestSocketEvents.java > > I added a micro benchmark which measures the overhead of handling the jfr > socket events. > test/micro/org/openjdk/bench/java/net/SocketEventOverhead.java. > It needs access the jdk.internal.event package, which is done at runtime with > annotations that add the extra arguments. > At compile time the build arguments had to be augmented in > make/test/BuildMicrobenchmark.gmk Tim Prinzing has updated the pull request incrementally with one additional commit since the last revision: More changes from review: I didn't like the name of the helper method 'checkForCommit' because it doesn't indicate that it might commit the event. I also don't like 'commitEvent' because it might not. Since JFR events are sort of like a queue I went with a name from collections and called it 'offer' so using it is something like 'SocketReadEvent.offer(...)' which seems like it gets the idea across better. Also improved the javadoc for it. Removed the comments about being instrumented by JFR in Socket.SocketInputStream and Socket.SocketOutputStream. I went ahead and moved the event commiting out of the finally block so that we don't emit events when the read/write did not actually happen. The bugid JDK-8310979 will be used to determine if more should be done in this area. The implRead and implWrite were moved up with the other support methods for read/write. - Changes: - all: https://git.openjdk.org/jdk/pull/14342/files - new: https://git.openjdk.org/jdk/pull/14342/files/d6f7df72..9fa27459 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14342&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14342&range=03-04 Stats: 151 lines in 5 files changed: 57 ins; 81 del; 13 mod Patch: https://git.openjdk.org/jdk/pull/14342.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14342/head:pull/14342 PR: https://git.openjdk.org/jdk/pull/14342
Re: RFR: 8308995: Update Network IO JFR events to be static mirror events [v3]
On Tue, 22 Aug 2023 07:18:21 GMT, Alan Bateman wrote:
>> src/java.base/share/classes/java/net/Socket.java line 1133:
>>
>>> 1131: return parent.getSoTimeout();
>>> 1132: } catch (Throwable t) {
>>> 1133: // ignored - avoiding exceptions in jfr event data
>>> gathering
>>
>> This should be SocketException, not Throwable. That said, I think it would
>> be useful to know why the SocketReadEvent includes the timeout. Is this used
>> to see If read durations are close to the timeout? I assume once this code
>> is fixed to deal with the exceptional case that the need to include the
>> timeout for the success case will mostly go away.
>
> Were you able to find out what the timeout is used for?
No. I think it's a relic from the distant past though. I think the timeout
field should be removed. It's not used on SocketChannel at all, and it doesn't
seem useful on Socket.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14342#discussion_r1319152153
Re: RFR: JDK-8314901: AES-GCM interleaved implementation using AVX2 instructions
On Mon, 4 Sep 2023 15:00:23 GMT, Ferenc Rakoczi wrote:
>> Hi All,
>> I would like to submit AES-GCM optimization for x86_64 architectures using
>> AVX2 instructions. This optimization interleaves AES and GHASH operations.
>>
>> Below are the performance numbers on my desktop system with -XX:UseAVX=2
>> option:
>>
>> |Benchmark | Data Size | Base version (ops/s) | Patched version (ops/s) |
>> Speedup
>> |-||---|--|---|
>> |full.AESGCMBench.decrypt | 8192 | 526274.678 | 670014.543 | 1.27
>> full.AESGCMBench.encrypt | 8192 | 538293.315 | 680716.207 | 1.26
>> small.AESGCMBench.decrypt | 8192 | 527854.353 |663131.48 | 1.25
>> small.AESGCMBench.encrypt | 8192 | 548193.804 | 683624.232 |1.24
>> full.AESGCMBench.decryptMultiPart | 8192 | 299865.766 | 299815.851 | 0.99
>> full.AESGCMBench.encryptMultiPart | 8192 | 534406.564 |539235.462 | 1.00
>> small.AESGCMBench.decryptMultiPart | 8192 | 299960.202 |298913.629 | 0.99
>> small.AESGCMBench.encryptMultiPart | 8192 | 542669.258 | 540552.293 | 0.99
>> | | | |
>> full.AESGCMBench.decrypt | 16384 | 307266.364 |390397.778 | 1.27
>> full.AESGCMBench.encrypt | 16384 | 311491.901 | 397279.681 | 1.27
>> small.AESGCMBench.decrypt | 16384 | 306257.801 | 389531.665 |1.27
>> small.AESGCMBench.encrypt | 16384 | 311468.972 | 397804.753 | 1.27
>> full.AESGCMBench.decryptMultiPart | 16384 | 159634.341 | 181271.487 | 1.13
>> full.AESGCMBench.encryptMultiPart | 16384 | 308980.992 | 385606.113 | 1.24
>> small.AESGCMBench.decryptMultiPart | 16384 | 160476.064 |181019.205 | 1.12
>> small.AESGCMBench.encryptMultiPart | 16384 | 308382.656 | 391126.417 | 1.26
>> | | | |
>> full.AESGCMBench.decrypt | 32768 | 162284.703 | 213257.481 |1.31
>> full.AESGCMBench.encrypt | 32768 | 164833.104 | 215568.639 | 1.30
>> small.AESGCMBench.decrypt | 32768 | 164416.491 | 213422.347 | 1.29
>> small.AESGCMBench.encrypt | 32768 | 166619.205 | 214584.208 |1.28
>> full.AESGCMBench.decryptMultiPart | 32768 | 83306.239 | 93762.988 |1.12
>> full.AESGCMBench.encryptMultiPart | 32768 | 166109.391 |211701.969 | 1.27
>> small.AESGCMBench.decryptMultiPart | 32768 | 83792.559 | 94530.786 | 1.12
>> small.AESGCMBench.encryptMultiPart | 32768 | 162975.904 |212085.047 | 1.30
>> | | | |
>> full.AESGCMBench.decrypt | 65536 | 85765.835 | 112244.611 | 1.30
>> full.AESGCMBench.encrypt | 65536 | 86471.805 | 113320.536 |1.31
>> small.AESGCMBench.decrypt | 65536 | 84490.816 | 112122.358 |1.32
>> small.AESGCMBench.encrypt | 65536 | 85403.025 | 112741.811 | 1.32
>> full.AES...
>
> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java
> line 590:
>
>> 588: private static int implGCMCrypt(byte[] in, int inOfs, int inLen,
>> byte[] ct,
>> 589: int ctOfs, byte[] out, int outOfs,
>> 590: GCTR gctr, GHASH ghash, boolean
>> encryption) {
>
> It looks to me that you don't need to introduce this "boolean encryption"
> here as it is simply (ct == out), which can easily be calculated in the
> intrinsics and that saves a lot of code change.
@ferakocz Thank you for your comment. I will make the change.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15410#discussion_r1319203520
Re: RFR: JDK-8314901: AES-GCM interleaved implementation using AVX2 instructions
On Thu, 7 Sep 2023 23:23:13 GMT, Smita Kamath wrote:
>> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java
>> line 590:
>>
>>> 588: private static int implGCMCrypt(byte[] in, int inOfs, int inLen,
>>> byte[] ct,
>>> 589: int ctOfs, byte[] out, int outOfs,
>>> 590: GCTR gctr, GHASH ghash, boolean
>>> encryption) {
>>
>> It looks to me that you don't need to introduce this "boolean encryption"
>> here as it is simply (ct == out), which can easily be calculated in the
>> intrinsics and that saves a lot of code change.
>
> @ferakocz Thank you for your comment. I will make the change.
@ascarpino Apologies for the delay in responding, I was away on vacation. There
are fewer number of registers available in the AVX2 algorithm as compared to
AVX512. That's why its essential for the intrinsic to know if it is encryption
or decryption this time around. I will be implementing Ferenc's suggestion and
remove the boolean variable.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15410#discussion_r1319206174
