Re: RFR: 8288414: Long::compress/expand samples are not correct

2022-06-14 Thread Alan Bateman
On Tue, 14 Jun 2022 16:28:37 GMT, Paul Sandoz  wrote:

> Update the code examples in the api notes of Long::compress/expand. Some 
> constants need to be explicitly long values.

Marked as reviewed by alanb (Reviewer).

-

PR: https://git.openjdk.org/jdk19/pull/14


Re: RFR: JDK-8288207: Enhance MalformedURLException in Uri.parseCompat [v4]

2022-06-14 Thread Alan Bateman
On Tue, 14 Jun 2022 12:18:52 GMT, Matthias Baesken  wrote:

>> When trying to construct an LdapURL object with a bad input string (in this 
>> example the _ in ad_jbs is causing issues), and not using
>> the backward compatibility flag -Dcom.sun.jndi.ldapURLParsing="legacy" we 
>> run into the exception below :
>> 
>> import com.sun.jndi.ldap.LdapURL;
>>  
>> String url = "ldap://ad_jbs.ttt.net:389/xyz;; // bad input string containing 
>> _
>> LdapURL ldapUrl = new LdapURL(url);
>> 
>> 
>> java --add-opens java.naming/com.sun.jndi.ldap=ALL-UNNAMED LdapParseUrlTest
>> Exception in thread "main" javax.naming.NamingException: Cannot parse url: 
>> ldap://ad_jbs.ttt.net:389/xyz [Root exception is 
>> java.net.MalformedURLException: unsupported authority: ad_jbs.ttt.net:389]
>> at java.naming/com.sun.jndi.ldap.LdapURL.(LdapURL.java:115)
>> at LdapParseUrlTest.main(LdapParseUrlTest.java:9)
>> Caused by: java.net.MalformedURLException: unsupported authority: 
>> ad_jbs.ttt.net:389
>> at java.naming/com.sun.jndi.toolkit.url.Uri.parseCompat(Uri.java:367)
>> at java.naming/com.sun.jndi.toolkit.url.Uri.parse(Uri.java:230)
>> at java.naming/com.sun.jndi.toolkit.url.Uri.init(Uri.java:174)
>> at java.naming/com.sun.jndi.ldap.LdapURL.(LdapURL.java:105)
>> 
>> I would like to add the host and port info to the exception (in the example 
>> it is host:port of URI:null:-1] ) so that it is directly visible that the 
>> input caused the construction of a URI
>> with "special"/problematic host and port values.
>
> Matthias Baesken has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   avoid very long line

Marked as reviewed by alanb (Reviewer).

-

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


Re: Handling of USR2 signal in JVM on Linux

2022-06-14 Thread Alan Bateman

On 14/06/2022 10:44, Andrey Turbanov wrote:

Hello.
During investigation of signal handling in JVM (for
https://github.com/openjdk/jdk/pull/9100#discussion_r894992558 )
I found out that sending USR2 crashes my JDK. (Linux fastdebug x64)

kill -USR2 1346792

# assert(thread != __null) failed: Missing current thread in SR_handler
# Internal Error
(/home/turbanoff/Projects/official_jdk/src/hotspot/os/posix/signals_posix.cpp:1600),
pid=1346792, tid=1346792

Full hs_err_pid1346792.log:
https://gist.github.com/turbanoff/2099327ea13357a90df43a2d6b0e2e6a


Is it known/expected behaviour?
I found some description there
https://docs.oracle.com/en/java/javase/11/troubleshoot/handle-signals-and-exceptions.html
that USR2 is used for SUSPEND/RESUME. Is it supported by Hotspot?


In general you have to be very careful when using signals. Yes, it can 
easily break things and probably notice it quickly with debug builds as 
asserts are compiled in to the builds (like the above). So I think 
you've found the right page to read up on this. In this case, you can 
set _JAVA_SR_SIGNUM to specify a different signal for S/R.


-Alan


Re: RFR: JDK-8288207: Enhance MalformedURLException in Uri.parseCompat [v3]

2022-06-14 Thread Alan Bateman
On Tue, 14 Jun 2022 11:36:36 GMT, Matthias Baesken  wrote:

>> When trying to construct an LdapURL object with a bad input string (in this 
>> example the _ in ad_jbs is causing issues), and not using
>> the backward compatibility flag -Dcom.sun.jndi.ldapURLParsing="legacy" we 
>> run into the exception below :
>> 
>> import com.sun.jndi.ldap.LdapURL;
>>  
>> String url = "ldap://ad_jbs.ttt.net:389/xyz;; // bad input string containing 
>> _
>> LdapURL ldapUrl = new LdapURL(url);
>> 
>> 
>> java --add-opens java.naming/com.sun.jndi.ldap=ALL-UNNAMED LdapParseUrlTest
>> Exception in thread "main" javax.naming.NamingException: Cannot parse url: 
>> ldap://ad_jbs.ttt.net:389/xyz [Root exception is 
>> java.net.MalformedURLException: unsupported authority: ad_jbs.ttt.net:389]
>> at java.naming/com.sun.jndi.ldap.LdapURL.(LdapURL.java:115)
>> at LdapParseUrlTest.main(LdapParseUrlTest.java:9)
>> Caused by: java.net.MalformedURLException: unsupported authority: 
>> ad_jbs.ttt.net:389
>> at java.naming/com.sun.jndi.toolkit.url.Uri.parseCompat(Uri.java:367)
>> at java.naming/com.sun.jndi.toolkit.url.Uri.parse(Uri.java:230)
>> at java.naming/com.sun.jndi.toolkit.url.Uri.init(Uri.java:174)
>> at java.naming/com.sun.jndi.ldap.LdapURL.(LdapURL.java:105)
>> 
>> I would like to add the host and port info to the exception (in the example 
>> it is host:port of URI:null:-1] ) so that it is directly visible that the 
>> input caused the construction of a URI
>> with "special"/problematic host and port values.
>
> Matthias Baesken has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   fix copy paste error

src/java.naming/share/classes/com/sun/jndi/toolkit/url/Uri.java line 368:

> 366: // throw if we have user info or regname
> 367: throw new MalformedURLException("Authority 
> component is not server-based, or contains user info. Unsupported authority: 
> " + auth);
> 368: }

This looks okay but you may have to split up the line to avoid adding a 150+ 
char line (most of the file seems to keep the lines under 100 or so).

-

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


RFR: 8286176: Add JNI_VERSION_19 to jni.h and JNI spec

2022-06-13 Thread Alan Bateman
JNI is updated in Java 19 so we need to define JNI_VERSION_19 and change 
GetVersion to return this version.

test/hotspot/jtreg/native_sanity/JniVersion.java is updated to check that 
JNI_VERSION_19 is returned. The native library in the JMX agent, and several 
tests, define JNI_OnLoad that return JNI_VERSION_10 as the JNI version needed. 
These are updated to return JNI_VERSION_19 although these updates aren't 
strictly needed.

-

Commit messages:
 - Initial commit

Changes: https://git.openjdk.org/jdk19/pull/7/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk19=7=00
  Issue: https://bugs.openjdk.org/browse/JDK-8286176
  Stats: 16 lines in 10 files changed: 2 ins; 0 del; 14 mod
  Patch: https://git.openjdk.org/jdk19/pull/7.diff
  Fetch: git fetch https://git.openjdk.org/jdk19 pull/7/head:pull/7

PR: https://git.openjdk.org/jdk19/pull/7


Re: RFR: 8279047: Remove expired flags in JDK 20

2022-06-10 Thread Alan Bateman
On Fri, 10 Jun 2022 13:23:51 GMT, David Holmes  wrote:

> Expired Flags in 20:
> 
> - FilterSpuriousWakeups
> - MinInliningThreshold
> - PrefetchFieldsAhead
> 
> No remaining usages in code or tests.
> 
> - UseHeavyMonitors  (expired in PRODUCT ONLY)
> 
> As this flag now only exists for debug builds it has to be a "develop" flag 
> rather than product. There are then changes to two tests that use this flag, 
> so that they only run on a debug VM.
> 
> - test/jdk/java/lang/Thread/virtual/HoldsLock.java
> 
> The second @run that uses UseHeavyMonitors is moved to a second test segment 
> that only applies to the debug VM.
> 
> - test/jdk/java/util/concurrent/ConcurrentHashMap/MapLoops.java
> 
> Change the test section that uses UseHeavyMonitors to only run on a debug VM 
> and remove the IgnoreUnrecognizedOptions flag. Note that the existing test 
> logic would run the same test twice on product builds.
> 
> No documentation in the manpage exists for any of the newly expired flags.
> 
> Obsoleted flags in 20:
> 
> - ExtendedDTraceProbes
> 
> Documented in manpage so moved from Deprecated section to Obsolete section. 
> There is special handling for messages about use of this flag so that won't 
> be updated until the flag is actually obsoleted (JDK-8279913).
> 
> - UseContainerCpuShares
> - PreferContainerQuotaForCPUCount
> - AliasLevel
> 
> Undocumented.
> 
> Java manpage updates:
> - Updates Java version to 20
> - Moved ExtendedDTraceProbe info as previously mentioned
> - Applied changes from the .1 file that had not been applied to the markdown 
> source so that they were not lost (and note the .1 file was also missing 
> changes from the markdown file that had not been propagated).
> - Removed an unrepresentable character (the section symbol) that was not 
> being generated into either the html or nroff file correctly

Marked as reviewed by alanb (Reviewer).

test/jdk/java/lang/Thread/virtual/HoldsLock.java line 39:

> 37:  * @modules java.base/java.lang:+open
> 38:  * @compile --enable-preview -source ${jdk.version} HoldsLock.java
> 39:  * @run testng/othervm --enable-preview -XX:+UseHeavyMonitors HoldsLock

The test updates look okay, probably don't need to duplicate the summary tag 
when it's unchanged but what you have is okay.

-

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


Re: RFR: 8287760: --do-not-resolve-by-default gets overwritten if --warn-if-resolved flags is used [v8]

2022-06-10 Thread Alan Bateman
On Fri, 10 Jun 2022 18:19:42 GMT, Thiago Henrique Hüpner 
 wrote:

>> test/jdk/tools/jar/modularJar/Basic.java line 44:
>> 
>>> 42: 
>>> 43: import jdk.internal.module.ModuleReferenceImpl;
>>> 44: import jdk.internal.module.ModuleResolution;
>> 
>> At some point we need to put in test infrastructure to avoid this and a few 
>> other tests from depending on JDK internals.
>
> Do you think it is better to parse the output of `javap` instead of using the 
> internals to detect if it worked?

Parsing the output of javap would be fragile. Instead, I think we'll just add 
some test infrastructure at some point, maybe when this class file attribute is 
documented somewhere.

-

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


Re: RFR: 8287760: --do-not-resolve-by-default gets overwritten if --warn-if-resolved flags is used [v9]

2022-06-10 Thread Alan Bateman
On Fri, 10 Jun 2022 18:25:09 GMT, Thiago Henrique Hüpner 
 wrote:

>> 8287760: --do-not-resolve-by-default gets overwritten if --warn-if-resolved 
>> flags is used
>
> Thiago Henrique Hüpner has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Rename dataprovider

Marked as reviewed by alanb (Reviewer).

-

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


Re: RFR: 8287760: --do-not-resolve-by-default gets overwritten if --warn-if-resolved flags is used [v8]

2022-06-10 Thread Alan Bateman
On Fri, 10 Jun 2022 16:30:57 GMT, Thiago Henrique Hüpner 
 wrote:

>> 8287760: --do-not-resolve-by-default gets overwritten if --warn-if-resolved 
>> flags is used
>
> Thiago Henrique Hüpner has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Fix copyright year

test/jdk/tools/jar/modularJar/Basic.java line 44:

> 42: 
> 43: import jdk.internal.module.ModuleReferenceImpl;
> 44: import jdk.internal.module.ModuleResolution;

At some point we need to put in test infrastructure to avoid this and a few 
other tests from depending on JDK internals.

test/jdk/tools/jar/modularJar/Basic.java line 951:

> 949: 
> 950: @DataProvider(name = "resolutionNames")
> 951: public Object[][] resolutionNames() {

This is a data provider for resolution warnings, maybe it should be renamed.

-

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


Re: RFR: JDK-8288227: Refactor annotation implementation to use pattern matching for instanceof

2022-06-10 Thread Alan Bateman
On Fri, 10 Jun 2022 16:15:36 GMT, Joe Darcy  wrote:

> There are many instanceof checks in the sun.reflection.annotation code; these 
> would be improved by using pattern matching for instanceof.

Marked as reviewed by alanb (Reviewer).

-

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


Re: RFR: JDK-8288207: Enhance MalformedURLException in Uri.parseCompat

2022-06-10 Thread Alan Bateman
On Fri, 10 Jun 2022 13:41:48 GMT, Matthias Baesken  wrote:

> Hi Alan , sure we could use something like the already existing hostInfo of 
> property jdk.includeInException private static final boolean 
> enhancedExceptionText = SecurityProperties.includedInExceptions("hostInfo"); 
> and make the enhancement optional/switchable this way. On the other hand we 
> already print the url (_**Cannot parse url: ldap://ad_jbs.ttt.net:389/xyz**_ 
> ) in the existing exception text so I wonder what additional problem the 
> added info would bring? That's why I did not use the property so far. But if 
> you think there could be special cases were it would be problematic to have 
> the enhancement, I'll add the usage of the property.

This is a security sensitive area and not possible to discuss all issues in JBS 
or in this PR. If this code is changed then it will require someone from 
security-dev to review.

-

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


Re: RFR: JDK-8288207: Enhance MalformedURLException in Uri.parseCompat

2022-06-10 Thread Alan Bateman
On Fri, 10 Jun 2022 12:16:17 GMT, Matthias Baesken  wrote:

> When trying to construct an LdapURL object with a bad input string (in this 
> example the _ in ad_jbs is causing issues), and not using
> the backward compatibility flag -Dcom.sun.jndi.ldapURLParsing="legacy" we run 
> into the exception below :
> 
> import com.sun.jndi.ldap.LdapURL;
>  
> String url = "ldap://ad_jbs.ttt.net:389/xyz;; // bad input string containing _
> LdapURL ldapUrl = new LdapURL(url);
> 
> 
> java --add-opens java.naming/com.sun.jndi.ldap=ALL-UNNAMED LdapParseUrlTest
> Exception in thread "main" javax.naming.NamingException: Cannot parse url: 
> ldap://ad_jbs.ttt.net:389/xyz [Root exception is 
> java.net.MalformedURLException: unsupported authority: ad_jbs.ttt.net:389]
> at java.naming/com.sun.jndi.ldap.LdapURL.(LdapURL.java:115)
> at LdapParseUrlTest.main(LdapParseUrlTest.java:9)
> Caused by: java.net.MalformedURLException: unsupported authority: 
> ad_jbs.ttt.net:389
> at java.naming/com.sun.jndi.toolkit.url.Uri.parseCompat(Uri.java:367)
> at java.naming/com.sun.jndi.toolkit.url.Uri.parse(Uri.java:230)
> at java.naming/com.sun.jndi.toolkit.url.Uri.init(Uri.java:174)
> at java.naming/com.sun.jndi.ldap.LdapURL.(LdapURL.java:105)
> 
> I would like to add the host and port info to the exception (in the example 
> it is host:port of URI:null:-1] ) so that it is directly visible that the 
> input caused the construction of a URI
> with "special"/problematic host and port values.

We have to be cautious about leaking security sensitive configuration in 
exception messages. Can you look at the security property 
jdk.includeInException (conf/security/java.security) and usages in the JDK for 
ideas on how this might be implemented as opt-in?

-

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


Re: RFR: 8287186: JDK modules participating in preview [v2]

2022-06-08 Thread Alan Bateman
On Wed, 8 Jun 2022 18:24:35 GMT, Paul Sandoz  wrote:

>> Allow JDK modules that use preview features (preview language features or 
>> preview API features from dependent modules) to participate without the need 
>> to compile with `--enable-preview`.
>> 
>> It's difficult to enable participation using an annotation due to the nature 
>> in which symbols are encountered when processing source as there is no 
>> guaranteed order to the processing of certain symbols.
>> 
>> Instead a JDK module participates if the `java.base` package 
>> `jdk.internal.javac` is exported to that module (@lahodaj clever idea!). An 
>> internal annotation `jdk.internal.javac.ParticipatesInPreview` can be 
>> declared on the module. Such a declaration cannot be enforced but does by 
>> its use require the `jdk.internal.javac`'s export list to be updated.
>> 
>> The modules `jdk.incubator.vector` and `jdk.incubator.concurrent` have been 
>> updated accordingly, both of which participate in preview APIs (APIs in 
>> `java.lang.foreign` and `Thread.ofVirtual`, respectively).
>
> Paul Sandoz has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Let java.management participate in preview features.

The updates to java.base, java.management, and jdk.incubator.* looks fine, it's 
good to have the reflection code go away.

-

Marked as reviewed by alanb (Reviewer).

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


Re: RFR: 8287186: JDK modules participating in preview

2022-06-08 Thread Alan Bateman
On Wed, 8 Jun 2022 15:46:24 GMT, Paul Sandoz  wrote:

> Allow JDK modules that use preview features (preview language features or 
> preview API features from dependent modules) to participate without the need 
> to compile with `--enable-preview`.
> 
> It's difficult to enable participation using an annotation due to the nature 
> in which symbols are encountered when processing source as there is no 
> guaranteed order to the processing of certain symbols.
> 
> Instead a JDK module participates if the `java.base` package 
> `jdk.internal.javac` is exported to that module (@lahodaj clever idea!). An 
> internal annotation `jdk.internal.javac.ParticipatesInPreview` can be 
> declared on the module. Such a declaration cannot be enforced but does by its 
> use require the `jdk.internal.javac`'s export list to be updated.
> 
> The modules `jdk.incubator.vector` and `jdk.incubator.concurrent` have been 
> updated accordingly, both of which participate in preview APIs (APIs in 
> `java.lang.foreign` and `Thread.ofVirtual`, respectively).

Can java.management participate too? It would allow 
sun.management.Util.isVirtual(Thread) to go away (lots of methods in 
sun.management.ThreadImpl need to test if a thread is virtual).

-

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


Re: RFR: 8287760: --do-not-resolve-by-default gets overwritten if --warn-if-resolved flags is used [v5]

2022-06-08 Thread Alan Bateman
On Wed, 8 Jun 2022 14:51:59 GMT, Thiago Henrique Hüpner  
wrote:

>> 8287760: --do-not-resolve-by-default gets overwritten if --warn-if-resolved 
>> flags is used
>
> Thiago Henrique Hüpner has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Fix test message

test/jdk/tools/jar/modularJar/Basic.java line 1050:

> 1048:  */
> 1049: @Test(dataProvider = "resolutionNames")
> 1050: public void 
> updateFooModuleResolutionDoNotResolveByDefaultAndWarnIfResolved(String 
> resolutionName, Predicate hasWarning) throws IOException {

Update the existing tests is good but overly long lines make it too hard to 
read (it will make future side-by-side view impossible). Can you trim all these 
lines down to something closer to the existing tests in this file?

-

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


Re: CFV: new Core Libraries Group member: Roger Riggs

2022-06-07 Thread Alan Bateman

Vote: yes


Re: RFR: 8287760: --do-not-resolve-by-default gets overwritten if --warn-if-resolved flags is used

2022-06-07 Thread Alan Bateman
On Tue, 7 Jun 2022 00:45:17 GMT, Thiago Henrique Hüpner  
wrote:

> 8287760: --do-not-resolve-by-default gets overwritten if --warn-if-resolved 
> flags is used

The change looks okay but I think we should add a test. You'll have to check 
the test tree to see if there are existing tests for incubator modules packages 
as JAR files, there may not be as it's an unusual setup.

-

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


Re: CVF: new Core Libraries Group member: Naoto Sato

2022-06-07 Thread Alan Bateman

Vote: yes


Re: RFR: JDK-8287838: Update Float and Double to use snippets [v2]

2022-06-07 Thread Alan Bateman
On Mon, 6 Jun 2022 16:54:29 GMT, Joe Darcy  wrote:

>> src/java.base/share/classes/java/lang/Double.java line 683:
>> 
>>> 681:  *   "[\\x00-\\x20]*");// Optional trailing "whitespace"
>>> 682:  *
>>> 683:  *  if (Pattern.matches(fpRegex, myString)) // @link 
>>> substring="Pattern.matches" target ="java.util.regex.Pattern#matches"
>> 
>> If you want to avoid the annoyingly long line then you can put the "// @link 
>> ..." on the previous line if you want.
>
> It can be done with a snippet region like this:
> 
> 
>  *  // @link region substring="Pattern.matches" target 
> ="java.util.regex.Pattern#matches"
>  *  if (Pattern.matches(fpRegex, myString))
>  *  Double.valueOf(myString); // Will not throw NumberFormatException
>  * // @end

Yes, that works. The alternative is to terminate the comment line with a colon 
(:) and that applies markup tag to the line that follows. In this case, it 
would be:

 ```
// @link substring="Pattern.matches" target ="java.util.regex.Pattern#matches" :

-

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


Re: RFR: JDK-8287838: Update Float and Double to use snippets [v2]

2022-06-06 Thread Alan Bateman
On Mon, 6 Jun 2022 20:37:07 GMT, Joe Darcy  wrote:

>> Various code blocks in Float and Double would be better as snippets.
>
> Joe Darcy has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - Use idiom for shorter lines
>  - Respond to review feedback.

src/java.base/share/classes/java/lang/Double.java line 683:

> 681:  *   "[\\x00-\\x20]*");// Optional trailing "whitespace"
> 682:  *
> 683:  *  if (Pattern.matches(fpRegex, myString)) // @link 
> substring="Pattern.matches" target ="java.util.regex.Pattern#matches"

If you want to avoid the annoyingly long line then you can put the "// @link 
..." on the previous line if you want.

-

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


Re: RFR: 8279283 - BufferedInputStream should override transferTo [v5]

2022-06-06 Thread Alan Bateman
On Mon, 27 Dec 2021 13:43:12 GMT, Markus KARG  wrote:

>> Implementation of JDK-8279283
>
> Markus KARG has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   fixed missing BufferedInputStream

Can this PR be closed or returned to daft?

-

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


Re: RFR: JDK-8287838: Update Float and Double to use snippets

2022-06-05 Thread Alan Bateman
On Sun, 5 Jun 2022 21:19:37 GMT, Joe Darcy  wrote:

> Various code blocks in Float and Double would be better as snippets.

One other thing you could do is link Pattern.matches in the snippet to the 
matches method.

-

Marked as reviewed by alanb (Reviewer).

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


Integrated: 8284199: Implementation of Structured Concurrency (Incubator)

2022-06-04 Thread Alan Bateman
On Thu, 19 May 2022 13:05:54 GMT, Alan Bateman  wrote:

> This is the implementation of JEP 428: Structured Concurrency (Incubator).
> 
> This is a non-final API that provides a gentle on-ramp to structure a task as 
> a family of concurrent subtasks, and to coordinate the subtasks as a unit.

This pull request has now been integrated.

Changeset: e4e1e8f6
Author:Alan Bateman 
URL:   
https://git.openjdk.java.net/jdk/commit/e4e1e8f66c9b0321cdb1aaf3b1c5d9b67224b210
Stats: 2816 lines in 11 files changed: 2815 ins; 0 del; 1 mod

8284199: Implementation of Structured Concurrency (Incubator)

Co-authored-by: Ron Pressler 
Co-authored-by: Alan Bateman 
Co-authored-by: Brian Goetz 
Co-authored-by: Paul Sandoz 
Reviewed-by: psandoz, mcimadamore, darcy

-

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


Re: RFR: 8287541: Files.writeString fails to throw IOException for charset "windows-1252"

2022-06-03 Thread Alan Bateman
On Fri, 3 Jun 2022 16:48:46 GMT, Naoto Sato  wrote:

> The code path calls `String.getBytesNoRepl()`, but it blindly replaces 
> unmappable characters with replacements if the encoder is an `ArrayEncoder`. 
> Changed only to do so if `doReplace` is `true` in 
> `String.encodeWithEncoder()`.

Marked as reviewed by alanb (Reviewer).

-

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


Re: RFR: 8287746: ProblemList jni/nullCaller/NullCallerTest.java

2022-06-02 Thread Alan Bateman
On Thu, 2 Jun 2022 18:57:36 GMT, Mandy Chung  wrote:

> 8287746: ProblemList jni/nullCaller/NullCallerTest.java

Marked as reviewed by alanb (Reviewer).

-

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


Re: RFR: 8284199: Implementation of Structured Concurrency (Incubator) [v5]

2022-06-02 Thread Alan Bateman
> This is the implementation of JEP 428: Structured Concurrency (Incubator).
> 
> This is a non-final API that provides a gentle on-ramp to structure a task as 
> a family of concurrent subtasks, and to coordinate the subtasks as a unit.

Alan Bateman 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 12 additional commits since the 
last revision:

 - Add test directory to jdk_other so tests run in tier2
 - Merge
 - Fix typo in javadoc
 - Merge
 - Javadoc updates, add to jdk_loom test group
 - Add statement to close about thread termination
 - Use {@code ...}, replace task->subtask, fix typos, add jls ref
 - Merge
 - @ignore StructuredThreadDumpTest until test infra in place
 - Refresh
 - ... and 2 more: https://git.openjdk.java.net/jdk/compare/d40ed463...7f48656e

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/8787/files
  - new: https://git.openjdk.java.net/jdk/pull/8787/files/d11b24bc..7f48656e

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=8787=04
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8787=03-04

  Stats: 46596 lines in 447 files changed: 23349 ins; 17945 del; 5302 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8787.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8787/head:pull/8787

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


Integrated: 8287496: Alternative virtual thread implementation that maps to OS thread

2022-06-02 Thread Alan Bateman
On Sun, 29 May 2022 14:46:39 GMT, Alan Bateman  wrote:

> This patch adds an alternative virtual thread implementation where each 
> virtual thread is backed by an OS thread. It doesn't scale but it can be used 
> by ports that don't have continuations support in the VM. Aside from 
> scalability, the lack of continuations support means:
> 
> 1. JVM TI is not supported when running with --enable-preview (the JVM TI 
> spec allows for this) 
> 2. jshell --enable-preview can't be used (as jshell uses the debugger APIs 
> and so needs JVM TI)
> 
> The VM option "VMContinuations" is added as an experimental option so it can 
> be used by tests. A number of tests are changed to re-run with 
> -XX:-VMContinuations. A new jtreg property is added so that tests that need 
> the underlying VM support to be present can use "@requires vm.continuations" 
> in the test description. A follow-up change would be to add "@requires 
> vm.continuations" to the ~70 serviceability/jvmti/vthread that run with 
> preview features enabled.

This pull request has now been integrated.

Changeset: 6ff2d89e
Author:Alan Bateman 
URL:   
https://git.openjdk.java.net/jdk/commit/6ff2d89ea11934bb13c8a419e7bad4fd40f76759
Stats: 747 lines in 72 files changed: 574 ins; 53 del; 120 mod

8287496: Alternative virtual thread implementation that maps to OS thread

Reviewed-by: rehn, mchung

-

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


Re: RFR: 8287496: Alternative virtual thread implementation that maps to OS thread [v3]

2022-06-02 Thread Alan Bateman
On Wed, 1 Jun 2022 06:26:23 GMT, David Holmes  wrote:

>> Alan Bateman has updated the pull request with a new target base due to a 
>> merge or a rebase. The pull request now contains 11 commits:
>> 
>>  - Fixed another typo in comment
>>  - Merge
>>  - Fix typos in comments
>>  - Allowing linking without intrinsic stub, contributed-by: rehn
>>  - Continuation clinit should fail if no continuations support
>>  - Fix merge issue with test
>>  - Change VMContinuations to be experimental option, ensure JNI GetEnv 
>> returns EVERSION
>>  - Update
>>  - Expend test coverage
>>  - Merge
>>  - ... and 1 more: 
>> https://git.openjdk.java.net/jdk/compare/3deb58a8...a5c06cb6
>
> src/java.base/share/classes/java/lang/Thread.java line 742:
> 
>> 740:  * @param name thread name, can be null
>> 741:  * @param characteristics thread characteristics
>> 742:  * @param bound true when bound to an OS thread
> 
> Nit: s/OS/VM/ ?

I think it would be confusing to use "VM" here. The intention is that "true" 
means the virtual thread will be bound to an OS thread once it has started. 
Yes, technically it's whatever the VM implements but I think that is too much 
to try to explain in the parameter description.

-

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


Re: RFR: 4511638: Double.toString(double) sometimes produces incorrect results [v14]

2022-06-01 Thread Alan Bateman
On Wed, 1 Jun 2022 08:56:38 GMT, Raffaello Giulietti  
wrote:

>> src/java.base/share/classes/jdk/internal/math/MathUtils.java line 38:
>> 
>>> 36:  *
>>> 37:  * Giulietti, "The Schubfach way to render doubles",
>>> 38:  * 
>>> https://drive.google.com/file/d/1gp5xv4CAa78SVgCeWfGqqI4FfYYYuNFb
>> 
>> Even though not public, should the reference use the `` tag and 
>> perhaps be in a `@see` annotation?
>> 
>> @see > href=“https://drive.google.com/file/d/1gp5xv4CAa78SVgCeWfGqqI4FfYYYuNFb”>
>>  The Schubfach way to render doubles
>
> These references are inside a normal multi-line comment, so I'm not sure that 
> Javadoc or html tags or have a well-defined semantics there.

I'm not sure about linking to a document on Google Drive, even from JDK 
internal classes. Is the paper uploaded to anywhere else that could be linked 
to instead?

-

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


Re: RFR: 8287496: Alternative virtual thread implementation that maps to OS thread [v3]

2022-06-01 Thread Alan Bateman
> This patch adds an alternative virtual thread implementation where each 
> virtual thread is backed by an OS thread. It doesn't scale but it can be used 
> by ports that don't have continuations support in the VM. Aside from 
> scalability, the lack of continuations support means:
> 
> 1. JVM TI is not supported when running with --enable-preview (the JVM TI 
> spec allows for this) 
> 2. jshell --enable-preview can't be used (as jshell uses the debugger APIs 
> and so needs JVM TI)
> 
> The VM option "VMContinuations" is added as an experimental option so it can 
> be used by tests. A number of tests are changed to re-run with 
> -XX:-VMContinuations. A new jtreg property is added so that tests that need 
> the underlying VM support to be present can use "@requires vm.continuations" 
> in the test description. A follow-up change would be to add "@requires 
> vm.continuations" to the ~70 serviceability/jvmti/vthread that run with 
> preview features enabled.

Alan Bateman has updated the pull request with a new target base due to a merge 
or a rebase. The pull request now contains 11 commits:

 - Fixed another typo in comment
 - Merge
 - Fix typos in comments
 - Allowing linking without intrinsic stub, contributed-by: rehn
 - Continuation clinit should fail if no continuations support
 - Fix merge issue with test
 - Change VMContinuations to be experimental option, ensure JNI GetEnv returns 
EVERSION
 - Update
 - Expend test coverage
 - Merge
 - ... and 1 more: https://git.openjdk.java.net/jdk/compare/3deb58a8...a5c06cb6

-

Changes: https://git.openjdk.java.net/jdk/pull/8939/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8939=02
  Stats: 747 lines in 72 files changed: 574 ins; 53 del; 120 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8939.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8939/head:pull/8939

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


Re: RFR: 8287496: Alternative virtual thread implementation that maps to OS thread [v2]

2022-05-31 Thread Alan Bateman
On Tue, 31 May 2022 17:03:54 GMT, Aleksey Shipilev  wrote:

> I expected this change to fix the broken ARM32 port, but it doesn't work.

There is work required to get the arm32 port working again, currently tracked 
as JDK-828636 but there may be further issues beyond that.

-

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


Re: RFR: 8287496: Alternative virtual thread implementation that maps to OS thread [v2]

2022-05-31 Thread Alan Bateman
> This patch adds an alternative virtual thread implementation where each 
> virtual thread is backed by an OS thread. It doesn't scale but it can be used 
> by ports that don't have continuations support in the VM. Aside from 
> scalability, the lack of continuations support means:
> 
> 1. JVM TI is not supported when running with --enable-preview (the JVM TI 
> spec allows for this) 
> 2. jshell --enable-preview can't be used (as jshell uses the debugger APIs 
> and so needs JVM TI)
> 
> The VM option "VMContinuations" is added as an experimental option so it can 
> be used by tests. A number of tests are changed to re-run with 
> -XX:-VMContinuations. A new jtreg property is added so that tests that need 
> the underlying VM support to be present can use "@requires vm.continuations" 
> in the test description. A follow-up change would be to add "@requires 
> vm.continuations" to the ~70 serviceability/jvmti/vthread that run with 
> preview features enabled.

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

  Allowing linking without intrinsic stub, contributed-by: rehn

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/8939/files
  - new: https://git.openjdk.java.net/jdk/pull/8939/files/7989708b..126e38b6

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=8939=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8939=00-01

  Stats: 5 lines in 1 file changed: 4 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8939.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8939/head:pull/8939

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


Re: RFR: 8287520: Shrink x86_32 problemlists after JDK-8287437

2022-05-31 Thread Alan Bateman
On Mon, 30 May 2022 13:20:17 GMT, Aleksey Shipilev  wrote:

> [JDK-8287137](https://bugs.openjdk.java.net/browse/JDK-8287137) added a bunch 
> of tests into problemlist. Those lists basically exclude every test that runs 
> with --enable-preview. 
> [JDK-8287437](https://bugs.openjdk.java.net/browse/JDK-8287437) makes it 
> better: it only disables Loom parts, even with --enable-preview. This allows 
> testing x86_32 on other preview features and avoids accidental bug slippage 
> in non-Loom code paths. We can and should shrink the problem lists now.
> 
> Additional testing:
>  - [x] Removed tests with Linux x86_32 fastdebug; mostly pass, some non-Loom 
> failures
>  - [x] Linux x86_32 fastdebug `tier1`
>  - [ ] GHA run

This looks okay.  If we get PR8939 in then it should be possible to do more 
clear out of the exclude lists.

-

Marked as reviewed by alanb (Reviewer).

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


Re: RFR: 8287496: Alternative virtual thread implementation that maps to OS thread

2022-05-31 Thread Alan Bateman
On Sun, 29 May 2022 14:46:39 GMT, Alan Bateman  wrote:

> This patch adds an alternative virtual thread implementation where each 
> virtual thread is backed by an OS thread. It doesn't scale but it can be used 
> by ports that don't have continuations support in the VM. Aside from 
> scalability, the lack of continuations support means:
> 
> 1. JVM TI is not supported when running with --enable-preview (the JVM TI 
> spec allows for this) 
> 2. jshell --enable-preview can't be used (as jshell uses the debugger APIs 
> and so needs JVM TI)
> 
> The VM option "VMContinuations" is added as an experimental option so it can 
> be used by tests. A number of tests are changed to re-run with 
> -XX:-VMContinuations. A new jtreg property is added so that tests that need 
> the underlying VM support to be present can use "@requires vm.continuations" 
> in the test description. A follow-up change would be to add "@requires 
> vm.continuations" to the ~70 serviceability/jvmti/vthread that run with 
> preview features enabled.

> Since the package `jdk.internal.access` is 
> exported[1](#user-content-fn-1-97aea7d7960164849e591e42b91fb5c4) to the 
> `java.management` module, this can use `MethodHandle`s obtained using the 
> trusted lookup.

That export is for another reason, and probably should be re-examined so that 
java.base doesn't need to export this package to java.management. In any case, 
we expect there will be compiler support soon to allow java.management be 
compiled with code that makes use of preview APIs in java.base. That will at 
least us reference Thread::isVirtual from code.

-

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


RFR: 8287496: Alternative virtual thread implementation that maps to OS thread

2022-05-31 Thread Alan Bateman
This patch adds an alternative virtual thread implementation where each virtual 
thread is backed by an OS thread. It doesn't scale but it can be used by ports 
that don't have continuations support in the VM. Aside from scalability, the 
lack of continuations support means:

1. JVM TI is not supported when running with --enable-preview (the JVM TI spec 
allows for this) 
2. jshell --enable-preview can't be used (as jshell uses the debugger APIs and 
so needs JVM TI)

The VM option "VMContinuations" is added as an experimental option so it can be 
used by tests. A number of tests are changed to re-run with 
-XX:-VMContinuations. A new jtreg property is added so that tests that need the 
underlying VM support to be present can use "@requires vm.continuations" in the 
test description. A follow-up change would be to add "@requires 
vm.continuations" to the ~70 serviceability/jvmti/vthread that run with preview 
features enabled.

-

Commit messages:
 - Continuation clinit should fail if no continuations support
 - Fix merge issue with test
 - Change VMContinuations to be experimental option, ensure JNI GetEnv returns 
EVERSION
 - Update
 - Expend test coverage
 - Merge
 - Initial commit

Changes: https://git.openjdk.java.net/jdk/pull/8939/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8939=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8287496
  Stats: 742 lines in 71 files changed: 570 ins; 53 del; 119 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8939.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8939/head:pull/8939

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


Re: RFR: 8284199: Implementation of Structured Concurrency (Incubator) [v4]

2022-05-30 Thread Alan Bateman
> This is the implementation of JEP 428: Structured Concurrency (Incubator).
> 
> This is a non-final API that provides a gentle on-ramp to structure a task as 
> a family of concurrent subtasks, and to coordinate the subtasks as a unit.

Alan Bateman 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 10 additional commits since the 
last revision:

 - Fix typo in javadoc
 - Merge
 - Javadoc updates, add to jdk_loom test group
 - Add statement to close about thread termination
 - Use {@code ...}, replace task->subtask, fix typos, add jls ref
 - Merge
 - @ignore StructuredThreadDumpTest until test infra in place
 - Refresh
 - Merge
 - Initial commit

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/8787/files
  - new: https://git.openjdk.java.net/jdk/pull/8787/files/4fc454a9..d11b24bc

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=8787=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8787=02-03

  Stats: 33428 lines in 507 files changed: 6180 ins; 25559 del; 1689 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8787.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8787/head:pull/8787

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


Re: RFR: 8284638: store skip buffers in InputStream Object [v15]

2022-05-30 Thread Alan Bateman
On Mon, 30 May 2022 05:29:01 GMT, Alan Bateman  wrote:

>> More practically.
>> This PR has a noticeable negative effect - it increases the size of 
>> InputStream objects. Moreover, it increases the size of InputStream 
>> subclasses which has own skip() implementation and don't need this 
>> superclass modification.
>> 
>> Let's look into InputStream subclasses.
>> I've checked 51 InputStream from classlib. 30 of them either have their own 
>> skip() implementation or use super.skip() other than from InputStream. This 
>> PR is definitely harmful to these 30 classes. These 30 classes can be 
>> divided into the following categories:
>> 
>> 1) Clean non-allocating skip() implementation (22 classes):
>>  - BufferedInputStream (java.io) 
>>  - ByteArrayInputStream (java.io)
>>  - FileInputStream (java.io) 
>>  - FilterInputStream (java.io)
>>  - PeekInputStream in ObjectInputStream (java.io)
>>  - BlockDataInputStream in ObjectInputStream (java.io) 
>>  - PushbackInputStream (java.io) 
>>  - FastInputStream in Manifest (java.util.jar)
>>  - ZipFileInputStream in ZipFile (java.util.zip)
>>  - CipherInputStream (javax.crypto)
>>  - MeteredStream (sun.net.www)
>>  - Anonymous in nullInputStream() in InputStream (java.io)
>>  - DataInputStream (java.io)
>>  - Anonymous in readTrailer() in GZIPInputStream (java.util.zip)
>>  - FtpInputStream in FtpURLConnection (sun.net.www.protocol.ftp)
>>  - JarURLInputStream in JarURLConnection (sun.net.www.protocol.jar)
>>  - PlainTextInputStream (sun.net.www.content.text)
>>  - PushbackInputStream (java.io)
>>  - TelnetInputStream (sun.net)
>>  - UnclosableInputStream in FileInputStreamPool (sun.security.provider)
>>  - MeteredStream (sun.net.www)
>>  - KeepAliveStream (sun.net.www.http)
>> 
>> 2) Partially clean skip() implementation (1 class):
>>  - ChannelInputStream (sun.nio.ch)
>>Note: clean skip impl when using SeekableByteChannel (most usages) 
>> otherwise super.skip() is used, and it may be easily rewritten using the 
>> existing internal buffer.
>> 
>> 3) Unavoidable skip buffers (7 classes):
>>  - PipeInputStream in Process (java.lang) // per skip() invocation buffer 
>> byte[2048]
>>  - CheckedInputStream (java.util.zip) // per skip() invocation buffer 
>> byte[512]
>>  - InflaterInputStream (java.util.zip)// per skip() invocation buffer 
>> byte[512]
>>  - AppInputStream in SSLSocketImpl (sun.security.ssl) // per skip() 
>> invocation buffer byte[256]
>>  - DeflaterInputStream (java.util.zip)   //  cached  skip buffer, byte[512], 
>> allocated on demand
>>  - ZipInputStream (java.util.zip)   //   preallocated skip buffer 
>> byte[512]
>>  - HttpInputStream in HttpURLConnection (sun.net.www.protocol.http) //  
>> cached  skip buffer, byte[8096], allocated on demand
>> 
>> It would be better to clean all skip implementations for the latter category 
>> and make it consistent. Now it's totally a mess. All possible strategies 
>> were implemented.
>> 
>> Now let's consider classes which uses InputStream.skip() implementation (21 
>> classes):
>> 
>> 4) skip() is not implemented, when trivial implementation is possible (4 
>> classes):
>>  - EmptyInputStream (sun.net.www.protocol.http)
>>  - NullInputStream in ProcessBuilder (java.lang)
>>  - ObjectInputStream (java.io)
>>  - extObjectInputStream (javax.crypto)
>> 
>> 5) skip() is not implemented, when not-so-trivial implementation is possible 
>> (9 classes):
>>  - Anonymous in getInputStream() in NioSocketImpl (sun.nio.ch)  
>>Notes: temp direct buffer is used for reading, it's possible to implement 
>> skip over the direct buffer, save allocation + no copying from direct buffer 
>> to byte[]
>>  - Anonymous in newInputStream() in Channels (java.nio.channels)
>>Notes: temp direct buffer is used for reading, it's possible to implement 
>> skip over the direct buffer, save allocation + no copying from direct buffer 
>> to byte[]
>>  - ChunkedInputStream (sun.net.www.http)
>>Notes: skip() maybe implemented with the existing internal buffer
>>  - DecInputStream in Base64 (java.util)   
>>  - ErrorStream in HttpURLConnection (sun.net.www.protocol.http)
>>Notes: skip (but easily can be implemented with internal buffer + 
>> delegation to tail stream)
>>  - PipedInputStream (java.io)
>>Notes: skip() may be implemented with the existing internal buffer
>>  - SequenceInputStream (java.io)   
>>Notes: skip() maybe implemented with delegat

Re: RFR: 8287171: Refactor null caller tests to a single directory [v3]

2022-05-29 Thread Alan Bateman
On Mon, 30 May 2022 00:10:50 GMT, Tim Prinzing  wrote:

>> Created a test at test/jdk/jdk/nullCaller called NullCallerTest that creates 
>> a test module with some resources in it for the actual tests that occur at 
>> the native level. The native part was switched to c++ instead of c to make 
>> it easier to create helper objects that reduce the redundant code performing 
>> error checking. The two helper classes InstanceCall and StaticCall were 
>> placed in an include file called CallHelper.hpp. The main part of the tests 
>> are in exeNullCallerTest.cpp, and there is a separate function for each of 
>> the bug reports.
>
> Tim Prinzing has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   problem with iostream on Windows, use printf

I don't think jdk/nullCaller is right location for this. Maybe jni/nullCaller 
could work. You'll probably need to add the location to an appropriate 
group/tier in test/jdk/TEST.groups, otherwise it won't be run.

-

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


Re: RFR: 8287363: null pointer should use NULL instead of 0

2022-05-29 Thread Alan Bateman
On Sat, 28 May 2022 03:31:30 GMT, Yasumasa Suenaga  wrote:

> We found using `0` as `NULL` in java_md_common.c . `0` is not a pointer, so 
> we should use `NULL` where we want to handle it.
> 
> https://github.com/openjdk/jdk/pull/8646#discussion_r882294076
> 
> Also I found using `0` as NUL char in java_md_common.c , so I replaced it to 
> `\0` in this PR.

Marked as reviewed by alanb (Reviewer).

-

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


Re: RFR: 8284638: store skip buffers in InputStream Object [v15]

2022-05-29 Thread Alan Bateman
On Sun, 29 May 2022 18:15:52 GMT, Sergey Kuksenko  wrote:

> 5. skip() is not implemented, when not-so-trivial implementation is possible 
> (9 classes):

For the low-level streams (e.g. connected to socket) then it would be common to 
see them wrapped by buffered streams. So it might not be worth doing anything 
there.

However, I think your suggestion to change the no-arg read/write be 
non-abstract is interesting as it's always a pain to have to implement that.

-

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


Re: RFR: 8287285: Avoid redundant HashMap.containsKey call in java.util.zip.ZipFile.Source.get

2022-05-26 Thread Alan Bateman
On Sat, 30 Apr 2022 08:56:23 GMT, Andrey Turbanov  wrote:

> The method `java.util.zip.ZipFile.Source#get` could be improved by usage of 
> `Map.putIfAbsent` instead of separate `containsKey`/`get`/`put` calls. We 
> known that HashMap `java.util.zip.ZipFile.Source#files` can contain only 
> non-null values. And to check if putIfAbsent was successful or not we can 
> just compare result with `null`.
> It makes code a bit cleaner and faster.

Marked as reviewed by alanb (Reviewer).

-

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


Re: RFR: 8284199: Implementation of Structured Concurrency (Incubator) [v3]

2022-05-24 Thread Alan Bateman
> This is the implementation of JEP 428: Structured Concurrency (Incubator).
> 
> This is a non-final API that provides a gentle on-ramp to structure a task as 
> a family of concurrent subtasks, and to coordinate the subtasks as a unit.

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

  Add statement to close about thread termination

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/8787/files
  - new: https://git.openjdk.java.net/jdk/pull/8787/files/c72f0330..4fc454a9

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=8787=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8787=01-02

  Stats: 12 lines in 3 files changed: 5 ins; 0 del; 7 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8787.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8787/head:pull/8787

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


Re: RFR: 8284199: Implementation of Structured Concurrency (Incubator) [v2]

2022-05-24 Thread Alan Bateman
On Tue, 24 May 2022 10:52:07 GMT, Maurizio Cimadamore  
wrote:

>> Alan Bateman 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 six additional 
>> commits since the last revision:
>> 
>>  - Use {@code ...}, replace task->subtask, fix typos, add jls ref
>>  - Merge
>>  - @ignore StructuredThreadDumpTest until test infra in place
>>  - Refresh
>>  - Merge
>>  - Initial commit
>
> src/jdk.incubator.concurrent/share/classes/jdk/incubator/concurrent/StructuredTaskScope.java
>  line 214:
> 
>> 212:  * Tree structure
>> 213:  *
>> 214:  * StructuredTaskScopes form a tree where parent-child relations are 
>> established
> 
> Should we mention what happens in the owner thread completes its execution 
> and the scope's `close` method has not been called? I think that, as 
> discussed offline, the fact that the thread will attempt to close any nested 
> scopes when terminating is an important aspect of this API.

The close method might be the right place for this. It has to specify that an 
attempt to close out of order will close the nested scopes and terminating 
without close is much the same. I'll see what I can do, thanks for this 
suggestion.

-

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


Re: RFR: 8284199: Implementation of Structured Concurrency (Incubator) [v2]

2022-05-24 Thread Alan Bateman
On Tue, 24 May 2022 10:48:02 GMT, Maurizio Cimadamore  
wrote:

>> More generally, I see that you used `{@code ... }` in a lot of places where 
>> `{@link ... }` could also be used. In some of those places (like this one) 
>> where there is a clear cross-reference, I think `@link` could be 
>> preferrable. The only case where `@code` is fine is when referring to the 
>> name of the class itself (e.g. `{@code StructuredTaskScope}`). But of course 
>> this is subjective.
>
> Also, note the typo `the join is invoked`. Either `the` is dropped, or 
> `method` is added. I've seen more than one occurrence of this.

It should be "before join is invoked". It doesn't use a link here because it 
already links to join and joinUntil in the previous sentence.

-

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


Re: RFR: 8284199: Implementation of Structured Concurrency (Incubator) [v2]

2022-05-24 Thread Alan Bateman
> This is the implementation of JEP 428: Structured Concurrency (Incubator).
> 
> This is a non-final API that provides a gentle on-ramp to structure a task as 
> a family of concurrent subtasks, and to coordinate the subtasks as a unit.

Alan Bateman 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 six additional commits since the 
last revision:

 - Use {@code ...}, replace task->subtask, fix typos, add jls ref
 - Merge
 - @ignore StructuredThreadDumpTest until test infra in place
 - Refresh
 - Merge
 - Initial commit

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/8787/files
  - new: https://git.openjdk.java.net/jdk/pull/8787/files/6a9553b9..c72f0330

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=8787=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8787=00-01

  Stats: 6142 lines in 192 files changed: 3679 ins; 1909 del; 554 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8787.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8787/head:pull/8787

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


Re: RFR: 8284199: Implementation of Structured Concurrency (Incubator)

2022-05-24 Thread Alan Bateman
On Tue, 24 May 2022 04:18:44 GMT, Joe Darcy  wrote:

>> This is the implementation of JEP 428: Structured Concurrency (Incubator).
>> 
>> This is a non-final API that provides a gentle on-ramp to structure a task 
>> as a family of concurrent subtasks, and to coordinate the subtasks as a unit.
>
> src/jdk.incubator.concurrent/share/classes/jdk/incubator/concurrent/StructuredTaskScope.java
>  line 237:
> 
>> 235:  * the task result is retrieved via its {@code Future}, or 
>> happen-before any actions
>> 236:  * taken in a thread after {@linkplain #join() joining} of the task 
>> scope.
>> 237:  *
> 
> Would a @-jls reference to the appropriate section of the memory model 
> chapter help here?

Yes, it can reference JLS 17.4.5.

-

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


Re: RFR: 8284199: Implementation of Structured Concurrency (Incubator)

2022-05-24 Thread Alan Bateman
On Mon, 23 May 2022 21:09:24 GMT, Maurizio Cimadamore  
wrote:

>> This is the implementation of JEP 428: Structured Concurrency (Incubator).
>> 
>> This is a non-final API that provides a gentle on-ramp to structure a task 
>> as a family of concurrent subtasks, and to coordinate the subtasks as a unit.
>
> src/jdk.incubator.concurrent/share/classes/jdk/incubator/concurrent/StructuredTaskScope.java
>  line 88:
> 
>> 86:  * {@code join} method after forking.
>> 87:  *
>> 88:  *  StructuredTaskScope defines the {@link #shutdown() shutdown} 
>> method to shut down a
> 
> This sentence, because of the place where it appears, is a bit confusing. So 
> far we only know about the fact that a scope has an owner thread. So it seems 
> odd that shutdown could be called _while_ the owner thread is waiting on a 
> `join`. Of course, then you read what's next, and you discover that: (a) 
> shutdown might be called by a custom scope subclass and that (b) shutdown is 
> confined to the threads contained in this task scope - but this definition is 
> only given much later.

I see your point. The intention is to introduce all the public methods before 
introducing the subclasses or policies. I think I can adjust this sentence to 
make it clear that a subtask may call shutdown while the owner is waiting in 
join.

> src/jdk.incubator.concurrent/share/classes/jdk/incubator/concurrent/StructuredTaskScope.java
>  line 353:
> 
>> 351:  *
>> 352:  *  The {@code handleComplete} method should be thread safe. It 
>> may be
>> 353:  * invoked by several threads at around the same.
> 
> Something is missing? E.g. "at around the same TIME" ? (I'd suggest just 
> using "concurrently")

Thanks, it was supposed to say "around the same time" but saying "concurrently" 
would be better.

> src/jdk.incubator.concurrent/share/classes/jdk/incubator/concurrent/StructuredTaskScope.java
>  line 376:
> 
>> 374:  *
>> 375:  *  If this task scope is {@linkplain #shutdown() shutdown} (or 
>> in the process
>> 376:  * of shutting down) then {@code fork} returns a Future 
>> representing a {@link
> 
> Future in plaintext?

Yes, Daniel also pointed this point that there are a few uses of 
"StructuredTaskScope" that should also use {@code ...}.

-

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


Re: RFR: 8284199: Implementation of Structured Concurrency (Incubator)

2022-05-24 Thread Alan Bateman
On Mon, 23 May 2022 13:11:29 GMT, Daniel Fuchs  wrote:

>> This is the implementation of JEP 428: Structured Concurrency (Incubator).
>> 
>> This is a non-final API that provides a gentle on-ramp to structure a task 
>> as a family of concurrent subtasks, and to coordinate the subtasks as a unit.
>
> src/jdk.incubator.concurrent/share/classes/jdk/incubator/concurrent/StructuredTaskScope.java
>  line 54:
> 
>> 52: 
>> 53: /**
>> 54:  * A basic API for structured concurrency. StructuredTaskScope 
>> supports cases
> 
> Should StructuredTaskScope in this class-level API doc comment be surrounded 
> by `{@code }` to appear in code font?

Okay, there are quite a few of these so I may have to adjust some of the lines 
to avoid too many inconsistent line lengths.

-

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


Re: RFR: 8287137: Problemlist failing x86_32 tests after Loom integration [v3]

2022-05-24 Thread Alan Bateman
On Tue, 24 May 2022 07:01:52 GMT, Aleksey Shipilev  wrote:

> Please approve, @AlanBateman, @mcimadamore and others?

Okay with me.

-

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


Re: RFR: 8287206: Use WrongThreadException for confinement errors

2022-05-24 Thread Alan Bateman
On Tue, 24 May 2022 09:26:44 GMT, Maurizio Cimadamore  
wrote:

> This patch tweaks the foreign API to use the newly added 
> `WrongThreadException` instead of `IllegalStateException` to report 
> confinement errors.

Marked as reviewed by alanb (Reviewer).

-

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


Re: RFR: 8287162: (zipfs) Performance regression related to support for POSIX file permissions

2022-05-23 Thread Alan Bateman
On Mon, 23 May 2022 19:47:33 GMT, Lance Andersen  wrote:

> Hi all,
> 
> This PR addresses the performance issue that is described in JDK-8287162.
> 
> With this fix,  the ZipFileSystem methods:  initOwner, initGroup, and 
> initPermissions will not be invoked unless enablePosixFileAttributes=true.  
> 
> Mach5 tiers1-3 are currently running and have not encountered any issues.

Marked as reviewed by alanb (Reviewer).

-

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


Re: RFR: 8287137: Problemlist failing x86_32 tests after Loom integration

2022-05-23 Thread Alan Bateman
On Mon, 23 May 2022 15:18:10 GMT, Aleksey Shipilev  wrote:

>> test/jdk/ProblemList.txt line 894:
>> 
>>> 892: 
>>> java/util/stream/test/org/openjdk/tests/java/util/stream/CollectionAndMapModifyStreamTest.java
>>>   8286642 generic-i586
>>> 893: 
>>> java/util/stream/test/org/openjdk/tests/java/util/stream/CollectorExample.java
>>>   8286642 generic-i586
>>> 894: 
>>> java/util/stream/test/org/openjdk/tests/java/util/stream/CollectorToUnmodListTest.java
>>>   8286642 generic-i586
>> 
>> I'm surprised that the stream tests are included as they don't run with 
>> --enable-preview.
>
> Those tests *do* run with preview enabled: 
> https://github.com/openjdk/jdk/blob/master/test/jdk/java/util/stream/test/TEST.properties#L10-L11
>  -- and I know that because I basically copy-pasted all jtreg failures that 
> lead to `Unimplemented()` in x86_32 code.

Okay, so the issue is SpliteratorTest uses preview APIs and having the 
configuration in TEST.properties means that all the streams tests are run with 
this option.

-

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


Re: RFR: 8287137: Problemlist failing x86_32 tests after Loom integration

2022-05-23 Thread Alan Bateman
On Mon, 23 May 2022 12:28:30 GMT, Aleksey Shipilev  wrote:

> [JDK-8284161](https://bugs.openjdk.java.net/browse/JDK-8284161) broke a lot 
> of x86_32 code. The x86_32 porting is done under 
> [JDK-8286642](https://bugs.openjdk.java.net/browse/JDK-8286642). Meanwhile, 
> we can problemlist the failing tests to get cleaner runs for other patches. 
> This should also make GHA runs cleaner.
> 
> Additional testing:
>  - [x] Linux x86_32 fastdebug `tier1` (some unrelated failures in hotspot)
>  - [x] Linux x86_32 fastdebug `tier2` (some unrelated failures in hotspot)
>  - [x] Linux x86_32 fastdebug `tier3` (clean now)
>  - [ ] GHA run

test/jdk/ProblemList.txt line 894:

> 892: 
> java/util/stream/test/org/openjdk/tests/java/util/stream/CollectionAndMapModifyStreamTest.java
>   8286642 generic-i586
> 893: 
> java/util/stream/test/org/openjdk/tests/java/util/stream/CollectorExample.java
>   8286642 generic-i586
> 894: 
> java/util/stream/test/org/openjdk/tests/java/util/stream/CollectorToUnmodListTest.java
>   8286642 generic-i586

I'm surprised that the stream tests are included as they don't run with 
--enable-preview.

-

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


Re: RFR: 8284199: Implementation of Structured Concurrency (Incubator)

2022-05-23 Thread Alan Bateman
On Sat, 21 May 2022 14:09:59 GMT, ExE Boss  wrote:

>> This is the implementation of JEP 428: Structured Concurrency (Incubator).
>> 
>> This is a non-final API that provides a gentle on-ramp to structure a task 
>> as a family of concurrent subtasks, and to coordinate the subtasks as a unit.
>
> src/jdk.incubator.concurrent/share/classes/jdk/incubator/concurrent/StructuredTaskScope.java
>  line 1172:
> 
>> 1170: }
>> 1171: };
>> 1172: return AccessController.doPrivileged(pa);
> 
> It might be better to use `MethodHandle`s obtained using 
> [jdk.internal.access.SharedSecrets].getJavaLangInvokeAccess()
>  and [JavaLangInvokeAccess].findStatic(…) and 
> [JavaLangInvokeAccess].findVirtual(…) for this, which 
> would avoid going through `AccessController.doPrivilaged(…)`.
> 
> [jdk.internal.access.SharedSecrets]: 
> https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/jdk/internal/access/SharedSecrets.java
> [JavaLangInvokeAccess]: 
> https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/jdk/internal/access/JavaLangInvokeAccess.java

I'd prefer not export jdk.internal.access to this module, if possible.  In 
general, it's a lot easier to reason about the security and integrity of the 
core platform when java.base doesn't export any of its internal packages.

In any case, I expect this issue will resolve itself once there is a way for 
code in "core" modules to use preview APIs without needing to be compiled with 
--enable-preview. The java.management and jdk.incubator.vector modules have the 
same issue.

-

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


RFR: 8284199: Implementation of Structured Concurrency (Incubator)

2022-05-23 Thread Alan Bateman
This is the implementation of JEP 428: Structured Concurrency (Incubator).

This is a non-final API that provides a gentle on-ramp to structure a task as a 
family of concurrent subtasks, and to coordinate the subtasks as a unit.

-

Commit messages:
 - @ignore StructuredThreadDumpTest until test infra in place
 - Refresh
 - Merge
 - Initial commit

Changes: https://git.openjdk.java.net/jdk/pull/8787/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8787=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8284199
  Stats: 2801 lines in 10 files changed: 2800 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8787.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8787/head:pull/8787

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


Re: RFR: 8287121: Fix typo in jlink's description resource key lookup

2022-05-23 Thread Alan Bateman
On Sun, 22 May 2022 05:58:25 GMT, Christian Stein  wrote:

> Commit 
> https://github.com/openjdk/jdk/commit/655500a4f5e3abcff176599604deceefb6ca6640
>  for issue [JDK-8286654](https://bugs.openjdk.java.net/browse/JDK-8286654) 
> added an optional description accessor on the `ToolProvider` interface. It 
> included a typo in` jlink`'s description resource key lookup: 
> `"jlink.desciption"`
> 
> This follow-up commit fixes the typo by adding the missing `r` character: 
> `"jlink.description"`.
> 
> This commit also also adds an automated check that ensures all current and 
> future tool provider implementations within the JDK don't throw an exception 
> when invoking their name and description accessor methods. Specific tool 
> names and descriptions are not expected by this general test.

Marked as reviewed by alanb (Reviewer).

-

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


Re: RFR: 8286571: java source launcher from a minimal jdk image containing jdk.compiler fails with --enable-preview option [v3]

2022-05-23 Thread Alan Bateman
On Mon, 23 May 2022 07:29:56 GMT, Adam Sotona  wrote:

> Sorry, I wrongly assumed it was `java.compiler` (the library module), but 
> this is about `jdk.compiler` (the tool)... Nevertheless, the same question 
> applies: Does `jdk.compiler` have any meaningful functionality even without 
> `jdk.zipfs` ?

This came up in JDK 9 and the decision at the time was not to require jdk.zipfs 
as it's a service provider module. The addition of the source launcher, the 
addition of --enable-preview, and more use of --release N means more ct.sym 
usage. It probably isn't too interesting to have a small runtime image that 
contains jdk.compiler and not jdk.zipfs so Adam's proposal mightn't be too bad, 
it's just a bit weird and not something that we'd want other libraries in the 
eco system to do.

-

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


Re: RFR: 8287121: Fix typo in jlink's description resource key lookup

2022-05-22 Thread Alan Bateman
On Sun, 22 May 2022 05:58:25 GMT, Christian Stein  wrote:

> Commit 
> https://github.com/openjdk/jdk/commit/655500a4f5e3abcff176599604deceefb6ca6640
>  for issue [JDK-8286654](https://bugs.openjdk.java.net/browse/JDK-8286654) 
> added an optional description accessor on the `ToolProvider` interface. It 
> included a typo in` jlink`'s description resource key lookup: 
> `"jlink.desciption"`
> 
> This follow-up commit fixes the typo by adding the missing `r` character: 
> `"jlink.description"`.
> 
> This commit also also adds an automated check that ensures all current and 
> future tool provider implementations within the JDK don't throw an exception 
> when invoking their name and description accessor methods. Specific tool 
> names and descriptions are not expected by this general test.

test/jdk/java/util/spi/ToolProviderDescriptionTest.java line 46:

> 44: 
> 45: static List listToolDescriptions() {
> 46: return 
> StreamSupport.stream(ServiceLoader.load(ToolProvider.class).spliterator(), 
> false)

it doesn't matter for this test but SL does have a stream method that could be 
used instead:

ServiceLoader.load(ToolProvider.class)
 .stream()
.map(ServiceLoader.Provider::get)
.sorted(Comparator.comparing(ToolProvider::name))
...

-

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


Re: RFR: 8286956: Loom: Define test groups for development/porting use

2022-05-18 Thread Alan Bateman
On Wed, 18 May 2022 11:10:58 GMT, Aleksey Shipilev  wrote:

> It would be beneficial to bring over the Loom-specific test groups from the 
> loom repo to aid development/porting work.
> 
> https://github.com/openjdk/loom/blob/fibers/test/jdk/TEST.groups#L97-L108
> https://github.com/openjdk/loom/blob/6f42923b3342e41d95b262733205283068802b40/test/hotspot/jtreg/TEST.groups#L111-L125
> 
> I had to drop `jdk/incubator/concurrent` from JDK definition, because there 
> are no tests in that folder and tests break.
> 
> Additional testing:
>  - [x] Linux x86_64 fastdebug `hotspot_loom jdk_loom`
>  - [x] Linux AArch64 fastdebug `hotspot_loom jdk_loom`

Marked as reviewed by alanb (Reviewer).

-

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


Integrated: 8286788: Test java/lang/Thread/virtual/ThreadAPI.testGetStackTrace3 fails

2022-05-17 Thread Alan Bateman
On Tue, 17 May 2022 11:15:19 GMT, Alan Bateman  wrote:

> This is a test fix. ThreadAPI.testGetStackTrace3 tests Thread::getStackTrace 
> on a thread doing a selection operation. The test is not reliable as it 
> expects to see the "select" method in the stack trace after waiting 200ms. 
> The test is changed to poll the stack trace so that it's no longer dependent 
> on sleep.
> 
> The update includes a drive-by change to the test description to use 
> `@enablePreview`.

This pull request has now been integrated.

Changeset: 8535d51d
Author:Alan Bateman 
URL:   
https://git.openjdk.java.net/jdk/commit/8535d51db7e1c33218c4254e774de4ca4ca60023
Stats: 10 lines in 1 file changed: 3 ins; 2 del; 5 mod

8286788: Test java/lang/Thread/virtual/ThreadAPI.testGetStackTrace3 fails

Reviewed-by: darcy, jpai

-

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


RFR: 8286788: Test java/lang/Thread/virtual/ThreadAPI.testGetStackTrace3 fails

2022-05-17 Thread Alan Bateman
This is a test fix. ThreadAPI.testGetStackTrace3 tests Thread::getStackTrace on 
a thread doing a selection operation. The test is not reliable as it expects to 
see the "select" method in the stack trace after waiting 200ms. The test is 
changed to poll the stack trace so that it's no longer dependent on sleep.

The update includes a drive-by change to the test description to use 
`@enablePreview`.

-

Commit messages:
 - Initial commit

Changes: https://git.openjdk.java.net/jdk/pull/8743/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8743=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8286788
  Stats: 10 lines in 1 file changed: 3 ins; 2 del; 5 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8743.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8743/head:pull/8743

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


Re: RFR: JDK-8286783: Expand use of @inheritDoc in InputStream and OutputStream subclasses [v4]

2022-05-16 Thread Alan Bateman
On Mon, 16 May 2022 17:29:16 GMT, Joe Darcy  wrote:

>> Make the javadoc in the InputStream and OutputStream subclasses in core libs 
>> DRY-er by use of inheritDoc. (Any analagous changes to AudioInputStream in 
>> client libs will be done another a separate bug.) When the time comes, will 
>> do any necessary merging for the changes in[JDK-8286200.
>> 
>> Please also review the CSR to cover the introduction of implspec and 
>> implNote tags: https://bugs.openjdk.java.net/browse/JDK-8286784
>
> Joe Darcy has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Add @Override's requested in review feedback.

thanks for the updates.

-

Marked as reviewed by alanb (Reviewer).

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


Re: RFR: JDK-8286783: Expand use of @inheritDoc in InputStream and OutputStream subclasses [v3]

2022-05-16 Thread Alan Bateman
On Mon, 16 May 2022 16:56:00 GMT, Joe Darcy  wrote:

> I added the `@Override` annotations to methods I reviewed and/or updated; it 
> was not necessarily an exhaustive process. 

Yeah, there are inconsistencies as a result and I think we should go the extra 
mile and add to the methods such as readAllBbytes, readNBytes that weren't 
changed.

-

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


Re: RFR: JDK-8286783: Expand use of @inheritDoc in InputStream and OutputStream subclasses

2022-05-15 Thread Alan Bateman
On Sun, 15 May 2022 18:36:07 GMT, Joe Darcy  wrote:

> Make the javadoc in the InputStream and OutputStream subclasses in core libs 
> DRY-er by use of inheritDoc. (Any analagous changes to AudioInputStream in 
> client libs will be done another a separate bug.) When the time comes, will 
> do any necessary merging for the changes in[JDK-8286200.
> 
> Please also review the CSR to cover the introduction of implspec and implNote 
> tags: https://bugs.openjdk.java.net/browse/JDK-8286784

Picking FileInputStream at random, I wonder if the `@inheritDoc` on transferTo 
is needed. Also, I think you've attempted to add `@Override` to all overridden 
methods but some may have been missed (e.g. readAllBytes and readNBytes).

-

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


Re: RFR: 8286559: Re-examine synchronization of mark and reset methods on InflaterInputStream [v2]

2022-05-13 Thread Alan Bateman
On Fri, 13 May 2022 07:14:30 GMT, Jaikiran Pai  wrote:

>> Can I please get a review of this change that addresses 
>> https://bugs.openjdk.java.net/browse/JDK-8286559? 
>> 
>> The commit here removes the `synchronized` on `mark` and `reset` methods of 
>> `InflaterInputStream`. The `mark` method is a no-op method and the `reset` 
>> method only always throws a `IOException`. So `synchronized` isn't adding 
>> any value here. 
>> 
>> Additionally, the commit does a minor change to the javadoc of these methods 
>> to use `@implNote` to describe what the implementation does. Please let me 
>> know if the `@implNote` is unnecessary, in which case, I'll revert that part.
>> 
>> This change is similar to what was recently done for `FilterInputStream` 
>> https://github.com/openjdk/jdk/pull/8309 and `PushbackInputStream` 
>> https://github.com/openjdk/jdk/pull/8433
>> 
>> tier1, tier2 and tier3 tests were run and no related failures were noticed.
>
> Jaikiran Pai has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Incorporate review comment made on CSR by Joe - Change @implNote to 
> @implSpec

Marked as reviewed by alanb (Reviewer).

-

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


Re: RFR: JDK-8286604: Update InputStream and OutputStream to use @implSpec [v2]

2022-05-13 Thread Alan Bateman
On Thu, 12 May 2022 20:04:33 GMT, Joe Darcy  wrote:

>> While doing a CSR review of another issue, I noticed some cases in 
>> InputStream and OutputStream what would benefit from being upgraded to 
>> implSpec and related javadoc tags.
>> 
>> The "A subclass must provide an implementation of this method." statements 
>> on several abstract methods don't add much value, but I chose to leave them 
>> in for this request.
>> 
>> Please also review the corresponding CSR: 
>> https://bugs.openjdk.java.net/browse/JDK-8286605
>
> Joe Darcy has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Respond to review feedback.

Marked as reviewed by alanb (Reviewer).

-

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


Re: RFR: 8286681: ShenandoahControlThread::request_gc misses the case of GCCause::_codecache_GC_threshold

2022-05-13 Thread Alan Bateman
On Fri, 13 May 2022 06:47:20 GMT, Jie Fu  wrote:

>> test/jdk/java/foreign/TestIntrinsics.java line 48:
>> 
>>> 46:  *   -XX:+UseShenandoahGC
>>> 47:  *   TestIntrinsics
>>> 48:  */
>> 
>> Is this needed? The parameters looks the same as the first test description 
>> so if you are testing with +ShenandoahGC then it will run already.
>
> Without `-XX:+UseShenandoahGC`, this bug wouldn't be exposed.
> 
> What do you mean by `if you are testing with +ShenandoahGC then it will run 
> already`?

I assume you are running the tests with:
   make run-tests TEST_OPTS_JAVA_OPTIONS="-XX:+UseShenandoahGC" 
in which case, all of the tests you select to run will be run with that GC. 

What you have is not wrong but wouldn't be a better to add a new test to 
test/hotspot/jtreg/gc rather than relying on test in test/jdk/java/foreign?

-

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


Re: RFR: 8286681: ShenandoahControlThread::request_gc misses the case of GCCause::_codecache_GC_threshold

2022-05-13 Thread Alan Bateman
On Fri, 13 May 2022 02:43:55 GMT, Jie Fu  wrote:

> Hi all,
> 
> Some tests fail with Shenandoah GC after JDK-8282191.
> The reason is that the assert in `ShenandoahControlThread::request_gc` misses 
> the case of `GCCause::_codecache_GC_threshold`.
> It would be better to fix it.
> 
> Thanks.
> Best regards,
> Jie

test/jdk/java/foreign/TestIntrinsics.java line 48:

> 46:  *   -XX:+UseShenandoahGC
> 47:  *   TestIntrinsics
> 48:  */

Is this needed? The parameters looks the same as the first test description so 
if you are testing with +ShenandoahGC then it will run already.

-

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


Re: RFR: 8286378: Address possibly lossy conversions in java.base [v3]

2022-05-12 Thread Alan Bateman
On Fri, 13 May 2022 04:41:03 GMT, ExE Boss  wrote:

>> Roger Riggs has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Updated copyrights
>>   Fixed cast style to add a space after cast, (where consistent with file 
>> style)
>>   Improved code per review comments in PollSelectors.
>
> src/java.base/linux/classes/sun/nio/ch/EPollSelectorImpl.java line 128:
> 
>> 126: // timed poll interrupted so need to adjust timeout
>> 127: long adjust = System.nanoTime() - startTime;
>> 128: to =- (int) TimeUnit.NANOSECONDS.toMillis(adjust);
> 
> This will now always assign a negative number to `to`.
> 
> 
> 
> `=-` is not a compound assignment, it’s negation followed by a normal 
> assignment.

Well spotted, I don't think that change was intentionally.

-

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


Re: RFR: 8286560: Remove user parameter from jdk.internal.perf.Perf.attach()

2022-05-12 Thread Alan Bateman
On Wed, 11 May 2022 23:08:32 GMT, Ioi Lam  wrote:

> The API `jdk.internal.perf.Perf.::attach(String user, int lvmid)` is never 
> used. It should be removed, and all the handling of a specified user name 
> should be removed.

Marked as reviewed by alanb (Reviewer).

src/java.base/share/classes/jdk/internal/perf/Perf.java line 246:

> 244:  */
> 245: private native ByteBuffer attach0(int lvmid)
> 246:throws IllegalArgumentException, IOException;

You can drop the "throws IllegalArgumentException" here if you want, it's not 
needed as it's a runtime exception.

-

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


Re: RFR: JDK-8286604: Update InputStream and OutputStream to use @implSpec

2022-05-12 Thread Alan Bateman
On Wed, 11 May 2022 20:40:30 GMT, Joe Darcy  wrote:

> While doing a CSR review of another issue, I noticed some cases in 
> InputStream and OutputStream what would benefit from being upgraded to 
> implSpec and related javadoc tags.
> 
> The "A subclass must provide an implementation of this method." statements on 
> several abstract methods don't add much value, but I chose to leave them in 
> for this request.
> 
> Please also review the corresponding CSR: 
> https://bugs.openjdk.java.net/browse/JDK-8286605

src/java.base/share/classes/java/io/InputStream.java line 177:

> 175:  *
> 176:  * @apiNote
> 177:  * A subclass must provide an implementation of this method.

Is this sentence useful to keep? The method is abstract so a concrete 
implementation has to implement it. On the other other hand, an abstract 
subclass does not need to implement it.

src/java.base/share/classes/java/io/InputStream.java line 688:

> 686:  * @implSpec
> 687:  * The {@code mark} method of {@code InputStream} does
> 688:  * nothing.

Minor nit but the line break can be removed so that "nothing" is on the same 
line.

-

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


Re: RFR: 8286378: Address possibly lossy conversions in java.base [v3]

2022-05-11 Thread Alan Bateman
On Wed, 11 May 2022 16:30:41 GMT, Roger Riggs  wrote:

>> PR#8599 8244681: proposes to add compiler warnings for possible lossy 
>> conversions
>> From the CSR:
>> 
>> "If the type of the right-hand operand of a compound assignment is not 
>> assignment compatible with the type of the variable, a cast is implied and 
>> possible lossy conversion may silently occur. While similar situations are 
>> resolved as compilation errors for primitive assignments, there are no 
>> similar rules defined for compound assignments."
>> 
>> This PR anticipates the warnings and adds explicit casts to replace the 
>> implicit casts.
>> In most cases, the cast matches the type of the right-hand side to type of 
>> the compound assignment.
>> Since these casts are truncating the value, there might be a different 
>> refactoring that avoids the cast
>> but a direct replacement was chosen here.
>> 
>> (Advise and suggestions will inform similar updates to other OpenJDK 
>> modules).
>
> Roger Riggs has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Updated copyrights
>   Fixed cast style to add a space after cast, (where consistent with file 
> style)
>   Improved code per review comments in PollSelectors.

Marked as reviewed by alanb (Reviewer).

-

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


Re: RFR: 8286441: Remove mode parameter from jdk.internal.perf.Perf.attach() [v2]

2022-05-11 Thread Alan Bateman
On Wed, 11 May 2022 02:43:21 GMT, Ioi Lam  wrote:

>> The `mode` parameter for ` jdk.internal.perf.Perf.attach()` has never 
>> supported the value `"rw"` since the source code was imported to the openjdk 
>> repo more than 15 years ago. In fact HotSpot throws 
>> `IllegalArgumentException` when such a mode is specified.
>> 
>> It's unlikely such a mode will be required for future enhancements. Support 
>> for `"rw"` is removed. The "mode" parameter is also removed, since now `"r"` 
>> is the only supported mode.
>> 
>> I also cleaned up related code in the JDK and HotSpot.
>> 
>> Testing:
>> - Passed tiers 1 and 2
>> - Tiers 3, 4, 5 are in progress
>
> Ioi Lam has updated the pull request incrementally with one additional commit 
> since the last revision:
> 
>   review comments

Marked as reviewed by alanb (Reviewer).

-

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


Re: RFR: 8286582: Build fails on macos aarch64 when using --with-zlib=bundled

2022-05-11 Thread Alan Bateman
On Wed, 11 May 2022 12:47:08 GMT, Jaikiran Pai  wrote:

>> src/java.base/share/native/libzip/zlib/gzwrite.c line 452:
>> 
>>> 450: len = strlen(next);
>>> 451: #  else
>>> 452: #   ifdef __APPLE__ // ignore format-nonliteral warning on macOS
>> 
>> Instead of patching 3rd party code to fix a compilation warning, you should 
>> disable that warning instead.
>> 
>> In `make/modules/java.base/lib/CoreLibraries.gmk`, add 
>> 
>> DISABLED_WARNINGS_clang := format-nonliteral, \
>> 
>> as line 138.
>
> Thank you for these useful inputs Magnus. I did these changes locally but for 
> some reason this format-nonliteral is not getting picked up while building 
> that library. I will investigate and see what's going on. Will update the PR 
> once I figure it out.

I agree with Magnus and try to avoid changing the imported zlib code.

-

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


Re: RFR: 8286562: GCC 12 reports some compiler warnings [v2]

2022-05-11 Thread Alan Bateman
On Wed, 11 May 2022 08:40:21 GMT, Yasumasa Suenaga  wrote:

>> I saw some compiler warnings when I tried to build OpenJDK with GCC 12.0.1 
>> on Fedora 36.
>> As you can see, the warnings spreads several areas. Let me know if I should 
>> separate them by area.
>> 
>> * -Wstringop-overflow
>> * src/hotspot/share/oops/array.hpp
>> * 
>> src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdBits.inline.hpp
>> 
>> In member function 'void Array::at_put(int, const T&) [with T = unsigned 
>> char]',
>> inlined from 'void ConstantPool::tag_at_put(int, jbyte)' at 
>> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:126:64,
>> inlined from 'void ConstantPool::method_at_put(int, int, int)' at 
>> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:380:15,
>> inlined from 'ConstantPool* 
>> BytecodeConstantPool::create_constant_pool(JavaThread*) const' at 
>> /home/ysuenaga/github-forked/jdk/src/hotspot/share/classfile/bytecodeAssembler.cpp:85:26:
>
> Yasumasa Suenaga has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Avoid pragma error in before GCC 12

src/java.base/unix/native/libjli/java_md_common.c line 135:

> 133: if ((JLI_StrLen(indir) + JLI_StrLen(cmd) + 2) > sizeof(name)) return 
> 0;
> 134: JLI_Snprintf(name, sizeof(name), "%s%c%s", indir, FILE_SEPARATOR, 
> cmd);
> 135: #pragma GCC diagnostic pop

Can we just replace this code rather than putting pragmas here?

-

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


Re: RFR: 8286444: javac errors after JDK-8251329 are not helpful enough to find root cause

2022-05-10 Thread Alan Bateman
On Tue, 10 May 2022 21:30:23 GMT, Sean Mullan  wrote:

> > It's probably ok, but the bug report is either incomplete or I am missing 
> > something. It says "This can be improved to something like: ..." but the 
> > same text as is emitted now is used. Can you fix this so I have a better 
> > example of what will be included in the message?
> 
> The bug report also says "The error message does not give a clue which jar 
> file is causing the problem" but the error message includes the name 
> "invalid.jar" so I am also confused about that.

There are two parts to it. In the case of initCEN method, the proposed change 
is to include the name of the rejected entry in the exception message. This is 
not the same thing as leaking a file path in the exception message so I don't 
think we have a concern here.

-

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


Re: RFR: 8286378: Address possibly lossy conversions in java.base [v2]

2022-05-10 Thread Alan Bateman
On Tue, 10 May 2022 23:01:33 GMT, Roger Riggs  wrote:

>> PR#8599 8244681: proposes to add compiler warnings for possible lossy 
>> conversions
>> From the CSR:
>> 
>> "If the type of the right-hand operand of a compound assignment is not 
>> assignment compatible with the type of the variable, a cast is implied and 
>> possible lossy conversion may silently occur. While similar situations are 
>> resolved as compilation errors for primitive assignments, there are no 
>> similar rules defined for compound assignments."
>> 
>> This PR anticipates the warnings and adds explicit casts to replace the 
>> implicit casts.
>> In most cases, the cast matches the type of the right-hand side to type of 
>> the compound assignment.
>> Since these casts are truncating the value, there might be a different 
>> refactoring that avoids the cast
>> but a direct replacement was chosen here.
>> 
>> (Advise and suggestions will inform similar updates to other OpenJDK 
>> modules).
>
> Roger Riggs has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   remove stray file

src/java.base/linux/classes/sun/nio/ch/EPollSelectorImpl.java line 128:

> 126: // timed poll interrupted so need to adjust timeout
> 127: long adjust = System.nanoTime() - startTime;
> 128: to -= (int)TimeUnit.MILLISECONDS.convert(adjust, 
> TimeUnit.NANOSECONDS);

Can we change this `to =- (int) TimeUnit.NANOSECONDS.toMillis(adjust);` while 
we here?

src/java.base/share/classes/jdk/internal/org/objectweb/asm/Frame.java line 615:

> 613: if (outputStackTop >= elements) {
> 614: outputStackTop -= (short)elements;
> 615: } else {

I think we usually try to avoid changing the ASM code if possible but maybe you 
have to do this because the compiler option is used for compiling java.base?

src/java.base/unix/classes/sun/nio/ch/PollSelectorImpl.java line 123:

> 121: // timed poll interrupted so need to adjust timeout
> 122: long adjust = System.nanoTime() - startTime;
> 123: to -= (int)TimeUnit.MILLISECONDS.convert(adjust, 
> TimeUnit.NANOSECONDS);

This one can also be changed to:

`to =- (int) TimeUnit.NANOSECONDS.toMillis(adjust);`

-

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


Re: RFR: 8286441: Remove mode parameter from jdk.internal.perf.Perf.attach()

2022-05-10 Thread Alan Bateman
On Tue, 10 May 2022 04:00:29 GMT, Ioi Lam  wrote:

> The `mode` parameter for ` jdk.internal.perf.Perf.attach()` has never 
> supported the value `"rw"` since the source code was imported to the openjdk 
> repo more than 15 years ago. In fact HotSpot throws 
> `IllegalArgumentException` when such a mode is specified.
> 
> It's unlikely such a mode will be required for future enhancements. Support 
> for `"rw"` is removed. The "mode" parameter is also removed, since now `"r"` 
> is the only supported mode.
> 
> I also cleaned up related code in the JDK and HotSpot.
> 
> Testing:
> - Passed tiers 1 and 2
> - Tiers 3, 4, 5 are in progress

I skimmed through the changes and I think they look okay. In the distant past 
there were tools outside of the JDK that used the jvmstat API directly. It's 
possible that VisualVM still does but it would only compile/run if 
--add-exports is used to export the sun.jvmstat.* packages. So it might be that 
dropping the parameter from a method in RemoteHost is noticed and I think that 
is okay because this package is not exported and is not meant to be used by 
code outside of the JDK.

-

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


Re: RFR: 8286441: Remove mode parameter from jdk.internal.perf.Perf.attach()

2022-05-10 Thread Alan Bateman
On Tue, 10 May 2022 04:00:29 GMT, Ioi Lam  wrote:

> The `mode` parameter for ` jdk.internal.perf.Perf.attach()` has never 
> supported the value `"rw"` since the source code was imported to the openjdk 
> repo more than 15 years ago. In fact HotSpot throws 
> `IllegalArgumentException` when such a mode is specified.
> 
> It's unlikely such a mode will be required for future enhancements. Support 
> for `"rw"` is removed. The "mode" parameter is also removed, since now `"r"` 
> is the only supported mode.
> 
> I also cleaned up related code in the JDK and HotSpot.
> 
> Testing:
> - Passed tiers 1 and 2
> - Tiers 3, 4, 5 are in progress

src/jdk.internal.jvmstat/share/classes/sun/jvmstat/perfdata/monitor/protocol/file/PerfDataBuffer.java
 line 60:

> 58: FileChannel fc = new RandomAccessFile(f, "r").getChannel();
> 59: ByteBuffer bb = fc.map(FileChannel.MapMode.READ_ONLY, 0L, 
> (int)fc.size());
> 60: fc.close();   // doesn't need to remain open

I think you can change this to:


try (FileChannel fc = FileChannel.open(f.toPath())) {
ByteBuffer bb = ...
createPerfDataBuffer(bb, 0);
}

-

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


Re: RFR: 8286444: javac errors after JDK-8251329 are not helpful enough to find root cause

2022-05-10 Thread Alan Bateman
On Tue, 10 May 2022 16:48:30 GMT, Christoph Langer  wrote:

> I think this would be OK, but would get to get someone from our security team 
> to bless it.

It's print the entry name, I don't think it is leaking the file path to the zip 
file.

-

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


Re: RFR: 8286474: Drop --enable-preview from Sealed Classes related tests [v2]

2022-05-10 Thread Alan Bateman
On Tue, 10 May 2022 14:54:39 GMT, Aleksey Shipilev  wrote:

>> There are plenty of tests failing on many architectures due to 
>> `--enable-preview` VM code introduced by Loom. This improvement eliminates 
>> some of the redundant `--enable-preview` clauses from the Sealed Classes 
>> tests, since Sealed Classes have been graduated from preview in JDK 17.
>> 
>> Additional testing:
>>  - [x] Linux x86_64 fastdebug, affected tests still pass
>>  - [x] Linux x86_32 fastdebug, affected tests start to pass
>
> Aleksey Shipilev has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Review comments

Marked as reviewed by alanb (Reviewer).

-

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


Re: RFR: 8286473: Drop --enable-preview from Record related tests [v2]

2022-05-10 Thread Alan Bateman
On Tue, 10 May 2022 14:51:52 GMT, Aleksey Shipilev  wrote:

>> There are plenty of tests failing on many architectures due to 
>> `--enable-preview` VM code introduced by Loom. This improvements eliminates 
>> some of the redundant `--enable-preview` clauses from the Record tests, 
>> since Records have been graduated from preview in JDK 16. 
>> 
>> Additional testing:
>>  - [x] Linux x86_64 fastdebug, affected tests still pass
>>  - [x]  Linux x86_32 fastdebug, affected tests start to pass
>
> Aleksey Shipilev has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Review comments

Marked as reviewed by alanb (Reviewer).

-

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


Re: RFR: 8286473: Drop --enable-preview from Record related tests

2022-05-10 Thread Alan Bateman
On Tue, 10 May 2022 12:03:09 GMT, Aleksey Shipilev  wrote:

> There are plenty of tests failing on many architectures due to 
> `--enable-preview` VM code introduced by Loom. This improvements eliminates 
> some of the redundant `--enable-preview` clauses from the Record tests, since 
> Records have been graduated from preview in JDK 16. 
> 
> Additional testing:
>  - [x] Linux x86_64 fastdebug, affected tests still pass
>  - [x]  Linux x86_32 fastdebug, affected tests start to pass

test/jdk/java/nio/Buffer/BulkPutBuffer.java line 57:

> 55:  * @run testng/othervm BulkPutBuffer
> 56:  */
> 57: public class BulkPutBuffer {

You can drop the -source option from these tests too.

-

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


Re: RFR: 8286444: javac errors after JDK-8251329 are not helpful enough to find root cause

2022-05-10 Thread Alan Bateman
On Mon, 9 May 2022 22:32:57 GMT, Christoph Langer  wrote:

> @LanceAndersen @AlanBateman do you think adding the entry name in the 
> exception in ZipFileSystem is ok? If so, should it maybe go into a different 
> patch?

It should be okay as this is the name of an entry in the zip file. It might be 
a bit cleaner to add a method to IndexNode to return the name as String. 
Alternatively maybe its toString could be changed to drop the index (I would 
need to dig into the history to find out if there is really any use for that in 
the String representation).

-

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


Re: RFR: 8286474: Drop --enable-preview from Sealed Classes related tests

2022-05-10 Thread Alan Bateman
On Tue, 10 May 2022 12:07:31 GMT, Aleksey Shipilev  wrote:

> There are plenty of tests failing on many architectures due to 
> `--enable-preview` VM code introduced by Loom. This improvement eliminates 
> some of the redundant `--enable-preview` clauses from the Sealed Classes 
> tests, since Sealed Classes have been graduated from preview in JDK 17.
> 
> Additional testing:
>  - [x] Linux x86_64 fastdebug, affected tests still pass
>  - [x] Linux x86_32 fastdebug, affected tests start to pass

test/jdk/java/lang/reflect/sealed_classes/SealedClassesReflectionTest.java line 
29:

> 27:  * @summary reflection test for sealed classes
> 28:  * @compile -source ${jdk.version} SealedClassesReflectionTest.java
> 29:  * @run testng/othervm SealedClassesReflectionTest

You should be able to drop` -source ${jdk.version}` too. It was required when 
compiling with `--enable-preview`.

-

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


Re: RFR: JDK-8285730: unify _WIN32_WINNT settings [v4]

2022-05-09 Thread Alan Bateman
On Wed, 4 May 2022 08:00:08 GMT, Matthias Baesken  wrote:

>> Currently we set _WIN32_WINNT at various places in the codebase; this is 
>> used to target a minimum Windows version we want to support. See also for 
>> more detailled information :
>> https://docs.microsoft.com/en-us/windows/win32/winprog/using-the-windows-headers?redirectedfrom=MSDN#setting-winver-or-_win32_winnt
>> Macros for Conditional Declarations
>> "Certain functions that depend on a particular version of Windows are 
>> declared using conditional code. This enables you to use the compiler to 
>> detect whether your application uses functions that are not supported on its 
>> target version(s) of Windows."
>> 
>> However currently we have quite a lot of differing settings of _WIN32_WINNT 
>> in the codebase ; setting _WIN32_WINNT to 0x0601 (Windows 7) where possible 
>> would make sense because we have this setting already at   java_props_md.c  
>> (so targeting older Windows versions at other places is most likely not 
>> useful).
>
> Matthias Baesken has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   adjust API level to Windows 8 for security.cpp and do some cleanup

Marked as reviewed by alanb (Reviewer).

-

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


Re: RFR: 8286368: Cleanup problem lists after loom integration

2022-05-09 Thread Alan Bateman
On Mon, 9 May 2022 18:17:13 GMT, Leonid Mesnik  wrote:

> 8286368: Cleanup problem lists after loom integration

Marked as reviewed by alanb (Reviewer).

-

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


Re: RFR: 8286363: BigInteger.parallelMultiply missing @since 19

2022-05-09 Thread Alan Bateman
On Mon, 9 May 2022 15:26:20 GMT, Brian Burkhalter  wrote:

> Add missing `@since 19` tag.

Marked as reviewed by alanb (Reviewer).

-

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


Re: RFR: 8286294 : ForkJoinPool.commonPool().close() spins [v5]

2022-05-09 Thread Alan Bateman
On Sun, 8 May 2022 12:08:14 GMT, Doug Lea  wrote:

>> Changes ForkJoinPool.close spec and code to trap close as a no-op if called 
>> on common pool
>
> Doug Lea has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Test improvements

Marked as reviewed by alanb (Reviewer).

-

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


Re: RFR: 8284550: test failure_handler is not properly invoking jhsdb jstack, resulting in failure to produce a stack when a test times out

2022-05-09 Thread Alan Bateman
On Sun, 8 May 2022 21:57:20 GMT, Leonid Mesnik  wrote:

> …resulting in failure to produce a stack when a test times out

Marked as reviewed by alanb (Reviewer).

-

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


Re: RFR: 8286298: Remove unused methods in sun.invoke.util.VerifyType

2022-05-08 Thread Alan Bateman
On Fri, 6 May 2022 11:32:25 GMT, Claes Redestad  wrote:

> A few untested and unused methods in `VerifyType` which can be removed. 
> (Possibly used by native JSR 292 implementations in JDK 7).

Marked as reviewed by alanb (Reviewer).

-

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


Re: RFR: JDK-6725221 Standardize obtaining boolean properties with defaults

2022-05-07 Thread Alan Bateman
On Sun, 8 May 2022 02:22:08 GMT, Phil Race  wrote:

> I did wonder why it has security-libs as the sub-category and if the intent 
> was not what we see here.

I suspect the JBS issue was initially created to to look at the usages of 
getProperty in the security code but it has been extended. The issue is that it 
changes the meaning of the empty value case in at least two places so each one 
will require careful attention.

-

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


Integrated: 8284161: Implementation of Virtual Threads (Preview)

2022-05-07 Thread Alan Bateman
On Fri, 8 Apr 2022 13:43:39 GMT, Alan Bateman  wrote:

> This is the implementation of JEP 425: Virtual Threads (Preview).
> 
> We will refresh this PR periodically to pick up changes and fixes from the 
> loom repo.
> 
> Most of the new mechanisms in the HotSpot VM are disabled by default and 
> require running with `--enable-preview` to enable.
> 
> The patch has support for x64 and aarch64 on the usual operating systems 
> (Linux, macOS, and Windows). There are stubs (calling _Unimplemented_) for 
> zero and some of the other ports. Additional ports can be contributed via PRs 
> against the fibers branch in the loom repo.
> 
> There are changes in many areas. To reduce notifications/mails, the labels 
> have been trimmed down for now to hotspot, serviceability and core-libs. We 
> can add additional labels, if required, as the PR progresses.
> 
> The changes include a refresh of java.util.concurrent and ForkJoinPool from 
> Doug Lea's CVS. These changes will probably be proposed and integrated in 
> advance of this PR.
> 
> The changes include some non-exposed and low-level infrastructure to support 
> the (in draft) JEPs for Structured Concurrency and Extent Locals. This is to 
> make life a bit easier and avoid having to separate VM changes and juggle 
> branches at this time.

This pull request has now been integrated.

Changeset: 9583e365
Author:Alan Bateman 
URL:   
https://git.openjdk.java.net/jdk/commit/9583e3657e43cc1c6f2101a64534564db2a9bd84
Stats: 99468 lines in 1133 files changed: 91198 ins; 3598 del; 4672 mod

8284161: Implementation of Virtual Threads (Preview)

Co-authored-by: Ron Pressler 
Co-authored-by: Alan Bateman 
Co-authored-by: Erik Österlund 
Co-authored-by: Andrew Haley 
Co-authored-by: Rickard Bäckman 
Co-authored-by: Markus Grönlund 
Co-authored-by: Leonid Mesnik 
Co-authored-by: Serguei Spitsyn 
Co-authored-by: Chris Plummer 
Co-authored-by: Coleen Phillimore 
Co-authored-by: Robbin Ehn 
Co-authored-by: Stefan Karlsson 
Co-authored-by: Thomas Schatzl 
Co-authored-by: Sergey Kuksenko 
Reviewed-by: lancea, eosterlund, rehn, sspitsyn, stefank, tschatzl, dfuchs, 
lmesnik, dcubed, kevinw, amenkov, dlong, mchung, psandoz, bpb, coleenp, smarks, 
egahlin, mseledtsov, coffeys, darcy

-

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


Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v14]

2022-05-07 Thread Alan Bateman
> This is the implementation of JEP 425: Virtual Threads (Preview).
> 
> We will refresh this PR periodically to pick up changes and fixes from the 
> loom repo.
> 
> Most of the new mechanisms in the HotSpot VM are disabled by default and 
> require running with `--enable-preview` to enable.
> 
> The patch has support for x64 and aarch64 on the usual operating systems 
> (Linux, macOS, and Windows). There are stubs (calling _Unimplemented_) for 
> zero and some of the other ports. Additional ports can be contributed via PRs 
> against the fibers branch in the loom repo.
> 
> There are changes in many areas. To reduce notifications/mails, the labels 
> have been trimmed down for now to hotspot, serviceability and core-libs. We 
> can add additional labels, if required, as the PR progresses.
> 
> The changes include a refresh of java.util.concurrent and ForkJoinPool from 
> Doug Lea's CVS. These changes will probably be proposed and integrated in 
> advance of this PR.
> 
> The changes include some non-exposed and low-level infrastructure to support 
> the (in draft) JEPs for Structured Concurrency and Extent Locals. This is to 
> make life a bit easier and avoid having to separate VM changes and juggle 
> branches at this time.

Alan Bateman has updated the pull request with a new target base due to a merge 
or a rebase. The pull request now contains 23 commits:

 - Refresh 4e99b5185eef9398c4cc4e90544de4a3153d61a9
 - Merge with 5212535a276a92d96ca20bdcfccfbce956febdb1
 - Refresh 6ace49bf42e5504c69fa11c564e8e865c0a95fb3
 - Merge with 9425ab2b43b649bd591706bfc820e9c8795a6fdf
 - Refresh 12aa09ce7efd48425cc080d0b8761aca0f3e215f
 - Merge with 1bb4de2e2868a539846ec48dd43fd623c2ba69a5
 - Refresh d77f7a49af75bcc5b20686805ff82a93a20dde05
 - Merge with 4b2c82200fdc01de868cf414e40a4d891e753f89
 - Refresh 6091080db743ece5f1b2111fcc35a5f2179a403a
 - Merge with cfcba1fccc8e3e6a68e1cb1826b70e076d5d83c4
 - ... and 13 more: https://git.openjdk.java.net/jdk/compare/5212535a...df43e6fc

-

Changes: https://git.openjdk.java.net/jdk/pull/8166/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8166=13
  Stats: 99468 lines in 1133 files changed: 91198 ins; 3598 del; 4672 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8166.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8166/head:pull/8166

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


Re: RFR: JDK-6725221 Standardize obtaining boolean properties with defaults

2022-05-06 Thread Alan Bateman
On Thu, 5 May 2022 16:49:07 GMT, Mark Powers  wrote:

> JDK-6725221 Standardize obtaining boolean properties with defaults

src/java.base/share/classes/java/lang/reflect/AccessibleObject.java line 777:

> 775: if (!printStackPropertiesSet && VM.initLevel() >= 1) {
> 776: printStackWhenAccessFails = GetBooleanAction.
> 777: 
> privilegedGetProperty("sun.reflect.debugModuleAccessChecks");

Running with `-Dsun.reflect.debugModuleAccessChecks` should set 
printStackWhenAccessFails to true, not false.

-

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


Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v13]

2022-05-06 Thread Alan Bateman
On Fri, 6 May 2022 06:48:46 GMT, Alan Bateman  wrote:

>> This is the implementation of JEP 425: Virtual Threads (Preview).
>> 
>> We will refresh this PR periodically to pick up changes and fixes from the 
>> loom repo.
>> 
>> Most of the new mechanisms in the HotSpot VM are disabled by default and 
>> require running with `--enable-preview` to enable.
>> 
>> The patch has support for x64 and aarch64 on the usual operating systems 
>> (Linux, macOS, and Windows). There are stubs (calling _Unimplemented_) for 
>> zero and some of the other ports. Additional ports can be contributed via 
>> PRs against the fibers branch in the loom repo.
>> 
>> There are changes in many areas. To reduce notifications/mails, the labels 
>> have been trimmed down for now to hotspot, serviceability and core-libs. We 
>> can add additional labels, if required, as the PR progresses.
>> 
>> The changes include a refresh of java.util.concurrent and ForkJoinPool from 
>> Doug Lea's CVS. These changes will probably be proposed and integrated in 
>> advance of this PR.
>> 
>> The changes include some non-exposed and low-level infrastructure to support 
>> the (in draft) JEPs for Structured Concurrency and Extent Locals. This is to 
>> make life a bit easier and avoid having to separate VM changes and juggle 
>> branches at this time.
>
> Alan Bateman has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now contains 21 commits:
> 
>  - Refresh 6ace49bf42e5504c69fa11c564e8e865c0a95fb3
>  - Merge with 9425ab2b43b649bd591706bfc820e9c8795a6fdf
>  - Refresh 12aa09ce7efd48425cc080d0b8761aca0f3e215f
>  - Merge with 1bb4de2e2868a539846ec48dd43fd623c2ba69a5
>  - Refresh d77f7a49af75bcc5b20686805ff82a93a20dde05
>  - Merge with 4b2c82200fdc01de868cf414e40a4d891e753f89
>  - Refresh 6091080db743ece5f1b2111fcc35a5f2179a403a
>  - Merge with cfcba1fccc8e3e6a68e1cb1826b70e076d5d83c4
>  - Refresh ee9fa8ed05ec22de7a13383052d68aa8aa7832ec
>  - Merge with jdk-19+20
>  - ... and 11 more: 
> https://git.openjdk.java.net/jdk/compare/9425ab2b...6d968671

> It is understandable to ship the preview feature not implemented on all 
> platforms. It is even understandable to ship the final product feature that 
> breaks some platforms in an clearly understandable way, prompting platform 
> maintainers to implement the agreed-upon, final Java feature. What I am 
> seeing, though, is that just running `java -version` (!) after integrating a 
> _preview feature_ is broken!

Preview features need to be implemented by a port before it can be released. 
For JDK 19 this means that the maintainers of ports will have work to do for 
both JEP 424 and JEP 425 (ifdef is not an option).

I think the issue that are concerned about is the interim period between the 
JEP 425 integration and the port/implementation of this feature to other 
architectures. I think the answer is that it will vary. It may be that some 
port maintainers decide to do something very short term so they can run without 
--enable-preview and buy time before they do the port/implementation for JDK 
19. Good progress has been reported on at least ppc64le port and maybe that 
port be ready before others. So yes, some ports may crash until they receive 
attention, others (like zero) should be able to run tests that don't use 
--enable-preview. I've no doubt there will be a flurry of changes post 
integration.

The motivation for Continuations::enabled() was to reduce risk and disable a 
lot of the new code by default. The motivation wasn't porting but it may be 
helpful during the interim period. That is probably a topic for loom-dev rather 
here.

-

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


Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v13]

2022-05-06 Thread Alan Bateman
> This is the implementation of JEP 425: Virtual Threads (Preview).
> 
> We will refresh this PR periodically to pick up changes and fixes from the 
> loom repo.
> 
> Most of the new mechanisms in the HotSpot VM are disabled by default and 
> require running with `--enable-preview` to enable.
> 
> The patch has support for x64 and aarch64 on the usual operating systems 
> (Linux, macOS, and Windows). There are stubs (calling _Unimplemented_) for 
> zero and some of the other ports. Additional ports can be contributed via PRs 
> against the fibers branch in the loom repo.
> 
> There are changes in many areas. To reduce notifications/mails, the labels 
> have been trimmed down for now to hotspot, serviceability and core-libs. We 
> can add additional labels, if required, as the PR progresses.
> 
> The changes include a refresh of java.util.concurrent and ForkJoinPool from 
> Doug Lea's CVS. These changes will probably be proposed and integrated in 
> advance of this PR.
> 
> The changes include some non-exposed and low-level infrastructure to support 
> the (in draft) JEPs for Structured Concurrency and Extent Locals. This is to 
> make life a bit easier and avoid having to separate VM changes and juggle 
> branches at this time.

Alan Bateman has updated the pull request with a new target base due to a merge 
or a rebase. The pull request now contains 21 commits:

 - Refresh 6ace49bf42e5504c69fa11c564e8e865c0a95fb3
 - Merge with 9425ab2b43b649bd591706bfc820e9c8795a6fdf
 - Refresh 12aa09ce7efd48425cc080d0b8761aca0f3e215f
 - Merge with 1bb4de2e2868a539846ec48dd43fd623c2ba69a5
 - Refresh d77f7a49af75bcc5b20686805ff82a93a20dde05
 - Merge with 4b2c82200fdc01de868cf414e40a4d891e753f89
 - Refresh 6091080db743ece5f1b2111fcc35a5f2179a403a
 - Merge with cfcba1fccc8e3e6a68e1cb1826b70e076d5d83c4
 - Refresh ee9fa8ed05ec22de7a13383052d68aa8aa7832ec
 - Merge with jdk-19+20
 - ... and 11 more: https://git.openjdk.java.net/jdk/compare/9425ab2b...6d968671

-

Changes: https://git.openjdk.java.net/jdk/pull/8166/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8166=12
  Stats: 99456 lines in 1130 files changed: 91188 ins; 3598 del; 4670 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8166.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8166/head:pull/8166

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


Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v12]

2022-05-05 Thread Alan Bateman
On Thu, 5 May 2022 17:43:58 GMT, Aleksey Shipilev  wrote:

> I am sorry to be a buzzkill here, but this integration would break lots of 
> platforms even when Loom functionality is not enabled/used. For example, 
> running `java -version` on RISC-V runs into many issues: 
> `TemplateInterpreterGenerator::generate_Continuation_doYield_entry` runs into 
> `Unimplemented()`, `UnsafeCopyMemory` asserts about UCM table size, 
> `NativeDeoptInstruction::is_deopt` would run into `Unimplemented()` while 
> being called from signal handler.
> 
> This effectively blocks development on those platforms, and it seems to be 
> too much breakage for a preview feature. Are we really breaking these 
> platforms? Do we have plans in place with maintainers of those platforms to 
> fix all this in JDK 19 timeframe? I'd suggest holding off the integration 
> until such a plan and agreement is in place.

We mailed porters-dev in Sep 2021 to give a heads up that this feature would 
require porting work so it shouldn't be a surprise. We have been open to 
including other ports with the initial integration but it was never a goal to 
have all the ports done in advance of that.

Most of the new code in the VM is only used when running with --enable-preview. 
You'll see several places that test Continuations::enabled(). It should be 
possible to get these port running without -enable-preview without much effort. 
The feature has to be implemented to pass all the tests of course.

-

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


Re: RFR: JDK-8285987: executing shell scripts without #! fails on Alpine linux

2022-05-05 Thread Alan Bateman
On Wed, 4 May 2022 12:04:47 GMT, Matthias Baesken  wrote:

> A couple a tests like java/lang/ProcessBuilder/Basic.java#id0.Basic_id0 and 
> jdk/jshell/ExternalEditorTest.java.ExternalEditorTest try to start small 
> shell scripts without #! at the first line of the script. This fails with 
> error=8, Exec format error when running on Alpine 3.15 .
> Looks like this is a known issue on musl / Alpine, see also
> https://www.openwall.com/lists/musl/2018/03/09/2
> and
> https://github.com/scala-steward-org/scala-steward/issues/1374
> (we see it on Alpine 3.15).

test/lib/jdk/test/lib/Platform.java line 192:

> 190: }
> 191: 
> 192: public static boolean isMusl() {

I think this will need test/lib/TestMutuallyExclusivePlatformPredicates.java to 
be updated too.

-

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


Re: RFR: 8274517: java/util/DoubleStreamSums/CompensatedSums.java fails with expected [true] but found [false]

2022-05-05 Thread Alan Bateman
On Tue, 19 Apr 2022 08:40:51 GMT, Raffaello Giulietti  
wrote:

> Please review these small changes to address intermittent failures, as of 
> JDK-8274517.
> 
> - Usage of jdk.test.lib.RandomFactory for reproducible random generation.
> - Slightly less restrictive assertion about badParallelStreamError on L94 
> (former L88).
> - Verbatim copy of computeFinalSum() from j.u.s.Collectors 18.
> 
> While these changes do not necessarily guarantee absence of intermittent 
> failures, the usage of jdk.test.lib.RandomFactory should at least help to pin 
> down specific double sequences that do not pass the test.
> 
> There is still an inherent variability due to the use of parallel streams, 
> though. As double addition is not perfectly associative, even a fully known 
> sequence of doubles may lead to slightly different results with 
> parallelization.

.

-

Marked as reviewed by alanb (Reviewer).

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


  1   2   3   4   5   6   7   8   9   10   >