Re: RFR: 8323782: Race: Thread::interrupt vs. AbstractInterruptibleChannel.begin [v2]

[Richard Reingruber]

On Wed, 17 Jan 2024 15:38:22 GMT, Richard Reingruber  wrote:

>> Set `interrupted` in `Thread::interrupt` before reading `nioBlocker` for 
>> correct (Dekker scheme) synchronization with concurrent execution of 
>> [`AbstractInterruptibleChannel::begin`](https://github.com/openjdk/jdk/blob/59062402b9c5ed5612a13c1c40eb22cf1b97c41a/src/java.base/share/classes/java/nio/channels/spi/AbstractInterruptibleChannel.java#L176).
>> 
>> The change passed our CI functional testing: JTReg tests: tier1-4 of hotspot 
>> and jdk. All of Langtools and jaxp. SPECjvm2008, SPECjbb2015, Renaissance 
>> Suite, and SAP specific tests.
>> Testing was done with fastdebug and release builds on the main platforms and 
>> also on Linux/PPC64le and AIX.
>
> Richard Reingruber has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Review Alan

I noticed that VirtualThread overrides `isInterrupted` 

https://github.com/openjdk/jdk/blob/05dad67cc23fb49627fabfb306acee247ff67aef/src/java.base/share/classes/java/lang/VirtualThread.java#L902-L905

with just the same implementation as Thread has:

https://github.com/openjdk/jdk/blob/05dad67cc23fb49627fabfb306acee247ff67aef/src/java.base/share/classes/java/lang/Thread.java#L1741-L1751

This potentially hinders performance. Is there a reason to have this override?

-

PR Comment: https://git.openjdk.org/jdk/pull/17444#issuecomment-1899918903

Re: RFR: 8322768: Optimize non-subword vector compress and expand APIs for AVX2 target. [v5]

[Emanuel Peter]

On Thu, 18 Jan 2024 17:06:55 GMT, Jatin Bhateja  wrote:

>> @jatin-bhateja so why do you shift by 5? I thought 4 longs are 32 bit?
>
> For long/double each permute row is 32 byte in size, so a shift by 5 to 
> compute row address.

Ah right. Maybe we could say `32byte = 4 long = 4 * 64bit`.
Because "64bit row" sounds like the whole row is only 64 bit long. It is 
actually the cells that are 64bits, not the rows!

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17261#discussion_r1458509886

Re: RFR: 8323782: Race: Thread::interrupt vs. AbstractInterruptibleChannel.begin [v2]

[David Holmes]

On Thu, 18 Jan 2024 09:21:24 GMT, Richard Reingruber  wrote:

>>> It is really safe/correct to move this outside the synchronized block? I 
>>> know things have changed a bit with loom but we've "always" held a lock 
>>> when doing the actual interrupt. I'd have to check the VM logic to be sure 
>>> it can be called concurrently from multiple threads for the same target 
>>> thread.
>> 
>> This hasn't changed. The interruptLock is used to coordinate the add/remove 
>> of the nioBlocker. When there is no nioBlocker set then the interrupt status 
>> and unparking (as in JavaThread::interrupt) has always executed without the 
>> interruptLock (named "blockerLock" in the past).
>
> I think that interrupting is just asynchronous to some extent.
> E.g. a thread polls its interrupt status clearing it thereby (without lock) 
> before calling nio. A concurrent interrupt can be lost then even if the lock 
> is acquired.
> (Maybe clearing should not be done by a public method)

Yep my bad on the VM side of things - no change there. But in the nioBlocker 
case doesn't this inherently make things more racy? Now maybe those races are 
allowed, but this might lead to a change in behaviour.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17444#discussion_r1458188281

Re: Avoiding Side Effects of ForkJoinPool.managedBlock

[David Holmes]

On 18/01/2024 7:30 pm, Johannes Spangenberg wrote:

Hello,

I have a question about dealing with side effects caused by ForkJoinPool. I am 
not certain if this mailing list is the appropriate platform, but I hope it is. 
The issue I am facing is that running code within a ForkJoinPool may change the 
behavior of the code. These changes in behavior have resulted in 
non-deterministic test failures in one of the repositories I work on. JUnit 
uses a ForkJoinPool when you enable parallel test execution. I would like to 
disable or avoid the side effects of various methods if called from a 
ForkJoinPool. So far, I haven't found a solution, except for completely 
replacing the use of the ForkJoinPool in JUnit with some custom scheduler which 
is not a ForkJoinPool.

Is there a solution to force an "unmanaged" block (as opposed to 
ForkJoinPool.managedBlock)? Is there alternatively a good approach to transfer CPU bound 
subtasks to another thread pool while blocking the ForkJoinWorkerThread without 
compensation? I have implemented a workaround which I explain below, but I am not sure if 
this will remain effective in future JDK versions. I am currently using JDK 17 but were 
also able to reproduce the issues with JDK 21.

I have observed the following side-effects caused by managed blocks or similar 
mechanisms:

1. Parallel stream methods execute another task (i.e. JUnit test) from the pool 
recursively, which is particularly problematic if your code utilizes any 
ThreadLocal.


The parallel stream implementation utilises the current ForkJoinPool, if 
any, else the common FJP.


David
-


2. Blocking methods spawn around 256 threads in total to "compensate" for the 
blocking operation. Consequently, you end up with up to 256 tests running concurrently, 
each of them might or might not be CPU bound (unknown to the ForkJoinPool).

3. Blocking methods may throw a RejectedExecutionException when the thread 
limit is reached. This is effectively a race condition which may lead to 
exceptions.

I have not been able to determine under which circumstances each behavior 
occurs. I am unaware of any thorough documentation that clearly outlines the 
expected behavior in different scenarios with different blocking methods. While 
(1.) and (3.) have caused test failures, (2.) simply causes JUnit to run 256 
tests in parallel instead of the intended 12. I attached a JUnit test to 
reproduce (1.) and (3.), but it might not fail on every run.

Many of the blocking methods of the standard library include a check if the current 
thread is an instance of ForkJoinWorkerThread. My current workaround involves wrapping 
the code that makes blocking calls into a FutureTask which is executed on another thread 
and then joining this task afterwards. As of now, FutureTask.get() seems not to implement 
any of the side-effects. As the missing instanceof-check in FutureTask makes it 
inconsistent with other Futures like CompletableFuture, I fear it might be considered a 
"bug". I would like to know a safe solution which is specified to continue to 
work in future JDKs.

PS: There is also a ticket on the JUnit project about this topic, but it only 
talks about side-effect (2.), but not the other side-effects we observed.
https://github.com/junit-team/junit5/issues/3108

Thanks,
Johannes

Re: RFR: 8324065: Daylight saving information for `Africa/Casablanca` are incorrect [v2]

[Naoto Sato]

On Thu, 18 Jan 2024 23:19:44 GMT, Justin Lu  wrote:

> LGTM, _NegativeDSTTest.java_ can bump the copyright year to 2024 as well.

Forgot that! Fixed.

-

PR Comment: https://git.openjdk.org/jdk/pull/17492#issuecomment-1899410249

Re: RFR: 8324065: Daylight saving information for `Africa/Casablanca` are incorrect [v3]

[Naoto Sato]

> Fixing incorrect std/dst transitions for time zones that have rules beyond 
> 2037. The year 2037 restriction seems to come from the old `zic` command 
> implementation and thus can be expanded in the JDK. Arbitrary I chose 2100 
> which is greater than the year 2087 which is the farthest rule at the moment.

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

  copyright year

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/17492/files
  - new: https://git.openjdk.org/jdk/pull/17492/files/9ce8f35b..cb652b81

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

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

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

Re: RFR: 8322744: VirtualThread.notifyJvmtiDisableSuspend should be static [v2]

[Serguei Spitsyn]

On Thu, 11 Jan 2024 13:09:39 GMT, Serguei Spitsyn  wrote:

>> The notification method `VirtualThread.notifyJvmtiDisableSuspend` should be 
>> static.
>> The method disables/enables suspend of the current virtual thread, a no-op 
>> if the current thread is a platform thread. It is confusing for this to be 
>> an instance method, it should be static to make it clearer that it doesn't 
>> change the target thread.
>> The notification method `VirtualThread.notifyJvmtiHideFrames` also has to be 
>> static as it does not use/need the virtual thread `this` argument.
>> One detail to underline is the intrinsic implementation needs to use the 
>> argument #0 instead of #1.
>> 
>> Testing:
>> - The mach5 tiers 1-6 show no regressions
>
> Serguei Spitsyn has updated the pull request with a new target base due to a 
> merge or a rebase. The incremental webrev excludes the unrelated changes 
> brought in by the merge/rebase. The pull request contains two additional 
> commits since the last revision:
> 
>  - Merge
>  - 8322744: VirtualThread.notifyJvmtiDisableSuspend should be static

PING! It would be nice to get this reviewed. :)

-

PR Comment: https://git.openjdk.org/jdk/pull/17298#issuecomment-1899386396

Re: RFR: 8323699: MessageFormat.toPattern() generates non-equivalent MessageFormat pattern [v2]

[Archie Cobbs]

On Thu, 18 Jan 2024 23:02:57 GMT, Justin Lu  wrote:

> Sorry if I'm going over stuff you already know...

No apology needed, this stuff is obscure detail madness!

> So with this change, although calling `toPattern()` on `new 
> MessageFormat("{0,choice,0.0#foo {1} {2} {3} }")` would return the String 
> `"{0,choice,0.0#foo '{'1'}' '{'2'}' '{'3'}' }"` which **I** would think, 
> formats differently, **in reality**, they format equivalently, so I think it 
> is okay. I brought it up not necessarily as an issue, but just something to 
> make apparent. Although I suppose something like this is up for 
> interpretation, so perhaps its only wrong to me.

OK gotcha. I totally agree that what you think it should do makes a lot more 
sense that what it actually does!

-

PR Comment: https://git.openjdk.org/jdk/pull/17416#issuecomment-1899385354

Re: RFR: 8324065: Daylight saving information for `Africa/Casablanca` are incorrect [v2]

[Justin Lu]

On Thu, 18 Jan 2024 21:02:26 GMT, Naoto Sato  wrote:

>> Fixing incorrect std/dst transitions for time zones that have rules beyond 
>> 2037. The year 2037 restriction seems to come from the old `zic` command 
>> implementation and thus can be expanded in the JDK. Arbitrary I chose 2100 
>> which is greater than the year 2087 which is the farthest rule at the moment.
>
> Naoto Sato has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Added comment to clarify the possible need for the last year expansion

LGTM, _NegativeDSTTest.java_ can bump the copyright year to 2024 as well.

-

Marked as reviewed by jlu (Committer).

PR Review: https://git.openjdk.org/jdk/pull/17492#pullrequestreview-1830712470

Re: RFR: 8323699: MessageFormat.toPattern() generates non-equivalent MessageFormat pattern [v2]

[Justin Lu]

On Wed, 17 Jan 2024 21:31:22 GMT, Archie Cobbs  wrote:

>> Please consider this fix to ensure that going from `MessageFormat` to 
>> pattern string via `toPattern()` and then back via `new MessageFormat()` 
>> results in a format that is equivalent to the original.
>> 
>> The quoting and escaping rules for `MessageFormat` pattern strings are 
>> really tricky. I admit not completely understanding them. At a high level, 
>> they work like this: The normal way one would "nest" strings containing 
>> special characters is with straightforward recursive escaping like with the 
>> `bash` command line. For example, if you want to echo `a "quoted string" 
>> example` then you enter `echo "a "quoted string" example"`. With this scheme 
>> it's always the "outer" layer's job to (un)escape special characters as 
>> needed. That is, the echo command never sees the backslash characters.
>> 
>> In contrast, with `MessageFormat` and friends, nested subformat pattern 
>> strings are always provided "pre-escaped". So to build an "outer" string 
>> (e.g., for `ChoiceFormat`) the "inner" subformat pattern strings are more or 
>> less just concatenated, and then only the `ChoiceFormat` option separator 
>> characters (e.g., `<`, `#`, `|`, etc.) are escaped.
>> 
>> The "pre-escape" escaping algorithm escapes `{` characters, because `{` 
>> indicates the beginning of a format argument. However, it doesn't escape `}` 
>> characters. This is OK because the format string parser treats any "extra" 
>> closing braces (where "extra" means not matching an opening brace) as plain 
>> characters.
>> 
>> So far, so good... at least, until a format string containing an extra 
>> closing brace is nested inside a larger format string, where the extra 
>> closing brace, which was previously "extra", can now suddenly match an 
>> opening brace in the outer pattern containing it, thus truncating it by 
>> "stealing" the match from some subsequent closing brace.
>> 
>> An example is the `MessageFormat` string `"{0,choice,0.0#option A: 
>> {1}|1.0#option B: {1}'}'}"`. Note the second option format string has a 
>> trailing closing brace in plain text. If you create a `MessageFormat` with 
>> this string, you see a trailing `}` only with the second option.
>> 
>> However, if you then invoke `toPattern()`, the result is 
>> `"{0,choice,0.0#option A: {1}|1.0#option B: {1}}}"`. Oops, now because the 
>> "extra" closing brace is no longer quoted, it matches the opening brace at 
>> the beginning of the string, and the following closing  brace, which was the 
>> previous match, is now just plain text in the outer `MessageFormat` string.
>> 
>> As a result, invoking `f.format(new ...
>
> Archie Cobbs has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Quote '{' and '}' in subformat patterns, but only it not already quoted.

Hi Archie,

> The XXX that you see inside a MessageFormat pattern string as part of a 
> {0,choice,XXX} subformat.

I think we both agree that XXX may not be the same as the Subformat pattern 
string.

Sorry if I'm going over stuff you already know, but to clarify my previous 
statement, MessageFormat escapes brackets so that they are syntactically not 
evaluated and simply injected into the pattern as a literal bracket. Why I 
brought up ChoiceFormat, is that it is unique in the sense that you can insert 
a `{ FormatElement }` into a subFormat pattern, which causes recursion.

Take the example from the class specification, 

`new MessageFormat(There {0,choice,0#are no files|1#is one file|1https://git.openjdk.org/jdk/pull/17416#issuecomment-1899355910

Integrated: 8324053: Use the blessed modifier order for sealed in java.base

[Pavel Rappo]

On Wed, 17 Jan 2024 21:22:07 GMT, Pavel Rappo  wrote:

> Please review this trivial PR to reorder the `sealed` class or interface 
> modifier. For context of this change see: 
> https://github.com/openjdk/jdk/pull/17242#issuecomment-1887338396.

This pull request has now been integrated.

Changeset: 9efdd242
Author:Pavel Rappo 
URL:   
https://git.openjdk.org/jdk/commit/9efdd242fb40a8270e489cc071ff1c891878e24f
Stats: 8 lines in 7 files changed: 0 ins; 0 del; 8 mod

8324053: Use the blessed modifier order for sealed in java.base

Reviewed-by: naoto, darcy, ihse, dfuchs

-

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

Re: RFR: 8324065: Daylight saving information for `Africa/Casablanca` are incorrect [v2]

[Joe Wang]

On Thu, 18 Jan 2024 21:02:26 GMT, Naoto Sato  wrote:

>> Fixing incorrect std/dst transitions for time zones that have rules beyond 
>> 2037. The year 2037 restriction seems to come from the old `zic` command 
>> implementation and thus can be expanded in the JDK. Arbitrary I chose 2100 
>> which is greater than the year 2087 which is the farthest rule at the moment.
>
> Naoto Sato has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Added comment to clarify the possible need for the last year expansion

You've saved generations of engineers from having to look at it again. By then, 
the dev AI will look at it and ...

-

Marked as reviewed by joehw (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17492#pullrequestreview-1830645855

Re: RFR: 8324065: Daylight saving information for `Africa/Casablanca` are incorrect [v2]

[Iris Clark]

On Thu, 18 Jan 2024 21:02:26 GMT, Naoto Sato  wrote:

>> Fixing incorrect std/dst transitions for time zones that have rules beyond 
>> 2037. The year 2037 restriction seems to come from the old `zic` command 
>> implementation and thus can be expanded in the JDK. Arbitrary I chose 2100 
>> which is greater than the year 2087 which is the farthest rule at the moment.
>
> Naoto Sato has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Added comment to clarify the possible need for the last year expansion

Still looks good!

-

Marked as reviewed by iris (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17492#pullrequestreview-1830637536

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

[Mandy Chung]

On Mon, 11 Dec 2023 16:37:38 GMT, Severin Gehwolf  wrote:

>> Please review this patch which adds a jlink mode to the JDK which doesn't 
>> need the packaged modules being present. A.k.a run-time image based jlink. 
>> 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 `JRTArchive` 
>> 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 jdk.jlink) 
>> <(../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.i...
>
> Severin Gehwolf has updated the pull request incrementally with two 
> additional commits since the last revision:
> 
>  - Disallow packaged modules and run-time image link
>  - Only check for existing path when not a scratch task
>
>When using a run-time image link and the initial build was produced with
>the --keep-packaged-modules option, we don't need to check existing
>paths to the location where packaged modules need to be copied. This
>breaks the --verbose output validation.

> Which filtering plugins do you see with category TRANSFORMER? perhaps? Happy 
> to move them to the FILTER category.

Yes `strip-java-debug-attributes` and `strip-native-debug-symbols`  are the 
ones.  `dedup-legal-notices` is another one.

> > The use of `--exclude-resources` plugin to exclude transformed data to 
> > restore the data back to original form is clever and works for plugins that 
> > add new resources. But it does not work for plugins that modifying existing 
> > data (`--vendor-bug-url` etc plugins). The change in 
> > `TaskHelper::getPluginsConfig` raises the question that it isn't the right 
> > way to support this feature. I think we need to explore a better way to add 
> > this support. One possibility I consider is to run non-filtering plugins of 
> > ADDER and TRANSFORMER categories always (similar to auto-enabled). It has 
> > to determine if it's requested by the user or to restore the data to 
> > original form via `Plugin::configure` time. `Plugin::transform` will handle 
> > if the data is from packaged-modules and from runtime image which may 
> > contain the data transformed by this plugin. I haven't explored this fully 
> > yet. More discussion is needed and Alan may have opinions.
> 
> So would it be acceptable if I changed those plugins to restore the old 
> behaviour if they were not requested by the user with the respective cli 
> option? Basically, my thinking would be that `Plugin::configure` would gain 
> extra argument so as to determined whether this is a runtime-image based link 
> and triggered by the current jlink run with its CLI option. This should be 
> sufficient to configure for `transform` appropriately.

My intent is to bring up my observation for discussion.   The plugin authors 
are the ones who should decide if the 

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

[Mandy Chung]

On Mon, 11 Dec 2023 16:37:38 GMT, Severin Gehwolf  wrote:

>> Please review this patch which adds a jlink mode to the JDK which doesn't 
>> need the packaged modules being present. A.k.a run-time image based jlink. 
>> 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 `JRTArchive` 
>> 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 jdk.jlink) 
>> <(../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.i...
>
> Severin Gehwolf has updated the pull request incrementally with two 
> additional commits since the last revision:
> 
>  - Disallow packaged modules and run-time image link
>  - Only check for existing path when not a scratch task
>
>When using a run-time image link and the initial build was produced with
>the --keep-packaged-modules option, we don't need to check existing
>paths to the location where packaged modules need to be copied. This
>breaks the --verbose output validation.

`JRTArchive::collectFiles` is the relevant code:


// add/persist a special, empty file for jdk.jlink so as to support
// the single-hop-only run-time image jlink
if (singleHop && JDK_JLINK_MODULE.equals(module)) {
files.add(createRuntimeImageSingleHopStamp());
}

-

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

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

[Mandy Chung]

On Thu, 18 Jan 2024 13:37:12 GMT, Severin Gehwolf  wrote:

> > If I read the code correctly, the image created with this option will 
> > enable multi-hops unconditionally? i.e. no timestamp file created and 
> > disable the check completely. I think the .stamp file should be present in 
> > any image created without packaged modules.
> 
> The option is currently used in tests (and can also be used to verify binary 
> equivalence of jlinking Java SE with and without packaged modules), which is 
> a nice property to have. If the stamp file is present in one, but not the 
> other this is sufficient to fail the equivalence test.

What I tried to point out is the following:

1. run `myruntime/bin/jlink` to create `image1` without packaged module
2. run `image1/bin/jlink` to create a runtime image will fail
3. run `image1/bin/jlink --ignore-modified-runtime` will successfully to create 
`image2`

I expect `image2/bin/jlink` should fail creating a runtime image since it's 
multi-hop.   Another scenario: If I modify `image2/conf/net.properties` , 
`image2/bin/jlink` should fail.  In both cases, `image2/bin/jlink 
--ignore-modified-runtime` will ignore the error and create the runtime image.

-

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

Re: RFR: JDK-8266431: Dual-Pivot Quicksort improvements (Radix sort) [v11]

[Vladimir Yaroslavskiy]

On Mon, 11 Dec 2023 03:42:51 GMT, Srinivas Vamsi Parasa  
wrote:

>> Hello Vamsi (@vamsi-parasa),
>> 
>> I made the process simpler: added all variants to be compared into 
>> ArraysSort class
>> (set the same package org.openjdk.bench.java.util). It will run all sorts 
>> incl. sort from jdk
>> in the same environment. It should provide more accurate results, otherwise 
>> we see some anomalies.
>> 
>> Could you please find time to run the benchmarking?
>> Take all classes below and put them in the package 
>> org.openjdk.bench.java.util.
>> https://github.com/iaroslavski/sorting/blob/master/radixsort/ArraysSort.java
>> 
>> https://github.com/iaroslavski/sorting/blob/master/radixsort/DualPivotQuicksort_a10.java
>> https://github.com/iaroslavski/sorting/blob/master/radixsort/DualPivotQuicksort_r14.java
>> https://github.com/iaroslavski/sorting/blob/master/radixsort/DualPivotQuicksort_r17.java
>> https://github.com/iaroslavski/sorting/blob/master/radixsort/DualPivotQuicksort_r18.java
>> 
>> Many thanks,
>> Vladimir
>
> Hi Vladimir (@iaroslavski)
> 
> Please see the data below using the latest version of AVX512 sort that got 
> integrated into OpenJDK.
> 
>  xmlns:o="urn:schemas-microsoft-com:office:office"
> xmlns:x="urn:schemas-microsoft-com:office:excel"
> xmlns="http://www.w3.org/TR/REC-html40;>
> 
> 
> 
> 
> 
>  href="file:///C:/Users/sparasa/AppData/Local/Temp/msohtmlclip1/01/clip.htm">
>  href="file:///C:/Users/sparasa/AppData/Local/Temp/msohtmlclip1/01/clip_filelist.xml">
> 
> 
> 
> 
> 
> 
> 
> 
> Benchmark   (us/op) | (builder) | Stock JDK | a10 | r14 | r17 | r18
> -- | -- | -- | -- | -- | -- | --
> ArraysSort.Int.testSort | RANDOM | 2.202 | 2.226 | 1.535 | 1.556 | 1.546
> ArraysSort.Int.testSort | RANDOM | 35.128 | 34.804 | 30.808 | 30.914 | 31.284
> ArraysSort.Int.testSort | RANDOM | 78.571 | 77.224 | 72.567 | 73.098 | 73.337
> ArraysSort.Int.testSort | RANDOM | 2466.487 | 2470.66 | 2504.654 | 2494.051 | 
> 2499.746
> ArraysSort.Int.testSort | RANDOM | 20704.14 | 20668.19 | 21377.73 | 21362.63 
> | 21278.94
> ArraysSort.Int.testSort | REPEATED | 0.877 | 0.892 | 0.74 | 0.724 | 0.718
> ArraysSort.Int.testSort | REPEATED | 4.789 | 4.788 | 4.92 | 4.721 | 4.891
> ArraysSort.Int.testSort | REPEATED | 11.172 | 11.778 | 11.53 | 11.467 | 11.406
> ArraysSort.Int.testSort | REPEATED | 207.212 | 207.292 | 255.46 | 258.832 | 
> 254.44
> ArraysSort.Int.testSort | REPEATED | 1862.544 | 1876.759 | 1952.646 | 
> 1957.978 | 1981.906
> ArraysSort.Int.testSort | STAGGER | 2.092 | 2.137 | 1.999 | 2.031 | 2.015
> ArraysSort.Int.testSort | STAGGER | 29.891 | 30.321 | 25.626 | 26.318 | 26.396
> ArraysSort.Int.testSort | STAGGER | 60.979 | 83.439 | 57.864 | 57.213 | 79.762
> ArraysSort.Int.testSort | STAGGER | 1227.933 | 1224.495 | 1236.133 | 1229.773 
> | 1228.877
> ArraysSort.Int.testSort | STAGGER | 9514.873 | 9565.599 | 9491.509 | 9481.147 
> | 9481.905
> ArraysSort.Int.testSort | SHUFFLE | 1.608 | 1.595 | 1.419 | 1.442 | 1.491
> ArraysSort.Int.testSort | SHUFFLE | 31.566 | 32.789 | 28.718 | 28.768 | 28.671
> ArraysSort.Int.testSort | SHUFFLE | 82.157 | 83.741 | 70.889 | 69.951 | 71.196
> ArraysSort.Int.testSort | SHUFFLE | 2251.219 | 2248.496 | 2184.459 | 2163.969 
> | 2156.239
> ArraysSort.Int.testSort | SHUFFLE | 18211.05 | 18223.24 | 17987.4 | 18114.26 
> | 17994.98
> 
> 
> 
> 
> 
> 
> 
> Thanks,
> Vamsi

Hello Vamsi (@vamsi-parasa),

Could you please run the benchmarking of new DQPS in your environment with AVX?

Take all classes below and put them in the package org.openjdk.bench.java.util.
ArraysSort class contains all tests for the new versions and ready to use.
(it will run all tests in one execution).

https://github.com/iaroslavski/sorting/blob/master/radixsort/ArraysSort.java
https://github.com/iaroslavski/sorting/blob/master/radixsort/DualPivotQuicksort_a15.java
https://github.com/iaroslavski/sorting/blob/master/radixsort/DualPivotQuicksort_r20s.java
https://github.com/iaroslavski/sorting/blob/master/radixsort/DualPivotQuicksort_r20p.java

Many thanks,
Vladimir

-

PR Comment: https://git.openjdk.org/jdk/pull/13568#issuecomment-1899240013

Re: RFR: 8323699: MessageFormat.toPattern() generates non-equivalent MessageFormat pattern

[Archie Cobbs]

On Thu, 18 Jan 2024 20:08:27 GMT, Justin Lu  wrote:

>> Hi @justin-curtis-lu,
>> 
>> Thanks a lot for taking a look, I am glad for any other set of eyes on this 
>> tricky stuff!
>> 
>>> As it stands, it would be inconsistent to have the closing bracket quoted 
>>> and the opening bracket not quoted.
>> 
>> You're right - the problem exists with both `{` and `}`, as is shown here 
>> (unmodified jshell):
>> 
>> 
>> $ jshell
>> |  Welcome to JShell -- Version 17.0.9
>> |  For an introduction type: /help intro
>> 
>> jshell> import java.text.*;
>> 
>> jshell> new MessageFormat("Test: {0,number,foo'{'#.00}");
>> $2 ==> java.text.MessageFormat@951c5b58
>> 
>> jshell> $2.toPattern()
>> $3 ==> "Test: {0,number,foo{#.00}"
>> 
>> jshell> new MessageFormat($3)
>> |  Exception java.lang.IllegalArgumentException: Unmatched braces in the 
>> pattern.
>> |at MessageFormat.applyPattern (MessageFormat.java:521)
>> |at MessageFormat. (MessageFormat.java:371)
>> |at (#4:1)
>> 
>> 
>> I've been missing the forest for the trees a bit and I think the fix can be 
>> simpler now.
>> 
>> For the record, here is my latest understanding of what's going on...
>> 
>> 1. When `MessageFormat.toPattern()` constructs the combined pattern string, 
>> it concatenates the plain text bits, suitably escaped, and the pattern 
>> strings from each subformat.
>> 1. The subformat strings are already escaped, in the sense that you can take 
>> (for example) a `ChoiceFormat` format string and use it as-is to recreate 
>> that same `ChoiceFormat`, but they are escaped _only for their own purposes_.
>> 1. The original example is an example of where this matters - `ChoiceFormat` 
>> needs to escape `#`, `|`, etc., but doesn't need to escape `{` or `}` - but 
>> a `MessageFormat` format string does need to escape those.
>> 1. Therefore, the correct fix is for `MessageFormat.toPattern()` to modify 
>> the subformat strings by quoting any _unquoted_ `{` or `}` characters while 
>> leaving any already-quoted characters alone.
>> 
>> Updated in a9d78c76f5f. I've also updated the test so it does more thorough 
>> round-trip checks.
>
> Hi @archiecobbs , I think the recent commit is good progress.
> 
> First off I think this change will need a CSR as there are behavioral 
> concerns, although minimal. Please let me know if you have access on JBS to 
> file one, otherwise I can file one for you.
> 
>> Therefore, the correct fix is for MessageFormat.toPattern() to modify the 
>> subformat strings by quoting any unquoted {or } characters while leaving any 
>> already-quoted characters alone.
> 
> Yes, I think this behavior allows for the String returned by `toPattern()` to 
> create a MessageFormat that can format equivalently to the original 
> MessageFormat. Although to clarify, the original String pattern will not be 
> guaranteed to be equivalent to the one returned by `toPattern()` as we are 
> adding quotes to all brackets in the subformatPattern, regardless if they 
> were quoted in the original String. I think the former is more important 
> here, so the latter is okay.
> 
> For DecimalFormat and SimpleDateFormat subformats, adding quotes to brackets 
> found in the subformatPattern is always right, those subformats can not be 
> used to create recursive format behavior. Thus you would **never** except a 
> nested ArgumentIndex in the subformatPattern (ex: `"{0,number,{1}foo0.0"}`), 
> and can confidently escape all brackets found for these subFormats (which you 
> do).
> 
> To me, ChoiceFormat is where there is some concern. Consider the following,
> 
> `new MessageFormat("{0,choice,0# {1} {2} {3} }”)`
> 
> With this change, invoking `toPattern()` on the created MessageFormat would 
> return 
> 
> `"{0,choice,0.0# '{'1'}' '{'2'}' '{'3'}' }"`.
> 
> This would technically be incorrect. One would think instead of allowing 3 
> nested elements, we are now printing the literal `{1} {2} {3}` since the 
> brackets are escaped. But, this is not actually the case. Escaping brackets 
> with a choice subFormat does not function properly. This is due to the fact 
> that a recursive messageFormat is created, but the pattern passed has already 
> lost the quotes.
> 
> This means that `new MessageFormat("{0,choice,0.0# '{'1'}' '{'2'}' '{'3'}' 
> }")`.
> 
> is still equivalent to `new MessageFormat("{0,choice,0# {1} {2} {3} }”).`
> 
> So the behavior of quoting all brackets would still guarantee _the String 
> returned by `toPattern()` to create a MessageFormat that can format 
> equivalently to the original MessageFormat_ but only because the current 
> behavior of formatting with a choice subFormat is technically wrong. I think 
> this is okay, since this incorrect behavior is long-standing, but a CSR would 
> be good to ad...

Hi @justin-curtis-lu,

Thanks for your comments.

> First off I think this change will need a CSR as there are behavioral 
> concerns, although minimal. Please let me know if you have access on JBS to 
> file one, 

Re: RFR: 8324065: Daylight saving information for `Africa/Casablanca` are incorrect [v2]

[Naoto Sato]

> Fixing incorrect std/dst transitions for time zones that have rules beyond 
> 2037. The year 2037 restriction seems to come from the old `zic` command 
> implementation and thus can be expanded in the JDK. Arbitrary I chose 2100 
> which is greater than the year 2087 which is the farthest rule at the moment.

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

  Added comment to clarify the possible need for the last year expansion

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/17492/files
  - new: https://git.openjdk.org/jdk/pull/17492/files/1779673d..9ce8f35b

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

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

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

Re: RFR: 8324065: Daylight saving information for `Africa/Casablanca` are incorrect [v2]

[Naoto Sato]

On Thu, 18 Jan 2024 20:48:03 GMT, Iris Clark  wrote:

> Hopefully "2100" is unique enough that you'll be able to easily 
> search/replace the next time you need to extend.

Thank you, Iris. Added a comment to clarify the need for possible expansion.

-

PR Comment: https://git.openjdk.org/jdk/pull/17492#issuecomment-1899195604

Re: RFR: 8324065: Daylight saving information for `Africa/Casablanca` are incorrect

[Iris Clark]

On Thu, 18 Jan 2024 19:37:33 GMT, Naoto Sato  wrote:

> Fixing incorrect std/dst transitions for time zones that have rules beyond 
> 2037. The year 2037 restriction seems to come from the old `zic` command 
> implementation and thus can be expanded in the JDK. Arbitrary I chose 2100 
> which is greater than the year 2087 which is the farthest rule at the moment.

Hopefully "2100" is unique enough that you'll be able to easily search/replace 
the next time you need to extend.

-

Marked as reviewed by iris (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17492#pullrequestreview-1830530349

Re: RFR: JDK-8263261 Extend String::translateEscapes to support unicode escapes [v2]

[Jim Laskey]

> Currently String::translateEscapes does not support unicode escapes, reported 
> as a IllegalArgumentException("Invalid escape sequence: ..."). 
> String::translateEscapes should translate unicode escape sequences to provide 
> full coverage,

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

  Recommended changes

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/17491/files
  - new: https://git.openjdk.org/jdk/pull/17491/files/4ae16f8b..9deac037

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

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

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

Re: RFR: JDK-8263261 Extend String::translateEscapes to support unicode escapes

[Jim Laskey]

On Thu, 18 Jan 2024 19:25:28 GMT, Raffaello Giulietti  
wrote:

>> Currently String::translateEscapes does not support unicode escapes, 
>> reported as a IllegalArgumentException("Invalid escape sequence: ..."). 
>> String::translateEscapes should translate unicode escape sequences to 
>> provide full coverage,
>
> src/java.base/share/classes/java/lang/String.java line 4274:
> 
>> 4272: break;
>> 4273: case 'u':
>> 4274: if (from + 4 <= length) {
> 
> Avoids a potential overflow
> Suggestion:
> 
> if (from <= length - 4) {

Good.

> src/java.base/share/classes/java/lang/String.java line 4281:
> 
>> 4279: } catch (NumberFormatException ex) {
>> 4280: throw new 
>> IllegalArgumentException("Invalid unicode sequence: " + hex);
>> 4281: }
> 
> Avoids an allocation on a valid sequence, but is perhaps slower.
> Suggestion:
> 
> from += 4;
> try {
> ch = (char) Integer.parseInt(this, from - 4, 
> from, 16);
> } catch (NumberFormatException ex) {
> throw new IllegalArgumentException("Invalid 
> unicode sequence: " + substring(from - 4, from));
> }

Good.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17491#discussion_r1457941024
PR Review Comment: https://git.openjdk.org/jdk/pull/17491#discussion_r1457941676

Re: RFR: 8323699: MessageFormat.toPattern() generates non-equivalent MessageFormat pattern

[Justin Lu]

On Wed, 17 Jan 2024 21:31:49 GMT, Archie Cobbs  wrote:

>> Hi Archie, thanks for the proposed fix. I am still taking a look, but I 
>> wanted to demonstrate a current issue,
>> 
>> (Jshell with your patch)
>> 
>> 
>> var pattIn = "Test: {0,number,foo'{'#.00}";
>> MessageFormat mFmt = new MessageFormat(pattIn);
>> var pattOut  = mFmt.toPattern(); // returns "Test: {0,number,foo{#.00}";
>> 
>> 
>> 
>> var pattIn = "Test: {0,number,foo'}'#.00}";
>> MessageFormat mFmt = new MessageFormat(pattIn);
>> var pattOut  = mFmt.toPattern(); // returns "Test: {0,number,foo'}'#.00}";
>> 
>> 
>> As it stands, it would be inconsistent to have the closing bracket quoted 
>> and the opening bracket not quoted. 
>> 
>> Also in response to your earlier question on core-libs-dev, ideally invoking 
>> toPattern() can roundtrip, but there are known issues, such as a custom user 
>> defined Format subclass, or one of the newer Format subclasses that do not 
>> implement the toPattern() method. I am working on making this apparent in 
>> the specification of the method in a separate issue.
>
> Hi @justin-curtis-lu,
> 
> Thanks a lot for taking a look, I am glad for any other set of eyes on this 
> tricky stuff!
> 
>> As it stands, it would be inconsistent to have the closing bracket quoted 
>> and the opening bracket not quoted.
> 
> You're right - the problem exists with both `{` and `}`, as is shown here 
> (unmodified jshell):
> 
> 
> $ jshell
> |  Welcome to JShell -- Version 17.0.9
> |  For an introduction type: /help intro
> 
> jshell> import java.text.*;
> 
> jshell> new MessageFormat("Test: {0,number,foo'{'#.00}");
> $2 ==> java.text.MessageFormat@951c5b58
> 
> jshell> $2.toPattern()
> $3 ==> "Test: {0,number,foo{#.00}"
> 
> jshell> new MessageFormat($3)
> |  Exception java.lang.IllegalArgumentException: Unmatched braces in the 
> pattern.
> |at MessageFormat.applyPattern (MessageFormat.java:521)
> |at MessageFormat. (MessageFormat.java:371)
> |at (#4:1)
> 
> 
> I've been missing the forest for the trees a bit and I think the fix can be 
> simpler now.
> 
> For the record, here is my latest understanding of what's going on...
> 
> 1. When `MessageFormat.toPattern()` constructs the combined pattern string, 
> it concatenates the plain text bits, suitably escaped, and the pattern 
> strings from each subformat.
> 1. The subformat strings are already escaped, in the sense that you can take 
> (for example) a `ChoiceFormat` format string and use it as-is to recreate 
> that same `ChoiceFormat`, but they are escaped _only for their own purposes_.
> 1. The original example is an example of where this matters - `ChoiceFormat` 
> needs to escape `#`, `|`, etc., but doesn't need to escape `{` or `}` - but a 
> `MessageFormat` format string does need to escape those.
> 1. Therefore, the correct fix is for `MessageFormat.toPattern()` to modify 
> the subformat strings by quoting any _unquoted_ `{` or `}` characters while 
> leaving any already-quoted characters alone.
> 
> Updated in a9d78c76f5f. I've also updated the test so it does more thorough 
> round-trip checks.

Hi @archiecobbs , I think the recent commit is good progress.

First off I think this change will need a CSR as there are behavioral concerns, 
although minimal. Please let me know if you have access on JBS to file one, 
otherwise I can file one for you.

> Therefore, the correct fix is for MessageFormat.toPattern() to modify the 
> subformat strings by quoting any unquoted {or } characters while leaving any 
> already-quoted characters alone.

Yes, I think this behavior allows for the String returned by `toPattern()` to 
create a MessageFormat that can format equivalently to the original 
MessageFormat. Although to clarify, the original String pattern will not be 
guaranteed to be equivalent to the one returned by `toPattern()` as we are 
adding quotes to all brackets in the subformatPattern, regardless if they were 
quoted in the original String. I think the former is more important here, so 
the latter is okay.

For DecimalFormat and SimpleDateFormat subformats, adding quotes to brackets 
found in the subformatPattern is always right, those subformats can not be used 
to create recursive format behavior. Thus you would **never** except a nested 
ArgumentIndex in the subformatPattern (ex: `"{0,number,{1}foo0.0"}`), and can 
confidently escape all brackets found for these subFormats (which you do).

To me, ChoiceFormat is where there is some concern. Consider the following,

`new MessageFormat("{0,choice,0# {1} {2} {3} }”)`

With this change, invoking `toPattern()` on the created MessageFormat would 
return 

`"{0,choice,0.0# '{'1'}' '{'2'}' '{'3'}' }"`.

This would technically be incorrect. One would think instead of allowing 3 
nested elements, we are now printing the literal `{1} {2} {3}` since the 
brackets are escaped. But, this is not actually the case. Escaping brackets 
with a choice subFormat does not function properly. This is due to 

RFR: 8324065: Daylight saving information for `Africa/Casablanca` are incorrect

[Naoto Sato]

Fixing incorrect std/dst transitions for time zones that have rules beyond 
2037. The year 2037 restriction seems to come from the old `zic` command 
implementation and thus can be expanded in the JDK. Arbitrary I chose 2100 
which is greater than the year 2087 which is the farthest rule at the moment.

-

Commit messages:
 - initial commit

Changes: https://git.openjdk.org/jdk/pull/17492/files
 Webrev: https://webrevs.openjdk.org/?repo=jdk=17492=00
  Issue: https://bugs.openjdk.org/browse/JDK-8324065
  Stats: 41 lines in 6 files changed: 3 ins; 7 del; 31 mod
  Patch: https://git.openjdk.org/jdk/pull/17492.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/17492/head:pull/17492

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

Re: RFR: JDK-8263261 Extend String::translateEscapes to support unicode escapes

[Raffaello Giulietti]

On Thu, 18 Jan 2024 18:50:56 GMT, Jim Laskey  wrote:

> Currently String::translateEscapes does not support unicode escapes, reported 
> as a IllegalArgumentException("Invalid escape sequence: ..."). 
> String::translateEscapes should translate unicode escape sequences to provide 
> full coverage,

src/java.base/share/classes/java/lang/String.java line 4274:

> 4272: break;
> 4273: case 'u':
> 4274: if (from + 4 <= length) {

Avoids a potential overflow
Suggestion:

if (from <= length - 4) {

src/java.base/share/classes/java/lang/String.java line 4281:

> 4279: } catch (NumberFormatException ex) {
> 4280: throw new IllegalArgumentException("Invalid 
> unicode sequence: " + hex);
> 4281: }

Avoids an allocation on a valid sequence, but is perhaps slower.
Suggestion:

from += 4;
try {
ch = (char) Integer.parseInt(this, from - 4, from, 
16);
} catch (NumberFormatException ex) {
throw new IllegalArgumentException("Invalid unicode 
sequence: " + substring(from - 4, from));
}

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17491#discussion_r1457889653
PR Review Comment: https://git.openjdk.org/jdk/pull/17491#discussion_r1457897030

RFR: JDK-8263261 Extend String::translateEscapes to support unicode escapes

[Jim Laskey]

Currently String::translateEscapes does not support unicode escapes, reported 
as a IllegalArgumentException("Invalid escape sequence: ..."). 
String::translateEscapes should translate unicode escape sequences to provide 
full coverage,

-

Commit messages:
 - Unicode escape sequences in translateEscapes

Changes: https://git.openjdk.org/jdk/pull/17491/files
 Webrev: https://webrevs.openjdk.org/?repo=jdk=17491=00
  Issue: https://bugs.openjdk.org/browse/JDK-8263261
  Stats: 56 lines in 2 files changed: 50 ins; 5 del; 1 mod
  Patch: https://git.openjdk.org/jdk/pull/17491.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/17491/head:pull/17491

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

Integrated: 8324161: validate-source fails after JDK-8275338

[Daniel D . Daugherty]

On Thu, 18 Jan 2024 17:39:47 GMT, Daniel D. Daugherty  
wrote:

> A trivial fix to the copyright line in 
> test/jdk/jdk/jfr/event/io/TestSerializationMisdeclarationEvent.java.

This pull request has now been integrated.

Changeset: 5c874c19
Author:Daniel D. Daugherty 
URL:   
https://git.openjdk.org/jdk/commit/5c874c19cb08e5c10204a7ad47fb3075f65633db
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod

8324161: validate-source fails after JDK-8275338

Reviewed-by: darcy

-

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

Re: Integrated: 8324161: validate-source fails after JDK-8275338

[Daniel D . Daugherty]

On Thu, 18 Jan 2024 17:41:04 GMT, Joe Darcy  wrote:

>> A trivial fix to the copyright line in 
>> test/jdk/jdk/jfr/event/io/TestSerializationMisdeclarationEvent.java.
>
> Marked as reviewed by darcy (Reviewer).

@jddarcy - Thanks for the lightning fast review!

-

PR Comment: https://git.openjdk.org/jdk/pull/17490#issuecomment-1898936525

Re: Integrated: 8324161: validate-source fails after JDK-8275338

[Joe Darcy]

On Thu, 18 Jan 2024 17:39:47 GMT, Daniel D. Daugherty  
wrote:

> A trivial fix to the copyright line in 
> test/jdk/jdk/jfr/event/io/TestSerializationMisdeclarationEvent.java.

Marked as reviewed by darcy (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/17490#pullrequestreview-1830248699

Integrated: 8324161: validate-source fails after JDK-8275338

[Daniel D . Daugherty]

A trivial fix to the copyright line in 
test/jdk/jdk/jfr/event/io/TestSerializationMisdeclarationEvent.java.

-

Commit messages:
 - 8324161: validate-source fails after JDK-8275338

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

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

Re: RFR: 8159927: Add a test to verify JMOD files created in the images do not have debug symbols

[Jim Laskey]

On Wed, 17 Jan 2024 20:51:23 GMT, Mandy Chung  wrote:

> The build excludes the native debug symbols in JMOD files created for JDK 
> modules (see make/CreateJmods.gmk).   This PR adds a test to verify that 
> native debug symbols are excluded as expected.

LGTM

-

Marked as reviewed by jlaskey (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17467#pullrequestreview-1830228257

Re: RFR: 8322768: Optimize non-subword vector compress and expand APIs for AVX2 target. [v5]

[Jatin Bhateja]

On Tue, 16 Jan 2024 07:08:57 GMT, Emanuel Peter  wrote:

>> Each long/double permute lane holds 64 bit value.
>
> @jatin-bhateja so why do you shift by 5? I thought 4 longs are 32 bit?

For long/double each permute row is 32 byte in size, so a shift by 5 to compute 
row address.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17261#discussion_r1457747672

Re: RFR: 8322768: Optimize non-subword vector compress and expand APIs for AVX2 target. [v6]

[Jatin Bhateja]

> Hi,
> 
> Patch optimizes non-subword vector compress and expand APIs for x86 AVX2 only 
> targets.
> Upcoming E-core Xeons (Sierra Forest) and Hybrid CPUs only support AVX2 
> instruction set.
> These are very frequently used APIs in columnar database filter operation.
> 
> Implementation uses a lookup table to record permute indices. Table index is 
> computed using
> mask argument of compress/expand operation.
> 
> Following are the performance number of JMH micro included with the patch.
> 
> 
> System : Intel(R) Xeon(R) Platinum 8480+ (Sapphire Rapids)
> 
> Baseline:
> Benchmark (size)   Mode  CntScore   Error 
>   Units
> ColumnFilterBenchmark.filterDoubleColumn1024  thrpt2  142.767 
>  ops/ms
> ColumnFilterBenchmark.filterDoubleColumn2047  thrpt2   71.436 
>  ops/ms
> ColumnFilterBenchmark.filterDoubleColumn4096  thrpt2   35.992 
>  ops/ms
> ColumnFilterBenchmark.filterFloatColumn 1024  thrpt2  182.151 
>  ops/ms
> ColumnFilterBenchmark.filterFloatColumn 2047  thrpt2   91.096 
>  ops/ms
> ColumnFilterBenchmark.filterFloatColumn 4096  thrpt2   44.757 
>  ops/ms
> ColumnFilterBenchmark.filterIntColumn   1024  thrpt2  184.099 
>  ops/ms
> ColumnFilterBenchmark.filterIntColumn   2047  thrpt2   91.981 
>  ops/ms
> ColumnFilterBenchmark.filterIntColumn   4096  thrpt2   45.170 
>  ops/ms
> ColumnFilterBenchmark.filterLongColumn  1024  thrpt2  148.017 
>  ops/ms
> ColumnFilterBenchmark.filterLongColumn  2047  thrpt2   73.516 
>  ops/ms
> ColumnFilterBenchmark.filterLongColumn  4096  thrpt2   36.844 
>  ops/ms
> 
> Withopt:
> Benchmark (size)   Mode  Cnt Score   
> Error   Units
> ColumnFilterBenchmark.filterDoubleColumn1024  thrpt2  2051.707
>   ops/ms
> ColumnFilterBenchmark.filterDoubleColumn2047  thrpt2   914.072
>   ops/ms
> ColumnFilterBenchmark.filterDoubleColumn4096  thrpt2   489.898
>   ops/ms
> ColumnFilterBenchmark.filterFloatColumn 1024  thrpt2  5324.195
>   ops/ms
> ColumnFilterBenchmark.filterFloatColumn 2047  thrpt2  2587.229
>   ops/ms
> ColumnFilterBenchmark.filterFloatColumn 4096  thrpt2  1278.665
>   ops/ms
> ColumnFilterBenchmark.filterIntColumn   1024  thrpt2  4149.384
>   ops/ms
> ColumnFilterBenchmark.filterIntColumn   2047  thrpt2  1791.170
>   ops/ms
> ColumnFilterBenchmark.filterIntColumn   4096...

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

  Space fixup

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/17261/files
  - new: https://git.openjdk.org/jdk/pull/17261/files/c3f1c50e..3ed6b8bf

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

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

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

Integrated: 8275338: Add JFR events for notable serialization situations

[Raffaello Giulietti]

On Fri, 15 Dec 2023 17:34:53 GMT, Raffaello Giulietti  
wrote:

> Adds serialization misdeclaration events to JFR.

This pull request has now been integrated.

Changeset: bfd2afe5
Author:Raffaello Giulietti 
URL:   
https://git.openjdk.org/jdk/commit/bfd2afe5adc315928fdedbfbe73049d8774400de
Stats: 781 lines in 10 files changed: 781 ins; 0 del; 0 mod

8275338: Add JFR events for notable serialization situations

Reviewed-by: rriggs, egahlin

-

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

Re: Gatherer: spliterator characteristics are not propagated

[Viktor Klang]

And for A.andThen(B), A.flags & B.flags should work, the stream is sorted if 
both gatherers keep it sorted.
That is unfortunately not the case. That would presume that you can implement 
the composition such that it can preserve all the common flags. Some flags 
could be "dominant" and some "recessive" to use genetics nomenclature.

I suppose that if those flags already exist, it's because they have a purpose 
and i do not understand how it can make the other operations slower.

Extra invocations, extra storage, and extra composition overhead is not free. 
Since Stream is one-shot you need to include the construction cost with the 
execution cost. For something like an empty Stream construction cost scan be 
90+% of the total costs.

Cheers,
√


Viktor Klang
Software Architect, Java Platform Group
Oracle


From: fo...@univ-mlv.fr 
Sent: Thursday, 18 January 2024 16:17
To: Viktor Klang 
Cc: core-libs-dev ; Paul Sandoz 

Subject: [External] : Re: Gatherer: spliterator characteristics are not 
propagated




From: "Viktor Klang" 
To: "Remi Forax" 
Cc: "core-libs-dev" , "Paul Sandoz" 

Sent: Thursday, January 18, 2024 3:36:07 PM
Subject: Re: Gatherer: spliterator characteristics are not propagated
I suspect that it is a rather slippery slope, once KEEP-flags are added, then 
others will want to be able to have INJECT-flags, and then people might have 
different opinions w.r.t. the default should be to clear all flags etc.

And that's even before one looks at the composition-part of it, what are the 
flags for A.andThen(B)? (then extrapolate to N compositions and the available 
set of flags always approaches 0)

I spent quite a bit of time on this and in the end tracking all this info, and 
making sure that the flags of implementations correspond to the actual 
behavior, just ended up costing performance for most streams and introduced an 
extra dimension to creation and maintenance which I had a hard time justifying.

It can be a slippery slope if we were designing from the ground up but the 
stream implementation already exists and SORTED, DISTINCT and SIZED are the 
flags that are already tracked by the current implementation.

Currently only SHORT_CIRCUIT is set (if not greedy),
see 
https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/util/stream/GathererOp.java#L209

And for A.andThen(B), A.flags & B.flags should work, the stream is sorted if 
both gatherers keep it sorted.


Making specific, rare, combinations of operations faster at the expense of 
making 99% of all others slower is a hard pill for most to swallow.

I suppose that if those flags already exist, it's because they have a purpose 
and i do not understand how it can make the other operations slower.



Cheers,
√

regards,
Rémi



Viktor Klang
Software Architect, Java Platform Group
Oracle

From: fo...@univ-mlv.fr 
Sent: Thursday, 18 January 2024 10:28
To: Viktor Klang 
Cc: core-libs-dev ; Paul Sandoz 

Subject: [External] : Re: Gatherer: spliterator characteristics are not 
propagated




From: "Viktor Klang" 
To: "Remi Forax" , "core-libs-dev" 

Sent: Wednesday, January 17, 2024 8:48:07 PM
Subject: Re: Gatherer: spliterator characteristics are not propagated
Hi Rémi,

You can find some of my benches here: 
https://github.com/openjdk/jdk/tree/master/test/micro/org/openjdk/bench/java/util/stream/ops/ref

Initially I had Characteristics such as ORDERED etc on Gatherer but it just 
didn't end up worth it when looking at the bench results over a wide array of 
stream sizes and number of operations.

I think there are 3 gatherer characteristics that make sense: KEEP_SORTED, 
KEEP_DISTINCT and KEEP_SIZED,
all of them say that if the stream was sorted/distinct/sized then the stream 
returned by a call to gather() is still sorted (with the same comparator), 
distinct or sized.

As examples, map() is KEEP_SIZED, filter() is KEEP_SORTED | KEEP_DISTINCT and 
windowFixed is KEEP_DISTINCT.

[CC Paul, so he can correct me if i'm saying something stupid]

Now for the benchmarks, it depends what you want to measure, benchmarking 
streams is tricky. This is what i know about benchmarking streams.
First, the JIT has two ways to profile types at runtime,
Either a method takes a function as parameter
  void map(Function function) {
function.apply(...)
  }
and when map is called with a subtype of Function, the JIT will propagate the 

Re: Avoiding Side Effects of ForkJoinPool.managedBlock

[Johannes Spangenberg]

> On the REE, this is also controlled by JUnit when it creates the FJP.
> The saturate parameter is the predicate that is determines if REE is
> thrown or the pool continues without an additional thread.

Thanks, I missed that unfortunately. My version of JUnit uses null for
the saturate parameter, but it has been changed in more recent versions.

https://github.com/junit-team/junit5/commit/324802d8d0e189a4bddac5a2f93e4c52d2431f5b

So, I guess (2.) and (3.) can be fixed by using a more recent version of
JUnit... Regarding (1.), I suppose there is no option to disable the
thread-stealing mechanism with parallel streams. I find it a bit
unfortunate that using a ForkJoinPool changes the behavior of various
methods which are otherwise unrelated to the ForkJoinPool. (Maybe
virtual threads can be used to fix that at some point.) Anyway, I have
everything I need right now.

Thank you again,
Johannes



smime.p7s
Description: S/MIME cryptographic signature

Re: [jdk22] RFR: 8321938: java/foreign/critical/TestCriticalUpcall.java does not need a core file

[Daniel D . Daugherty]

On Thu, 18 Jan 2024 15:36:07 GMT, Albert Mingkun Yang  wrote:

>> A trivial fix to disable core file generation in 
>> java/foreign/critical/TestCriticalUpcall.java.
>
> Marked as reviewed by ayang (Reviewer).

@albertnetymk - Thanks for the fast review!

-

PR Comment: https://git.openjdk.org/jdk22/pull/90#issuecomment-1898718533

[jdk22] Integrated: 8321938: java/foreign/critical/TestCriticalUpcall.java does not need a core file

[Daniel D . Daugherty]

On Thu, 18 Jan 2024 15:20:13 GMT, Daniel D. Daugherty  
wrote:

> A trivial fix to disable core file generation in 
> java/foreign/critical/TestCriticalUpcall.java.

This pull request has now been integrated.

Changeset: b1788944
Author:Daniel D. Daugherty 
URL:   
https://git.openjdk.org/jdk22/commit/b17889442385752929f4cbf77f34a79678dc492a
Stats: 3 lines in 1 file changed: 1 ins; 0 del; 2 mod

8321938: java/foreign/critical/TestCriticalUpcall.java does not need a core file

Reviewed-by: ayang
Backport-of: a22ae909bc53344afd9bb6b1f08ff06858c10820

-

PR: https://git.openjdk.org/jdk22/pull/90

Re: [jdk22] RFR: 8321938: java/foreign/critical/TestCriticalUpcall.java does not need a core file

[Albert Mingkun Yang]

On Thu, 18 Jan 2024 15:20:13 GMT, Daniel D. Daugherty  
wrote:

> A trivial fix to disable core file generation in 
> java/foreign/critical/TestCriticalUpcall.java.

Marked as reviewed by ayang (Reviewer).

-

PR Review: https://git.openjdk.org/jdk22/pull/90#pullrequestreview-1829973047

Re: Gatherer API : wildcards complaint

[forax]

> From: "Viktor Klang" 
> To: "Remi Forax" 
> Cc: "core-libs-dev" 
> Sent: Wednesday, January 17, 2024 9:00:32 PM
> Subject: Re: Gatherer API : wildcards complaint

> Ah, now I see what you mean! Thank you \uD83D\uDC4D

> The reason for the signature of `Gatherer.of` was to mirror as much as 
> possible
> of `Collector.of`[1] so I would argue that if we tweak the variance of one 
> then
> we should consider tweaking it for both.

> 1: [
> https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/util/stream/Collector.java#L264
> |
> https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/util/stream/Collector.java#L264
> ]

I agree. 
In terms of code, i suppose that we will need to add some unsafe cast because 
the Gatherer interface should not use wildcard (it will make the code that use 
the Gatherer awkward) but the factory methods should use wildcards. 
Those unsafe casts are safe because function parameter types are contravariant 
and return type covariant but there is no way to express than in the Java type 
system. 

> Cheers,
> √

Rémi 

> Viktor Klang
> Software Architect, Java Platform Group
> Oracle

> From: fo...@univ-mlv.fr 
> Sent: Wednesday, 17 January 2024 20:49
> To: Viktor Klang 
> Cc: core-libs-dev 
> Subject: [External] : Re: Gatherer API : wildcards complaint

>> From: "Viktor Klang" 
>> To: "Remi Forax" , "core-libs-dev"
>> 
>> Sent: Wednesday, January 17, 2024 5:49:01 PM
>> Subject: Re: Gatherer API : wildcards complaint

>> Hi Rémi,

>> Thank you for the feedback—examples would be much appreciated!

> Here is an example with an interface and a class,

> interface Counter {
> void increment ();
> int value ();
> }

> Gatherer < String , Counter , Integer > count () {
> class CounterImpl implements Counter {
> int counter ;

> @Override
> public void increment () {
> counter ++;
> }

> @Override
> public int value () {
> return counter ;
> }
> }
> Supplier < CounterImpl > initializer = CounterImpl :: new ;
> Gatherer . Integrator < Counter , String , Gatherer . Downstream  Integer >> integrator = ( counter , _ , _ ) -> {
> counter .increment();
> return true ;
> };
> BiConsumer < Counter , Gatherer . Downstream > finisher = (
> counter , downstream ) -> {
> downstream .push( counter .value());
> };
> return Gatherer . ofSequential ( initializer , integrator , finisher ); // 
> does
> not compile :(
> }

> void main () {
> System . out .println( Stream . of ( "foo"
> ).gather(count()).findFirst().orElseThrow());
> }

> if instead of explicitly typing each functions, we directly call ofSequential,
> it works

> return Gatherer . ofSequential (
> CounterImpl :: new ,
> ( Counter counter , String _ , Gatherer . Downstream  _ ) 
> -> {
> counter .increment();
> return true ;
> },
> ( Counter counter , Gatherer . Downstream  downstream ) -> {
> downstream .push( counter .value());
> }
> );

> because here, CounterImpl::new is inferred as Supplier.

>> Cheers,
>> √

>> Viktor Klang
>> Software Architect, Java Platform Group
>> Oracle

>> From: core-libs-dev  on behalf of Remi Forax
>> 
>> Sent: Wednesday, 17 January 2024 16:55
>> To: core-libs-dev 
>> Subject: Gatherer API : wildcards complaint
>> Hello,
>> this is a minor complaint but I do not see a raison to not getting this 
>> right.

>> Currently the Gatherer API does not use the wildcard correctly, which is not
>> fully an issue because there is "enough" wildcards that if you rely on the
>> inference, it will work.

>> The problem is that when you write code, you make mistakes and usually when 
>> you
>> have a typing issue, a way to debug it is to fix the type arguments
>> de-activating the inference.
>> But because there are some missing wildcards, this debugging strategy just 
>> fail
>> flat with more typing errors.

>> I think that fixing the missing wildcards will hep users (or a least me) to 
>> have
>> a better experience when using the Gatherer API.

>> I can help and propose a PR if you want.

>> regards,
>> Rémi

[jdk22] RFR: 8321938: java/foreign/critical/TestCriticalUpcall.java does not need a core file

[Daniel D . Daugherty]

A trivial fix to disable core file generation in 
java/foreign/critical/TestCriticalUpcall.java.

-

Commit messages:
 - Backport a22ae909bc53344afd9bb6b1f08ff06858c10820

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

PR: https://git.openjdk.org/jdk22/pull/90

Re: Gatherer: spliterator characteristics are not propagated

[forax]

> From: "Viktor Klang" 
> To: "Remi Forax" 
> Cc: "core-libs-dev" , "Paul Sandoz"
> 
> Sent: Thursday, January 18, 2024 3:36:07 PM
> Subject: Re: Gatherer: spliterator characteristics are not propagated

> I suspect that it is a rather slippery slope, once KEEP-flags are added, then
> others will want to be able to have INJECT-flags, and then people might have
> different opinions w.r.t. the default should be to clear all flags etc.

> And that's even before one looks at the composition-part of it, what are the
> flags for A.andThen(B)? (then extrapolate to N compositions and the available
> set of flags always approaches 0)

> I spent quite a bit of time on this and in the end tracking all this info, and
> making sure that the flags of implementations correspond to the actual
> behavior, just ended up costing performance for most streams and introduced an
> extra dimension to creation and maintenance which I had a hard time 
> justifying.

It can be a slippery slope if we were designing from the ground up but the 
stream implementation already exists and SORTED, DISTINCT and SIZED are the 
flags that are already tracked by the current implementation. 

Currently only SHORT_CIRCUIT is set (if not greedy), 
see [ 
https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/util/stream/GathererOp.java#L209
 | 
https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/util/stream/GathererOp.java#L209
 ] 

And for A.andThen(B), A.flags & B.flags should work, the stream is sorted if 
both gatherers keep it sorted. 

> Making specific, rare, combinations of operations faster at the expense of
> making 99% of all others slower is a hard pill for most to swallow.

I suppose that if those flags already exist, it's because they have a purpose 
and i do not understand how it can make the other operations slower. 

> Cheers,
> √

regards, 
Rémi 

> Viktor Klang
> Software Architect, Java Platform Group
> Oracle

> From: fo...@univ-mlv.fr 
> Sent: Thursday, 18 January 2024 10:28
> To: Viktor Klang 
> Cc: core-libs-dev ; Paul Sandoz
> 
> Subject: [External] : Re: Gatherer: spliterator characteristics are not
> propagated

>> From: "Viktor Klang" 
>> To: "Remi Forax" , "core-libs-dev"
>> 
>> Sent: Wednesday, January 17, 2024 8:48:07 PM
>> Subject: Re: Gatherer: spliterator characteristics are not propagated

>> Hi Rémi,

>> You can find some of my benches here: [
>> https://urldefense.com/v3/__https://github.com/openjdk/jdk/tree/master/test/micro/org/openjdk/bench/java/util/stream/ops/ref__;!!ACWV5N9M2RV99hQ!JJy6F9NoL6wKZQK5158up_fTRvH8X7F6JK8T7Euuf8vzbSQbr23eWa9S_yb61ksONVrLrdesCF_au5zyje2l$
>> |
>> https://github.com/openjdk/jdk/tree/master/test/micro/org/openjdk/bench/java/util/stream/ops/ref
>> ]

>> Initially I had Characteristics such as ORDERED etc on Gatherer but it just
>> didn't end up worth it when looking at the bench results over a wide array of
>> stream sizes and number of operations.

> I think there are 3 gatherer characteristics that make sense: KEEP_SORTED,
> KEEP_DISTINCT and KEEP_SIZED,
> all of them say that if the stream was sorted/distinct/sized then the stream
> returned by a call to gather() is still sorted (with the same comparator),
> distinct or sized.

> As examples, map() is KEEP_SIZED, filter() is KEEP_SORTED | KEEP_DISTINCT and
> windowFixed is KEEP_DISTINCT.

> [CC Paul, so he can correct me if i'm saying something stupid]

> Now for the benchmarks, it depends what you want to measure, benchmarking
> streams is tricky. This is what i know about benchmarking streams.
> First, the JIT has two ways to profile types at runtime,
> Either a method takes a function as parameter
> void map(Function function) {
> function.apply(...)
> }
> and when map is called with a subtype of Function, the JIT will propagate the
> exact type when map is inlined,
> Or a method use a field
> class Op {
> Function function;

> void map() {
> function.apply(...)
> }
> }
> in that case, the VM records the profile of function.apply() and if there are
> more than two different profiles, the VM declare profile poluttion and do not
> try to optimize.

> The Stream implementation tries very hard to use only parameters instead of
> fields, that's why it does not use classical Iterator that are pull iterator 
> (a
> filter iterator requires a field) but a Spliterator which is a push iterator,
> the element is sent as parameter of the consumer.That's also why collect does
> not use the builder pattern (that accumulate values in fields) but a Collector
> that publish the functions to be called as parameter.

> Obvisously, this is more complex than that, a Collector stores the functions 
> in
> fields so it should not work well but the implementation uses a record that
> plays well with escape analysis. Escape analysis see the fields of an instance
> as parameters so the functions of a Collector are correctly propagated (if the
> escape analysis works). And lambdas are using invokedynamic, 

Re: Gatherer: spliterator characteristics are not propagated

[Viktor Klang]

I suspect that it is a rather slippery slope, once KEEP-flags are added, then 
others will want to be able to have INJECT-flags, and then people might have 
different opinions w.r.t. the default should be to clear all flags etc.

And that's even before one looks at the composition-part of it, what are the 
flags for A.andThen(B)? (then extrapolate to N compositions and the available 
set of flags always approaches 0)

I spent quite a bit of time on this and in the end tracking all this info, and 
making sure that the flags of implementations correspond to the actual 
behavior, just ended up costing performance for most streams and introduced an 
extra dimension to creation and maintenance which I had a hard time justifying.

Making specific, rare, combinations of operations faster at the expense of 
making 99% of all others slower is a hard pill for most to swallow.

Cheers,
√


Viktor Klang
Software Architect, Java Platform Group
Oracle

From: fo...@univ-mlv.fr 
Sent: Thursday, 18 January 2024 10:28
To: Viktor Klang 
Cc: core-libs-dev ; Paul Sandoz 

Subject: [External] : Re: Gatherer: spliterator characteristics are not 
propagated




From: "Viktor Klang" 
To: "Remi Forax" , "core-libs-dev" 

Sent: Wednesday, January 17, 2024 8:48:07 PM
Subject: Re: Gatherer: spliterator characteristics are not propagated
Hi Rémi,

You can find some of my benches here: 
https://github.com/openjdk/jdk/tree/master/test/micro/org/openjdk/bench/java/util/stream/ops/ref

Initially I had Characteristics such as ORDERED etc on Gatherer but it just 
didn't end up worth it when looking at the bench results over a wide array of 
stream sizes and number of operations.

I think there are 3 gatherer characteristics that make sense: KEEP_SORTED, 
KEEP_DISTINCT and KEEP_SIZED,
all of them say that if the stream was sorted/distinct/sized then the stream 
returned by a call to gather() is still sorted (with the same comparator), 
distinct or sized.

As examples, map() is KEEP_SIZED, filter() is KEEP_SORTED | KEEP_DISTINCT and 
windowFixed is KEEP_DISTINCT.

[CC Paul, so he can correct me if i'm saying something stupid]

Now for the benchmarks, it depends what you want to measure, benchmarking 
streams is tricky. This is what i know about benchmarking streams.
First, the JIT has two ways to profile types at runtime,
Either a method takes a function as parameter
  void map(Function function) {
function.apply(...)
  }
and when map is called with a subtype of Function, the JIT will propagate the 
exact type when map is inlined,
Or a method use a field
  class Op {
Function function;

void map() {
   function.apply(...)
}
  }
in that case, the VM records the profile of function.apply() and if there are 
more than two different profiles, the VM declare profile poluttion and do not 
try to optimize.

The Stream implementation tries very hard to use only parameters instead of 
fields, that's why it does not use classical Iterator that are pull iterator (a 
filter iterator requires a field) but a Spliterator which is a push iterator, 
the element is sent as parameter of the consumer.That's also why collect does 
not use the builder pattern (that accumulate values in fields) but a Collector 
that publish the functions to be called as parameter.

Obvisously, this is more complex than that, a Collector stores the functions in 
fields so it should not work well but the implementation uses a record that 
plays well with escape analysis. Escape analysis see the fields of an instance 
as parameters so the functions of a Collector are correctly propagated (if the 
escape analysis works). And lambdas are using invokedynamic, and the VM tries 
really hard to inline invokedynamic, so lambdas (that captures value or not) 
are routinely fully inlined with the intermediate operation of a stream.

In your tests, i've not seen comparaisons between an existing method like map() 
or filter() followed by a sorted()/distinct()/count()/toList(), i.e. operations 
where the characteristics KEEP_* have an impact and their equivalent using a 
Gatherer.



Cheers,
√

regards,
Rémi



Viktor Klang
Software Architect, Java Platform Group
Oracle

From: core-libs-dev  on behalf of Remi Forax 

Sent: Wednesday, 17 January 2024 16:48
To: core-libs-dev 
Subject: Gatherer: spliterator characteristics are not propagated

While doing some benchmarking of the Gatherer API, i've found that the 
characteristics of the spliterator was not propagated by the method 
Stream.gather() making the stream slower than it should.

As an example, there is no way when reimplementing map() using a Gatherer to 
say that this intermediate operation keep the 

Withdrawn: 8315585: Optimization for decimal to string

[duke]

On Mon, 2 Oct 2023 05:40:03 GMT, Shaojin Wen  wrote:

> I submitted PR #1 before, and there were too many changes. I split it 
> into multiple PRs with small changes. This one is one of them.
> 
> this PR removed the duplicate code for getChars in 
> BigDecimal#StringBuilderHelper, i also make performance faster.
> Please review and don't hesitate to critique my approach and patch.

This pull request has been closed without being integrated.

-

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

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

[Severin Gehwolf]

On Tue, 19 Dec 2023 19:14:42 GMT, Mandy Chung  wrote:

>> Severin Gehwolf has updated the pull request incrementally with two 
>> additional commits since the last revision:
>> 
>>  - Disallow packaged modules and run-time image link
>>  - Only check for existing path when not a scratch task
>>
>>When using a run-time image link and the initial build was produced with
>>the --keep-packaged-modules option, we don't need to check existing
>>paths to the location where packaged modules need to be copied. This
>>breaks the --verbose output validation.
>
> FWIW.   
> [f623930](https://github.com/mlchung/jdk/commit/f623930cd529085ddb730a60b7facf106ea01955)
>  for your reference.   I pulled your branch and refactored and made 
> suggestions to the code while I was walking through the code.   Some 
> observations:
> 
> The constants such as the pathname of the timestamp file and the internal 
> file listing
> per-module non-class non-resource files are part of jlink.  I move the 
> constants to
> `JlinkTask`to follow where `OPTIONS_RESOURCE` is defined.
> 
> `JRTArchive` scans the class and resource files of a given module from the 
> runtime image.
> It should also read `fs_$MODULE_files` to find the list of non-class and 
> non-resource files.
> 
> The current implementation checks if a file is modified lazily when 
> `Entry::stream`
> is called and so it has to remember if file modification has been checked and
> warning has been emitted.   I think doing the file modification check eagerly
> when `collectFiles` is called would simplify the code.
> 
> For maintainability, better to move the reading of and writing to 
> `fs_$MODULE_files`
> together in a single class rather than separated in `JRTArchive` and 
> `JlinkResourcesListPlugin`.
> I move them to `JRTArchive.ResourceFileEntry` for now.   There may be a 
> better place.

@mlchung Thanks for your review and code-cleanup!

> `--unlock-run-image` is changed to be an internal option. It is ok while we 
> should revisit this per the customer feedback.
> If not needed, we should take this out in another release.

OK.

> The help output for this option in `jlink.properties` is no longer needed and 
> should be removed.

OK, removed locally.

> Maybe this option should be `--ignore-modified-runtime` or something explicit.

Sure. I've renamed it to `--ignore-modified-runtime`.

> If I read the code correctly, the image created with this option will enable 
> multi-hops unconditionally? i.e. no timestamp file created and disable the 
> check completely. I think the .stamp file should be present in any image 
> created without packaged modules.

The option is currently used in tests (and can also be used to verify binary 
equivalence of jlinking Java SE with and without packaged modules), which is a 
nice property to have. If the stamp file is present in one, but not the other 
this is sufficient to fail the equivalence test.

> I agree that there is no difference to the plugins of SORTER, PROCESSOR, 
> COMPRESSOR categories if the input to linking is packaged modules vs runtime 
> image.
> 
> I also agree that the transformation done by filtering plugins will persist 
> in the new image if linking from the runtime image.

Good.

> Currently, filtering plugins can be of FILTER and TRANSFORMER category.

Which filtering plugins do you see with category TRANSFORMER? 
`strip-java-debug-attributes` and `strip-native-debug-symbols` perhaps? Happy 
to move them to the FILTER category.

> For the remaining plugins (ADDER and TRANSFORMER category) such as 
> `--add-options`, `--system-modules` etc, it sounds right not to persist the 
> transformed data. But a few exceptions:
> 
> ```
>   --vendor-bug-url
>   --vendor-version
>   --vendor-vm-bug-url
> ```
> 
> These plugins change the value of `java.vendor.*` system properties defined 
> in `VersionProps.class`. This behavioral difference when linking with 
> packaged modules and runtime image is hard to notice as those would need the 
> internal knowledge of these plugins (note consider the future plugins too).
>
> I also agree with Alan that --verbose output is a bit confusing (my initial 
> feedback). The image has already been created. The users can't tell which 
> plugins are implicitly run. To list the plugin options, it has to delete the 
> already-created image and re-do the jlink command with `--verbose`.

Are you suggesting to remove it?

> In addition, this change would consider that the default module path now 
> becomes: :$JAVA_HOME/jmods:

I cannot parse this. Do you mean the default module path becoming `$JAVA_HOME` 
or `$JAVA_HOME/jmods`?

> I wonder if the warning about "$JAVA_HOME/jmods" not present is strictly 
> needed.

I don't think it's needed. We could just mention `Performing a 
runtime-image-based jlink.` or something like that.

> I am inclined to consider that non-filtering plugins are responsible to 
> restore the data if transformed to the original form. It seems to be 

Integrated: 8321938: java/foreign/critical/TestCriticalUpcall.java does not need a core file

[Daniel D . Daugherty]

On Thu, 18 Jan 2024 00:58:49 GMT, Daniel D. Daugherty  
wrote:

> A trivial fix to disable core file generation in 
> java/foreign/critical/TestCriticalUpcall.java.

This pull request has now been integrated.

Changeset: a22ae909
Author:Daniel D. Daugherty 
URL:   
https://git.openjdk.org/jdk/commit/a22ae909bc53344afd9bb6b1f08ff06858c10820
Stats: 3 lines in 1 file changed: 1 ins; 0 del; 2 mod

8321938: java/foreign/critical/TestCriticalUpcall.java does not need a core file

Reviewed-by: dholmes

-

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

Re: RFR: 8321938: java/foreign/critical/TestCriticalUpcall.java does not need a core file [v2]

[Daniel D . Daugherty]

On Thu, 18 Jan 2024 07:47:54 GMT, David Holmes  wrote:

>> Daniel D. Daugherty has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Update copyright year.
>
> Good and trivial.
> 
> Copyright year needs updating.
> 
> Thanks

@dholmes-ora - Thanks for the review. Copyright year updated.

-

PR Comment: https://git.openjdk.org/jdk/pull/17476#issuecomment-1898458868

Re: RFR: 8321938: java/foreign/critical/TestCriticalUpcall.java does not need a core file [v2]

[Daniel D . Daugherty]

> A trivial fix to disable core file generation in 
> java/foreign/critical/TestCriticalUpcall.java.

Daniel D. Daugherty has updated the pull request incrementally with one 
additional commit since the last revision:

  Update copyright year.

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/17476/files
  - new: https://git.openjdk.org/jdk/pull/17476/files/44ddcbb8..1982e5bd

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

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

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

Re: Avoiding Side Effects of ForkJoinPool.managedBlock

[Alan Bateman]

On 18/01/2024 09:30, Johannes Spangenberg wrote:

Hello,

I have a question about dealing with side effects caused by ForkJoinPool. I am 
not certain if this mailing list is the appropriate platform, but I hope it is. 
The issue I am facing is that running code within a ForkJoinPool may change the 
behavior of the code. These changes in behavior have resulted in 
non-deterministic test failures in one of the repositories I work on. JUnit 
uses a ForkJoinPool when you enable parallel test execution. I would like to 
disable or avoid the side effects of various methods if called from a 
ForkJoinPool. So far, I haven't found a solution, except for completely 
replacing the use of the ForkJoinPool in JUnit with some custom scheduler which 
is not a ForkJoinPool.

Is there a solution to force an "unmanaged" block (as opposed to 
ForkJoinPool.managedBlock)? Is there alternatively a good approach to transfer CPU bound 
subtasks to another thread pool while blocking the ForkJoinWorkerThread without 
compensation? I have implemented a workaround which I explain below, but I am not sure if 
this will remain effective in future JDK versions. I am currently using JDK 17 but were 
also able to reproduce the issues with JDK 21.

I have observed the following side-effects caused by managed blocks or similar 
mechanisms:

1. Parallel stream methods execute another task (i.e. JUnit test) from the pool 
recursively, which is particularly problematic if your code utilizes any 
ThreadLocal.

2. Blocking methods spawn around 256 threads in total to "compensate" for the 
blocking operation. Consequently, you end up with up to 256 tests running concurrently, 
each of them might or might not be CPU bound (unknown to the ForkJoinPool).

3. Blocking methods may throw a RejectedExecutionException when the thread 
limit is reached. This is effectively a race condition which may lead to 
exceptions.

I have not been able to determine under which circumstances each behavior 
occurs. I am unaware of any thorough documentation that clearly outlines the 
expected behavior in different scenarios with different blocking methods. While 
(1.) and (3.) have caused test failures, (2.) simply causes JUnit to run 256 
tests in parallel instead of the intended 12. I attached a JUnit test to 
reproduce (1.) and (3.), but it might not fail on every run.

Many of the blocking methods of the standard library include a check if the current 
thread is an instance of ForkJoinWorkerThread. My current workaround involves wrapping 
the code that makes blocking calls into a FutureTask which is executed on another thread 
and then joining this task afterwards. As of now, FutureTask.get() seems not to implement 
any of the side-effects. As the missing instanceof-check in FutureTask makes it 
inconsistent with other Futures like CompletableFuture, I fear it might be considered a 
"bug". I would like to know a safe solution which is specified to continue to 
work in future JDKs.

I think it would be useful to understand how JUnit creates the 
ForkJoinPool. The reason is that it controls the parallelism and "max 
pool size". If there is interference due to managedBlocker then JUnit 
can set maxPoolSize to the same value as parallelism.


On the REE, this is also controlled by JUnit when it creates the FJP. 
The saturate parameter is the predicate that is determines if REE is 
thrown or the pool continues without an additional thread.


-Alan

Re: RFR: 8324053: Use the blessed modifier order for sealed in java.base

[Pavel Rappo]

On Thu, 18 Jan 2024 09:29:11 GMT, Magnus Ihse Bursie  wrote:

> Thanks! When this has been integrated, I can take a shot at the missorted 
> `default`s.

Thanks, I could've done that myself, but decided not to. You see, `default` 
should ideally be a [sole] modifier on a method: all other modifiers of a 
`default` method should be deleted. I'm not sure that reaching such an ideal 
would be welcomed.

Whatever you decide to do, be prepared to work manually. 
`bin/blessed-modifier-order.sh` is simple and robust but not specific. You'll 
have to carefully sift through other missortings it finds.

[sole]: 
https://mail.openjdk.org/pipermail/core-libs-dev/2024-January/117398.html

-

PR Comment: https://git.openjdk.org/jdk/pull/17468#issuecomment-1898162068

Re: RFR: 8324053: Use the blessed modifier order for sealed in java.base

[Daniel Fuchs]

On Wed, 17 Jan 2024 21:22:07 GMT, Pavel Rappo  wrote:

> Please review this trivial PR to reorder the `sealed` class or interface 
> modifier. For context of this change see: 
> https://github.com/openjdk/jdk/pull/17242#issuecomment-1887338396.

LGTM

-

Marked as reviewed by dfuchs (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17468#pullrequestreview-1829273407

Avoiding Side Effects of ForkJoinPool.managedBlock

[Johannes Spangenberg]

Hello,

I have a question about dealing with side effects caused by ForkJoinPool. I am 
not certain if this mailing list is the appropriate platform, but I hope it is. 
The issue I am facing is that running code within a ForkJoinPool may change the 
behavior of the code. These changes in behavior have resulted in 
non-deterministic test failures in one of the repositories I work on. JUnit 
uses a ForkJoinPool when you enable parallel test execution. I would like to 
disable or avoid the side effects of various methods if called from a 
ForkJoinPool. So far, I haven't found a solution, except for completely 
replacing the use of the ForkJoinPool in JUnit with some custom scheduler which 
is not a ForkJoinPool.

Is there a solution to force an "unmanaged" block (as opposed to 
ForkJoinPool.managedBlock)? Is there alternatively a good approach to transfer 
CPU bound subtasks to another thread pool while blocking the 
ForkJoinWorkerThread without compensation? I have implemented a workaround 
which I explain below, but I am not sure if this will remain effective in 
future JDK versions. I am currently using JDK 17 but were also able to 
reproduce the issues with JDK 21.

I have observed the following side-effects caused by managed blocks or similar 
mechanisms:

1. Parallel stream methods execute another task (i.e. JUnit test) from the pool 
recursively, which is particularly problematic if your code utilizes any 
ThreadLocal.

2. Blocking methods spawn around 256 threads in total to "compensate" for the 
blocking operation. Consequently, you end up with up to 256 tests running 
concurrently, each of them might or might not be CPU bound (unknown to the 
ForkJoinPool).

3. Blocking methods may throw a RejectedExecutionException when the thread 
limit is reached. This is effectively a race condition which may lead to 
exceptions.

I have not been able to determine under which circumstances each behavior 
occurs. I am unaware of any thorough documentation that clearly outlines the 
expected behavior in different scenarios with different blocking methods. While 
(1.) and (3.) have caused test failures, (2.) simply causes JUnit to run 256 
tests in parallel instead of the intended 12. I attached a JUnit test to 
reproduce (1.) and (3.), but it might not fail on every run.

Many of the blocking methods of the standard library include a check if the 
current thread is an instance of ForkJoinWorkerThread. My current workaround 
involves wrapping the code that makes blocking calls into a FutureTask which is 
executed on another thread and then joining this task afterwards. As of now, 
FutureTask.get() seems not to implement any of the side-effects. As the missing 
instanceof-check in FutureTask makes it inconsistent with other Futures like 
CompletableFuture, I fear it might be considered a "bug". I would like to know 
a safe solution which is specified to continue to work in future JDKs.

PS: There is also a ticket on the JUnit project about this topic, but it only 
talks about side-effect (2.), but not the other side-effects we observed.
https://github.com/junit-team/junit5/issues/3108

Thanks,
Johannes



ForkJoinPoolTest.java
Description: ForkJoinPoolTest.java


smime.p7s
Description: S/MIME cryptographic signature

Re: RFR: 8324053: Use the blessed modifier order for sealed in java.base

[Magnus Ihse Bursie]

On Wed, 17 Jan 2024 21:22:07 GMT, Pavel Rappo  wrote:

> Please review this trivial PR to reorder the `sealed` class or interface 
> modifier. For context of this change see: 
> https://github.com/openjdk/jdk/pull/17242#issuecomment-1887338396.

Thanks! When this has been integrated, I can take a shot at the missorted 
`default`s.

-

Marked as reviewed by ihse (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17468#pullrequestreview-1829238473

Re: Gatherer: spliterator characteristics are not propagated

[forax]

> From: "Viktor Klang" 
> To: "Remi Forax" , "core-libs-dev"
> 
> Sent: Wednesday, January 17, 2024 8:48:07 PM
> Subject: Re: Gatherer: spliterator characteristics are not propagated

> Hi Rémi,

> You can find some of my benches here: [
> https://github.com/openjdk/jdk/tree/master/test/micro/org/openjdk/bench/java/util/stream/ops/ref
> |
> https://github.com/openjdk/jdk/tree/master/test/micro/org/openjdk/bench/java/util/stream/ops/ref
> ]

> Initially I had Characteristics such as ORDERED etc on Gatherer but it just
> didn't end up worth it when looking at the bench results over a wide array of
> stream sizes and number of operations.

I think there are 3 gatherer characteristics that make sense: KEEP_SORTED, 
KEEP_DISTINCT and KEEP_SIZED, 
all of them say that if the stream was sorted/distinct/sized then the stream 
returned by a call to gather() is still sorted (with the same comparator), 
distinct or sized. 

As examples, map() is KEEP_SIZED, filter() is KEEP_SORTED | KEEP_DISTINCT and 
windowFixed is KEEP_DISTINCT. 

[CC Paul, so he can correct me if i'm saying something stupid] 

Now for the benchmarks, it depends what you want to measure, benchmarking 
streams is tricky. This is what i know about benchmarking streams. 
First, the JIT has two ways to profile types at runtime, 
Either a method takes a function as parameter 
void map(Function function) { 
function.apply(...) 
} 
and when map is called with a subtype of Function, the JIT will propagate the 
exact type when map is inlined, 
Or a method use a field 
class Op { 
Function function; 

void map() { 
function.apply(...) 
} 
} 
in that case, the VM records the profile of function.apply() and if there are 
more than two different profiles, the VM declare profile poluttion and do not 
try to optimize. 

The Stream implementation tries very hard to use only parameters instead of 
fields, that's why it does not use classical Iterator that are pull iterator (a 
filter iterator requires a field) but a Spliterator which is a push iterator, 
the element is sent as parameter of the consumer.That's also why collect does 
not use the builder pattern (that accumulate values in fields) but a Collector 
that publish the functions to be called as parameter. 

Obvisously, this is more complex than that, a Collector stores the functions in 
fields so it should not work well but the implementation uses a record that 
plays well with escape analysis. Escape analysis see the fields of an instance 
as parameters so the functions of a Collector are correctly propagated (if the 
escape analysis works). And lambdas are using invokedynamic, and the VM tries 
really hard to inline invokedynamic, so lambdas (that captures value or not) 
are routinely fully inlined with the intermediate operation of a stream. 

In your tests, i've not seen comparaisons between an existing method like map() 
or filter() followed by a sorted()/distinct()/count()/toList(), i.e. operations 
where the characteristics KEEP_* have an impact and their equivalent using a 
Gatherer. 

> Cheers,
> √

regards, 
Rémi 

> Viktor Klang
> Software Architect, Java Platform Group
> Oracle

> From: core-libs-dev  on behalf of Remi Forax
> 
> Sent: Wednesday, 17 January 2024 16:48
> To: core-libs-dev 
> Subject: Gatherer: spliterator characteristics are not propagated
> While doing some benchmarking of the Gatherer API, i've found that the
> characteristics of the spliterator was not propagated by the method
> Stream.gather() making the stream slower than it should.

> As an example, there is no way when reimplementing map() using a Gatherer to 
> say
> that this intermediate operation keep the size, which is important if the
> terminal operation is toList() because if the size is known, toList() will
> presize the List and avoid the creation of an intermediary ArrayList.

> See [
> https://github.com/forax/we_are_all_to_gather/blob/master/src/main/java/com/gihtub/forax/wearealltogather/bench/MapGathererBenchmark.java
> |
> https://github.com/forax/we_are_all_to_gather/blob/master/src/main/java/com/gihtub/forax/wearealltogather/bench/MapGathererBenchmark.java
> ]

> I think that adding a way to propagate the spliterator characteristics 
> through a
> Gatherer would greatly improve the performance of commons streams (at least 
> all
> the ones that end with a call to toList).

> I have some idea of how to do that, but I prefer first to hear if i've 
> overlook
> something and if improving the performance is something worth changing the 
> API.

> regards,
> Rémi

Re: RFR: 8323782: Race: Thread::interrupt vs. AbstractInterruptibleChannel.begin [v2]

[Richard Reingruber]

On Thu, 18 Jan 2024 08:32:02 GMT, Alan Bateman  wrote:

>> src/java.base/share/classes/java/lang/Thread.java line 1709:
>> 
>>> 1707: // Setting the interrupt status must be done before reading 
>>> nioBlocker.
>>> 1708: interrupted = true;
>>> 1709: interrupt0();  // inform VM of interrupt
>> 
>> It is really safe/correct to move this outside the synchronized block? I 
>> know things have changed a bit with loom but we've "always" held a lock when 
>> doing the actual interrupt. I'd have to check the VM logic to be sure it can 
>> be called concurrently from multiple threads for the same target thread.
>
>> It is really safe/correct to move this outside the synchronized block? I 
>> know things have changed a bit with loom but we've "always" held a lock when 
>> doing the actual interrupt. I'd have to check the VM logic to be sure it can 
>> be called concurrently from multiple threads for the same target thread.
> 
> This hasn't changed. The interruptLock is used to coordinate the add/remove 
> of the nioBlocker. When there is no nioBlocker set then the interrupt status 
> and unparking (as in JavaThread::interrupt) has always executesd without the 
> interruptLock.

I think that interrupting is just asynchronous to some extent.
E.g. a thread polls its interrupt status clearing it thereby (without lock) 
before calling nio. A concurrent interrupt can be lost then even if the lock is 
acquired.
(Maybe clearing should not be done by a public method)

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17444#discussion_r1457152809

Re: Seing a Collector as a Gatherer

[Viktor Klang]

Yes, having the conversion in Gatherers would seem like the most sensible 
option.

Then the question becomes whether gather is the most sensible name for it―I'm 
thinking of readability under the use of static imports etc. gatherUsing(), 
viaCollector(), adapt(), etc.

Cheers,
√


Viktor Klang
Software Architect, Java Platform Group
Oracle

From: fo...@univ-mlv.fr 
Sent: Wednesday, 17 January 2024 21:16
To: Viktor Klang 
Cc: core-libs-dev 
Subject: [External] : Re: Seing a Collector as a Gatherer




From: "Viktor Klang" 
To: "Remi Forax" 
Sent: Wednesday, January 17, 2024 6:01:38 PM
Subject: Re: Seing a Collector as a Gatherer
Hi Rémi,

Yes, this was something I was hoping to get into the preview, but I wasn't sure 
where that conversion should end up.

There are a few different places where it might go:

Gatherer.of(Collector)
Gatherers.collect(Collector)
Collector.asGatherer()
Collectors.gather(Collector)

I wasn't really convinced where it should go, and I was hesitant to making any 
changes to existing public interfaces as a "nice to have", so I opted to leave 
it out for now.

I think people would prefer to see it on Collector as a default method, but as 
I said before, making changes to Collector wasn't something I was ready to 
prioritize for the (first) JEP.

I think this method is also important pedagogically, there should be a place 
that describe the relationship between a Collector and a Gatherer.

For Gatherer.of(), this one seems alien compared to the other overloads of 
of()/ofSequential() and to a lesser extend I do not like too much to have 
overloads with one parameter with two different interfaces, because someone can 
comes with a class that implements both Collector and Integrator (as stupid as 
it is),

For Gatherers.collect(Collector) is fine,

For Collector.asGatherer(), a default method has the disadvantage that usually 
a Collector is typed from left to right so calling an instance method requires 
an intermediary variable
Collector> collector = Collector.toList();  // ok
Gatherer> gatherer = 
Collector.toList().asGatherer();  // we are in trouble here
 that's why Collectors.collectingAndThen() (aka compose the finisher) is a 
static method in Collectors and not an instance method in Collector 
(finishAndThen ?),

For Collectors.gather(), methods inside Collectors tend to return a Collector.




Cheers,
√

regards,
Rémi



Viktor Klang
Software Architect, Java Platform Group
Oracle

From: core-libs-dev  on behalf of Remi Forax 

Sent: Wednesday, 17 January 2024 17:08
To: core-libs-dev 
Subject: Seing a Collector as a Gatherer

Hello,
I may have overlook that, but it seems there is no method to see a Collector as 
a Gatherer.

A Gatherer is more general than a Collector and a Gatherer with a greedy 
integrator that does not call Downstream.push in the intergator and only once 
is the finisher is basicaly a Collector.

In code:
 Gatherer asGatherer(Collector 
collector) {
  var supplier = collector.supplier();
  var accumulator = collector.accumulator();
  var combiner = collector.combiner();
  var finisher = collector.finisher();
  return Gatherer.of(supplier,
  Gatherer.Integrator.ofGreedy((state, element, _) -> {
accumulator.accept(state, element);
return true;
  }),
  combiner,
  (state, downstream) -> downstream.push(finisher.apply(state)));
}

This is eaxctly how Gatherer.fold() works.

Is there a reason why such method does not exist ?

regards,
Rémi

Re: RFR: 8323782: Race: Thread::interrupt vs. AbstractInterruptibleChannel.begin [v2]

[Alan Bateman]

On Thu, 18 Jan 2024 08:02:27 GMT, David Holmes  wrote:

> It is really safe/correct to move this outside the synchronized block? I know 
> things have changed a bit with loom but we've "always" held a lock when doing 
> the actual interrupt. I'd have to check the VM logic to be sure it can be 
> called concurrently from multiple threads for the same target thread.

This hasn't changed. The interruptLock is used to coordinate the add/remove of 
the nioBlocker. When there is no nioBlocker set then the interrupt status and 
unparking (as in JavaThread::interrupt) executes without the interruptLock.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17444#discussion_r1457092058

Re: RFR: 8323782: Race: Thread::interrupt vs. AbstractInterruptibleChannel.begin [v2]

[Richard Reingruber]

On Wed, 17 Jan 2024 21:18:27 GMT, Christoph Langer  wrote:

>> Richard Reingruber has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Review Alan
>
> test/jdk/java/nio/channels/Selector/LotsOfInterrupts.java line 2:
> 
>> 1: /*
>> 2:  * Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved.
> 
> I guess you should credit SAP here.

Alan wrote the test. I added him as contributor.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17444#discussion_r1457074572

Re: RFR: 8323782: Race: Thread::interrupt vs. AbstractInterruptibleChannel.begin [v2]

[David Holmes]

On Wed, 17 Jan 2024 15:38:22 GMT, Richard Reingruber  wrote:

>> Set `interrupted` in `Thread::interrupt` before reading `nioBlocker` for 
>> correct (Dekker scheme) synchronization with concurrent execution of 
>> [`AbstractInterruptibleChannel::begin`](https://github.com/openjdk/jdk/blob/59062402b9c5ed5612a13c1c40eb22cf1b97c41a/src/java.base/share/classes/java/nio/channels/spi/AbstractInterruptibleChannel.java#L176).
>> 
>> The change passed our CI functional testing: JTReg tests: tier1-4 of hotspot 
>> and jdk. All of Langtools and jaxp. SPECjvm2008, SPECjbb2015, Renaissance 
>> Suite, and SAP specific tests.
>> Testing was done with fastdebug and release builds on the main platforms and 
>> also on Linux/PPC64le and AIX.
>
> Richard Reingruber has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Review Alan

src/java.base/share/classes/java/lang/Thread.java line 1709:

> 1707: // Setting the interrupt status must be done before reading 
> nioBlocker.
> 1708: interrupted = true;
> 1709: interrupt0();  // inform VM of interrupt

It is really safe/correct to move this outside the synchronized block? I know 
things have changed a bit with loom but we've "always" held a lock when doing 
the actual interrupt. I'd have to check the VM logic to be sure it can be 
called concurrently from multiple threads for the same target thread.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17444#discussion_r1457061745