Re: RFR: 8297385: Remove duplicated null typos in javadoc

[Yi Yang]

On Tue, 15 Nov 2022 15:05:45 GMT, Dongxu Wang  wrote:

> 8297385: Remove duplicated null typos in javadoc

good catch, do you need a JBS issue for this?

-

PR: https://git.openjdk.org/jdk/pull/11169

Re: RFR: 8297385: Remove duplicated null typos in javadoc

[Yi Yang]

On Tue, 22 Nov 2022 02:22:20 GMT, Dongxu Wang  wrote:

> > good catch, do you need a JBS issue for this?
> 
> Thank you if you can help with that.

I filed https://bugs.openjdk.org/browse/JDK-8297385 for this, you can change 
your PR title and commit message to [8297385: Remove duplicated null typo in 
javadoc](https://bugs.openjdk.org/browse/JDK-8297385), OpenJDK robot will guide 
you remaining processes.

-

PR: https://git.openjdk.org/jdk/pull/11169

Re: RFR: 8297385: Remove duplicated null typos in javadoc

[Dongxu Wang]

On Mon, 21 Nov 2022 15:16:14 GMT, Yi Yang  wrote:

> good catch, do you need a JBS issue for this?

Thank you if you can help with that.

-

PR: https://git.openjdk.org/jdk/pull/11169

RFR: 8297385: Remove duplicated null typos in javadoc

[Dongxu Wang]

8297385: Remove duplicated null typos in javadoc

-

Commit messages:
 - Minor remove duplicate null typo

Changes: https://git.openjdk.org/jdk/pull/11169/files
 Webrev: https://webrevs.openjdk.org/?repo=jdk=11169=00
  Issue: https://bugs.openjdk.org/browse/JDK-8297385
  Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod
  Patch: https://git.openjdk.org/jdk/pull/11169.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/11169/head:pull/11169

PR: https://git.openjdk.org/jdk/pull/11169

Integrated: JDK-8297299 SequenceInputStream should not use Vector

[Markus KARG]

On Sat, 19 Nov 2022 21:36:56 GMT, Markus KARG  wrote:

> There is no need to use a temporary Vector within the constructor of 
> SynchronizedInputStream, as more efficient (non-synchronized) alternative 
> code (like List.of) will do the same in possibly less time. While the 
> optimization is not dramatic, it still makes sense to replace Vector unless 
> synchronization is really needed.

This pull request has now been integrated.

Changeset: 06968548
Author:Markus Karg 
Committer: Jaikiran Pai 
URL:   
https://git.openjdk.org/jdk/commit/069685489afcea9b31491f0d9fec8cc52e210e99
Stats: 11 lines in 1 file changed: 2 ins; 5 del; 4 mod

8297299: SequenceInputStream should not use Vector

Reviewed-by: alanb, jpai

-

PR: https://git.openjdk.org/jdk/pull/11249

Integrated: 8297301: Cleanup unused methods in JavaUtilJarAccess

[pandaapo]

On Thu, 10 Nov 2022 01:02:00 GMT, pandaapo  wrote:

> The cache named `signerToCodeSource` in `JarVerifier` is never used now.

This pull request has now been integrated.

Changeset: f0e99c63
Author:pandaapo <1052156...@qq.com>
Committer: Weijun Wang 
URL:   
https://git.openjdk.org/jdk/commit/f0e99c634693fafc0c5d1103184e73c6669629db
Stats: 652 lines in 4 files changed: 3 ins; 636 del; 13 mod

8297301: Cleanup unused methods in JavaUtilJarAccess

Reviewed-by: weijun

-

PR: https://git.openjdk.org/jdk/pull/11072

Re: RFR: 8297301: Cleanup unused methods in JavaUtilJarAccess [v7]

[pandaapo]

On Tue, 22 Nov 2022 01:44:29 GMT, Weijun Wang  wrote:

> Everything looks fine now. Thanks so much for the cleanup. It's always nice 
> to see unused lines removed.

Thank you for your review.

-

PR: https://git.openjdk.org/jdk/pull/11072

Re: RFR: 8297301: Cleanup unused methods in JavaUtilJarAccess [v7]

[Weijun Wang]

On Tue, 22 Nov 2022 00:45:00 GMT, pandaapo  wrote:

>> The cache named `signerToCodeSource` in `JarVerifier` is never used now.
>
> pandaapo has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Modify as reviews.

Everything looks fine now. Thanks so much for the cleanup. It's always nice to 
see unused lines removed.

-

Marked as reviewed by weijun (Reviewer).

PR: https://git.openjdk.org/jdk/pull/11072

Re: RFR: JDK-8297299 SequenceInputStream should not use Vector [v6]

[Jaikiran Pai]

On Mon, 21 Nov 2022 17:43:23 GMT, Markus KARG  wrote:

>> There is no need to use a temporary Vector within the constructor of 
>> SynchronizedInputStream, as more efficient (non-synchronized) alternative 
>> code (like List.of) will do the same in possibly less time. While the 
>> optimization is not dramatic, it still makes sense to replace Vector unless 
>> synchronization is really needed.
>
> Markus KARG has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   private member variables

Thank you Markus for these changes. The latest state in `8d298318` looks good 
to me. I'll run some tests today before sponsoring this.

-

Marked as reviewed by jpai (Reviewer).

PR: https://git.openjdk.org/jdk/pull/11249

Re: RFR: 8296477: Foreign linker implementation update following JEP 434 [v7]

[Jorn Vernee]

On Fri, 18 Nov 2022 14:54:52 GMT, Jorn Vernee  wrote:

>> Pull in linker implementation changes, that include non-trivial changes to 
>> VM code, from the panama-foreign repo into the main JDK.
>> 
>> This is split off from the main JEP integration to make reviewing easier.
>> 
>> This includes the following patches:
>> 
>> 1. https://github.com/openjdk/panama-foreign/pull/698
>> 2. https://github.com/openjdk/panama-foreign/pull/699
>> 3. (part of) https://github.com/openjdk/panama-foreign/pull/731
>> 4. https://github.com/openjdk/panama-foreign/pull/740
>> 5. https://github.com/openjdk/panama-foreign/pull/746
>> 6. https://github.com/openjdk/panama-foreign/pull/742
>> 7. https://github.com/openjdk/panama-foreign/pull/743
>> 
>> Probably the biggest change to the code comes from replacing `VMReg` - which 
>> can not represent offsets into the stack that are not a multiple of the VM's 
>> stack slot size (32-bits) - with the new `VMStorage` class, which can 
>> describe byte offsets into the stack, as well as having a register mask to 
>> indicate only certain register segments.
>> 
>> The only part of 3. that is in this PR is the part that turns the 
>> `VMStorage` class in Java into a record.
>> 
>> Please refer to the PR of each individual patch for a more detailed 
>> description.
>
> Jorn Vernee has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   8296973: saving errno on a value-returning function crashes the JVM
>   
>   Reviewed-by: mcimadamore

Thanks for the review, I've dropped the `.inline` from the header file names.

-

PR: https://git.openjdk.org/jdk/pull/11019

Re: RFR: 8296477: Foreign linker implementation update following JEP 434 [v8]

[Jorn Vernee]

> Pull in linker implementation changes, that include non-trivial changes to VM 
> code, from the panama-foreign repo into the main JDK.
> 
> This is split off from the main JEP integration to make reviewing easier.
> 
> This includes the following patches:
> 
> 1. https://github.com/openjdk/panama-foreign/pull/698
> 2. https://github.com/openjdk/panama-foreign/pull/699
> 3. (part of) https://github.com/openjdk/panama-foreign/pull/731
> 4. https://github.com/openjdk/panama-foreign/pull/740
> 5. https://github.com/openjdk/panama-foreign/pull/746
> 6. https://github.com/openjdk/panama-foreign/pull/742
> 7. https://github.com/openjdk/panama-foreign/pull/743
> 
> Probably the biggest change to the code comes from replacing `VMReg` - which 
> can not represent offsets into the stack that are not a multiple of the VM's 
> stack slot size (32-bits) - with the new `VMStorage` class, which can 
> describe byte offsets into the stack, as well as having a register mask to 
> indicate only certain register segments.
> 
> The only part of 3. that is in this PR is the part that turns the `VMStorage` 
> class in Java into a record.
> 
> Please refer to the PR of each individual patch for a more detailed 
> description.

Jorn Vernee has updated the pull request incrementally with one additional 
commit since the last revision:

  drop .inline from vmstorage header names

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/11019/files
  - new: https://git.openjdk.org/jdk/pull/11019/files/0fa0e8cf..03be64c9

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=11019=07
 - incr: https://webrevs.openjdk.org/?repo=jdk=11019=06-07

  Stats: 7 lines in 11 files changed: 0 ins; 0 del; 7 mod
  Patch: https://git.openjdk.org/jdk/pull/11019.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/11019/head:pull/11019

PR: https://git.openjdk.org/jdk/pull/11019

Re: RFR: 8296477: Foreign linker implementation update following JEP 434 [v7]

[Vladimir Ivanov]

On Fri, 18 Nov 2022 14:54:52 GMT, Jorn Vernee  wrote:

>> Pull in linker implementation changes, that include non-trivial changes to 
>> VM code, from the panama-foreign repo into the main JDK.
>> 
>> This is split off from the main JEP integration to make reviewing easier.
>> 
>> This includes the following patches:
>> 
>> 1. https://github.com/openjdk/panama-foreign/pull/698
>> 2. https://github.com/openjdk/panama-foreign/pull/699
>> 3. (part of) https://github.com/openjdk/panama-foreign/pull/731
>> 4. https://github.com/openjdk/panama-foreign/pull/740
>> 5. https://github.com/openjdk/panama-foreign/pull/746
>> 6. https://github.com/openjdk/panama-foreign/pull/742
>> 7. https://github.com/openjdk/panama-foreign/pull/743
>> 
>> Probably the biggest change to the code comes from replacing `VMReg` - which 
>> can not represent offsets into the stack that are not a multiple of the VM's 
>> stack slot size (32-bits) - with the new `VMStorage` class, which can 
>> describe byte offsets into the stack, as well as having a register mask to 
>> indicate only certain register segments.
>> 
>> The only part of 3. that is in this PR is the part that turns the 
>> `VMStorage` class in Java into a record.
>> 
>> Please refer to the PR of each individual patch for a more detailed 
>> description.
>
> Jorn Vernee has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   8296973: saving errno on a value-returning function crashes the JVM
>   
>   Reviewed-by: mcimadamore

Unless I'm missing something important, 
`vmstorage.inline.hpp`/`vmstorage_.inline.hpp` should be named 
`vmstorage.hpp`/`vmstorage_.hpp`.

Otherwise, looks good.

-

Marked as reviewed by vlivanov (Reviewer).

PR: https://git.openjdk.org/jdk/pull/11019

Re: RFR: 8295857: Clarify that cleanup code can be skipped when the JVM terminates (e.g. when calling halt()) [v4]

[Brent Christian]

> [JDK-8290036](https://bugs.openjdk.org/browse/JDK-8290036) documented the 
> shutdown sequence, noting that calling Runtime.halt() skips the shutdown 
> sequence and immediately terminates the VM. Thus, "threads' current methods 
> do not complete normally or abruptly; no finally clause of any method is 
> executed".
> 
> One ramification of this is that resources within try-with-resource blocks 
> will not be released. It would be good to state this explicitly.

Brent Christian has updated the pull request incrementally with one additional 
commit since the last revision:

  Update Runtime class doc re: other unexpected behaviors

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/11218/files
  - new: https://git.openjdk.org/jdk/pull/11218/files/cbba781f..6df8acf8

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=11218=03
 - incr: https://webrevs.openjdk.org/?repo=jdk=11218=02-03

  Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod
  Patch: https://git.openjdk.org/jdk/pull/11218.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/11218/head:pull/11218

PR: https://git.openjdk.org/jdk/pull/11218

Re: RFR: 8297301: Cleanup unused methods in JavaUtilJarAccess [v7]

[pandaapo]

> The cache named `signerToCodeSource` in `JarVerifier` is never used now.

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

  Modify as reviews.

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/11072/files
  - new: https://git.openjdk.org/jdk/pull/11072/files/fb933804..1aa65b2b

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=11072=06
 - incr: https://webrevs.openjdk.org/?repo=jdk=11072=05-06

  Stats: 10 lines in 1 file changed: 4 ins; 5 del; 1 mod
  Patch: https://git.openjdk.org/jdk/pull/11072.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/11072/head:pull/11072

PR: https://git.openjdk.org/jdk/pull/11072

Re: RFR: 8282664: Unroll by hand StringUTF16 and StringLatin1 polynomial hash loops [v13]

[Sandhya Viswanathan]

On Fri, 11 Nov 2022 13:00:06 GMT, Claes Redestad  wrote:

>> Continuing the work initiated by @luhenry to unroll and then intrinsify 
>> polynomial hash loops.
>> 
>> I've rewired the library changes to route via a single `@IntrinsicCandidate` 
>> method. To make this work I've harmonized how they are invoked so that 
>> there's less special handling and checks in the intrinsic. Mainly do the 
>> null-check outside of the intrinsic for `Arrays.hashCode` cases.
>> 
>> Having a centralized entry point means it'll be easier to parameterize the 
>> factor and start values which are now hard-coded (always 31, and a start 
>> value of either one for `Arrays` or zero for `String`). It seems somewhat 
>> premature to parameterize this up front.
>> 
>> The current implementation is performance neutral on microbenchmarks on all 
>> tested platforms (x64, aarch64) when not enabling the intrinsic. We do add a 
>> few trivial method calls which increase the call stack depth, so surprises 
>> cannot be ruled out on complex workloads.
>> 
>> With the most recent fixes the x64 intrinsic results on my workstation look 
>> like this:
>> 
>> Benchmark   (size)  Mode  Cnt ScoreError 
>>  Units
>> StringHashCode.Algorithm.defaultLatin1   1  avgt5 2.199 ±  0.017 
>>  ns/op
>> StringHashCode.Algorithm.defaultLatin1  10  avgt5 6.933 ±  0.049 
>>  ns/op
>> StringHashCode.Algorithm.defaultLatin1 100  avgt529.935 ±  0.221 
>>  ns/op
>> StringHashCode.Algorithm.defaultLatin1   1  avgt5  1596.982 ±  7.020 
>>  ns/op
>> 
>> Baseline:
>> 
>> Benchmark   (size)  Mode  Cnt ScoreError 
>>  Units
>> StringHashCode.Algorithm.defaultLatin1   1  avgt5 2.200 ±  0.013 
>>  ns/op
>> StringHashCode.Algorithm.defaultLatin1  10  avgt5 9.424 ±  0.122 
>>  ns/op
>> StringHashCode.Algorithm.defaultLatin1 100  avgt590.541 ±  0.512 
>>  ns/op
>> StringHashCode.Algorithm.defaultLatin1   1  avgt5  9425.321 ± 67.630 
>>  ns/op
>> 
>> I.e. no measurable overhead compared to baseline even for `size == 1`.
>> 
>> The vectorized code now nominally works for all unsigned cases as well as 
>> ints, though more testing would be good.
>> 
>> Benchmark for `Arrays.hashCode`:
>> 
>> Benchmark  (size)  Mode  Cnt ScoreError  Units
>> ArraysHashCode.bytes1  avgt5 1.884 ±  0.013  ns/op
>> ArraysHashCode.bytes   10  avgt5 6.955 ±  0.040  ns/op
>> ArraysHashCode.bytes  100  avgt587.218 ±  0.595  ns/op
>> ArraysHashCode.bytes1  avgt5  9419.591 ± 38.308  ns/op
>> ArraysHashCode.chars1  avgt5 2.200 ±  0.010  ns/op
>> ArraysHashCode.chars   10  avgt5 6.935 ±  0.034  ns/op
>> ArraysHashCode.chars  100  avgt530.216 ±  0.134  ns/op
>> ArraysHashCode.chars1  avgt5  1601.629 ±  6.418  ns/op
>> ArraysHashCode.ints 1  avgt5 2.200 ±  0.007  ns/op
>> ArraysHashCode.ints10  avgt5 6.936 ±  0.034  ns/op
>> ArraysHashCode.ints   100  avgt529.412 ±  0.268  ns/op
>> ArraysHashCode.ints 1  avgt5  1610.578 ±  7.785  ns/op
>> ArraysHashCode.shorts   1  avgt5 1.885 ±  0.012  ns/op
>> ArraysHashCode.shorts  10  avgt5 6.961 ±  0.034  ns/op
>> ArraysHashCode.shorts 100  avgt587.095 ±  0.417  ns/op
>> ArraysHashCode.shorts   1  avgt5  9420.617 ± 50.089  ns/op
>> 
>> Baseline:
>> 
>> Benchmark  (size)  Mode  Cnt ScoreError  Units
>> ArraysHashCode.bytes1  avgt5 3.213 ±  0.207  ns/op
>> ArraysHashCode.bytes   10  avgt5 8.483 ±  0.040  ns/op
>> ArraysHashCode.bytes  100  avgt590.315 ±  0.655  ns/op
>> ArraysHashCode.bytes1  avgt5  9422.094 ± 62.402  ns/op
>> ArraysHashCode.chars1  avgt5 3.040 ±  0.066  ns/op
>> ArraysHashCode.chars   10  avgt5 8.497 ±  0.074  ns/op
>> ArraysHashCode.chars  100  avgt590.074 ±  0.387  ns/op
>> ArraysHashCode.chars1  avgt5  9420.474 ± 41.619  ns/op
>> ArraysHashCode.ints 1  avgt5 2.827 ±  0.019  ns/op
>> ArraysHashCode.ints10  avgt5 7.727 ±  0.043  ns/op
>> ArraysHashCode.ints   100  avgt589.405 ±  0.593  ns/op
>> ArraysHashCode.ints 1  avgt5  9426.539 ± 51.308  ns/op
>> ArraysHashCode.shorts   1  avgt5 3.071 ±  0.062  ns/op
>> ArraysHashCode.shorts  10  avgt5 8.168 ±  0.049  ns/op
>> ArraysHashCode.shorts 100  avgt590.399 ±  0.292  ns/op
>> ArraysHashCode.shorts   1  avgt5  9420.171 ± 44.474  ns/op
>> 
>> 
>> As we can see the `Arrays` intrinsics are faster for small inputs, and 
>> faster on large inputs for `char` and `int` (the ones currently vectorized). 
>> I aim to fix `byte` and `short` cases before integrating, though it might be 
>> acceptable to hand that off as 

Integrated: JDK-8297164: Update troff man pages and CheckManPageOptions.java

[Jonathan Gibbons]

On Thu, 17 Nov 2022 22:23:53 GMT, Jonathan Gibbons  wrote:

> Please review an update for the troff man pages, following the recent update 
> to upgrade to use pandoc 2.19.2
> (See https://bugs.openjdk.org/browse/JDK-8297165)
> 
> In conjunction with this, one javadoc test also needs to be updated, to work 
> with the new form of output generated by the new version of pandoc.

This pull request has now been integrated.

Changeset: 5a45c251
Author:Jonathan Gibbons 
URL:   
https://git.openjdk.org/jdk/commit/5a45c25151b1da8e329ea2be21a0e4d2652f8b4a
Stats: 8963 lines in 29 files changed: 471 ins; 1587 del; 6905 mod

8297164: Update troff man pages and CheckManPageOptions.java

Reviewed-by: dholmes

-

PR: https://git.openjdk.org/jdk/pull/11223

Re: RFR: 8236919: Refactor com.sun.tools.javac.main.CommandLine into a reusable module for other JDK tools

[Jonathan Gibbons]

On Mon, 21 Nov 2022 15:40:19 GMT, Christian Stein  wrote:

> This PR copies the `CommandLine.java` file from module `jdk.compiler` 
> (package `com.sun.tools.javac.main`) into the `jdk.internal.opt` module, 
> creating a new package with name `jdk.internal.opt`. That new 
> `jdk.internal.opt` package is then exported to the following modules:
> - `jdk.jartool`
> - `jdk.jlink`
> - `jdk.jpackage`
> 
> Now, `jar`, `jlink`, and `jpackage` use a shared `CommandLine` class. In a 
> future commit (presumable for JDK 21)  the original `CommandLine.java` file 
> in `jdk.compiler` can and will be replaced with this new one in 
> `jdk.internal.opt`. Same goes for the `jdk.javadoc` module.
> 
> - [x] Keep `CommandLine.java` in `jdk.compiler` module for the time being due 
> to "JDK N-1 rule".
> - [x] Keep `CommandLine.java` in `jdk.javadoc` module for the time being due 
> to "JDK N-1 rule".
> - [x] Remove `CommandLine.java` from `jdk.jartool` module
> - [x] Remove `CommandLine.java` from `jdk.jlink` module
> - [x] Remove `CommandLine.java` from `jdk.jpackage` module
> - [x] Check for related but renamed(?) usages of `CommandLine.java` in other 
> JDK tools: `jshell`, `jdeps`, `jfr`, ...

I guess git is "confused" by not realizing that the "new" `CommandLine.java` is 
just a clone of the existing `javac` version. (It is, isn't it ... apart from 
the package rename?)

src/jdk.internal.opt/share/classes/jdk/internal/opt/CommandLine.java line 51:

> 49:  *  If you write code that depends on this, you do so at your own risk.
> 50:  *  This code and its internal interfaces are subject to change or
> 51:  *  deletion without notice.

Maybe we can drop this part of the comment

-

Marked as reviewed by jjg (Reviewer).

PR: https://git.openjdk.org/jdk/pull/11272

Re: RFR: 8295044: Implementation of Foreign Function and Memory API (Second Preview) [v28]

[Maurizio Cimadamore]

> This PR contains the API and implementation changes for JEP-434 [1]. A more 
> detailed description of such changes, to avoid repetitions during the review 
> process, is included as a separate comment.
> 
> [1] - https://openjdk.org/jeps/434

Maurizio Cimadamore has updated the pull request incrementally with three 
additional commits since the last revision:

 - Address more review comments
 - Fix bad @throws in MemorySegment::copy methods
 - Address review comments

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/10872/files
  - new: https://git.openjdk.org/jdk/pull/10872/files/876587c3..a0cee7b0

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=10872=27
 - incr: https://webrevs.openjdk.org/?repo=jdk=10872=26-27

  Stats: 19 lines in 4 files changed: 8 ins; 0 del; 11 mod
  Patch: https://git.openjdk.org/jdk/pull/10872.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/10872/head:pull/10872

PR: https://git.openjdk.org/jdk/pull/10872

RFR: 8236919: Refactor com.sun.tools.javac.main.CommandLine into a reusable module for other JDK tools

[Christian Stein]

This PR copies the `CommandLine.java` file from module `jdk.compiler` (package 
`com.sun.tools.javac.main`) into the `jdk.internal.opt` module, creating a new 
package with name `jdk.internal.opt`. That new `jdk.internal.opt` package is 
then exported to the following modules:
- `jdk.jartool`
- `jdk.jlink`
- `jdk.jpackage`

Now, `jar`, `jlink`, and `jpackage` use a shared `CommandLine` class. In a 
future commit (presumable for JDK 21)  the original `CommandLine.java` file in 
`jdk.compiler` can and will be replaced with this new one in 
`jdk.internal.opt`. Same goes for the `jdk.javadoc` module.

- [x] Keep `CommandLine.java` in `jdk.compiler` module for the time being due 
to "JDK N-1 rule".
- [x] Keep `CommandLine.java` in `jdk.javadoc` module for the time being due to 
"JDK N-1 rule".
- [x] Remove `CommandLine.java` from `jdk.jartool` module
- [x] Remove `CommandLine.java` from `jdk.jlink` module
- [x] Remove `CommandLine.java` from `jdk.jpackage` module
- [x] Check for related but renamed(?) usages of `CommandLine.java` in other 
JDK tools: `jshell`, `jdeps`, `jfr`, ...

-

Commit messages:
 - 8236919: Delete superseded `CommandLine` classes
 - 8236919: Refactor com.sun.tools.javac.main.CommandLine into a reusable 
module for other JDK tools

Changes: https://git.openjdk.org/jdk/pull/11272/files
 Webrev: https://webrevs.openjdk.org/?repo=jdk=11272=00
  Issue: https://bugs.openjdk.org/browse/JDK-8236919
  Stats: 514 lines in 10 files changed: 35 ins; 472 del; 7 mod
  Patch: https://git.openjdk.org/jdk/pull/11272.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/11272/head:pull/11272

PR: https://git.openjdk.org/jdk/pull/11272

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v4]

[Alan Bateman]

On Mon, 21 Nov 2022 17:00:52 GMT, Maurizio Cimadamore  
wrote:

>>> Although this looks much simpler and concise, it means "a new object is 
>>> created for each invocation" 
>> 
>> My comment was actually to see if DirectBuffer could extend AutoCloseable so 
>> that the acquire returns "this" for the non-session case.  Doing the 
>> equivalent for the session case might leak into MemorySessionImpl 
>> implementing DirectBuffer which you probably don't want to do. If 
>> NOP_CLOSE.close can do the Reference.reachabilityFence(this) then it would 
>> at least improve some of the use-sites.
>
>> > Although this looks much simpler and concise, it means "a new object is 
>> > created for each invocation"
>> 
>> My comment was actually to see if DirectBuffer could extend AutoCloseable so 
>> that the acquire returns "this" for the non-session case. Doing the 
>> equivalent for the session case might leak into MemorySessionImpl 
>> implementing DirectBuffer which you probably don't want to do. If 
>> NOP_CLOSE.close can do the Reference.reachabilityFence(this) then it would 
>> at least improve some of the use-sites.
> 
> Not sure that is simpler. ByteBuffer <: AutoCloseable doesn't seem to make 
> sense to me. I'm also not sure how much object allocation (all this stuff 
> will become value types) should be the driving factor in these code paths.

Right, I didn't mean BB <: AC but to see if we avoid needing to wrap it because 
this PR is touching several low level and performance critical code paths. For 
the faraway places then having the close do a Reference.reachabilityFence(this) 
should avoid the surprise that the buffer has to kept alive even though it 
appears that the try-with-resources is doing it already.

-

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: JDK-8297299 SequenceInputStream should not use Vector [v2]

[Jens Lidestrom]

On Mon, 21 Nov 2022 17:43:58 GMT, Markus KARG  wrote:

> I think the public visibility of my Twitter account is not wide enough to get 
> really robust answers, unfortunately.

One alternative is to search GitHub. It's amazing how fast they can search such 
a huge code corpus.

Example: 
https://github.com/search?l==%22new+SequenceInputStream%22+-filename%3ASequenceInputStreamTest.java=code

One problem with GitHub search is that often you get a lot of duplicate 
matches. In the example above I have filtered out 
`SequenceInputStreamTest.java` which showed up a lot.

-

PR: https://git.openjdk.org/jdk/pull/11249

Re: RFR: JDK-8297164: Update troff man pages and CheckManPageOptions.java [v2]

[Daniel Fuchs]

On Mon, 21 Nov 2022 17:54:15 GMT, Jonathan Gibbons  wrote:

>> Please review an update for the troff man pages, following the recent update 
>> to upgrade to use pandoc 2.19.2
>> (See https://bugs.openjdk.org/browse/JDK-8297165)
>> 
>> In conjunction with this, one javadoc test also needs to be updated, to work 
>> with the new form of output generated by the new version of pandoc.
>
> Jonathan Gibbons has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now contains three commits:
> 
>  - Fix merge issue
>  - Merge with upstream/master
>  - JDK-8297164: Update troff man pages and CheckManPageOptions.java

The diffs are a bit difficult to read but I didn't spot anything obviously 
wrong with jwebserver. I could see that the new compress option was there in 
jmod man page too. +1.

-

PR: https://git.openjdk.org/jdk/pull/11223

Re: RFR: JDK-8297164: Update troff man pages and CheckManPageOptions.java [v2]

[Jonathan Gibbons]

> Please review an update for the troff man pages, following the recent update 
> to upgrade to use pandoc 2.19.2
> (See https://bugs.openjdk.org/browse/JDK-8297165)
> 
> In conjunction with this, one javadoc test also needs to be updated, to work 
> with the new form of output generated by the new version of pandoc.

Jonathan Gibbons has updated the pull request with a new target base due to a 
merge or a rebase. The pull request now contains three commits:

 - Fix merge issue
 - Merge with upstream/master
 - JDK-8297164: Update troff man pages and CheckManPageOptions.java

-

Changes: https://git.openjdk.org/jdk/pull/11223/files
 Webrev: https://webrevs.openjdk.org/?repo=jdk=11223=01
  Stats: 8963 lines in 29 files changed: 471 ins; 1587 del; 6905 mod
  Patch: https://git.openjdk.org/jdk/pull/11223.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/11223/head:pull/11223

PR: https://git.openjdk.org/jdk/pull/11223

Re: RFR: JDK-8297299 SequenceInputStream should not use Vector [v2]

[Markus KARG]

On Mon, 21 Nov 2022 07:42:35 GMT, Alan Bateman  wrote:

>> N.B.: Regarding usage numbers, I will do a quick poll on Twitter.
>
>> Indeed my intention solely is to get rid of `Vector`, so I am fine with 
>> keeping a low profile and being full backwards compatible. I assume SIS is 
>> seldomly used, so opening a can of warms is not worth it (I think).
> 
> If you have the cycles then collecting data on usages and finding examples 
> where the constructor is called with a second parameter of null would be 
> useful. It's a lot of work to do that of course. The changes in update 
> 2e957354 look okay, I can't immediately see any behavior change with that 
> version.

I think the public visibility of my Twitter account is not wide enough to get 
*really robust* answers, unfortunately. So far, 92% answered that they not even 
used SIS in their whole life. So the users of two-args constructor must be 
really neglectable. Nevertheless, they are definitively not zero, so all we 
could do is marking it deprecated some day.

Anyways, thanks for the review. At least we get rid of `Vector` here. Would be 
happy if you mark the PR as reviewed. :-)

-

PR: https://git.openjdk.org/jdk/pull/11249

Re: RFR: JDK-8297299 SequenceInputStream should not use Vector [v5]

[Markus KARG]

On Mon, 21 Nov 2022 06:31:47 GMT, Jaikiran Pai  wrote:

>> Markus KARG has refreshed the contents of this pull request, and previous 
>> commits have been removed. The incremental views will show differences 
>> compared to the previous content of the PR. The pull request contains one 
>> new commit since the last revision:
>> 
>>   Jens' Proposal
>
> Thank you Markus for the changes. The latest version in `2e957354` looks fine 
> to me. While we are at it, would you be willing to change the member 
> variables `e` to `private final` and the `in` to `private`? From what I can 
> see, I don't see any other class accessing these package private fields.

@jaikiran Kindly requesting your review approval. :-)

-

PR: https://git.openjdk.org/jdk/pull/11249

Re: RFR: JDK-8297299 SequenceInputStream should not use Vector [v5]

[Markus KARG]

On Mon, 21 Nov 2022 07:48:01 GMT, Markus KARG  wrote:

> While we are at it, would you be willing to change the member variables `e` 
> to `private final` and the `in` to `private`? From what I can see, I don't 
> see any other class accessing these package private fields.

Fixed in 8d29831 :-)

-

PR: https://git.openjdk.org/jdk/pull/11249

Re: RFR: JDK-8297299 SequenceInputStream should not use Vector [v6]

[Alan Bateman]

On Mon, 21 Nov 2022 17:39:24 GMT, Markus KARG  wrote:

>> There is no need to use a temporary Vector within the constructor of 
>> SynchronizedInputStream, as more efficient (non-synchronized) alternative 
>> code (like List.of) will do the same in possibly less time. While the 
>> optimization is not dramatic, it still makes sense to replace Vector unless 
>> synchronization is really needed.
>
> Markus KARG has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   private member variables

Marked as reviewed by alanb (Reviewer).

-

PR: https://git.openjdk.org/jdk/pull/11249

Re: RFR: JDK-8297299 SequenceInputStream should not use Vector [v6]

[Markus KARG]

> There is no need to use a temporary Vector within the constructor of 
> SynchronizedInputStream, as more efficient (non-synchronized) alternative 
> code (like List.of) will do the same in possibly less time. While the 
> optimization is not dramatic, it still makes sense to replace Vector unless 
> synchronization is really needed.

Markus KARG has updated the pull request incrementally with one additional 
commit since the last revision:

  private member variables

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/11249/files
  - new: https://git.openjdk.org/jdk/pull/11249/files/2e957354..8d298318

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=11249=05
 - incr: https://webrevs.openjdk.org/?repo=jdk=11249=04-05

  Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod
  Patch: https://git.openjdk.org/jdk/pull/11249.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/11249/head:pull/11249

PR: https://git.openjdk.org/jdk/pull/11249

Re: RFR: JDK-8285932 Implementation of JEP 430 String Templates (Preview) [v26]

[Jim Laskey]

On Mon, 21 Nov 2022 13:38:58 GMT, Claes Redestad  wrote:

>> Jim Laskey has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Typo
>
> src/java.base/share/classes/java/util/Digits.java line 39:
> 
>> 37:  */
>> 38: @PreviewFeature(feature=PreviewFeature.Feature.STRING_TEMPLATES)
>> 39: interface Digits {
> 
> Should this be modeled as an `abstract sealed` class hierarchy instead?

Changing

-

PR: https://git.openjdk.org/jdk/pull/10889

Re: RFR: JDK-8285932 Implementation of JEP 430 String Templates (Preview) [v28]

[Jim Laskey]

> Enhance the Java programming language with string templates, which are 
> similar to string literals but contain embedded expressions. A string 
> template is interpreted at run time by replacing each expression with the 
> result of evaluating that expression, possibly after further validation and 
> transformation. This is a [preview language feature and 
> API](http://openjdk.java.net/jeps/12).

Jim Laskey has updated the pull request incrementally with one additional 
commit since the last revision:

  Seal Digits

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/10889/files
  - new: https://git.openjdk.org/jdk/pull/10889/files/f2562ab3..da3ea20d

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=10889=27
 - incr: https://webrevs.openjdk.org/?repo=jdk=10889=26-27

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.org/jdk/pull/10889.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/10889/head:pull/10889

PR: https://git.openjdk.org/jdk/pull/10889

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v4]

[Maurizio Cimadamore]

On Mon, 21 Nov 2022 16:34:46 GMT, Alan Bateman  wrote:

> > Although this looks much simpler and concise, it means "a new object is 
> > created for each invocation"
> 
> My comment was actually to see if DirectBuffer could extend AutoCloseable so 
> that the acquire returns "this" for the non-session case. Doing the 
> equivalent for the session case might leak into MemorySessionImpl 
> implementing DirectBuffer which you probably don't want to do. If 
> NOP_CLOSE.close can do the Reference.reachabilityFence(this) then it would at 
> least improve some of the use-sites.

Not sure that is simpler. ByteBuffer <: AutoCloseable doesn't seem to make 
sense to me. I'm also not sure how much object allocation (all this stuff will 
become value types) should be the driving factor in these code paths.

-

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8292914: Introduce a system property that enables stable names for lambda proxy classes [v7]

[Strahinja Stanojevic]

> This PR introduces an option to output stable names for the lambda classes in 
> the JDK. A stable name consists of two parts: The first part is the 
> predefined value `$$Lambda$` appended to the lambda capturing class, and the 
> second is a 64-bit hash part of the name. Thus, it looks like 
> `lambdaCapturingClass$$Lambda$hashValue`.
> Parameters used to create a stable hash are a superset of the parameters used 
> for lambda class archiving when the CDS dumping option is enabled. During 
> this process, all the mutual parameters are in the same form as they are in 
> the low-level implementation 
> (`SystemDictionaryShared::add_lambda_proxy_class`) of the archiving process.
> We decided to use a well-specified `CRC32` algorithm from the standard Java 
> library. We created two 32-bit hashes from the parameters used to create 
> stable names. Then, we combined those two 32-bit hashes into one 64-bit hash 
> value.
> We chose `CRC32` because it is a well-specified hash function, and we don't 
> need to write additional code in the JDK. `SHA-256, MD5`, and all other hash 
> functions that rely on `MessageDigest` use lambdas in the implementation, so 
> they are unsuitable for our purpose. We also considered a few different hash 
> functions with a low collision rate. All these functions would require at 
> least 100 lines of additional code in the JDK. The best alternative we found 
> is 64-bit` MurmurHash2`: 
> https://commons.apache.org/proper/commons-codec/jacoco/org.apache.commons.codec.digest/MurmurHash2.java.html.
>   In case adding a new hash implementation (e.g., Murmur2) to the JDK is 
> preferred, this PR can be easily modified.
> We found the post 
> (https://softwareengineering.stackexchange.com/questions/49550/which-hashing-algorithm-is-best-for-uniqueness-and-speed/145633#145633)
>  that compares different hash functions.
> We also tested the `CRC32` hash function against half a billion generated 
> strings, and there were no collisions. Note that the capturing-class name is 
> also part of the lambda class name, so the potential collisions can only 
> appear in a single class. Thus, we do not expect to have name collisions due 
> to a relatively low number of lambdas per class. Every tool that uses this 
> feature should handle potential collisions on its own.  
> We found an overall approximation of the collision rate too. You can find it 
> here: https://preshing.com/20110504/hash-collision-probabilities/.
> 
> JDK currently adds an atomic integer after `$$Lambda$`, and the names of the 
> lambdas depend on the creation order. In the `TestStableLambdaNames`, we 
> generate all the lambdas two times. In the first run, the method 
> createPlainLambdas generate the following lambdas:
> 
> - TestStableLambdaNames$$Lambda$1/0x000800c00400
> - TestStableLambdaNames$$Lambda$2/0x000800c01800
> - TestStableLambdaNames$$Lambda$3/0x000800c01a38
> The same method in the second run generates lambdas with different names:
> - TestStableLambdaNames$$Lambda$1471/0x000800d1
> - TestStableLambdaNames$$Lambda$1472/0x000800d10238
> - TestStableLambdaNames$$Lambda$1473/0x000800d10470
> 
> If we use the introduced flag, generated lambdas are:
> - TestStableLambdaNames$$Lambda$65ba26bbc6c7500d/0x000800c00400
> - TestStableLambdaNames$$Lambda$1569c8c4abe3ab18/0x000800c01800
> - TestStableLambdaNames$$Lambda$493c0ecaaf682428/0x000800c01a38
> In the second run of the method, generated lambdas are:
> - TestStableLambdaNames$$Lambda$65ba26bbc6c7500d/0x000800d1
> - TestStableLambdaNames$$Lambda$1569c8c4abe3ab18/0x000800d10238
> - TestStableLambdaNames$$Lambda$493c0ecaaf682428/0x000800d10470
> 
> We can see that the introduced hash value does not change between two calls 
> of the method `createPlainLambdas`. That was not the case in the JDK run 
> without this change. Those lambdas are extracted directly from the test.

Strahinja Stanojevic has updated the pull request incrementally with one 
additional commit since the last revision:

  Remove address from lambda class names in test on the 32-bit architecture too

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/10024/files
  - new: https://git.openjdk.org/jdk/pull/10024/files/f5feb430..275e23f2

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=10024=06
 - incr: https://webrevs.openjdk.org/?repo=jdk=10024=05-06

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.org/jdk/pull/10024.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/10024/head:pull/10024

PR: https://git.openjdk.org/jdk/pull/10024

Re: RFR: 8290313: Produce warning when user specified java.io.tmpdir directory doesn't exist [v5]

[Sean Coffey]

On Mon, 21 Nov 2022 16:35:46 GMT, Weibing Xiao  wrote:

>> print warning message for java.io.tmpdir when it is set through the command 
>> line and the value is not good for creating file folder.
>
> Weibing Xiao has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   update according to the review comments

Marked as reviewed by coffeys (Reviewer).

-

PR: https://git.openjdk.org/jdk/pull/11174

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v4]

[Alan Bateman]

On Mon, 21 Nov 2022 16:08:20 GMT, Per Minborg  wrote:

> Although this looks much simpler and concise, it means "a new object is 
> created for each invocation" 

My comment was actually to see if DirectBuffer could extend AutoCloseable so 
that the acquire returns "this" for the non-session case.  Doing the equivalent 
for the session case might leak into MemorySessionImpl implementing 
DirectBuffer which you probably don't want to do. If NOP_CLOSE.close can do the 
Reference.reachabilityFence(this) then it would at least improve some of the 
use-sites.

-

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8290313: Produce warning when user specified java.io.tmpdir directory doesn't exist [v5]

[Weibing Xiao]

> print warning message for java.io.tmpdir when it is set through the command 
> line and the value is not good for creating file folder.

Weibing Xiao has updated the pull request incrementally with one additional 
commit since the last revision:

  update according to the review comments

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/11174/files
  - new: https://git.openjdk.org/jdk/pull/11174/files/38f8855b..ba12fc0b

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=11174=04
 - incr: https://webrevs.openjdk.org/?repo=jdk=11174=03-04

  Stats: 8 lines in 3 files changed: 1 ins; 2 del; 5 mod
  Patch: https://git.openjdk.org/jdk/pull/11174.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/11174/head:pull/11174

PR: https://git.openjdk.org/jdk/pull/11174

Re: RFR: JDK-8285932 Implementation of JEP 430 String Templates (Preview) [v27]

[Jim Laskey]

> Enhance the Java programming language with string templates, which are 
> similar to string literals but contain embedded expressions. A string 
> template is interpreted at run time by replacing each expression with the 
> result of evaluating that expression, possibly after further validation and 
> transformation. This is a [preview language feature and 
> API](http://openjdk.java.net/jeps/12).

Jim Laskey has updated the pull request incrementally with one additional 
commit since the last revision:

  Requested changes #11

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/10889/files
  - new: https://git.openjdk.org/jdk/pull/10889/files/c51f88c7..f2562ab3

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=10889=26
 - incr: https://webrevs.openjdk.org/?repo=jdk=10889=25-26

  Stats: 85 lines in 6 files changed: 73 ins; 2 del; 10 mod
  Patch: https://git.openjdk.org/jdk/pull/10889.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/10889/head:pull/10889

PR: https://git.openjdk.org/jdk/pull/10889

Re: RFR: 8290313: Produce warning when user specified java.io.tmpdir directory doesn't exist [v4]

[Sean Coffey]

On Fri, 18 Nov 2022 18:36:51 GMT, Weibing Xiao  wrote:

>> print warning message for java.io.tmpdir when it is set through the command 
>> line and the value is not good for creating file folder.
>
> Weibing Xiao has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   updated from the review comment

src/java.base/share/classes/java/lang/System.java line 2247:

> 2245: 
> Unsafe.getUnsafe().ensureClassInitialized(StringConcatFactory.class);
> 2246: 
> 2247: // Emit a warning if java.io.tmpdir is set via the command line 
> to a directory that doesn't exist

could you split this comment across 2 lines ? - Keeps length similar to comment 
just above this code.

test/jdk/java/io/File/TempDirDoesNotExist.java line 30:

> 28:  */
> 29: 
> 30: import jdk.test.lib.Platform;

unused import

-

PR: https://git.openjdk.org/jdk/pull/11174

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v4]

[Per Minborg]

On Mon, 21 Nov 2022 14:07:38 GMT, Alan Bateman  wrote:

>> Per Minborg has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Rename methods
>
> src/java.base/share/classes/java/util/zip/Adler32.java line 105:
> 
>> 103: adler = updateByteBuffer(adler, 
>> ((DirectBuffer)buffer).address(), pos, rem);
>> 104: } finally {
>> 105: Reference.reachabilityFence(buffer);
> 
> The updated naming is a bit better but there are it still feels that that 
> there are too many things going on at the use sites ("guard", "session", 
> "reachability fence"). Ideally the acquire would return something with an 
> accessor for the direct buffer address but that may not be possible.  I 
> wonder if changing NOP_CLOSE.close to do reachabilityFence(this) would work 
> as expected and improve many of these use sites?

This can certainly be done. We could, for example, add a new method to the 
`JavaNioAccess` interface:

```AddressAcquisition acquireSession2(Buffer buffer); // to be renamed```

This would allow us to go from:


try (var guard = NIO_ACCESS.acquireSession(buffer)) {
adler = updateByteBuffer(adler, 
((DirectBuffer)buffer).address(), pos, rem);
} finally {
Reference.reachabilityFence(buffer);
}


to


try (var guard = NIO_ACCESS.acquireSession2(buffer)) {
adler = updateByteBuffer(adler, guard.address(), pos, rem);
}


Although this looks much simpler and concise, it means "a new object is created 
for each invocation" (*). This also allows us to remove the 
`@SupressWarnings("try")` annotations.

Here is how the undercarriage might look like:


@Override
public AddressAcquisition acquireSession2(Buffer buffer) {
var session = buffer.session();
if (session == null) {
return AddressAcquisition.create(buffer, () -> {});
}
session.acquire0();
return AddressAcquisition.create(buffer, session::release0);
}



and


sealed interface AddressAcquisition extends AutoCloseable {

long address();

@Override
void close();

static AddressAcquisition create(Buffer buffer, Runnable closeActions) {
return new AddressAcquisitionImpl(buffer, closeActions);
}

final class AddressAcquisitionImpl implements AddressAcquisition {

private final DirectBuffer directBuffer;
private final Runnable closeAction;

public AddressAcquisitionImpl(Buffer buffer,
  Runnable closeAction) {
this.directBuffer = (DirectBuffer) buffer;
this.closeAction = closeAction;
}

@Override
public long address() {
return directBuffer.address();
}

@Override
public void close() {
try {
closeAction.run();
} finally {
Reference.reachabilityFence(directBuffer);
}
}
}

}



(*) In reality,  a representation of the object might be allocated on the stack 
instead.

-

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8290313: Produce warning when user specified java.io.tmpdir directory doesn't exist [v2]

[Sean Coffey]

On Wed, 16 Nov 2022 15:06:14 GMT, Roger Riggs  wrote:

>> Weibing Xiao has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   the change according to review comment
>
> src/java.base/share/classes/jdk/internal/util/SystemProps.java line 52:
> 
>> 50:  * @return a boolean value
>> 51:  */
>> 52: public static boolean checkIfWarningRequired() {
> 
> I would rename the method to `checkIoTmpdir`; it will be easier to see the 
> purpose in the call in System.

+1 I might suggest `isBadIoTmpdir` for method name

-

PR: https://git.openjdk.org/jdk/pull/11174

Re: RFR: JDK-8285932 Implementation of JEP 430 String Templates (Preview) [v26]

[Jim Laskey]

On Fri, 18 Nov 2022 22:34:35 GMT, Roger Riggs  wrote:

>> Jim Laskey has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Typo
>
> src/java.base/share/classes/java/lang/template/StringTemplate.java line 300:
> 
>> 298:  * {@link StringTemplate StringTemplates}.
>> 299:  *
>> 300:  * @param stringTemplates  one or more {@link StringTemplate}
> 
> Suggestion:
> 
>  * @param stringTemplates  zero or more {@link StringTemplate}
> 
> 
> Also the comment in `TemplateSupport.combine` should say zero or more.

Changing.

-

PR: https://git.openjdk.org/jdk/pull/10889

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v8]

[Per Minborg]

> This PR proposes the introduction of **guarding** of the use of 
> `DirectBuffer::address` within the JDK. With the introduction of the Foreign 
> Function and Memory access, it is possible to derive Buffer instances that 
> are backed by native memory that, in turn, can be closed asynchronously by 
> the user (and not only via a `Cleaner` when there is no other reference to 
> the `Buffer` object). If another thread is invoking `MemorySession::close` 
> while a thread is doing work using raw addresses, the outcome is undefined. 
> This means the JVM might crash or even worse, silent modification of 
> unrelated memory might occur. 
> 
> Design choices in this PR:
> 
> There is already a method `MemorySession::whileAlive` that takes a runnable 
> and that will perform the provided action while acquiring the underlying` 
> MemorySession` (if any). However, using this method entailed relatively large 
> changes while converting larger/nested code segments into lambdas. This would 
> also risk introducing lambda capturing. So, instead, a try-with-resources 
> friendly access method was added. This made is more easy to add guarding and 
> did not risk lambda capturing. Also, introducing lambdas in certain 
> fundamental JDK classes might incur bootstrap problems.
> 
> The aforementioned TwR is using a "session acquisition" that is not used 
> explicitly in the try block itself. This raises a warning that is suppressed 
> using `@SuppressWarnings("try")`. In the future, we might be able to remove 
> these suppressions by using the reserved variable name `_`. Another 
> alternative was evaluated where a non-autocloseable resource was used. 
> However, it became relatively complicated to guarantee that the session was 
> always released and, at the same time, the existing 'final` block was always 
> executed properly (as they both might throw exceptions). In the end, the 
> proposed solution was much simpler and robust despite requiring a 
> non-referenced TwR variable.
> 
> In some cases, where is is guaranteed that the backing memory session is 
> non-closeable, we do not have to guard the use of `DirectBuffer::address`. 
> ~~These cases have been documented in the code.~~
> 
> On some occasions, a plurality of acquisitions are made. This would never 
> lead to deadlocks as acquisitions are fundamentally concurrent counters and 
> not resources that only one thread can "own".
> 
> I have added comments (and not javadocs) before the declaration of the 
> non-public-api `DirectBuffer::address` method, that the use of the returned 
> address needs to be guarded. It can be discussed if this is preferable or not.
> 
> This PR spawns several areas of responsibility and so, I expect more than one 
> reviewer before promoting the PR.

Per Minborg has updated the pull request incrementally with three additional 
commits since the last revision:

 - Merge branch 'guard-directbuffer-address' of https://github.com/minborg/jdk 
into guard-directbuffer-address
 - Update src/java.base/share/classes/jdk/internal/access/JavaNioAccess.java
   
   Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
 - Remove comments

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/11260/files
  - new: https://git.openjdk.org/jdk/pull/11260/files/17a72e9f..88ed3aa2

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=11260=07
 - incr: https://webrevs.openjdk.org/?repo=jdk=11260=06-07

  Stats: 5 lines in 3 files changed: 0 ins; 4 del; 1 mod
  Patch: https://git.openjdk.org/jdk/pull/11260.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/11260/head:pull/11260

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v6]

[Per Minborg]

On Mon, 21 Nov 2022 14:14:14 GMT, Alan Bateman  wrote:

>> Per Minborg has updated the pull request incrementally with two additional 
>> commits since the last revision:
>> 
>>  - Update src/java.base/share/classes/jdk/internal/access/JavaNioAccess.java
>>
>>Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
>>  - Update src/java.base/share/classes/sun/nio/ch/DirectBuffer.java
>>
>>Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
>
> src/java.base/share/classes/sun/nio/fs/NativeBuffers.java line 92:
> 
>> 90:  * allocated from the heap.
>> 91:  * 
>> 92:  * The returned NativeBuffer is guaranteed not to be asynchronously 
>> closeable.
> 
> This class, and the temporary buffer cache in sun.nio.ch.Util, are intended 
> to be used with try-finally. There isn't any notion of  asynchronously close 
> so maybe it would best to not add this comment to these sources.

That is clear to me but I am trying to prevent future redundant guarding. 
Anyway, I will remove the comments.

-

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: JDK-8285932 Implementation of JEP 430 String Templates (Preview) [v26]

[Jim Laskey]

On Mon, 21 Nov 2022 12:52:42 GMT, Jim Laskey  wrote:

>> src/java.base/share/classes/java/lang/template/StringTemplate.java line 35:
>> 
>>> 33: /**
>>> 34:  * {@link StringTemplate} is the run-time representation of a string 
>>> template or
>>> 35:  * text block template in a template expression.
>> 
>> Can the link to the JLS 15.8.6 be included earlier so the definition of 
>> "template expression" isn't hanging until the end of the class javadoc.
>
> Didn't know that it was positional. Will change.

Turns out that @jls is sensitive to its position.

-

PR: https://git.openjdk.org/jdk/pull/10889

Re: RFR: JDK-8285932 Implementation of JEP 430 String Templates (Preview) [v26]

[Claes Redestad]

On Fri, 18 Nov 2022 14:10:11 GMT, Jim Laskey  wrote:

>> Enhance the Java programming language with string templates, which are 
>> similar to string literals but contain embedded expressions. A string 
>> template is interpreted at run time by replacing each expression with the 
>> result of evaluating that expression, possibly after further validation and 
>> transformation. This is a [preview language feature and 
>> API](http://openjdk.java.net/jeps/12).
>
> Jim Laskey has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Typo

src/java.base/share/classes/java/util/Digits.java line 39:

> 37:  */
> 38: @PreviewFeature(feature=PreviewFeature.Feature.STRING_TEMPLATES)
> 39: interface Digits {

Should this be modeled as an `abstract sealed` class hierarchy instead?

test/micro/org/openjdk/bench/java/lang/template/StringTemplateFMT.java line 41:

> 39: /*
> 40:  * This benchmark measures StringTemplate.FMT FormatProcessor performance;
> 41:  * exactly mirroring {@link org.openjdk.bench.java.lang.StringFormat} 
> benchmark

While it's good to mirror the `StringFormat` benchmark, I think we'd see 
benefit from building this out to be a bit more comprehensive and for example 
cover `FormatConcatItem` cases. What you have here is probably good enough for 
first integration, though.

-

PR: https://git.openjdk.org/jdk/pull/10889

Re: RFR: JDK-8285932 Implementation of JEP 430 String Templates (Preview) [v26]

[Jim Laskey]

On Fri, 18 Nov 2022 22:25:22 GMT, Roger Riggs  wrote:

>> Jim Laskey has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Typo
>
> src/java.base/share/classes/java/lang/template/package-info.java line 27:
> 
>> 25: 
>> 26: /**
>> 27:  * Provides support for string templates and template processors.
> 
> Not just support...
> 
> String templates are provided by {@link StringTemplate} and supporting 
> interfaces and classes.
> It might also be helpful to include a link to the JLS 15.8.6 Template 
> Expression spec.
> 
> A brief example here (drawn from the JEP or StringTemplate would provide a 
> good hook for the reader to get started.

Will expand.

-

PR: https://git.openjdk.org/jdk/pull/10889

Re: RFR: 8292914: Introduce a system property that enables stable names for lambda proxy classes [v6]

[Strahinja Stanojevic]

> This PR introduces an option to output stable names for the lambda classes in 
> the JDK. A stable name consists of two parts: The first part is the 
> predefined value `$$Lambda$` appended to the lambda capturing class, and the 
> second is a 64-bit hash part of the name. Thus, it looks like 
> `lambdaCapturingClass$$Lambda$hashValue`.
> Parameters used to create a stable hash are a superset of the parameters used 
> for lambda class archiving when the CDS dumping option is enabled. During 
> this process, all the mutual parameters are in the same form as they are in 
> the low-level implementation 
> (`SystemDictionaryShared::add_lambda_proxy_class`) of the archiving process.
> We decided to use a well-specified `CRC32` algorithm from the standard Java 
> library. We created two 32-bit hashes from the parameters used to create 
> stable names. Then, we combined those two 32-bit hashes into one 64-bit hash 
> value.
> We chose `CRC32` because it is a well-specified hash function, and we don't 
> need to write additional code in the JDK. `SHA-256, MD5`, and all other hash 
> functions that rely on `MessageDigest` use lambdas in the implementation, so 
> they are unsuitable for our purpose. We also considered a few different hash 
> functions with a low collision rate. All these functions would require at 
> least 100 lines of additional code in the JDK. The best alternative we found 
> is 64-bit` MurmurHash2`: 
> https://commons.apache.org/proper/commons-codec/jacoco/org.apache.commons.codec.digest/MurmurHash2.java.html.
>   In case adding a new hash implementation (e.g., Murmur2) to the JDK is 
> preferred, this PR can be easily modified.
> We found the post 
> (https://softwareengineering.stackexchange.com/questions/49550/which-hashing-algorithm-is-best-for-uniqueness-and-speed/145633#145633)
>  that compares different hash functions.
> We also tested the `CRC32` hash function against half a billion generated 
> strings, and there were no collisions. Note that the capturing-class name is 
> also part of the lambda class name, so the potential collisions can only 
> appear in a single class. Thus, we do not expect to have name collisions due 
> to a relatively low number of lambdas per class. Every tool that uses this 
> feature should handle potential collisions on its own.  
> We found an overall approximation of the collision rate too. You can find it 
> here: https://preshing.com/20110504/hash-collision-probabilities/.
> 
> JDK currently adds an atomic integer after `$$Lambda$`, and the names of the 
> lambdas depend on the creation order. In the `TestStableLambdaNames`, we 
> generate all the lambdas two times. In the first run, the method 
> createPlainLambdas generate the following lambdas:
> 
> - TestStableLambdaNames$$Lambda$1/0x000800c00400
> - TestStableLambdaNames$$Lambda$2/0x000800c01800
> - TestStableLambdaNames$$Lambda$3/0x000800c01a38
> The same method in the second run generates lambdas with different names:
> - TestStableLambdaNames$$Lambda$1471/0x000800d1
> - TestStableLambdaNames$$Lambda$1472/0x000800d10238
> - TestStableLambdaNames$$Lambda$1473/0x000800d10470
> 
> If we use the introduced flag, generated lambdas are:
> - TestStableLambdaNames$$Lambda$65ba26bbc6c7500d/0x000800c00400
> - TestStableLambdaNames$$Lambda$1569c8c4abe3ab18/0x000800c01800
> - TestStableLambdaNames$$Lambda$493c0ecaaf682428/0x000800c01a38
> In the second run of the method, generated lambdas are:
> - TestStableLambdaNames$$Lambda$65ba26bbc6c7500d/0x000800d1
> - TestStableLambdaNames$$Lambda$1569c8c4abe3ab18/0x000800d10238
> - TestStableLambdaNames$$Lambda$493c0ecaaf682428/0x000800d10470
> 
> We can see that the introduced hash value does not change between two calls 
> of the method `createPlainLambdas`. That was not the case in the JDK run 
> without this change. Those lambdas are extracted directly from the test.

Strahinja Stanojevic has updated the pull request incrementally with one 
additional commit since the last revision:

  Create stable lambda names using CRC32 hash function

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/10024/files
  - new: https://git.openjdk.org/jdk/pull/10024/files/6179915a..f5feb430

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=10024=05
 - incr: https://webrevs.openjdk.org/?repo=jdk=10024=04-05

  Stats: 82 lines in 1 file changed: 27 ins; 30 del; 25 mod
  Patch: https://git.openjdk.org/jdk/pull/10024.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/10024/head:pull/10024

PR: https://git.openjdk.org/jdk/pull/10024

Re: RFR: 8292914: Introduce a system property that enables stable names for lambda proxy classes [v5]

[Strahinja Stanojevic]

> This PR introduces an option to output stable names for the lambda classes in 
> the JDK. A stable name consists of two parts: The first part is the 
> predefined value `$$Lambda$` appended to the lambda capturing class, and the 
> second is a 64-bit hash part of the name. Thus, it looks like 
> `lambdaCapturingClass$$Lambda$hashValue`.
> Parameters used to create a stable hash are a superset of the parameters used 
> for lambda class archiving when the CDS dumping option is enabled. During 
> this process, all the mutual parameters are in the same form as they are in 
> the low-level implementation 
> (`SystemDictionaryShared::add_lambda_proxy_class`) of the archiving process.
> We decided to use a well-specified `CRC32` algorithm from the standard Java 
> library. We created two 32-bit hashes from the parameters used to create 
> stable names. Then, we combined those two 32-bit hashes into one 64-bit hash 
> value.
> We chose `CRC32` because it is a well-specified hash function, and we don't 
> need to write additional code in the JDK. `SHA-256, MD5`, and all other hash 
> functions that rely on `MessageDigest` use lambdas in the implementation, so 
> they are unsuitable for our purpose. We also considered a few different hash 
> functions with a low collision rate. All these functions would require at 
> least 100 lines of additional code in the JDK. The best alternative we found 
> is 64-bit` MurmurHash2`: 
> https://commons.apache.org/proper/commons-codec/jacoco/org.apache.commons.codec.digest/MurmurHash2.java.html.
>   In case adding a new hash implementation (e.g., Murmur2) to the JDK is 
> preferred, this PR can be easily modified.
> We found the post 
> (https://softwareengineering.stackexchange.com/questions/49550/which-hashing-algorithm-is-best-for-uniqueness-and-speed/145633#145633)
>  that compares different hash functions.
> We also tested the `CRC32` hash function against half a billion generated 
> strings, and there were no collisions. Note that the capturing-class name is 
> also part of the lambda class name, so the potential collisions can only 
> appear in a single class. Thus, we do not expect to have name collisions due 
> to a relatively low number of lambdas per class. Every tool that uses this 
> feature should handle potential collisions on its own.  
> We found an overall approximation of the collision rate too. You can find it 
> here: https://preshing.com/20110504/hash-collision-probabilities/.
> 
> JDK currently adds an atomic integer after `$$Lambda$`, and the names of the 
> lambdas depend on the creation order. In the `TestStableLambdaNames`, we 
> generate all the lambdas two times. In the first run, the method 
> createPlainLambdas generate the following lambdas:
> 
> - TestStableLambdaNames$$Lambda$1/0x000800c00400
> - TestStableLambdaNames$$Lambda$2/0x000800c01800
> - TestStableLambdaNames$$Lambda$3/0x000800c01a38
> The same method in the second run generates lambdas with different names:
> - TestStableLambdaNames$$Lambda$1471/0x000800d1
> - TestStableLambdaNames$$Lambda$1472/0x000800d10238
> - TestStableLambdaNames$$Lambda$1473/0x000800d10470
> 
> If we use the introduced flag, generated lambdas are:
> - TestStableLambdaNames$$Lambda$65ba26bbc6c7500d/0x000800c00400
> - TestStableLambdaNames$$Lambda$1569c8c4abe3ab18/0x000800c01800
> - TestStableLambdaNames$$Lambda$493c0ecaaf682428/0x000800c01a38
> In the second run of the method, generated lambdas are:
> - TestStableLambdaNames$$Lambda$65ba26bbc6c7500d/0x000800d1
> - TestStableLambdaNames$$Lambda$1569c8c4abe3ab18/0x000800d10238
> - TestStableLambdaNames$$Lambda$493c0ecaaf682428/0x000800d10470
> 
> We can see that the introduced hash value does not change between two calls 
> of the method `createPlainLambdas`. That was not the case in the JDK run 
> without this change. Those lambdas are extracted directly from the test.

Strahinja Stanojevic has refreshed the contents of this pull request, and 
previous commits have been removed. The incremental views will show differences 
compared to the previous content of the PR. The pull request contains one new 
commit since the last revision:

  Calculate stable names for lambda classes using different hash function.

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/10024/files
  - new: https://git.openjdk.org/jdk/pull/10024/files/dd8e592d..6179915a

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=10024=04
 - incr: https://webrevs.openjdk.org/?repo=jdk=10024=03-04

  Stats: 0 lines in 0 files changed: 0 ins; 0 del; 0 mod
  Patch: https://git.openjdk.org/jdk/pull/10024.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/10024/head:pull/10024

PR: https://git.openjdk.org/jdk/pull/10024

Integrated: 8296472: Remove ObjectLocker around appendToClassPathForInstrumentation call

[Coleen Phillimore]

On Mon, 7 Nov 2022 17:07:01 GMT, Coleen Phillimore  wrote:

> This patch moves the acquisition of the boot class loader lock out of the JVM 
> and into the Java function.
> Tested with tier1-4, and jvmti and jdi tests locally.

This pull request has now been integrated.

Changeset: 5c334540
Author:Coleen Phillimore 
URL:   
https://git.openjdk.org/jdk/commit/5c3345404d850cf01d9629b48015f1783a32bfc0
Stats: 3 lines in 1 file changed: 0 ins; 2 del; 1 mod

8296472: Remove ObjectLocker around appendToClassPathForInstrumentation call

Reviewed-by: sspitsyn, alanb, dholmes

-

PR: https://git.openjdk.org/jdk/pull/11023

Re: RFR: 8292914: Introduce a system property that enables stable names for lambda proxy classes [v4]

[Strahinja Stanojevic]

> This PR introduces an option to output stable names for the lambda classes in 
> the JDK. A stable name consists of two parts: The first part is the 
> predefined value `$$Lambda$` appended to the lambda capturing class, and the 
> second is a 64-bit hash part of the name. Thus, it looks like 
> `lambdaCapturingClass$$Lambda$hashValue`.
> Parameters used to create a stable hash are a superset of the parameters used 
> for lambda class archiving when the CDS dumping option is enabled. During 
> this process, all the mutual parameters are in the same form as they are in 
> the low-level implementation 
> (`SystemDictionaryShared::add_lambda_proxy_class`) of the archiving process.
> We decided to use a well-specified `CRC32` algorithm from the standard Java 
> library. We created two 32-bit hashes from the parameters used to create 
> stable names. Then, we combined those two 32-bit hashes into one 64-bit hash 
> value.
> We chose `CRC32` because it is a well-specified hash function, and we don't 
> need to write additional code in the JDK. `SHA-256, MD5`, and all other hash 
> functions that rely on `MessageDigest` use lambdas in the implementation, so 
> they are unsuitable for our purpose. We also considered a few different hash 
> functions with a low collision rate. All these functions would require at 
> least 100 lines of additional code in the JDK. The best alternative we found 
> is 64-bit` MurmurHash2`: 
> https://commons.apache.org/proper/commons-codec/jacoco/org.apache.commons.codec.digest/MurmurHash2.java.html.
>   In case adding a new hash implementation (e.g., Murmur2) to the JDK is 
> preferred, this PR can be easily modified.
> We found the post 
> (https://softwareengineering.stackexchange.com/questions/49550/which-hashing-algorithm-is-best-for-uniqueness-and-speed/145633#145633)
>  that compares different hash functions.
> We also tested the `CRC32` hash function against half a billion generated 
> strings, and there were no collisions. Note that the capturing-class name is 
> also part of the lambda class name, so the potential collisions can only 
> appear in a single class. Thus, we do not expect to have name collisions due 
> to a relatively low number of lambdas per class. Every tool that uses this 
> feature should handle potential collisions on its own.  
> We found an overall approximation of the collision rate too. You can find it 
> here: https://preshing.com/20110504/hash-collision-probabilities/.
> 
> JDK currently adds an atomic integer after `$$Lambda$`, and the names of the 
> lambdas depend on the creation order. In the `TestStableLambdaNames`, we 
> generate all the lambdas two times. In the first run, the method 
> createPlainLambdas generate the following lambdas:
> 
> - TestStableLambdaNames$$Lambda$1/0x000800c00400
> - TestStableLambdaNames$$Lambda$2/0x000800c01800
> - TestStableLambdaNames$$Lambda$3/0x000800c01a38
> The same method in the second run generates lambdas with different names:
> - TestStableLambdaNames$$Lambda$1471/0x000800d1
> - TestStableLambdaNames$$Lambda$1472/0x000800d10238
> - TestStableLambdaNames$$Lambda$1473/0x000800d10470
> 
> If we use the introduced flag, generated lambdas are:
> - TestStableLambdaNames$$Lambda$65ba26bbc6c7500d/0x000800c00400
> - TestStableLambdaNames$$Lambda$1569c8c4abe3ab18/0x000800c01800
> - TestStableLambdaNames$$Lambda$493c0ecaaf682428/0x000800c01a38
> In the second run of the method, generated lambdas are:
> - TestStableLambdaNames$$Lambda$65ba26bbc6c7500d/0x000800d1
> - TestStableLambdaNames$$Lambda$1569c8c4abe3ab18/0x000800d10238
> - TestStableLambdaNames$$Lambda$493c0ecaaf682428/0x000800d10470
> 
> We can see that the introduced hash value does not change between two calls 
> of the method `createPlainLambdas`. That was not the case in the JDK run 
> without this change. Those lambdas are extracted directly from the test.

Strahinja Stanojevic has updated the pull request incrementally with one 
additional commit since the last revision:

  Calculate stable names for lambda classes using another hash function

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/10024/files
  - new: https://git.openjdk.org/jdk/pull/10024/files/6a8f9128..dd8e592d

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=10024=03
 - incr: https://webrevs.openjdk.org/?repo=jdk=10024=02-03

  Stats: 606 lines in 2 files changed: 298 ins; 308 del; 0 mod
  Patch: https://git.openjdk.org/jdk/pull/10024.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/10024/head:pull/10024

PR: https://git.openjdk.org/jdk/pull/10024

Re: RFR: 8296472: Remove ObjectLocker around appendToClassPathForInstrumentation call [v3]

[Coleen Phillimore]

On Mon, 7 Nov 2022 20:40:33 GMT, Coleen Phillimore  wrote:

>> This patch moves the acquisition of the boot class loader lock out of the 
>> JVM and into the Java function.
>> Tested with tier1-4, and jvmti and jdi tests locally.
>
> Coleen Phillimore has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   really revert the file

Thanks David and Alan for the code review and all the help with the CSR and 
release notes.

-

PR: https://git.openjdk.org/jdk/pull/11023

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v6]

[Alan Bateman]

On Mon, 21 Nov 2022 14:02:38 GMT, Per Minborg  wrote:

>> This PR proposes the introduction of **guarding** of the use of 
>> `DirectBuffer::address` within the JDK. With the introduction of the Foreign 
>> Function and Memory access, it is possible to derive Buffer instances that 
>> are backed by native memory that, in turn, can be closed asynchronously by 
>> the user (and not only via a `Cleaner` when there is no other reference to 
>> the `Buffer` object). If another thread is invoking `MemorySession::close` 
>> while a thread is doing work using raw addresses, the outcome is undefined. 
>> This means the JVM might crash or even worse, silent modification of 
>> unrelated memory might occur. 
>> 
>> Design choices in this PR:
>> 
>> There is already a method `MemorySession::whileAlive` that takes a runnable 
>> and that will perform the provided action while acquiring the underlying` 
>> MemorySession` (if any). However, using this method entailed relatively 
>> large changes while converting larger/nested code segments into lambdas. 
>> This would also risk introducing lambda capturing. So, instead, a 
>> try-with-resources friendly access method was added. This made is more easy 
>> to add guarding and did not risk lambda capturing. Also, introducing lambdas 
>> in certain fundamental JDK classes might incur bootstrap problems.
>> 
>> The aforementioned TwR is using a "session acquisition" that is not used 
>> explicitly in the try block itself. This raises a warning that is suppressed 
>> using `@SuppressWarnings("try")`. In the future, we might be able to remove 
>> these suppressions by using the reserved variable name `_`. Another 
>> alternative was evaluated where a non-autocloseable resource was used. 
>> However, it became relatively complicated to guarantee that the session was 
>> always released and, at the same time, the existing 'final` block was always 
>> executed properly (as they both might throw exceptions). In the end, the 
>> proposed solution was much simpler and robust despite requiring a 
>> non-referenced TwR variable.
>> 
>> In some cases, where is is guaranteed that the backing memory session is 
>> non-closeable, we do not have to guard the use of `DirectBuffer::address`. 
>> ~~These cases have been documented in the code.~~
>> 
>> On some occasions, a plurality of acquisitions are made. This would never 
>> lead to deadlocks as acquisitions are fundamentally concurrent counters and 
>> not resources that only one thread can "own".
>> 
>> I have added comments (and not javadocs) before the declaration of the 
>> non-public-api `DirectBuffer::address` method, that the use of the returned 
>> address needs to be guarded. It can be discussed if this is preferable or 
>> not.
>> 
>> This PR spawns several areas of responsibility and so, I expect more than 
>> one reviewer before promoting the PR.
>
> Per Minborg has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - Update src/java.base/share/classes/jdk/internal/access/JavaNioAccess.java
>
>Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
>  - Update src/java.base/share/classes/sun/nio/ch/DirectBuffer.java
>
>Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>

src/java.base/share/classes/sun/nio/fs/NativeBuffers.java line 92:

> 90:  * allocated from the heap.
> 91:  * 
> 92:  * The returned NativeBuffer is guaranteed not to be asynchronously 
> closeable.

This class, and the temporary buffer cache in sun.nio.ch.Util, are intended to 
be used with try-finally. There isn't any notion of  asynchronously close so 
maybe it would best to not add this comment to these sources.

-

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v4]

[Alan Bateman]

On Mon, 21 Nov 2022 13:02:48 GMT, Per Minborg  wrote:

>> This PR proposes the introduction of **guarding** of the use of 
>> `DirectBuffer::address` within the JDK. With the introduction of the Foreign 
>> Function and Memory access, it is possible to derive Buffer instances that 
>> are backed by native memory that, in turn, can be closed asynchronously by 
>> the user (and not only via a `Cleaner` when there is no other reference to 
>> the `Buffer` object). If another thread is invoking `MemorySession::close` 
>> while a thread is doing work using raw addresses, the outcome is undefined. 
>> This means the JVM might crash or even worse, silent modification of 
>> unrelated memory might occur. 
>> 
>> Design choices in this PR:
>> 
>> There is already a method `MemorySession::whileAlive` that takes a runnable 
>> and that will perform the provided action while acquiring the underlying` 
>> MemorySession` (if any). However, using this method entailed relatively 
>> large changes while converting larger/nested code segments into lambdas. 
>> This would also risk introducing lambda capturing. So, instead, a 
>> try-with-resources friendly access method was added. This made is more easy 
>> to add guarding and did not risk lambda capturing. Also, introducing lambdas 
>> in certain fundamental JDK classes might incur bootstrap problems.
>> 
>> The aforementioned TwR is using a "session acquisition" that is not used 
>> explicitly in the try block itself. This raises a warning that is suppressed 
>> using `@SuppressWarnings("try")`. In the future, we might be able to remove 
>> these suppressions by using the reserved variable name `_`. Another 
>> alternative was evaluated where a non-autocloseable resource was used. 
>> However, it became relatively complicated to guarantee that the session was 
>> always released and, at the same time, the existing 'final` block was always 
>> executed properly (as they both might throw exceptions). In the end, the 
>> proposed solution was much simpler and robust despite requiring a 
>> non-referenced TwR variable.
>> 
>> In some cases, where is is guaranteed that the backing memory session is 
>> non-closeable, we do not have to guard the use of `DirectBuffer::address`. 
>> ~~These cases have been documented in the code.~~
>> 
>> On some occasions, a plurality of acquisitions are made. This would never 
>> lead to deadlocks as acquisitions are fundamentally concurrent counters and 
>> not resources that only one thread can "own".
>> 
>> I have added comments (and not javadocs) before the declaration of the 
>> non-public-api `DirectBuffer::address` method, that the use of the returned 
>> address needs to be guarded. It can be discussed if this is preferable or 
>> not.
>> 
>> This PR spawns several areas of responsibility and so, I expect more than 
>> one reviewer before promoting the PR.
>
> Per Minborg has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Rename methods

src/java.base/share/classes/java/util/zip/Adler32.java line 105:

> 103: adler = updateByteBuffer(adler, 
> ((DirectBuffer)buffer).address(), pos, rem);
> 104: } finally {
> 105: Reference.reachabilityFence(buffer);

The updated naming is a bit better but there are it still feels that that there 
are too many things going on at the use sites ("guard", "session", 
"reachability fence"). Ideally the acquire would return something with an 
accessor for the direct buffer address but that may not be possible.  I wonder 
if changing NOP_CLOSE.close to do reachabilityFence(this) would work as 
expected and improve many of these use sites?

-

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v7]

[Per Minborg]

> This PR proposes the introduction of **guarding** of the use of 
> `DirectBuffer::address` within the JDK. With the introduction of the Foreign 
> Function and Memory access, it is possible to derive Buffer instances that 
> are backed by native memory that, in turn, can be closed asynchronously by 
> the user (and not only via a `Cleaner` when there is no other reference to 
> the `Buffer` object). If another thread is invoking `MemorySession::close` 
> while a thread is doing work using raw addresses, the outcome is undefined. 
> This means the JVM might crash or even worse, silent modification of 
> unrelated memory might occur. 
> 
> Design choices in this PR:
> 
> There is already a method `MemorySession::whileAlive` that takes a runnable 
> and that will perform the provided action while acquiring the underlying` 
> MemorySession` (if any). However, using this method entailed relatively large 
> changes while converting larger/nested code segments into lambdas. This would 
> also risk introducing lambda capturing. So, instead, a try-with-resources 
> friendly access method was added. This made is more easy to add guarding and 
> did not risk lambda capturing. Also, introducing lambdas in certain 
> fundamental JDK classes might incur bootstrap problems.
> 
> The aforementioned TwR is using a "session acquisition" that is not used 
> explicitly in the try block itself. This raises a warning that is suppressed 
> using `@SuppressWarnings("try")`. In the future, we might be able to remove 
> these suppressions by using the reserved variable name `_`. Another 
> alternative was evaluated where a non-autocloseable resource was used. 
> However, it became relatively complicated to guarantee that the session was 
> always released and, at the same time, the existing 'final` block was always 
> executed properly (as they both might throw exceptions). In the end, the 
> proposed solution was much simpler and robust despite requiring a 
> non-referenced TwR variable.
> 
> In some cases, where is is guaranteed that the backing memory session is 
> non-closeable, we do not have to guard the use of `DirectBuffer::address`. 
> ~~These cases have been documented in the code.~~
> 
> On some occasions, a plurality of acquisitions are made. This would never 
> lead to deadlocks as acquisitions are fundamentally concurrent counters and 
> not resources that only one thread can "own".
> 
> I have added comments (and not javadocs) before the declaration of the 
> non-public-api `DirectBuffer::address` method, that the use of the returned 
> address needs to be guarded. It can be discussed if this is preferable or not.
> 
> This PR spawns several areas of responsibility and so, I expect more than one 
> reviewer before promoting the PR.

Per Minborg has refreshed the contents of this pull request, and previous 
commits have been removed. Incremental views are not available. The pull 
request now contains six commits:

 - Update src/java.base/share/classes/sun/nio/ch/DirectBuffer.java
   
   Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
 - Cleanup static declarations
 - Rename methods
 - Remove comments
 - Cleanup after comments
 - Guard usages of DirectBuffer::address

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/11260/files
  - new: https://git.openjdk.org/jdk/pull/11260/files/a0de3bcf..17a72e9f

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=11260=06
 - incr: https://webrevs.openjdk.org/?repo=jdk=11260=05-06

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.org/jdk/pull/11260.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/11260/head:pull/11260

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8297301: Cleanup unused methods in JavaUtilJarAccess [v6]

[Weijun Wang]

On Mon, 21 Nov 2022 00:29:34 GMT, pandaapo  wrote:

>> The cache named `signerToCodeSource` in `JarVerifier` is never used now.
>
> pandaapo has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Modify as review and update copyright.

Looks almost complete. Just 2 small comments.

src/java.base/share/classes/java/util/jar/JarVerifier.java line 330:

> 328: }
> 329: 
> 330: public Certificate[] getCerts(JarEntry entry)

You might want to move the javadoc of the removed `getCerts(String)` method 
here. It looks there was a single javadoc for 2 methods. We should keep one.

src/java.base/share/classes/java/util/jar/JarVerifier.java line 345:

> 343: }
> 344: 
> 345: public CodeSigner[] getCodeSigners(JarEntry entry)

Since the 1st `getCodeSigners` is only used by the 2nd one. We can inline the 
1st one into the 2nd one.

-

PR: https://git.openjdk.org/jdk/pull/11072

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v6]

[Per Minborg]

> This PR proposes the introduction of **guarding** of the use of 
> `DirectBuffer::address` within the JDK. With the introduction of the Foreign 
> Function and Memory access, it is possible to derive Buffer instances that 
> are backed by native memory that, in turn, can be closed asynchronously by 
> the user (and not only via a `Cleaner` when there is no other reference to 
> the `Buffer` object). If another thread is invoking `MemorySession::close` 
> while a thread is doing work using raw addresses, the outcome is undefined. 
> This means the JVM might crash or even worse, silent modification of 
> unrelated memory might occur. 
> 
> Design choices in this PR:
> 
> There is already a method `MemorySession::whileAlive` that takes a runnable 
> and that will perform the provided action while acquiring the underlying` 
> MemorySession` (if any). However, using this method entailed relatively large 
> changes while converting larger/nested code segments into lambdas. This would 
> also risk introducing lambda capturing. So, instead, a try-with-resources 
> friendly access method was added. This made is more easy to add guarding and 
> did not risk lambda capturing. Also, introducing lambdas in certain 
> fundamental JDK classes might incur bootstrap problems.
> 
> The aforementioned TwR is using a "session acquisition" that is not used 
> explicitly in the try block itself. This raises a warning that is suppressed 
> using `@SuppressWarnings("try")`. In the future, we might be able to remove 
> these suppressions by using the reserved variable name `_`. Another 
> alternative was evaluated where a non-autocloseable resource was used. 
> However, it became relatively complicated to guarantee that the session was 
> always released and, at the same time, the existing 'final` block was always 
> executed properly (as they both might throw exceptions). In the end, the 
> proposed solution was much simpler and robust despite requiring a 
> non-referenced TwR variable.
> 
> In some cases, where is is guaranteed that the backing memory session is 
> non-closeable, we do not have to guard the use of `DirectBuffer::address`. 
> ~~These cases have been documented in the code.~~
> 
> On some occasions, a plurality of acquisitions are made. This would never 
> lead to deadlocks as acquisitions are fundamentally concurrent counters and 
> not resources that only one thread can "own".
> 
> I have added comments (and not javadocs) before the declaration of the 
> non-public-api `DirectBuffer::address` method, that the use of the returned 
> address needs to be guarded. It can be discussed if this is preferable or not.
> 
> This PR spawns several areas of responsibility and so, I expect more than one 
> reviewer before promoting the PR.

Per Minborg has updated the pull request incrementally with two additional 
commits since the last revision:

 - Update src/java.base/share/classes/jdk/internal/access/JavaNioAccess.java
   
   Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
 - Update src/java.base/share/classes/sun/nio/ch/DirectBuffer.java
   
   Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/11260/files
  - new: https://git.openjdk.org/jdk/pull/11260/files/eca7c388..a0de3bcf

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=11260=05
 - incr: https://webrevs.openjdk.org/?repo=jdk=11260=04-05

  Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod
  Patch: https://git.openjdk.org/jdk/pull/11260.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/11260/head:pull/11260

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v4]

[ExE Boss]

On Mon, 21 Nov 2022 13:43:56 GMT, Per Minborg  wrote:

>> src/java.base/share/classes/sun/nio/ch/DirectBuffer.java line 41:
>> 
>>> 39: // An example of a guarded use of a memory address is shown here:
>>> 40: //
>>> 41: // try (var guard = NIO_ACCESS.acquireSessionAsAutoCloseable(bb)) {
>> 
>> Suggestion:
>> 
>> // try (var guard = NIO_ACCESS.acquireSession(bb)) {
>
> Are you happy with the proposed renaming now @ExE-Boss ? I think our 
> propositions crossed in time?

This change was missed in commit 
.

-

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v4]

[Per Minborg]

On Mon, 21 Nov 2022 13:32:43 GMT, ExE Boss  wrote:

>> Per Minborg has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Rename methods
>
> src/java.base/share/classes/sun/nio/ch/DirectBuffer.java line 41:
> 
>> 39: // An example of a guarded use of a memory address is shown here:
>> 40: //
>> 41: // try (var guard = NIO_ACCESS.acquireSessionAsAutoCloseable(bb)) {
> 
> Suggestion:
> 
> // try (var guard = NIO_ACCESS.acquireSession(bb)) {

Are you happy with the proposed renaming now @ExE-Boss ? I think our 
propositions crossed in time?

-

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v5]

[Per Minborg]

> This PR proposes the introduction of **guarding** of the use of 
> `DirectBuffer::address` within the JDK. With the introduction of the Foreign 
> Function and Memory access, it is possible to derive Buffer instances that 
> are backed by native memory that, in turn, can be closed asynchronously by 
> the user (and not only via a `Cleaner` when there is no other reference to 
> the `Buffer` object). If another thread is invoking `MemorySession::close` 
> while a thread is doing work using raw addresses, the outcome is undefined. 
> This means the JVM might crash or even worse, silent modification of 
> unrelated memory might occur. 
> 
> Design choices in this PR:
> 
> There is already a method `MemorySession::whileAlive` that takes a runnable 
> and that will perform the provided action while acquiring the underlying` 
> MemorySession` (if any). However, using this method entailed relatively large 
> changes while converting larger/nested code segments into lambdas. This would 
> also risk introducing lambda capturing. So, instead, a try-with-resources 
> friendly access method was added. This made is more easy to add guarding and 
> did not risk lambda capturing. Also, introducing lambdas in certain 
> fundamental JDK classes might incur bootstrap problems.
> 
> The aforementioned TwR is using a "session acquisition" that is not used 
> explicitly in the try block itself. This raises a warning that is suppressed 
> using `@SuppressWarnings("try")`. In the future, we might be able to remove 
> these suppressions by using the reserved variable name `_`. Another 
> alternative was evaluated where a non-autocloseable resource was used. 
> However, it became relatively complicated to guarantee that the session was 
> always released and, at the same time, the existing 'final` block was always 
> executed properly (as they both might throw exceptions). In the end, the 
> proposed solution was much simpler and robust despite requiring a 
> non-referenced TwR variable.
> 
> In some cases, where is is guaranteed that the backing memory session is 
> non-closeable, we do not have to guard the use of `DirectBuffer::address`. 
> ~~These cases have been documented in the code.~~
> 
> On some occasions, a plurality of acquisitions are made. This would never 
> lead to deadlocks as acquisitions are fundamentally concurrent counters and 
> not resources that only one thread can "own".
> 
> I have added comments (and not javadocs) before the declaration of the 
> non-public-api `DirectBuffer::address` method, that the use of the returned 
> address needs to be guarded. It can be discussed if this is preferable or not.
> 
> This PR spawns several areas of responsibility and so, I expect more than one 
> reviewer before promoting the PR.

Per Minborg has updated the pull request incrementally with one additional 
commit since the last revision:

  Cleanup static declarations

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/11260/files
  - new: https://git.openjdk.org/jdk/pull/11260/files/c081b4ae..eca7c388

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=11260=04
 - incr: https://webrevs.openjdk.org/?repo=jdk=11260=03-04

  Stats: 4 lines in 4 files changed: 0 ins; 0 del; 4 mod
  Patch: https://git.openjdk.org/jdk/pull/11260.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/11260/head:pull/11260

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v4]

[ExE Boss]

On Mon, 21 Nov 2022 13:02:48 GMT, Per Minborg  wrote:

>> This PR proposes the introduction of **guarding** of the use of 
>> `DirectBuffer::address` within the JDK. With the introduction of the Foreign 
>> Function and Memory access, it is possible to derive Buffer instances that 
>> are backed by native memory that, in turn, can be closed asynchronously by 
>> the user (and not only via a `Cleaner` when there is no other reference to 
>> the `Buffer` object). If another thread is invoking `MemorySession::close` 
>> while a thread is doing work using raw addresses, the outcome is undefined. 
>> This means the JVM might crash or even worse, silent modification of 
>> unrelated memory might occur. 
>> 
>> Design choices in this PR:
>> 
>> There is already a method `MemorySession::whileAlive` that takes a runnable 
>> and that will perform the provided action while acquiring the underlying` 
>> MemorySession` (if any). However, using this method entailed relatively 
>> large changes while converting larger/nested code segments into lambdas. 
>> This would also risk introducing lambda capturing. So, instead, a 
>> try-with-resources friendly access method was added. This made is more easy 
>> to add guarding and did not risk lambda capturing. Also, introducing lambdas 
>> in certain fundamental JDK classes might incur bootstrap problems.
>> 
>> The aforementioned TwR is using a "session acquisition" that is not used 
>> explicitly in the try block itself. This raises a warning that is suppressed 
>> using `@SuppressWarnings("try")`. In the future, we might be able to remove 
>> these suppressions by using the reserved variable name `_`. Another 
>> alternative was evaluated where a non-autocloseable resource was used. 
>> However, it became relatively complicated to guarantee that the session was 
>> always released and, at the same time, the existing 'final` block was always 
>> executed properly (as they both might throw exceptions). In the end, the 
>> proposed solution was much simpler and robust despite requiring a 
>> non-referenced TwR variable.
>> 
>> In some cases, where is is guaranteed that the backing memory session is 
>> non-closeable, we do not have to guard the use of `DirectBuffer::address`. 
>> ~~These cases have been documented in the code.~~
>> 
>> On some occasions, a plurality of acquisitions are made. This would never 
>> lead to deadlocks as acquisitions are fundamentally concurrent counters and 
>> not resources that only one thread can "own".
>> 
>> I have added comments (and not javadocs) before the declaration of the 
>> non-public-api `DirectBuffer::address` method, that the use of the returned 
>> address needs to be guarded. It can be discussed if this is preferable or 
>> not.
>> 
>> This PR spawns several areas of responsibility and so, I expect more than 
>> one reviewer before promoting the PR.
>
> Per Minborg has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Rename methods

src/java.base/share/classes/jdk/internal/access/JavaNioAccess.java line 94:

> 92:  * required to guarantee safety.
> 93:  * {@snippet lang = java:
> 94:  * var handler = acquireSessionOrNoOp(buffer);

Suggestion:

 * var handler = acquireSessionOrNull(buffer, async);

src/java.base/share/classes/sun/nio/ch/DirectBuffer.java line 41:

> 39: // An example of a guarded use of a memory address is shown here:
> 40: //
> 41: // try (var guard = NIO_ACCESS.acquireSessionAsAutoCloseable(bb)) {

Suggestion:

// try (var guard = NIO_ACCESS.acquireSession(bb)) {

-

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v4]

[Per Minborg]

> This PR proposes the introduction of **guarding** of the use of 
> `DirectBuffer::address` within the JDK. With the introduction of the Foreign 
> Function and Memory access, it is possible to derive Buffer instances that 
> are backed by native memory that, in turn, can be closed asynchronously by 
> the user (and not only via a `Cleaner` when there is no other reference to 
> the `Buffer` object). If another thread is invoking `MemorySession::close` 
> while a thread is doing work using raw addresses, the outcome is undefined. 
> This means the JVM might crash or even worse, silent modification of 
> unrelated memory might occur. 
> 
> Design choices in this PR:
> 
> There is already a method `MemorySession::whileAlive` that takes a runnable 
> and that will perform the provided action while acquiring the underlying` 
> MemorySession` (if any). However, using this method entailed relatively large 
> changes while converting larger/nested code segments into lambdas. This would 
> also risk introducing lambda capturing. So, instead, a try-with-resources 
> friendly access method was added. This made is more easy to add guarding and 
> did not risk lambda capturing. Also, introducing lambdas in certain 
> fundamental JDK classes might incur bootstrap problems.
> 
> The aforementioned TwR is using a "session acquisition" that is not used 
> explicitly in the try block itself. This raises a warning that is suppressed 
> using `@SuppressWarnings("try")`. In the future, we might be able to remove 
> these suppressions by using the reserved variable name `_`. Another 
> alternative was evaluated where a non-autocloseable resource was used. 
> However, it became relatively complicated to guarantee that the session was 
> always released and, at the same time, the existing 'final` block was always 
> executed properly (as they both might throw exceptions). In the end, the 
> proposed solution was much simpler and robust despite requiring a 
> non-referenced TwR variable.
> 
> In some cases, where is is guaranteed that the backing memory session is 
> non-closeable, we do not have to guard the use of `DirectBuffer::address`. 
> ~~These cases have been documented in the code.~~
> 
> On some occasions, a plurality of acquisitions are made. This would never 
> lead to deadlocks as acquisitions are fundamentally concurrent counters and 
> not resources that only one thread can "own".
> 
> I have added comments (and not javadocs) before the declaration of the 
> non-public-api `DirectBuffer::address` method, that the use of the returned 
> address needs to be guarded. It can be discussed if this is preferable or not.
> 
> This PR spawns several areas of responsibility and so, I expect more than one 
> reviewer before promoting the PR.

Per Minborg has updated the pull request incrementally with one additional 
commit since the last revision:

  Rename methods

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/11260/files
  - new: https://git.openjdk.org/jdk/pull/11260/files/9fcf2bb3..c081b4ae

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=11260=03
 - incr: https://webrevs.openjdk.org/?repo=jdk=11260=02-03

  Stats: 51 lines in 20 files changed: 0 ins; 6 del; 45 mod
  Patch: https://git.openjdk.org/jdk/pull/11260.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/11260/head:pull/11260

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: JDK-8285932 Implementation of JEP 430 String Templates (Preview) [v26]

[Jim Laskey]

On Fri, 18 Nov 2022 23:00:49 GMT, Roger Riggs  wrote:

>> Jim Laskey has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Typo
>
> src/java.base/share/classes/java/lang/template/StringTemplate.java line 35:
> 
>> 33: /**
>> 34:  * {@link StringTemplate} is the run-time representation of a string 
>> template or
>> 35:  * text block template in a template expression.
> 
> Can the link to the JLS 15.8.6 be included earlier so the definition of 
> "template expression" isn't hanging until the end of the class javadoc.

Didn't know that it was positional. Will change.

-

PR: https://git.openjdk.org/jdk/pull/10889

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v3]

[Per Minborg]

On Mon, 21 Nov 2022 12:19:21 GMT, Alan Bateman  wrote:

>> maybe just `bufferLock` and and just `acquireBuffer` ?
>
> Would it be possible to re-examine acquireSession returning Runnable and 
> acquireSessionAsAutoCloseable returning AutoCloseable. The naming is bit 
> awkward at most of the use sites and maybe we can do better.

I think it would be confusing to have overloads with the same name. 

The `aquireSession(Buffer, boolean)` method is only used once and is used in 
`IOUtil` so we could rename it to `aquireSessionOrNull` and then rename 
`acquireSessionAsAutoCloseable` to `acquireSession`.

-

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: JDK-8285932 Implementation of JEP 430 String Templates (Preview) [v26]

[Jim Laskey]

On Fri, 18 Nov 2022 23:05:30 GMT, Roger Riggs  wrote:

>> Jim Laskey has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Typo
>
> src/java.base/share/classes/java/lang/template/StringProcessor.java line 49:
> 
>> 47:  * @implNote Implementations using {@link StringProcessor} are 
>> equivalent to implementations using
>> 48:  * {@code TemplateProcessor} or {@code 
>> ValidatingProcessor},
>> 49:  * however the dominance of {@link String} producing template processors 
>> supercedes the redundancy.
> 
> Amusing to read...  
> Perhaps just,  "however, StringProcessor is clearer and easier to understand".

Changing.

> src/java.base/share/classes/java/lang/template/StringTemplate.java line 115:
> 
>> 113: public interface StringTemplate {
>> 114: /**
>> 115:  * Returns s list of fragment literals for this {@link 
>> StringTemplate}.
> 
> Suggestion:
> 
>  * Returns a list of fragment literals for this {@link StringTemplate}.

Changing.

> src/java.base/share/classes/java/lang/template/StringTemplate.java line 155:
> 
>> 153:  * For better visibility and when practical, it is recommended that 
>> users use the
>> 154:  * {@link StringTemplate#STR} processor instead of invoking the
>> 155:  * {@link StringTemplate#interpolate()} method directly.
> 
> Removing "users use"...
> 
> Suggestion:
> 
>  * For better visibility and when practical, it is recommended to use the
>  * {@link StringTemplate#STR} processor instead of invoking the
>  * {@link StringTemplate#interpolate()} method.

Changing

> src/java.base/share/classes/java/lang/template/StringTemplate.java line 206:
> 
>> 204:  * @param stringTemplate  the {@link StringTemplate} to represent
>> 205:  *
>> 206:  * @return diagnostic string representing the supplied templated 
>> string
> 
> Suggestion:
> 
>  * @return diagnostic string representing the supplied string template

Changing.

> src/java.base/share/classes/java/lang/template/StringTemplate.java line 317:
> 
>> 315: /**
>> 316:  * The {@link StringProcessor} instance conventionally used for the 
>> string interpolation
>> 317:  * of a supplied {@link StringTemplate}.
> 
> Suggestion:
> 
>  * The {@link StringProcessor} instance is conventionally used for the 
> string interpolation
>  * of a supplied {@link StringTemplate}.

Same issue as `STR`.

> src/java.base/share/classes/java/lang/template/StringTemplate.java line 334:
> 
>> 332: /**
>> 333:  * The {@link TemplateProcessor} instance conventionally used to 
>> indicate that the
>> 334:  * processing of the {@link StringTemplate} is to be deferred to a 
>> later time. Deferred
> 
> Suggestion:
> 
>  * The {@link TemplateProcessor} instance is conventionally used to 
> indicate that the
>  * processing of the {@link StringTemplate} is to be deferred to a later 
> time. Deferred

Interestingly enough, someone asked me to remove the 'is'. If you leave the 
'is' in it read as 'The TemplateProcessor instance is used to indicate that 
the' vs 'The TemplateProcessor instance used to indicate that the' which makes 
more sense. Maybe 'This TemplateProcessor instance is used to indicate that 
the'. Changing.

-

PR: https://git.openjdk.org/jdk/pull/10889

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v3]

[Per Minborg]

> This PR proposes the introduction of **guarding** of the use of 
> `DirectBuffer::address` within the JDK. With the introduction of the Foreign 
> Function and Memory access, it is possible to derive Buffer instances that 
> are backed by native memory that, in turn, can be closed asynchronously by 
> the user (and not only via a `Cleaner` when there is no other reference to 
> the `Buffer` object). If another thread is invoking `MemorySession::close` 
> while a thread is doing work using raw addresses, the outcome is undefined. 
> This means the JVM might crash or even worse, silent modification of 
> unrelated memory might occur. 
> 
> Design choices in this PR:
> 
> There is already a method `MemorySession::whileAlive` that takes a runnable 
> and that will perform the provided action while acquiring the underlying` 
> MemorySession` (if any). However, using this method entailed relatively large 
> changes while converting larger/nested code segments into lambdas. This would 
> also risk introducing lambda capturing. So, instead, a try-with-resources 
> friendly access method was added. This made is more easy to add guarding and 
> did not risk lambda capturing. Also, introducing lambdas in certain 
> fundamental JDK classes might incur bootstrap problems.
> 
> The aforementioned TwR is using a "session acquisition" that is not used 
> explicitly in the try block itself. This raises a warning that is suppressed 
> using `@SuppressWarnings("try")`. In the future, we might be able to remove 
> these suppressions by using the reserved variable name `_`. Another 
> alternative was evaluated where a non-autocloseable resource was used. 
> However, it became relatively complicated to guarantee that the session was 
> always released and, at the same time, the existing 'final` block was always 
> executed properly (as they both might throw exceptions). In the end, the 
> proposed solution was much simpler and robust despite requiring a 
> non-referenced TwR variable.
> 
> In some cases, where is is guaranteed that the backing memory session is 
> non-closeable, we do not have to guard the use of `DirectBuffer::address`. 
> ~~These cases have been documented in the code.~~
> 
> On some occasions, a plurality of acquisitions are made. This would never 
> lead to deadlocks as acquisitions are fundamentally concurrent counters and 
> not resources that only one thread can "own".
> 
> I have added comments (and not javadocs) before the declaration of the 
> non-public-api `DirectBuffer::address` method, that the use of the returned 
> address needs to be guarded. It can be discussed if this is preferable or not.
> 
> This PR spawns several areas of responsibility and so, I expect more than one 
> reviewer before promoting the PR.

Per Minborg has updated the pull request incrementally with one additional 
commit since the last revision:

  Remove comments

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/11260/files
  - new: https://git.openjdk.org/jdk/pull/11260/files/9fafafac..9fcf2bb3

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=11260=02
 - incr: https://webrevs.openjdk.org/?repo=jdk=11260=01-02

  Stats: 18 lines in 6 files changed: 4 ins; 14 del; 0 mod
  Patch: https://git.openjdk.org/jdk/pull/11260.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/11260/head:pull/11260

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v2]

[Maurizio Cimadamore]

On Mon, 21 Nov 2022 12:03:35 GMT, Per Minborg  wrote:

>> src/java.base/share/classes/java/util/zip/Adler32.java line 102:
>> 
>>> 100: return;
>>> 101: if (buffer.isDirect()) {
>>> 102: try (var sessionAcquisition = 
>>> NIO_ACCESS.acquireSessionAsAutoCloseable(buffer)) {
>> 
>> We need to find something better than "sessionAcquisition", it looks very 
>> messy at all these use sites.
>
> Eventually, I hope most of them will be named `_`.

maybe just `bufferLock` and and just `acquireBuffer` ?

-

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v2]

[Per Minborg]

> This PR proposes the introduction of **guarding** of the use of 
> `DirectBuffer::address` within the JDK. With the introduction of the Foreign 
> Function and Memory access, it is possible to derive Buffer instances that 
> are backed by native memory that, in turn, can be closed asynchronously by 
> the user (and not only via a `Cleaner` when there is no other reference to 
> the `Buffer` object). If another thread is invoking `MemorySession::close` 
> while a thread is doing work using raw addresses, the outcome is undefined. 
> This means the JVM might crash or even worse, silent modification of 
> unrelated memory might occur. 
> 
> Design choices in this PR:
> 
> There is already a method `MemorySession::whileAlive` that takes a runnable 
> and that will perform the provided action while acquiring the underlying` 
> MemorySession` (if any). However, using this method entailed relatively large 
> changes while converting larger/nested code segments into lambdas. This would 
> also risk introducing lambda capturing. So, instead, a try-with-resources 
> friendly access method was added. This made is more easy to add guarding and 
> did not risk lambda capturing. Also, introducing lambdas in certain 
> fundamental JDK classes might incur bootstrap problems.
> 
> The aforementioned TwR is using a "session acquisition" that is not used 
> explicitly in the try block itself. This raises a warning that is suppressed 
> using `@SuppressWarnings("try")`. In the future, we might be able to remove 
> these suppressions by using the reserved variable name `_`. Another 
> alternative was evaluated where a non-autocloseable resource was used. 
> However, it became relatively complicated to guarantee that the session was 
> always released and, at the same time, the existing 'final` block was always 
> executed properly (as they both might throw exceptions). In the end, the 
> proposed solution was much simpler and robust despite requiring a 
> non-referenced TwR variable.
> 
> In some cases, where is is guaranteed that the backing memory session is 
> non-closeable, we do not have to guard the use of `DirectBuffer::address`. 
> These cases have been documented in the code.
> 
> On some occasions, a plurality of acquisitions are made. This would never 
> lead to deadlocks as acquisitions are fundamentally concurrent counters and 
> not resources that only one thread can "own".
> 
> I have added comments (and not javadocs) before the declaration of the 
> non-public-api `DirectBuffer::address` method, that the use of the returned 
> address needs to be guarded. It can be discussed if this is preferable or not.
> 
> This PR spawns several areas of responsibility and so, I expect more than one 
> reviewer before promoting the PR.

Per Minborg has updated the pull request incrementally with one additional 
commit since the last revision:

  Cleanup after comments

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/11260/files
  - new: https://git.openjdk.org/jdk/pull/11260/files/23a6fd14..9fafafac

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=11260=01
 - incr: https://webrevs.openjdk.org/?repo=jdk=11260=00-01

  Stats: 50 lines in 20 files changed: 6 ins; 0 del; 44 mod
  Patch: https://git.openjdk.org/jdk/pull/11260.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/11260/head:pull/11260

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v2]

[Alan Bateman]

On Mon, 21 Nov 2022 12:16:01 GMT, Maurizio Cimadamore  
wrote:

>> Eventually, I hope most of them will be named `_`.
>
> maybe just `bufferLock` and and just `acquireBuffer` ?

Would it be possible to re-examine acquireSession returning Runnable and 
acquireSessionAsAutoCloseable returning AutoCloseable. The naming is awkward at 
most of the use sites and maybe we can do better.

-

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded

[Alan Bateman]

On Mon, 21 Nov 2022 11:36:44 GMT, Per Minborg  wrote:

>> src/java.base/share/classes/sun/nio/ch/NioSocketImpl.java line 252:
>> 
>>> 250: try {
>>> 251: // 'dst' is guaranteed not to be associated with a 
>>> closeable session.
>>> 252: // Hence, there is no need for acquiring any session.
>> 
>> This comment is will be confusing to anyone reading this code. Is this 
>> really needed?
>
> The reason for the comment is to make it clear why `DirectBuffer::address` 
> can be used directly without guarding. This will also reduce the probability 
> of unnecessary guarding being added in the future. However, if the consensus 
> is that these comments just adds confusion, I am happy to remove them.

I'd prefer to see this comment removed from all places that are obviously 
interacting with the direct buffer cache. These usages are try-finally to 
acquire and return the temporary direct buffer cache back to the cache. Talking 
about closable sessions here is definitely confusing.

-

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded

[Per Minborg]

On Mon, 21 Nov 2022 11:32:35 GMT, Alan Bateman  wrote:

>> This PR proposes the introduction of **guarding** of the use of 
>> `DirectBuffer::address` within the JDK. With the introduction of the Foreign 
>> Function and Memory access, it is possible to derive Buffer instances that 
>> are backed by native memory that, in turn, can be closed asynchronously by 
>> the user (and not only via a `Cleaner` when there is no other reference to 
>> the `Buffer` object). If another thread is invoking `MemorySession::close` 
>> while a thread is doing work using raw addresses, the outcome is undefined. 
>> This means the JVM might crash or even worse, silent modification of 
>> unrelated memory might occur. 
>> 
>> Design choices in this PR:
>> 
>> There is already a method `MemorySession::whileAlive` that takes a runnable 
>> and that will perform the provided action while acquiring the underlying` 
>> MemorySession` (if any). However, using this method entailed relatively 
>> large changes while converting larger/nested code segments into lambdas. 
>> This would also risk introducing lambda capturing. So, instead, a 
>> try-with-resources friendly access method was added. This made is more easy 
>> to add guarding and did not risk lambda capturing. Also, introducing lambdas 
>> in certain fundamental JDK classes might incur bootstrap problems.
>> 
>> The aforementioned TwR is using a "session acquisition" that is not used 
>> explicitly in the try block itself. This raises a warning that is suppressed 
>> using `@SuppressWarnings("try")`. In the future, we might be able to remove 
>> these suppressions by using the reserved variable name `_`. Another 
>> alternative was evaluated where a non-autocloseable resource was used. 
>> However, it became relatively complicated to guarantee that the session was 
>> always released and, at the same time, the existing 'final` block was always 
>> executed properly (as they both might throw exceptions). In the end, the 
>> proposed solution was much simpler and robust despite requiring a 
>> non-referenced TwR variable.
>> 
>> In some cases, where is is guaranteed that the backing memory session is 
>> non-closeable, we do not have to guard the use of `DirectBuffer::address`. 
>> These cases have been documented in the code.
>> 
>> On some occasions, a plurality of acquisitions are made. This would never 
>> lead to deadlocks as acquisitions are fundamentally concurrent counters and 
>> not resources that only one thread can "own".
>> 
>> I have added comments (and not javadocs) before the declaration of the 
>> non-public-api `DirectBuffer::address` method, that the use of the returned 
>> address needs to be guarded. It can be discussed if this is preferable or 
>> not.
>> 
>> This PR spawns several areas of responsibility and so, I expect more than 
>> one reviewer before promoting the PR.
>
> src/java.base/share/classes/java/util/zip/Adler32.java line 102:
> 
>> 100: return;
>> 101: if (buffer.isDirect()) {
>> 102: try (var sessionAcquisition = 
>> NIO_ACCESS.acquireSessionAsAutoCloseable(buffer)) {
> 
> We need to find something better than "sessionAcquisition", it looks very 
> messy at all these use sites.

Eventually, I hope most of them will be named `_`.

-

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded

[Per Minborg]

On Mon, 21 Nov 2022 11:29:07 GMT, Alan Bateman  wrote:

>> This PR proposes the introduction of **guarding** of the use of 
>> `DirectBuffer::address` within the JDK. With the introduction of the Foreign 
>> Function and Memory access, it is possible to derive Buffer instances that 
>> are backed by native memory that, in turn, can be closed asynchronously by 
>> the user (and not only via a `Cleaner` when there is no other reference to 
>> the `Buffer` object). If another thread is invoking `MemorySession::close` 
>> while a thread is doing work using raw addresses, the outcome is undefined. 
>> This means the JVM might crash or even worse, silent modification of 
>> unrelated memory might occur. 
>> 
>> Design choices in this PR:
>> 
>> There is already a method `MemorySession::whileAlive` that takes a runnable 
>> and that will perform the provided action while acquiring the underlying` 
>> MemorySession` (if any). However, using this method entailed relatively 
>> large changes while converting larger/nested code segments into lambdas. 
>> This would also risk introducing lambda capturing. So, instead, a 
>> try-with-resources friendly access method was added. This made is more easy 
>> to add guarding and did not risk lambda capturing. Also, introducing lambdas 
>> in certain fundamental JDK classes might incur bootstrap problems.
>> 
>> The aforementioned TwR is using a "session acquisition" that is not used 
>> explicitly in the try block itself. This raises a warning that is suppressed 
>> using `@SuppressWarnings("try")`. In the future, we might be able to remove 
>> these suppressions by using the reserved variable name `_`. Another 
>> alternative was evaluated where a non-autocloseable resource was used. 
>> However, it became relatively complicated to guarantee that the session was 
>> always released and, at the same time, the existing 'final` block was always 
>> executed properly (as they both might throw exceptions). In the end, the 
>> proposed solution was much simpler and robust despite requiring a 
>> non-referenced TwR variable.
>> 
>> In some cases, where is is guaranteed that the backing memory session is 
>> non-closeable, we do not have to guard the use of `DirectBuffer::address`. 
>> These cases have been documented in the code.
>> 
>> On some occasions, a plurality of acquisitions are made. This would never 
>> lead to deadlocks as acquisitions are fundamentally concurrent counters and 
>> not resources that only one thread can "own".
>> 
>> I have added comments (and not javadocs) before the declaration of the 
>> non-public-api `DirectBuffer::address` method, that the use of the returned 
>> address needs to be guarded. It can be discussed if this is preferable or 
>> not.
>> 
>> This PR spawns several areas of responsibility and so, I expect more than 
>> one reviewer before promoting the PR.
>
> src/java.base/share/classes/sun/nio/ch/NioSocketImpl.java line 252:
> 
>> 250: try {
>> 251: // 'dst' is guaranteed not to be associated with a 
>> closeable session.
>> 252: // Hence, there is no need for acquiring any session.
> 
> This comment is will be confusing to anyone reading this code. Is this really 
> needed?

The reason for the comment is to make it clear why `DirectBuffer::address` can 
be used directly without guarding. This will also reduce the probability of 
unnecessary guarding being added in the future. However, if the consensus is 
that these comments just adds confusion, I am happy to remove them.

-

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded

[Alan Bateman]

On Mon, 21 Nov 2022 10:53:07 GMT, Per Minborg  wrote:

> This PR proposes the introduction of **guarding** of the use of 
> `DirectBuffer::address` within the JDK. With the introduction of the Foreign 
> Function and Memory access, it is possible to derive Buffer instances that 
> are backed by native memory that, in turn, can be closed asynchronously by 
> the user (and not only via a `Cleaner` when there is no other reference to 
> the `Buffer` object). If another thread is invoking `MemorySession::close` 
> while a thread is doing work using raw addresses, the outcome is undefined. 
> This means the JVM might crash or even worse, silent modification of 
> unrelated memory might occur. 
> 
> Design choices in this PR:
> 
> There is already a method `MemorySession::whileAlive` that takes a runnable 
> and that will perform the provided action while acquiring the underlying` 
> MemorySession` (if any). However, using this method entailed relatively large 
> changes while converting larger/nested code segments into lambdas. This would 
> also risk introducing lambda capturing. So, instead, a try-with-resources 
> friendly access method was added. This made is more easy to add guarding and 
> did not risk lambda capturing. Also, introducing lambdas in certain 
> fundamental JDK classes might incur bootstrap problems.
> 
> The aforementioned TwR is using a "session acquisition" that is not used 
> explicitly in the try block itself. This raises a warning that is suppressed 
> using `@SuppressWarnings("try")`. In the future, we might be able to remove 
> these suppressions by using the reserved variable name `_`. Another 
> alternative was evaluated where a non-autocloseable resource was used. 
> However, it became relatively complicated to guarantee that the session was 
> always released and, at the same time, the existing 'final` block was always 
> executed properly (as they both might throw exceptions). In the end, the 
> proposed solution was much simpler and robust despite requiring a 
> non-referenced TwR variable.
> 
> In some cases, where is is guaranteed that the backing memory session is 
> non-closeable, we do not have to guard the use of `DirectBuffer::address`. 
> These cases have been documented in the code.
> 
> On some occasions, a plurality of acquisitions are made. This would never 
> lead to deadlocks as acquisitions are fundamentally concurrent counters and 
> not resources that only one thread can "own".
> 
> I have added comments (and not javadocs) before the declaration of the 
> non-public-api `DirectBuffer::address` method, that the use of the returned 
> address needs to be guarded. It can be discussed if this is preferable or not.
> 
> This PR spawns several areas of responsibility and so, I expect more than one 
> reviewer before promoting the PR.

src/java.base/share/classes/java/util/zip/Adler32.java line 102:

> 100: return;
> 101: if (buffer.isDirect()) {
> 102: try (var sessionAcquisition = 
> NIO_ACCESS.acquireSessionAsAutoCloseable(buffer)) {

We need to find something better than "sessionAcquisition", it looks very messy 
at all these use sites.

-

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded

[Alan Bateman]

On Mon, 21 Nov 2022 10:53:07 GMT, Per Minborg  wrote:

> This PR proposes the introduction of **guarding** of the use of 
> `DirectBuffer::address` within the JDK. With the introduction of the Foreign 
> Function and Memory access, it is possible to derive Buffer instances that 
> are backed by native memory that, in turn, can be closed asynchronously by 
> the user (and not only via a `Cleaner` when there is no other reference to 
> the `Buffer` object). If another thread is invoking `MemorySession::close` 
> while a thread is doing work using raw addresses, the outcome is undefined. 
> This means the JVM might crash or even worse, silent modification of 
> unrelated memory might occur. 
> 
> Design choices in this PR:
> 
> There is already a method `MemorySession::whileAlive` that takes a runnable 
> and that will perform the provided action while acquiring the underlying` 
> MemorySession` (if any). However, using this method entailed relatively large 
> changes while converting larger/nested code segments into lambdas. This would 
> also risk introducing lambda capturing. So, instead, a try-with-resources 
> friendly access method was added. This made is more easy to add guarding and 
> did not risk lambda capturing. Also, introducing lambdas in certain 
> fundamental JDK classes might incur bootstrap problems.
> 
> The aforementioned TwR is using a "session acquisition" that is not used 
> explicitly in the try block itself. This raises a warning that is suppressed 
> using `@SuppressWarnings("try")`. In the future, we might be able to remove 
> these suppressions by using the reserved variable name `_`. Another 
> alternative was evaluated where a non-autocloseable resource was used. 
> However, it became relatively complicated to guarantee that the session was 
> always released and, at the same time, the existing 'final` block was always 
> executed properly (as they both might throw exceptions). In the end, the 
> proposed solution was much simpler and robust despite requiring a 
> non-referenced TwR variable.
> 
> In some cases, where is is guaranteed that the backing memory session is 
> non-closeable, we do not have to guard the use of `DirectBuffer::address`. 
> These cases have been documented in the code.
> 
> On some occasions, a plurality of acquisitions are made. This would never 
> lead to deadlocks as acquisitions are fundamentally concurrent counters and 
> not resources that only one thread can "own".
> 
> I have added comments (and not javadocs) before the declaration of the 
> non-public-api `DirectBuffer::address` method, that the use of the returned 
> address needs to be guarded. It can be discussed if this is preferable or not.
> 
> This PR spawns several areas of responsibility and so, I expect more than one 
> reviewer before promoting the PR.

src/java.base/share/classes/java/nio/Buffer.java line 785:

> 783: static final JavaNioAccess.SessionAcquisition 
> NO_OP_CLOSE = new JavaNioAccess.SessionAcquisition() {
> 784: @Override
> 785: public void close() throws RuntimeException {}

The throws RuntimeException is not needed here. Also would it be possible to 
reflow the comment to avoid the wildly long line.

src/java.base/share/classes/jdk/internal/access/JavaNioAccess.java line 161:

> 159: 
> 160: @Override
> 161: void close() throws RuntimeException;

You can drop throws RuntimeEXcepton here too.

src/java.base/share/classes/sun/nio/ch/DirectBuffer.java line 38:

> 36: // try (var sessionAcquisition = 
> NIO_ACCESS.acquireSessionAsAutoCloseable(bb)) {
> 37: // performOperation(bb.address());
> 38: // }

This comment is very messy and needs to be cleaned up.

src/java.base/share/classes/sun/nio/ch/NioSocketImpl.java line 252:

> 250: try {
> 251: // 'dst' is guaranteed not to be associated with a closeable 
> session.
> 252: // Hence, there is no need for acquiring any session.

This comment is will be confusing to anyone reading this code. Is this really 
needed?

-

PR: https://git.openjdk.org/jdk/pull/11260

RFR: 8296024: Usage of DIrectBuffer::address should be guarded

[Per Minborg]

This PR proposes the introduction of **guarding** of the use of 
`DirectBuffer::address` within the JDK. With the introduction of the Foreign 
Function and Memory access, it is possible to derive Buffer instances that are 
backed by native memory that, in turn, can be closed asynchronously by the user 
(and not only via a `Cleaner` when there is no other reference to the `Buffer` 
object). If another thread is invoking `MemorySession::close` while a thread is 
doing work using raw addresses, the outcome is undefined. This means the JVM 
might crash or even worse, silent modification of unrelated memory might occur. 

Design choices in this PR:

There is already a method `MemorySession::whileAlive` that takes a runnable and 
that will perform the provided action while acquiring the underlying` 
MemorySession` (if any). However, using this method entailed relatively large 
changes while converting larger/nested code segments into lambdas. This would 
also risk introducing lambda capturing. So, instead, a try-with-resources 
friendly access method was added. This made is more easy to add guarding and 
did not risk lambda capturing. Also, introducing lambdas in certain fundamental 
JDK classes might incur bootstrap problems.

The aforementioned TwR is using a "session acquisition" that is not used 
explicitly in the try block itself. This raises a warning that is suppressed 
using `@SuppressWarnings("try")`. In the future, we might be able to remove 
these suppressions by using the reserved variable name `_`. Another alternative 
was evaluated where a non-autocloseable resource was used. However, it became 
relatively complicated to guarantee that the session was always released and, 
at the same time, the existing 'final` block was always executed properly (as 
they both might throw exceptions). In the end, the proposed solution was much 
simpler and robust despite requiring a non-referenced TwR variable.

In some cases, where is is guaranteed that the backing memory session is 
non-closeable, we do not have to guard the use of `DirectBuffer::address`. 
These cases have been documented in the code.

On some occasions, a plurality of acquisitions are made. This would never lead 
to deadlocks as acquisitions are fundamentally concurrent counters and not 
resources that only one thread can "own".

I have added comments (and not javadocs) before the declaration of the 
non-public-api `DirectBuffer::address` method, that the use of the returned 
address needs to be guarded. It can be discussed if this is preferable or not.

This PR spawns several areas of responsibility and so, I expect more than one 
reviewer before promoting the PR.

-

Commit messages:
 - Guard usages of DirectBuffer::address

Changes: https://git.openjdk.org/jdk/pull/11260/files
 Webrev: https://webrevs.openjdk.org/?repo=jdk=11260=00
  Issue: https://bugs.openjdk.org/browse/JDK-8296024
  Stats: 277 lines in 26 files changed: 191 ins; 4 del; 82 mod
  Patch: https://git.openjdk.org/jdk/pull/11260.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/11260/head:pull/11260

PR: https://git.openjdk.org/jdk/pull/11260

Re: What are the policies for internal modules?

[Alan Bateman]

On 21/11/2022 01:02, Ethan McCue wrote:
There are some modules like jdk.internal.le which contain no 
publicly exported packages. In some of the experimentation we are 
doing, We want to use the 
jdk.internal.org.jline.utils.Levenshtien class from jdk.compiler.


Mechanically, we can do this without creating any new modules by 
adding a qualified export of the utils package to jdk.compiler. We 
could also make a brand new, tiny pointless module called 
jdk.internal.levenshtien. That would be the "cleanest" approach, but 
only if we didn't consider the existence of the internal modules to be 
public API.


So that's my general question - what is the bar for making a new 
internal module, and is the set of internal modules subject to 
backwards compatibility concerns?


There aren't many compatibility constraints on jdk.internal modules. 
Assume they can change or be removed in any release. It's important they 
don't export any packages to all modules, otherwise they cease to be 
"internal".  Also if they provide services then the name of the module 
may be something that users of jlink may become dependent on, so we have 
to be careful there.


A general point is that we don't want the JDK to backslide to a big ball 
of mud. We put a lot of effort several years ago to de-tangle the 
libraries so it would be disappointing see jdk.compiler, with no 
interest in line editing, add a dependency on jdk.internal.le so that it 
can make use of one of utility classes. So I don't think we should do that.


Can you say a bit more about what you are doing? Is this just a method 
to compute the Levenshtein distance between two strings? I assume you 
can just implement that in jdk.compiler without touching the module 
graph. It might be useful to get some sense on how common fuzzy matching 
is in the eco system to see if there is any merit to exposing APIs for this.


-Alan

Re: RFR: 8293041: --disable-@files option doesn't work and cause an error

[Adam Sotona]

On Mon, 21 Nov 2022 01:11:55 GMT, David Holmes  wrote:

> > tools/launcher/ArgsFileTest.java was working because it didn't contain any 
> > test with --disable-@files option verifying VM really starts
> 
> What about the killswitch test ??

KillSwitch test did not construct valid command line, it didn't expect VM to 
start correctly and so it didn't test the VM exit value. KillSwitch test just 
checked expanded options presence in the debug log (before passing to 
ParseArguments). 
I've added one simple test composing valid cmd line 'java --disable-@files 
--version' and verifying successful VM execution.

-

PR: https://git.openjdk.org/jdk/pull/11183