RFR: 8267544: (test) rmi test NonLocalSkeleton fails if network has multiple adapters with the same address

[Roger Riggs]

Correct test code that creates a set of unique hostnames from an array of 
non-unique hostname.
Was using Set.of that throws if the elements are not unique.

-

Commit messages:
 - Correct API to get list of unique hostnames

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

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

Re: RFR: 8191441: (Process) add Readers and Writer access to java.lang.Process streams [v2]

[Roger Riggs]

> Methods are added to java.lang.Process to read and write characters and lines 
> from and to a spawned Process.
> The Charset used to encode and decode characters to bytes can be specified or 
> use the
> operating system native encoding as is available from the "native.encoding" 
> system property.

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

  Use BufferedWriter and OutputStreamWriter and address review comments

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4134/files
  - new: https://git.openjdk.java.net/jdk/pull/4134/files/a49b0b72..5a4a85d2

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

  Stats: 66 lines in 3 files changed: 19 ins; 14 del; 33 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4134.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4134/head:pull/4134

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

Re: Please, help me with Java Strings class

[-]

I personally think the annotation ones are overkill; jdk doesn't use
annotations for automatic code generation for most of its core libs, and
these annotations will decrease the readability of code.

As far as I see, most of your utility methods are bypassing nulls. Imo the
right approach in the future would be avoiding null references like the
direction of modern jdk than adding a lot of null handlers. Otherwise, new
utility methods should go to java.lang.String I think.

On Fri, May 21, 2021 at 3:27 PM Alberto Otero Rodríguez <
albest...@hotmail.com> wrote:

> Hi, I'm Alberto Otero Rodríguez,
>
> You have answered several questions I made in:
> https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-May/thread.html
>
> Could you have a look at my other question?:
> https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-May/077882.html
>
> I have proposed there creating a new java.util.Strings class and maybe
> some new annotations.
>
> I know the annotations would require too much effort, but the Strings
> class is pretty simple and I think it would be really useful.
>
> Thank you so much for your answers!
>
> Regards,
>
> Alberto.
>

Re: RFR: 8264859: Implement Context-Specific Deserialization Filters [v4]

[Brent Christian]

On Fri, 21 May 2021 17:47:53 GMT, Roger Riggs  wrote:

>> JEP 415: Context-specific Deserialization Filters extends the 
>> deserialization filtering mechanisms with more flexible and customizable 
>> protections against malicious deserialization.  See JEP 415: 
>> https://openjdk.java.net/jeps/415.
>> The `java.io.ObjectInputFilter` and `java.io.ObjectInputStream` classes are 
>> extended with additional
>> configuration mechanisms and filter utilities.
>> 
>> javadoc for `ObjectInputFilter`, `ObjectInputFilter.Config`, and 
>> `ObjectInputStream`:
>> 
>> http://cr.openjdk.java.net/~rriggs/filter-factory/java.base/java/io/ObjectInputFilter.html
>
> Roger Riggs has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Editorial updates to review comments
>   Add filter tracing support

Changes requested by bchristi (Reviewer).

src/java.base/share/classes/java/io/ObjectInputFilter.java line 84:

> 82:  *
> 83:  *  When a filter is set on an {@link ObjectInputStream}, the {@link 
> #checkInput checkInput(FilterInfo)}
> 84:  * method is called to validate classes, the length of each array,

This sounds a bit like checkInput() is called at the time that the filter is 
set.  I'd recommend something like, "If a filter is set on an 
ObjectInputStream, the checkInput() method is called during deserialization to 
validate..."

src/java.base/share/classes/java/io/ObjectInputFilter.java line 123:

> 121:  * the depth, number of references, and stream size and return a status
> 122:  * that reflects only one or only some of the values.
> 123:  * This allows a filter to specific about the choice it is reporting and

...to _**be**_ specific ?

src/java.base/share/classes/java/io/ObjectInputFilter.java line 386:

> 384:  * {@link ObjectInputStream#setObjectInputFilter(ObjectInputFilter) 
> ObjectInputStream.setObjectInputFilter}.
> 385:  * If {@code ObjectInputStream.setObjectInputFilter} is called, the 
> filter factory is called a second time
> 386:  * with the initial filter returned from the first call and the 
> requested new filter.

Maybe shorten to, "with the stream's initial filter, and the requested new 
filter."

src/java.base/share/classes/java/io/ObjectInputFilter.java line 404:

> 402:  * The JVM-wide filter is configured during the initialization of the
> 403:  * {@code ObjectInputFilter.Config} class.
> 404:  * For example, by calling {@link #getSerialFilter() 
> Config.getSerialFilter}.

Can the "For example..." sentence be removed?

src/java.base/share/classes/java/io/ObjectInputFilter.java line 423:

> 421:  * The class must be public, must have a public zero-argument 
> constructor, implement the
> 422:  * {@link BinaryOperator {@literal 
> BinaryOperator}} interface, provide its implementation and
> 423:  * be accessible via the {@linkplain 
> ClassLoader#getSystemClassLoader() the application class loader}.

two "the"s

src/java.base/share/classes/java/io/ObjectInputFilter.java line 1258:

> 1256: /**
> 1257:  * Apply the filter and return the status if not UNDECIDED 
> and checking a class.
> 1258:  * Make an exception for Primitive classes that are 
> implicitly allowed by the pattern based filte.

filte -> filter

src/java.base/share/classes/java/io/ObjectInputStream.java line 200:

> 198:  * and every object read from the stream can be checked.
> 199:  * The {@linkplain #ObjectInputStream() ObjectInputStream constructors} 
> invoke the filter factory
> 200:  * to select the initial filter and it is updated by {@link 
> #setObjectInputFilter}.

In ObjectInputFilter, the `"The deserialization filter for a stream is 
determined in one of the following ways"` section has a good description of the 
various pieces and how they work together to determine a stream's filter.  I 
would consider leaning on that to provide the details, and have a shorter, 
high-level description here.  For instance:

"Each stream has an optional deserialization filter to check the classes and 
resource limits during deserialization.  The filter can be set for all streams 
JVM-wide, or customized on a per-stream basis.  See `` 
for details on how the filter for a stream is determined.
ObjectInputFilter describes how to use filters and..."

src/java.base/share/classes/java/io/ObjectInputStream.java line 370:

> 368:  *
> 369:  * The serialization filter is initialized to the filter returned
> 370:  * by invoking the {@link Config#getSerialFilterFactory()} with 
> {@code null} for the current filter

`linkplain` to "JVM-wide filter factory" ?

src/java.base/share/classes/java/io/ObjectInputStream.java line 371:

> 369:  * The serialization filter is initialized to the filter returned
> 370:  * by invoking the {@link Config#getSerialFilterFactory()} with 
> {@code null} for the current filter
> 371:  * and {@linkplain  Config#getSerialFilter() static JVM-wide filter} 
> for the requ

Re: RFR: 8263561: Re-examine uses of LinkedList

[Maurizio Cimadamore]

On Fri, 21 May 2021 14:22:47 GMT, Daniel Fuchs  wrote:

>> The usage of `LinkedList` is senseless and can be replaced with either 
>> `ArrayList` or `ArrayDeque` which are both more compact and effective.
>> 
>> jdk:tier1 and jdk:tier2 are both ok
>
> I don't remember all the comments you have received in this thread but have 
> you verified that arbitrarily changing `LinkedList` into `ArrayList` in these 
> classes is not going to significantly increase the footprint in the case 
> where lists are empty or contain only one or two elements?
> 
> I am not convinced that a global replacement of  `LinkedList` by `ArrayList` 
> is necessarily good - even though I agree that `ArrayList` is generally more 
> efficient.

I second the footprint concerns from @dfuch. I've seen with first hand cases 
where widespread uses of array lists for holding 1-2-3 elements (e.g. think of 
a graph where each node might only have a very small number of outgoing edges) 
lead to massive memory over-utilization. If the average size is known, at the 
very least I'd argue to replace with an array list which is sized correctly.

And, all this said, the general assumption implied in this PR which linked 
lists are just to be avoided doesn't match my experience. If you want a "pay 
only as much memory as you use" data structure, you don't care about random 
access (e.g. all accesses are linear walks), a linked list is a perfectly fine 
choice. If there are use cases in the JDK where a LinkedList is used in places 
where it shouldn't be, then obviously _those cases_ should be replaced.

-

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

Re: RFR: 8266310: deadlock while loading the JNI code [v3]

[Jorn Vernee]

On Fri, 21 May 2021 15:39:33 GMT, Aleksei Voitylov  
wrote:

>> src/java.base/share/classes/jdk/internal/loader/NativeLibraries.java line 
>> 262:
>> 
>>> 260: } finally {
>>> 261: releaseNativeLibraryLock(name);
>>> 262: }
>> 
>> The new locking scheme looks incorrect to me. It now seems possible for 2 
>> different class loaders in 2 different threads to load the same library 
>> (which should not be possible).
>> 
>> Thread 1:
>>   - acquires name lock
>>   - checks library name is already in `loadedLibraryNames` (it's not)
>>   - release name lock
>>   - start loading library
>>   
>> Then thread 2 comes in and does the same
>>   
>> Then again thread 1 finishes loading and:
>>  - acquires name lock
>>  - puts library name in `loadedLibraryNames`
>>  - puts library name in it's own `libraries`
>>  - release lock
>>  
>> Then thread 2 comes in and does the same again (although adding the name to 
>> `loadedLibraryNames` will silently return `false`).
>> 
>> It seems that this scenario is possible, and the library will be loaded by 2 
>> different class loaders, each with their own `lib` object. (If there's a 
>> race, there has to be a loser as well, which would need to discard their 
>> work when finding out they lost)
>> 
>> You might be able to stress this by checking if 
>> `loadedLibraryNames.add(name);` returns `true`, and throwing an exception if 
>> not.
>> 
>> I think a possible solution would be to claim the library name up front for 
>> a particular loader. Then 2 threads can still race to load the same library 
>> for the same class loader, but 2 threads with 2 different class loaders 
>> racing to load the library should not be possible.
>> 
>> Actually, you might be able to prevent a race on JNI_OnLoad altogether by 
>> claiming the library name for a particular thread upfront as well (e.g. 
>> using a `ConcurrentHashMap`).
>
> Thank you, the previous version of the PR suffered from other problems as 
> well. 
> 
> The PR is now reverted to a scheme with a lock held on library name, thus 
> such a situation is no longer possible.

Ok, thanks.

FWIW, I think trying to lock on a per-name basis instead of globally is a good 
idea, and should improve the current situation. It is also safe to do as far as 
I can see.

-

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

Re: RFR: 8191441: (Process) add Readers and Writer access to java.lang.Process streams

[Roger Riggs]

Hi,

Yes, I'm testing BufferedWriter-> OutputStreamWriter now.
And it can be wrapped in PrintWriter if someone is eager to get the 
missing text formatting

or auto-flush behaviors.

Roger


On 5/21/21 5:23 PM, Rémi Forax wrote:

On Thu, 20 May 2021 20:37:57 GMT, Roger Riggs  wrote:


OutputStreamWriter would be a better choice with that in mind. It does not have 
the convenience methods for converting various types to strings but would not 
hide the exceptions. Developers could wrap it in a PrintWriter to get the 
convenience and take on the responsibility of dealing with exceptions by 
polling.

yes, instead of OutputStreamWriter, i wonder if BufferedWriter is not better 
given that reader part uses BufferedReader and that is mirror 
java.nio.file.Files which also uses BufferedReader/BufferedWriter as types for 
the pair reader/writer.

-

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

Re: RFR: 8191441: (Process) add Readers and Writer access to java.lang.Process streams

[Rémi Forax]

On Thu, 20 May 2021 20:37:57 GMT, Roger Riggs  wrote:

> OutputStreamWriter would be a better choice with that in mind. It does not 
> have the convenience methods for converting various types to strings but 
> would not hide the exceptions. Developers could wrap it in a PrintWriter to 
> get the convenience and take on the responsibility of dealing with exceptions 
> by polling.

yes, instead of OutputStreamWriter, i wonder if BufferedWriter is not better 
given that reader part uses BufferedReader and that is mirror 
java.nio.file.Files which also uses BufferedReader/BufferedWriter as types for 
the pair reader/writer.

-

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

Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]

[Weijun Wang]

On Fri, 21 May 2021 20:43:05 GMT, Phil Race  wrote:

> I haven't seen any response to this comment I made a couple of days ago and I 
> suspect it got missed
> 
> > Weijun Wang has updated the pull request incrementally with one additional 
> > commit since the last revision:
> > fixing awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java
> 
> test/jdk/java/awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java line 59:
> 
> > 57: ProcessCommunicator
> > 58: .executeChildProcess(Consumer.class, new 
> > String[0]);
> > 59: if (!"Hello".equals(processResults.getStdOut())) {
> 
> Who or what prompted this change ?

I replied right in the thread but unfortunately GitHub does not display it at 
the end of page.

This is because the process is now launched with 
`-Djava.security.manager=allow` (because of another change in 
https://github.com/openjdk/jdk/pull/4071), and a new warning is displayed in 
stderr. Therefore I switched to stdout.

-

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

Re: RFR: 8191441: (Process) add Readers and Writer access to java.lang.Process streams

[Roger Riggs]

Hi,

The list is used to test the inputReader and errorReader methods that 
accept a Charset.


The child process is launched with -Dsun.stdout.encoding and 
-Dsun.stderr.encoding
to make the child encode its output in the requested charset (overriding 
the native encoding).

Then the parent reads the output with the same charset.
The test will be skipped if the encoding is not supported.

The test for the native charset uses only the native encoding in the 
default configuration.


Thanks, Roger

On 5/20/21 5:30 PM, Bernd Eckenfels wrote:

Hello,

Hm, how is that list used? - StandardCharaet.ISO_8859_1 is a guaranteed Charset 
for JVM, and since the encoding is done in Java it should be fine. Added 
benefit is, it’s 8bit transparent.

As for OS there is not a single standard charset (ebcdic vs latin families) but 
ASCII is probably the widest available (with latin1 variants to follow)


--
https://Bernd.eckenfels.net

From: core-libs-dev  on behalf of Roger Riggs 

Sent: Thursday, May 20, 2021 10:52 PM
To: core-libs-dev@openjdk.java.net
Subject: Re: RFR: 8191441: (Process) add Readers and Writer access to 
java.lang.Process streams

On Thu, 20 May 2021 20:42:35 GMT, Naoto Sato  wrote:


Methods are added to java.lang.Process to read and write characters and lines 
from and to a spawned Process.
The Charset used to encode and decode characters to bytes can be specified or 
use the
operating system native encoding as is available from the "native.encoding" 
system property.

test/jdk/java/lang/ProcessBuilder/ReaderWriterTest.java line 64:


62: return new Object[][] {
63: {"UTF-8"},
64: {"ISO8859-1"},

`ISO8859-1` may not be available on all underlying OSes.

Is there a safe subset?
I haven't seen a failure yet, if/when it occurs, we make an exception or narrow 
the test to known systems.


test/jdk/java/lang/ProcessBuilder/ReaderWriterTest.java line 111:


109: @Test(dataProvider = "CharsetCases", enabled = true)
110: void testCase(String nativeEncoding) throws IOException {
111: String osName = 
System.getProperty("os.name").toLowerCase(Locale.ROOT);

Not used anywhere else.

Right, dead code now without host dependencies.


test/jdk/java/lang/ProcessBuilder/ReaderWriterTest.java line 122:


120: "ReaderWriterTest$ChildWithCharset");
121: var env = pb.environment();
122: env.put("LANG", "en_US." + cleanCSName);

Does this work on Windows?

Should be removed, the tests work because they set sun.stdout/stderr.encoding.

-

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

Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]

[Phil Race]

On Wed, 19 May 2021 13:47:53 GMT, Weijun Wang  wrote:

>> Please review this implementation of [JEP 
>> 411](https://openjdk.java.net/jeps/411).
>> 
>> The code change is divided into 3 commits. Please review them one by one.
>> 
>> 1. 
>> https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1
>>  The essential change for this JEP, including the `@Deprecate` annotations 
>> and spec change. It also update the default value of the 
>> `java.security.manager` system property to "disallow", and necessary test 
>> change following this update.
>> 2. 
>> https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66
>>  Manual changes to several files so that the next commit can be generated 
>> programatically.
>> 3. 
>> https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950
>>  Automatic changes to other source files to avoid javac warnings on 
>> deprecation for removal
>> 
>> The 1st and 2nd commits should be reviewed carefully. The 3rd one is 
>> generated programmatically, see the comment below for more details. If you 
>> are only interested in a portion of the 3rd commit and would like to review 
>> it as a separate file, please comment here and I'll generate an individual 
>> webrev.
>> 
>> Due to the size of this PR, no attempt is made to update copyright years for 
>> any file to minimize unnecessary merge conflict.
>> 
>> Furthermore, since the default value of `java.security.manager` system 
>> property is now "disallow", most of the tests calling 
>> `System.setSecurityManager()` need to launched with 
>> `-Djava.security.manager=allow`. This is covered in a different PR at 
>> https://github.com/openjdk/jdk/pull/4071.
>> 
>> Update: the deprecation annotations and javadoc tags, build, compiler, 
>> core-libs, hotspot, i18n, jmx, net, nio, security, and serviceability are 
>> reviewed. Rest are 2d, awt, beans, sound, and swing.
>
> Weijun Wang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   fixing awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java

I haven't seen any response to this comment I made a couple of days ago and I 
suspect it got missed
>
> Weijun Wang has updated the pull request incrementally with one additional 
> commit since the last revision:
>
>   fixing awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java

test/jdk/java/awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java line 59:

> 57: ProcessCommunicator
> 58: .executeChildProcess(Consumer.class, new 
> String[0]);
> 59: if (!"Hello".equals(processResults.getStdOut())) {

Who or what prompted this change ?

-

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

Re: RFR: 8267521: Post JEP 411 refactoring: maximum covering > 50K [v3]

[Weijun Wang]

> The code change refactors classes that have a `SuppressWarnings("removal")` 
> annotation that covers more than 50KB of code. The big annotation is often 
> quite faraway from the actual deprecated API usage it is suppressing, and 
> with the annotation covering too big a portion it's easy to call other 
> deprecated methods without being noticed.
> 
> The code change shows some common solutions to avoid such too wide 
> annotations:
> 
> 1. Extract calls into a method and add annotation on that method
> 2. Assign the return value of a deprecated method call to a new local 
> variable and add annotation on the declaration, and then assign that value to 
> the original l-value.
> 3. Put declaration and assignment into a single statement if possible.
> 4. Rewrite code to achieve #3 above.

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

  update FtpClient.java

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4138/files
  - new: https://git.openjdk.java.net/jdk/pull/4138/files/d460efb8..60d97a4c

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

  Stats: 23 lines in 1 file changed: 0 ins; 12 del; 11 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4138.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4138/head:pull/4138

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

Re: RFR: 8267521: Post JEP 411 refactoring: maximum covering > 50K [v2]

[Weijun Wang]

On Fri, 21 May 2021 15:37:48 GMT, Daniel Fuchs  wrote:

>> Weijun Wang has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   typo on windows
>
> src/java.base/share/classes/sun/net/ftp/impl/FtpClient.java line 550:
> 
>> 548:  * @throws IOException if the connection was unsuccessful.
>> 549:  */
>> 550: @SuppressWarnings("removal")
> 
> Could the scope of the annotation be further reduced by applying it to the 
> two places where `doPrivileged` is called in this method?

I'll probably need to assign the doPriv result on L631 to a tmp variable and 
then assign it to `s`. Are you OK with this ugliness? Update: Ah, I see you 
already have similar suggestion in the next comment.

-

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

Re: RFR: 8267521: Post JEP 411 refactoring: maximum covering > 50K [v2]

[Weijun Wang]

On Fri, 21 May 2021 15:35:05 GMT, Daniel Fuchs  wrote:

>> Weijun Wang has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   typo on windows
>
> src/java.base/share/classes/sun/net/ftp/impl/FtpClient.java line 120:
> 
>> 118: vals[1] = 
>> Integer.getInteger("sun.net.client.defaultConnectTimeout", 
>> 300_000).intValue();
>> 119: return System.getProperty("file.encoding", 
>> "ISO8859_1");
>> 120: }
> 
> It is a bit strange that "file.encoding" seem to get a special treatment - 
> but I guess that's OK.

You might say we thus avoid wasting the return value, as much as possible.

-

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

Re: RFR: 8266310: deadlock while loading the JNI code [v2]

[Aleksei Voitylov]

Peter, David,

I updated the PR to focus on solving the problem I originally intended
to solve that is observed in the wild - a deadlock when two different
libraries are being loaded. The case when the same library JNI_OnLoad
has FindClass with a circular dependency on loading the same library is
artificial. I thought of two ways to resolve it with the following
conclusions:

- not holding a lock when JNI_OnLoad is executed can only be done in a
such a way that would lead to a possibility to get JNI_OnLoad executed
multiple times, and the JNI code can rely on the fact that it's called
only once (though the JNI spec does not say so). Decoupling
dlopen/LoadLibrary from JNI_OnLoad does not solve this problem as well.
- throwing something like LibraryBeingLoadedError similar to
ClassCircularityError for JNI calls trying to use a class which
initializes the same library is also not a good option, as it is
behavior change. I as a user would not know what to do with that
(remember it can appear for a different thread that just happened to be
initializing the same library as well). Such a thread should just sit
and wait on the lock.

Both such solutions would make innocent code suffer, so are a no-go. I'm
open to ideas how to resolve it, if at all, but IMO it has a different
priority compared to the original bug report. I thus focused on solving
the original problem.

Thanks,

-Aleksei

On 21/05/2021 21:10, Peter Levart wrote:
>
> On 21/05/2021 10:29, Peter Levart wrote:
>> I still haven't found a scenario of a possible deadlock when Sergei's
>> initial patch is combined with...
>
>
> Sorry Aleksei, I renamed you to Sergei without intention. Please
> excuse me.
>
>
> Peter
>
>

Re: RFR: 8267184: Add -Djava.security.manager=allow to tests calling System.setSecurityManager [v3]

[Weijun Wang]

> Please review the test changes for [JEP 
> 411](https://openjdk.java.net/jeps/411).
> 
> With JEP 411 and the default value of `-Djava.security.manager` becoming 
> `disallow`, tests calling `System.setSecurityManager()`  need 
> `-Djava.security.manager=allow` when launched. This PR covers such changes 
> for tier1 to tier3 (except for the JCK tests).
> 
> To make it easier to focus your review on the tests in your area, this PR is 
> divided into multiple commits for different areas (~~serviceability~~, 
> ~~hotspot-compiler~~, ~~i18n~~, ~~rmi~~, ~~javadoc~~, swing, 2d, 
> ~~security~~, ~~hotspot-runtime~~, ~~nio~~, ~~xml~~, ~~beans~~, 
> ~~core-libs~~, ~~net~~, ~~compiler~~, ~~hotspot-gc~~). Mostly the rule is the 
> same as how Skara adds labels, but there are several small tweaks:
> 
> 1. When a file is covered by multiple labels, only one is chosen. I make my 
> best to avoid putting too many tests into `core-libs`. If a file is covered 
> by `core-libs` and another label, I categorized it into the other label.
> 2. If a file is in `core-libs` but contains `/xml/` in its name, it's in the 
> `xml` commit.
> 3. If a file is in `core-libs` but contains `/rmi/` in its name, it's in the 
> `rmi` commit.
> 4. One file not covered by any label -- 
> `test/jdk/com/sun/java/accessibility/util/8051626/Bug8051626.java` -- is in 
> the `swing` commit.
> 
> Due to the size of this PR, no attempt is made to update copyright years for 
> all files to minimize unnecessary merge conflict.
> 
> Please note that this PR can be integrated before the source changes for JEP 
> 411, as the possible values of this system property was already defined long 
> time ago in JDK 9.
> 
> Most of the change in this PR is a simple adding of 
> `-Djava.security.manager=allow` to the `@run main/othervm` line. Sometimes it 
> was not `othervm` and we add one. Sometimes there's no `@run` at all and we 
> add the line.
> 
> There are several tests that launch another Java process that needs to call 
> the `System.setSecurityManager()` method, and the system property is added to 
> `ProcessBuilder`, `ProcessTools`, or the java command line (if the test is a 
> shell test).
> 
> 3 langtools tests are added into problem list due to 
> [JDK-8265611](https://bugs.openjdk.java.net/browse/JDK-8265611).
> 
> 2 SQL tests are moved because they need different options on the `@run` line 
> but they are inside a directory that has a `TEST.properties`:
> 
> rename test/jdk/java/sql/{testng/test/sql/othervm => 
> permissionTests}/DriverManagerPermissionsTests.java (93%)
> rename test/jdk/javax/sql/{testng/test/rowset/spi => 
> permissionTests}/SyncFactoryPermissionsTests.java (95%)
>  ```
> 
> The source change for JEP 411 is at https://github.com/openjdk/jdk/pull/4073.

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

  feedback from Phil
  
  reverted:

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4071/files
  - new: https://git.openjdk.java.net/jdk/pull/4071/files/bfa955ad..9a3ec578

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

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

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

Re: RFR: 8267184: JEP 411: Add -Djava.security.manager=allow to tests calling System.setSecurityManager [v2]

[Weijun Wang]

On Fri, 21 May 2021 18:26:48 GMT, Phil Race  wrote:

>> Weijun Wang has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   adjust order of VM options
>
> test/jdk/java/awt/FontClass/CreateFont/fileaccess/FontFile.java line 3:
> 
>> 1: /*
>> 2:  * Copyright (c) 2008, 2021, Oracle and/or its affiliates. All rights 
>> reserved.
>> 3:  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
> 
> Probably the (c) update was meant to be on the .sh file that is actually 
> modified.

Oops, I'll back it out. Thanks.

-

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

Re: Builder pattern for Java records

[-]

As far as I see, builders are more useful when there are too many arguments
or a lot of them have default values. These needs aren't as significant as
on records where few components are inferred.

In addition, using builders may lead to extra object allocation and
increase distribution size for the extra bytecode. So I guess people can
use an annotation processor to generate record builders optionally than
mandating a builder type for every record.

On Friday, May 21, 2021, Alberto Otero Rodríguez 
wrote:
> Hi, I have found this project on GitHub which creates a Builder for Java
records:
> https://github.com/Randgalt/record-builder
> [
https://opengraph.githubassets.com/a4e3a7b3c7b16b51e0854011fe4b031577bcc09919058baef412b03613295d20/Randgalt/record-builder
]
> GitHub - Randgalt/record-builder: Record builder generator for Java
records
> The target package for generation is the same as the package that
contains the "Include" annotation. Use packagePattern to change this (see
Javadoc for details).. Usage Maven. Add the dependency that contains the
@RecordBuilder annotation. < dependency > < groupId
>io.soabase.record-builder < artifactId >record-builder-core < version >set-version-here github.com
>
> And I was wondering if this could be made by default in all Java Records.
>
> Technically a Builder is only used when creating the Object so I think is
possible and, if Java creates it by default, would be less error prone than
creating it manually.
>
> Regards,
>
> Alberto.
>

Java records used in enums

[-]

Thanks for the info Alberto!

In my opinion, your best bet would be using an enum map to track these
information. Unfortunately, java enums and records are both static and you
should indeed avoid modifying them.

As a result, making enums deeply unmodifiable (immutable) is a good
practice, like your suggestion. Yet there are other uses of enums, such as
singletons, in which people may still want mutable instance fields. Lazy
field initialization are possible usages, too.

Best!

On Friday, May 21, 2021, Alberto Otero Rodríguez 
wrote:
> The problem in my case was that we have an enum ErrorEnum with several
fields.
> One of the fields was "description" which have a getter and a setter.
> The problem appeared when testing with JUnit. Basically the logic of the
application was working correctly but the descriptions of the errors of
different tests were overriden with the description of the errors of
another tests.
> The problem was solved by making the fields of the enums final and
removing all the setter methods (after trying a lot of things and
refactoring the application almost entirely to remove all the static
methods and static members we had in the application).
> Regards,
> Alberto.
> 
> De: core-libs-dev  en nombre de
liangchenb...@gmail.com 
> Enviado: viernes, 21 de mayo de 2021 17:15
> Para: core-libs-dev@openjdk.java.net 
> Asunto: Fwd: Java records used in enums
>
> -- Forwarded message -
> From: - 
> Date: Fri, May 21, 2021 at 10:14 AM
> Subject: Re: Java records used in enums
> To: Alberto Otero Rodríguez 
>
>
> I fail to understand what you want. How does making enums records fix the
> multithreading issue?
>
> If you wish to have enums declared like records code-wise, I see it as a
> possibility, but I don't see how it affects any enum behavior in any way.
>
> If you want a combination of record and enum, or a record with enumerated
> possible instances, it may be possible to add an alternative syntax for
> such enum declaration; but restricting all enums to be such enum records
> offers no benefit that I can see, especially given records themselves have
> severe restrictions (must have an accessible canonical ctor available;
that
> ctor cannot throw checked exceptions; field/getter method name limits, so
a
> "enum record" cannot declare fields like "values" "name"); also records
are
> value-based while enums are identity based, which is another problem.
>
> In short, I believe regular enum suffices and a "record enum" offers
little
> benefit while it cannot fix your issue at all.
>
> On Fri, May 21, 2021 at 10:00 AM Alberto Otero Rodríguez <
> albest...@hotmail.com> wrote:
>
>> It seems a non-existent problem until you face it, which is what happened
>> me today.
>>
>> I think at least (supposing enums can't be records) all the fields in
>> enums should be made final by default.
>>
>> Regards,
>>
>> Alberto.
>> 
>> De: Kasper Nielsen 
>> Enviado: viernes, 21 de mayo de 2021 16:28
>> Para: Alberto Otero Rodríguez 
>> Cc: core-libs-dev@openjdk.java.net 
>> Asunto: Re: Java records used in enums
>>
>> On Fri, 21 May 2021 at 14:51, Alberto Otero Rodríguez <
>> albest...@hotmail.com> wrote:
>> Hi,
>>
>> I think enums in Java should be immutable. When you let the programmer
>> change values in an enum instance, an unexpected behaviour can happen
when
>> using multiple threads as enum instances are static (singleton).
>>
>> So, I was wondering why not make enums instances be defined as records
>> instead of normal classes.
>>
>> Lots of reasons, for example:
>> * It would be a breaking change for an almost non-existent problem.
>> * All internal state of enums would now be public.
>> * Enum's extend java.lang.Enum, records extend java.lang.Record.
>>   java.lang.Enum cannot extend java.lang.Record because it has instance
>> fields.
>>
>> /Kasper
>>
>

Re: RFR: 8224158: assertion related to NPE at DynamicCallSiteDesc::withArgs should be reworded

[Vicente Romero]

On Mon, 17 May 2021 16:53:24 GMT, Vicente Romero  wrote:

> This is a very small patch that is just rewording the spec for 
> DynamicCallSiteDesc::withArgs. Basically adding that NPE can also be thrown 
> if the content of the argument is `null`
> 
> TIA for the review

any reviewers for this simple PR?

-

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

Re: RFR: 8267184: JEP 411: Add -Djava.security.manager=allow to tests calling System.setSecurityManager [v2]

[Phil Race]

On Tue, 18 May 2021 21:44:43 GMT, Weijun Wang  wrote:

>> Please review the test changes for [JEP 
>> 411](https://openjdk.java.net/jeps/411).
>> 
>> With JEP 411 and the default value of `-Djava.security.manager` becoming 
>> `disallow`, tests calling `System.setSecurityManager()`  need 
>> `-Djava.security.manager=allow` when launched. This PR covers such changes 
>> for tier1 to tier3 (except for the JCK tests).
>> 
>> To make it easier to focus your review on the tests in your area, this PR is 
>> divided into multiple commits for different areas (~~serviceability~~, 
>> ~~hotspot-compiler~~, ~~i18n~~, ~~rmi~~, ~~javadoc~~, swing, 2d, 
>> ~~security~~, ~~hotspot-runtime~~, ~~nio~~, ~~xml~~, ~~beans~~, 
>> ~~core-libs~~, ~~net~~, ~~compiler~~, ~~hotspot-gc~~). Mostly the rule is 
>> the same as how Skara adds labels, but there are several small tweaks:
>> 
>> 1. When a file is covered by multiple labels, only one is chosen. I make my 
>> best to avoid putting too many tests into `core-libs`. If a file is covered 
>> by `core-libs` and another label, I categorized it into the other label.
>> 2. If a file is in `core-libs` but contains `/xml/` in its name, it's in the 
>> `xml` commit.
>> 3. If a file is in `core-libs` but contains `/rmi/` in its name, it's in the 
>> `rmi` commit.
>> 4. One file not covered by any label -- 
>> `test/jdk/com/sun/java/accessibility/util/8051626/Bug8051626.java` -- is in 
>> the `swing` commit.
>> 
>> Due to the size of this PR, no attempt is made to update copyright years for 
>> all files to minimize unnecessary merge conflict.
>> 
>> Please note that this PR can be integrated before the source changes for JEP 
>> 411, as the possible values of this system property was already defined long 
>> time ago in JDK 9.
>> 
>> Most of the change in this PR is a simple adding of 
>> `-Djava.security.manager=allow` to the `@run main/othervm` line. Sometimes 
>> it was not `othervm` and we add one. Sometimes there's no `@run` at all and 
>> we add the line.
>> 
>> There are several tests that launch another Java process that needs to call 
>> the `System.setSecurityManager()` method, and the system property is added 
>> to `ProcessBuilder`, `ProcessTools`, or the java command line (if the test 
>> is a shell test).
>> 
>> 3 langtools tests are added into problem list due to 
>> [JDK-8265611](https://bugs.openjdk.java.net/browse/JDK-8265611).
>> 
>> 2 SQL tests are moved because they need different options on the `@run` line 
>> but they are inside a directory that has a `TEST.properties`:
>> 
>> rename test/jdk/java/sql/{testng/test/sql/othervm => 
>> permissionTests}/DriverManagerPermissionsTests.java (93%)
>> rename test/jdk/javax/sql/{testng/test/rowset/spi => 
>> permissionTests}/SyncFactoryPermissionsTests.java (95%)
>>  ```
>> 
>> The source change for JEP 411 is at https://github.com/openjdk/jdk/pull/4073.
>
> Weijun Wang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   adjust order of VM options

The client changes are fine except for the one misplaced (c)

test/jdk/java/awt/FontClass/CreateFont/fileaccess/FontFile.java line 3:

> 1: /*
> 2:  * Copyright (c) 2008, 2021, Oracle and/or its affiliates. All rights 
> reserved.
> 3:  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.

Probably the (c) update was meant to be on the .sh file that is actually 
modified.

-

Marked as reviewed by prr (Reviewer).

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

Integrated: 8267190: Optimize Vector API test operations

[Sandhya Viswanathan]

On Fri, 14 May 2021 23:58:38 GMT, Sandhya Viswanathan 
 wrote:

> Vector API test operations (IS_DEFAULT, IS_FINITE, IS_INFINITE, IS_NAN and 
> IS_NEGATIVE) are computed in three steps:
> 1) reinterpreting the floating point vectors as integral vectors (int/long)
> 2) perform the test in integer domain to get a int/long mask
> 3) reinterpret the int/long mask as float/double mask
> Step 3) currently is very slow. It can be optimized by modifying the Java 
> code to utilize the existing reinterpret intrinsic.
> 
> For the VectorTestPerf attached to the JBS for JDK-8267190, the performance 
> improves as follows:
> 
> Base:
> Benchmark (size) Mode Cnt Score Error Units
> VectorTestPerf.IS_DEFAULT 1024 thrpt 5 223.156 ± 90.452 ops/ms
> VectorTestPerf.IS_FINITE 1024 thrpt 5 223.841 ± 91.685 ops/ms
> VectorTestPerf.IS_INFINITE 1024 thrpt 5 224.561 ± 83.890 ops/ms
> VectorTestPerf.IS_NAN 1024 thrpt 5 223.777 ± 70.629 ops/ms
> VectorTestPerf.IS_NEGATIVE 1024 thrpt 5 218.392 ± 79.806 ops/ms
> 
> With patch:
> Benchmark (size) Mode Cnt Score Error Units
> VectorTestPerf.IS_DEFAULT 1024 thrpt 5 8812.357 ± 40.477 ops/ms
> VectorTestPerf.IS_FINITE 1024 thrpt 5 7425.739 ± 296.622 ops/ms
> VectorTestPerf.IS_INFINITE 1024 thrpt 5 8932.730 ± 269.988 ops/ms
> VectorTestPerf.IS_NAN 1024 thrpt 5 8574.872 ± 498.649 ops/ms
> VectorTestPerf.IS_NEGATIVE 1024 thrpt 5 8838.400 ± 11.849 ops/ms
> 
> Best Regards,
> Sandhya

This pull request has now been integrated.

Changeset: 8f10c5a8
Author:Sandhya Viswanathan 
URL:   
https://git.openjdk.java.net/jdk/commit/8f10c5a8900517cfa04256eab909e18535086b98
Stats: 1274 lines in 32 files changed: 652 ins; 279 del; 343 mod

8267190: Optimize Vector API test operations

Reviewed-by: psandoz, kvn

-

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

Re: RFR: 8266310: deadlock while loading the JNI code [v2]

[Peter Levart]

On 21/05/2021 10:29, Peter Levart wrote:
I still haven't found a scenario of a possible deadlock when Sergei's 
initial patch is combined with...



Sorry Aleksei, I renamed you to Sergei without intention. Please excuse me.


Peter

Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]

[Phil Race]

On Thu, 20 May 2021 07:06:00 GMT, Alan Bateman  wrote:

>> The JEP isn't PTT for 17 so there's plenty of time isn't there ?
>
> There are 827 files in this patch. Phil is right that adding SW at the class 
> level is introducing technical debt but if addressing that requires 
> refactoring and re-testing of AWT or font code then it would be better to 
> have someone from that area working on. Is there any reason why this can't be 
> going on now on awt-dev/2d-dev and integrated immediately after JEP 411? I 
> don't think we should put Max through the wringer here as there are too many 
> areas to cover.

Are you suggesting that the patch doesn't need testing as it is ? It should be 
the same either way.
It is very straight forward to run the automated tests across all platforms 
these days.
I get the impression that no one is guaranteeing to do this straight after 
integration.
It sounds like it is up for deferral if time runs out.

The amount of follow-on work that I am hearing about here, and may be for 
tests, does not  make it sound
like this JEP is nearly as done as first presented.

If there was some expectation that groups responsible for an area would get 
involved with this
issue which I am assured was already known about, then why was it not raised 
before and made
part of the plan ?

-

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

Re: RFR: 8264859: Implement Context-Specific Deserialization Filters [v4]

[Roger Riggs]

On Fri, 21 May 2021 17:47:53 GMT, Roger Riggs  wrote:

>> JEP 415: Context-specific Deserialization Filters extends the 
>> deserialization filtering mechanisms with more flexible and customizable 
>> protections against malicious deserialization.  See JEP 415: 
>> https://openjdk.java.net/jeps/415.
>> The `java.io.ObjectInputFilter` and `java.io.ObjectInputStream` classes are 
>> extended with additional
>> configuration mechanisms and filter utilities.
>> 
>> javadoc for `ObjectInputFilter`, `ObjectInputFilter.Config`, and 
>> `ObjectInputStream`:
>> 
>> http://cr.openjdk.java.net/~rriggs/filter-factory/java.base/java/io/ObjectInputFilter.html
>
> Roger Riggs has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Editorial updates to review comments
>   Add filter tracing support

To avoid spamming folks with individual emails, I marked as 'resolved' comments 
that didn't need a response other than 'ok, will fix'.  Feel free to re-open 
them or re-comment if you don't see the fix. 
Thanks for the comments.

-

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

Re: RFR: 8264859: Implement Context-Specific Deserialization Filters [v3]

[Roger Riggs]

On Thu, 20 May 2021 18:59:58 GMT, Daniel Fuchs  wrote:

>> Roger Riggs has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Simplify factory interface to BinaryOperator and 
>> cleanup the example
>
> src/java.base/share/classes/java/io/ObjectInputFilter.java line 1079:
> 
>> 1077: }
>> 1078: // There are no classes to check and none of the 
>> limits have been exceeded.
>> 1079: return Status.ALLOWED;
> 
> Should this be addressed outside of this JEP and pushed separately?

Reverted, it might be a useful change but has compatibility concerns.

-

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

Re: RFR: 8264859: Implement Context-Specific Deserialization Filters [v4]

[Roger Riggs]

> JEP 415: Context-specific Deserialization Filters extends the deserialization 
> filtering mechanisms with more flexible and customizable protections against 
> malicious deserialization.  See JEP 415: https://openjdk.java.net/jeps/415.
> The `java.io.ObjectInputFilter` and `java.io.ObjectInputStream` classes are 
> extended with additional
> configuration mechanisms and filter utilities.
> 
> javadoc for `ObjectInputFilter`, `ObjectInputFilter.Config`, and 
> `ObjectInputStream`:
> 
> http://cr.openjdk.java.net/~rriggs/filter-factory/java.base/java/io/ObjectInputFilter.html

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

  Editorial updates to review comments
  Add filter tracing support

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3996/files
  - new: https://git.openjdk.java.net/jdk/pull/3996/files/f1c5cd85..a0785e88

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

  Stats: 160 lines in 3 files changed: 69 ins; 21 del; 70 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3996.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3996/head:pull/3996

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

Re: RFR: 8264859: Implement Context-Specific Deserialization Filters [v3]

[Roger Riggs]

On Fri, 21 May 2021 16:25:58 GMT, Chris Hegarty  wrote:

>> Roger Riggs has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Simplify factory interface to BinaryOperator and 
>> cleanup the example
>
> src/java.base/share/classes/java/io/ObjectInputFilter.java line 559:
> 
>> 557:  * Returns the static JVM-wide deserialization filter or {@code 
>> null} if not configured.
>> 558:  *
>> 559:  * @return the static JVM-wide deserialization filter or {@code 
>> null} if not configured
> 
> Is "static" significant here? Can it be dropped?   I fine myself questioning 
> if the "static JVM-wide" and "JVM-wide" are two different filters. If we do 
> this then we have just two terms: 1) the "JVM-wide deserialization filter" 
> and 2) the "JVM-wide deserialization filter factory".
> 
> Additionally, can you please check all occurrence of these, to ensure that 
> they are used consistently in all parts of the spec. I think I see 
> serial/serialization (without the "de" ) used in a few places.

The static is intended to distinguish that single filter from the others.  The 
static vs current distinction is part of JEP 290 from which this evolved.  The 
migration to "de-serialization" from the previous "serialization" is as yet 
incomplete.

-

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

Re: RFR: 8264859: Implement Context-Specific Deserialization Filters [v3]

[Roger Riggs]

On Fri, 21 May 2021 16:09:45 GMT, Chris Hegarty  wrote:

>> Roger Riggs has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Simplify factory interface to BinaryOperator and 
>> cleanup the example
>
> src/java.base/share/classes/java/io/ObjectInputFilter.java line 107:
> 
>> 105:  * Note that the filter may be used directly or combined with other 
>> filters by the
>> 106:  * {@linkplain Config#setSerialFilterFactory(BinaryOperator) 
>> JVM-wide filter factory}.
>> 107:  * 
> 
> This list is a little confusing to me, but could be that I don't fully grok 
> it yet. getSerialFilterFactory returns THE JVM-wide factory, whether that be 
> the built-in factory implementation or not is somewhat irrelevant. No need to 
> treat them differently - one either sets a factory (in which case that is the 
> JVM-wide factory), or one does not set the factory (in which case the 
> built-in factory is the JVM-wide factory).

In previous versions, calling OIS.setObjectInputFilter determined exactly the 
filter used for the stream.
With the filter factory enhancement, the current filter factory determines how 
the argument to OIS.setObjectInputFilter is used. There are plenty of cases 
where the filter factory will combine it with other filters and the composite 
will becomes the filter for the stream.

-

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

Re: RFR: 8264859: Implement Context-Specific Deserialization Filters [v3]

[Roger Riggs]

On Fri, 21 May 2021 16:05:59 GMT, Chris Hegarty  wrote:

>> Roger Riggs has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Simplify factory interface to BinaryOperator and 
>> cleanup the example
>
> src/java.base/share/classes/java/io/ObjectInputFilter.java line 75:
> 
>> 73:  * protect individual contexts by providing a custom filter for each. 
>> When the stream
>> 74:  * is constructed, the filter factory can identify the execution context 
>> based upon
>> 75:  * the current thread-local state, hierarchy of callers, library, 
>> module, and class loader.
> 
> This list looks like it is enumerating what the filter factory does, but it 
> is just an example of what could be done. Maybe that needs to be made more 
> explicit. ".. the filter factory can identify execution context based upon, 
> say, ... whatever it likes .. system nanoTime, ... " Or just add "e.g."

More verbiage  from the JEP.  Adding 'for example' or 'including' reinforces 
the 'can'.

-

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

Re: RFR: 8264859: Implement Context-Specific Deserialization Filters [v3]

[Roger Riggs]

On Fri, 21 May 2021 15:58:15 GMT, Chris Hegarty  wrote:

>> Roger Riggs has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Simplify factory interface to BinaryOperator and 
>> cleanup the example
>
> src/java.base/share/classes/java/io/ObjectInputFilter.java line 56:
> 
>> 54:  * 
>> 55:  *
>> 56:  * To protect the JVM against deserialization vulnerabilities, 
>> application developers
> 
> I would personally drop "the JVM", leaving "To protect against 
> deserialization ..", since the protection is applicable to more than the JVM 
> ( applications, libraries, etc).

There's a terminology push and pull about what to call everything in the java 
runtime.
Some use the term system, a few use JVM, etc.  The terminology here came from 
the JEP.
In this context is it unnecessary.

-

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

Re: RFR: 8264859: Implement Context-Specific Deserialization Filters [v3]

[Roger Riggs]

On Fri, 21 May 2021 15:54:50 GMT, Chris Hegarty  wrote:

>> Roger Riggs has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Simplify factory interface to BinaryOperator and 
>> cleanup the example
>
> src/java.base/share/classes/java/io/ObjectInputFilter.java line 365:
> 
>> 363:  * A utility class to set and get the JVM-wide deserialization 
>> filter factory,
>> 364:  * the static JVM-wide filter, or to create a filter from a pattern 
>> string.
>> 365:  * If a JVM-wide filter factory or static JVM-wide filter is set, 
>> it will determine the filter
> 
> This concerns me, "A JVM-wide filter factory". I was going to suggest that it 
> should be "The ..", but then realised that there can only ever be one present 
> at a time, but in the lifetime of a JVM there can be two (since 
> getSerialFilterFactory if invoked before setSerialFilterFactory will 
> subsequently return a different JVM-wide factory).   Is this intentional? It 
> would great if this could be "The ..", so that setXXX can only be invoked 
> successfully if getXXX has not been.   This may seen somewhat insignificant, 
> but the fact that the JVM-wide factory can change make the model harder 
> understand.

It is reasonable to require that the factory be set before any OIS is 
constructed.
Similar to the restriction that the filter on a stream cannot be changed after 
the first call to readObject.
So an IllegalStateException added to Config.setSerialFilterFactory.

-

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

Re: RFR: 8265130: Make ConstantDesc class hierarchy sealed [v4]

[Mandy Chung]

On Fri, 21 May 2021 08:53:51 GMT, Gavin Bierman  wrote:

>> Hi all,
>> 
>> Please review this patch to make the ConstantDesc hierarchy `sealed`, as was 
>> promised in its Javadoc, now that sealed classes are finalising in JDK 17. 
>> 
>> Thanks,
>> Gavin
>
> Gavin Bierman has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Reordering class modifiers

Marked as reviewed by mchung (Reviewer).

-

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

Re: RFR: 8264859: Implement Context-Specific Deserialization Filters [v3]

[Roger Riggs]

On Thu, 20 May 2021 19:11:34 GMT, Daniel Fuchs  wrote:

>> Roger Riggs has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Simplify factory interface to BinaryOperator and 
>> cleanup the example
>
> src/java.base/share/classes/java/io/ObjectInputFilter.java line 1270:
> 
>> 1268:  *   is not {@code null} and is not the JVM-wide filter
>> 1269:  * @throws SecurityException if there is security manager 
>> and the
>> 1270:  *   {@code SerializablePermission("serialFilter")} is 
>> not granted
> 
> Where is this thrown? I don't see it in the implementation of `apply` below.

The throws clause is not needed, it is thrown in OIS.setObjectInputFilter.

-

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

Re: RFR: 8264859: Implement Context-Specific Deserialization Filters [v3]

[Roger Riggs]

On Thu, 20 May 2021 19:04:25 GMT, Daniel Fuchs  wrote:

>> Roger Riggs has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Simplify factory interface to BinaryOperator and 
>> cleanup the example
>
> src/java.base/share/classes/java/io/ObjectInputFilter.java line 1139:
> 
>> 1137:  * and not classes.
>> 1138:  */
>> 1139: private static class AllowMaxLimitsFilter implements 
>> ObjectInputFilter {
> 
> This class is maybe misnamed. If limitCheck == REJECTED it will not allow max 
> limits. Or am I missing something?

Rejection always wins in the larger scheme of things; another filter may reject 
based on other limits.
In the composition of filters, any UNDECIDED results must eventually be decided.
This filter maps, for a limit check, the UNDECIDED to allowed; it does nothing 
for checks for classes.
Other names considered,  allowUnlimited().  Also, not guaranteed.
Perhaps, something in the xxxElseYyy family.  Will reconsider the name.

-

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

Re: RFR: 8264859: Implement Context-Specific Deserialization Filters [v3]

[Chris Hegarty]

On Thu, 20 May 2021 16:10:11 GMT, Roger Riggs  wrote:

>> JEP 415: Context-specific Deserialization Filters extends the 
>> deserialization filtering mechanisms with more flexible and customizable 
>> protections against malicious deserialization.  See JEP 415: 
>> https://openjdk.java.net/jeps/415.
>> The `java.io.ObjectInputFilter` and `java.io.ObjectInputStream` classes are 
>> extended with additional
>> configuration mechanisms and filter utilities.
>> 
>> javadoc for `ObjectInputFilter`, `ObjectInputFilter.Config`, and 
>> `ObjectInputStream`:
>> 
>> http://cr.openjdk.java.net/~rriggs/filter-factory/java.base/java/io/ObjectInputFilter.html
>
> Roger Riggs has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Simplify factory interface to BinaryOperator and cleanup 
> the example

src/java.base/share/classes/java/io/ObjectInputFilter.java line 559:

> 557:  * Returns the static JVM-wide deserialization filter or {@code 
> null} if not configured.
> 558:  *
> 559:  * @return the static JVM-wide deserialization filter or {@code 
> null} if not configured

Is "static" significant here? Can it be dropped?   I fine myself questioning if 
the "static JVM-wide" and "JVM-wide" are two different filters. If we do this 
then we have just two terms: 1) the "JVM-wide deserialization filter" and 2) 
the "JVM-wide deserialization filter factory".

Additionally, can you please check all occurrence of these, to ensure that they 
are used consistently in all parts of the spec. I think I see 
serial/serialization (without the "de" ) used in a few places.

-

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

Re: RFR: 8264859: Implement Context-Specific Deserialization Filters [v3]

[Chris Hegarty]

On Thu, 20 May 2021 16:10:11 GMT, Roger Riggs  wrote:

>> JEP 415: Context-specific Deserialization Filters extends the 
>> deserialization filtering mechanisms with more flexible and customizable 
>> protections against malicious deserialization.  See JEP 415: 
>> https://openjdk.java.net/jeps/415.
>> The `java.io.ObjectInputFilter` and `java.io.ObjectInputStream` classes are 
>> extended with additional
>> configuration mechanisms and filter utilities.
>> 
>> javadoc for `ObjectInputFilter`, `ObjectInputFilter.Config`, and 
>> `ObjectInputStream`:
>> 
>> http://cr.openjdk.java.net/~rriggs/filter-factory/java.base/java/io/ObjectInputFilter.html
>
> Roger Riggs has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Simplify factory interface to BinaryOperator and cleanup 
> the example

src/java.base/share/classes/java/io/ObjectInputFilter.java line 56:

> 54:  * 
> 55:  *
> 56:  * To protect the JVM against deserialization vulnerabilities, 
> application developers

I would personally drop "the JVM", leaving "To protect against deserialization 
..", since the protection is applicable to more than the JVM ( applications, 
libraries, etc).

src/java.base/share/classes/java/io/ObjectInputFilter.java line 58:

> 56:  * To protect the JVM against deserialization vulnerabilities, 
> application developers
> 57:  * need a clear description of the objects that can be serialized or 
> deserialized
> 58:  * by each component or library. For each context and use case, 
> developers should

drop "serialized or" - filtering is not relevant to serializing.

src/java.base/share/classes/java/io/ObjectInputFilter.java line 75:

> 73:  * protect individual contexts by providing a custom filter for each. 
> When the stream
> 74:  * is constructed, the filter factory can identify the execution context 
> based upon
> 75:  * the current thread-local state, hierarchy of callers, library, module, 
> and class loader.

This list looks like it is enumerating what the filter factory does, but it is 
just an example of what could be done. Maybe that needs to be made more 
explicit. ".. the filter factory can identify execution context based upon, 
say, ... whatever it likes .. system nanoTime, ... " Or just add "e.g."

src/java.base/share/classes/java/io/ObjectInputFilter.java line 107:

> 105:  * Note that the filter may be used directly or combined with other 
> filters by the
> 106:  * {@linkplain Config#setSerialFilterFactory(BinaryOperator) 
> JVM-wide filter factory}.
> 107:  * 

This list is a little confusing to me, but could be that I don't fully grok it 
yet. getSerialFilterFactory returns THE JVM-wide factory, whether that be the 
built-in factory implementation or not is somewhat irrelevant. No need to treat 
them differently - one either sets a factory (in which case that is the 
JVM-wide factory), or one does not set the factory (in which case the built-in 
factory is the JVM-wide factory).

-

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

Re: RFR: 8264859: Implement Context-Specific Deserialization Filters [v3]

[Chris Hegarty]

On Fri, 21 May 2021 03:02:43 GMT, Brent Christian  wrote:

>> Roger Riggs has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Simplify factory interface to BinaryOperator and 
>> cleanup the example
>
> src/java.base/share/classes/java/io/ObjectInputStream.java line 193:
> 
>> 191:  * the state.
>> 192:  *
>> 193:  * The deserialization filter for a stream is determined in one of the 
>> following ways:
> 
> Should this line start with a `` ?

At this point in the OIS spec it would be good to introduce the idea of a 
stream-specific filter. Maybe open this paragraph with some additional context, 
like say:


 *  An {@code ObjectInputStream} has an optional stream-specific
 * deserialization filter. The stream-specific deserialization filter
 * is determined in one of the following ways:


then enumerate the ways in which the stream-specific deserialization filter is 
determine, and drop how the filter factory can be set, etc. That is best left 
to the Config.

-

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

Re: RFR: 8264859: Implement Context-Specific Deserialization Filters [v3]

[Chris Hegarty]

On Thu, 20 May 2021 16:10:11 GMT, Roger Riggs  wrote:

>> JEP 415: Context-specific Deserialization Filters extends the 
>> deserialization filtering mechanisms with more flexible and customizable 
>> protections against malicious deserialization.  See JEP 415: 
>> https://openjdk.java.net/jeps/415.
>> The `java.io.ObjectInputFilter` and `java.io.ObjectInputStream` classes are 
>> extended with additional
>> configuration mechanisms and filter utilities.
>> 
>> javadoc for `ObjectInputFilter`, `ObjectInputFilter.Config`, and 
>> `ObjectInputStream`:
>> 
>> http://cr.openjdk.java.net/~rriggs/filter-factory/java.base/java/io/ObjectInputFilter.html
>
> Roger Riggs has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Simplify factory interface to BinaryOperator and cleanup 
> the example

src/java.base/share/classes/java/io/ObjectInputFilter.java line 365:

> 363:  * A utility class to set and get the JVM-wide deserialization 
> filter factory,
> 364:  * the static JVM-wide filter, or to create a filter from a pattern 
> string.
> 365:  * If a JVM-wide filter factory or static JVM-wide filter is set, it 
> will determine the filter

This concerns me, "A JVM-wide filter factory". I was going to suggest that it 
should be "The ..", but then realised that there can only ever be one present 
at a time, but in the lifetime of a JVM there can be two (since 
getSerialFilterFactory if invoked before setSerialFilterFactory will 
subsequently return a different JVM-wide factory).   Is this intentional? It 
would great if this could be "The ..", so that setXXX can only be invoked 
successfully if getXXX has not been.   This may seen somewhat insignificant, 
but the fact that the JVM-wide factory can change make the model harder 
understand.

-

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

Re: RFR: 8266310: deadlock while loading the JNI code [v5]

[Aleksei Voitylov]

On Fri, 21 May 2021 15:49:09 GMT, Aleksei Voitylov  
wrote:

>> Please review this PR which fixes the deadlock in ClassLoader between the 
>> two lock objects - a lock object associated with the class being loaded, and 
>> the ClassLoader.loadedLibraryNames hash map, locked during the native 
>> library load operation.
>> 
>> Problem being fixed:
>> 
>> The initial reproducer demonstrated a deadlock between the JarFile/ZipFile 
>> and the hash map. That deadlock exists even when the ZipFile/JarFile lock is 
>> removed because there's another lock object in the class loader, associated 
>> with the name of the class being loaded. Such objects are stored in 
>> ClassLoader.parallelLockMap. The deadlock occurs when JNI_OnLoad() loads 
>> exactly the same class, whose signature is being verified in another thread.
>> 
>> Proposed fix:
>> 
>> The proposed patch suggests to get rid of locking loadedLibraryNames hash 
>> map and synchronize on each entry name, as it's done with class names in see 
>> ClassLoader.getClassLoadingLock(name) method.
>> 
>> The patch introduces nativeLibraryLockMap which holds the lock objects for 
>> each library name, and the getNativeLibraryLock() private method is used to 
>> lazily initialize the corresponding lock object. nativeLibraryContext was 
>> changed to ThreadLocal, so that in any concurrent thread it would have a 
>> NativeLibrary object on top of the stack, that's being currently 
>> loaded/unloaded in that thread. nativeLibraryLockMap accumulates the names 
>> of all native libraries loaded - in line with class loading code, it is not 
>> explicitly cleared.
>> 
>> Testing:  jtreg and jck testing with no regressions. A new regression test 
>> was developed.
>
> Aleksei Voitylov has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   fix whitespace

The PR was updated as follows:

- the fix resolves the deadlock problem when different libraries are loaded. 
- the lock on the library name is held just like in the first version of PR to 
avoid simultaneous JNI_OnLoad calls. 
- the test now checks that JNI_Onload and JNI_OnUnload are executed only once.

Within the current limitations it is not possible to resolve the deadlock when 
a library being loaded attempts, in JNI_OnLoad,  to load a class that depends 
on the same library. It is not the intent of this PR to do so.

-

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

Re: RFR: 8267521: Post JEP 411 refactoring: maximum covering > 50K [v2]

[Daniel Fuchs]

On Fri, 21 May 2021 14:00:39 GMT, Weijun Wang  wrote:

>> The code change refactors classes that have a `SuppressWarnings("removal")` 
>> annotation that covers more than 50KB of code. The big annotation is often 
>> quite faraway from the actual deprecated API usage it is suppressing, and 
>> with the annotation covering too big a portion it's easy to call other 
>> deprecated methods without being noticed.
>> 
>> The code change shows some common solutions to avoid such too wide 
>> annotations:
>> 
>> 1. Extract calls into a method and add annotation on that method
>> 2. Assign the return value of a deprecated method call to a new local 
>> variable and add annotation on the declaration, and then assign that value 
>> to the original l-value.
>> 3. Put declaration and assignment into a single statement if possible.
>> 4. Rewrite code to achieve #3 above.
>
> Weijun Wang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   typo on windows

src/java.base/share/classes/sun/net/ftp/impl/FtpClient.java line 120:

> 118: vals[1] = 
> Integer.getInteger("sun.net.client.defaultConnectTimeout", 
> 300_000).intValue();
> 119: return System.getProperty("file.encoding", 
> "ISO8859_1");
> 120: }

It is a bit strange that "file.encoding" seem to get a special treatment - but 
I guess that's OK.

src/java.base/share/classes/sun/net/ftp/impl/FtpClient.java line 550:

> 548:  * @throws IOException if the connection was unsuccessful.
> 549:  */
> 550: @SuppressWarnings("removal")

Could the scope of the annotation be further reduced by applying it to the two 
places where `doPrivileged` is called in this method?

src/java.base/share/classes/sun/net/ftp/impl/FtpClient.java line 921:

> 919: }
> 920: 
> 921: @SuppressWarnings("removal")

Could the scope be further reduced by applying it at line 926 in this method - 
at the cost of creating a temporary variable? The code could probably be 
further simplified by using a lambda...


PrivilegedAction pa = () -> new Socket(proxy);
@SuppressWarnings("removal")
var ps = AccessController.doPrivileged(pa);
s = ps;

-

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

Re: RFR: 8266310: deadlock while loading the JNI code [v5]

[Aleksei Voitylov]

> Please review this PR which fixes the deadlock in ClassLoader between the two 
> lock objects - a lock object associated with the class being loaded, and the 
> ClassLoader.loadedLibraryNames hash map, locked during the native library 
> load operation.
> 
> Problem being fixed:
> 
> The initial reproducer demonstrated a deadlock between the JarFile/ZipFile 
> and the hash map. That deadlock exists even when the ZipFile/JarFile lock is 
> removed because there's another lock object in the class loader, associated 
> with the name of the class being loaded. Such objects are stored in 
> ClassLoader.parallelLockMap. The deadlock occurs when JNI_OnLoad() loads 
> exactly the same class, whose signature is being verified in another thread.
> 
> Proposed fix:
> 
> The proposed patch suggests to get rid of locking loadedLibraryNames hash map 
> and synchronize on each entry name, as it's done with class names in see 
> ClassLoader.getClassLoadingLock(name) method.
> 
> The patch introduces nativeLibraryLockMap which holds the lock objects for 
> each library name, and the getNativeLibraryLock() private method is used to 
> lazily initialize the corresponding lock object. nativeLibraryContext was 
> changed to ThreadLocal, so that in any concurrent thread it would have a 
> NativeLibrary object on top of the stack, that's being currently 
> loaded/unloaded in that thread. nativeLibraryLockMap accumulates the names of 
> all native libraries loaded - in line with class loading code, it is not 
> explicitly cleared.
> 
> Testing:  jtreg and jck testing with no regressions. A new regression test 
> was developed.

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

  fix whitespace

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3976/files
  - new: https://git.openjdk.java.net/jdk/pull/3976/files/5e62b308..8e735117

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

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

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

Re: RFR: 8266310: deadlock while loading the JNI code [v4]

[Aleksei Voitylov]

> Please review this PR which fixes the deadlock in ClassLoader between the two 
> lock objects - a lock object associated with the class being loaded, and the 
> ClassLoader.loadedLibraryNames hash map, locked during the native library 
> load operation.
> 
> Problem being fixed:
> 
> The initial reproducer demonstrated a deadlock between the JarFile/ZipFile 
> and the hash map. That deadlock exists even when the ZipFile/JarFile lock is 
> removed because there's another lock object in the class loader, associated 
> with the name of the class being loaded. Such objects are stored in 
> ClassLoader.parallelLockMap. The deadlock occurs when JNI_OnLoad() loads 
> exactly the same class, whose signature is being verified in another thread.
> 
> Proposed fix:
> 
> The proposed patch suggests to get rid of locking loadedLibraryNames hash map 
> and synchronize on each entry name, as it's done with class names in see 
> ClassLoader.getClassLoadingLock(name) method.
> 
> The patch introduces nativeLibraryLockMap which holds the lock objects for 
> each library name, and the getNativeLibraryLock() private method is used to 
> lazily initialize the corresponding lock object. nativeLibraryContext was 
> changed to ThreadLocal, so that in any concurrent thread it would have a 
> NativeLibrary object on top of the stack, that's being currently 
> loaded/unloaded in that thread. nativeLibraryLockMap accumulates the names of 
> all native libraries loaded - in line with class loading code, it is not 
> explicitly cleared.
> 
> Testing:  jtreg and jck testing with no regressions. A new regression test 
> was developed.

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

  revert to a version with lock on library name while fixing all other comments

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3976/files
  - new: https://git.openjdk.java.net/jdk/pull/3976/files/8f47d890..5e62b308

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

  Stats: 119 lines in 3 files changed: 43 ins; 22 del; 54 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3976.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3976/head:pull/3976

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

Re: RFR: 8266310: deadlock while loading the JNI code [v3]

[Aleksei Voitylov]

On Wed, 19 May 2021 18:47:52 GMT, Jorn Vernee  wrote:

>> Aleksei Voitylov has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   fix trailing whitespace
>
> src/java.base/share/classes/jdk/internal/loader/NativeLibraries.java line 262:
> 
>> 260: } finally {
>> 261: releaseNativeLibraryLock(name);
>> 262: }
> 
> The new locking scheme looks incorrect to me. It now seems possible for 2 
> different class loaders in 2 different threads to load the same library 
> (which should not be possible).
> 
> Thread 1:
>   - acquires name lock
>   - checks library name is already in `loadedLibraryNames` (it's not)
>   - release name lock
>   - start loading library
>   
> Then thread 2 comes in and does the same
>   
> Then again thread 1 finishes loading and:
>  - acquires name lock
>  - puts library name in `loadedLibraryNames`
>  - puts library name in it's own `libraries`
>  - release lock
>  
> Then thread 2 comes in and does the same again (although adding the name to 
> `loadedLibraryNames` will silently return `false`).
> 
> It seems that this scenario is possible, and the library will be loaded by 2 
> different class loaders, each with their own `lib` object. (If there's a 
> race, there has to be a loser as well, which would need to discard their work 
> when finding out they lost)
> 
> You might be able to stress this by checking if 
> `loadedLibraryNames.add(name);` returns `true`, and throwing an exception if 
> not.
> 
> I think a possible solution would be to claim the library name up front for a 
> particular loader. Then 2 threads can still race to load the same library for 
> the same class loader, but 2 threads with 2 different class loaders racing to 
> load the library should not be possible.
> 
> Actually, you might be able to prevent a race on JNI_OnLoad altogether by 
> claiming the library name for a particular thread upfront as well (e.g. using 
> a `ConcurrentHashMap`).

Thank you, the previous version of the PR suffered from other problems as well. 

The PR is now reverted to a scheme with a lock held on library name, thus such 
a situation is no longer possible.

-

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

Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]

[Daniel Fuchs]

On Wed, 19 May 2021 13:47:53 GMT, Weijun Wang  wrote:

>> Please review this implementation of [JEP 
>> 411](https://openjdk.java.net/jeps/411).
>> 
>> The code change is divided into 3 commits. Please review them one by one.
>> 
>> 1. 
>> https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1
>>  The essential change for this JEP, including the `@Deprecate` annotations 
>> and spec change. It also update the default value of the 
>> `java.security.manager` system property to "disallow", and necessary test 
>> change following this update.
>> 2. 
>> https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66
>>  Manual changes to several files so that the next commit can be generated 
>> programatically.
>> 3. 
>> https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950
>>  Automatic changes to other source files to avoid javac warnings on 
>> deprecation for removal
>> 
>> The 1st and 2nd commits should be reviewed carefully. The 3rd one is 
>> generated programmatically, see the comment below for more details. If you 
>> are only interested in a portion of the 3rd commit and would like to review 
>> it as a separate file, please comment here and I'll generate an individual 
>> webrev.
>> 
>> Due to the size of this PR, no attempt is made to update copyright years for 
>> any file to minimize unnecessary merge conflict.
>> 
>> Furthermore, since the default value of `java.security.manager` system 
>> property is now "disallow", most of the tests calling 
>> `System.setSecurityManager()` need to launched with 
>> `-Djava.security.manager=allow`. This is covered in a different PR at 
>> https://github.com/openjdk/jdk/pull/4071.
>> 
>> Update: the deprecation annotations and javadoc tags, build, compiler, 
>> core-libs, hotspot, i18n, jmx, net, nio, security, and serviceability are 
>> reviewed. Rest are 2d, awt, beans, sound, and swing.
>
> Weijun Wang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   fixing awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java

src/java.base/share/classes/java/lang/SecurityManager.java line 104:

> 102:  * method will throw an {@code UnsupportedOperationException}). If the
> 103:  * {@systemProperty java.security.manager} system property is set to the
> 104:  * special token "{@code allow}", then a security manager will not be 
> set at

Can/should the `{@systemProperty ...}` tag be used more than once for a given 
system property? I thought it should be used only once, at the place where the 
system property is defined. Maybe @jonathan-gibbons can offer some more 
guidance on this.

-

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

Builder pattern for Java records

[Alberto Otero Rodríguez]

Hi, I have found this project on GitHub which creates a Builder for Java 
records:
https://github.com/Randgalt/record-builder
[https://opengraph.githubassets.com/a4e3a7b3c7b16b51e0854011fe4b031577bcc09919058baef412b03613295d20/Randgalt/record-builder]
GitHub - Randgalt/record-builder: Record builder generator for Java 
records
The target package for generation is the same as the package that contains the 
"Include" annotation. Use packagePattern to change this (see Javadoc for 
details).. Usage Maven. Add the dependency that contains the @RecordBuilder 
annotation. < dependency > < groupId >io.soabase.record-builder < 
artifactId >record-builder-core < version >set-version-here

Re: RFR: 8263087: Add a MethodHandle combinator that switches over a set of MethodHandles [v2]

[Jorn Vernee]

On Mon, 17 May 2021 17:19:16 GMT, Jorn Vernee  wrote:

>> This patch adds a `tableSwitch` combinator that can be used to switch over a 
>> set of method handles given an index, with a fallback in case the index is 
>> out of bounds, much like the `tableswitch` bytecode. Here is a description 
>> of how it works (copied from the javadoc):
>> 
>>  Creates a table switch method handle, which can be used to switch over 
>> a set of target
>>  method handles, based on a given target index, called selector.
>> 
>>  For a selector value of {@code n}, where {@code n} falls in the range 
>> {@code [0, N)},
>>  and where {@code N} is the number of target method handles, the table 
>> switch method
>>  handle will invoke the n-th target method handle from the list of 
>> target method handles.
>> 
>>  For a selector value that does not fall in the range {@code [0, N)}, 
>> the table switch
>>  method handle will invoke the given fallback method handle.
>> 
>>  All method handles passed to this method must have the same type, with 
>> the additional
>>  requirement that the leading parameter be of type {@code int}. The 
>> leading parameter
>>  represents the selector.
>> 
>>  Any trailing parameters present in the type will appear on the returned 
>> table switch
>>  method handle as well. Any arguments assigned to these parameters will 
>> be forwarded,
>>  together with the selector value, to the selected method handle when 
>> invoking it.
>> 
>> The combinator does not support specifying the starting index, so the switch 
>> cases always run from 0 to however many target handles are specified. A 
>> starting index can be added manually with another combination step that 
>> filters the input index by adding or subtracting a constant from it, which 
>> does not affect performance. One of the reasons for not supporting a 
>> starting index is that it allows for more lambda form sharing, but also 
>> simplifies the implementation somewhat. I guess an open question is if a 
>> convenience overload should be added for that case?
>> 
>> Lookup switch can also be simulated by filtering the input through an 
>> injection function that translates it into a case index, which has also 
>> proven to have the ability to have comparable performance to, or even better 
>> performance than, a bytecode-native `lookupswitch` instruction. I plan to 
>> add such an injection function to the runtime libraries in the future as 
>> well. Maybe at that point it could be evaluated if it's worth it to add a 
>> lookup switch combinator as well, but I don't see an immediate need to 
>> include it in this patch.
>> 
>> The current bytecode intrinsification generates a call for each switch case, 
>> which guarantees full inlining of the target method handles. Alternatively 
>> we could only have 1 callsite at the end of the switch, where each case just 
>> loads the target method handle, but currently this does not allow for 
>> inlining of the handles, since they are not constant.
>> 
>> Maybe a future C2 optimization could look at the receiver input for 
>> invokeBasic call sites, and if the input is a phi node, clone the call for 
>> each constant input of the phi. I believe that would allow simplifying the 
>> bytecode without giving up on inlining.
>> 
>> Some numbers from the added benchmarks:
>> 
>> Benchmark(numCases)  (offset)  
>> (sorted)  Mode  Cnt   Score   Error  Units
>> MethodHandlesTableSwitchConstant.testSwitch   5 0   
>> N/A  avgt   30   4.186 � 0.054  ms/op
>> MethodHandlesTableSwitchConstant.testSwitch   5   150   
>> N/A  avgt   30   4.164 � 0.057  ms/op
>> MethodHandlesTableSwitchConstant.testSwitch  10 0   
>> N/A  avgt   30   4.124 � 0.023  ms/op
>> MethodHandlesTableSwitchConstant.testSwitch  10   150   
>> N/A  avgt   30   4.126 � 0.025  ms/op
>> MethodHandlesTableSwitchConstant.testSwitch  25 0   
>> N/A  avgt   30   4.137 � 0.042  ms/op
>> MethodHandlesTableSwitchConstant.testSwitch  25   150   
>> N/A  avgt   30   4.113 � 0.016  ms/op
>> MethodHandlesTableSwitchConstant.testSwitch  50 0   
>> N/A  avgt   30   4.118 � 0.028  ms/op
>> MethodHandlesTableSwitchConstant.testSwitch  50   150   
>> N/A  avgt   30   4.127 � 0.019  ms/op
>> MethodHandlesTableSwitchConstant.testSwitch 100 0   
>> N/A  avgt   30   4.116 � 0.013  ms/op
>> MethodHandlesTableSwitchConstant.testSwitch 100   150   
>> N/A  avgt   30   4.121 � 0.020  ms/op
>> MethodHandlesTableSwitchOpaqueSingle.testSwitch   5 0   
>> N/A  avgt   30   4.113 � 0.009  ms/op
>> MethodHandlesTableSwitchOpaqueSingle.testSwitch   5   150   
>> N/A  avgt   30   4.149 � 0.041  ms/op
>> MethodHandlesTableSwitchOpaqueSingle.testS

Re: RFR: 8263087: Add a MethodHandle combinator that switches over a set of MethodHandles [v3]

[Jorn Vernee]

> This patch adds a `tableSwitch` combinator that can be used to switch over a 
> set of method handles given an index, with a fallback in case the index is 
> out of bounds, much like the `tableswitch` bytecode. Here is a description of 
> how it works (copied from the javadoc):
> 
>  Creates a table switch method handle, which can be used to switch over a 
> set of target
>  method handles, based on a given target index, called selector.
> 
>  For a selector value of {@code n}, where {@code n} falls in the range 
> {@code [0, N)},
>  and where {@code N} is the number of target method handles, the table 
> switch method
>  handle will invoke the n-th target method handle from the list of target 
> method handles.
> 
>  For a selector value that does not fall in the range {@code [0, N)}, the 
> table switch
>  method handle will invoke the given fallback method handle.
> 
>  All method handles passed to this method must have the same type, with 
> the additional
>  requirement that the leading parameter be of type {@code int}. The 
> leading parameter
>  represents the selector.
> 
>  Any trailing parameters present in the type will appear on the returned 
> table switch
>  method handle as well. Any arguments assigned to these parameters will 
> be forwarded,
>  together with the selector value, to the selected method handle when 
> invoking it.
> 
> The combinator does not support specifying the starting index, so the switch 
> cases always run from 0 to however many target handles are specified. A 
> starting index can be added manually with another combination step that 
> filters the input index by adding or subtracting a constant from it, which 
> does not affect performance. One of the reasons for not supporting a starting 
> index is that it allows for more lambda form sharing, but also simplifies the 
> implementation somewhat. I guess an open question is if a convenience 
> overload should be added for that case?
> 
> Lookup switch can also be simulated by filtering the input through an 
> injection function that translates it into a case index, which has also 
> proven to have the ability to have comparable performance to, or even better 
> performance than, a bytecode-native `lookupswitch` instruction. I plan to add 
> such an injection function to the runtime libraries in the future as well. 
> Maybe at that point it could be evaluated if it's worth it to add a lookup 
> switch combinator as well, but I don't see an immediate need to include it in 
> this patch.
> 
> The current bytecode intrinsification generates a call for each switch case, 
> which guarantees full inlining of the target method handles. Alternatively we 
> could only have 1 callsite at the end of the switch, where each case just 
> loads the target method handle, but currently this does not allow for 
> inlining of the handles, since they are not constant.
> 
> Maybe a future C2 optimization could look at the receiver input for 
> invokeBasic call sites, and if the input is a phi node, clone the call for 
> each constant input of the phi. I believe that would allow simplifying the 
> bytecode without giving up on inlining.
> 
> Some numbers from the added benchmarks:
> 
> Benchmark(numCases)  (offset)  
> (sorted)  Mode  Cnt   Score   Error  Units
> MethodHandlesTableSwitchConstant.testSwitch   5 0   
> N/A  avgt   30   4.186 � 0.054  ms/op
> MethodHandlesTableSwitchConstant.testSwitch   5   150   
> N/A  avgt   30   4.164 � 0.057  ms/op
> MethodHandlesTableSwitchConstant.testSwitch  10 0   
> N/A  avgt   30   4.124 � 0.023  ms/op
> MethodHandlesTableSwitchConstant.testSwitch  10   150   
> N/A  avgt   30   4.126 � 0.025  ms/op
> MethodHandlesTableSwitchConstant.testSwitch  25 0   
> N/A  avgt   30   4.137 � 0.042  ms/op
> MethodHandlesTableSwitchConstant.testSwitch  25   150   
> N/A  avgt   30   4.113 � 0.016  ms/op
> MethodHandlesTableSwitchConstant.testSwitch  50 0   
> N/A  avgt   30   4.118 � 0.028  ms/op
> MethodHandlesTableSwitchConstant.testSwitch  50   150   
> N/A  avgt   30   4.127 � 0.019  ms/op
> MethodHandlesTableSwitchConstant.testSwitch 100 0   
> N/A  avgt   30   4.116 � 0.013  ms/op
> MethodHandlesTableSwitchConstant.testSwitch 100   150   
> N/A  avgt   30   4.121 � 0.020  ms/op
> MethodHandlesTableSwitchOpaqueSingle.testSwitch   5 0   
> N/A  avgt   30   4.113 � 0.009  ms/op
> MethodHandlesTableSwitchOpaqueSingle.testSwitch   5   150   
> N/A  avgt   30   4.149 � 0.041  ms/op
> MethodHandlesTableSwitchOpaqueSingle.testSwitch  10 0   
> N/A  avgt   30   4.121 � 0.026  ms/op
> MethodHandlesTableSwitchOpaqueSingle.testSwitch  10   150

Re: RFR: 8265130: Make ConstantDesc class hierarchy sealed [v4]

[Vicente Romero]

On Fri, 21 May 2021 08:53:51 GMT, Gavin Bierman  wrote:

>> Hi all,
>> 
>> Please review this patch to make the ConstantDesc hierarchy `sealed`, as was 
>> promised in its Javadoc, now that sealed classes are finalising in JDK 17. 
>> 
>> Thanks,
>> Gavin
>
> Gavin Bierman has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Reordering class modifiers

looks good to me

-

Marked as reviewed by vromero (Reviewer).

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

Re: Java records used in enums

[Kasper Nielsen]

On Fri, 21 May 2021 at 16:00, Alberto Otero Rodríguez 
wrote:

> It seems a non-existent problem until you face it, which is what happened
> me today.
>
> I think at least (supposing enums can't be records) all the fields in
> enums should be made final by default.
>

A suppressible compiler warning on non-final enum fields would probably be
the best bet.

You can actually find some build-in types where the intended behavior is
final
but it was not declared. For example, java.sql.JDBCType [1]. And a couple
of
other internal classes.

/Kasper

[1]
https://github.com/openjdk/jdk/blob/master/src/java.sql/share/classes/java/sql/JDBCType.java

RE: Java records used in enums

[Alberto Otero Rodríguez]

The problem in my case was that we have an enum ErrorEnum with several fields.

One of the fields was "description" which have a getter and a setter.

The problem appeared when testing with JUnit. Basically the logic of the 
application was working correctly but the descriptions of the errors of 
different tests were overriden with the description of the errors of another 
tests.

The problem was solved by making the fields of the enums final and removing all 
the setter methods (after trying a lot of things and refactoring the 
application almost entirely to remove all the static methods and static members 
we had in the application).

Regards,

Alberto.

De: core-libs-dev  en nombre de 
liangchenb...@gmail.com 
Enviado: viernes, 21 de mayo de 2021 17:15
Para: core-libs-dev@openjdk.java.net 
Asunto: Fwd: Java records used in enums

-- Forwarded message -
From: - 
Date: Fri, May 21, 2021 at 10:14 AM
Subject: Re: Java records used in enums
To: Alberto Otero Rodríguez 


I fail to understand what you want. How does making enums records fix the
multithreading issue?

If you wish to have enums declared like records code-wise, I see it as a
possibility, but I don't see how it affects any enum behavior in any way.

If you want a combination of record and enum, or a record with enumerated
possible instances, it may be possible to add an alternative syntax for
such enum declaration; but restricting all enums to be such enum records
offers no benefit that I can see, especially given records themselves have
severe restrictions (must have an accessible canonical ctor available; that
ctor cannot throw checked exceptions; field/getter method name limits, so a
"enum record" cannot declare fields like "values" "name"); also records are
value-based while enums are identity based, which is another problem.

In short, I believe regular enum suffices and a "record enum" offers little
benefit while it cannot fix your issue at all.

On Fri, May 21, 2021 at 10:00 AM Alberto Otero Rodríguez <
albest...@hotmail.com> wrote:

> It seems a non-existent problem until you face it, which is what happened
> me today.
>
> I think at least (supposing enums can't be records) all the fields in
> enums should be made final by default.
>
> Regards,
>
> Alberto.
> 
> De: Kasper Nielsen 
> Enviado: viernes, 21 de mayo de 2021 16:28
> Para: Alberto Otero Rodríguez 
> Cc: core-libs-dev@openjdk.java.net 
> Asunto: Re: Java records used in enums
>
> On Fri, 21 May 2021 at 14:51, Alberto Otero Rodríguez <
> albest...@hotmail.com> wrote:
> Hi,
>
> I think enums in Java should be immutable. When you let the programmer
> change values in an enum instance, an unexpected behaviour can happen when
> using multiple threads as enum instances are static (singleton).
>
> So, I was wondering why not make enums instances be defined as records
> instead of normal classes.
>
> Lots of reasons, for example:
> * It would be a breaking change for an almost non-existent problem.
> * All internal state of enums would now be public.
> * Enum's extend java.lang.Enum, records extend java.lang.Record.
>   java.lang.Enum cannot extend java.lang.Record because it has instance
> fields.
>
> /Kasper
>

Fwd: Java records used in enums

[-]

-- Forwarded message -
From: - 
Date: Fri, May 21, 2021 at 10:14 AM
Subject: Re: Java records used in enums
To: Alberto Otero Rodríguez 


I fail to understand what you want. How does making enums records fix the
multithreading issue?

If you wish to have enums declared like records code-wise, I see it as a
possibility, but I don't see how it affects any enum behavior in any way.

If you want a combination of record and enum, or a record with enumerated
possible instances, it may be possible to add an alternative syntax for
such enum declaration; but restricting all enums to be such enum records
offers no benefit that I can see, especially given records themselves have
severe restrictions (must have an accessible canonical ctor available; that
ctor cannot throw checked exceptions; field/getter method name limits, so a
"enum record" cannot declare fields like "values" "name"); also records are
value-based while enums are identity based, which is another problem.

In short, I believe regular enum suffices and a "record enum" offers little
benefit while it cannot fix your issue at all.

On Fri, May 21, 2021 at 10:00 AM Alberto Otero Rodríguez <
albest...@hotmail.com> wrote:

> It seems a non-existent problem until you face it, which is what happened
> me today.
>
> I think at least (supposing enums can't be records) all the fields in
> enums should be made final by default.
>
> Regards,
>
> Alberto.
> 
> De: Kasper Nielsen 
> Enviado: viernes, 21 de mayo de 2021 16:28
> Para: Alberto Otero Rodríguez 
> Cc: core-libs-dev@openjdk.java.net 
> Asunto: Re: Java records used in enums
>
> On Fri, 21 May 2021 at 14:51, Alberto Otero Rodríguez <
> albest...@hotmail.com> wrote:
> Hi,
>
> I think enums in Java should be immutable. When you let the programmer
> change values in an enum instance, an unexpected behaviour can happen when
> using multiple threads as enum instances are static (singleton).
>
> So, I was wondering why not make enums instances be defined as records
> instead of normal classes.
>
> Lots of reasons, for example:
> * It would be a breaking change for an almost non-existent problem.
> * All internal state of enums would now be public.
> * Enum's extend java.lang.Enum, records extend java.lang.Record.
>   java.lang.Enum cannot extend java.lang.Record because it has instance
> fields.
>
> /Kasper
>

Re: RFR: 8263561: Re-examine uses of LinkedList

[Thiago Henrique Hüpner]

On Fri, 21 May 2021 15:00:15 GMT, Alan Bateman  wrote:

>> src/java.base/share/classes/jdk/internal/util/jar/JarIndex.java line 154:
>> 
>>> 152:  * @param fileName the key of the mapping
>>> 153:  */
>>> 154: public List get(String fileName) {
>> 
>> IcedTea-Web seems to be using this method reflectively:
>> https://github.com/AdoptOpenJDK/IcedTea-Web/blob/master/common/src/main/java/net/adoptopenjdk/icedteaweb/jdk89access/JarIndexAccess.java#L80
>
>> IcedTea-Web seems to be using this method reflectively:
>> https://github.com/AdoptOpenJDK/IcedTea-Web/blob/master/common/src/main/java/net/adoptopenjdk/icedteaweb/jdk89access/JarIndexAccess.java#L80
> 
> I assume this doesn't work with JDK 16, at least not without using 
> --add-options to export jdk.internal.util.jar.

Just for completeness, they're using the add-exports:
https://github.com/AdoptOpenJDK/IcedTea-Web/blob/master/launchers/itw-modularjdk.args#L19

-

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

Re: RFR: 8263561: Re-examine uses of LinkedList

[Alan Bateman]

On Fri, 21 May 2021 14:52:21 GMT, Thiago Henrique Hüpner 
 wrote:

> IcedTea-Web seems to be using this method reflectively:
> https://github.com/AdoptOpenJDK/IcedTea-Web/blob/master/common/src/main/java/net/adoptopenjdk/icedteaweb/jdk89access/JarIndexAccess.java#L80

I assume this doesn't work with JDK 16, at least not without using 
--add-options to export jdk.internal.util.jar.

-

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

RE: Java records used in enums

[Alberto Otero Rodríguez]

It seems a non-existent problem until you face it, which is what happened me 
today.

I think at least (supposing enums can't be records) all the fields in enums 
should be made final by default.

Regards,

Alberto.

De: Kasper Nielsen 
Enviado: viernes, 21 de mayo de 2021 16:28
Para: Alberto Otero Rodríguez 
Cc: core-libs-dev@openjdk.java.net 
Asunto: Re: Java records used in enums

On Fri, 21 May 2021 at 14:51, Alberto Otero Rodríguez 
mailto:albest...@hotmail.com>> wrote:
Hi,

I think enums in Java should be immutable. When you let the programmer change 
values in an enum instance, an unexpected behaviour can happen when using 
multiple threads as enum instances are static (singleton).

So, I was wondering why not make enums instances be defined as records instead 
of normal classes.

Lots of reasons, for example:
* It would be a breaking change for an almost non-existent problem.
* All internal state of enums would now be public.
* Enum's extend java.lang.Enum, records extend java.lang.Record.
  java.lang.Enum cannot extend java.lang.Record because it has instance fields.

/Kasper

Re: RFR: 8263561: Re-examine uses of LinkedList

[Thiago Henrique Hüpner]

On Fri, 26 Feb 2021 10:48:33 GMT, Сергей Цыпанов 
 wrote:

> The usage of `LinkedList` is senseless and can be replaced with either 
> `ArrayList` or `ArrayDeque` which are both more compact and effective.
> 
> jdk:tier1 and jdk:tier2 are both ok

src/java.base/share/classes/jdk/internal/util/jar/JarIndex.java line 154:

> 152:  * @param fileName the key of the mapping
> 153:  */
> 154: public List get(String fileName) {

IcedTea-Web seems to be using this method reflectively:
https://github.com/AdoptOpenJDK/IcedTea-Web/blob/master/common/src/main/java/net/adoptopenjdk/icedteaweb/jdk89access/JarIndexAccess.java#L80

-

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

Re: Java records used in enums

[Robert Marcano]

On 5/21/21 10:28 AM, Kasper Nielsen wrote:

On Fri, 21 May 2021 at 14:51, Alberto Otero Rodríguez 
wrote:


Hi,

I think enums in Java should be immutable. When you let the programmer
change values in an enum instance, an unexpected behaviour can happen when
using multiple threads as enum instances are static (singleton).

So, I was wondering why not make enums instances be defined as records
instead of normal classes.



Lots of reasons, for example:
* It would be a breaking change for an almost non-existent problem.
* All internal state of enums would now be public.
* Enum's extend java.lang.Enum, records extend java.lang.Record.
   java.lang.Enum cannot extend java.lang.Record because it has instance
fields.

/Kasper



I could add that making enum records would not solve the proposed 
"problem" because records are shallowly immutable.


I am sure that theoretically, simple enum implementations could be 
treated by the JIT internally as simple indexes as an optimization, I 
don't know if it does something like that already. So I don't think 
records will bring any improvement to enums

JDK-8246677 caused 16x performance regression in OpenJDK Tip SynchronousQueue

[Astigeevich, Evgeny]

Hi Alan,

I was benchmarking SynchronousQueue from the OpenJDK Tip on Linux. I found its 
performance is 16 times lower than the performance of SynchronousQueue from 
OpenJDK 11. The regression exists on Linux x86_64 and Linux aarch64. Comparing 
SynchronousQueue implementations I found the OpenJDK Tip SynchronousQueue is 
changed by PR https://github.com/openjdk/jdk/pull/1645 
(JDK-8246677). Reverting 
SynchronousQueue to the version before the PR fixes the regression. I cannot 
find any performance data backing the PR.

I raised a JBS issue: https://bugs.openjdk.java.net/browse/JDK-8267502
It has details how to reproduce the issue.

As we are approaching JDK 17 Rampdown Phase One (2021/06/10), could the issue 
be triaged?

Thanks,
Evgeny Astigeevich



Amazon Development Centre (London) Ltd. Registered in England and Wales with 
registration number 04543232 with its registered office at 1 Principal Place, 
Worship Street, London EC2A 2FA, United Kingdom.

Re: Java records used in enums

[Kasper Nielsen]

On Fri, 21 May 2021 at 14:51, Alberto Otero Rodríguez 
wrote:

> Hi,
>
> I think enums in Java should be immutable. When you let the programmer
> change values in an enum instance, an unexpected behaviour can happen when
> using multiple threads as enum instances are static (singleton).
>
> So, I was wondering why not make enums instances be defined as records
> instead of normal classes.
>

Lots of reasons, for example:
* It would be a breaking change for an almost non-existent problem.
* All internal state of enums would now be public.
* Enum's extend java.lang.Enum, records extend java.lang.Record.
  java.lang.Enum cannot extend java.lang.Record because it has instance
fields.

/Kasper

Re: RFR: 8263561: Re-examine uses of LinkedList

[Alan Bateman]

On Fri, 21 May 2021 13:13:04 GMT, Сергей Цыпанов 
 wrote:

> Hi guys, any more comments here? Please ping me if I've missed something

I suspect this will require renaming some fields and changing comments, e.g. 
requestList is no longer a good name for the field in AbstractPoller, its 
comments need changes too. The field in ResolverConfigurationImpl is 
searchList, it will require a few changes. There may be more, I just picked out 
a few.

-

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

Re: RFR: 8263561: Re-examine uses of LinkedList

[Daniel Fuchs]

On Fri, 26 Feb 2021 10:48:33 GMT, Сергей Цыпанов 
 wrote:

> The usage of `LinkedList` is senseless and can be replaced with either 
> `ArrayList` or `ArrayDeque` which are both more compact and effective.
> 
> jdk:tier1 and jdk:tier2 are both ok

I don't remember all the comments you have received in this thread but have you 
verified that arbitrarily changing `LinkedList` into `ArrayList` in these 
classes is not going to significantly increase the footprint in the case where 
lists are empty or contain only one or two elements?

I am not convinced that a global replacement of  `LinkedList` by `ArrayList` is 
necessarily good - even though I agree that `ArrayList` is generally more 
efficient.

src/java.base/share/classes/jdk/internal/util/jar/JarIndex.java line 155:

> 153:  */
> 154: public List get(String fileName) {
> 155: ArrayList jarFiles;

This could probably be declared as:


List jarFiles;

src/java.base/share/classes/jdk/internal/util/jar/JarIndex.java line 264:

> 262: String jar = jarFiles[i];
> 263: bw.write(jar + "\n");
> 264: ArrayList jarlist = jarMap.get(jar);

Here again, jarList could probably be declared as `List`

-

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

Re: RFR: 8263561: Re-examine uses of LinkedList

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

On Fri, 26 Feb 2021 10:48:33 GMT, Сергей Цыпанов 
 wrote:

> The usage of `LinkedList` is senseless and can be replaced with either 
> `ArrayList` or `ArrayDeque` which are both more compact and effective.
> 
> jdk:tier1 and jdk:tier2 are both ok

Hi guys, any more comments here? Please ping me if I've missed something

-

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

Re: RFR: 8267110: Update java.util to use instanceof pattern variable [v4]

[Patrick Concannon]

> Hi,
> 
> Could someone please review my code for updating the code in the `java.util` 
> package to make use of the `instanceof` pattern variable?
> 
> Kind regards,
> Patrick

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

  8267110: Reverted changes made to files in java.util.concurrent

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4088/files
  - new: https://git.openjdk.java.net/jdk/pull/4088/files/da615e7d..2c076a55

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

  Stats: 31 lines in 5 files changed: 16 ins; 0 del; 15 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4088.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4088/head:pull/4088

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

Re: RFR: 8267521: Post JEP 411 refactoring: maximum covering > 50K [v2]

[Weijun Wang]

> The code change refactors classes that have a `SuppressWarnings("removal")` 
> annotation that covers more than 50KB of code. The big annotation is often 
> quite faraway from the actual deprecated API usage it is suppressing, and 
> with the annotation covering too big a portion it's easy to call other 
> deprecated methods without being noticed.
> 
> The code change shows some common solutions to avoid such too wide 
> annotations:
> 
> 1. Extract calls into a method and add annotation on that method
> 2. Assign the return value of a deprecated method call to a new local 
> variable and add annotation on the declaration, and then assign that value to 
> the original l-value.
> 3. Put declaration and assignment into a single statement if possible.
> 4. Rewrite code to achieve #3 above.

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

  typo on windows

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4138/files
  - new: https://git.openjdk.java.net/jdk/pull/4138/files/e3f0405a..d460efb8

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

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

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

Re: RFR: 8265130: Make ConstantDesc class hierarchy sealed [v4]

[Jorn Vernee]

On Fri, 21 May 2021 08:53:51 GMT, Gavin Bierman  wrote:

>> Hi all,
>> 
>> Please review this patch to make the ConstantDesc hierarchy `sealed`, as was 
>> promised in its Javadoc, now that sealed classes are finalising in JDK 17. 
>> 
>> Thanks,
>> Gavin
>
> Gavin Bierman has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Reordering class modifiers

This looks good to me.

Does this require a CSR as well?

I see a CSR here for another change that seals a hierarchy: 
https://bugs.openjdk.java.net/browse/JDK-8267506

Ah, nvm, it's already indicated on the PR :)

-

Marked as reviewed by jvernee (Reviewer).

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

Java records used in enums

[Alberto Otero Rodríguez]

Hi,

I think enums in Java should be immutable. When you let the programmer change 
values in an enum instance, an unexpected behaviour can happen when using 
multiple threads as enum instances are static (singleton).

So, I was wondering why not make enums instances be defined as records instead 
of normal classes.

What do you think?

Regards,

Alberto.

Re: RFR: 8267110: Update java.util to use instanceof pattern variable [v3]

[Patrick Concannon]

> Hi,
> 
> Could someone please review my code for updating the code in the `java.util` 
> package to make use of the `instanceof` pattern variable?
> 
> Kind regards,
> Patrick

Patrick Concannon 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 four additional 
commits since the last revision:

 - Merge remote-tracking branch 'origin/master' into JDK-8267110
 - 8267110: Reverted changes in java/util/Formatter as primitive to boxed types 
may have semantic/performance implications
 - Merge remote-tracking branch 'origin/master' into JDK-8267110
 - 8267110: Update java.util to use instanceof pattern variable

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4088/files
  - new: https://git.openjdk.java.net/jdk/pull/4088/files/cd99dc49..da615e7d

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

  Stats: 4905 lines in 201 files changed: 2421 ins; 2058 del; 426 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4088.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4088/head:pull/4088

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

Integrated: 8203359: Container level resources events

[Jaroslav Bachorik]

On Mon, 22 Mar 2021 15:57:12 GMT, Jaroslav Bachorik  
wrote:

> With this change it becomes possible to surface various cgroup level metrics 
> (available via `jdk.internal.platform.Metrics`) as JFR events.
> 
> Only a subset of the metrics exposed by `jdk.internal.platform.Metrics` is 
> turned into JFR events to start with.
> * CPU related metrics
> * Memory related metrics
> * I/O related metrics
> 
> For each of those subsystems a configuration data will be emitted as well. 
> The initial proposal is to emit the configuration data events at least once 
> per chunk and the metrics values at 30 seconds interval. 
> By using these values the emitted events seem to contain useful information 
> without increasing overhead (the metrics values are read from `/proc` 
> filesystem so that should not be done too frequently).

This pull request has now been integrated.

Changeset: ee2651b9
Author:Jaroslav Bachorik 
URL:   
https://git.openjdk.java.net/jdk/commit/ee2651b9e5a9ab468b4be73d43b8f643e9e92042
Stats: 598 lines in 14 files changed: 582 ins; 4 del; 12 mod

8203359: Container level resources events

Reviewed-by: sgehwolf, egahlin

-

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

RE: New java.util.Strings class

[Alberto Otero Rodríguez]

Hi,

I have made other changes to the Strings class I proposed in my previous 
messages.

The changes are:

  *   Added the new methods compareTo and compareToIgnoreCase
  *   Changed WhiteSpace for Whitespace

You can see the new code here:
https://github.com/Aliuken/Java-Strings/blob/main/Strings.java

With those changes, the annotations suggested in the previous message should 
change to:
- @NonNullNorWhitespace
- @NonNullNorWhitespaceElse(defaultValue)
- @NonNullNorWhitespaceElseGet(class::supplierMethod)

Regards,

Alberto Otero Rodríguez.

De: Alberto Otero Rodríguez 
Enviado: miércoles, 19 de mayo de 2021 11:36
Para: core-libs-dev@openjdk.java.net 
Asunto: RE: New java.util.Strings class

Hi,

I have made some changes to the Strings class I proposed in my previous message.

The changes are:

  *   Use str.isEmpty() instead of EMPTY_STRING.equals(str)
  *   Create methods using str.strip() to check a String is not Whitespace

You can see the new code here:
https://github.com/Aliuken/Java-Strings/blob/main/Strings.java

With those changes, the following annotations could also be created for method 
arguments and return types:
- @NonNullNorWhiteSpace
- @NonNullNorWhiteSpaceElse(defaultValue)
- @NonNullNorWhiteSpaceElseGet(class::supplierMethod)

I didn't have any response to the previous message.

Please, take this one in consideration.

Regards,

Alberto Otero Rodríguez.


De: Alberto Otero Rodríguez
Enviado: lunes, 17 de mayo de 2021 14:58
Para: core-libs-dev@openjdk.java.net 
Asunto: New java.util.Strings class

Hi, members of the core-libs developers of OpenJDK.

I think Java needs a Strings class similar to the java.util.Objects class of 
Java 16 to be used in method arguments, return types and Stream filters.

I have coded it myself here based on the Objects implementation of Java 16 
(please have a look):
https://github.com/Aliuken/Java-Strings/blob/main/Strings.java

In fact, I think it would be useful also adding annotations for method 
arguments and return types such as:
- @NonNull
- @NonNullElse(defaultValue)
- @NonNullElseGet(class::supplierMethod)
- @NonNullNorEmpty
- @NonNullNorEmptyElse(defaultValue)
- @NonNullNorEmptyElseGet(class::supplierMethod)

With that kind of annotations, you could assume thinks like:
- an argument or return type cannot have value null, but an Exception
- an argument or return type cannot have value null, but a default value

What do you think?

Waiting for your response.

PS: I am sending this email repeated. I have sended it yesterday with my other 
email account (alber8...@gmail.com), but I wasn't a member of this mailing 
list. Now I have become a member with this other email account.

Regards,

Alberto Otero Rodríguez.

Re: RFR: 8265130: Make ConstantDesc class hierarchy sealed [v3]

[Gavin Bierman]

On Fri, 21 May 2021 03:26:49 GMT, liach  
wrote:

>> Gavin Bierman has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Removing file constants.patch added by mistake
>
> src/java.base/share/classes/java/lang/constant/ClassDesc.java line 56:
> 
>> 54:  * @since 12
>> 55:  */
>> 56: sealed public interface ClassDesc
> 
> Should move `sealed` behind `public` per the blessed modifier order from JLS. 
> https://docs.oracle.com/javase/specs/jls/se16/preview/specs/sealed-classes-jls.html#jls-8.1.1
> 
> Per http://openjdk.java.net/jeps/409 the finalized sealed classes have no 
> difference from that in 16, so the order suggested there should be valid.

Thanks. Now changed.

-

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

Re: RFR: 8265130: Make ConstantDesc class hierarchy sealed [v4]

[Gavin Bierman]

> Hi all,
> 
> Please review this patch to make the ConstantDesc hierarchy `sealed`, as was 
> promised in its Javadoc, now that sealed classes are finalising in JDK 17. 
> 
> Thanks,
> Gavin

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

  Reordering class modifiers

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4135/files
  - new: https://git.openjdk.java.net/jdk/pull/4135/files/c8f632f6..c36075d2

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

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

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

Re: RFR: 8266310: deadlock while loading the JNI code [v2]

[Peter Levart]

On 21/05/2021 01:11, David Holmes wrote:

Hi Peter,

On 21/05/2021 12:42 am, Peter Levart wrote:

Hi Aleksei,

Are you trying to solve this in principle or do you have a concrete 
problem at hand which triggers this deadlock? If it is the later, 
then some rearrangement of code might do the trick... For example, 
native libraries are typically loaded by a class initializer of some 
class that is guaranteed to be initialized before the 1st invocation 
of a native method from such library. But if such class can also be 
loaded and initialized by some other trigger, deadlock can occur. 
Best remedy for such situation is to move all native methods to a 
special class that serves just for interfacing with native code and 
also contains an initializer that loads the native library and 
nothing else. Such arrangement would ensure that the order of taking 
locks is always the same: classLoadingLock -> nativeLibraryLock ...


There were specific examples for this problem, but Aleksei is trying 
to define a general solution - which unfortunately doesn't exist.


The basic deadlock scenario is a special variant of the general class 
initialization deadlock:


Thread 1:
- loadLibrary
 - acquire loadLibrary global lock
   - call JNI_OnLoad
 - use class Foo (which needs to be loaded and initialized)
   - block acquiring  lock for Foo

Thread 2:
 - Initialize class Foo
  - Acquire  lock for Foo
    - 
  - call loadLibrary(x) // for any X
   - block acquiring loadLibrary global lock

We can reduce the chance of deadlock by using a per-native-library 
lock instead of the global loadLibrary lock - which is what Aleksei's 
initial version did. But we cannot remove all deadlock possibility 
because we must ensure only one thread can be executing JNI_OnLoad for 
any given native library.



Right, I was just trying to suggest that by exercising some discipline 
about how to arrange code that loads native libraries, deadlocks can be 
avoided if also Aleksei's initial version of the patch is used (using a 
per-native-library lock instead of the global loadLibrary lock). The 
discipline would be to load a particular native library only from 
 of a unique class associated with that native library. If 
everybody followed this discipline (which fortunately is typical use of 
loadLibrary), then without Aleksei's patch, deadlock is still possible. 
Imagine two native libraries A and B, each loaded from  of 
corresponding classes ClassA and ClassB. Library A also has JNI_OnLoad 
which uses ClassB. So we have:


Thread 1:
    - initialize ClassA
    - acquire  lock for ClassA
        - ClassA.
        - call loadLibrary(A)
        - acquire loadLibrary global lock
            - call JNI_OnLoad for A
        - use class ClassB (which needs to be loaded and initialized)
            - block acquiring  lock for ClassB

Thread 2:
    - initialize ClassB
    - acquire  lock for ClassB
        - ClassB.
        - call loadLibrary(B)
        - block acquiring loadLibrary global lock

With Aleksei's initial patch, such scenario would not result in a 
deadlock. And since such scenario can arise from typical use of 
loadLibrary, I think this patch is a good thing. It reduces number of 
scenarios where deadlock is a possible outcome. Here's another scenario 
where deadlock would still occur even with Sergei's initial patch. It is 
a modified variant of above scenario where JNI_OnLoad of library A uses 
ClassB and JNI_OnLoad of library B uses ClassA:


Thread 1:
    - initialize ClassA
    - acquire  lock for ClassA
        - ClassA.
        - call loadLibrary(A)
        - acquire loadLibrary(A) lock
            - call JNI_OnLoad for A
        - use class ClassB (which needs to be loaded and initialized)
            - block acquiring  lock for ClassB

Thread 2:
    - initialize ClassB
    - acquire  lock for ClassB
        - ClassB.
        - call loadLibrary(B)
        - acquire loadLibrary(B) lock
            - call JNI_OnLoad for B
        - use class ClassA (which needs to be loaded and initialized)
            - block acquiring  lock for ClassA

But in this scenario, deadlock is provoked without blocking on any 
loadLibrary lock. This deadlock arises from circular dependencies among 
classes in their  methods. It just happens that these 
dependencies are evaluated via a chain of: X. -> loadLibrary -> 
JNI_OnLoad -> use class Y calls. Such deadlock is possible without 
involvement of native libraries loading too, so I would not classify it 
as the problem that Sergei's patch is trying to solve.


I still haven't found a scenario of a possible deadlock when Sergei's 
initial patch is combined with the above mentioned discipline in which a 
loadLibrary lock would be involved.


Regards, Peter



Cheers,
David
-



Regards, Peter

On 5/20/21 12:31 AM, David Holmes wrote:

On 20/05/2021 2:29 am, Aleksei Voitylov wrote:
On Wed, 19 May 2021 16:21:41 GMT, Aleksei Voitylov 
 wrote:


Please review this PR which fixes t