Re: RFR: 8312425: [vectorapi] AArch64: Optimize vector math operations with SLEEF [v4]

[Xiaohong Gong]

On Thu, 23 Nov 2023 14:05:51 GMT, Magnus Ihse Bursie  wrote:

>> Xiaohong Gong has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Address review comments in build system
>
> make/autoconf/lib-vmath.m4 line 70:
> 
>> 68: if test "x$SYSROOT" = "x" &&
>> 69:test "x${LIBSLEEF_FOUND}" = "xno"; then
>> 70:   PKG_CHECK_MODULES([LIBSLEEF], [sleef], [LIBSLEEF_FOUND=yes], 
>> [LIBSLEEF_FOUND=no])
> 
> Suggestion:
> 
>   PKG_CHECK_MODULES([SLEEF], [sleef], [LIBSLEEF_FOUND=yes], 
> [LIBSLEEF_FOUND=no])
> 
> 
> Otherwise `PKG_CHECK_MODULES` will set the variables  LIBSLEEF_CFLAGS and 
> LIBSLEEF_LIBS.

Keep using `LIBSLEEF`, as the cflags and libs are named with `LIBSLEEF_` 
prefix. Thanks!

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16234#discussion_r1410275161

Re: RFR: 8312425: [vectorapi] AArch64: Optimize vector math operations with SLEEF [v5]

[Xiaohong Gong]

> Currently the vector floating-point math APIs like 
> `VectorOperators.SIN/COS/TAN...` are not intrinsified on AArch64 platform, 
> which causes large performance gap on AArch64. Note that those APIs are 
> optimized by C2 compiler on X86 platforms by calling Intel's SVML code [1]. 
> To close the gap, we would like to optimize these APIs for AArch64 by calling 
> a third-party vector library called libsleef [2], which are available in 
> mainstream Linux distros (e.g. [3] [4]).
> 
> SLEEF supports multiple accuracies. To match Vector API's requirement and 
> implement the math ops on AArch64, we 1) call 1.0 ULP accuracy with FMA 
> instructions used stubs in libsleef for most of the operations by default, 
> and 2) add the vector calling convention to apply with the runtime calls to 
> stub code in libsleef. Note that for those APIs that libsleef does not 
> support 1.0 ULP, we choose 0.5 ULP instead.
> 
> To help loading the expected libsleef library, this patch also adds an 
> experimental JVM option (i.e. `-XX:UseSleefLib`) for AArch64 platforms. 
> People can use it to denote the libsleef path/name explicitly. By default, it 
> points to the system installed library. If the library does not exist or the 
> dynamic loading of it in runtime fails, the math vector ops will fall-back to 
> use the default scalar version without error. But a warning is printed out if 
> people specifies a nonexistent library explicitly.
> 
> Note that this is a part of the original proposed patch in panama-dev [5], 
> just with some initial review comments addressed. And now we'd like to get 
> some wider feedbacks from more hotspot experts.
> 
> [1] https://github.com/openjdk/jdk/pull/3638
> [2] https://sleef.org/
> [3] https://packages.fedoraproject.org/pkgs/sleef/sleef/
> [4] https://packages.debian.org/bookworm/libsleef3
> [5] https://mail.openjdk.org/pipermail/panama-dev/2022-December/018172.html

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

  Rename vmath to sleef in configure

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/16234/files
  - new: https://git.openjdk.org/jdk/pull/16234/files/2c3c4a64..c1ce1968

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=16234&range=04
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16234&range=03-04

  Stats: 268 lines in 6 files changed: 137 ins; 126 del; 5 mod
  Patch: https://git.openjdk.org/jdk/pull/16234.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/16234/head:pull/16234

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

Re: RFR: 8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is trusted

[jmehrens]

On Wed, 29 Nov 2023 22:38:59 GMT, Markus KARG  wrote:

>> src/java.base/share/classes/java/io/BufferedInputStream.java line 647:
>> 
>>> 645: if (avail > 0) {
>>> 646: // trust all OutputStreams from java.io
>>> 647: if (out.getClass().getPackageName() == 
>>> BufferedInputStream.class.getPackageName()) {
>> 
>> I don't think Class::getPackageName documents that the returned String is 
>> intern so I wonder if the == check will lead to questions and suggestions of 
>> a bug. Classes with names starting with "java." can only be defined to the 
>> boot or platform class loader (details in the ClassLoader API docs) so you 
>> could just check if the package name equals "java.io".
>
> Do we only want to trust java.io or anything starting with java.*?

I don't think checking if the package is java.io is secure:

ByteArrayInputStream bais = new ByteArrayInputStream(bytes);
BufferedInputStream bis = new BufferedInputStream(bais);
UntrustedOutputStream uos = new UntrustedOutputStream();
bis.transferTo(new java.io.DataOutputStream(uos)); 

You have to know that it is in the java.io package and it doesn't wrap another 
stream.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16879#discussion_r1410142200

Re: RFR: 8321053: Use ByteArrayInputStream.buf directly when parameter of transferTo() is trusted

[jmehrens]

On Thu, 30 Nov 2023 00:03:21 GMT, Brian Burkhalter  wrote:

> Pass `ByteArrayInputStream.buf ` directly to the `OutputStream` parameter of 
> `BAIS.transferTo` only if the target stream is in the `java.io` package.

src/java.base/share/classes/java/io/ByteArrayInputStream.java line 211:

> 209: if (len > 0) {
> 210: byte[] tmp;
> 211: if ("java.io".equals(out.getClass().getPackageName()))

Isn't this protection defeated with:

ByteArrayInputStream bais = new ByteArrayInputStream(bytes);
UntrustedOutputStream uos = new UntrustedOutputStream();
bais.transferTo(new java.io.DataOutputStream(uos)); 


Or am I missing something?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16893#discussion_r1410139928

Re: RFR: 8319577: x86_64 AVX2 intrinsics for Arrays.sort methods (int, float arrays) [v2]

[Sandhya Viswanathan]

On Tue, 28 Nov 2023 20:52:35 GMT, Srinivas Vamsi Parasa  
wrote:

>> Thanks Sandhya, will fix this issue.
>
> Thanks Sandhya for suggesting the change to use supports_simd_sort(BasicType 
> bt). Please see the updated code upstreamed.

@vamsi-parasa Thanks, your changes look good to me.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16534#discussion_r1410117695

Re: RFR: 8319872: AArch64: [vectorapi] Implementation of unsigned (zero extended) casts [v4]

[Eric Liu]

On Wed, 22 Nov 2023 07:05:21 GMT, Eric Liu  wrote:

>> Vector API defines zero-extend operations [1], which are going to be 
>> intrinsified and generated to `VectorUCastNode` by C2. This patch adds 
>> backend implementation for `VectorUCastNode` on AArch64.
>> 
>> The micro benchmark shows significant performance improvement. In my test 
>> machine (SVE, 256-bit), the result is shown as below:
>> 
>> 
>> 
>>   Benchmark Before After   Units   Gain
>>   VectorZeroExtend.byte2Int 3168.251   243012.399  ops/ms  75.70
>>   VectorZeroExtend.byte2Long3212.201   216291.588  ops/ms  66.33
>>   VectorZeroExtend.byte2Short   3391.968   182655.365  ops/ms  52.85
>>   VectorZeroExtend.int2Long 1012.19780448.553  ops/ms  78.48
>>   VectorZeroExtend.short2Int1812.471   153416.828  ops/ms  83.65
>>   VectorZeroExtend.short2Long   1788.382   129794.814  ops/ms  71.58
>> 
>> 
>> On other Neon systems, we can get similar performance boost as a result of 
>> intrinsification success.
>> 
>> Since `VectorUCastNode` only used in Vector API's zero extension currently, 
>> this patch also adds assertion on nodes' definitions to clarify their usages.
>> 
>> [TEST]
>> compiler/vectorapi and jdk/incubator/vector passed on NEON and SVE machines.
>> 
>> [1] 
>> https://github.com/openjdk/jdk/blob/master/src/jdk.incubator.vector/share/classes/jdk/incubator/vector/VectorOperators.java#L726
>
> Eric Liu has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   small fix
>   
>   Change-Id: Icfe9619af1c9e7d5ea8cac457ccebb4eec5c34ad

@theRealAph Could you help to take a look? Thanks.

-

PR Comment: https://git.openjdk.org/jdk/pull/16670#issuecomment-1832934884

Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref

[Brent Christian]

On Tue, 21 Nov 2023 22:46:32 GMT, Roger Riggs  wrote:

>> src/java.base/share/classes/java/lang/ref/Reference.java line 489:
>> 
>>> 487:  * If this reference was already enqueued (by the garbage 
>>> collector, or a
>>> 488:  * previous call to {@code enqueue}), this method is not 
>>> successful,
>>> 489:  * and returns false.
>> 
>> If we're going to talk about successful and unsuccessful calls, we should 
>> define what success is first. I guess that would be something like: if this 
>> ref is registered with a queue and not already enqueued, it is enqueued 
>> successfully and the method returns true. Otherwise, not successful, and 
>> returns false.
>
> This could be worded as a post condition, after the call the ref is enqueued 
> with the queue; the return is true iff the ref was not previously enqueued.
> The enqueuing is not conditional (assuming the queue is non-null).

I'll give that some thought. The enqueuing is not conditional, but the 
_happens-before_ is...

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16644#discussion_r1410031119

Re: RFR: 6230751: [Fmt-Ch] Recursive MessageFormats in ChoiceFormats ignore indicated subformats

[Justin Lu]

On Wed, 29 Nov 2023 23:34:14 GMT, Naoto Sato  wrote:

> Do you think it would be helpful if we describe that limitation so that such 
> confusion will not arise?

Yes, that's a good point. I didn't initially include one because I wanted to 
look into [JDK-4270867](https://bugs.openjdk.org/browse/JDK-4270867) (which 
would be the fix to the limitation) first.

-

PR Comment: https://git.openjdk.org/jdk/pull/16891#issuecomment-1832905952

Re: RFR: 8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is trusted

[Brian Burkhalter]

On Wed, 29 Nov 2023 11:57:37 GMT, Sergey Tsypanov  wrote:

> It looks like we can skip copying of `byte[]` in 
> `BufferedInputStream.implTransferTo()` for `OutputStreams` residing in 
> `java.io`.
> 
> See comment by @vlsi in 
> https://github.com/openjdk/jdk/pull/10525/files#diff-e19c508d1bb6ee78697ecca66947c395adda0d9c49a85bf696e677ecbd977af1R612

If there is already test coverage of this area (I don't think there is), then 
the `noreg-other` label with a comment should be added to the issue. Otherwise, 
test coverage should be added, or some other `noreg-*` label added to the 
issue, if justified.

-

PR Comment: https://git.openjdk.org/jdk/pull/16879#issuecomment-1832894268

RFR: 8321053: Use ByteArrayInputStream.buf directly when parameter of transferTo() is trusted

[Brian Burkhalter]

Pass `ByteArrayInputStream.buf ` directly to the `OutputStream` parameter of 
`BAIS.transferTo` only if the target stream is in the `java.io` package.

-

Commit messages:
 - 8321053: Use ByteArrayInputStream.buf directly when parameter of 
transferTo() is trusted

Changes: https://git.openjdk.org/jdk/pull/16893/files
 Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=16893&range=00
  Issue: https://bugs.openjdk.org/browse/JDK-8321053
  Stats: 95 lines in 2 files changed: 94 ins; 0 del; 1 mod
  Patch: https://git.openjdk.org/jdk/pull/16893.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/16893/head:pull/16893

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

Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v10]

[Mandy Chung]

On Fri, 24 Nov 2023 13:25:35 GMT, Severin Gehwolf  wrote:

>> Please review this patch which adds a "jmodless" jlink mode to the JDK. 
>> Fundamentally this patch adds an option to use `jlink` even though your JDK 
>> install might not come with the packaged modules (directory `jmods`). This 
>> is particularly useful to further reduce the size of a jlinked runtime. 
>> After the removal of the concept of a JRE, a common distribution mechanism 
>> is still the full JDK with all modules and packaged modules. However, 
>> packaged modules can incur an additional size tax. For example in a 
>> container scenario it could be useful to have a base JDK container including 
>> all modules, but without also delivering the packaged modules. This comes at 
>> a size advantage of `~25%`. Such a base JDK container could then be used to 
>> `jlink` application specific runtimes, further reducing the size of the 
>> application runtime image (App + JDK runtime; as a single image *or* 
>> separate bundles, depending on the app being modularized).
>> 
>> The basic design of this approach is to add a jlink plugin for tracking 
>> non-class and non-resource files of a JDK install. I.e. files which aren't 
>> present in the jimage (`lib/modules`). This enables producing a 
>> `JmodLessArchive` class which has all the info of what constitutes the final 
>> jlinked runtime.
>> 
>> Basic usage example:
>> 
>> $ diff -u <(./bin/java --list-modules --limit-modules java.se) 
>> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules 
>> --limit-modules java.se)
>> $ diff -u <(./bin/java --list-modules --limit-modules java.se) 
>> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules 
>> --limit-modules jdk.jlink)
>> $ ls ../linux-x86_64-server-release/images/jdk/jmods
>> java.base.jmodjava.net.http.jmod   java.sql.rowset.jmod  
>> jdk.crypto.ec.jmod jdk.internal.opt.jmod 
>> jdk.jdi.jmod jdk.management.agent.jmod  jdk.security.auth.jmod
>> java.compiler.jmodjava.prefs.jmod  java.transaction.xa.jmod  
>> jdk.dynalink.jmod  jdk.internal.vm.ci.jmod   
>> jdk.jdwp.agent.jmod  jdk.management.jfr.jmodjdk.security.jgss.jmod
>> java.datatransfer.jmodjava.rmi.jmodjava.xml.crypto.jmod  
>> jdk.editpad.jmod   jdk.internal.vm.compiler.jmod 
>> jdk.jfr.jmod jdk.management.jmodjdk.unsupported.desktop.jmod
>> java.desktop.jmod java.scripting.jmod  java.xml.jmod 
>> jdk.hotspot.agent.jmod jdk.internal.vm.compiler.management.jmod  
>> jdk.jlink.jmod   jdk.naming.dns.j...
>
> Severin Gehwolf has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Tighten ModifiedFilesExitTest
>   
>   Ensure the error message is reasonable and doesn't include
>   Exceptions presented to the user.

Thanks.  I'll continue the review on the revised version.

src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JlinkTask.java line 88:

> 86: 
> 87: // Run time based link internal resources files
> 88: private static final String OTHER_RESOURCES_FILE = 
> "jdk/tools/jlink/internal/runlink_%s_resources";

This can be shared with the plugin writing this file.   This file collects the 
hash of the non-class resource files in the image.   "runlink" is a confusing 
prefix.   What about:
Suggestion:

// jlink's internal resource file keeps track of the hash of per-module 
non-class resources in the run-time image
public static final String MODULE_RESOURCES_LIST = 
"jdk/tools/jlink/internal/%s_resources";

src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JlinkTask.java line 286:

> 284: }
> 285: 
> 286: boolean useModulePath = !options.modulePath.isEmpty();

Is there any reason why the run-time image based linking is only supported if 
`--module-path` is not specified?   Linking user modules specified on the 
module path with the run-time image seems useful.   If the module exists in the 
given module path and the run-time image, the modules from the module path 
should probably take precedence but it needs to work through the details and 
potential issues.

src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JlinkTask.java line 561:

> 559: // Print info message when a run-image link is being 
> performed
> 560: if (log != null) {
> 561: log.println("'jmods' folder not present, performing a 
> run-time image based link.");

This should be a localized message defined in `jlink.properties`.

src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JlinkTask.java line 570:

> 568:   .sorted(Comparator.comparing(ResolvedModule::name))
> 569:   .forEach(rm -> log.format("%s %s%s%n",
> 570: rm.name(), 
> rm.reference().location().get(), config.useModulePath() ? "" : " (run-time 
> 

Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref

[Brent Christian]

On Tue, 21 Nov 2023 09:12:49 GMT, Viktor Klang  wrote:

>> Classes in the `java.lang.ref` package would benefit from an update to bring 
>> the spec in line with how the VM already behaves. The changes would focus on 
>> _happens-before_ edges at some key points during reference processing.
>> 
>> A couple key things we want to be able to say are:
>> - `Reference.reachabilityFence(x)` _happens-before_ reference processing 
>> occurs for 'x'.
>> - `Cleaner.register()` _happens-before_ the Cleaner thread runs the 
>> registered cleaning action.
>> 
>> This will bring Cleaner in line (or close) with the memory visibility 
>> guarantees made for finalizers in [JLS 
>> 17.4.5](https://docs.oracle.com/javase/specs/jls/se18/html/jls-17.html#jls-17.4.5):
>> _"There is a happens-before edge from the end of a constructor of an object 
>> to the start of a finalizer (§12.6) for that object."_
>
> src/java.base/share/classes/java/lang/ref/Reference.java line 497:
> 
>> 495:  * or {@link ReferenceQueue#remove}.
>> 496:  *
>> 497:  * This method is invoked only by Java code; when the garbage 
>> collector
> 
> Perhaps something along the lines of:
> 
> Suggestion:
> 
>  * This method is only invoked explicitly; when the garbage collector

I think the important part to convey is that the GC won't call this method. So 
how about _just_:

When the garbage collector enqueues references it does so directly, without 
invoking this method.

?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16644#discussion_r1409993103

Re: RFR: 6230751: [Fmt-Ch] Recursive MessageFormats in ChoiceFormats ignore indicated subformats

[Naoto Sato]

On Wed, 29 Nov 2023 22:41:26 GMT, Justin Lu  wrote:

> Please review this PR which updates an incorrect code example in 
> _java/text/ChoiceFormat_.
> 
> ChoiceFormat (and MessageFormat) provide an example of how to produce a 
> pattern that supports singular and plural forms. The ChoiceFormat example is 
> incorrect, as recursive MessageFormats produced by a ChoiceFormat subformat 
> only recognize subformats defined through the MessageFormat pattern syntax, 
> not through the subformats contained within the top level MessageFormat. 
> 
> In the original example,
> `Format[] testFormats = {fileform, null, NumberFormat.getInstance()};` could 
> have been replaced with 
> `Format[] testFormats = {fileform, null, new ChoiceFormat("0#BROKEN")};` and 
> the original output would have been the same.
> 
> This PR replaces the example with the one used in MessageFormat, which is 
> correct. 
> 
> This PR also includes a drive-by fix to remove leftover ``s from 
> a previous `@snippet` conversion.

Do you think it would be helpful if we describe that limitation so that such 
confusion will not arise?

-

PR Review: https://git.openjdk.org/jdk/pull/16891#pullrequestreview-1756397913

Re: RFR: 8294696 - BufferedInputStream.transferTo should drain buffer when mark set [v6]

[Brian Burkhalter]

On Wed, 29 Nov 2023 22:49:58 GMT, Markus KARG  wrote:

> As Alan pointed out, it is a bug (actually even a security risk), so BAIS 
> should get fixed, too.

I am going to file an issue on this.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/10525#discussion_r1409965074

Re: RFR: 8294696 - BufferedInputStream.transferTo should drain buffer when mark set [v6]

[Markus KARG]

On Wed, 29 Nov 2023 05:55:03 GMT, Vladimir Sitnikov  
wrote:

>> src/java.base/share/classes/java/io/BufferedInputStream.java line 612:
>> 
>>> 610: if (avail > 0) {
>>> 611: // Prevent poisoning and leaking of buf
>>> 612: byte[] buffer = Arrays.copyOfRange(getBufIfOpen(), 
>>> pos, count);
>> 
>> @mkarg , could you please clarify why you added `Arrays.copyOfRange` here?
>> It seems to be an excessive copy that doesn't help much.
>> 
>> `buf` is `protected` in `BufferedInputStream`, so if someone really wants to 
>> get hold of the actual buffer, they can subclass `BufferedInputStream` and 
>> expose the buffer directly.
>> What do you think of removing `copyOfRange`?
>
> Buffer copy was not there before, and defensive copy was never present in 
> `ByteArrayInputStream` as well: 
> https://github.com/openjdk/jdk/blob/9a6ca233c7e91ffa2ce9451568b3be88ccd04504/src/java.base/share/classes/java/io/ByteArrayInputStream.java#L213

Alan asked for this, and for good reason: While we implicitly trust subclasses 
as the buffer is "theirs" as part of the inheritance, we do not know where 
target stream comes from. It could be an injected verhicle to perform (at 
least) the following:
* Leaking data. The target stream could access data beyond the intended region.
* Poisoning. The target stream could write into the buffer.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/10525#discussion_r1409957433

Re: RFR: 8294696 - BufferedInputStream.transferTo should drain buffer when mark set [v6]

[Markus KARG]

On Wed, 29 Nov 2023 22:49:17 GMT, Markus KARG  wrote:

>> Buffer copy was not there before, and defensive copy was never present in 
>> `ByteArrayInputStream` as well: 
>> https://github.com/openjdk/jdk/blob/9a6ca233c7e91ffa2ce9451568b3be88ccd04504/src/java.base/share/classes/java/io/ByteArrayInputStream.java#L213
>
> Alan asked for this, and for good reason: While we implicitly trust 
> subclasses as the buffer is "theirs" as part of the inheritance, we do not 
> know where target stream comes from. It could be an injected verhicle to 
> perform (at least) the following:
> * Leaking data. The target stream could access data beyond the intended 
> region.
> * Poisoning. The target stream could write into the buffer.

As Alan pointed out, it is a bug (actually even a security risk), so BAIS 
should get fixed, too.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/10525#discussion_r1409958304

RFR: 6230751: [Fmt-Ch] Recursive MessageFormats in ChoiceFormats ignore indicated subformats

[Justin Lu]

Please review this PR which updates an incorrect code example in 
_java/text/ChoiceFormat_.

ChoiceFormat (and MessageFormat) provide an example of how to produce a pattern 
that supports singular and plural forms. The ChoiceFormat example is incorrect, 
as recursive MessageFormats produced by a ChoiceFormat subformat only recognize 
subformats defined through the MessageFormat pattern syntax, not through the 
subformats contained within the top level MessageFormat. 

In the original example,
`Format[] testFormats = {fileform, null, NumberFormat.getInstance()};` could 
have been replaced with 
`Format[] testFormats = {fileform, null, new ChoiceFormat("0#BROKEN")};` and 
the original output would have been the same.

This PR replaces the example with the one used in MessageFormat, which is 
correct. 

This PR also includes a drive-by fix to remove leftover ``s from a 
previous `@snippet` conversion.

-

Commit messages:
 - change var name in trailing example
 - replace '.' with ':'
 - init

Changes: https://git.openjdk.org/jdk/pull/16891/files
 Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=16891&range=00
  Issue: https://bugs.openjdk.org/browse/JDK-6230751
  Stats: 55 lines in 2 files changed: 1 ins; 29 del; 25 mod
  Patch: https://git.openjdk.org/jdk/pull/16891.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/16891/head:pull/16891

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

Re: RFR: 8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is trusted

[Markus KARG]

On Wed, 29 Nov 2023 19:59:03 GMT, Alan Bateman  wrote:

>> It looks like we can skip copying of `byte[]` in 
>> `BufferedInputStream.implTransferTo()` for `OutputStreams` residing in 
>> `java.io`.
>> 
>> See comment by @vlsi in 
>> https://github.com/openjdk/jdk/pull/10525/files#diff-e19c508d1bb6ee78697ecca66947c395adda0d9c49a85bf696e677ecbd977af1R612
>
> src/java.base/share/classes/java/io/BufferedInputStream.java line 647:
> 
>> 645: if (avail > 0) {
>> 646: // trust all OutputStreams from java.io
>> 647: if (out.getClass().getPackageName() == 
>> BufferedInputStream.class.getPackageName()) {
> 
> I don't think Class::getPackageName documents that the returned String is 
> intern so I wonder if the == check will lead to questions and suggestions of 
> a bug. Classes with names starting with "java." can only be defined to the 
> boot or platform class loader (details in the ClassLoader API docs) so you 
> could just check if the package name equals "java.io".

Do we only want to trust java.io or anything starting with java.*?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16879#discussion_r1409947085

Re: RFR: 8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is trusted

[Markus KARG]

On Wed, 29 Nov 2023 11:57:37 GMT, Sergey Tsypanov  wrote:

> It looks like we can skip copying of `byte[]` in 
> `BufferedInputStream.implTransferTo()` for `OutputStreams` residing in 
> `java.io`.
> 
> See comment by @vlsi in 
> https://github.com/openjdk/jdk/pull/10525/files#diff-e19c508d1bb6ee78697ecca66947c395adda0d9c49a85bf696e677ecbd977af1R612

Thank you for further optimizing transferTo()! If you fix what Alan proposed, 
this PR LGTM! 👍

-

Changes requested by mk...@github.com (no known OpenJDK username).

PR Review: https://git.openjdk.org/jdk/pull/16879#pullrequestreview-1756336805

Re: RFR: 8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is trusted

[Markus KARG]

On Wed, 29 Nov 2023 20:07:37 GMT, Vladimir Sitnikov  
wrote:

>>> What do you think of passing the buffer as is?
>> 
>> No, it should only do for trusted targets. BAIS has an issue in that area 
>> that should be fixed.
>
> The buffer in question is protected, so any subclass can directly access it. 
> In other words, untrusted code can easily acoess the buffer, and it does not 
> sound fair to add extra overhead to the method which was created for the 
> performance reasons.
> 
> Does copyOfRange do any good here? Do you mean JDK should copy every buffer 
> it passes to non-JDK code?

@vlsi Yes, unless the JRE comes up with read-only buffers all untrusted code 
should get copies of JRE-internal buffers only to provide buffer poisoning and 
spying data located beyond range limits. Subclasses are free to do what they 
want with the inherited buffer (it is *their* buffer implicitly), but target 
output stream might be an injected bad guy that we must not trust in any regard.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16879#discussion_r1409945330

Re: Unneeded array assignments in core libs

[Anthony Goubard]

Thanks for the answers.
I've created an issue in bugreport.java.com with internal review ID :
9076287

Hope that helps,
Anthony

Le mer. 29 nov. 2023 à 21:55, Roger Riggs  a écrit :

> Hi Anthony,
>
> Go ahead a file a single bug (for the two cases).
> That code may have been written before Arrays.fill(...).
>
> Regards, Roger
>
> On 11/29/23 7:54 AM, Anthony Goubard wrote:
>
> Hello,
>
>  Last Friday, I decided to look if there were some classes that did
> unneeded array assignments (e.g. to 0 / 0.0f / null / false) in the JDK.
>
>  I've found a few places and in particular 2 places where it's done in big
> for loops (looping more than 500 times). So I thought you might be
> interested to know about them.
>
>
> https://github.com/openjdk/jdk/blob/6871a2ff1207d3ee70973b1c4ee9bd09969c185b/src/java.base/share/classes/java/text/MergeCollation.java#L72
>
> https://github.com/openjdk/jdk/blob/6871a2ff1207d3ee70973b1c4ee9bd09969c185b/src/java.base/share/classes/sun/text/CompactByteArray.java#L85
> Also for CompactByteArray, maybe it's worth checking that defaultValue is
> different than 0 before entering the for loop (Line 80)
>
> Let me know if you're interested in fixing them and if you want me to fill
> 2 bugs for them in the JBS or in bugreport.java.com.
>
> Best regards,
> Anthony Goubard
>
>
>

Re: Unneeded array assignments in core libs

[Roger Riggs]

Hi Anthony,

Go ahead a file a single bug (for the two cases).
That code may have been written before Arrays.fill(...).

Regards, Roger

On 11/29/23 7:54 AM, Anthony Goubard wrote:

Hello,

 Last Friday, I decided to look if there were some classes that did 
unneeded array assignments (e.g. to 0 / 0.0f / null / false) in the JDK.


 I've found a few places and in particular 2 places where it's done in 
big for loops (looping more than 500 times). So I thought you might be 
interested to know about them.


https://github.com/openjdk/jdk/blob/6871a2ff1207d3ee70973b1c4ee9bd09969c185b/src/java.base/share/classes/java/text/MergeCollation.java#L72
https://github.com/openjdk/jdk/blob/6871a2ff1207d3ee70973b1c4ee9bd09969c185b/src/java.base/share/classes/sun/text/CompactByteArray.java#L85
Also for CompactByteArray, maybe it's worth checking that defaultValue 
is different than 0 before entering the for loop (Line 80)


Let me know if you're interested in fixing them and if you want me to 
fill 2 bugs for them in the JBS or in bugreport.java.com 
.


Best regards,
Anthony Goubard

Re: RFR: JDK-8315458 Implement JEP 463: Implicitly Declared Classes and Instance Main Method (Second Preview) [v35]

[Joe Darcy]

On Tue, 28 Nov 2023 15:00:43 GMT, Jim Laskey  wrote:

>> test/langtools/tools/javac/ImplicitClass/TestImplicitClass.java line 35:
>> 
>>> 33: import java.lang.reflect.Modifier;
>>> 34: 
>>> 35: public class TestImplicitClass {
>> 
>> The test looks to be testing core reflection behavior (i.e. runtime 
>> behavior) and not compile-time behavior via javax.lang.model.
>> 
>> Core reflection tests should be done too, but done the core libraries tests, 
>> not langtools tests.
>> 
>> I might have overlooked it, but if some other exercise of compile-time 
>> modeling is not being done, please restore and update accordingly the sort 
>> of tests previously done in the now-deleted
>> 
>> test/langtools/tools/javac/processing/model/element/TestUnnamedClass.java
>
> Added

To clarify, there should be separate tests of what implicit classes look like 
under core reflection and under javax.lang.model.

As currently written, the current test is a core reflection tests located under 
the javac/annotation processing test area, which is a misplacement. Please move 
this test to core reflection, replacing 445's 

test/jdk/java/lang/Class/UnnamedClass/TestUnnamedClass.java

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16461#discussion_r1409819479

Re: RFR: 8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is trusted

[Alan Bateman]

On Wed, 29 Nov 2023 20:07:37 GMT, Vladimir Sitnikov  
wrote:

> The buffer in question is protected, so any subclass can directly access it. 
> In other words, untrusted code can easily acoess the buffer, and it does not 
> sound fair to add extra overhead to the method which was created for the 
> performance reasons.

If something extends BIS then it has access to the protected fields, this isn't 
a concern here. Instead, the concern is about passing a reference to the target 
output stream.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16879#discussion_r1409822279

Re: RFR: JDK-8315458 Implement JEP 463: Implicitly Declared Classes and Instance Main Method (Second Preview) [v43]

[Joe Darcy]

On Wed, 29 Nov 2023 15:25:15 GMT, Jim Laskey  wrote:

>> Address changes from JEP 445 to JEP 463.
>> 
>> - Move from a SYNTHETIC unnamed class to a MANDATED implicit class.
>> 
>> - Don't mark class on read.
>> 
>> - Remove reflection and annotation processing related to unnamed classes.
>> 
>> - Simplify main method search.
>
> Jim Laskey has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - Update test/jdk/tools/launcher/InstanceMainTest.java
>
>Co-authored-by: Jan Lahoda <51319204+laho...@users.noreply.github.com>
>  - Update test/jdk/tools/launcher/InstanceMainTest.java
>
>Co-authored-by: Jan Lahoda <51319204+laho...@users.noreply.github.com>

test/langtools/tools/javac/processing/model/element/TestImplicitClass.java line 
120:

> 118: 
> 119: /*
> 120:  * From JEP 445 JLS changes:

Update JEP number.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16461#discussion_r1409820065

Re: RFR: 8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is trusted

[Vladimir Sitnikov]

On Wed, 29 Nov 2023 19:55:09 GMT, Alan Bateman  wrote:

>> src/java.base/share/classes/java/io/BufferedInputStream.java line 653:
>> 
>>> 651: byte[] buffer = Arrays.copyOfRange(getBufIfOpen(), 
>>> pos, count);
>>> 652: out.write(buffer);
>>> 653: }
>> 
>> Suggestion:
>> 
>> out.write(getBufIfOpen(), pos, count);
>> 
>> 
>> What do you think of passing the buffer as is?
>> `ByteArrayInputStream` passes the buffer without extra copies anyway: 
>> https://github.com/openjdk/jdk/blob/9a6ca233c7e91ffa2ce9451568b3be88ccd04504/src/java.base/share/classes/java/io/ByteArrayInputStream.java#L207-L213
>
>> What do you think of passing the buffer as is?
> 
> No, it should only do for trusted targets. BAIS has an issue in that area 
> that should be fixed.

The buffer in question is protected, so any subclass can directly access it. In 
other words, untrusted code can easily acoess the buffer, and it does not sound 
fair to add extra overhead to the method which was created for the performance 
reasons.

Does copyOfRange do any good here? Do you mean JDK should copy every buffer it 
passes to non-JDK code?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16879#discussion_r1409810571

Re: RFR: JDK-8315458 Implement JEP 463: Implicitly Declared Classes and Instance Main Method (Second Preview) [v43]

[Joe Darcy]

On Wed, 29 Nov 2023 15:25:15 GMT, Jim Laskey  wrote:

>> Address changes from JEP 445 to JEP 463.
>> 
>> - Move from a SYNTHETIC unnamed class to a MANDATED implicit class.
>> 
>> - Don't mark class on read.
>> 
>> - Remove reflection and annotation processing related to unnamed classes.
>> 
>> - Simplify main method search.
>
> Jim Laskey has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - Update test/jdk/tools/launcher/InstanceMainTest.java
>
>Co-authored-by: Jan Lahoda <51319204+laho...@users.noreply.github.com>
>  - Update test/jdk/tools/launcher/InstanceMainTest.java
>
>Co-authored-by: Jan Lahoda <51319204+laho...@users.noreply.github.com>

src/jdk.compiler/share/classes/com/sun/tools/javac/processing/PrintingProcessor.java
 line 129:

> 127: Element enclosing = e.getEnclosingElement();
> 128: 
> 129: // Don't print out the constructor of an anonymous

Please add back "class" so that the comment reads "Don't print out the 
constructor of an anonymous class."

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16461#discussion_r1409810773

Re: RFR: 8320716: ResolvedModule::reads includes self when configuration contains two or more automatic modules

[Mandy Chung]

On Sun, 26 Nov 2023 18:18:10 GMT, Alan Bateman  wrote:

> This is update to the specification of the j.l.module.ResolvedModule.reads 
> method to clarify that the set of resolved modules returned does not include 
> itself. There is a small implementation change to align with the 
> specification and fix an anomaly that arises with configurations that contain 
> two or more automatic modules.
> 
> Every module reads itself but the intent with ResolvedModule.reads is that it 
> returns a set that doesn't include self. As things stand right now, the 
> returned set does not include self when all modules in the configuration are 
> explicit modules.  However, if the configuration contains two or more 
> automatic modules then the set includes self, a side effect of augmenting the 
> readability graph due to implied readability.
> 
> The specification of the "reads" method is updated. The implementation is 
> also changed to skip automatic modules when augmenting the graph due to 
> implied readability. It is skipped as each automatic module already reads all 
> other modules. Note that it is not goal here to change the algorithm for 
> handling implied readability, this may be a topic for a follow on PR.
> 
> The existing ConfigurationTest already includes several tests for 
> configurations consisting solely of explicit modules, no updates are needed. 
> For configurations that include automatic modules, the existing 
> AutomaticModulesTest is updated to add new asserts to each of the 
> testConfigurationXXX methods. I decided to migrate this test to JUnit while 
> visiting, most of it is just migrating the TestNG data providers to be 
> parameterized tests. If needed then we could separate this but it's a simple 
> migration so I left it in.

I read through the resolver implementation and the test changes.   Skipping the 
automatic modules in the process of propagating requires transitive is a simple 
fix to this issue.  Looks good.

src/java.base/share/classes/java/lang/module/Resolver.java line 628:

> 626: for (Map.Entry> e : 
> g1.entrySet()) {
> 627: ResolvedModule m1 = e.getKey();
> 628: if (!m1.descriptor().isAutomatic()) {

May be useful to add a comment to explain why automatic modules can be skipped 
as requires transitive edges for automatic modules already propagated in g1.

-

Marked as reviewed by mchung (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/16818#pullrequestreview-1756073701
PR Review Comment: https://git.openjdk.org/jdk/pull/16818#discussion_r1409779490

Re: RFR: 8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is trusted

[Alan Bateman]

On Wed, 29 Nov 2023 11:57:37 GMT, Sergey Tsypanov  wrote:

> It looks like we can skip copying of `byte[]` in 
> `BufferedInputStream.implTransferTo()` for `OutputStreams` residing in 
> `java.io`.
> 
> See comment by @vlsi in 
> https://github.com/openjdk/jdk/pull/10525/files#diff-e19c508d1bb6ee78697ecca66947c395adda0d9c49a85bf696e677ecbd977af1R612

src/java.base/share/classes/java/io/BufferedInputStream.java line 647:

> 645: if (avail > 0) {
> 646: // trust all OutputStreams from java.io
> 647: if (out.getClass().getPackageName() == 
> BufferedInputStream.class.getPackageName()) {

I don't think Class::getPackageName documents that the returned String is 
intern so I wonder if the == check will lead to questions and suggestions of a 
bug. Classes with names starting with "java." can only be defined to the boot 
or platform class loader (details in the ClassLoader API docs) so you could 
just check if the package name equals "java.io".

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16879#discussion_r1409802437

Re: RFR: 8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is trusted

[Alan Bateman]

On Wed, 29 Nov 2023 13:56:34 GMT, Vladimir Sitnikov  
wrote:

> What do you think of passing the buffer as is?

No, it should only do for trusted targets. BAIS has an issue in that area that 
should be fixed.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16879#discussion_r1409798218

Integrated: JDK-8210410: Refactor java.util.Currency:i18n shell tests to plain java tests

[Justin Lu]

On Thu, 23 Nov 2023 00:26:43 GMT, Justin Lu  wrote:

> Please review this PR which converts the shell test, 
> _java/util/currency/PropertiesTest.sh_ to a normal java test.
> 
> This test is a test runner that launches test methods from 
> _PropertiesTest.java_. It tests both the ways to supersede the default 
> currencies, that is, either using a custom properties file under the lib 
> directory, or setting the path to a custom properties file via the system 
> property.

This pull request has now been integrated.

Changeset: 2584bf87
Author:Justin Lu 
URL:   
https://git.openjdk.org/jdk/commit/2584bf87aef66744a8e586805735cded0d2f98f1
Stats: 301 lines in 2 files changed: 157 ins; 144 del; 0 mod

8210410: Refactor java.util.Currency:i18n shell tests to plain java tests

Reviewed-by: naoto, lancea

-

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

Re: RFR: JDK-8319413: Start of release updates for JDK 23 [v2]

[Joe Darcy]

> Time to start making preparations for JDK 23.

Joe Darcy has updated the pull request with a new target base due to a merge or 
a rebase. The pull request now contains seven commits:

 - Merge branch 'master' into JDK-8319413
 - Add symbol files for JDK 22 build 25.
 - Merge branch 'master' into JDK-8319413
 - Merge branch 'master' into JDK-8319413
 - Adjust expected release date.
 - Fix omission in Classfile.java
 - JDK-8319413: Start of release updates for JDK 23

-

Changes: https://git.openjdk.org/jdk/pull/16505/files
 Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=16505&range=01
  Stats: 7694 lines in 70 files changed: 7656 ins; 0 del; 38 mod
  Patch: https://git.openjdk.org/jdk/pull/16505.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/16505/head:pull/16505

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

Re: RFR: JDK-8320448 Accelerate IndexOf using AVX2 [v2]

[Scott Gibbons]

On Wed, 29 Nov 2023 15:01:32 GMT, Scott Gibbons  wrote:

>> Re-write the IndexOf code without the use of the pcmpestri instruction, only 
>> using AVX2 instructions.  This change accelerates String.IndexOf on average 
>> 1.3x for AVX2.  The benchmark numbers:
>> 
>> 
>> Benchmark   Score
>> Latest  
>> StringIndexOf.advancedWithMediumSub   343.573317.934 
>> 0.925375393x
>> StringIndexOf.advancedWithShortSub11039.081  1053.96 
>> 1.014319384x
>> StringIndexOf.advancedWithShortSub255.828110.541 
>> 1.980027943x
>> StringIndexOf.constantPattern9.361   11.906  
>> 1.271872663x
>> StringIndexOf.searchCharLongSuccess  4.216   4.218   
>> 1.000474383x
>> StringIndexOf.searchCharMediumSuccess3.133   3.216   
>> 1.02649218x
>> StringIndexOf.searchCharShortSuccess 3.763.761   
>> 1.000265957x
>> StringIndexOf.success9.186   
>> 9.713   1.057369911x
>> StringIndexOf.successBig   14.34146.343  
>> 3.231504079x
>> StringIndexOfChar.latin1_AVX2_String   6220.918  12154.52
>> 1.953814533x
>> StringIndexOfChar.latin1_AVX2_char 5503.556  5540.044
>> 1.006629895x
>> StringIndexOfChar.latin1_SSE4_String   6978.854  6818.689
>> 0.977049957x
>> StringIndexOfChar.latin1_SSE4_char 5657.499  5474.624
>> 0.967675646x
>> StringIndexOfChar.latin1_Short_String  7132.541  
>> 6863.3590.962260014x
>> StringIndexOfChar.latin1_Short_char  16013.389 16162.437 
>> 1.009307711x
>> StringIndexOfChar.latin1_mixed_String  7386.12314771.622 
>> 1.15517x
>> StringIndexOfChar.latin1_mixed_char9901.671  9782.245
>> 0.987938803
>
> Scott Gibbons has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Only use optimization when EnableX86ECoreOpts is true

Latest numbers vs. top-of-tree JDK.

![JBS 
IndexOf](https://github.com/openjdk/jdk/assets/6704669/6ed74968-d333-4c70-8fe2-f94a56aaeca9)

-

PR Comment: https://git.openjdk.org/jdk/pull/16753#issuecomment-1832486450

Re: RFR: 8318966: Some methods make promises about Java array element alignment that are too strong

[Jorn Vernee]

On Wed, 15 Nov 2023 22:46:03 GMT, Jorn Vernee  wrote:

> See the JBS issue for an extended problem description.
> 
> This patch changes the specification and implementation of 
> `MethodHandles::byteArrayViewVarHandle`, 
> `MethodHandles::byteBufferViewVarHandle`, `ByteBuffer::alignedSlice`, and 
> `ByteBuffer::alignmentOffset` to weaken the guarantees they make about the 
> alignment of Java array elements, in order to bring them in line with the 
> guarantees made by an arbitrary JVM implementation (which makes no guarantees 
> about array element alignment beyond them being aligned to their natural 
> alignment).
> 
> - `MethodHandles::byteArrayViewVarHandle`: we can not guarantee any alignment 
> for the accesses. Which means we can only reliably support plain get and set 
> access modes. The javadoc text explaining which other access modes are 
> supported, or how to compute aligned offsets into the array is dropped, as it 
> is not guaranteed to be correct on all JVM implementations. The 
> implementation of the returned VarHandle is changed to throw an 
> `UnsupportedOperationException` for the unsupported access modes, as mandated 
> by the spec of `VarHandle` [1].
> 
> - `MethodHandles::byteBufferViewVarHandle`: the implementation/spec is 
> incorrect when accessing a heap buffer (wrapping a byte[]), for the same 
> reasons as `byteArrayViewVarHandle`. The spec is changed to specify that when 
> accessing a _heap buffer_, only plain get and set access modes are supported. 
> The implementation of the returned var handle is changed to throw an 
> `IllegalStateException` when an access is attempted on a heap buffer using an 
> access mode other than plain get or set. Note that we don't throw an outright 
> `UnsupportedOperationException` for this case, since whether the access modes 
> are supported depends on the byte buffer instance being used.
> 
> - `ByteBuffer::alignedSlice` and `ByteBuffer::alignmentOffset`: The former 
> method depends directly on the latter for all its alignment computations. We 
> change the implementation of the latter method to throw an 
> `UnsupportedOperationException` for all unit sizes greater than 1, when the 
> buffer is non-direct. This change is largely covered by the existing 
> specification:
> 
> 
>  * @throws UnsupportedOperationException
>  * If the native platform does not guarantee stable alignment 
> offset
>  * values for the given unit size when managing the memory regions
>  * of buffers of the same kind as this buffer (direct or
>  * non-direct).  For example, if garbage collection would result
>  * in the mo...

Anything else needed to move this forward? I think I've addressed all the 
review comments made so far.

-

PR Comment: https://git.openjdk.org/jdk/pull/16681#issuecomment-1828491878

Integrated: JDK-8320940: Fix typo in java.lang.Double

[Joe Darcy]

On Wed, 29 Nov 2023 02:00:14 GMT, Joe Darcy  wrote:

> Typo fix to to the new text added in JDK-8295391.

This pull request has now been integrated.

Changeset: d783aa31
Author:Joe Darcy 
URL:   
https://git.openjdk.org/jdk/commit/d783aa31a9c20f5ac2ee52c55bdc9be2388b1705
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod

8320940: Fix typo in java.lang.Double

Reviewed-by: rriggs, iris, shade, lancea, bpb

-

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

Re: RFR: JDK-8320940: Fix typo in java.lang.Double

[Brian Burkhalter]

On Wed, 29 Nov 2023 02:00:14 GMT, Joe Darcy  wrote:

> Typo fix to to the new text added in JDK-8295391.

Marked as reviewed by bpb (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/16872#pullrequestreview-1755813777

Re: RFR: JDK-8315458 Implement JEP 463: Implicitly Declared Classes and Instance Main Method (Second Preview) [v43]

[Jim Laskey]

> Address changes from JEP 445 to JEP 463.
> 
> - Move from a SYNTHETIC unnamed class to a MANDATED implicit class.
> 
> - Don't mark class on read.
> 
> - Remove reflection and annotation processing related to unnamed classes.
> 
> - Simplify main method search.

Jim Laskey has updated the pull request incrementally with two additional 
commits since the last revision:

 - Update test/jdk/tools/launcher/InstanceMainTest.java
   
   Co-authored-by: Jan Lahoda <51319204+laho...@users.noreply.github.com>
 - Update test/jdk/tools/launcher/InstanceMainTest.java
   
   Co-authored-by: Jan Lahoda <51319204+laho...@users.noreply.github.com>

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/16461/files
  - new: https://git.openjdk.org/jdk/pull/16461/files/2f7fdded..7ed4045a

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=16461&range=42
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16461&range=41-42

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

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

Re: RFR: JDK-8320448 Accelerate IndexOf using AVX2 [v2]

[Scott Gibbons]

> Re-write the IndexOf code without the use of the pcmpestri instruction, only 
> using AVX2 instructions.  This change accelerates String.IndexOf on average 
> 1.3x for AVX2.  The benchmark numbers:
> 
> 
> BenchmarkScore
> Latest  
> StringIndexOf.advancedWithMediumSub   343.573 317.934 
> 0.925375393x
> StringIndexOf.advancedWithShortSub1 1039.081  1053.96 
> 1.014319384x
> StringIndexOf.advancedWithShortSub2 55.828110.541 
> 1.980027943x
> StringIndexOf.constantPattern 9.361   11.906  
> 1.271872663x
> StringIndexOf.searchCharLongSuccess   4.216   4.218   
> 1.000474383x
> StringIndexOf.searchCharMediumSuccess 3.133   3.216   
> 1.02649218x
> StringIndexOf.searchCharShortSuccess  3.763.761   
> 1.000265957x
> StringIndexOf.success 9.186   9.713   
> 1.057369911x
> StringIndexOf.successBig14.34146.343  
> 3.231504079x
> StringIndexOfChar.latin1_AVX2_String6220.918  12154.52
> 1.953814533x
> StringIndexOfChar.latin1_AVX2_char  5503.556  5540.044
> 1.006629895x
> StringIndexOfChar.latin1_SSE4_String6978.854  6818.689
> 0.977049957x
> StringIndexOfChar.latin1_SSE4_char  5657.499  5474.624
> 0.967675646x
> StringIndexOfChar.latin1_Short_String   7132.541  6863.359
> 0.962260014x
> StringIndexOfChar.latin1_Short_char   16013.389 16162.437 
> 1.009307711x
> StringIndexOfChar.latin1_mixed_String   7386.12314771.622 
> 1.15517x
> StringIndexOfChar.latin1_mixed_char 9901.671  9782.245
> 0.987938803

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

  Only use optimization when EnableX86ECoreOpts is true

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/16753/files
  - new: https://git.openjdk.org/jdk/pull/16753/files/60d762b9..e614b86f

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=16753&range=01
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16753&range=00-01

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

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

Re: RFR: JDK-8315458 Implement JEP 463: Implicitly Declared Classes and Instance Main Method (Second Preview) [v42]

[Jan Lahoda]

On Wed, 29 Nov 2023 14:30:50 GMT, Jim Laskey  wrote:

>> Address changes from JEP 445 to JEP 463.
>> 
>> - Move from a SYNTHETIC unnamed class to a MANDATED implicit class.
>> 
>> - Don't mark class on read.
>> 
>> - Remove reflection and annotation processing related to unnamed classes.
>> 
>> - Simplify main method search.
>
> Jim Laskey has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Windows line endings

javac changes seem reasonably to me. Adding two nits to `InstanceMainTest.java`.

test/jdk/tools/launcher/InstanceMainTest.java line 45:

> 43: """,
> 44: 
> 45: // instance dominating instance

Nit:
Suggestion:

// instance dominating static

test/jdk/tools/launcher/InstanceMainTest.java line 84:

> 82: """,
> 83: 
> 84: // unnamed class dominating instance

Nit:
Suggestion:

// main with args dominating main without args

-

Marked as reviewed by jlahoda (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/16461#pullrequestreview-1755452564
PR Review Comment: https://git.openjdk.org/jdk/pull/16461#discussion_r1409394109
PR Review Comment: https://git.openjdk.org/jdk/pull/16461#discussion_r1409395226

Re: RFR: JDK-8315458 Implement JEP 463: Implicitly Declared Classes and Instance Main Method (Second Preview) [v42]

[Jim Laskey]

> Address changes from JEP 445 to JEP 463.
> 
> - Move from a SYNTHETIC unnamed class to a MANDATED implicit class.
> 
> - Don't mark class on read.
> 
> - Remove reflection and annotation processing related to unnamed classes.
> 
> - Simplify main method search.

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

  Windows line endings

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/16461/files
  - new: https://git.openjdk.org/jdk/pull/16461/files/30170407..2f7fdded

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=16461&range=41
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16461&range=40-41

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

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

Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v10]

[Severin Gehwolf]

On Fri, 24 Nov 2023 13:25:35 GMT, Severin Gehwolf  wrote:

>> Please review this patch which adds a "jmodless" jlink mode to the JDK. 
>> Fundamentally this patch adds an option to use `jlink` even though your JDK 
>> install might not come with the packaged modules (directory `jmods`). This 
>> is particularly useful to further reduce the size of a jlinked runtime. 
>> After the removal of the concept of a JRE, a common distribution mechanism 
>> is still the full JDK with all modules and packaged modules. However, 
>> packaged modules can incur an additional size tax. For example in a 
>> container scenario it could be useful to have a base JDK container including 
>> all modules, but without also delivering the packaged modules. This comes at 
>> a size advantage of `~25%`. Such a base JDK container could then be used to 
>> `jlink` application specific runtimes, further reducing the size of the 
>> application runtime image (App + JDK runtime; as a single image *or* 
>> separate bundles, depending on the app being modularized).
>> 
>> The basic design of this approach is to add a jlink plugin for tracking 
>> non-class and non-resource files of a JDK install. I.e. files which aren't 
>> present in the jimage (`lib/modules`). This enables producing a 
>> `JmodLessArchive` class which has all the info of what constitutes the final 
>> jlinked runtime.
>> 
>> Basic usage example:
>> 
>> $ diff -u <(./bin/java --list-modules --limit-modules java.se) 
>> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules 
>> --limit-modules java.se)
>> $ diff -u <(./bin/java --list-modules --limit-modules java.se) 
>> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules 
>> --limit-modules jdk.jlink)
>> $ ls ../linux-x86_64-server-release/images/jdk/jmods
>> java.base.jmodjava.net.http.jmod   java.sql.rowset.jmod  
>> jdk.crypto.ec.jmod jdk.internal.opt.jmod 
>> jdk.jdi.jmod jdk.management.agent.jmod  jdk.security.auth.jmod
>> java.compiler.jmodjava.prefs.jmod  java.transaction.xa.jmod  
>> jdk.dynalink.jmod  jdk.internal.vm.ci.jmod   
>> jdk.jdwp.agent.jmod  jdk.management.jfr.jmodjdk.security.jgss.jmod
>> java.datatransfer.jmodjava.rmi.jmodjava.xml.crypto.jmod  
>> jdk.editpad.jmod   jdk.internal.vm.compiler.jmod 
>> jdk.jfr.jmod jdk.management.jmodjdk.unsupported.desktop.jmod
>> java.desktop.jmod java.scripting.jmod  java.xml.jmod 
>> jdk.hotspot.agent.jmod jdk.internal.vm.compiler.management.jmod  
>> jdk.jlink.jmod   jdk.naming.dns.j...
>
> Severin Gehwolf has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Tighten ModifiedFilesExitTest
>   
>   Ensure the error message is reasonable and doesn't include
>   Exceptions presented to the user.

Thanks for the review! I'll work on an update.

-

PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-1831937187

Re: RFR: 8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is trusted

[Vladimir Sitnikov]

On Wed, 29 Nov 2023 11:57:37 GMT, Sergey Tsypanov  wrote:

> It looks like we can skip copying of `byte[]` in 
> `BufferedInputStream.implTransferTo()` for `OutputStreams` residing in 
> `java.io`.
> 
> See comment by @vlsi in 
> https://github.com/openjdk/jdk/pull/10525/files#diff-e19c508d1bb6ee78697ecca66947c395adda0d9c49a85bf696e677ecbd977af1R612

src/java.base/share/classes/java/io/BufferedInputStream.java line 653:

> 651: byte[] buffer = Arrays.copyOfRange(getBufIfOpen(), 
> pos, count);
> 652: out.write(buffer);
> 653: }

Suggestion:

out.write(getBufIfOpen(), pos, count);


What do you think of passing the buffer as is?
`ByteArrayInputStream` passes the buffer without extra copies anyway: 
https://github.com/openjdk/jdk/blob/9a6ca233c7e91ffa2ce9451568b3be88ccd04504/src/java.base/share/classes/java/io/ByteArrayInputStream.java#L207-L213

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16879#discussion_r1409319731

Unneeded array assignments in core libs

[Anthony Goubard]

Hello,

 Last Friday, I decided to look if there were some classes that did
unneeded array assignments (e.g. to 0 / 0.0f / null / false) in the JDK.

 I've found a few places and in particular 2 places where it's done in big
for loops (looping more than 500 times). So I thought you might be
interested to know about them.

https://github.com/openjdk/jdk/blob/6871a2ff1207d3ee70973b1c4ee9bd09969c185b/src/java.base/share/classes/java/text/MergeCollation.java#L72
https://github.com/openjdk/jdk/blob/6871a2ff1207d3ee70973b1c4ee9bd09969c185b/src/java.base/share/classes/sun/text/CompactByteArray.java#L85
Also for CompactByteArray, maybe it's worth checking that defaultValue is
different than 0 before entering the for loop (Line 80)

Let me know if you're interested in fixing them and if you want me to fill
2 bugs for them in the JBS or in bugreport.java.com.

Best regards,
Anthony Goubard

Integrated: 8310644: Make panama memory segment close use async handshakes

[Erik Österlund]

On Thu, 23 Nov 2023 11:14:29 GMT, Erik Österlund  wrote:

> The current logic for closing memory in panama today is susceptible to live 
> lock if we have a closing thread that wants to close the memory in a loop 
> that keeps failing, and a bunch of accessing threads that want to perform 
> accesses as long as the memory is alive. They can both create impediments for 
> the other.
> 
> By using asynchronous handshakes to install an exception onto threads that 
> are in @Scoped memory accesses, we can have close always succeed, and the 
> accessing threads bail out. The idea is that we perform a synchronous 
> handshake first to find threads that are in scoped methods. They might 
> however be in the middle of throwing an exception or something wild like 
> there, where an exception can't be delivered. We install an async handshake 
> that will roll us forward to the first place where we can indeed install 
> exceptions, then we reevaluate if we still need to do that, or if we have 
> unwound out from the scoped method. If we are still inside of it, we ensure 
> an exception is installed so we don't continue executing bytecodes that might 
> access the memory that we have freed.
> 
> Tested tier 1-5 as well as running test/jdk/java/foreign/TestHandshake.java 
> hundreds of times, which tests this API pretty well.

This pull request has now been integrated.

Changeset: 15946532
Author:Erik Österlund 
URL:   
https://git.openjdk.org/jdk/commit/159465324fc45325d0df438991032ebca9229ca2
Stats: 222 lines in 8 files changed: 76 ins; 63 del; 83 mod

8310644: Make panama memory segment close use async handshakes

Reviewed-by: jvernee, mcimadamore, pchilanomate

-

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

Re: RFR: 8310644: Make panama memory segment close use async handshakes [v3]

[Erik Österlund]

On Thu, 23 Nov 2023 16:32:28 GMT, Jorn Vernee  wrote:

>> Erik Österlund has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Comments from Jorn
>
> LGTM

Thanks for the reviews @JornVernee @pchilano and @mcimadamore!

-

PR Comment: https://git.openjdk.org/jdk/pull/16792#issuecomment-1831817744

RFR: 8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is trusted

[Sergey Tsypanov]

It looks like we can skip copying of `byte[]` in 
`BufferedInputStream.implTransferTo()` for `OutputStreams` residing in 
`java.io`.

See comment by @vlsi in 
https://github.com/openjdk/jdk/pull/10525/files#diff-e19c508d1bb6ee78697ecca66947c395adda0d9c49a85bf696e677ecbd977af1R612

-

Commit messages:
 - 8320971: Use BufferedInputStream.class.getPackageName()
 - 8320971: Use BufferedInputStream.buf directly when param of implTransferTo() 
is trusted

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

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

Re: RFR: JDK-8320940: Fix typo in java.lang.Double

[Lance Andersen]

On Wed, 29 Nov 2023 02:00:14 GMT, Joe Darcy  wrote:

> Typo fix to to the new text added in JDK-8295391.

Marked as reviewed by lancea (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/16872#pullrequestreview-1755023783

Re: RFR: JDK-8320940: Fix typo in java.lang.Double

[Aleksey Shipilev]

On Wed, 29 Nov 2023 02:00:14 GMT, Joe Darcy  wrote:

> Typo fix to to the new text added in JDK-8295391.

Marked as reviewed by shade (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/16872#pullrequestreview-1754814207

Integrated: 8320858: Move jpackage tests to tier3

[Alan Bateman]

On Tue, 28 Nov 2023 08:51:17 GMT, Alan Bateman  wrote:

> Update the jtreg test group configuration in the jdk test tree so that the 
> jpackage tests run in tier3 rather than tier2.
> 
> At this time, the jpackage tests run in jdk:tier2, more specifically 
> jdk:tier2_part2 as part of the core_tools test group. The  jpackage tests 
> take a significant amount of time on some platforms (on macOS and Windows in 
> particular). The changes here remove tools/jpackage from core_tools (it's not 
> a core tool anyway) and adds the tests to tier3. In the future then maybe 
> tier3 can be split up if needed.

This pull request has now been integrated.

Changeset: e44d4b24
Author:Alan Bateman 
URL:   
https://git.openjdk.org/jdk/commit/e44d4b24ed794957c47c140ab6f15544efa2b278
Stats: 11 lines in 1 file changed: 6 ins; 0 del; 5 mod

8320858: Move jpackage tests to tier3

Reviewed-by: mchung, asemenyuk, almatvee

-

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

Re: RFR: 8308753: Class-File API transition to Preview [v32]

[Per Minborg]

On Tue, 28 Nov 2023 13:03:48 GMT, Adam Sotona  wrote:

>> src/java.base/share/classes/java/lang/classfile/ClassFile.java line 1404:
>> 
>>> (failed to retrieve contents of file, check the PR for context)
>> Is there a more scalable way to express the major class version? The current 
>> approach means we have to add two fields per year.  Maybe a lookup function 
>> would be better?
>
> This is explicit list of supported class file versions, so I don't see any 
> other way.

Would it not be possible to expose an immutable `Map` that 
maps from Java version to major class version?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/15706#discussion_r1408941961