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

[Brent Christian]

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 273:

> 271:  * {@link ObjectInputFilter.Status#REJECTED}, if the filter 
> is checking a class
> 272:  * and the filter returns {@link 
> ObjectInputFilter.Status#UNDECIDED}, 
> 273:  * Otherwise, return the status

"Otherwise, return the status of this filter" ?

-

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

Re: RFR: 8224243: Make AccessibleObject a sealed class

[Éamonn McManus]

Hi Joe,

I see that I blogged
 about
AccessibleObject in 2006 and already complained about its protected
constructor back then. :-)

Still, if the main motivation of sealing it is to avoid adding @implSpec to
a few methods, perhaps it would be better just to bite the bullet and do
that? We can fix Guava's Invokable class so it no longer inherits from
AccessibleObject, and I think we probably should, but it would still be the
case that an app using a version of Guava from before the fix would fail
when deployed to a JDK with this change.

Thanks,
Éamonn

On Thu, 20 May 2021 at 15:47, Joe Darcy  wrote:

> Hi Éamonn,
>
> On 5/20/2021 2:59 PM, Éamonn McManus wrote:
> > Thanks for the tip, Alan. Guava's public Invokable
> > <
> https://javadoc.io/doc/com.google.guava/guava/latest/com/google/common/reflect/Invokable.html
> >
> > class does inherit from AccessibleObject. It would probably not be too
> hard
> > to make it stop doing so. That's technically a breaking API change, but
> the
> > class is marked @Beta so that's fair game.
> >
> > I looked in Google's code base and I found one other project that extends
> > AccessibleObject, picocli, here
> >  >.
> > That's not to say that nobody else does it, but apparently cases are
> rare.
> >
> > I do wonder what the purpose of the change would be. The constructor
> > javadoc text doesn't say you *can't* extend this class. Is there some
> > benefit from making it not work anymore?
> >
>
> The intent of the constructor's text to me implies the class is not
> intended to be subclassed outside of the JDK. (I'm not sure why the
> constructor isn't package private as all its subclasses are in the same
> package which would have accomplished that, but the SCM history from
> circa JDK 1.2 would need to be consulted for guidance.)
>
> The original impetus for 8224243 was the implementation of certain
> overridable methods in AccessibleObject (getAnnotation,
> getAnnotationsByType, and getDeclaredAnnotations) not @implSpec'ing
> their behavior. The behavior is to thrown an error as the method should
> be overridden in the subclasses. If all the subclasses are within the
> JDK and the class is sealed, that @implSpec'ing need not occur.
>
> If all the subclasses were in the JDK (which we know is not that case),
> it is preferable to avoid the @implSpec'ing throwing the exception as
> that is just an implementation-internal comment.
>
> HTH,
>
> -Joe
>
>

RFR: 8267403: tools/jpackage/share/FileAssociationsTest.java#id0 failed with "Error: Bundler "Mac PKG Package" (pkg) failed to produce a package"

[Alexander Matveev]

Looks like another issue similar to hdiutil (JDK-8249395) when process is gone, 
but we still waiting for it. I was not able to reproduce this issue by running 
test or pkgbuild separately and conclusion was made based on logs. Fixed in 
same way as hdiutil issue. Also, I added logging PID for external commands to 
simplify log analysis. Log will have multiple hdiutil and/or pkgbuild 
processes, since we running multiple tests in parallel and matching external 
process to test failure requires looking at command line for particular 
process, so PID should simplify this.

-

Commit messages:
 - 8267403: tools/jpackage/share/FileAssociationsTest.java#id0 failed with 
"Error: Bundler \"Mac PKG Package\" (pkg) failed to produce a package"

Changes: https://git.openjdk.java.net/jdk/pull/4139/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=4139&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8267403
  Stats: 31 lines in 5 files changed: 16 ins; 1 del; 14 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4139.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4139/head:pull/4139

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

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

[liach]

On Thu, 20 May 2021 22:13: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:
> 
>   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.

-

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

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

[Brent Christian]

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/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 `` ?

-

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

Re: RFR: 8224243: Make AccessibleObject a sealed class [v2]

[Joe Darcy]

On Fri, 21 May 2021 02:42:50 GMT, Joe Darcy  wrote:

>> Conceptually, AccessbileObject is a sealed class with a protected 
>> constructor stating
>> 
>> Constructor: only used by the Java Virtual Machine.
>> 
>> With the language now supporting sealed classes, the AccessbileObject should 
>> be marked as sealed.
>> 
>> Executable and Field are the subclasses of AccessbileObject in the JDK; as 
>> Executable has subclasses, it is marked as non-sealed.
>> 
>> Please also review the corresponding CSR:
>> 
>> https://bugs.openjdk.java.net/browse/JDK-8224243
>
> Joe Darcy has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Update in response to review feedback.

Pushed an updated version of the fix that deprecates the AccessibleObject 
constructor, adds the @implSpec tags, and makes Executable a sealed class.

-

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

Re: RFR: 8224243: Make AccessibleObject a sealed class [v2]

[Joe Darcy]

> Conceptually, AccessbileObject is a sealed class with a protected constructor 
> stating
> 
> Constructor: only used by the Java Virtual Machine.
> 
> With the language now supporting sealed classes, the AccessbileObject should 
> be marked as sealed.
> 
> Executable and Field are the subclasses of AccessbileObject in the JDK; as 
> Executable has subclasses, it is marked as non-sealed.
> 
> Please also review the corresponding CSR:
> 
> https://bugs.openjdk.java.net/browse/JDK-8224243

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

  Update in response to review feedback.

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4133/files
  - new: https://git.openjdk.java.net/jdk/pull/4133/files/f605cfa8..be517cda

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

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

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

Re: RFR: 8224243: Make AccessibleObject a sealed class

[Éamonn McManus]

Thanks for the tip, Alan. Guava's public Invokable

class does inherit from AccessibleObject. It would probably not be too hard
to make it stop doing so. That's technically a breaking API change, but the
class is marked @Beta so that's fair game.

I looked in Google's code base and I found one other project that extends
AccessibleObject, picocli, here
.
That's not to say that nobody else does it, but apparently cases are rare.

I do wonder what the purpose of the change would be. The constructor
javadoc text doesn't say you *can't* extend this class. Is there some
benefit from making it not work anymore?

On Thu, 20 May 2021 at 10:36, Alan Bateman  wrote:

> On Thu, 20 May 2021 17:14:57 GMT, Joe Darcy  wrote:
>
> > Conceptually, AccessbileObject is a sealed class with a protected
> constructor stating
> >
> > Constructor: only used by the Java Virtual Machine.
> >
> > With the language now supporting sealed classes, the AccessbileObject
> should be marked as sealed.
> >
> > Executable and Field are the subclasses of AccessbileObject in the JDK;
> as Executable has subclasses, it is marked as non-sealed.
> >
> > Please also review the corresponding CSR:
> >
> > https://bugs.openjdk.java.net/browse/JDK-8224243
>
> I think this will require reaching out to Google Guava, I think its their
> Invocable API that extends AccessibleObject outside of the JDK. We ran this
> when doing the module system where we didn't initially take into account
> sub-classes that were outside of java.base.
>
> -
>
> PR: https://git.openjdk.java.net/jdk/pull/4133
>

RFR: 8267521: Post JEP 411 refactoring: maximum covering > 50K

[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.

-

Depends on: https://git.openjdk.java.net/jdk/pull/4073

Commit messages:
 - 8267521: Post JEP 411 refactoring: maximum covering > 50K

Changes: https://git.openjdk.java.net/jdk/pull/4138/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=4138&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8267521
  Stats: 226 lines in 18 files changed: 142 ins; 29 del; 55 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: jpackage GraalVM on Windows 10

[Michael Hall]

> On May 20, 2021, at 6:45 PM, Michael Hall  wrote:
> 
> Has anyone done this who can indicate how it should be done?
> 

It seems to be the non-standard JDK name. I renamed it to jdk-11 and it worked. 
Letting it default still set it to the current jdk-16 even though that was 
later in PATH.
But using —runtime-image worked.
I’ll bug report if non-standard JDK names can be expected to be supported.

Integrated: 8267056: tools/jpackage/share/RuntimePackageTest.java fails with NoSuchFileException

[Alexander Matveev]

On Wed, 19 May 2021 21:00:07 GMT, Alexander Matveev  
wrote:

> For debug build on macOS, runtime which used for test fill be located in 
> /path/jdk-17/fastdebug and /path/jdk-17 will not contain any other files 
> except fastdebug and in this case our check to decide if we need to copy app 
> or runtime will return /path/jdk-17 which is not correct. Fixed by skipping 
> this check for runtime and only using it for app. Also, added ignoring 
> .DS_Store to test which is needed if user used Finder to look inside runtime 
> folder which will cause .DS_Store to be created and will cause test to fail, 
> since this file is not being packaged.

This pull request has now been integrated.

Changeset: 9eaa4afc
Author:Alexander Matveev 
URL:   
https://git.openjdk.java.net/jdk/commit/9eaa4afc99b09f4704e4d641f95104be40b9ea66
Stats: 15 lines in 2 files changed: 5 ins; 0 del; 10 mod

8267056: tools/jpackage/share/RuntimePackageTest.java fails with 
NoSuchFileException

Reviewed-by: asemenyuk, herrick

-

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

jpackage GraalVM on Windows 10

[Michael Hall]

Has anyone done this who can indicate how it should be done?

I recently set this up for my app on OS/X and thought I’d have cross platform 
versions.
I have tried it both with —runtime-image and making Graal the installed JVM.

Both ways I seem to get an application with directory structure

runtime
—— graalvm-ce-java11-21.1.0
—bin
—conf
…

It appears it should not have the Graal dir in there. I removed it and the 
application runs. Otherwise no.
Again it seems to do this even if I let it default to the installed Graal jdk - 
not —runtime-image.

If no one else has run into this and figured something out or just knows how to 
do it correctly I can just do a bug report.

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

[Mandy Chung]

On Thu, 20 May 2021 22:35:38 GMT, Thiago Henrique Hüpner 
 wrote:

>> I should have made it clear.  I was expecting `DynamicConstantDesc` should 
>> be `sealed`.  Do you expect non-JDK implementation class extending 
>> `DynamicConstantDesc`?
>
> From the ConstantDesc Javadoc:
> 
>  * Non-platform classes should not implement {@linkplain ConstantDesc} 
> directly.
>  * Instead, they should extend {@link DynamicConstantDesc} (as {@link 
> EnumDesc}
>  * and {@link VarHandleDesc} do.)

Thanks.  I missed this javadoc.

-

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

Re: RFR: 8267190: Optimize Vector API test operations [v3]

[Sandhya Viswanathan]

On Thu, 20 May 2021 23:19:01 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
>
> Sandhya Viswanathan has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Implement review comments

Thanks Paul. I have implemented these two suggestions as well.

If no objections from any one else, I plan to integrate this tomorrow, Friday 
May 21.

-

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

Re: RFR: 8267056: tools/jpackage/share/RuntimePackageTest.java fails with NoSuchFileException [v2]

[Andy Herrick]

On Thu, 20 May 2021 22:25:10 GMT, Alexander Matveev  
wrote:

>> For debug build on macOS, runtime which used for test fill be located in 
>> /path/jdk-17/fastdebug and /path/jdk-17 will not contain any other files 
>> except fastdebug and in this case our check to decide if we need to copy app 
>> or runtime will return /path/jdk-17 which is not correct. Fixed by skipping 
>> this check for runtime and only using it for app. Also, added ignoring 
>> .DS_Store to test which is needed if user used Finder to look inside runtime 
>> folder which will cause .DS_Store to be created and will cause test to fail, 
>> since this file is not being packaged.
>
> Alexander Matveev has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   8267056: tools/jpackage/share/RuntimePackageTest.java fails with 
> NoSuchFileException [v2]

Marked as reviewed by herrick (Reviewer).

-

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

Re: RFR: 8267190: Optimize Vector API test operations [v3]

[Sandhya Viswanathan]

> 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

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

  Implement review comments

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4039/files
  - new: https://git.openjdk.java.net/jdk/pull/4039/files/b506fc45..f318c0ee

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

  Stats: 372 lines in 31 files changed: 31 ins; 62 del; 279 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4039.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4039/head:pull/4039

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

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

[David Holmes]

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.


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 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:


   address review comments, add tests


Dear colleagues,

The updated PR addresses review comment regarding ThreadLocal as well 
as David' concern around the lock being held during 
JNI_OnLoad/JNI_OnUnload calls, and ensures all lock objects are 
deallocated. Multiple threads are allowed to enter 
NativeLibrary.load() to prevent any thread from locking while another 
thread loads a library. Before the update, there could be a class 
loading lock held by a parallel capable class loader, which can 
deadlock with the library loading lock. As proposed by David Holmes, 
the library loading lock was removed because dlopen/LoadLibrary are 
thread safe and they maintain internal reference counters on 
libraries. There's still a lock being held while a pair of containers 
are read/updated. It's not going to deadlock as there's no lock/wait 
operation performed while that lock is held. Multiple threads may 
create their own copies of NativeLibrary object and register it for 
auto unloading.


Tests for auto unloading were added along with the PR update. There 
are now 3 jtreg tests:

- one checks for deadlock, similar to the one proposed by Chris Hegarty
- two other tests are for library unload.

The major side effect of that multiple threads are allowed to enter 
is that JNI_OnLoad/JNI_OnUnload may be called multiple (but sam

Re: RFR: 8224243: Make AccessibleObject a sealed class

[Joe Darcy]

Hi Éamonn,

On 5/20/2021 2:59 PM, Éamonn McManus wrote:

Thanks for the tip, Alan. Guava's public Invokable

class does inherit from AccessibleObject. It would probably not be too hard
to make it stop doing so. That's technically a breaking API change, but the
class is marked @Beta so that's fair game.

I looked in Google's code base and I found one other project that extends
AccessibleObject, picocli, here
.
That's not to say that nobody else does it, but apparently cases are rare.

I do wonder what the purpose of the change would be. The constructor
javadoc text doesn't say you *can't* extend this class. Is there some
benefit from making it not work anymore?



The intent of the constructor's text to me implies the class is not 
intended to be subclassed outside of the JDK. (I'm not sure why the 
constructor isn't package private as all its subclasses are in the same 
package which would have accomplished that, but the SCM history from 
circa JDK 1.2 would need to be consulted for guidance.)


The original impetus for 8224243 was the implementation of certain 
overridable methods in AccessibleObject (getAnnotation, 
getAnnotationsByType, and getDeclaredAnnotations) not @implSpec'ing 
their behavior. The behavior is to thrown an error as the method should 
be overridden in the subclasses. If all the subclasses are within the 
JDK and the class is sealed, that @implSpec'ing need not occur.


If all the subclasses were in the JDK (which we know is not that case), 
it is preferable to avoid the @implSpec'ing throwing the exception as 
that is just an implementation-internal comment.


HTH,

-Joe

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

[Thiago Henrique Hüpner]

On Thu, 20 May 2021 22:28:56 GMT, Mandy Chung  wrote:

>> It's a permitted subclass of `ConstantDesc`, so it must be either `final`, 
>> `sealed`, or `non-sealed`. Making it `non-sealed` means it can still be 
>> extended.
>
> I should have made it clear.  I was expecting `DynamicConstantDesc` should be 
> `sealed`.  Do you expect non-JDK implementation class extending 
> `DynamicConstantDesc`?

>From the ConstantDesc Javadoc:

 * Non-platform classes should not implement {@linkplain ConstantDesc} 
directly.
 * Instead, they should extend {@link DynamicConstantDesc} (as {@link EnumDesc}
 * and {@link VarHandleDesc} do.)

-

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

Re: RFR: 8267056: tools/jpackage/share/RuntimePackageTest.java fails with NoSuchFileException [v2]

[Alexey Semenyuk]

On Thu, 20 May 2021 22:25:10 GMT, Alexander Matveev  
wrote:

>> For debug build on macOS, runtime which used for test fill be located in 
>> /path/jdk-17/fastdebug and /path/jdk-17 will not contain any other files 
>> except fastdebug and in this case our check to decide if we need to copy app 
>> or runtime will return /path/jdk-17 which is not correct. Fixed by skipping 
>> this check for runtime and only using it for app. Also, added ignoring 
>> .DS_Store to test which is needed if user used Finder to look inside runtime 
>> folder which will cause .DS_Store to be created and will cause test to fail, 
>> since this file is not being packaged.
>
> Alexander Matveev has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   8267056: tools/jpackage/share/RuntimePackageTest.java fails with 
> NoSuchFileException [v2]

Marked as reviewed by asemenyuk (Reviewer).

-

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

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

[Mandy Chung]

On Thu, 20 May 2021 22:17:03 GMT, Gavin Bierman  wrote:

>> src/java.base/share/classes/java/lang/constant/DynamicConstantDesc.java line 
>> 59:
>> 
>>> 57:  * @since 12
>>> 58:  */
>>> 59: non-sealed public abstract class DynamicConstantDesc
>> 
>> can you explain why `DynamicConstantDesc` is non-sealed?
>
> It's a permitted subclass of `ConstantDesc`, so it must be either `final`, 
> `sealed`, or `non-sealed`. Making it `non-sealed` means it can still be 
> extended.

I should have made it clear.  I was expecting `DynamicConstantDesc` should be 
`sealed`.  Do you expect non-JDK implementation class extending 
`DynamicConstantDesc`?

-

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

Re: RFR: 8267056: tools/jpackage/share/RuntimePackageTest.java fails with NoSuchFileException [v2]

[Alexander Matveev]

On Thu, 20 May 2021 16:15:21 GMT, Alexey Semenyuk  wrote:

>> Alexander Matveev has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   8267056: tools/jpackage/share/RuntimePackageTest.java fails with 
>> NoSuchFileException [v2]
>
> src/jdk.jpackage/macosx/classes/jdk/jpackage/internal/MacPkgBundler.java line 
> 396:
> 
>> 394: // if parent does not have any other files
>> 395: if (!StandardBundlerParam.isRuntimeInstaller(params)) {
>> 396: Path[] list = Files.list(rootDir).toArray(Path[]::new);
> 
> `Files.list()` requires try-with-resources construct.

Fixed. I also removed check for null, since I do not think that Files.list can 
return null. I think it was leftover when we used File APIs instead of Files.

-

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

Re: RFR: 8267056: tools/jpackage/share/RuntimePackageTest.java fails with NoSuchFileException [v2]

[Alexander Matveev]

> For debug build on macOS, runtime which used for test fill be located in 
> /path/jdk-17/fastdebug and /path/jdk-17 will not contain any other files 
> except fastdebug and in this case our check to decide if we need to copy app 
> or runtime will return /path/jdk-17 which is not correct. Fixed by skipping 
> this check for runtime and only using it for app. Also, added ignoring 
> .DS_Store to test which is needed if user used Finder to look inside runtime 
> folder which will cause .DS_Store to be created and will cause test to fail, 
> since this file is not being packaged.

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

  8267056: tools/jpackage/share/RuntimePackageTest.java fails with 
NoSuchFileException [v2]

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4120/files
  - new: https://git.openjdk.java.net/jdk/pull/4120/files/de575160..bce1c7e4

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

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

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

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

[Gavin Bierman]

On Thu, 20 May 2021 21:32:08 GMT, Mandy Chung  wrote:

>> Gavin Bierman has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Removing javadoc comments about future use of sealed
>
> src/java.base/share/classes/java/lang/constant/DynamicConstantDesc.java line 
> 59:
> 
>> 57:  * @since 12
>> 58:  */
>> 59: non-sealed public abstract class DynamicConstantDesc
> 
> can you explain why `DynamicConstantDesc` is non-sealed?

It's a permitted subclass of ConstantDesc, so it must be either `final`, 
`sealed`, or `non-sealed`. Making it `non-sealed` means it can still be 
extended.

-

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

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

[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:

  Removing file constants.patch added by mistake

-

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

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

  Stats: 102 lines in 1 file changed: 0 ins; 102 del; 0 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: 8265130: Make ConstantDesc class hierarchy sealed [v2]

[Mandy Chung]

On Thu, 20 May 2021 21:32:05 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:
> 
>   Removing javadoc comments about future use of sealed

Looks like `constants.patch` is accidentally added in this commit.  That should 
be removed.

src/java.base/share/classes/java/lang/constant/DynamicConstantDesc.java line 59:

> 57:  * @since 12
> 58:  */
> 59: non-sealed public abstract class DynamicConstantDesc

can you explain why `DynamicConstantDesc` is non-sealed?

-

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

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

[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:

  Removing javadoc comments about future use of sealed

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4135/files
  - new: https://git.openjdk.java.net/jdk/pull/4135/files/8084e90b..1cd54624

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

  Stats: 131 lines in 6 files changed: 102 ins; 29 del; 0 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: 8191441: (Process) add Readers and Writer access to java.lang.Process streams

[Bernd Eckenfels]

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: 8267190: Optimize Vector API test operations [v2]

[Paul Sandoz]

On Thu, 20 May 2021 01:27:48 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
>
> Sandhya Viswanathan has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Implement Paul's review comments

Java changes are good, some minor comments if you choose to accept them, no 
need for me to review further.

src/jdk.incubator.vector/share/classes/jdk/incubator/vector/X-VectorBits.java.template
 line 852:

> 850: private final 
> 851: VectorMask defaultMaskCast(AbstractSpecies dsp) {
> 852: boolean[] maskArray = toArray();

Can you add an `assert length() != species.laneCount()`?

src/jdk.incubator.vector/share/classes/jdk/incubator/vector/X-VectorBits.java.template
 line 854:

> 852: boolean[] maskArray = toArray();
> 853: // enum-switches don't optimize properly JDK-8161245
> 854: return (

Minor syntactic quibble: you don't need the '(` and `)` surrounding the switch 
expressions e.g.:

   return switch (dsp.laneType.switchKey) {
   case ...
   }

-

Marked as reviewed by psandoz (Reviewer).

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

RFR: 8265130: Make ConstantDesc class hierarchy sealed

[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

-

Commit messages:
 - 8265130: Make ConstantDesc class hierarchy sealed

Changes: https://git.openjdk.java.net/jdk/pull/4135/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=4135&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8265130
  Stats: 25 lines in 6 files changed: 16 ins; 0 del; 9 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: 8191441: (Process) add Readers and Writer access to java.lang.Process streams

[Naoto Sato]

On Thu, 20 May 2021 20:46:36 GMT, Roger Riggs  wrote:

>> 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.

Lucky you :-)
I wasn't so lucky that I got an intermittent issue 
(https://bugs.openjdk.java.net/browse/JDK-8265918), where it only fails if the 
test is run on an Ubuntu CI test machine. It's not only depending on Linux 
distros, but on the user's configuration (minimal install or full, etc.), so I 
don't think there is any safe subset.
That said, the test code uses those encoding names as `sun.stdout/err.encoding` 
properties values, not setting the real native encoding, so the test should be 
fine. Only the argument name in the test (`nativeEncoding`) is a bit confusing.

-

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

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

[Roger Riggs]

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: 8191441: (Process) add Readers and Writer access to java.lang.Process streams

[Naoto Sato]

On Thu, 20 May 2021 19:57:22 GMT, Roger Riggs  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.

Good to see `native.encoding` is utilized. Some comments follow.

src/java.base/share/classes/java/lang/Process.java line 25:

> 23:  * questions.
> 24:  */
> 25: 

Copyright year -> 2021

src/java.base/share/classes/java/lang/Process.java line 181:

> 179:  * @return a {@link BufferedReader BufferedReader} using the {@code 
> native.encoding} if supported,
> 180:  *  otherwise, the {@link Charset#defaultCharset()}
> 181:  */

`@since 17` is needed (and for other public methods)

src/java.base/share/classes/java/lang/Process.java line 771:

> 769: 
> 770: /**
> 771:  * {@return Charset for the native encoding or {@link 
> Charset#defaultCharset()}

Need some edit here?

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.

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.

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?

-

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

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

[Roger Riggs]

On Thu, 20 May 2021 20:12:07 GMT, Rémi Forax  wrote:

> I don't think that using PrintWriter is a good idea here given that a 
> PrintWriter shallow the IOException

Good point, but...

PrintStream does also and it is used frequently for Stdout and Stderr. 

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.

-

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 19:57:22 GMT, Roger Riggs  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.

I've added the same comment on the bug itself.
I don't think that using PrintWriter is a good idea here given that a 
PrintWriter shallow the IOException

-

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

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

[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.

-

Commit messages:
 - 8191441: (Process) add Readers and Writer access to java.lang.Process streams

Changes: https://git.openjdk.java.net/jdk/pull/4134/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=4134&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8191441
  Stats: 416 lines in 2 files changed: 412 ins; 0 del; 4 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: RFR: 8264859: Implement Context-Specific Deserialization Filters [v3]

[Daniel Fuchs]

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/ObjectInputStream.java line 201:

> 199:  * when a filter is set for a stream.
> 200:  * The filter factory determines the filter to be used for each 
> stream based
> 201:  * on its inputs, thread context, other filters, or state that is 
> available.

Maybe a link to the ObjectInputFilter API documentation where it is explained 
what the two filters passed to the factory are in each of these cases should be 
provided here.

Namely: 

- in the constructor, `factory.apply(null, Config.getSerialFilter())` is 
invoked.
- in `setObjectInputFilter(newfilter)`, `factory.apply(filter, newFilter)` is 
invoked - where `filter` is the filter that the stream is currently using.

Or maybe link to the constructor and setObjectInputFilter method where this is 
explained.

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

> 202:  * If a JVM-wide filter factory is not set, a builtin 
> deserialization filter factory
> 203:  * provides the {@link Config#getSerialFilter static JVM-wide 
> filter} when invoked from the
> 204:  * {@link ObjectInputStream#ObjectInputStream(InputStream) 
> ObjectInputStream constructors}

These two links should be `{@linkplain ...}`

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

> 1253:  * Returns the serialization filter for this stream.
> 1254:  * The filter is the result of invoking the
> 1255:  * {@link Config#getSerialFilterFactory() JVM-wide filter factory}

`{@linkplain }`

-

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

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

[Daniel Fuchs]

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 410:

> 408:  * The class must have a public zero-argument constructor, implement 
> the
> 409:  * {@link BinaryOperator} interface,
> 410:  * and provide its implementation.

I wonder if some more explanation is needed to explain how the filter factory 
class is located. I mean - I expect it will be looked up using the System 
ClassLoader - doesn't this need to be specified? Or if it's already specified 
elsewhere then maybe a link should be provided?

Maybe the spec should also say that the class should be public. And I guess 
that if it is in another module then it should be in an exported package - or 
possibly in a package opened to reflection? Or does this only work if the class 
is in the unnamed module (deployed on the classpath)?

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

> 429:  * Used as a system property and a java.security.Security 
> property.
> 430:  */
> 431: private final static String SERIAL_FILTER_PROPNAME = 
> "jdk.serialFilter";

should be `private static final` (in canonical order).

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

> 587: /**
> 588:  * Returns the JVM-wide deserialization filter factory.
> 589:  * If the filter factory has been {@link 
> #setSerialFilterFactory(BinaryOperator) set} it is returned,

Should be `{@linkplain ...}`

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

> 590:  * otherwise, a builtin deserialization filter factory is 
> returned.
> 591:  * The filter factory provides a filter for every 
> ObjectInputStream when invoked from
> 592:  * {@link ObjectInputStream#ObjectInputStream(InputStream) 
> ObjectInputStream constructors}

Should be {@linkplain ...}

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

> 599:  * {@link ObjectInputStream#ObjectInputStream(InputStream) 
> ObjectInputStream constructors}.
> 600:  * When invoked {@link 
> ObjectInputStream#setObjectInputFilter(ObjectInputFilter)
> 601:  * to set the stream-specific filter} the requested filter 
> replaces the static JVM-wide filter,

These three should be {@linkplain ...} too.

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

> 601:  * to set the stream-specific filter} the requested filter 
> replaces the static JVM-wide filter,
> 602:  * unless it has already been set. The stream-specific filter 
> can only be set once,
> 603:  * if it has already been set, an {@link IllegalStateException} 
> is thrown.

I am assuming the `IllegalStateException` exception is thrown by 
`ObjecInputStream::setObjectInputFilter` - maybe that should be clarified here.

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

> 607:  *
> 608:  * @return the JVM-wide deserialization filter factory; non-null
> 609:  * @since TBD

Need to replace TBD before pushing ;-)

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

> 630:  * The factory determines the filter to be used for {@code 
> ObjectInputStream} streams based
> 631:  * on its inputs, and any other filters, context, or state that 
> is available.
> 632:  * The factory may throw runtime exceptions to signal incorrect 
> use or invalid parameters.

What happens if the factory throws an exception?

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

> 637:  * @throws SecurityException if there is security manager and the
> 638:  *   {@code SerializablePermission("serialFilter")} is not 
> granted
> 639:  * @since TBD

... TBD ...

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

> 754:  * 
> 755:  * ObjectInputFilter f = allowFilter(cl -> 
> cl.getClassLoader() == ClassLoader.getPlatformClassLoader()
> 756:  *  || 
> cl.getClassLoader() == null);

The example here seems to be missing the `otherStatus` p

Re: Suppressed IllegalAccessException in MethodHandle.setVarargs

[Mandy Chung]

This seems a good improvement.  I created 
https://bugs.openjdk.java.net/browse/JDK-8267509 for this issue.


How did you run into this illegal varargs method?  Is that class file 
generated at runtime?


Mandy

On 5/20/21 10:18 AM, x4e_x4e wrote:

Hello,

If a MethodHandles.Lookup is used to locate a MethodHandle for a varargs method 
who's final parameter is not an array an IllegalArgumentException will be 
thrown:


java.lang.IllegalArgumentException: not an array type: int
  at 
java.base/java.lang.invoke.MethodHandleStatics.newIllegalArgumentException(MethodHandleStatics.java:138)
  at 
java.base/java.lang.invoke.MethodHandle.spreadArrayChecks(MethodHandle.java:1066)
  at 
java.base/java.lang.invoke.MethodHandle.asCollectorChecks(MethodHandle.java:1259)
  at 
java.base/java.lang.invoke.MethodHandle.asVarargsCollector(MethodHandle.java:1427)
  at 
java.base/java.lang.invoke.MethodHandle.withVarargs(MethodHandle.java:)
  at 
java.base/java.lang.invoke.MethodHandle.setVarargs(MethodHandle.java:1634)
      at 
java.base/java.lang.invoke.MethodHandles$Lookup.getDirectMethodCommon(MethodHandles.java:3755)
  at 
java.base/java.lang.invoke.MethodHandles$Lookup.getDirectMethod(MethodHandles.java:3691)
  at 
java.base/java.lang.invoke.MethodHandles$Lookup.findStatic(MethodHandles.java:2399)

This makes sense, however it seems that the exception is swallowed and instead 
a generic IllegalAccessException is thrown:


Caused by: java.lang.IllegalAccessException: cannot make variable arity: 
package.Class.method(Object[],int)Object/invokeStatic
 at 
java.base/java.lang.invoke.MemberName.makeAccessException(MemberName.java:957)
 at 
java.base/java.lang.invoke.MethodHandle.setVarargs(MethodHandle.java:1636)
 at 
java.base/java.lang.invoke.MethodHandles$Lookup.getDirectMethodCommon(MethodHandles.java:3755)
 at 
java.base/java.lang.invoke.MethodHandles$Lookup.getDirectMethod(MethodHandles.java:3691)
 at 
java.base/java.lang.invoke.MethodHandles$Lookup.findStatic(MethodHandles.java:2399)

It would probably be helpful to make the IllegalAccessException caused by the 
IllegalArgumentException, or at the very least add it as a suppressed exception.
Otherwise the only way to find the helpful exception here is through runtime 
debugging.

Relevant code: 
https://github.com/openjdk/jdk/blob/7b98400c81900a8c779394d549b5fb61f1dd8638/src/java.base/share/classes/java/lang/invoke/MethodHandle.java#L1635

Thanks,
x4e

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

[Kevin Rushforth]

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

This isn't a review of the code changes, although I did take a quick look at 
the manual changes, and they look fine.

I did a local build of the PR branch on Windows, Mac, and Linux, and then did a 
build / test of JavaFX using that locally built JDK to find all the places 
where I need to add `-Djava.security.manager=allow` to the JavaFX tests to fix 
[JDK-8264140](https://bugs.openjdk.java.net/browse/JDK-8264140). It's working 
as expected.

-

Marked as reviewed by kcr (Author).

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

Make java.lang.constant.ConstantDesc selead

[Thiago Henrique Hupner]

Now that Sealed classes are integrated, should be the moment to seal the
ConstantDesc class and its subclasses, or would be better to wait a little
bit more?

Best regards

Thiago

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

[Daniel Fuchs]

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 197:

> 195:  * }
> 196:  * }
> 197:  * Using the Filter Factory

Should this be rather a subsection of the example above? Otherwise it gives the 
impression that the `doWithSerialFilter` method is provided or invoked by the 
JDK.

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

> 261: /**
> 262:  * Returns a filter, that when applied to this filter that is 
> checking a class, maps
> 263:  * {@code Status.UNDECIDED} to {@code Status.REJECTED}, otherwise 
> returns the status of the other filter.

Which `other filter` is that? Shouldn't that be `..., otherwise returns the 
status of this filter.`

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

> 263:  * {@code Status.UNDECIDED} to {@code Status.REJECTED}, otherwise 
> returns the status of the other filter.
> 264:  * Object serialization accepts a class if the filter returns {@code 
> UNDECIDED}.
> 265:  * Appending a filter to reject undecided results for classes that 
> have not been

Could this be rephrased? I am not sure what "Appending" means in this context: 
more specifically appending to what? Maybe there's a way to  describe that in 
terms of "Mapping" rather than "Appending"?

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

> 362: /**
> 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.

FWIW: In other APIs we talk of `system-wide` rather than `JVM-wide`.

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

> 369:  * When each {@link ObjectInputStream#ObjectInputStream() 
> ObjectInputStream}
> 370:  * is created the {@linkplain Config#getSerialFilterFactory() filter 
> factory}
> 371:  * is invoked to determine the initial filter for the stream. A 
> stream-specific filter can be set with

Maybe the concept of system-wide filter should be introduced first, then you 
could say that the factory is invoked with `null` as first parameter and the 
system-wide filter as second parameter when the `ObjectInputStream` is created. 
Then the next paragraph could expand on what happens when 
`setObjectInputFilter` is called.

-

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

Re: RFR: 8264774: Implementation of Foreign Function and Memory API (Incubator) [v24]

[Alan Bateman]

On Wed, 28 Apr 2021 08:20:05 GMT, Chris Hegarty  wrote:

>> src/java.base/share/classes/sun/nio/ch/IOUtil.java line 466:
>> 
>>> 464: }
>>> 465: 
>>> 466: private static final JavaNioAccess NIO_ACCESS = 
>>> SharedSecrets.getJavaNioAccess();
>> 
>> It might be cleaner to move to acquire/release methods to their own 
>> supporting class as it's not really IOUtil.
>
> I went back and forth on this a number of times already. I think where we 
> landed is a reasonable place, given the current shape of the code.
> 
> Scope is a private property of Buffer, and as such should be consulted 
> anywhere where a buffer's address is being accessed. In fact, a prior 
> prototype would not allow access to the underlying address value without the 
> caller passing a valid handle for the buffer view's scope. It's hard to find 
> the sweet-spot here between code reuse and safety, but the high-order bit is 
> that the code accessing the address is auditable and testable to avoid 
> accessing memory unsafely. Maybe there is a better alternative implementation 
> code structure (at the cost of some duplication), but it is not obvious to me 
> what that is (and I have given it some thought). Suggestions welcome.
> 
> Note, there is a little more follow-on work to be done in this area, if we 
> are to expand support to other non-TCP channel implementations. Maybe 
> investigation into possible code refactorings could be done as part of that?

Can you create a follow-on issue to re-visit the changes to IOUtil? The changes 
in this area that are in this PR will need to re-worked so that it more cleanly 
separate the synchronous and asynchronous usages.

-

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

Re: RFR: 8224243: Make AccessibleObject a sealed class

[Alan Bateman]

On Thu, 20 May 2021 17:14:57 GMT, Joe Darcy  wrote:

> Conceptually, AccessbileObject is a sealed class with a protected constructor 
> stating
> 
> Constructor: only used by the Java Virtual Machine.
> 
> With the language now supporting sealed classes, the AccessbileObject should 
> be marked as sealed.
> 
> Executable and Field are the subclasses of AccessbileObject in the JDK; as 
> Executable has subclasses, it is marked as non-sealed.
> 
> Please also review the corresponding CSR:
> 
> https://bugs.openjdk.java.net/browse/JDK-8224243

I think this will require reaching out to Google Guava, I think its their 
Invocable API that extends AccessibleObject outside of the JDK. We ran this 
when doing the module system where we didn't initially take into account 
sub-classes that were outside of java.base.

-

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

RFR: 8224243: Make AccessibleObject a sealed class

[Joe Darcy]

Conceptually, AccessbileObject is a sealed class with a protected constructor 
stating

Constructor: only used by the Java Virtual Machine.

With the language now supporting sealed classes, the AccessbileObject should be 
marked as sealed.

Executable and Field are the subclasses of AccessbileObject in the JDK; as 
Executable has subclasses, it is marked as non-sealed.

Please also review the corresponding CSR:

https://bugs.openjdk.java.net/browse/JDK-8224243

-

Commit messages:
 - 8224243: Make AccessibleObject a sealed class

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

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

Re: RFR: 8246677: LinkedTransferQueue and SynchronousQueue synchronization updates [v2]

[Evgeny Astigeevich]

On Tue, 8 Dec 2020 05:54:24 GMT, Martin Buchholz  wrote:

>> 8246677: LinkedTransferQueue and SynchronousQueue synchronization updates
>
> Martin Buchholz has refreshed the contents of this pull request, and previous 
> commits have been removed. The incremental views will show differences 
> compared to the previous content of the PR.

This PR causes 16x performance regression in SynchronousQueue. Details are in 
https://bugs.openjdk.java.net/browse/JDK-8267502

-

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

Re: RFR: 8267056: tools/jpackage/share/RuntimePackageTest.java fails with NoSuchFileException

[Alexey Semenyuk]

On Wed, 19 May 2021 21:00:07 GMT, Alexander Matveev  
wrote:

> For debug build on macOS, runtime which used for test fill be located in 
> /path/jdk-17/fastdebug and /path/jdk-17 will not contain any other files 
> except fastdebug and in this case our check to decide if we need to copy app 
> or runtime will return /path/jdk-17 which is not correct. Fixed by skipping 
> this check for runtime and only using it for app. Also, added ignoring 
> .DS_Store to test which is needed if user used Finder to look inside runtime 
> folder which will cause .DS_Store to be created and will cause test to fail, 
> since this file is not being packaged.

Changes requested by asemenyuk (Reviewer).

src/jdk.jpackage/macosx/classes/jdk/jpackage/internal/MacPkgBundler.java line 
396:

> 394: // if parent does not have any other files
> 395: if (!StandardBundlerParam.isRuntimeInstaller(params)) {
> 396: Path[] list = Files.list(rootDir).toArray(Path[]::new);

`Files.list()` requires try-with-resources construct.

-

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

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

[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:

  Simplify factory interface to BinaryOperator and cleanup 
the example

-

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

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

  Stats: 193 lines in 4 files changed: 135 ins; 9 del; 49 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: 8266310: deadlock while loading the JNI code [v2]

[Peter Levart]

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 ...


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 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:


   address review comments, add tests


Dear colleagues,

The updated PR addresses review comment regarding ThreadLocal as well 
as David' concern around the lock being held during 
JNI_OnLoad/JNI_OnUnload calls, and ensures all lock objects are 
deallocated. Multiple threads are allowed to enter 
NativeLibrary.load() to prevent any thread from locking while another 
thread loads a library. Before the update, there could be a class 
loading lock held by a parallel capable class loader, which can 
deadlock with the library loading lock. As proposed by David Holmes, 
the library loading lock was removed because dlopen/LoadLibrary are 
thread safe and they maintain internal reference counters on 
libraries. There's still a lock being held while a pair of containers 
are read/updated. It's not going to deadlock as there's no lock/wait 
operation performed while that lock is held. Multiple threads may 
create their own copies of NativeLibrary object and register it for 
auto unloading.


Tests for auto unloading were added along with the PR update. There 
are now 3 jtreg tests:

- one checks for deadlock, similar to the one proposed by Chris Hegarty
- two other tests are for library unload.

The major side effect of that multiple threads are allowed to enter 
is that JNI_OnLoad/JNI_OnUnload may be called multiple (but same) 
number of times from concurrent threads. In particular, the number of 
calls to JNI_OnLoad must be equal to the number of calls to 
JNI_OnUnload after the relevant class loader is garbage collected. 
This may affect the behaviour that relies on specific order or the 
number of JNI_OnLoad/JNI_OnUnload calls. The current JNI 
specification does not mandate how many times JNI_OnLoad/JNI_OnUnload 
are called. Also, we could not locate tests in jck/jtreg/vmTestbase 
that would rely on the specific order or number of calls to 
JNI_OnLoad/JNI_OnUnload.


But you can't make such a change! That was my point. To fix the 
deadlock we must not hold a lock. But we must ensure only a single 
call to JNI_OnLoad is possible. It is an unsolvable problem with those 
constraints. You can't just change the behaviour of JNI_OnLoad like that.


David
-



If this is really a problem that several people are facing, then perhaps 
a change in the API could solve it. 

Integrated: 8261880: Change nested classes in java.base to static nested classes where possible

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

On Tue, 16 Feb 2021 14:30:58 GMT, Сергей Цыпанов 
 wrote:

> Non-static classes hold a link to their parent classes, which in many cases 
> can be avoided.

This pull request has now been integrated.

Changeset: 9425d3de
Author:Sergey Tsypanov 
Committer: Claes Redestad 
URL:   
https://git.openjdk.java.net/jdk/commit/9425d3de83fe8f4caddef03ffa3f4dd4de58f236
Stats: 15 lines in 11 files changed: 0 ins; 0 del; 15 mod

8261880: Change nested classes in java.base to static nested classes where 
possible

Reviewed-by: redestad

-

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

Re: RFR: 8261880: Change nested classes in java.base to static nested classes where possible [v2]

[Martin Desruisseaux]

On Wed, 17 Feb 2021 16:38:03 GMT, Сергей Цыпанов 
 wrote:

>> Non-static classes hold a link to their parent classes, which in many cases 
>> can be avoided.
>
> Сергей Цыпанов has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   8261880: Remove static from declarations of Holder nested classes

Just for information there is similar issues in 
`javax.imageio.metadata.IIOMetadataFormatImpl` class in the `java.desktop` 
module.

-

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

Re: RFR: 8264774: Implementation of Foreign Function and Memory API (Incubator) [v24]

[Jorn Vernee]

On Thu, 20 May 2021 13:00:14 GMT, Maurizio Cimadamore  
wrote:

>> Maurizio Cimadamore 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 35 
>> additional commits since the last revision:
>> 
>>  - Add sealed modifiers in morally sealed API interfaces
>>  - Merge branch 'master' into JEP-412
>>  - Fix VaList test
>>Remove unused code in Utils
>>  - Merge pull request #11 from JornVernee/JEP-412-MXCSR
>>
>>Add MXCSR save and restore to upcall stubs for non-windows platforms
>>  - Add MXCSR save and restore to upcall stubs for non-windows platforms
>>  - Merge branch 'master' into JEP-412
>>  - Fix issue with bounded arena allocator
>>  - Rename is_entry_blob to is_optimized_entry_blob
>>Rename as_entry_blob to as_optimized_entry_blob
>>Fix misc typos and indentation issues
>>  - Take care of remaining references to "Panama"
>>  - * Replace is_panama_entry_frame() with is_optimized_entry_frame()
>>* Replace EntryBlob with OptimizedEntryBlob
>>  - ... and 25 more: 
>> https://git.openjdk.java.net/jdk/compare/26a8cf83...e927c235
>
> src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemoryLayout.java
>  line 200:
> 
>> 198:  * Implementations of this interface are immutable, thread-safe and > href="{@docRoot}/java.base/java/lang/doc-files/ValueBased.html">value-based.
>> 199:  */
>> 200: public sealed interface MemoryLayout extends Constable permits 
>> AbstractLayout, SequenceLayout, GroupLayout, PaddingLayout, ValueLayout {
> 
> In principle we could just permit AbstractLayout and be done. In practice, 
> the javadoc comes out better if we list all the concrete API interfaces which 
> extend MemoryLayout, as the `permit` list will be displayed in the javadoc.

At least the internal class name is hidden in the javadoc:
![image](https://user-images.githubusercontent.com/5962029/118983890-37042080-b97d-11eb-9ee0-ae5d5db5cd7e.png)

-

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

Re: RFR: 8264774: Implementation of Foreign Function and Memory API (Incubator) [v24]

[Maurizio Cimadamore]

On Thu, 20 May 2021 13:05:22 GMT, Maurizio Cimadamore  
wrote:

>> This PR contains the API and implementation changes for JEP-412 [1]. A more 
>> detailed description of such changes, to avoid repetitions during the review 
>> process, is included as a separate comment.
>> 
>> [1] - https://openjdk.java.net/jeps/412
>
> Maurizio Cimadamore 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 35 additional 
> commits since the last revision:
> 
>  - Add sealed modifiers in morally sealed API interfaces
>  - Merge branch 'master' into JEP-412
>  - Fix VaList test
>Remove unused code in Utils
>  - Merge pull request #11 from JornVernee/JEP-412-MXCSR
>
>Add MXCSR save and restore to upcall stubs for non-windows platforms
>  - Add MXCSR save and restore to upcall stubs for non-windows platforms
>  - Merge branch 'master' into JEP-412
>  - Fix issue with bounded arena allocator
>  - Rename is_entry_blob to is_optimized_entry_blob
>Rename as_entry_blob to as_optimized_entry_blob
>Fix misc typos and indentation issues
>  - Take care of remaining references to "Panama"
>  - * Replace is_panama_entry_frame() with is_optimized_entry_frame()
>* Replace EntryBlob with OptimizedEntryBlob
>  - ... and 25 more: 
> https://git.openjdk.java.net/jdk/compare/354e6edf...e927c235

Latest javadoc:
http://cr.openjdk.java.net/~mcimadamore/JEP-412/v3/javadoc/jdk/incubator/foreign/package-summary.html
Latest specdiff:
http://cr.openjdk.java.net/~mcimadamore/JEP-412/v3/specdiff/overview-summary.html

-

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

Re: RFR: 8261880: Change nested classes in java.base to static nested classes where possible [v2]

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

On Thu, 20 May 2021 10:42:49 GMT, Claes Redestad  wrote:

>> Сергей Цыпанов has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   8261880: Remove static from declarations of Holder nested classes
>
> Marked as reviewed by redestad (Reviewer).

@cl4es now you can sponsor :)

-

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

Re: RFR: 8264774: Implementation of Foreign Function and Memory API (Incubator) [v24]

[Maurizio Cimadamore]

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

Maurizio Cimadamore 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 35 additional commits 
since the last revision:

 - Add sealed modifiers in morally sealed API interfaces
 - Merge branch 'master' into JEP-412
 - Fix VaList test
   Remove unused code in Utils
 - Merge pull request #11 from JornVernee/JEP-412-MXCSR
   
   Add MXCSR save and restore to upcall stubs for non-windows platforms
 - Add MXCSR save and restore to upcall stubs for non-windows platforms
 - Merge branch 'master' into JEP-412
 - Fix issue with bounded arena allocator
 - Rename is_entry_blob to is_optimized_entry_blob
   Rename as_entry_blob to as_optimized_entry_blob
   Fix misc typos and indentation issues
 - Take care of remaining references to "Panama"
 - * Replace is_panama_entry_frame() with is_optimized_entry_frame()
   * Replace EntryBlob with OptimizedEntryBlob
 - ... and 25 more: https://git.openjdk.java.net/jdk/compare/788875f9...e927c235

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3699/files
  - new: https://git.openjdk.java.net/jdk/pull/3699/files/f5668ffc..e927c235

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3699&range=23
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3699&range=22-23

  Stats: 7087 lines in 360 files changed: 4302 ins; 1796 del; 989 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3699.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3699/head:pull/3699

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

Re: RFR: 8264774: Implementation of Foreign Function and Memory API (Incubator) [v24]

[Maurizio Cimadamore]

On Thu, 20 May 2021 13:00:15 GMT, Maurizio Cimadamore  
wrote:

>> This PR contains the API and implementation changes for JEP-412 [1]. A more 
>> detailed description of such changes, to avoid repetitions during the review 
>> process, is included as a separate comment.
>> 
>> [1] - https://openjdk.java.net/jeps/412
>
> Maurizio Cimadamore 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 35 additional 
> commits since the last revision:
> 
>  - Add sealed modifiers in morally sealed API interfaces
>  - Merge branch 'master' into JEP-412
>  - Fix VaList test
>Remove unused code in Utils
>  - Merge pull request #11 from JornVernee/JEP-412-MXCSR
>
>Add MXCSR save and restore to upcall stubs for non-windows platforms
>  - Add MXCSR save and restore to upcall stubs for non-windows platforms
>  - Merge branch 'master' into JEP-412
>  - Fix issue with bounded arena allocator
>  - Rename is_entry_blob to is_optimized_entry_blob
>Rename as_entry_blob to as_optimized_entry_blob
>Fix misc typos and indentation issues
>  - Take care of remaining references to "Panama"
>  - * Replace is_panama_entry_frame() with is_optimized_entry_frame()
>* Replace EntryBlob with OptimizedEntryBlob
>  - ... and 25 more: 
> https://git.openjdk.java.net/jdk/compare/629f67e6...e927c235

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemoryLayout.java 
line 200:

> 198:  * Implementations of this interface are immutable, thread-safe and  href="{@docRoot}/java.base/java/lang/doc-files/ValueBased.html">value-based.
> 199:  */
> 200: public sealed interface MemoryLayout extends Constable permits 
> AbstractLayout, SequenceLayout, GroupLayout, PaddingLayout, ValueLayout {

In principle we could just permit AbstractLayout and be done. In practice, the 
javadoc comes out better if we list all the concrete API interfaces which 
extend MemoryLayout, as the `permit` list will be displayed in the javadoc.

-

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

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

[Aleksei Voitylov]

On Wed, 19 May 2021 16:29:33 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 trailing whitespace

Sorry, let me get back to this after I talk with the chalkboard.

-

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

Re: RFR: 8261880: Change nested classes in java.base to static nested classes where possible [v2]

[Claes Redestad]

On Wed, 17 Feb 2021 16:38:03 GMT, Сергей Цыпанов 
 wrote:

>> Non-static classes hold a link to their parent classes, which in many cases 
>> can be avoided.
>
> Сергей Цыпанов has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   8261880: Remove static from declarations of Holder nested classes

Marked as reviewed by redestad (Reviewer).

-

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

RFR: 8267452: Delegate forEachRemaining in Spliterators.iterator()

[Tagir F . Valeev]

Sometimes, `Spliterator::forEachRemaining` has much more optimized 
implementation, compared to `Spliterator::tryAdvance`. For example, if a 
`Stream::spliterator` called for a stream that has intermediate operations, 
`tryAdvance()` may buffer intermediate elements while `forEachRemaining()` just 
delegates to the `wrapAndCopyInto` (see 
`StreamSpliterators.AbstractWrappingSpliterator` and its inheritors).

`Spliterators::iterator` methods (used in particular by `Stream::iterator`) 
provide adapter iterators that delegate to the supplied spliterator. 
Unfortunately, they don't have a specialized implementation for 
`Iterator::forEachRemaining`. As a result, a default implementation is used 
that delegates to `hasNext`/`next`, which in turn causes the delegation to 
`tryAdvance`. It's quite simple to implement `Iterator::forEachRemaining` here, 
taking advantage of possible spliterator optimizations if the iterator client 
decides to use `forEachRemaining`.

This patch implements Iterator::forEachRemaining in Spliterators::iterator 
methods. Also, I nullize the `nextElement` in `Iterator::next` to avoid 
accidental prolongation of the user's object lifetime when iteration is 
finished. Finally, a small cleanup: I added the `final` modifier where possible 
to private fields in `Spliterators`.

Test-wise, some scenarios are already covered by 
SpliteratorTraversingAndSplittingTest. However, the resulting iterator is 
always wrapped into `Spliterators::spliterator`, so usage scenarios are 
somewhat limited. In particular, calling `hasNext` (without `next`) before 
`forEachRemaining` was not covered there. I added more tests in 
`IteratorFromSpliteratorTest` to cover these scenarios. I checked that removing 
`valueReady = false;` or `action.accept(t);` lines from newly implemented 
`forEachRemaining` method causes new tests to fail (but old tests don't fail 
due to this).

-

Commit messages:
 - 8267452: Linebreaks fixed
 - 8267452: @bug tag
 - 8267452: Delegate forEachRemaining in Spliterators.iterator()

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

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

Integrated: 8260517: implement Sealed Classes as a standard feature in Java

[Vicente Romero]

On Fri, 16 Apr 2021 01:08:57 GMT, Vicente Romero  wrote:

> Please review this PR that intents to make sealed classes a final feature in 
> Java. This PR contains compiler and VM changes. In line with similar PRs, 
> which has made preview features final, this one is mostly removing preview 
> related comments from APIs plus options in test cases. Please also review the 
> related [CSR](https://bugs.openjdk.java.net/browse/JDK-8265090)
> 
> Thanks
> 
> note: this PR is related to 
> [PR-3528](https://github.com/openjdk/jdk/pull/3528) and must be integrated 
> after it.

This pull request has now been integrated.

Changeset: 0fa9223f
Author:Vicente Romero 
URL:   
https://git.openjdk.java.net/jdk/commit/0fa9223f34bc33635079763362f42f0a5c53759b
Stats: 444 lines in 54 files changed: 51 ins; 275 del; 118 mod

8260517: implement Sealed Classes as a standard feature in Java

Co-authored-by: Harold Seigel 
Co-authored-by: Vicente Romero 
Reviewed-by: dholmes, mcimadamore, jlahoda

-

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

Re: JEP draft: Scope Locals

[Andrew Haley]

On 5/19/21 10:51 PM, David Lloyd wrote:
> I think it would be really nice if the snapshot class could hold its
> run/call method rather than making it a static method of `ScopeLocal`. This
> reads more naturally to me (and is nicer to write):

True, but inheritance is *extremely* time-critical when creating a
ForkJoin task. In many cases there won't be any scope locals to inherit,
and by allowing null snapshots you save a significant fraction of a
nanosecond when creating one. And I know, this is hard to believe, but
such an overhead has a significant macroscopic effect on benchmarks.

However, I do intend to fix this in a different way, if I can.

-- 
Andrew Haley  (he/him)
Java Platform Lead Engineer
Red Hat UK Ltd. 
https://keybase.io/andrewhaley
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671

Re: JEP draft: Scope Locals

[Andrew Haley]

On 5/19/21 9:55 PM, David Lloyd wrote:
> Turning this around, I would argue that there are few (or perhaps *no*)
> cases where it would ever be desirable to inherit scope locals across
> thread creation; in cases where this is explicitly desired, one can always
> resume the snapshot from within the thread's Runnable. Was there a
> particular use case this was meant to address?

Structured Concurrency, but also possibly anywhere that inheritable
thread locals are used now.

-- 
Andrew Haley  (he/him)
Java Platform Lead Engineer
Red Hat UK Ltd. 
https://keybase.io/andrewhaley
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671

Re: JEP draft: Scope Locals

[Andrew Haley]

On 5/19/21 5:55 PM, Peter Levart wrote:
> In other words, non-inheritable bindings are 
> never transferred from thread to thread automatically or by 
> snapshot/runWithSnapshot. I can see that snapshot/runWithSnapshot was 
> meant as a mechanism to "simulate" inheritance of bindings when 
> execution is transferred from one thread to another which is not a newly 
> started child thread.

Yes. However, this part of the draft proposal is undergoing some revision,
and it looks like it will make more sense to control inheritance in a
different way, one that will permit more flexible control over what
gets inherited and when.

-- 
Andrew Haley  (he/him)
Java Platform Lead Engineer
Red Hat UK Ltd. 
https://keybase.io/andrewhaley
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671

Re: JEP draft: Scope Locals

[Andrew Haley]

On 5/15/21 6:15 PM, Remi Forax wrote:
> I think the JEP should be more explicit about the shortcoming of ThreadLocal 
> and how the design of scope local fix both issues.

Yes. It's in progress.

-- 
Andrew Haley  (he/him)
Java Platform Lead Engineer
Red Hat UK Ltd. 
https://keybase.io/andrewhaley
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671

Re: [External] : Re: ReversibleCollection proposal

[Peter Levart]

On 20/05/2021 08:35, dfranken@gmail.com wrote:

I also think the proposal of Stuart is reasonable though, but it seemed
to me that we had reached some sort of impasse in this discussion. As
Remi points out, we have (default) methods which sometimes throw
exceptions when they are implemented to signify they don't actually
implement the given feature, but we also have interfaces which add new
methods. So any choice we make here seems to be inconsistent with some
choice we made in the past, but such is the nature of software
development I guess.



I think progress is still possible. The discussion has shown some 
alternatives which are not better than new 
ReversibleCollection/ReversibleSet types. The discussion has raised some 
concerns about compatibility but they are not to hard to overcome 
(mostly just source-level). There is clearly a benefit from new types 
although migration to use them will not be fast. But it might be faster 
than we are used to. The most difficult was (and for some still is) a 
JDK 8 -> JDK 9 jump, but once you are on JDK 9+ it is relatively easy to 
follow latest JDK release. With LTS JDK 17 around the corner, I expect 
most production code which now runs on JDK 11 will relatively quickly 
migrate to JDK 17 just for improvements in JVM/GC if not for 
Language/APIs. I think adding these types will surely trigger some 
disturbance but nothing major.



Peter

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

[Alan Bateman]

On Thu, 20 May 2021 04:22:32 GMT, Phil Race  wrote:

>> That is unfortunate, but nonetheless I think required to be done.
>> We have acknowledeged this can't reasonably be called an RFE, so the JEP is 
>> introducing bugs/technical debt/call it what you will. This should generally 
>> be part of a sandbox for the JEP and fixed before integration of the JEP.
>> From my point of view it is a blocker.
>
> 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.

-

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