Re: RFR: 8259498: Reduce overhead of MD5 and SHA digests

[Claes Redestad]

On Wed, 6 Jan 2021 01:27:52 GMT, Claes Redestad  wrote:

>> Hitting up `new MD5()` directly could be a great idea. I expect this would 
>> be just as fast as the cache+clone (if not faster), but I'm a bit worried 
>> we'd be short-circuiting the ability to install an alternative MD5 provider 
>> (which may or may not be a thing we must support..), but it's worth 
>> exploring.
>> 
>> Comparing performance of this against a `ByteBuffer` impl is on my TODO. The 
>> `VarHandle` gets heavily inlined and optimized here, though, with 
>> performance in my tests similar to the `Unsafe` use in `ByteArrayAccess`.
>
> I've identified a number of optimizations to the plumbing behind 
> `MessageDigest.getDigest(..)` over in #1933 that removes 80-90% of the 
> throughput overhead and all the allocation overhead compared to the `clone()` 
> approach prototyped here. The remaining 20ns/op overhead might not be enough 
> of a concern to do a point fix in `UUID::nameUUIDFromBytes`.

Removing the UUID clone cache and running the microbenchmark along with the 
changes in #1933:

Benchmark  (size)   Mode  Cnt   
 ScoreError   Units
UUIDBench.fromType3Bytes2  thrpt   12   
 2.182 ±  0.090  ops/us
UUIDBench.fromType3Bytes:·gc.alloc.rate 2  thrpt   12  
439.020 ± 18.241  MB/sec
UUIDBench.fromType3Bytes:·gc.alloc.rate.norm2  thrpt   12  
264.022 ±  0.003B/op

The goal now is if to simplify the digest code and compare alternatives.

-

PR: https://git.openjdk.java.net/jdk/pull/1855

Re: RFR: 8259498: Reduce overhead of MD5 and SHA digests

[Claes Redestad]

On Thu, 7 Jan 2021 14:45:03 GMT, Claes Redestad  wrote:

>> I've identified a number of optimizations to the plumbing behind 
>> `MessageDigest.getDigest(..)` over in #1933 that removes 80-90% of the 
>> throughput overhead and all the allocation overhead compared to the 
>> `clone()` approach prototyped here. The remaining 20ns/op overhead might not 
>> be enough of a concern to do a point fix in `UUID::nameUUIDFromBytes`.
>
> Removing the UUID clone cache and running the microbenchmark along with the 
> changes in #1933:
> 
> Benchmark  (size)   Mode  Cnt 
>ScoreError   Units
> UUIDBench.fromType3Bytes2  thrpt   12 
>2.182 ±  0.090  ops/us
> UUIDBench.fromType3Bytes:·gc.alloc.rate 2  thrpt   12 
>  439.020 ± 18.241  MB/sec
> UUIDBench.fromType3Bytes:·gc.alloc.rate.norm2  thrpt   12 
>  264.022 ±  0.003B/op
> 
> The goal now is if to simplify the digest code and compare alternatives.

I've run various tests and concluded that the `VarHandle`ized code is matching 
or improving upon the `Unsafe`-riddled code in `ByteArrayAccess`. I then went 
ahead and consolidated to use similar code pattern in `ByteArrayAccess` for 
consistency, which amounts to a good cleanup.

With MD5 intrinsics disabled, I get this baseline:

Benchmark  (size)   Mode  Cnt   
 ScoreError   Units
UUIDBench.fromType3Bytes2  thrpt   12   
 1.245 ±  0.077  ops/us
UUIDBench.fromType3Bytes:·gc.alloc.rate.norm2  thrpt   12  
488.042 ±  0.004B/op

With the current patch here (not including #1933): 
Benchmark  (size)   Mode  Cnt   
 ScoreError   Units
UUIDBench.fromType3Bytes2  thrpt   12   
 1.431 ±  0.106  ops/us
UUIDBench.fromType3Bytes:·gc.alloc.rate.norm2  thrpt   12  
408.035 ±  0.006B/op

If I isolate the `ByteArrayAccess` changes I'm getting performance neutral or 
slightly better numbers compared to baseline for these tests:

Benchmark  (size)   Mode  Cnt   
 ScoreError   Units
UUIDBench.fromType3Bytes2  thrpt   12   
 1.317 ±  0.092  ops/us
UUIDBench.fromType3Bytes:·gc.alloc.rate.norm2  thrpt   12  
488.042 ±  0.004B/op

-

PR: https://git.openjdk.java.net/jdk/pull/1855

Re: RFR: 8259498: Reduce overhead of MD5 and SHA digests

[Claes Redestad]

On Tue, 5 Jan 2021 23:08:43 GMT, DellCliff 
 wrote:

>> Since `java.util.UUID` and `sun.security.provider.MD5` are both in 
>> `java.base`, would it make sense to create new instances by calling `new 
>> MD5()` instead of `java.security.MessageDigest.getInstance("MD5")` and 
>> bypassing the whole MessageDigest logic?
>
> Are you sure you're not ending up paying more using a VarHandle and having to 
> cast and using a var args call `(long) LONG_ARRAY_HANDLE.get(buf, ofs);` 
> instead of creating a ByteBuffer once via 
> `ByteBuffer.wrap(buffer).order(ByteOrder.nativeOrder()).asLongBuffer()`?

Hitting up `new MD5()` directly could be a great idea. I expect this would be 
just as fast as the cache+clone (if not faster), but I'm a bit worried we'd be 
short-circuiting the ability to install an alternative MD5 provider (which may 
or may not be a thing we must support..), but it's worth exploring.

Comparing performance of this against a `ByteBuffer` impl is on my TODO. The 
`VarHandle` gets heavily inlined and optimized here, though, with performance 
in my tests similar to the `Unsafe` use in `ByteArrayAccess`.

-

PR: https://git.openjdk.java.net/jdk/pull/1855

Re: RFR: 8259498: Reduce overhead of MD5 and SHA digests

[Claes Redestad]

On Wed, 6 Jan 2021 00:41:29 GMT, Claes Redestad  wrote:

>> Are you sure you're not ending up paying more using a VarHandle and having 
>> to cast and using a var args call `(long) LONG_ARRAY_HANDLE.get(buf, ofs);` 
>> instead of creating a ByteBuffer once via 
>> `ByteBuffer.wrap(buffer).order(ByteOrder.nativeOrder()).asLongBuffer()`?
>
> Hitting up `new MD5()` directly could be a great idea. I expect this would be 
> just as fast as the cache+clone (if not faster), but I'm a bit worried we'd 
> be short-circuiting the ability to install an alternative MD5 provider (which 
> may or may not be a thing we must support..), but it's worth exploring.
> 
> Comparing performance of this against a `ByteBuffer` impl is on my TODO. The 
> `VarHandle` gets heavily inlined and optimized here, though, with performance 
> in my tests similar to the `Unsafe` use in `ByteArrayAccess`.

I've identified a number of optimizations to the plumbing behind 
`MessageDigest.getDigest(..)` over in #1933 that removes 80-90% of the 
throughput overhead and all the allocation overhead compared to the `clone()` 
approach prototyped here. The remaining 20ns/op overhead might not be enough of 
a concern to do a point fix in `UUID::nameUUIDFromBytes`.

-

PR: https://git.openjdk.java.net/jdk/pull/1855

Re: RFR: 8259498: Reduce overhead of MD5 and SHA digests

[DellCliff]

On Tue, 5 Jan 2021 21:51:51 GMT, DellCliff 
 wrote:

>> - The MD5 intrinsics added by 
>> [JDK-8250902](https://bugs.openjdk.java.net/browse/JDK-8250902) shows that 
>> the `int[] x` isn't actually needed. This also applies to the SHA intrinsics 
>> from which the MD5 intrinsic takes inspiration
>> - Using VarHandles we can simplify the code in `ByteArrayAccess` enough to 
>> make it acceptable to use inline and replace the array in MD5 wholesale. 
>> This improves performance both in the presence and the absence of the 
>> intrinsic optimization.
>> - Doing the exact same thing in the SHA impls would be unwieldy (64+ element 
>> arrays), but allocating the array lazily gets most of the speed-up in the 
>> presence of an intrinsic while being neutral in its absence.
>> 
>> Baseline:
>>   (digesterName)  (length)Cnt Score  
>> Error   Units
>> MessageDigests.digestMD516 15  
>> 2714.307 ±   21.133  ops/ms
>> MessageDigests.digestMD5  1024 15   
>> 318.087 ±0.637  ops/ms
>> MessageDigests.digest  SHA-116 15  
>> 1387.266 ±   40.932  ops/ms
>> MessageDigests.digest  SHA-1  1024 15   
>> 109.273 ±0.149  ops/ms
>> MessageDigests.digestSHA-25616 15   
>> 995.566 ±   21.186  ops/ms
>> MessageDigests.digestSHA-256  1024 15
>> 89.104 ±0.079  ops/ms
>> MessageDigests.digestSHA-51216 15   
>> 803.030 ±   15.722  ops/ms
>> MessageDigests.digestSHA-512  1024 15   
>> 115.611 ±0.234  ops/ms
>> MessageDigests.getAndDigest  MD516 15  
>> 2190.367 ±   97.037  ops/ms
>> MessageDigests.getAndDigest  MD5  1024 15   
>> 302.903 ±1.809  ops/ms
>> MessageDigests.getAndDigestSHA-116 15  
>> 1262.656 ±   43.751  ops/ms
>> MessageDigests.getAndDigestSHA-1  1024 15   
>> 104.889 ±3.554  ops/ms
>> MessageDigests.getAndDigest  SHA-25616 15   
>> 914.541 ±   55.621  ops/ms
>> MessageDigests.getAndDigest  SHA-256  1024 15
>> 85.708 ±1.394  ops/ms
>> MessageDigests.getAndDigest  SHA-51216 15   
>> 737.719 ±   53.671  ops/ms
>> MessageDigests.getAndDigest  SHA-512  1024 15   
>> 112.307 ±1.950  ops/ms
>> 
>> GC:
>> MessageDigests.getAndDigest:·gc.alloc.rate.norm  MD516 15   
>> 312.011 ±0.005B/op
>> MessageDigests.getAndDigest:·gc.alloc.rate.normSHA-116 15   
>> 584.020 ±0.006B/op
>> MessageDigests.getAndDigest:·gc.alloc.rate.norm  SHA-25616 15   
>> 544.019 ±0.016B/op
>> MessageDigests.getAndDigest:·gc.alloc.rate.norm  SHA-51216 15  
>> 1056.037 ±0.003B/op
>> 
>> Target:
>> Benchmark (digesterName)  (length)Cnt
>>  Score  Error   Units
>> MessageDigests.digestMD516 15  
>> 3134.462 ±   43.685  ops/ms
>> MessageDigests.digestMD5  1024 15   
>> 323.667 ±0.633  ops/ms
>> MessageDigests.digest  SHA-116 15  
>> 1418.742 ±   38.223  ops/ms
>> MessageDigests.digest  SHA-1  1024 15   
>> 110.178 ±0.788  ops/ms
>> MessageDigests.digestSHA-25616 15  
>> 1037.949 ±   21.214  ops/ms
>> MessageDigests.digestSHA-256  1024 15
>> 89.671 ±0.228  ops/ms
>> MessageDigests.digestSHA-51216 15   
>> 812.028 ±   39.489  ops/ms
>> MessageDigests.digestSHA-512  1024 15   
>> 116.738 ±0.249  ops/ms
>> MessageDigests.getAndDigest  MD516 15  
>> 2314.379 ±  229.294  ops/ms
>> MessageDigests.getAndDigest  MD5  1024 15   
>> 307.835 ±5.730  ops/ms
>> MessageDigests.getAndDigestSHA-116 15  
>> 1326.887 ±   63.263  ops/ms
>> MessageDigests.getAndDigestSHA-1  1024 15   
>> 106.611 ±2.292  ops/ms
>> MessageDigests.getAndDigest  SHA-25616 15   
>> 961.589 ±   82.052  ops/ms
>> MessageDigests.getAndDigest  SHA-256  1024 15
>> 88.646 ±0.194  ops/ms
>> MessageDigests.getAndDigest  SHA-51216 15   
>> 775.417 ±   56.775  ops/ms
>> MessageDigests.getAndDigest  SHA-512  1024 15   
>> 112.904 ±2.014  ops/ms
>> 
>> GC
>

RFR: 8259498: Reduce overhead of MD5 and SHA digests

[Claes Redestad]

- The MD5 intrinsics added by 
[JDK-8250902](https://bugs.openjdk.java.net/browse/JDK-8250902) shows that the 
`int[] x` isn't actually needed. This also applies to the SHA intrinsics from 
which the MD5 intrinsic takes inspiration
- Using VarHandles we can simplify the code in `ByteArrayAccess` enough to make 
it acceptable to use inline and replace the array in MD5 wholesale. This 
improves performance both in the presence and the absence of the intrinsic 
optimization.
- Doing the exact same thing in the SHA impls would be unwieldy (64+ element 
arrays), but allocating the array lazily gets most of the speed-up in the 
presence of an intrinsic while being neutral in its absence.

Baseline:
  (digesterName)  (length)Cnt Score  
Error   Units
MessageDigests.digestMD516 15  
2714.307 ±   21.133  ops/ms
MessageDigests.digestMD5  1024 15   
318.087 ±0.637  ops/ms
MessageDigests.digest  SHA-116 15  
1387.266 ±   40.932  ops/ms
MessageDigests.digest  SHA-1  1024 15   
109.273 ±0.149  ops/ms
MessageDigests.digestSHA-25616 15   
995.566 ±   21.186  ops/ms
MessageDigests.digestSHA-256  1024 15
89.104 ±0.079  ops/ms
MessageDigests.digestSHA-51216 15   
803.030 ±   15.722  ops/ms
MessageDigests.digestSHA-512  1024 15   
115.611 ±0.234  ops/ms
MessageDigests.getAndDigest  MD516 15  
2190.367 ±   97.037  ops/ms
MessageDigests.getAndDigest  MD5  1024 15   
302.903 ±1.809  ops/ms
MessageDigests.getAndDigestSHA-116 15  
1262.656 ±   43.751  ops/ms
MessageDigests.getAndDigestSHA-1  1024 15   
104.889 ±3.554  ops/ms
MessageDigests.getAndDigest  SHA-25616 15   
914.541 ±   55.621  ops/ms
MessageDigests.getAndDigest  SHA-256  1024 15
85.708 ±1.394  ops/ms
MessageDigests.getAndDigest  SHA-51216 15   
737.719 ±   53.671  ops/ms
MessageDigests.getAndDigest  SHA-512  1024 15   
112.307 ±1.950  ops/ms

GC:
MessageDigests.getAndDigest:·gc.alloc.rate.norm  MD516 15   
312.011 ±0.005B/op
MessageDigests.getAndDigest:·gc.alloc.rate.normSHA-116 15   
584.020 ±0.006B/op
MessageDigests.getAndDigest:·gc.alloc.rate.norm  SHA-25616 15   
544.019 ±0.016B/op
MessageDigests.getAndDigest:·gc.alloc.rate.norm  SHA-51216 15  
1056.037 ±0.003B/op

Target:
Benchmark (digesterName)  (length)Cnt 
Score  Error   Units
MessageDigests.digestMD516 15  
3134.462 ±   43.685  ops/ms
MessageDigests.digestMD5  1024 15   
323.667 ±0.633  ops/ms
MessageDigests.digest  SHA-116 15  
1418.742 ±   38.223  ops/ms
MessageDigests.digest  SHA-1  1024 15   
110.178 ±0.788  ops/ms
MessageDigests.digestSHA-25616 15  
1037.949 ±   21.214  ops/ms
MessageDigests.digestSHA-256  1024 15
89.671 ±0.228  ops/ms
MessageDigests.digestSHA-51216 15   
812.028 ±   39.489  ops/ms
MessageDigests.digestSHA-512  1024 15   
116.738 ±0.249  ops/ms
MessageDigests.getAndDigest  MD516 15  
2314.379 ±  229.294  ops/ms
MessageDigests.getAndDigest  MD5  1024 15   
307.835 ±5.730  ops/ms
MessageDigests.getAndDigestSHA-116 15  
1326.887 ±   63.263  ops/ms
MessageDigests.getAndDigestSHA-1  1024 15   
106.611 ±2.292  ops/ms
MessageDigests.getAndDigest  SHA-25616 15   
961.589 ±   82.052  ops/ms
MessageDigests.getAndDigest  SHA-256  1024 15
88.646 ±0.194  ops/ms
MessageDigests.getAndDigest  SHA-51216 15   
775.417 ±   56.775  ops/ms
MessageDigests.getAndDigest  SHA-512  1024 15   
112.904 ±2.014  ops/ms

GC
MessageDigests.getAndDigest:·gc.alloc.rate.norm  MD516 15   
232.009 ±0.006B/op
MessageDigests.getAndDigest:·gc.alloc.rate.normSHA-116 15   
584.021 ±0.001B/op
MessageDigests.getAndDigest:·gc.alloc.rate.norm  SHA-25616 15   
272.012 ±0.015B/op
MessageDigests.getAndDigest

Re: RFR: 8259498: Reduce overhead of MD5 and SHA digests

[DellCliff]

On Sun, 20 Dec 2020 20:27:03 GMT, Claes Redestad  wrote:

> - The MD5 intrinsics added by 
> [JDK-8250902](https://bugs.openjdk.java.net/browse/JDK-8250902) shows that 
> the `int[] x` isn't actually needed. This also applies to the SHA intrinsics 
> from which the MD5 intrinsic takes inspiration
> - Using VarHandles we can simplify the code in `ByteArrayAccess` enough to 
> make it acceptable to use inline and replace the array in MD5 wholesale. This 
> improves performance both in the presence and the absence of the intrinsic 
> optimization.
> - Doing the exact same thing in the SHA impls would be unwieldy (64+ element 
> arrays), but allocating the array lazily gets most of the speed-up in the 
> presence of an intrinsic while being neutral in its absence.
> 
> Baseline:
>   (digesterName)  (length)Cnt Score  
> Error   Units
> MessageDigests.digestMD516 15  
> 2714.307 ±   21.133  ops/ms
> MessageDigests.digestMD5  1024 15   
> 318.087 ±0.637  ops/ms
> MessageDigests.digest  SHA-116 15  
> 1387.266 ±   40.932  ops/ms
> MessageDigests.digest  SHA-1  1024 15   
> 109.273 ±0.149  ops/ms
> MessageDigests.digestSHA-25616 15   
> 995.566 ±   21.186  ops/ms
> MessageDigests.digestSHA-256  1024 15
> 89.104 ±0.079  ops/ms
> MessageDigests.digestSHA-51216 15   
> 803.030 ±   15.722  ops/ms
> MessageDigests.digestSHA-512  1024 15   
> 115.611 ±0.234  ops/ms
> MessageDigests.getAndDigest  MD516 15  
> 2190.367 ±   97.037  ops/ms
> MessageDigests.getAndDigest  MD5  1024 15   
> 302.903 ±1.809  ops/ms
> MessageDigests.getAndDigestSHA-116 15  
> 1262.656 ±   43.751  ops/ms
> MessageDigests.getAndDigestSHA-1  1024 15   
> 104.889 ±3.554  ops/ms
> MessageDigests.getAndDigest  SHA-25616 15   
> 914.541 ±   55.621  ops/ms
> MessageDigests.getAndDigest  SHA-256  1024 15
> 85.708 ±1.394  ops/ms
> MessageDigests.getAndDigest  SHA-51216 15   
> 737.719 ±   53.671  ops/ms
> MessageDigests.getAndDigest  SHA-512  1024 15   
> 112.307 ±1.950  ops/ms
> 
> GC:
> MessageDigests.getAndDigest:·gc.alloc.rate.norm  MD516 15   
> 312.011 ±0.005B/op
> MessageDigests.getAndDigest:·gc.alloc.rate.normSHA-116 15   
> 584.020 ±0.006B/op
> MessageDigests.getAndDigest:·gc.alloc.rate.norm  SHA-25616 15   
> 544.019 ±0.016B/op
> MessageDigests.getAndDigest:·gc.alloc.rate.norm  SHA-51216 15  
> 1056.037 ±0.003B/op
> 
> Target:
> Benchmark (digesterName)  (length)Cnt 
> Score  Error   Units
> MessageDigests.digestMD516 15  
> 3134.462 ±   43.685  ops/ms
> MessageDigests.digestMD5  1024 15   
> 323.667 ±0.633  ops/ms
> MessageDigests.digest  SHA-116 15  
> 1418.742 ±   38.223  ops/ms
> MessageDigests.digest  SHA-1  1024 15   
> 110.178 ±0.788  ops/ms
> MessageDigests.digestSHA-25616 15  
> 1037.949 ±   21.214  ops/ms
> MessageDigests.digestSHA-256  1024 15
> 89.671 ±0.228  ops/ms
> MessageDigests.digestSHA-51216 15   
> 812.028 ±   39.489  ops/ms
> MessageDigests.digestSHA-512  1024 15   
> 116.738 ±0.249  ops/ms
> MessageDigests.getAndDigest  MD516 15  
> 2314.379 ±  229.294  ops/ms
> MessageDigests.getAndDigest  MD5  1024 15   
> 307.835 ±5.730  ops/ms
> MessageDigests.getAndDigestSHA-116 15  
> 1326.887 ±   63.263  ops/ms
> MessageDigests.getAndDigestSHA-1  1024 15   
> 106.611 ±2.292  ops/ms
> MessageDigests.getAndDigest  SHA-25616 15   
> 961.589 ±   82.052  ops/ms
> MessageDigests.getAndDigest  SHA-256  1024 15
> 88.646 ±0.194  ops/ms
> MessageDigests.getAndDigest  SHA-51216 15   
> 775.417 ±   56.775  ops/ms
> MessageDigests.getAndDigest  SHA-512  1024 15   
> 112.904 ±2.014  ops/ms
> 
> GC
> MessageDigests.getAndDigest:·gc.alloc.rate.norm  MD516 15   
> 232.009 ± 

Integrated: 8258796: [test] Apply HexFormat to tests for java.security

[Roger Riggs]

On Fri, 8 Jan 2021 15:43:45 GMT, Roger Riggs  wrote:

> Followup to the addition of java.util.HexFormat.  
> Uses of adhoc hexadecimal conversions are replaced with HexFormat.
> Redundant utility methods are modified or removed.
> In a few places the data being printed is ASN.1 and the test utility 
> HexPrinter with the ASNFormatter is added.

This pull request has now been integrated.

Changeset: 628c546b
Author:Roger Riggs 
URL:   https://git.openjdk.java.net/jdk/commit/628c546b
Stats: 758 lines in 43 files changed: 81 ins; 466 del; 211 mod

8258796: [test] Apply HexFormat to tests for java.security

Reviewed-by: xuelei

-

PR: https://git.openjdk.java.net/jdk/pull/2006

Integrated: 8259493: [test] Use HexFormat instead of adhoc hex utilities in network code and locale SoftKeys

[Roger Riggs]

On Fri, 8 Jan 2021 20:34:10 GMT, Roger Riggs  wrote:

> Cleanup of tests test/jdk/java/net and test/jdk/sun/net that format 
> hexadecimal strings to use java.util.HexFormat methods.
> Also in tests test/jdk/java/util/Locale/SoftKeys.

This pull request has now been integrated.

Changeset: 876c7fb5
Author:Roger Riggs 
URL:   https://git.openjdk.java.net/jdk/commit/876c7fb5
Stats: 75 lines in 5 files changed: 2 ins; 63 del; 10 mod

8259493: [test] Use HexFormat instead of adhoc hex utilities in network code 
and locale SoftKeys

Reviewed-by: lancea, naoto

-

PR: https://git.openjdk.java.net/jdk/pull/2009

Re: RFR: 8259319: Illegal package access when SunPKCS11 requires SunJCE's classes [v2]

[Martin Balao]

On Fri, 8 Jan 2021 19:35:47 GMT, Valerie Peng  wrote:

>> Martin Balao has updated the pull request incrementally with two additional 
>> commits since the last revision:
>> 
>>  - Limit P11Util::getProvider privileged access to the required 
>> 'accessClassInPackage' RuntimePermission only.
>>  - New line character inserted at the end of IllegalPackageAccess.java test 
>> file
>
> test/jdk/sun/security/pkcs11/KeyAgreement/IllegalPackageAccess.java line 96:
> 
>> 94: }
>> 95: 
>> 96: }
> 
> nit: add a newline here, to get rid of the red icon...

Yes, sure. Thanks for your feedback.

> src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Util.java line 
> 102:
> 
>> 100: }
>> 101: }
>> 102: });
> 
> Sean's suggestion is to add additional arguments here, e.g. null, new 
> RuntimePermission("accessClassInPackage." + ).

Yes, replied to Sean above. Thanks.

-

PR: https://git.openjdk.java.net/jdk/pull/1961

Re: RFR: 8259319: Illegal package access when SunPKCS11 requires SunJCE's classes [v2]

[Martin Balao]

On Thu, 7 Jan 2021 21:23:55 GMT, Sean Mullan  wrote:

>> Martin Balao has updated the pull request incrementally with two additional 
>> commits since the last revision:
>> 
>>  - Limit P11Util::getProvider privileged access to the required 
>> 'accessClassInPackage' RuntimePermission only.
>>  - New line character inserted at the end of IllegalPackageAccess.java test 
>> file
>
> src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Util.java line 
> 90:
> 
>> 88: p = Security.getProvider(providerName);
>> 89: if (p == null) {
>> 90: p = AccessController.doPrivileged(
> 
> Could you use the limited version of doPrivileged and only assert the 
> permissions that are strictly necessary to instantiate a provider?

Yes, makes sense. Thanks for your feedback.

-

PR: https://git.openjdk.java.net/jdk/pull/1961

Re: RFR: 8259319: Illegal package access when SunPKCS11 requires SunJCE's classes

[Martin Balao]

On Thu, 7 Jan 2021 19:29:29 GMT, Valerie Peng  wrote:

>> As described in JDK-8259319 [1], this fix proposal is to set proper access 
>> permissions so the SunPKCS11 provider can create instances of SunJCE classes 
>> when a Security Manager is installed and the fallback scheme is used.
>> 
>> No regressions found in jdk/sun/security/pkcs11 tests category.
>> 
>> --
>> [1] - https://bugs.openjdk.java.net/browse/JDK-8259319
>
> Obscure bug, thanks for report and the fix. I will take a look.

New proposal limiting the privileges in P11Util::getProvider method and adding 
a new line character at the end of the IllegalPackageAccess test file.

-

PR: https://git.openjdk.java.net/jdk/pull/1961

Re: RFR: 8259319: Illegal package access when SunPKCS11 requires SunJCE's classes [v2]

[Martin Balao]

> As described in JDK-8259319 [1], this fix proposal is to set proper access 
> permissions so the SunPKCS11 provider can create instances of SunJCE classes 
> when a Security Manager is installed and the fallback scheme is used.
> 
> No regressions found in jdk/sun/security/pkcs11 tests category.
> 
> --
> [1] - https://bugs.openjdk.java.net/browse/JDK-8259319

Martin Balao has updated the pull request incrementally with two additional 
commits since the last revision:

 - Limit P11Util::getProvider privileged access to the required 
'accessClassInPackage' RuntimePermission only.
 - New line character inserted at the end of IllegalPackageAccess.java test file

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1961/files
  - new: https://git.openjdk.java.net/jdk/pull/1961/files/2fcfa69a..5e1e0a97

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1961&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1961&range=00-01

  Stats: 23 lines in 2 files changed: 10 ins; 0 del; 13 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1961.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1961/head:pull/1961

PR: https://git.openjdk.java.net/jdk/pull/1961

Re: RFR: 8259493: [test] Use HexFormat instead of adhoc hex utilities in network code and locale SoftKeys

[Naoto Sato]

On Fri, 8 Jan 2021 20:34:10 GMT, Roger Riggs  wrote:

> Cleanup of tests test/jdk/java/net and test/jdk/sun/net that format 
> hexadecimal strings to use java.util.HexFormat methods.
> Also in tests test/jdk/java/util/Locale/SoftKeys.

+1

-

Marked as reviewed by naoto (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2009

Re: RFR: 8259493: [test] Use HexFormat instead of adhoc hex utilities in network code and locale SoftKeys

[Lance Andersen]

On Fri, 8 Jan 2021 20:34:10 GMT, Roger Riggs  wrote:

> Cleanup of tests test/jdk/java/net and test/jdk/sun/net that format 
> hexadecimal strings to use java.util.HexFormat methods.
> Also in tests test/jdk/java/util/Locale/SoftKeys.

Looks good Roger.  Nice cleanup.

-

Marked as reviewed by lancea (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2009

RFR: 8259493: [test] Use HexFormat instead of adhoc hex utilities in network code and locale SoftKeys

[Roger Riggs]

Cleanup of tests test/jdk/java/net and test/jdk/sun/net that format hexadecimal 
strings to use java.util.HexFormat methods.
Also in tests test/jdk/java/util/Locale/SoftKeys.

-

Commit messages:
 - 8259493: [test] Use HexFormat instead of adhoc hex utilities in network code 
and locale SoftKeys

Changes: https://git.openjdk.java.net/jdk/pull/2009/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2009&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8259493
  Stats: 75 lines in 5 files changed: 2 ins; 63 del; 10 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2009.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2009/head:pull/2009

PR: https://git.openjdk.java.net/jdk/pull/2009

Re: RFR: 8259319: Illegal package access when SunPKCS11 requires SunJCE's classes

[Valerie Peng]

On Wed, 6 Jan 2021 15:33:59 GMT, Martin Balao  wrote:

> As described in JDK-8259319 [1], this fix proposal is to set proper access 
> permissions so the SunPKCS11 provider can create instances of SunJCE classes 
> when a Security Manager is installed and the fallback scheme is used.
> 
> No regressions found in jdk/sun/security/pkcs11 tests category.
> 
> --
> [1] - https://bugs.openjdk.java.net/browse/JDK-8259319

src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Util.java line 102:

> 100: }
> 101: }
> 102: });

Sean's suggestion is to add additional arguments here, e.g. null, new 
RuntimePermission("accessClassInPackage." + ).

-

PR: https://git.openjdk.java.net/jdk/pull/1961

Re: RFR: 8259319: Illegal package access when SunPKCS11 requires SunJCE's classes

[Valerie Peng]

On Wed, 6 Jan 2021 15:33:59 GMT, Martin Balao  wrote:

> As described in JDK-8259319 [1], this fix proposal is to set proper access 
> permissions so the SunPKCS11 provider can create instances of SunJCE classes 
> when a Security Manager is installed and the fallback scheme is used.
> 
> No regressions found in jdk/sun/security/pkcs11 tests category.
> 
> --
> [1] - https://bugs.openjdk.java.net/browse/JDK-8259319

test/jdk/sun/security/pkcs11/KeyAgreement/IllegalPackageAccess.java line 96:

> 94: }
> 95: 
> 96: }

nit: add a newline here, to get rid of the red icon...

-

PR: https://git.openjdk.java.net/jdk/pull/1961

Re: RFR: 8198540: Dynalink leaks memory when generating type converters [v5]

[Peter Levart]

On Fri, 8 Jan 2021 16:50:24 GMT, Attila Szegedi  wrote:

>> I checked the code of ClassValue and it can be assumed that it publishes 
>> associated values safely. The proof is that it keeps values that it 
>> publishes assigned to the final field `java.lang.ClassValue.Entry#value`.
>
> So, are you saying the solution where I kept the fields volatile and 
> initialized them with `Map.of()` is safe? If so, that'd be good news; I'm 
> inclined these days to write as much null-free code as possible :-)

Yes, the pre-initialized fields to Map.of() are safe regardless of whether they 
are volatile or not (so I would keep them non-volatile to optimize fast-path). 
Because the BiClassValues instance is published safely to other threads via 
ClassValue and because you never assign null to the fields later on.

-

PR: https://git.openjdk.java.net/jdk/pull/1918

Re: RFR: JDK-8250768: javac should be adapted to changes in JEP 12 [v15]

[Jan Lahoda]

> This is an update to javac and javadoc, to introduce support for Preview 
> APIs, and generally improve javac and javadoc behavior to more closely adhere 
> to JEP 12.
> 
> The notable changes are:
> 
>  * adding support for Preview APIs (javac until now supported primarily only 
> preview language features, and APIs associated with preview language 
> features). This includes:
>  * the @PreviewFeature annotation has boolean attribute "reflective", 
> which should be true for reflective Preview APIs, false otherwise. This 
> replaces the existing "essentialAPI" attribute with roughly inverted meaning.
>  * the preview warnings for preview APIs are auto-suppressed as described 
> in the JEP 12. E.g. the module that declares the preview API is free to use 
> it without warnings
>  * improving error/warning messages. Please see [1] for a list of 
> cases/examples.
>  * class files are only marked as preview if they are using a preview 
> feature. [1] also shows if a class file is marked as preview or not.
>  * the PreviewFeature annotation has been moved to jdk.internal.javac package 
> (originally was in the jdk.internal package).
>  * Preview API in JDK's javadoc no longer needs to specify @preview tag in 
> the source files. javadoc will auto-generate a note for @PreviewFeature 
> elements, see e.g. [2] and [3] (non-reflective and reflective API, 
> respectively). A summary of preview elements is also provided [4]. Existing 
> uses of @preview have been updated/removed.
>  * non-JDK javadoc is also enhanced to auto-generate preview notes for uses 
> of Preview elements, and for declaring elements using preview language 
> features [5].
>  
>  Please also see the CSR [6] for more information.
>  
>  [1] http://cr.openjdk.java.net/~jlahoda/8250768/JEP12-errors-warnings-v6.html
>  [2] 
> http://cr.openjdk.java.net/~jlahoda/8250768/jdk.javadoc.00/api/java.base/java/lang/Record.html
>  [3] 
> http://cr.openjdk.java.net/~jlahoda/8250768/jdk.javadoc.00/api/java.compiler/javax/lang/model/element/RecordComponentElement.html
>  [4] 
> http://cr.openjdk.java.net/~jlahoda/8250768/jdk.javadoc.00/api/preview-list.html
>  [5] http://cr.openjdk.java.net/~jlahoda/8250768/test.javadoc.00/
>  [6] https://bugs.openjdk.java.net/browse/JDK-8250769

Jan Lahoda has updated the pull request with a new target base due to a merge 
or a rebase. The pull request now contains 59 commits:

 - Merging master into JDK-8250768
 - Updating copyright years.
 - Fixing tests after a merge.
 - Merging master into JDK-8250768
 - Merging recent master changes into JDK-8250768
 - Fixing navigator for the PREVIEW page.
 - Fixing typo.
 - Removing obsolette @PreviewFeature.
 - Merging master into JDK-8250768
 - Removing unnecessary property keys.
 - ... and 49 more: https://git.openjdk.java.net/jdk/compare/090bd3af...4f654955

-

Changes: https://git.openjdk.java.net/jdk/pull/703/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=703&range=14
  Stats: 3802 lines in 133 files changed: 2724 ins; 695 del; 383 mod
  Patch: https://git.openjdk.java.net/jdk/pull/703.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/703/head:pull/703

PR: https://git.openjdk.java.net/jdk/pull/703

Re: RFR: 8258796: [test] Apply HexFormat to tests for java.security [v2]

[Xue-Lei Andrew Fan]

On Fri, 8 Jan 2021 16:26:09 GMT, Roger Riggs  wrote:

>> Followup to the addition of java.util.HexFormat.  
>> Uses of adhoc hexadecimal conversions are replaced with HexFormat.
>> Redundant utility methods are modified or removed.
>> In a few places the data being printed is ASN.1 and the test utility 
>> HexPrinter with the ASNFormatter is added.
>
> Roger Riggs has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Update copyrights to 2021

Looks good to me.  Thank you!

-

Marked as reviewed by xuelei (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2006

Re: RFR: 8198540: Dynalink leaks memory when generating type converters [v5]

[Attila Szegedi]

On Fri, 8 Jan 2021 13:32:08 GMT, Peter Levart  wrote:

>>> IIUC, your changes mostly all flow from the decision to declare the fields 
>>> as non-volatile; if they were still declared as volatile then it'd be 
>>> impossible to observe null in them, I think (correct me if I'm wrong; it 
>>> seems like you thought through this quite thoroughly) as then I don't see 
>>> how could a volatile read happen before the initial volatile writes as the 
>>> writes are part of the ClassValues constructor invocation and the reference 
>>> to the ClassValues object is unavailable externally before the constructor 
>>> completes. In any case, your approach definitely avoids any of these 
>>> concerns so I'm inclined to go with it.
>> 
>> It depends entirely on the guarantees of ClassValue and not on whether the 
>> fields are volatile or not. If ClassValue publishes the BiClassValues object 
>> via data race then even if the fields in BiClassValues are volatile and 
>> initialized in constructor, the publishing write in ClassValue could get 
>> reordered before the volatile writes of the fields, so you could observe the 
>> fields uninitialized.
>> I can't find in the spec of ClassValue any guarantees of ordering, but I 
>> guess the implementation does guarantee safe publication. So if you want to 
>> rely on ClassValue guaranteeing safe publication, you can pre-initialized 
>> the fields in constructor and code can assume they are never null even if 
>> they are not volatile.
>
> I checked the code of ClassValue and it can be assumed that it publishes 
> associated values safely. The proof is that it keeps values that it publishes 
> assigned to the final field `java.lang.ClassValue.Entry#value`.

So, are you saying the solution where I kept the fields volatile and 
initialized them with `Map.of()` is safe? If so, that'd be good news; I'm 
inclined these days to write as much null-free code as possible :-)

-

PR: https://git.openjdk.java.net/jdk/pull/1918

Re: Monitoring wrapped ThreadPoolExecutor returned from Executors

[Doug Lea]

On 1/7/21 12:57 PM, Jason Mehrens wrote:

Hi Doug,

What are your thoughts on promoting monitoring methods from TPE and or FJP to 
AbstractExecutorService?  The default implementations could just return -1.  An 
example precedent is OperatingSystemMXBean::getSystemLoadAverage.  The 
Executors.DelegatedExecutorService could then be modified to extend 
AbstractExecutorService and forward the new methods and the existing 
AES::taskFor calls when the wrapped Executor is also an 
AbstractExecutorService.  The return types of the Executors.newXXX would remain 
the same.


Maybe. But for now, here's a cheap trick that might be tolerable: Add to 
DelegatedExecutorService:


public String toString() { return e.toString(); }

The juc executors (ThreadPoolExecutor and ForkJoinPool that could be 
wrapped here) both print pool size, active threads, queued tasks, and 
completed tasks. It would require not-very-pleasant string parsing in 
monitoring tools, but this might be good enough for Micrometer and others?





I suppose the tradeoff is that adding any new default method to ExecutorService 
and or new methods to AbstractExecutorService could break 3rd party code.

Jason


From: core-libs-dev  on behalf of Doug Lea 

Sent: Thursday, January 7, 2021 7:09 AM
To: core-libs-dev@openjdk.java.net
Subject: Re: Monitoring wrapped ThreadPoolExecutor returned from Executors

On 1/5/21 10:11 PM, Tommy Ludwig wrote:

In the Micrometer project, we provide metrics instrumentation of 
`ExectorService`s. For `ThreadPoolExecutor`s, we track the number of completed 
tasks, active tasks, thread pool sizes, task queue size and remaining capacity 
via methods from `ThreadPoolExecutor`. We are currently using a brittle 
reflection hack[1] to do this for the wrapped `ThreadPoolExecutor` returned 
from `Executors` methods `newSingleThreadExecutor` and 
`newSingleThreadScheduledExecutor`. With the introduction of JEP-396 in JDK 16, 
our reflection hack throws an InaccessibleObjectException by default.

I am not seeing a proper way to get at the methods we use for the metrics (e.g. 
`ThreadPoolExecutor::getCompletedTaskCount`) in this case. Is there a way that 
I am missing?

There's no guarantee that newSingleThreadExecutor returns a restricted
view of a ThreadPoolExecutor, so there can't be a guaranteed way of
accessing it,

But I'm sympathetic to the idea that under the current implementation
(which is unlikely to change anytime soon), the stats are available, and
should be available to monitoring tools. But none of the ways to do this
are very attractive: Creating a MonitorableExecutorService interface and
returning that? Making the internal view class public with a protected
getExecutor method?

Re: RFR: 8258796: [test] Apply HexFormat to tests for java.security [v2]

[Roger Riggs]

> Followup to the addition of java.util.HexFormat.  
> Uses of adhoc hexadecimal conversions are replaced with HexFormat.
> Redundant utility methods are modified or removed.
> In a few places the data being printed is ASN.1 and the test utility 
> HexPrinter with the ASNFormatter is added.

Roger Riggs has updated the pull request incrementally with one additional 
commit since the last revision:

  Update copyrights to 2021

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2006/files
  - new: https://git.openjdk.java.net/jdk/pull/2006/files/c437a74b..b6908530

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2006&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2006&range=00-01

  Stats: 43 lines in 43 files changed: 0 ins; 0 del; 43 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2006.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2006/head:pull/2006

PR: https://git.openjdk.java.net/jdk/pull/2006

Re: RFR: 8258796: [test] Apply HexFormat to tests for java.security

[Roger Riggs]

On Fri, 8 Jan 2021 15:43:45 GMT, Roger Riggs  wrote:

> Followup to the addition of java.util.HexFormat.  
> Uses of adhoc hexadecimal conversions are replaced with HexFormat.
> Redundant utility methods are modified or removed.
> In a few places the data being printed is ASN.1 and the test utility 
> HexPrinter with the ASNFormatter is added.

Removing the hotspot-runtime label to avoid spamming that alias. 
Add it back if it is warranted.

-

PR: https://git.openjdk.java.net/jdk/pull/2006

RFR: 8258796: [test] Apply HexFormat to tests for java.security

[Roger Riggs]

Followup to the addition of java.util.HexFormat.  
Uses of adhoc hexadecimal conversions are replaced with HexFormat.
Redundant utility methods are modified or removed.
In a few places the data being printed is ASN.1 and the test utility HexPrinter 
with the ASNFormatter is added.

-

Commit messages:
 - 8258796: [test] Apply HexFormat to tests for java.security

Changes: https://git.openjdk.java.net/jdk/pull/2006/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2006&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8258796
  Stats: 738 lines in 43 files changed: 81 ins; 466 del; 191 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2006.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2006/head:pull/2006

PR: https://git.openjdk.java.net/jdk/pull/2006

Re: RFR: JDK-8250768: javac should be adapted to changes in JEP 12 [v14]

[Jan Lahoda]

> This is an update to javac and javadoc, to introduce support for Preview 
> APIs, and generally improve javac and javadoc behavior to more closely adhere 
> to JEP 12.
> 
> The notable changes are:
> 
>  * adding support for Preview APIs (javac until now supported primarily only 
> preview language features, and APIs associated with preview language 
> features). This includes:
>  * the @PreviewFeature annotation has boolean attribute "reflective", 
> which should be true for reflective Preview APIs, false otherwise. This 
> replaces the existing "essentialAPI" attribute with roughly inverted meaning.
>  * the preview warnings for preview APIs are auto-suppressed as described 
> in the JEP 12. E.g. the module that declares the preview API is free to use 
> it without warnings
>  * improving error/warning messages. Please see [1] for a list of 
> cases/examples.
>  * class files are only marked as preview if they are using a preview 
> feature. [1] also shows if a class file is marked as preview or not.
>  * the PreviewFeature annotation has been moved to jdk.internal.javac package 
> (originally was in the jdk.internal package).
>  * Preview API in JDK's javadoc no longer needs to specify @preview tag in 
> the source files. javadoc will auto-generate a note for @PreviewFeature 
> elements, see e.g. [2] and [3] (non-reflective and reflective API, 
> respectively). A summary of preview elements is also provided [4]. Existing 
> uses of @preview have been updated/removed.
>  * non-JDK javadoc is also enhanced to auto-generate preview notes for uses 
> of Preview elements, and for declaring elements using preview language 
> features [5].
>  
>  Please also see the CSR [6] for more information.
>  
>  [1] http://cr.openjdk.java.net/~jlahoda/8250768/JEP12-errors-warnings-v6.html
>  [2] 
> http://cr.openjdk.java.net/~jlahoda/8250768/jdk.javadoc.00/api/java.base/java/lang/Record.html
>  [3] 
> http://cr.openjdk.java.net/~jlahoda/8250768/jdk.javadoc.00/api/java.compiler/javax/lang/model/element/RecordComponentElement.html
>  [4] 
> http://cr.openjdk.java.net/~jlahoda/8250768/jdk.javadoc.00/api/preview-list.html
>  [5] http://cr.openjdk.java.net/~jlahoda/8250768/test.javadoc.00/
>  [6] https://bugs.openjdk.java.net/browse/JDK-8250769

Jan Lahoda has updated the pull request incrementally with one additional 
commit since the last revision:

  Updating copyright years.

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/703/files
  - new: https://git.openjdk.java.net/jdk/pull/703/files/a8046dde..56371c47

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=703&range=13
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=703&range=12-13

  Stats: 103 lines in 103 files changed: 0 ins; 0 del; 103 mod
  Patch: https://git.openjdk.java.net/jdk/pull/703.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/703/head:pull/703

PR: https://git.openjdk.java.net/jdk/pull/703

Re: RFR: JDK-8250768: javac should be adapted to changes in JEP 12 [v13]

[Jan Lahoda]

On Fri, 8 Jan 2021 01:51:52 GMT, Jonathan Gibbons  wrote:

>> Jan Lahoda has updated the pull request with a new target base due to a 
>> merge or a rebase. The pull request now contains 57 commits:
>> 
>>  - Fixing tests after a merge.
>>  - Merging master into JDK-8250768
>>  - Merging recent master changes into JDK-8250768
>>  - Fixing navigator for the PREVIEW page.
>>  - Fixing typo.
>>  - Removing obsolette @PreviewFeature.
>>  - Merging master into JDK-8250768
>>  - Removing unnecessary property keys.
>>  - Cleanup - removing unnecessary code.
>>  - Merging master into JDK-8250768-dev4
>>  - ... and 47 more: 
>> https://git.openjdk.java.net/jdk/compare/81c06242...a8046dde
>
> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/resources/doclets.properties
>  line 154:
> 
>> 152: doclet.Errors=Errors
>> 153: doclet.Classes=Classes
>> 154: doclet.Records=Records
> 
> I guess I'm mildly surprised to see all these items being removed...

These were used from DeprecatedListWriter.getSummaryKey which is removed by 
this patch (and is unused in the current mainline, as far as I know).

-

PR: https://git.openjdk.java.net/jdk/pull/703

Re: RFR: 8258422: Cleanup unnecessary null comparison before instanceof check in java.base [v4]

[Chris Hegarty]

On Fri, 8 Jan 2021 13:20:38 GMT, Aleksei Efimov  wrote:

>> This issue is blocked by [8258657][1]. It should not be integrated until 
>> after 8258657 has been resolved. The JIRA issues have been linked up to 
>> reflect this.
>> 
>> [1]: https://bugs.openjdk.java.net/browse/JDK-8258657
>
> [JDK-8258657](https://bugs.openjdk.java.net/browse/JDK-8258657) has been 
> resolved. The changes reverted by 
> [JDK-8258696](https://bugs.openjdk.java.net/browse/JDK-8258696) could also be 
> re-applied to `HttpClientImpl` class.

@AlekseiEfimov Yes please. Whatever is easier, include the changes to 
HttpClientImpl in this PR, or followup separately. Otherwise, I see no reason 
why this PR cannot proceed.

-

PR: https://git.openjdk.java.net/jdk/pull/20

Re: RFR: 8198540: Dynalink leaks memory when generating type converters [v5]

[Peter Levart]

On Fri, 8 Jan 2021 13:00:16 GMT, Peter Levart  wrote:

>>> So what do you think of this variant:
>> 
>> I like it. I originally kept the fields volatile so that we don't observe 
>> stale values on `getForward`/`getReverse`, but you're right in that even if 
>> we do, the correct value will be observed when doing a volatile read in 
>> compute, at the additional expense of evaluating class loader relationships, 
>> but again, that's the slow path.
>> 
>> IIUC, your changes mostly all flow from the decision to declare the fields 
>> as non-volatile; if they were still declared as volatile then it'd be 
>> impossible to observe null in them, I think (correct me if I'm wrong; it 
>> seems like you thought through this quite thoroughly) as then I don't see 
>> how could a volatile read happen before the initial volatile writes as the 
>> writes are part of the ClassValues constructor invocation and the reference 
>> to the ClassValues object is unavailable externally before the constructor 
>> completes. In any case, your approach definitely avoids any of these 
>> concerns so I'm inclined to go with it.
>
>> IIUC, your changes mostly all flow from the decision to declare the fields 
>> as non-volatile; if they were still declared as volatile then it'd be 
>> impossible to observe null in them, I think (correct me if I'm wrong; it 
>> seems like you thought through this quite thoroughly) as then I don't see 
>> how could a volatile read happen before the initial volatile writes as the 
>> writes are part of the ClassValues constructor invocation and the reference 
>> to the ClassValues object is unavailable externally before the constructor 
>> completes. In any case, your approach definitely avoids any of these 
>> concerns so I'm inclined to go with it.
> 
> It depends entirely on the guarantees of ClassValue and not on whether the 
> fields are volatile or not. If ClassValue publishes the BiClassValues object 
> via data race then even if the fields in BiClassValues are volatile and 
> initialized in constructor, the publishing write in ClassValue could get 
> reordered before the volatile writes of the fields, so you could observe the 
> fields uninitialized.
> I can't find in the spec of ClassValue any guarantees of ordering, but I 
> guess the implementation does guarantee safe publication. So if you want to 
> rely on ClassValue guaranteeing safe publication, you can pre-initialized the 
> fields in constructor and code can assume they are never null even if they 
> are not volatile.

I checked the code of ClassValue and it can be assumed that it publishes 
associated values safely. The proof is that it keeps values that it publishes 
assigned to the final field `java.lang.ClassValue.Entry#value`.

-

PR: https://git.openjdk.java.net/jdk/pull/1918

Re: RFR: 8258422: Cleanup unnecessary null comparison before instanceof check in java.base [v4]

[Aleksei Efimov]

On Sat, 19 Dec 2020 10:29:23 GMT, Chris Hegarty  wrote:

>> Thank you for checking `HttpURLConnection` and `Socket`.
>> The latest version looks good to me.
>
> This issue is blocked by [8258657][1]. It should not be integrated until 
> after 8258657 has been resolved. The JIRA issues have been linked up to 
> reflect this.
> 
> [1]: https://bugs.openjdk.java.net/browse/JDK-8258657

[JDK-8258657](https://bugs.openjdk.java.net/browse/JDK-8258657) has been 
resolved. The changes reverted by 
[JDK-8258696](https://bugs.openjdk.java.net/browse/JDK-8258696) could also be 
re-applied to `HttpClientImpl` class.

-

PR: https://git.openjdk.java.net/jdk/pull/20

Re: RFR: 8198540: Dynalink leaks memory when generating type converters [v5]

[Peter Levart]

On 1/8/21 2:03 PM, Peter Levart wrote:

On Fri, 8 Jan 2021 12:23:36 GMT, Attila Szegedi  wrote:


IIUC, your changes mostly all flow from the decision to declare the fields as 
non-volatile; if they were still declared as volatile then it'd be impossible 
to observe null in them, I think (correct me if I'm wrong; it seems like you 
thought through this quite thoroughly) as then I don't see how could a volatile 
read happen before the initial volatile writes as the writes are part of the 
ClassValues constructor invocation and the reference to the ClassValues object 
is unavailable externally before the constructor completes. In any case, your 
approach definitely avoids any of these concerns so I'm inclined to go with it.

It depends entirely on the guarantees of ClassValue and not on whether the 
fields are volatile or not. If ClassValue publishes the BiClassValues object 
via data race then even if the fields in BiClassValues are volatile and 
initialized in constructor, the publishing write in ClassValue could get 
reordered past



correction: past -> before



  the volatile writes of the fields, so you could observe the fields 
uninitialized.
I can't find in the spec of ClassValue any guarantees of ordering, but I guess 
the implementation does guarantee safe publication. So if you want to rely on 
ClassValue guaranteeing safe publication, you can pre-initialized the fields in 
constructor and code can assume they are never null even if they are not 
volatile.

-

PR: https://git.openjdk.java.net/jdk/pull/1918



To explain: Normal writes that appear in program order before a volatile 
write can not be observed to appear later than the volatile write. But 
normal writes that appear in program order after a volatile write can be 
observed to appear before the volatile write.



Regards, Peter

Re: RFR: 8198540: Dynalink leaks memory when generating type converters [v5]

[Peter Levart]

On Fri, 8 Jan 2021 12:47:17 GMT, Attila Szegedi  wrote:

>> _(NB: For this leak to occur, an application needs to be either creating and 
>> discarding linkers frequently, or creating and discarding class loaders 
>> frequently while creating Dynalink type converters for classes in them. 
>> Neither is typical usage, although they can occur in dynamic code loading 
>> environments such as OSGi.)_
>> 
>> I'll explain this one in a bit more detail. Dynalink creates and caches 
>> method handles for language-specific conversions between types. Each 
>> conversion is thus characterized by a `Class` object representing the type 
>> converted from, and a `Class` object representing the type converted to, 
>> thus they're keyed by a pair of `(Class, Class)`. Java API provides the 
>> excellent `ClassValue` class for associating values with a single class, but 
>> it lacks the – admittedly more niche – facility for associating a value with 
>> a pair of classes. I originally solved this problem using something that 
>> looks like a `ClassValue, T>>`. I say "looks like" because 
>> Dynalink has a specialized `ClassMap` class that works like `Map, 
>> T>` but it also goes to some pains to _not_ establish strong references from 
>> a class loader to either its children or to unrelated class loaders, for 
>> obvious reasons.
>> 
>> Surprisingly, the problem didn't lie in there, though. The problem was in 
>> the fact that `TypeConverterFactory` used `ClassValue` objects that were its 
>> non-static anonymous inner classes, and further the values they computed 
>> were also of non-static anonymous inner classes. Due to the way `ClassValue` 
>> internals work, this led to the `TypeConverterFactory` objects becoming 
>> anchored in a GC root of `Class.classValueMap` through the synthetic 
>> `this$0` reference chains in said inner classes… talk about non-obvious 
>> memory leaks. (I guess there's a lesson in there about not using 
>> `ClassValue` as an instance field, but there's a genuine need for them here, 
>> so…)
>> 
>> … so the first thing I did was I deconstructed all those anonymous inner 
>> classes into named static inner classes, and ended up with something that 
>> _did_ fix the memory leak (yay!) but at the same time there was a bunch of 
>> copying of constructor parameters from outer classes into the inner classes, 
>> the whole thing was very clunky with scary amounts of boilerplate. I felt 
>> there must exist a better solution.
>> 
>> And it exists! I ended up replacing the `ClassValue>` construct 
>> with a ground-up implementation of `BiClassValue`, representation of 
>> lazily computed values associated with a pair of types. This was the right 
>> abstraction the `TypeConverterFactory` code needed all along. 
>> `BiClassValue` is also not implemented as an abstract class but rather it 
>> is a final class and takes a `BiFunction, Class, T>` for 
>> computation of its values, thus there's never a risk of making it an 
>> instance-specific inner class (well, you still need to be careful with the 
>> bifunction lambda…) It also has an even better solution for avoiding strong 
>> references to child class loaders.
>> 
>> And that's why I'm not submitting here the smallest possible point fix to 
>> the memory leak, but rather a slightly larger one that architecturally fixes 
>> it the right way.
>> 
>> There are two test classes, they test subtly different scenarios. 
>> `TypeConverterFactoryMemoryLeakTest` tests that when a 
>> `TypeConverterFactory` instance becomes unreachable, all method handles it 
>> created also become eligible for collection. 
>> `TypeConverterFactoryRetentionTests` on the other hand test that the factory 
>> itself won't prevent class loaders from being collected by virtue of 
>> creating converters for them.
>
> Attila Szegedi has updated the pull request incrementally with three 
> additional commits since the last revision:
> 
>  - Reduce some code duplication; make some local vars final.
>  - Bugfix
>  - non-volatile forward/reverse

src/jdk.dynalink/share/classes/jdk/dynalink/BiClassValue.java line 136:

> 134: Map.Entry, T>[] entries = 
> map.entrySet().toArray(new Map.Entry[map.size() + 1]);
> 135: entries[map.size()] = Map.entry(c, newValue);
> 136: newMap = Map.ofEntries(entries);

Pardon me! A result of introducing new local late in the process.

-

PR: https://git.openjdk.java.net/jdk/pull/1918

Re: RFR: 8198540: Dynalink leaks memory when generating type converters [v5]

[Peter Levart]

On Fri, 8 Jan 2021 12:23:36 GMT, Attila Szegedi  wrote:

> IIUC, your changes mostly all flow from the decision to declare the fields as 
> non-volatile; if they were still declared as volatile then it'd be impossible 
> to observe null in them, I think (correct me if I'm wrong; it seems like you 
> thought through this quite thoroughly) as then I don't see how could a 
> volatile read happen before the initial volatile writes as the writes are 
> part of the ClassValues constructor invocation and the reference to the 
> ClassValues object is unavailable externally before the constructor 
> completes. In any case, your approach definitely avoids any of these concerns 
> so I'm inclined to go with it.

It depends entirely on the guarantees of ClassValue and not on whether the 
fields are volatile or not. If ClassValue publishes the BiClassValues object 
via data race then even if the fields in BiClassValues are volatile and 
initialized in constructor, the publishing write in ClassValue could get 
reordered past the volatile writes of the fields, so you could observe the 
fields uninitialized.
I can't find in the spec of ClassValue any guarantees of ordering, but I guess 
the implementation does guarantee safe publication. So if you want to rely on 
ClassValue guaranteeing safe publication, you can pre-initialized the fields in 
constructor and code can assume they are never null even if they are not 
volatile.

-

PR: https://git.openjdk.java.net/jdk/pull/1918

Re: RFR: 8198540: Dynalink leaks memory when generating type converters [v5]

[Attila Szegedi]

> _(NB: For this leak to occur, an application needs to be either creating and 
> discarding linkers frequently, or creating and discarding class loaders 
> frequently while creating Dynalink type converters for classes in them. 
> Neither is typical usage, although they can occur in dynamic code loading 
> environments such as OSGi.)_
> 
> I'll explain this one in a bit more detail. Dynalink creates and caches 
> method handles for language-specific conversions between types. Each 
> conversion is thus characterized by a `Class` object representing the type 
> converted from, and a `Class` object representing the type converted to, thus 
> they're keyed by a pair of `(Class, Class)`. Java API provides the excellent 
> `ClassValue` class for associating values with a single class, but it lacks 
> the – admittedly more niche – facility for associating a value with a pair of 
> classes. I originally solved this problem using something that looks like a 
> `ClassValue, T>>`. I say "looks like" because Dynalink has a 
> specialized `ClassMap` class that works like `Map, T>` but it also 
> goes to some pains to _not_ establish strong references from a class loader 
> to either its children or to unrelated class loaders, for obvious reasons.
> 
> Surprisingly, the problem didn't lie in there, though. The problem was in the 
> fact that `TypeConverterFactory` used `ClassValue` objects that were its 
> non-static anonymous inner classes, and further the values they computed were 
> also of non-static anonymous inner classes. Due to the way `ClassValue` 
> internals work, this led to the `TypeConverterFactory` objects becoming 
> anchored in a GC root of `Class.classValueMap` through the synthetic `this$0` 
> reference chains in said inner classes… talk about non-obvious memory leaks. 
> (I guess there's a lesson in there about not using `ClassValue` as an 
> instance field, but there's a genuine need for them here, so…)
> 
> … so the first thing I did was I deconstructed all those anonymous inner 
> classes into named static inner classes, and ended up with something that 
> _did_ fix the memory leak (yay!) but at the same time there was a bunch of 
> copying of constructor parameters from outer classes into the inner classes, 
> the whole thing was very clunky with scary amounts of boilerplate. I felt 
> there must exist a better solution.
> 
> And it exists! I ended up replacing the `ClassValue>` construct 
> with a ground-up implementation of `BiClassValue`, representation of 
> lazily computed values associated with a pair of types. This was the right 
> abstraction the `TypeConverterFactory` code needed all along. 
> `BiClassValue` is also not implemented as an abstract class but rather it 
> is a final class and takes a `BiFunction, Class, T>` for 
> computation of its values, thus there's never a risk of making it an 
> instance-specific inner class (well, you still need to be careful with the 
> bifunction lambda…) It also has an even better solution for avoiding strong 
> references to child class loaders.
> 
> And that's why I'm not submitting here the smallest possible point fix to the 
> memory leak, but rather a slightly larger one that architecturally fixes it 
> the right way.
> 
> There are two test classes, they test subtly different scenarios. 
> `TypeConverterFactoryMemoryLeakTest` tests that when a `TypeConverterFactory` 
> instance becomes unreachable, all method handles it created also become 
> eligible for collection. `TypeConverterFactoryRetentionTests` on the other 
> hand test that the factory itself won't prevent class loaders from being 
> collected by virtue of creating converters for them.

Attila Szegedi has updated the pull request incrementally with three additional 
commits since the last revision:

 - Reduce some code duplication; make some local vars final.
 - Bugfix
 - non-volatile forward/reverse

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1918/files
  - new: https://git.openjdk.java.net/jdk/pull/1918/files/3f934a21..a8d81ebb

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1918&range=04
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1918&range=03-04

  Stats: 42 lines in 1 file changed: 15 ins; 4 del; 23 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1918.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1918/head:pull/1918

PR: https://git.openjdk.java.net/jdk/pull/1918

Re: RFR: 8198540: Dynalink leaks memory when generating type converters [v4]

[Attila Szegedi]

On Fri, 8 Jan 2021 11:06:17 GMT, Peter Levart  wrote:

> So what do you think of this variant:

I like it. I originally kept the fields volatile so that we don't observe stale 
values on `getForward`/`getReverse`, but you're right in that even if we do, 
the correct value will be observed when doing a volatile read in compute, at 
the additional expense of evaluating class loader relationships, but again, 
that's the slow path.

IIUC, your changes mostly all flow from the decision to declare the fields as 
non-volatile; if they were still declared as volatile then it'd be impossible 
to observe null in them, I think (correct me if I'm wrong; it seems like you 
thought through this quite thoroughly) as then I don't see how could a volatile 
read happen before the initial volatile writes as the writes are part of the 
ClassValues constructor invocation and the reference to the ClassValues object 
is unavailable externally before the constructor completes. In any case, your 
approach definitely avoids any of these concerns so I'm inclined to go with it.

-

PR: https://git.openjdk.java.net/jdk/pull/1918

Re: RFR: 8193031: Collections.addAll is likely to perform worse than Collection.addAll

[Peter Levart]

On Fri, 8 Jan 2021 11:27:57 GMT, Peter Levart  wrote:

>> @forax, @plevart could I ask for review once again?
>
> Hi @stsypanov, 
> 
>> The **behavior** of this convenience method is **identical** to that of 
>> c.addAll(Arrays.asList(elements))
> 
> What about:
> 
>> The **behaviour** of this convenience method is **similar** to that of 
>> c.addAll(Arrays.asList(elements))
> 
> ...since it is not entirely identical. The outcome is usually identical 
> because collections usually adhere to the specification, but we can not claim 
> the behaviour is identical if the target collection does not adhere.

...but we could employ this method to guarantee more than 
`c.addAll(Arrays.asList(elements))` does. So what about:

> The behaviour of this convenience method is similar to that of 
> `c.addAll(Collections.unmodifiableList(Arrays.asList(elements)))`

This means that the method guarantees that the passed in `elements` array will 
not be modified even if given collection `c` is not trusted. Speaking of that, 
perhaps you could try benchmarking such 
`c.addAll(Collections.unmodifiableList(Arrays.asList(elements)))` and see how 
it compares. The speed-up you observed from `c.addAll(Arrays.asList(elements))` 
with some collections was probably a result of them calling .toArray() on the 
argument collection and incorporating the resulting array into their own data 
structure in a bulk-copying-way. So 
`c.addAll(Collections.unmodifiableList(Arrays.asList(elements)))` might have 
same performance characteristics while guaranteeing same things about argument 
array. It might be slower when Iterator is employed though because 
unmodifiableList wrapper wraps Iterator(s) too

-

PR: https://git.openjdk.java.net/jdk/pull/1764

Re: RFR: 8193031: Collections.addAll is likely to perform worse than Collection.addAll

[Peter Levart]

On Fri, 8 Jan 2021 09:37:23 GMT, Сергей Цыпанов 
 wrote:

>> Apart from the @SuppressWarnings, this looks good to me.
>> And i like the irony of this.
>
> @forax, @plevart could I ask for review once again?

Hi @stsypanov, 

> The **behavior** of this convenience method is **identical** to that of 
> c.addAll(Arrays.asList(elements))

What about:

> The **behaviour** of this convenience method is **similar** to that of 
> c.addAll(Arrays.asList(elements))

...since it is not entirely identical. The outcome is usually identical because 
collections usually adhere to the specification, but we can not claim the 
behaviour is identical if the target collection does not adhere.

-

PR: https://git.openjdk.java.net/jdk/pull/1764

Re: RFR: 8198540: Dynalink leaks memory when generating type converters [v4]

[Peter Levart]

On Fri, 8 Jan 2021 08:49:44 GMT, Attila Szegedi  wrote:

>> Hi Attila,
>> 
>> This looks good.
>> 
>> If I understand correctly, your `BiClassValue` is a holder for a 
>> `BiFunction` and a `BiClassValueRoot` which is a 
>> `ClassValue`. The `BiClassValues` contains two lazily 
>> constructed Map(s): `forward` and `reverse`. You then employ a logic where 
>> you either use the `forward` map obtained from c1 if c1.classLoader sees 
>> c2.classLoader or `backward` map obtained from c2 if c2.classLoader sees 
>> c1.classLoader or you don't employ caching if c1.classLoader and 
>> c2.classLoader are unrelated.
>> The need for two Maps is probably because the two Class components of the 
>> "bi-key" are ordered, right? So `BiClassValue bcv = ...;` `bcv.get(c1, c2)` 
>> is not the same as `bcv.get(c2, c1)` 
>> Alternatively you could have a `BiClassValue` with two embedded 
>> `ClassValue`(s):
>> 
>> final CVRoot forward = new CVRoot<>();
>> final CVRoot reverse = new CVRoor<>();
>> 
>> static class CVRoot extends ClassValue> {
>>   public CHMCL get(Class clazz) { return new 
>> CHMCL<>(getClassLoader(clazz)); }
>> }
>> 
>> static class CHMCL extends ConcurrentHashMap, T> {
>>   final ClassLoader classLoader;
>>   CHMCL(ClassLoader cl) { classLoader = cl; }
>> }
>> 
>> ...with that you could get rid of the intermediary BiClassValues object. It 
>> would be replaced with a ConcurrentHashMap subclass that would contain a 
>> field to hold the cached ClassLoader of the c1/c2. One de-reference less...
>> 
>> As for the main logic, it would be similar to what you have now. Or it could 
>> be different. I wonder what is the performance of canReferenceDirectly(). If 
>> you used SharedSecrets to obtain a ClassLoader of a Class without security 
>> checks (and thus overhead), you could perhaps evaluate the 
>> canReferenceDirectly() before lookups so you would not needlessly trigger 
>> initialization of ClassValue(s) that don't need to get initialized. 
>> 
>> WDYT?
>
> Hey Peter,
> 
> thank you for the most thoughtful review! You understood everything right. 
> Your approach looks mostly equivalent to mine, with bit of different 
> tradeoffs. I'm indeed using `BiClassValues` as an intermediate object; it has 
> two benefits. The smaller benefit is that the class loader lookup is 
> performed once per class and stored only once per class. The larger benefit 
> is that it defers the creation of the CHMs until it has something to put into 
> them. Most of the time, only one of either forward or reverse will be 
> created. You're right that the same benefit could be had if we checked 
> `canReferenceDirectly` first, although I'm trying to make the fast path as 
> fast as possible.
> 
> I'm even thinking I could afford to read the class loader on each use of main 
> logic when a cached value is not available and not even use a CHM subclass to 
> hold it. 
> [canReferenceDirectly](https://github.com/openjdk/jdk/blob/master/src/jdk.dynalink/share/classes/jdk/dynalink/internal/InternalTypeUtilities.java#L70)
>  is basically equivalent to "isDescendantOf" and can be evaluated fairly 
> quickly, unless there's a really long class loader chain. (FWIW, it is also 
> equivalent to [`!ClassLoader.needsClassLoaderPermissionCheck(from, 
> to)`](https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/lang/ClassLoader.java#L2016)
>  but, alas, that's private, and even `ClassLoader.isAncestor` that I could 
> similarly use is package-private.
> 
> In any case, your suggestion nudged me towards a different rework: 
> `BiClassValues` now uses immutable maps instead of concurrent ones, and uses 
> those `VarHandles` for `compareAndExchange` on them. In this sense, 
> `BiClassValue` is now little more than a pair of atomic references. I decided 
> CHM was an overkill here as the map contents stabilize over time; using 
> immutable maps feels more natural.
> 
> I also realized that `canReferenceDirectly` invocations also need to happen 
> in a `doPrivileged` block (Dynalink itself is loaded through the platform 
> class loader, so needs these.)
> 
> FWIW, your suggestion to use `SharedSecrets` is intriguing - if I could 
> access both `ClassLoader.isAncestor` and `ClassLoader.getClassLoader` through 
> `JavaLangAccess`, that'd indeed spare me having to go through `doPrivileged` 
> blocks. OTOH, there's other places in Dynalink that could benefit from those 
> methods, and strictly speaking it's a performance optimization, so I'll 
> rather save that for a separate PR instead of expanding this one's scope. If 
> I adopted using `JavaLangAccess` I might also look into whether I could 
> replace this class' implementation with a `ClassLoaderValue` instead, but 
> again, that's beyond the scope of this PR.

So what do you think of this variant:
https://github.com/plevart/jdk/commit/7af5b81d486d6ee3b7b4c6be90895478fb45868d
...reading of forward/reverse fields in compute can still be volatile while in 
fast path it can be r

Re: RFR: 8198540: Dynalink leaks memory when generating type converters [v4]

[Peter Levart]

On Fri, 8 Jan 2021 08:53:19 GMT, Attila Szegedi  wrote:

>> _(NB: For this leak to occur, an application needs to be either creating and 
>> discarding linkers frequently, or creating and discarding class loaders 
>> frequently while creating Dynalink type converters for classes in them. 
>> Neither is typical usage, although they can occur in dynamic code loading 
>> environments such as OSGi.)_
>> 
>> I'll explain this one in a bit more detail. Dynalink creates and caches 
>> method handles for language-specific conversions between types. Each 
>> conversion is thus characterized by a `Class` object representing the type 
>> converted from, and a `Class` object representing the type converted to, 
>> thus they're keyed by a pair of `(Class, Class)`. Java API provides the 
>> excellent `ClassValue` class for associating values with a single class, but 
>> it lacks the – admittedly more niche – facility for associating a value with 
>> a pair of classes. I originally solved this problem using something that 
>> looks like a `ClassValue, T>>`. I say "looks like" because 
>> Dynalink has a specialized `ClassMap` class that works like `Map, 
>> T>` but it also goes to some pains to _not_ establish strong references from 
>> a class loader to either its children or to unrelated class loaders, for 
>> obvious reasons.
>> 
>> Surprisingly, the problem didn't lie in there, though. The problem was in 
>> the fact that `TypeConverterFactory` used `ClassValue` objects that were its 
>> non-static anonymous inner classes, and further the values they computed 
>> were also of non-static anonymous inner classes. Due to the way `ClassValue` 
>> internals work, this led to the `TypeConverterFactory` objects becoming 
>> anchored in a GC root of `Class.classValueMap` through the synthetic 
>> `this$0` reference chains in said inner classes… talk about non-obvious 
>> memory leaks. (I guess there's a lesson in there about not using 
>> `ClassValue` as an instance field, but there's a genuine need for them here, 
>> so…)
>> 
>> … so the first thing I did was I deconstructed all those anonymous inner 
>> classes into named static inner classes, and ended up with something that 
>> _did_ fix the memory leak (yay!) but at the same time there was a bunch of 
>> copying of constructor parameters from outer classes into the inner classes, 
>> the whole thing was very clunky with scary amounts of boilerplate. I felt 
>> there must exist a better solution.
>> 
>> And it exists! I ended up replacing the `ClassValue>` construct 
>> with a ground-up implementation of `BiClassValue`, representation of 
>> lazily computed values associated with a pair of types. This was the right 
>> abstraction the `TypeConverterFactory` code needed all along. 
>> `BiClassValue` is also not implemented as an abstract class but rather it 
>> is a final class and takes a `BiFunction, Class, T>` for 
>> computation of its values, thus there's never a risk of making it an 
>> instance-specific inner class (well, you still need to be careful with the 
>> bifunction lambda…) It also has an even better solution for avoiding strong 
>> references to child class loaders.
>> 
>> And that's why I'm not submitting here the smallest possible point fix to 
>> the memory leak, but rather a slightly larger one that architecturally fixes 
>> it the right way.
>> 
>> There are two test classes, they test subtly different scenarios. 
>> `TypeConverterFactoryMemoryLeakTest` tests that when a 
>> `TypeConverterFactory` instance becomes unreachable, all method handles it 
>> created also become eligible for collection. 
>> `TypeConverterFactoryRetentionTests` on the other hand test that the factory 
>> itself won't prevent class loaders from being collected by virtue of 
>> creating converters for them.
>
> Attila Szegedi has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - Use immutable maps instead of concurrent ones
>  - Remove classLoader field from BiClassValues and evaluate class loader 
> relationships in a doPrivileged block

src/jdk.dynalink/share/classes/jdk/dynalink/BiClassValue.java line 106:

> 104: 
> 105: private volatile Map, T> forward = Map.of();
> 106: private volatile Map, T> reverse = Map.of();

Since maps are immutable now and allow safe publication via data race (i.e. 
their internal state is composed of final fields), you could relax the 
forward/referse fields to be plain fields. You would just have to leave them 
uninitialized then and check for null at some places if BiClassValues could be 
published via data race since I don't know whether ClassValue guarantees that 
the associated values are published safely or not. If it does, then you could 
pre-initialize the field with empty maps as now, if it does not, then even 
volatile modifiers don't guarantee that unsafely published BiClassValues object 
can only be seen with the fields initialized. i.e. you could read null from 
the

RFR: JDK-8259395 Patching automatic module with additional packages re-creates module without "requires java.base"

[Johannes Kuhn]

Simple fix, complex test case.

Thanks to @AlanBateman for pointing out the right fix.

-

Commit messages:
 - * Minor cleanup.
 - * Fix Whitespace V2
 - * Fix whitespace
 - * Fix JDK-8259395 - test now passes
 - Merge branch 'master' of https://github.com/openjdk/jdk into JDK-8259395
 - * Add failing tests for JDK-8259395

Changes: https://git.openjdk.java.net/jdk/pull/2000/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2000&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8259395
  Stats: 142 lines in 4 files changed: 140 ins; 0 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2000.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2000/head:pull/2000

PR: https://git.openjdk.java.net/jdk/pull/2000

Re: RFR: 8198540: Dynalink leaks memory when generating type converters [v4]

[Peter Levart]

On Fri, 8 Jan 2021 08:53:19 GMT, Attila Szegedi  wrote:

>> _(NB: For this leak to occur, an application needs to be either creating and 
>> discarding linkers frequently, or creating and discarding class loaders 
>> frequently while creating Dynalink type converters for classes in them. 
>> Neither is typical usage, although they can occur in dynamic code loading 
>> environments such as OSGi.)_
>> 
>> I'll explain this one in a bit more detail. Dynalink creates and caches 
>> method handles for language-specific conversions between types. Each 
>> conversion is thus characterized by a `Class` object representing the type 
>> converted from, and a `Class` object representing the type converted to, 
>> thus they're keyed by a pair of `(Class, Class)`. Java API provides the 
>> excellent `ClassValue` class for associating values with a single class, but 
>> it lacks the – admittedly more niche – facility for associating a value with 
>> a pair of classes. I originally solved this problem using something that 
>> looks like a `ClassValue, T>>`. I say "looks like" because 
>> Dynalink has a specialized `ClassMap` class that works like `Map, 
>> T>` but it also goes to some pains to _not_ establish strong references from 
>> a class loader to either its children or to unrelated class loaders, for 
>> obvious reasons.
>> 
>> Surprisingly, the problem didn't lie in there, though. The problem was in 
>> the fact that `TypeConverterFactory` used `ClassValue` objects that were its 
>> non-static anonymous inner classes, and further the values they computed 
>> were also of non-static anonymous inner classes. Due to the way `ClassValue` 
>> internals work, this led to the `TypeConverterFactory` objects becoming 
>> anchored in a GC root of `Class.classValueMap` through the synthetic 
>> `this$0` reference chains in said inner classes… talk about non-obvious 
>> memory leaks. (I guess there's a lesson in there about not using 
>> `ClassValue` as an instance field, but there's a genuine need for them here, 
>> so…)
>> 
>> … so the first thing I did was I deconstructed all those anonymous inner 
>> classes into named static inner classes, and ended up with something that 
>> _did_ fix the memory leak (yay!) but at the same time there was a bunch of 
>> copying of constructor parameters from outer classes into the inner classes, 
>> the whole thing was very clunky with scary amounts of boilerplate. I felt 
>> there must exist a better solution.
>> 
>> And it exists! I ended up replacing the `ClassValue>` construct 
>> with a ground-up implementation of `BiClassValue`, representation of 
>> lazily computed values associated with a pair of types. This was the right 
>> abstraction the `TypeConverterFactory` code needed all along. 
>> `BiClassValue` is also not implemented as an abstract class but rather it 
>> is a final class and takes a `BiFunction, Class, T>` for 
>> computation of its values, thus there's never a risk of making it an 
>> instance-specific inner class (well, you still need to be careful with the 
>> bifunction lambda…) It also has an even better solution for avoiding strong 
>> references to child class loaders.
>> 
>> And that's why I'm not submitting here the smallest possible point fix to 
>> the memory leak, but rather a slightly larger one that architecturally fixes 
>> it the right way.
>> 
>> There are two test classes, they test subtly different scenarios. 
>> `TypeConverterFactoryMemoryLeakTest` tests that when a 
>> `TypeConverterFactory` instance becomes unreachable, all method handles it 
>> created also become eligible for collection. 
>> `TypeConverterFactoryRetentionTests` on the other hand test that the factory 
>> itself won't prevent class loaders from being collected by virtue of 
>> creating converters for them.
>
> Attila Szegedi has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - Use immutable maps instead of concurrent ones
>  - Remove classLoader field from BiClassValues and evaluate class loader 
> relationships in a doPrivileged block

src/jdk.dynalink/share/classes/jdk/dynalink/BiClassValue.java line 126:

> 124: entries.add(Map.entry(c, value));
> 125: @SuppressWarnings("rawtypes")
> 126: final var newEntries = entries.toArray(new 
> Map.Entry[0]);

Since map is immutable now, instead of:

var entries = new ArrayList<>(map.entrySet());
entries.add(Map.entry(c, value));
var newEntries = entries.toArray(new Map.Entry[0]); 

you could write:

var entries = map.entrySet().toArray(new Map.Entry[map.size() + 1]);
entries[map.size()] = Map.entry(c, value);

-

PR: https://git.openjdk.java.net/jdk/pull/1918

Re: RFR: 8193031: Collections.addAll is likely to perform worse than Collection.addAll

[Сергей Цыпанов]

On Thu, 17 Dec 2020 10:36:17 GMT, Rémi Forax 
 wrote:

>> Looks like I've found the original ticket: 
>> https://bugs.openjdk.java.net/browse/JDK-8193031
>
> Apart from the @SuppressWarnings, this looks good to me.
> And i like the irony of this.

@forax, @plevart could I ask for review once again?

-

PR: https://git.openjdk.java.net/jdk/pull/1764

Re: RFR: 8198540: Dynalink leaks memory when generating type converters [v4]

[Attila Szegedi]

> _(NB: For this leak to occur, an application needs to be either creating and 
> discarding linkers frequently, or creating and discarding class loaders 
> frequently while creating Dynalink type converters for classes in them. 
> Neither is typical usage, although they can occur in dynamic code loading 
> environments such as OSGi.)_
> 
> I'll explain this one in a bit more detail. Dynalink creates and caches 
> method handles for language-specific conversions between types. Each 
> conversion is thus characterized by a `Class` object representing the type 
> converted from, and a `Class` object representing the type converted to, thus 
> they're keyed by a pair of `(Class, Class)`. Java API provides the excellent 
> `ClassValue` class for associating values with a single class, but it lacks 
> the – admittedly more niche – facility for associating a value with a pair of 
> classes. I originally solved this problem using something that looks like a 
> `ClassValue, T>>`. I say "looks like" because Dynalink has a 
> specialized `ClassMap` class that works like `Map, T>` but it also 
> goes to some pains to _not_ establish strong references from a class loader 
> to either its children or to unrelated class loaders, for obvious reasons.
> 
> Surprisingly, the problem didn't lie in there, though. The problem was in the 
> fact that `TypeConverterFactory` used `ClassValue` objects that were its 
> non-static anonymous inner classes, and further the values they computed were 
> also of non-static anonymous inner classes. Due to the way `ClassValue` 
> internals work, this led to the `TypeConverterFactory` objects becoming 
> anchored in a GC root of `Class.classValueMap` through the synthetic `this$0` 
> reference chains in said inner classes… talk about non-obvious memory leaks. 
> (I guess there's a lesson in there about not using `ClassValue` as an 
> instance field, but there's a genuine need for them here, so…)
> 
> … so the first thing I did was I deconstructed all those anonymous inner 
> classes into named static inner classes, and ended up with something that 
> _did_ fix the memory leak (yay!) but at the same time there was a bunch of 
> copying of constructor parameters from outer classes into the inner classes, 
> the whole thing was very clunky with scary amounts of boilerplate. I felt 
> there must exist a better solution.
> 
> And it exists! I ended up replacing the `ClassValue>` construct 
> with a ground-up implementation of `BiClassValue`, representation of 
> lazily computed values associated with a pair of types. This was the right 
> abstraction the `TypeConverterFactory` code needed all along. 
> `BiClassValue` is also not implemented as an abstract class but rather it 
> is a final class and takes a `BiFunction, Class, T>` for 
> computation of its values, thus there's never a risk of making it an 
> instance-specific inner class (well, you still need to be careful with the 
> bifunction lambda…) It also has an even better solution for avoiding strong 
> references to child class loaders.
> 
> And that's why I'm not submitting here the smallest possible point fix to the 
> memory leak, but rather a slightly larger one that architecturally fixes it 
> the right way.
> 
> There are two test classes, they test subtly different scenarios. 
> `TypeConverterFactoryMemoryLeakTest` tests that when a `TypeConverterFactory` 
> instance becomes unreachable, all method handles it created also become 
> eligible for collection. `TypeConverterFactoryRetentionTests` on the other 
> hand test that the factory itself won't prevent class loaders from being 
> collected by virtue of creating converters for them.

Attila Szegedi has updated the pull request incrementally with two additional 
commits since the last revision:

 - Use immutable maps instead of concurrent ones
 - Remove classLoader field from BiClassValues and evaluate class loader 
relationships in a doPrivileged block

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1918/files
  - new: https://git.openjdk.java.net/jdk/pull/1918/files/eaddc17b..3f934a21

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1918&range=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1918&range=02-03

  Stats: 92 lines in 1 file changed: 48 ins; 16 del; 28 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1918.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1918/head:pull/1918

PR: https://git.openjdk.java.net/jdk/pull/1918

Re: RFR: 8198540: Dynalink leaks memory when generating type converters [v4]

[Attila Szegedi]

On Mon, 4 Jan 2021 13:31:41 GMT, Peter Levart  wrote:

>> Just a small suggestion using `MethodHandles.empty` instead of 
>> `MethodHandles.constant().asType().dropArguments()`.
>
> Hi Attila,
> 
> This looks good.
> 
> If I understand correctly, your `BiClassValue` is a holder for a `BiFunction` 
> and a `BiClassValueRoot` which is a `ClassValue`. The 
> `BiClassValues` contains two lazily constructed Map(s): `forward` and 
> `reverse`. You then employ a logic where you either use the `forward` map 
> obtained from c1 if c1.classLoader sees c2.classLoader or `backward` map 
> obtained from c2 if c2.classLoader sees c1.classLoader or you don't employ 
> caching if c1.classLoader and c2.classLoader are unrelated.
> The need for two Maps is probably because the two Class components of the 
> "bi-key" are ordered, right? So `BiClassValue bcv = ...;` `bcv.get(c1, c2)` 
> is not the same as `bcv.get(c2, c1)` 
> Alternatively you could have a `BiClassValue` with two embedded 
> `ClassValue`(s):
> 
> final CVRoot forward = new CVRoot<>();
> final CVRoot reverse = new CVRoor<>();
> 
> static class CVRoot extends ClassValue> {
>   public CHMCL get(Class clazz) { return new 
> CHMCL<>(getClassLoader(clazz)); }
> }
> 
> static class CHMCL extends ConcurrentHashMap, T> {
>   final ClassLoader classLoader;
>   CHMCL(ClassLoader cl) { classLoader = cl; }
> }
> 
> ...with that you could get rid of the intermediary BiClassValues object. It 
> would be replaced with a ConcurrentHashMap subclass that would contain a 
> field to hold the cached ClassLoader of the c1/c2. One de-reference less...
> 
> As for the main logic, it would be similar to what you have now. Or it could 
> be different. I wonder what is the performance of canReferenceDirectly(). If 
> you used SharedSecrets to obtain a ClassLoader of a Class without security 
> checks (and thus overhead), you could perhaps evaluate the 
> canReferenceDirectly() before lookups so you would not needlessly trigger 
> initialization of ClassValue(s) that don't need to get initialized. 
> 
> WDYT?

Hey Peter,

thank you for the most thoughtful review! You understood everything right. Your 
approach looks mostly equivalent to mine, with bit of different tradeoffs. I'm 
indeed using `BiClassValues` as an intermediate object; it has two benefits. 
The smaller benefit is that the class loader lookup is performed once per class 
and stored only once per class. The larger benefit is that it defers the 
creation of the CHMs until it has something to put into them. Most of the time, 
only one of either forward or reverse will be created. You're right that the 
same benefit could be had if we checked `canReferenceDirectly` first, although 
I'm trying to make the fast path as fast as possible.

I'm even thinking I could afford to read the class loader on each use of main 
logic when a cached value is not available and not even use a CHM subclass to 
hold it. 
[canReferenceDirectly](https://github.com/openjdk/jdk/blob/master/src/jdk.dynalink/share/classes/jdk/dynalink/internal/InternalTypeUtilities.java#L70)
 is basically equivalent to "isDescendantOf" and can be evaluated fairly 
quickly, unless there's a really long class loader chain. (FWIW, it is also 
equivalent to [`!ClassLoader.needsClassLoaderPermissionCheck(from, 
to)`](https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/lang/ClassLoader.java#L2016)
 but, alas, that's private, and even `ClassLoader.isAncestor` that I could 
similarly use is package-private.

In any case, your suggestion nudged me towards a different rework: 
`BiClassValues` now uses immutable maps instead of concurrent ones, and uses 
those `VarHandles` for `compareAndExchange` on them. In this sense, 
`BiClassValue` is now little more than a pair of atomic references. I decided 
CHM was an overkill here as the map contents stabilize over time; using 
immutable maps feels more natural.

I also realized that `canReferenceDirectly` invocations also need to happen in 
a `doPrivileged` block (Dynalink itself is loaded through the platform class 
loader, so needs these.)

FWIW, your suggestion to use `SharedSecrets` is intriguing - if I could access 
both `ClassLoader.isAncestor` and `ClassLoader.getClassLoader` through 
`JavaLangAccess`, that'd indeed spare me having to go through `doPrivileged` 
blocks. OTOH, there's other places in Dynalink that could benefit from those 
methods, and strictly speaking it's a performance optimization, so I'll rather 
save that for a separate PR instead of expanding this one's scope. If I adopted 
using `JavaLangAccess` I might also look into whether I could replace this 
class' implementation with a `ClassLoaderValue` instead, but again, that's 
beyond the scope of this PR.

-

PR: https://git.openjdk.java.net/jdk/pull/1918