Re: RFR: jsr166 integration 2019-09

[David Holmes]

Except when I run it through our test system ReservedStackTest is still 
failing :(


I tested it initially when Fred proposed it and that went fine. It also 
passes for me locally on Linux.


David

On 24/09/2019 12:20 pm, David Holmes wrote:

Hi Martin,

That all seems fine to me.

Thanks,
David

On 24/09/2019 3:43 am, Martin Buchholz wrote:
We now have a fix-up integration that removes all the previously 
excluded tests from their exclude lists.


https://cr.openjdk.java.net/~martin/webrevs/jdk/jsr166-integration/overview.html 



8231031: runtime/ReservedStack/ReservedStackTest.java fails after 
jsr166 refresh
https://cr.openjdk.java.net/~martin/webrevs/jdk/jsr166-integration/ReservedStackTest/index.html 


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

LockInfo objects now restored to their previous values (although David 
was looking at future improvements).


8231032: ThreadMXBean locking tests fail after JSR 166 refresh
https://cr.openjdk.java.net/~martin/webrevs/jdk/jsr166-integration/wrong-blocker/index.html 


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

8231036: vmTestbase monitoring tests fail after JSR 166 refresh
https://cr.openjdk.java.net/~martin/webrevs/jdk/jsr166-integration/SynchronizerLockingThreads/index.html 


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

Re: RFR: jsr166 integration 2019-09

[David Holmes]

Hi Martin,

That all seems fine to me.

Thanks,
David

On 24/09/2019 3:43 am, Martin Buchholz wrote:
We now have a fix-up integration that removes all the previously 
excluded tests from their exclude lists.


https://cr.openjdk.java.net/~martin/webrevs/jdk/jsr166-integration/overview.html

8231031: runtime/ReservedStack/ReservedStackTest.java fails after jsr166 
refresh

https://cr.openjdk.java.net/~martin/webrevs/jdk/jsr166-integration/ReservedStackTest/index.html
https://bugs.openjdk.java.net/browse/JDK-8231031

LockInfo objects now restored to their previous values (although David 
was looking at future improvements).


8231032: ThreadMXBean locking tests fail after JSR 166 refresh
https://cr.openjdk.java.net/~martin/webrevs/jdk/jsr166-integration/wrong-blocker/index.html
https://bugs.openjdk.java.net/browse/JDK-8231032

8231036: vmTestbase monitoring tests fail after JSR 166 refresh
https://cr.openjdk.java.net/~martin/webrevs/jdk/jsr166-integration/SynchronizerLockingThreads/index.html
https://bugs.openjdk.java.net/browse/JDK-8231036

Re: [RFR] 8231171: remove remaining sun.java.launcher.pid references - was RE: sun.java.launcher.pid property usage

[David Holmes]

That looks fine to me.

Thanks,
David

On 20/09/2019 7:14 pm, Baesken, Matthias wrote:

Hi David   , I adjusted the  test  ( 
test/jdk/tools/launcher/TestSpecialArgs.java )   and removed the comments  in 
os_bsd.cpp  (suggested by you) .
New webrev :

http://cr.openjdk.java.net/~mbaesken/webrevs/8231171.1/

Best regards, Matthias





Hi Matthias,

On 20/09/2019 5:03 pm, Baesken, Matthias wrote:


Hello,  looks like   on Linux  there is a special check   in 
TestSpecialArgs.java

for


  launcherPidString = "launcher.pid="

that  fails after   8231171  .
Should I adjust the test ?  Or  keep the  setting in the launcher on Linux ?


IMHO adjust the test please.

Thanks,
David
-

Re: [RFR] 8231171: remove remaining sun.java.launcher.pid references - was RE: sun.java.launcher.pid property usage

[David Holmes]

Hi Matthias,

On 20/09/2019 5:03 pm, Baesken, Matthias wrote:


Hello,  looks like   on Linux  there is a special check   in 
TestSpecialArgs.java  for

 launcherPidString = "launcher.pid="

that  fails after   8231171  .
Should I adjust the test ?  Or  keep the  setting in the launcher on Linux ?


IMHO adjust the test please.

Thanks,
David
-



tools/launcher/TestSpecialArgs.java


 /*
  * On Linux, Launcher Tracking will print the PID.  Use this info
  * to validate what we got as the PID in the Launcher itself.
  * Linux is the only one that prints this, and trying to get it
  * here for win is awful.  So let the linux test make sure we get
  * the valid pid, and for non-linux, just make sure pid string is
  * non-zero.
  */
 if (isLinux) {
  . . . . .
 if (launcherPid == null) {
 System.out.println(tr);
 throw new RuntimeException("Error: failed to find launcher Pid in 
launcher tracking info");
 }
   ...

Exception  thrown by the test  :


begin detailed exceptions
java.lang.reflect.InvocationTargetException
at 
java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at 
java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at 
java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.base/java.lang.reflect.Method.invoke(Method.java:564)
at TestHelper.run(TestHelper.java:202)
at TestSpecialArgs.main(TestSpecialArgs.java:44)
at 
java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at 
java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at 
java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.base/java.lang.reflect.Method.invoke(Method.java:564)
at 
com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:127)
at java.base/java.lang.Thread.run(Thread.java:830)
Caused by: java.lang.RuntimeException: Error: failed to find launcher Pid in 
launcher tracking info
at TestSpecialArgs.testNativeMemoryTracking(TestSpecialArgs.java:193)
... 12 more


Best regards, Matthias





Hi Matthias,

Thanks for cleaning this up.

On 19/09/2019 4:57 pm, Baesken, Matthias wrote:

Hello, as discussed below ,   I want to cleanup  some older references to

sun.java.launcher.pid.

Please review the following change.

After removal  of some code belonging  old  LinuxThreads   (JDK-8078513)   ,

the   sun.java.launcher.pid   handling code remained  but

   seems to be  obsolete these days .

Bug/webrev :

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

http://cr.openjdk.java.net/~mbaesken/webrevs/8231171.0/


src/hotspot/os/bsd/os_bsd.cpp

Can you delete the entire comment block here:

1126   // Under the old bsd thread library, bsd gives each thread
...
1140   // OSThread::thread_id() method in osThread_bsd.hpp file

as it was simply copied from Linux and is nonsense on BSD.

Otherwise that all looks good to me. No need for an updated webrev.

Thanks,
David
-


Best regards, Matthias




Hi David, thanks for the additional  information .
I opened

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

8231171: remove reamining sun.java.launcher.pid references

to do the additional cleanup .

Best regards, Matthias



-Original Message-
From: David Holmes 
Sent: Mittwoch, 18. September 2019 03:16
To: Baesken, Matthias ; 'hotspot-
d...@openjdk.java.net' 
Subject: Re: sun.java.launcher.pid property usage

Hi Matthias,

On 18/09/2019 12:18 am, Baesken, Matthias wrote:

Hello,  while looking at some  atoi usages in the codebase I started to

wonder about the  "sun.java.launcher.pid"  property.

Currently in java_md_solinux.c  the property is set on Linux only  by

default

(code is guarded #ifdef __linux__ ).

Later it is passed  to the int  variable _sun_java_launcher_pid

(arguments.cpp) and can be retrieved by sun_java_launcher_pid() .

However only in src/hotspot/os/bsd/os_bsd.cpp it is really used.

Is the property still supported (one would need to set it from user side

on

the command line on non-Linux because it is not set by default on
bsd/macOS) ?

Can the coding be removed (or should it enabled for BSD/Mac like we

do

on Linux) ?


The os_bsd comment mentiones the bug 6351349 :

https://bugs.java.com/bugdatabase/view_bug.do?bug_id=6351349
JDK-6351349 : On linux with the old thread lib, jps should return the

same

PID as $!


but this looks very old.


That was the bug that added this code as it was needed on Linux with
LinuxThreads. The code was then removed on Linux under

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

The re

Re: [RFR] 8231171: remove remaining sun.java.launcher.pid references - was RE: sun.java.launcher.pid property usage

[David Holmes]

Hi Matthias,

Thanks for cleaning this up.

On 19/09/2019 4:57 pm, Baesken, Matthias wrote:

Hello, as discussed below ,   I want to cleanup  some older references to 
sun.java.launcher.pid.
Please review the following change.

After removal  of some code belonging  old  LinuxThreads   (JDK-8078513)   , 
the   sun.java.launcher.pid   handling code remained  but
  seems to be  obsolete these days .

Bug/webrev :

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

http://cr.openjdk.java.net/~mbaesken/webrevs/8231171.0/


src/hotspot/os/bsd/os_bsd.cpp

Can you delete the entire comment block here:

1126   // Under the old bsd thread library, bsd gives each thread
...
1140   // OSThread::thread_id() method in osThread_bsd.hpp file

as it was simply copied from Linux and is nonsense on BSD.

Otherwise that all looks good to me. No need for an updated webrev.

Thanks,
David
-


Best regards, Matthias




Hi David, thanks for the additional  information .
I opened

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

8231171: remove reamining sun.java.launcher.pid references

   to do the additional cleanup .

Best regards, Matthias



-Original Message-
From: David Holmes 
Sent: Mittwoch, 18. September 2019 03:16
To: Baesken, Matthias ; 'hotspot-
d...@openjdk.java.net' 
Subject: Re: sun.java.launcher.pid property usage

Hi Matthias,

On 18/09/2019 12:18 am, Baesken, Matthias wrote:

Hello,  while looking at some  atoi usages in the codebase I started to

wonder about the  "sun.java.launcher.pid"  property.

Currently in java_md_solinux.c  the property is set on Linux only  by

default

(code is guarded #ifdef __linux__ ).

Later it is passed  to the int  variable _sun_java_launcher_pid

(arguments.cpp) and can be retrieved by sun_java_launcher_pid() .

However only in src/hotspot/os/bsd/os_bsd.cpp it is really used.

Is the property still supported (one would need to set it from user side on

the command line on non-Linux because it is not set by default on
bsd/macOS) ?

Can the coding be removed (or should it enabled for BSD/Mac like we do

on Linux) ?


The os_bsd comment mentiones the bug 6351349 :

https://bugs.java.com/bugdatabase/view_bug.do?bug_id=6351349
JDK-6351349 : On linux with the old thread lib, jps should return the same

PID as $!


but this looks very old.


That was the bug that added this code as it was needed on Linux with
LinuxThreads. The code was then removed on Linux under

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

The review thread starts here:

http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2015-
May/014709.html

I think we were focussed solely on cleaning up the hotspot Linux code
and didn't really look at the wider implication of the use of
sun.java.launcher.pid. The code in os_bsd.cpp was simply copied from the
Linux code without giving it any consideration - as you can tell from
the comment:

// With BsdThreads the JavaMain thread pid (primordial thread)
// is different than the pid of the java launcher thread.
// So, on Bsd, the launcher thread pid is passed to the VM
// via the sun.java.launcher.pid property.

where you can tell that Linux was simply replaced by Bsd, so we
reference the non-existent BsdThreads :(

So yes this all seems to be dead code that should be removed - core-libs
folk will need to be involved of course as they own the launcher. :) It
looks like SetJavaLauncherPlatformProps() can be removed completely.

Thanks,
David

Re: Concurrent Hash Map javadoc question

[David Holmes]

Re-directing to core-libs-dev mailing list.

David

On 18/09/2019 5:45 am, Keith Turner wrote:

The javadoc for ConcurrentHashMap.computeIfAbsent() states the
remapping function is applied at most once.  The functions
computeIfPresent() and compute() do not explicitly state if the
remapping functions could possibly be run multiple times. Does anyone
know if computeIfPresent() and compute() are guaranteed to only run
the remapping functions at most once?  If so, should the javadoc be
updated?

Thanks,

Keith

Re: Urgent RFR: 8231034/8231033 Problem list tetss failing after JSR-166 refresh

[David Holmes]

Thanks Joe. Will push once CI testing is verified.

David

On 16/09/2019 9:54 am, Joe Darcy wrote:

Looks fine David; thanks,

-Joe

On 9/15/2019 4:49 PM, David Holmes wrote:

Bugs: https://bugs.openjdk.java.net/browse/JDK-8231033
  https://bugs.openjdk.java.net/browse/JDK-8231034

webrev: http://cr.openjdk.java.net/~dholmes/8231033-8231034/webrev/
(inline below)

A bunch of mainly management tests (mostly in hotspot repo) are 
failing after the JSR-166 refresh to due changes in stack information 
and usage. These are causing significant disruption to our CI and so 
need to be ProblemListed while the tests are investigated and fixed.


Testing: tier 1 - 3 (in progress)

Thanks,
David

--- old/test/hotspot/jtreg/ProblemList.txt    2019-09-15 
19:41:48.446127451 -0400
+++ new/test/hotspot/jtreg/ProblemList.txt    2019-09-15 
19:41:47.344115881 -0400

@@ -90,6 +90,7 @@
 # :hotspot_runtime

 runtime/jni/terminatedThread/TestTerminatedThread.java 8219652 aix-ppc64
+runtime/ReservedStack/ReservedStackTest.java 8231031 generic-all


# 



@@ -204,4 +205,16 @@


vmTestbase/nsk/jdwp/ThreadReference/ForceEarlyReturn/forceEarlyReturn001/forceEarlyReturn001.java 
7199837 generic-all


+vmTestbase/nsk/monitoring/ThreadMXBean/ThreadInfo/SynchronizerLockingThreads/SynchronizerLockingThreads001/TestDescription.java 
8231032 generic-all
+vmTestbase/nsk/monitoring/ThreadMXBean/ThreadInfo/SynchronizerLockingThreads/SynchronizerLockingThreads002/TestDescription.java 
8231032 generic-all
+vmTestbase/nsk/monitoring/ThreadMXBean/ThreadInfo/SynchronizerLockingThreads/SynchronizerLockingThreads003/TestDescription.java 
8231032 generic-all
+vmTestbase/nsk/monitoring/ThreadMXBean/ThreadInfo/SynchronizerLockingThreads/SynchronizerLockingThreads004/TestDescription.java 
8231032 generic-all
+vmTestbase/nsk/monitoring/ThreadMXBean/ThreadInfo/SynchronizerLockingThreads/SynchronizerLockingThreads005/TestDescription.java 
8231032 generic-all

+
+vmTestbase/nsk/monitoring/ThreadMXBean/ThreadInfo/Multi/Multi001/Multi001.java 
8231032 generic-all
+vmTestbase/nsk/monitoring/ThreadMXBean/ThreadInfo/Multi/Multi002/TestDescription.java 
 8231032 generic-all
+vmTestbase/nsk/monitoring/ThreadMXBean/ThreadInfo/Multi/Multi003/TestDescription.java 
 8231032 generic-all
+vmTestbase/nsk/monitoring/ThreadMXBean/ThreadInfo/Multi/Multi004/TestDescription.java 
 8231032 generic-all
+vmTestbase/nsk/monitoring/ThreadMXBean/ThreadInfo/Multi/Multi005/TestDescription.java 
 8231032 generic-all

+

# 


--- old/test/jdk/ProblemList.txt    2019-09-15 19:41:51.817162845 -0400
+++ new/test/jdk/ProblemList.txt    2019-09-15 19:41:50.703151149 -0400
@@ -564,6 +564,8 @@
 javax/management/monitor/DerivedGaugeMonitorTest.java 8042211 
generic-all


javax/management/remote/mandatory/connection/MultiThreadDeadLockTest.java 
8042215 generic-all


+java/lang/management/ThreadMXBean/LockedSynchronizers.java 8231032 
generic-all

+

 



 # jdk_io

Urgent RFR: 8231034/8231033 Problem list tetss failing after JSR-166 refresh

[David Holmes]

Bugs: https://bugs.openjdk.java.net/browse/JDK-8231033
  https://bugs.openjdk.java.net/browse/JDK-8231034

webrev: http://cr.openjdk.java.net/~dholmes/8231033-8231034/webrev/
(inline below)

A bunch of mainly management tests (mostly in hotspot repo) are failing 
after the JSR-166 refresh to due changes in stack information and usage. 
These are causing significant disruption to our CI and so need to be 
ProblemListed while the tests are investigated and fixed.


Testing: tier 1 - 3 (in progress)

Thanks,
David

--- old/test/hotspot/jtreg/ProblemList.txt	2019-09-15 19:41:48.446127451 
-0400
+++ new/test/hotspot/jtreg/ProblemList.txt	2019-09-15 19:41:47.344115881 
-0400

@@ -90,6 +90,7 @@
 # :hotspot_runtime

 runtime/jni/terminatedThread/TestTerminatedThread.java 8219652 aix-ppc64
+runtime/ReservedStack/ReservedStackTest.java 8231031 generic-all


#

@@ -204,4 +205,16 @@


vmTestbase/nsk/jdwp/ThreadReference/ForceEarlyReturn/forceEarlyReturn001/forceEarlyReturn001.java 
7199837 generic-all


+vmTestbase/nsk/monitoring/ThreadMXBean/ThreadInfo/SynchronizerLockingThreads/SynchronizerLockingThreads001/TestDescription.java 
8231032 generic-all
+vmTestbase/nsk/monitoring/ThreadMXBean/ThreadInfo/SynchronizerLockingThreads/SynchronizerLockingThreads002/TestDescription.java 
8231032 generic-all
+vmTestbase/nsk/monitoring/ThreadMXBean/ThreadInfo/SynchronizerLockingThreads/SynchronizerLockingThreads003/TestDescription.java 
8231032 generic-all
+vmTestbase/nsk/monitoring/ThreadMXBean/ThreadInfo/SynchronizerLockingThreads/SynchronizerLockingThreads004/TestDescription.java 
8231032 generic-all
+vmTestbase/nsk/monitoring/ThreadMXBean/ThreadInfo/SynchronizerLockingThreads/SynchronizerLockingThreads005/TestDescription.java 
8231032 generic-all

+
+vmTestbase/nsk/monitoring/ThreadMXBean/ThreadInfo/Multi/Multi001/Multi001.java 
8231032 generic-all
+vmTestbase/nsk/monitoring/ThreadMXBean/ThreadInfo/Multi/Multi002/TestDescription.java 
 8231032 generic-all
+vmTestbase/nsk/monitoring/ThreadMXBean/ThreadInfo/Multi/Multi003/TestDescription.java 
 8231032 generic-all
+vmTestbase/nsk/monitoring/ThreadMXBean/ThreadInfo/Multi/Multi004/TestDescription.java 
 8231032 generic-all
+vmTestbase/nsk/monitoring/ThreadMXBean/ThreadInfo/Multi/Multi005/TestDescription.java 
 8231032 generic-all

+

#
--- old/test/jdk/ProblemList.txt2019-09-15 19:41:51.817162845 -0400
+++ new/test/jdk/ProblemList.txt2019-09-15 19:41:50.703151149 -0400
@@ -564,6 +564,8 @@
 javax/management/monitor/DerivedGaugeMonitorTest.java 8042211 
generic-all


javax/management/remote/mandatory/connection/MultiThreadDeadLockTest.java 
8042215 generic-all


+java/lang/management/ThreadMXBean/LockedSynchronizers.java 8231032 
generic-all

+



 # jdk_io

Re: RFR: jsr166 integration 2019-09

[David Holmes]

Hi Martin,

These changes have caused a number of tests to break and wreaked havoc 
in our CI system (as those tests get executed a lot in different 
configs). The problem seems to be changes to stack use and contents:


- 
vmTestbase/nsk/monitoring/ThreadMXBean/ThreadInfo/SynchronizerLockingThreads/SynchronizerLockingThreads*

- vmTestbase/nsk/monitoring/ThreadMXBean/ThreadInfo/Multi/Multi*
- runtime/ReservedStack/ReservedStackTest.java
- java/lang/management/ThreadMXBean/LockedSynchronizers.java

I'm filing bugs for each test group, but we may need to problem-list 
them in the meantime.


David
-

On 14/09/2019 3:23 am, Martin Buchholz wrote:

Thank you!

On Fri, Sep 13, 2019 at 9:44 AM > wrote:



Android (d8 in fact) is able to desugar private access with
nestmates since last April,
your latest backport target 9 which is not supported anymore, so
when you will move the backport to support Java 11,
this tool can become handy.


In fact, jsr166 CVS has already abandoned support for jdk9 (and jdk10) 
so there's absolutely no reason not to apply the suggestions made 
by  Rémi's program.


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

Re: RFR 8230937 : Update bugid in ProblemList for vmTestbase/nsk/jdb/eval/eval001/eval001.java

[David Holmes]

Looks good.

Thanks for updating.

David

On 13/09/2019 9:02 am, Brent Christian wrote:

Hi,

 From the bug report:

"The ProblemList indicates that 
vmTestbase/nsk/jdb/eval/eval001/eval001.java was added due to 
JDK-8212117. That bug has been fixed, but this test still fails. That 
line in the ProblemList should instead use 8221503."


The change is:

diff -r 85e1de070bef test/hotspot/jtreg/ProblemList.txt
--- a/test/hotspot/jtreg/ProblemList.txt    Thu Sep 12 09:59:19 2019 -0700
+++ b/test/hotspot/jtreg/ProblemList.txt    Thu Sep 12 15:52:07 2019 -0700
@@ -167,7 +167,7 @@

vmTestbase/nsk/jdi/VirtualMachine/redefineClasses/redefineclasses023/TestDescription.java 
8065773 generic-all


-vmTestbase/nsk/jdb/eval/eval001/eval001.java 8212117 generic-all
+vmTestbase/nsk/jdb/eval/eval001/eval001.java 8221503 generic-all

  vmTestbase/metaspace/gc/firstGC_10m/TestDescription.java 8208250 
generic-all


Thanks,
-Brent

Re: jpackage and java.lang.OutOfMemoryError: Java heap space

[David Holmes]

Hi TK,

Try -J-Xmx4g

David

On 8/09/2019 7:58 am, Tomisław Kityński wrote:

Hello,

I've been trying to run jpackage with different heap sizes, as I get 
exception as in the subject from jlink:


java.io.IOException: jlink failed with: Error: Java heap space
java.lang.OutOfMemoryError: Java heap space
     at java.base/java.io.InputStream.readNBytes(InputStream.java:437)
     at 
java.base/java.io.InputStream.readAllBytes(InputStream.java:341)
     at 
jdk.jlink/jdk.tools.jlink.plugin.ResourcePoolEntry.contentBytes(ResourcePoolEntry.java:127) 

     at 
jdk.jlink/jdk.tools.jlink.plugin.ResourcePoolEntry.write(ResourcePoolEntry.java:140) 

     at 
jdk.jlink/jdk.tools.jlink.internal.ImageFileCreator.lambda$generateJImage$4(ImageFileCreator.java:239) 

     at 
jdk.jlink/jdk.tools.jlink.internal.ImageFileCreator$$Lambda$368/0x000800c49c40.accept(Unknown 
Source)
     at 
java.base/java.util.ArrayList$ArrayListSpliterator.forEachRemaining(ArrayList.java:1621) 

     at 
java.base/java.util.stream.ReferencePipeline$Head.forEach(ReferencePipeline.java:658) 

     at 
jdk.jlink/jdk.tools.jlink.internal.ImageFileCreator.generateJImage(ImageFileCreator.java:238) 

     at 
jdk.jlink/jdk.tools.jlink.internal.ImageFileCreator.writeImage(ImageFileCreator.java:161) 

     at 
jdk.jlink/jdk.tools.jlink.internal.ImageFileCreator.create(ImageFileCreator.java:100) 

     at 
jdk.jlink/jdk.tools.jlink.internal.JlinkTask$ImageHelper.retrieve(JlinkTask.java:851) 

     at 
jdk.jlink/jdk.tools.jlink.internal.ImagePluginStack.operate(ImagePluginStack.java:206) 

     at 
jdk.jlink/jdk.tools.jlink.internal.JlinkTask.createImage(JlinkTask.java:408) 

     at 
jdk.jlink/jdk.tools.jlink.internal.JlinkTask.run(JlinkTask.java:272)

     at jdk.jlink/jdk.tools.jlink.internal.Main.run(Main.java:54)
     at 
jdk.jlink/jdk.tools.jlink.internal.Main$JlinkToolProvider.run(Main.java:63)
     at 
jdk.jpackage/jdk.jpackage.internal.JLinkBundlerHelper.runJLink(JLinkBundlerHelper.java:382) 

     at 
jdk.jpackage/jdk.jpackage.internal.JLinkBundlerHelper.execute(JLinkBundlerHelper.java:187) 

     at 
jdk.jpackage/jdk.jpackage.internal.WinAppBundler.doAppBundle(WinAppBundler.java:178) 

     at 
jdk.jpackage/jdk.jpackage.internal.WinAppBundler.doBundle(WinAppBundler.java:166) 

     at 
jdk.jpackage/jdk.jpackage.internal.WinMsiBundler.prepareProto(WinMsiBundler.java:425) 

     at 
jdk.jpackage/jdk.jpackage.internal.WinMsiBundler.bundle(WinMsiBundler.java:499) 

     at 
jdk.jpackage/jdk.jpackage.internal.WinMsiBundler.execute(WinMsiBundler.java:233) 

     at 
jdk.jpackage/jdk.jpackage.internal.Arguments.generateBundle(Arguments.java:620) 

     at 
jdk.jpackage/jdk.jpackage.internal.Arguments.processArguments(Arguments.java:513) 


     at jdk.jpackage/jdk.jpackage.main.Main.execute(Main.java:97)
     at jdk.jpackage/jdk.jpackage.main.Main.main(Main.java:51)

     at 
jdk.jpackage/jdk.jpackage.internal.JLinkBundlerHelper.runJLink(JLinkBundlerHelper.java:386) 

     at 
jdk.jpackage/jdk.jpackage.internal.JLinkBundlerHelper.execute(JLinkBundlerHelper.java:187) 

     at 
jdk.jpackage/jdk.jpackage.internal.WinAppBundler.doAppBundle(WinAppBundler.java:178) 

     at 
jdk.jpackage/jdk.jpackage.internal.WinAppBundler.doBundle(WinAppBundler.java:166) 

     at 
jdk.jpackage/jdk.jpackage.internal.WinMsiBundler.prepareProto(WinMsiBundler.java:425) 

     at 
jdk.jpackage/jdk.jpackage.internal.WinMsiBundler.bundle(WinMsiBundler.java:499) 

     at 
jdk.jpackage/jdk.jpackage.internal.WinMsiBundler.execute(WinMsiBundler.java:233) 

     at 
jdk.jpackage/jdk.jpackage.internal.Arguments.generateBundle(Arguments.java:620) 

     at 
jdk.jpackage/jdk.jpackage.internal.Arguments.processArguments(Arguments.java:513) 


     at jdk.jpackage/jdk.jpackage.main.Main.execute(Main.java:97)
     at jdk.jpackage/jdk.jpackage.main.Main.main(Main.java:51)

e.g. with -Xmx4g or -Xmx:4g, but i get Error: Invalid Option: [-Xmx4g].

What can I do to increase heap size?

Greetings

TK

Re: RFR: 8212117 : Class.forName may return a reference to a loaded but not linked Class

[David Holmes]

Hi Brent,

Good to see this all come together :)

A couple of comments below.

On 5/09/2019 7:12 am, Brent Christian wrote:

Hi,

Please review my fix for JDK-8212117[1].  The webrev is here:

http://cr.openjdk.java.net/~bchristi/8212117/webrev09/


So to clarify for others any current caller to 
find_class_from_class_loader that passes true for "init" was already 
implicitly asking for a linked class (as you must be linked before you 
can be initialized). With that in mind I would suggest that in 
find_class_from_class_loader you add an assert:


! jclass find_class_from_class_loader(JNIEnv* env, Symbol* name, 
jboolean init, jboolean link,
  Handle loader, Handle 
protection_domain,

  jboolean throwError, TRAPS) {
+assert((init && link) || !init, "incorrect use of init/link arguments");

just to ensure the callers understand this.

Aside: in looking at this I've spotted another existing bug. JNI 
FindClass is not specified to perform class initialization, but the 
implementation passes init=true!


---

src/java.base/share/classes/java/lang/invoke/MethodHandles.java

I don't see the need for the new note given it already has

* @throws LinkageError if the linkage fails

---

src/java.base/share/classes/sun/launcher/LauncherHelper.java

So to clarify, the launcher would previously load the main class without 
linking via:


   mainClass = LoadMainClass(env, mode, what);
   CHECK_EXCEPTION_NULL_LEAVE(mainClass);

and then it would invoke the main method which implicitly initialized 
which implicitly linked and so any exceptions related to linking would 
be handled when the main thread detaches, and it all gets reported 
nicely. But now that we link earlier the exception could be pending 
after LoadMainClass and so we would "abort". So you catch that 
unexpected exception in the LauncherHelper.


Is AccessControlException the only exception, that is not a 
LinkageError, that might be thrown when linking? I would think a number 
of others are possible - in particular IllegalAccessError might occur 
during verification. Other than the fact a test obviously triggered 
this, it's not clear why ACE should be singled out here. ??


---

test/hotspot/jtreg/serviceability/jvmti/ClassStatus/ClassStatus.java

45 // public static void foo(Foo f) { }

Unclear why this exists. Also the test refers initially to class Foo but 
then uses Foo2 and Foo3. ??


Otherwise all seems good.


There is also a CSR[2] in need of review.


I've added a couple of comments and will add myself as a reviewer once 
things are near finalized.


Thanks,
David
-



The spec for the 2-arg and 3-arg Class.forName() methods states they 
will "locate, load, and link" the class, however the linking part is not 
ensured to happen.


Although the VM spec allows flexibility WRT when classes are linked, 
this is a corner where the Class.forName() spec is not being upheld. 
While this is not an issue for most usages,  8181144 [3] demonstrates 
how this can be a problem (with the debugging interface, in this case).


This fix ensures that linking happens during the course of 
Class.forName().  Class.forName() already @throws LinkageError, so no 
spec change is needed there. MethodHandles.Lookup.findClass() needs a 
small spec update, due to calling Class.forName with init=false,


Of course Errors are not required to be caught.  It is therefore 
possible that the new behavior could introduce previously unseen, 
possibly unhandled LinkageErrors.  A new VM flag 
(-XX:+ClassForNameDeferLinking) is introduced to restore the previous 
behavior (to keep such code running until it can be updated).


This change surfaced a couple new "A JNI error has occurred" situations 
(see 8181033[5]) in the Launcher, by way of

   test/jdk/tools/launcher/MainClassCantBeLoadedTest.java
(using the 3-arg Class::forName, detailed in the bug report[4]),
and
   test/jdk/tools/launcher/modules/basic/LauncherErrors.java
(using the 2-arg Class::forName).

The Launcher is updated to maintain non-confusing error messages :).

The new test included with this fix ensures that 8181144[3] is 
addressed.  Thanks go to Serguei Spitsyn for writing the test.


Automated corelibs and hotspot tests pass cleanly.

Thanks,
-Brent

--
1. https://bugs.openjdk.java.net/browse/JDK-8212117

2. https://bugs.openjdk.java.net/browse/JDK-8222071

3. https://bugs.openjdk.java.net/browse/JDK-8181144

4. 
https://bugs.openjdk.java.net/browse/JDK-8212117?focusedCommentId=14215986=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14215986 



5. https://bugs.openjdk.java.net/browse/JDK-8181033

Re: 回复： 回复： what to do next to fix JDK-8230557. thanks

[David Holmes]

Hi,

On 5/09/2019 4:11 pm, 未来阳光 wrote:


Thanks very much.

*BUG-LINK:* https://bugs.openjdk.java.net/browse/JDK-8230557

*Describe: *
the return type of the method BitSet#size is int, so the method may 
return a negative value in some case, for example, will return -2147483648.

```
BitSet bitSet = new BitSet(Integer.MAX_VALUE);
for (int i = 0; i < Integer.MAX_VALUE - 1000; i++) {
 bitSet.set(i);
}
System.out.println(bitSet.size());
```
EXPECTED: 2147483648, but ACTUAL: -2147483648.

*FIX*
I have put the patch in the attachment of the mail.


In case the attachment got stripped form the mailing list the proposed 
fix is:


-public int size() {
-return words.length * BITS_PER_WORD;
+public long size() {
+return (long) words.length * BITS_PER_WORD;

Unfortunately this simple fix it not possible. You can't just change the 
return type of the method to long as that is a source-incompatible 
change and would not be approved [1]. Instead the return value should be 
capped at Integer.MAX_VALUE - but I'll leave that for someone from 
core-libs team to pick up. Also look at the evaluation in:


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

Cheers,
David

[1] https://wiki.openjdk.java.net/display/csr/CSR+FAQs





-- 原始邮件 --
*发件人:* "David Holmes";
*发送时间:* 2019年9月5日(星期四) 下午2:00
*收件人:* "未来阳光"<2217232...@qq.com>;"core-libs-dev"d...@openjdk.java.net>;

*主题:* Re: 回复： what to do next to fix JDK-8230557. thanks

On 5/09/2019 3:46 pm, 未来阳光 wrote:
 >
 > hi, developers.
 >
 > Can someone help me? thanks very much !!

Help you how exactly. As I stated your are up to step 2 of the how to
contribute process. If you have a suggested fix for the bug then put
that in an email as described.

Thanks,
David

 >
 > -- 原始邮件 --
 > *发件人:* "David Holmes";
 > *发送时间:* 2019年9月5日(星期四) 中午1:44
 > *收件人:* "未来阳光"<2217232...@qq.com>;"core-libs-dev" d...@openjdk.java.net>;
 > *主题:* Re: what to do next to fix JDK-8230557. thanks
 >
 > On 5/09/2019 3:35 pm, 未来阳光 wrote:
 >  > Hi, leaders.
 >
 > Hi,
 >
 > No "leaders" here only developers :)
 >
 >  >
 >  > A few days ago, I report a bug in core lib[1]. According to the
 > contribute document[2], I had send oca to oracle andmy name has
 > been listed onoca[3].
 >
 > Welcome aboard!
 >
 >  > But I still can't push my changes to jdk, can someone tell me how to
 > do next? thanks very match!!
 >
 > You can't push anything until you become a Committer and before that you
 > have to become an Author. The steps for contributing are outlined here:
 >
 > http://openjdk.java.net/contribute/
 >
 > and you would seem to be up to step 2. :)
 >
 > Cheers,
 > David
 >
 >  >
 >  >
 >  >
 >  >
 >  > [1]https://bugs.openjdk.java.net/browse/JDK-8230557
 >  >
 >  > [2]http://openjdk.java.net/contribute/
 >  > [3]https://www.oracle.com/technetwork/community/oca-486395.html
 >  >
 >  >
 >  >
 >  >
 >  >
 >  > --原始邮件--
 >  > 发件人:"Bug Report
 > Notification"  > 发送时间:2019年9月5日(星期四) 凌晨3:33
 >  > 收件人:"未来阳光"<2217232...@qq.com;
 >  >
 >  > 主题:Update Notification: Bug Report  - JDK-8230557
 >  >
 >  >
 >  >
 >  >
 >    [This is an automated response. Please do not
 > reply.]
 >  > Dear Java Developer,
 >  > We have finished evaluating your report and have assigned it a Bug
 > ID: JDK-8230557. The issue is visible on bugs.java.com at the following
 > url JDK-8230557.
 >  >   To provide more information about this issue,
 > click  here.
 >  >  We work to resolve the issues that are submitted to
 > us according to their impact to the community as a whole, and make no
 > promises as to the time or release in which a bug might be fixed. If
 > this issue has a significant impact on your project you may want to
 > consider using one of the technical support offerings available at
 > Oracle Support.
 >  >   Regards,
 >  >   Java Developer Support
 >  >
 >  >
 >  >
 >  >
 >  >
 >  > Java SE
 >  >   Java SE Documentation
 >  >   Java SE Downloads
 >  >   Java Developer Forums
 >  >   Oracle Java SE Advanced
 >  >   Bug Database
 >  >
 >      Copyright © Oracle and/or
 > its affiliates. All rights reserved.
 >  >
 >    Terms of Use | Privacy
 >  >

Re: 回复： what to do next to fix JDK-8230557. thanks

[David Holmes]

On 5/09/2019 3:46 pm, 未来阳光 wrote:


hi, developers.

Can someone help me? thanks very much !!


Help you how exactly. As I stated your are up to step 2 of the how to 
contribute process. If you have a suggested fix for the bug then put 
that in an email as described.


Thanks,
David



-- 原始邮件 --
*发件人:* "David Holmes";
*发送时间:* 2019年9月5日(星期四) 中午1:44
*收件人:* "未来阳光"<2217232...@qq.com>;"core-libs-dev"d...@openjdk.java.net>;

*主题:* Re: what to do next to fix JDK-8230557. thanks

On 5/09/2019 3:35 pm, 未来阳光 wrote:
 > Hi, leaders.

Hi,

No "leaders" here only developers :)

 >
 > A few days ago, I report a bug in core lib[1]. According to the 
contribute document[2], I had send oca to oracle andmy name has 
been listed onoca[3].


Welcome aboard!

 > But I still can't push my changes to jdk, can someone tell me how to 
do next? thanks very match!!


You can't push anything until you become a Committer and before that you
have to become an Author. The steps for contributing are outlined here:

http://openjdk.java.net/contribute/

and you would seem to be up to step 2. :)

Cheers,
David

 >
 >
 >
 >
 > [1]https://bugs.openjdk.java.net/browse/JDK-8230557
 >
 > [2]http://openjdk.java.net/contribute/
 > [3]https://www.oracle.com/technetwork/community/oca-486395.html
 >
 >
 >
 >
 >
 > --原始邮件--
 > 发件人:"Bug Report 
Notification"
 > 发送时间:2019年9月5日(星期四) 凌晨3:33
 > 收件人:"未来阳光"<2217232...@qq.com;
 >
 > 主题:Update Notification: Bug Report  - JDK-8230557
 >
 >
 >
 > 
   [This is an automated response. Please do not 
reply.]

 > Dear Java Developer,
 > We have finished evaluating your report and have assigned it a Bug 
ID: JDK-8230557. The issue is visible on bugs.java.com at the following 
url JDK-8230557.
 >   To provide more information about this issue, 
click  here.
 >  We work to resolve the issues that are submitted to 
us according to their impact to the community as a whole, and make no 
promises as to the time or release in which a bug might be fixed. If 
this issue has a significant impact on your project you may want to 
consider using one of the technical support offerings available at   
Oracle Support.

 >   Regards,
 >   Java Developer Support
 >
 >
 >
 >
 >
 > Java SE
 >   Java SE Documentation
 >   Java SE Downloads
 >   Java Developer Forums
 >   Oracle Java SE Advanced
 >   Bug Database
 > 
     Copyright © Oracle and/or 
its affiliates. All rights reserved.
 >  
   Terms of Use | Privacy

 >

Re: contribute to core-lib module

[David Holmes]

Hi,

I responded to your other email.

David

On 5/09/2019 3:31 pm,  wrote:

Hi, leaders.


A few days ago, I report a bug in core lib[1]. According to the contribute document[2], 
I had send oca to oracle andmy name has been listed onoca[3].
But I still can't push my changes to jdk, can someone tell me how to do next? 
thanks very match!!





[1]https://bugs.openjdk.java.net/browse/JDK-8230557

[2]http://openjdk.java.net/contribute/
[3]https://www.oracle.com/technetwork/community/oca-486395.html

Re: what to do next to fix JDK-8230557. thanks

[David Holmes]

On 5/09/2019 3:35 pm, 未来阳光 wrote:

Hi, leaders.


Hi,

No "leaders" here only developers :)



A few days ago, I report a bug in core lib[1]. According to the contribute document[2], 
I had send oca to oracle andmy name has been listed onoca[3].


Welcome aboard!


But I still can't push my changes to jdk, can someone tell me how to do next? 
thanks very match!!


You can't push anything until you become a Committer and before that you 
have to become an Author. The steps for contributing are outlined here:


http://openjdk.java.net/contribute/

and you would seem to be up to step 2. :)

Cheers,
David






[1]https://bugs.openjdk.java.net/browse/JDK-8230557

[2]http://openjdk.java.net/contribute/
[3]https://www.oracle.com/technetwork/community/oca-486395.html





--原始邮件--
发件人:"Bug Report Notification"

Re: RFR: 8212117 : Class.forName may return a reference to a loaded but not linked Class

[David Holmes]

Hi Dan,

With my CSR Group member hat on 

On 5/09/2019 8:06 am, Daniel D. Daugherty wrote:

Brent,

You currently have '-XX:+ClassForNameDeferLinking' as a 'product' 
option, but

product options are harder to remove down the road. Would it be better as a
diagnostic option? A diagnostic option requires 


Whether a flag is product or diagnostic (or experimental) should be a 
basic property of the flag based on its purpose. I would not want to see 
a trend of making new flags diagnostic just because it is easier to get 
rid of them later. The expectation with this fix and flag (which I've 
been heavily involved in) is that production code may be impacted by the 
change and need to use the flag to restore previous behaviour. So it 
really is a product flag that end users should be comfortable in using 
if they need it.


The removal process for a product flag is phased (deprecate, obsolete, 
expire) but not particularly onerous. There is an expectation that this 
flag may be deprecated in 15 as it is intended as a transitional flag.


Thanks,
David
-


'-XX:+UnlockDiagnosticVMOptions'
to be specified before it can be used, e.g.:

     java -XX:+UnlockDiagnosticVMOptions -XX:+ClassForNameDeferLinking Foo

so it is a bit harder to use, but maybe that's a Good Thing (TM).

Dan


On 9/4/19 5:12 PM, Brent Christian wrote:

Hi,

Please review my fix for JDK-8212117[1].  The webrev is here:

http://cr.openjdk.java.net/~bchristi/8212117/webrev09/

There is also a CSR[2] in need of review.

The spec for the 2-arg and 3-arg Class.forName() methods states they 
will "locate, load, and link" the class, however the linking part is 
not ensured to happen.


Although the VM spec allows flexibility WRT when classes are linked, 
this is a corner where the Class.forName() spec is not being upheld. 
While this is not an issue for most usages,  8181144 [3] demonstrates 
how this can be a problem (with the debugging interface, in this case).


This fix ensures that linking happens during the course of 
Class.forName().  Class.forName() already @throws LinkageError, so no 
spec change is needed there. MethodHandles.Lookup.findClass() needs a 
small spec update, due to calling Class.forName with init=false,


Of course Errors are not required to be caught.  It is therefore 
possible that the new behavior could introduce previously unseen, 
possibly unhandled LinkageErrors.  A new VM flag 
(-XX:+ClassForNameDeferLinking) is introduced to restore the previous 
behavior (to keep such code running until it can be updated).


This change surfaced a couple new "A JNI error has occurred" 
situations (see 8181033[5]) in the Launcher, by way of

  test/jdk/tools/launcher/MainClassCantBeLoadedTest.java
(using the 3-arg Class::forName, detailed in the bug report[4]),
and
  test/jdk/tools/launcher/modules/basic/LauncherErrors.java
(using the 2-arg Class::forName).

The Launcher is updated to maintain non-confusing error messages :).

The new test included with this fix ensures that 8181144[3] is 
addressed.  Thanks go to Serguei Spitsyn for writing the test.


Automated corelibs and hotspot tests pass cleanly.

Thanks,
-Brent

--
1. https://bugs.openjdk.java.net/browse/JDK-8212117

2. https://bugs.openjdk.java.net/browse/JDK-8222071

3. https://bugs.openjdk.java.net/browse/JDK-8181144

4. 
https://bugs.openjdk.java.net/browse/JDK-8212117?focusedCommentId=14215986=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14215986 



5. https://bugs.openjdk.java.net/browse/JDK-8181033

Re: RFR: 8230043: Lazily load libverify

[David Holmes]

Hi Claes,

This cleanup all appears fine to me.

The unused Mutex declarations might be better handled in a separate RFE 
but I don't insist. You could file the RFE and still fix together in 
this one changeset.



Testing: tier1-4


Do you know which tests actually test the older verifier? Just thinking 
that it might be a good idea to make them more obvious.


Thanks,
David

On 23/08/2019 11:08 pm, Claes Redestad wrote:

Hi,

please review this cleanup to untangle the old bytecode verifier
(libverify). It's currently linked and loaded eagerly and early during
bootstrap.

Webrev: http://cr.openjdk.java.net/~redestad/8230043/webrev.00/
Bug:    https://bugs.openjdk.java.net/browse/JDK-8230043

- removes dependency on libverify from libjava by moving the 
check_format.c functions into libjava (they don't need to be exported 
via JNI)

- fixed build to not link libjava with libverify
- removes the eager initialization of libverify in
os::native_java_library
- removes the verify_stubs and directs verifier.cpp to load and call 
VerifyClassForMajorVersion in libverify directly. The initialization

is synchronized on a new mutex, Verify_lock
- remove the unused VerifyClass entry point and simplify code in the
verifier by removing some indirections

Testing: tier1-4

Thanks!

/Claes

Re: flatMap still prevents short circuiting when using .iterator()

[David Holmes]

Re-directing to core-libs-dev

David

On 15/08/2019 7:48 pm, Stephen Buergler wrote:

Not really sure where to send this.
flatMap was fixed so that it doesn't prevent short circuiting.
https://bugs.openjdk.java.net/browse/JDK-8075939
If you replace the test cases in the ticket with versions that use
.iterator().next() instead of .findFirst().get() then the problem
still happens.
I just tested this with openjdk:14 on docker hub.

Re: RFR JDK-8193325: StackFrameInfo::getByteCodeIndex returns wrong value if bci > 32767

[David Holmes]

On 13/08/2019 8:55 am, Mandy Chung wrote:

On 8/12/19 3:28 PM, David Holmes wrote:

Hi Mandy,

On 13/08/2019 6:24 am, Mandy Chung wrote:
Having a second thought, I'm keeping @Stable bci field while zero 
indicates an invalid BCI that makes it obvious that this field will 
be updated.  VM will set StackFrameInfo::bci to value+1.


I don't know this code but why have the VM set the value one too many 
and then have the Java code subtract one again. ???


I keep it as @Stable field be initialized once by VM and it means that 0 
indicates an invalid bci.   It could be made as final field but 
initialized in the constructor to -1 and then set by VM.  I opt for 
webrev.03 to make it clear it's initialized later by the VM once.


I see, so the zero is a constraint of it being @Stable. So we offset by 
1 at both ends so that we return -1 when not set. Otherwise we'd need to 
special-case for zero.


Okay I get it. Thanks. Not a review though as I do not know this code.

David


Mandy

Re: RFR JDK-8193325: StackFrameInfo::getByteCodeIndex returns wrong value if bci > 32767

[David Holmes]

Hi Mandy,

On 13/08/2019 6:24 am, Mandy Chung wrote:
Having a second thought, I'm keeping @Stable bci field while zero 
indicates an invalid BCI that makes it obvious that this field will be 
updated.  VM will set StackFrameInfo::bci to value+1.


I don't know this code but why have the VM set the value one too many 
and then have the Java code subtract one again. ???


David



http://cr.openjdk.java.net/~mchung/jdk14/8193325/webrev.03/

Mandy

Re: RFR JDK-8193325: StackFrameInfo::getByteCodeIndex returns wrong value if bci > 32767

[David Holmes]

On 11/08/2019 2:50 pm, Mandy Chung wrote:

On 8/10/19 12:30 AM, Aleksey Shipilev wrote:

On 8/9/19 10:19 PM, Mandy Chung wrote:

An earlier version of this patch was reviewed [1] but I
didn't get back to explore the other approach.  I rebase
the patch and take out the hotspot change which will be
covered by JDK-8229375:

http://cr.openjdk.java.net/~mchung/jdk14//8193325/webrev.01
I wonder if bci=-1 is meaningful, and should be returned when BCI is 
not available. After this patch, it would be converted to 65536?


This is my query as well. It is not obvious to me that the VM will never 
set a BCI of -1


David
-


I'm including Coleen to confirm.

If the stack frame is in a Java method, should BCI always be a valid 
index to the Code attribute?


If it's in a native method, StackFrameInfo::getByteCodeIndex returns -1 
and bci field is ignored.


Mandy

Re: [JSR] [JEP] Java Specification Requirement / Java Enhancement Proposal : 'Parallel OR' and 'Parallel AND'

[David Holmes]

Hi Prakhar,

The bar for getting new language features added is extremely high - new 
operators even higher I think. New operators for parallelism ... well I 
don't see that ever happening.


I'm not a fan of implicit parallelism like you propose, it simply raises 
too many issues for the system to be able to make good enough judgements 
in all cases. If you want to terminate parallel computations when the 
outcome of the operator has been determined then that gets very messy 
semantically as you basically have a race and can never know which 
computation will complete first, nor how far others got - you would have 
to use expressions that are completely side-effect free for this to not 
be an issue. (And we don't have very good mechanisms for cancelling 
computations in the first place.)


There's also an underlying assumption that parallelising the expression 
and managing that parallelism will actually yield performance benefits, 
but that will only happen if the expressions are each extremely 
computationally intensive. Fibers might eventually change the "sweet 
spot" for such decisions, but in general forking off parallel tasks, 
managing them and combining results has significant overhead.


Such things can already be written using library calls with the ForkJoin 
framework and/or parallel streams (as Bernd mentioned). Firing off a 
task for each sub-expression and then combining the results as desired. 
Having language syntax as a short-hand for that likely appeals to you as 
an end-user, but is less appealing to the language architects etc. As I 
said the bar is very high to get in new language features - parallel 
streams themselves don't have syntactic support.


It is an interesting topic academically at least, but I don't expect you 
will find that much interest here.


Cheers,
David
-

On 24/07/2019 10:19 pm, Prakhar Makhija wrote:

Hi David / All,


Earlier wanted to discuss just the implementation of 'OR operator', 
neither 'Conditional OR operator', nor 'Bitwise OR operator'


Same goes with 'AND operator'

Technically OR operator and AND operator are just binary operators, 
that's true


I don't find it wrong considering there can be n operands, in an 
expression, with either of the two operators or a combination of both, 
where n >= 2


n can go nearby anything in powers of 10, it will take those many 
sequential clock cycles, to actually resolve the expression


If you consider the same n as an expression of powers of 2, the same 
thing can be done in parallel clubbed with divide and conquer, taking 
the same number of clock cycles, but saving actual response time


This can be given as a input from console, or as a property, specifying 
which implementation to use at runtime, sequential or parallel


But with the latter would lead to very frequent resource starvage, this 
would need to be handled, to actually free the resources and give to pid 
or ppid, heirarically, or based upon who is the callee, or who asked who 
to wait, or some more other logic


All the threads parallelly evaluating the same expression, with either 
operand, should be terminated and concluded as, when any thread results to:

'true' in case of OR
'false' in case of AND

An expression can have further sub-expressions; so an expression with 
combination of both operands, will be considered as parent expression


Yes it does change the conventional implementation of OR operator, and 
AND operator, in Java


So it would be better to have two new operators/symbols itself

Parallel OR
|||

Parallel AND
&&&

We can go with the symbol |&& also in case of Parallel AND


Looking forward to hearing your thoughts


Best Wishes & Regards
Prakhar Makhija



Hi Prakhar,


On 22/06/2019 1:28 am, Prakhar Makhija wrote:
 > Topic: OR operator represented by ||

That should be the subject of your email - not a reply to a digest.


 > Query: The expression evaluation of the operands, of OR operator, does it
 > happen in parallel, when Java code runs, in the current versions?

No, there is no parallel evaluation of anything in Java. It would be
wrong to do so in this case as:

"The conditional-or operator || operator is like | (§15.22.2), but
evaluates its righthand operand only if the value of its left-hand
operand is false."


David

Fwd: Re: Verify error in hg:jdk/jdk -- repository now READ-ONLY

[David Holmes]

FYI open again
--- Begin Message ---
Hi.

hg:jdk/jdk is now open and verifies.  Even if your local repository is
corrupt, you may continue using it.  The corruption does not propagate
via pushes, since changeset IDs are unchanged.

I strongly advise everybody to at least understand whether their local
repositories are corrupt.  The following shows the output of the `hg
verify` command for a corrupt repository:
 
  $ hg verify
  checking changesets
  checking manifests
  crosschecking files in changesets and manifests
  checking files
   test/langtools/jdk/javadoc/doclet/testLinkOption/TestOptionOrder.java@55761: 
f7fb5d9b7490 not in manifests
  184109 files, 55795 changesets, 479538 total revisions
  1 integrity errors encountered!
  (first damaged changeset appears to be 55761)

If you want to work with a repository which verifies, you should do a
fresh clone of hg:jdk/jdk.  Confident users can create a new repository
by cloning the corrupt repository up to the damaged changeset (`hg clone
-r 55760 corrupt fixed`) and then pulling the remaining changesets from
hg:jdk/jdk (hg pull -u).

Thank you for your patience.

Iris


- Original Message - 
From: iris.cl...@oracle.com 
To: jdk-...@openjdk.java.net 
Cc: iris.cl...@oracle.com 
Sent: Wednesday, July 24, 2019 10:04:33 AM GMT -08:00 US/Canada Pacific 
Subject: Verify error in hg:jdk/jdk -- repository now READ-ONLY 

Hi. 

I’ve made the hg:jdk/jdk repository READ-ONLY. 

A verify error which was discovered in the repository and we’d like
to ensure that no further changesets are pushed as a solution is investigated. 

Thanks, 

Iris 

--- End Message ---

Fwd: Verify error in hg:jdk/jdk -- repository now READ-ONLY

[David Holmes]

FYI in case this was not seen on jdk-dev list.

David
--- Begin Message ---
Hi.

 

I've made the hg:jdk/jdk repository READ-ONLY.

 

A verify error which was discovered in the repository and we'd like to ensure 
that no further changesets are pushed as a solution is investigated.

 

Thanks,

Iris

 
--- End Message ---

Re: Bug in parallel sorting of float / double

[David Holmes]

Hi Vladimir,

On 24/07/2019 10:58 pm, Vladimir Yaroslavskiy wrote:

Hi David,

Please, see how it works: Arrays.parallelSort(double[]) 
invokes ArraysParallelSortHelpers.FJDouble.Sorter

if size is big enough and ForkJoinPool.getCommonPoolParallelism()) > 1.

FJDouble.Sorter divides given array into 4 parts, sorts them recursively 
in parallel and merges these parts.
Finally DualPivotQuicksort is invoked on small parts and only on this 
step NaNs and -0.0s are arranged.
In other words, NaNs and -0.0s are arranged inside each small parts, but 
this action must be

done once before the first splitting of the array.


My understanding** was that the merge of the correctly sorted sub-arrays 
would correctly cause NaNs to bubble to the end as required, while 
zeroes would also group - though I think I can see now that simply using 
< would not correctly order NaNs relative to other values, nor order 
-0.0 and +0.0


** It's been 7 years since I helped Doug Lea put the parallelising code 
into the JDK so I'm a bit rusty on the details :) and I'm surprised such 
a bug has not been detected before now.


Cheers,
David


Thank you,
Vladimir

Среда, 24 июля 2019, 15:39 +03:00 от David Holmes
:

Hi Vladimir,

On 24/07/2019 8:53 pm, Vladimir Yaroslavskiy wrote:
 > Hi all,
 >
 > I've found bug in parallel sorting of float / double arrays in
the latest JDK.
 >
 > When float / double values are sorted, additional actions are
 > required: NaNs must be moved to the end and negative zeros
 > must be placed before positive zeros.
 >
 > Current implementation of Arrays.parallelSort(float[] / double [])
 > invokes parallel merge sort from ArraysParallelSortHelpers class,
 > but it doesn't arrange NaNs and -0.0.

It ultimately uses DualPivotQuicksort which AFAICS does have code to
arrange NaNS:

  static void sort(float[] a, int left, int right,
   float[] work, int workBase, int workLen) {
  /*
   * Phase 1: Move NaNs to the end of the array.
   */
  while (left <= right && Float.isNaN(a[right])) {
  --right;
  }

and also order +/-0

   /*
    * Phase 3: Place negative zeros before positive zeros.
    */

where does the bug arise?

Thanks,
David
-


 > @Alan, Brent, Laurent
 > Could you please file a bug?
 >
 > New optimized version of DualPivotQuicksort (which is under
review) works fine and
 > doesn't contain this bug. Please, look at my test case to
reproduce it.
 >
 >

--
 > import java.util.Random;
 >
 > public class FailedFloat {
 >
 > private static final int MAX_N = (1 << 13) /*
Arrays.MIN_ARRAY_SORT_GRAN */ + 10;
 >
 > public static void main(String[] args) {
 > float[] a = new float[MAX_N];
 > random(a);
 > java.util.Arrays.parallelSort(a);
 > check(a);
 > System.out.println("PASSED");
 > }
 >
 > private static void random(float[] a) {
 > Random random = new Random(777);
 > for (int i = 0; i < MAX_N; i++) {
 > a[i] = random.nextBoolean() ? -0.0f : 0.0f;
 > }
 > }
 >
 > private static void check(float[] a) {
 > for (int i = 0; i < a.length - 1; ++i) {
 > if (Float.floatToRawIntBits(a[i]) == 0 &&
Float.floatToRawIntBits(a[i + 1]) < 0) {
 > throw new RuntimeException(a[i] + " goes before "+ a[i + 1] + "
at position " + i);
 > }
 > }
 > }
 > }
 >


 > Thank you,
 > Vladimir

Re: Bug in parallel sorting of float / double

[David Holmes]

Hi Vladimir,

On 24/07/2019 8:53 pm, Vladimir Yaroslavskiy wrote:

Hi all,

I've found bug in parallel sorting of float / double arrays in the latest JDK.

When float / double values are sorted, additional actions are
required: NaNs must be moved to the end and negative zeros
must be placed before positive zeros.

Current implementation of Arrays.parallelSort(float[] / double [])
invokes parallel merge sort from ArraysParallelSortHelpers class,
but it doesn't arrange NaNs and -0.0.


It ultimately uses DualPivotQuicksort which AFAICS does have code to 
arrange NaNS:


static void sort(float[] a, int left, int right,
 float[] work, int workBase, int workLen) {
/*
 * Phase 1: Move NaNs to the end of the array.
 */
while (left <= right && Float.isNaN(a[right])) {
--right;
}

and also order +/-0

 /*
  * Phase 3: Place negative zeros before positive zeros.
  */

where does the bug arise?

Thanks,
David
-



@Alan, Brent, Laurent
Could you please file a bug?

New optimized version of DualPivotQuicksort (which is under review) works fine 
and
doesn't contain this bug. Please, look at my test case to reproduce it.

--
import java.util.Random;

public class FailedFloat {

 private static final int MAX_N = (1 << 13) /* Arrays.MIN_ARRAY_SORT_GRAN 
*/ + 10;

 public static void main(String[] args) {
 float[] a = new float[MAX_N];
 random(a);
 java.util.Arrays.parallelSort(a);
 check(a);
 System.out.println("PASSED");
 }

 private static void random(float[] a) {
 Random random = new Random(777);
 for (int i = 0; i < MAX_N; i++) {
 a[i] = random.nextBoolean() ? -0.0f : 0.0f;
 }
 }

 private static void check(float[] a) {
 for (int i = 0; i < a.length - 1; ++i) {
 if (Float.floatToRawIntBits(a[i]) == 0 && Float.floatToRawIntBits(a[i 
+ 1]) < 0) {
 throw new RuntimeException(a[i] + " goes before "+ a[i + 1] + " at 
position " + i);
 }
 }
 }
}

Thank you,
Vladimir

Re: [OpenJDK 2D-Dev] 8187898: PrintStream should override FilterOutputStream#write(byte[]) with a method that has no throws clause

[David Holmes]

Sorry I was obviously missing the part of this thread where it was 
decided to not actually fix this as requested in the bug due to 
compatibility issues.


I agree with Brian that something must be added to the class-level 
javadoc explaining that due to a historical oversight there is one 
method that declares it throws IOException (and can actually throw it?).


I don't see why that method couldn't still honour the checkError 
protocol as well though.


David

On 24/07/2019 11:31 am, David Holmes wrote:

Jumping in here as this change is starting to really confuse me ...

On 24/07/2019 2:41 am, Brian Burkhalter wrote:



On Jul 23, 2019, at 8:27 AM, Brian Burkhalter 
 wrote:


On Jul 23, 2019, at 8:20 AM, Alan Bateman <mailto:alan.bate...@oracle.com>> wrote:


On 23/07/2019 16:08, Brian Burkhalter wrote:


I don’t see what you mean.
    @Override
    public void write(byte buf[]) throws IOException {
    super.write(buf);
    }
Should “trouble” be set and the IOE re-thrown?

super.write(byte[]) will invoke PrintStream overrides write(byte[], 
int, int) so IOE won't be thrown (and why the proposed change to the 
class description isn't right). The method description is probably 
the best place to describe the behavior.


Here is an update which accounts for the foregoing comments.

http://cr.openjdk.java.net/~bpb/8187898/webrev-alt.02/


  609 @Override
  610 public void write(byte buf[]) throws IOException {
  611 super.write(buf, 0, buf.length);
  612 }

This is wrong! The whole point of this bug is to ensure that PrintStream 
methods honour its contract of NEVER throwing IOException. I mean that 
is what the bug synopsis and description are all about!


David
-


Thanks,

Brian

Re: [OpenJDK 2D-Dev] 8187898: PrintStream should override FilterOutputStream#write(byte[]) with a method that has no throws clause

[David Holmes]

Jumping in here as this change is starting to really confuse me ...

On 24/07/2019 2:41 am, Brian Burkhalter wrote:




On Jul 23, 2019, at 8:27 AM, Brian Burkhalter  
wrote:


On Jul 23, 2019, at 8:20 AM, Alan Bateman mailto:alan.bate...@oracle.com>> wrote:

On 23/07/2019 16:08, Brian Burkhalter wrote:


I don’t see what you mean.
@Override
public void write(byte buf[]) throws IOException {
super.write(buf);
}
Should “trouble” be set and the IOE re-thrown?


super.write(byte[]) will invoke PrintStream overrides write(byte[], int, int) 
so IOE won't be thrown (and why the proposed change to the class description 
isn't right). The method description is probably the best place to describe the 
behavior.


Here is an update which accounts for the foregoing comments.

http://cr.openjdk.java.net/~bpb/8187898/webrev-alt.02/


 609 @Override
 610 public void write(byte buf[]) throws IOException {
 611 super.write(buf, 0, buf.length);
 612 }

This is wrong! The whole point of this bug is to ensure that PrintStream 
methods honour its contract of NEVER throwing IOException. I mean that 
is what the bug synopsis and description are all about!


David
-


Thanks,

Brian

Re: 8202471: Resolves generic receiver type for types with generic signatures

[David Holmes]

On 23/07/2019 3:08 pm, Rafael Winterhalter wrote:
Thanks, I'll send an inline version this evening. I have written a 
couple of reproducers for these issues. Should I add them to jtreg and 
also send them as an inline patch?


Yes, please do.

Thanks,
David


I'll submit the CSR tonight, too.

Thanks for the pointers!
Best regards, Rafael

David Holmes mailto:david.hol...@oracle.com>> 
schrieb am Di., 23. Juli 2019, 06:10:


Hi Rafael,

A couple of comments on process here ...

On 23/07/2019 6:48 am, Rafael Winterhalter wrote:
 > Hello,
 >
 > I have created a patch such that getReceiverType() returns a
parameterized
 > type if the receiver type declaration is itself generic.
Currently, the
 > receiver type is always a type representation of Class such that
 > annotations on the type variables or the receiver type's owner
type cannot
 > be resolved:
https://gist.github.com/raphw/a155d5ef66d11e5fb131b7e6b8fb10e5

All OpenJDK contributions must be provided via OpenJDK infrastructure,
so either direct code in an email to a mailing list (attachments
usually
get stripped), or a webrev hosted on cr.openjdk.java.net
<http://cr.openjdk.java.net> (available
directly to Authors else find someone to host for you). Links to github
are not acceptable at this time.

 > Note that this change can potentially break existing code if
callers of the
 > method expect this behavior for parameterized receiver types.
However,
 > without this change, the type information is lost and I would
argue that
 > the current behavior can be considered to be incorrect.

That argument needs to be made via a Compatibility and Specification
Review (CSR) request:

https://wiki.openjdk.java.net/display/csr/Main

Cheers,
David

 > Best regards, Rafael
 >

Re: 8202471: Resolves generic receiver type for types with generic signatures

[David Holmes]

Hi Rafael,

A couple of comments on process here ...

On 23/07/2019 6:48 am, Rafael Winterhalter wrote:

Hello,

I have created a patch such that getReceiverType() returns a parameterized
type if the receiver type declaration is itself generic. Currently, the
receiver type is always a type representation of Class such that
annotations on the type variables or the receiver type's owner type cannot
be resolved: https://gist.github.com/raphw/a155d5ef66d11e5fb131b7e6b8fb10e5


All OpenJDK contributions must be provided via OpenJDK infrastructure, 
so either direct code in an email to a mailing list (attachments usually 
get stripped), or a webrev hosted on cr.openjdk.java.net (available 
directly to Authors else find someone to host for you). Links to github 
are not acceptable at this time.



Note that this change can potentially break existing code if callers of the
method expect this behavior for parameterized receiver types. However,
without this change, the type information is lost and I would argue that
the current behavior can be considered to be incorrect.


That argument needs to be made via a Compatibility and Specification 
Review (CSR) request:


https://wiki.openjdk.java.net/display/csr/Main

Cheers,
David


Best regards, Rafael

Re: Fwd: Re: RFR: 8225648:[TESTBUG] java/lang/annotation/loaderLeak/Main.java fails with -Xcomp

[David Holmes]

On 22/07/2019 5:14 pm, Jie Fu wrote:

Hi David,

On 2019/7/22 下午2:55, David Holmes wrote:

Did you use -N when generating the webrev? You shouldn't in this case.

Yes, I had used `ksh webrev.ksh -u jiefu -N -r 55752` to generate 
webrev.02.


Updated: http://cr.openjdk.java.net/~jiefu/8225648/webrev.03/
webrev.03 was generated by `ksh webrev.ksh -c 8225648`.
Is that right?


Yep :)

Sponsored.

Cheers,
David
-


Thanks a lot.
Best regards,
Jie

Re: Fwd: Re: RFR: 8225648:[TESTBUG] java/lang/annotation/loaderLeak/Main.java fails with -Xcomp

[David Holmes]

Hi Jie,

On 22/07/2019 4:39 pm, Jie Fu wrote:

Hi David,

Thanks for correcting me and sorry for that.


No problem, it's a learning curve for everyone.


Updated: http://cr.openjdk.java.net/~jiefu/8225648/webrev.02/


The patch is still not a proper changeset:

http://cr.openjdk.java.net/~jiefu/8225648/webrev.02/jdk-dev.patch

--- old/test/jdk/java/lang/annotation/loaderLeak/Main.java	2019-07-22 
14:33:23.086509297 +0800
+++ new/test/jdk/java/lang/annotation/loaderLeak/Main.java	2019-07-22 
14:33:22.850508806 +0800

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2004, 2012, Oracle and/or its affiliates. All rights 
reserved.
+ * Copyright (c) 2004, 2019, Oracle and/or its affiliates. All rights 
reserved.

  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -56,6 +56,7 @@
 if (c.get() == null) throw new AssertionError();
 System.gc();
 System.gc();
+Reference.reachabilityFence(loader);
 loader = null;

 // Might require multiple calls to System.gc() for weak-references

Did you use -N when generating the webrev? You shouldn't in this case.

Thanks,
David


Thanks a lot.
Best regards,
Jie

On 2019/7/22 下午2:22, David Holmes wrote:

Hi Jie,

On 22/07/2019 4:18 pm, Jie Fu wrote:

Hi all,

Could someone help to push this: 
http://cr.openjdk.java.net/~jiefu/8225648/webrev.01/ ?

I need a sponsor.


To prepare your patch for your sponsor you should commit it with the 
correct format commit message, including reviewers, so that the 
sponsor can just do an hg import from the URL of the changeset in your 
webrev - or somewhere else you point them to.


Cheers,
David


It had been fully reviewed by Alan and Ioi and can be applied cleanly.

Thanks a lot.
Best regards,
Jie


 Forwarded Message 
Subject: Re: RFR: 8225648:[TESTBUG] 
java/lang/annotation/loaderLeak/Main.java fails with -Xcomp

Date: Thu, 20 Jun 2019 12:09:24 +0100
From: Alan Bateman 
To: Jie Fu 
CC: core-libs-dev@openjdk.java.net



On 19/06/2019 07:36, Jie Fu wrote:

Hi Alan,

I've updated the patch by adding the review info.
http://cr.openjdk.java.net/~jiefu/8225648/webrev.01/

Is it OK to be pushed?

I don't see any objections so go ahead.

-Alan

Re: Fwd: Re: RFR: 8225648:[TESTBUG] java/lang/annotation/loaderLeak/Main.java fails with -Xcomp

[David Holmes]

Hi Jie,

On 22/07/2019 4:18 pm, Jie Fu wrote:

Hi all,

Could someone help to push this: 
http://cr.openjdk.java.net/~jiefu/8225648/webrev.01/ ?

I need a sponsor.


To prepare your patch for your sponsor you should commit it with the 
correct format commit message, including reviewers, so that the sponsor 
can just do an hg import from the URL of the changeset in your webrev - 
or somewhere else you point them to.


Cheers,
David


It had been fully reviewed by Alan and Ioi and can be applied cleanly.

Thanks a lot.
Best regards,
Jie


 Forwarded Message 
Subject: Re: RFR: 8225648:[TESTBUG] 
java/lang/annotation/loaderLeak/Main.java fails with -Xcomp

Date: Thu, 20 Jun 2019 12:09:24 +0100
From: Alan Bateman 
To: Jie Fu 
CC: core-libs-dev@openjdk.java.net



On 19/06/2019 07:36, Jie Fu wrote:

Hi Alan,

I've updated the patch by adding the review info.
http://cr.openjdk.java.net/~jiefu/8225648/webrev.01/

Is it OK to be pushed?

I don't see any objections so go ahead.

-Alan

Re: Review Request: JDK-8219774: Reexamine the initialization of LangReflectAccess shared secret at AccessibleObject::

[David Holmes]

Hi Mandy,

This approach looks much cleaner and safer to me, and the slight 
shuffling in the init order should not cause any startup issues.


Thanks,
David
-

On 20/07/2019 2:20 am, Mandy Chung wrote:

This was a follow up to the observation during the code review
of JDK-82193798.

Webrev:
    http://cr.openjdk.java.net/~mchung/jdk14/8219774/webrev.01/

In the current implementation, Modifier:: provides a hook
to initialize the static ReflectionFactory::langReflectAccess field
lazily. This was introduced prior to the common shared secrets
mechanism.

Another observation is that when ReflectionFactory methods are called,
there is a Method, Field or Constructor object in hand. In addition,
Method class is initialized very early during startup by the VM and
so does AccessibleObject class. The ReflectionFactory::newField and
newMethod taking the Field/Method parameter are used but not the
one without (dead code).

This patch cleans up the initialization of LangReflectAccess to
AccessibleObject:: during early startup initPhase1.
I also move LangReflectAccess to jdk.internal.access to be consistent
with other *Access classes (LangReflectAccess was added before the common
SharedSecrets class was introduced).

This also addresses JDK-8227831 the overhead of langReflectAcces being
a volatile field on the platform with weak memory model (Thanks to
Ogata confirming that this patch is 4% better than the proposed patch
for JDK-8227831 [1]).

The impact to the class initialization order is minimal. 
JavaLangReflectAccess
is initialized during initPhase1 (which has been initialized during 
initPhase2).

SharedSecrets is initialized in initPhase2 and this patch moves it to be
initialized followed AccessibleObject.

Mandy
[1] 
https://mail.openjdk.java.net/pipermail/core-libs-dev/2019-July/061352.html

Re: RFR: JDK-8227831: Avoid using volatile for write-once, read-many class field

[David Holmes]

Hi Claes,

On 18/07/2019 1:04 am, Claes Redestad wrote:

Hi,

removing volatile aligns LangReflectAccess initialization with the
pattern used for other access objects: it's only initialized in the
static initializer of some class which we ensure is initialized, which
means any initialization race is guarded by the initialization of said
class - in this case j.l.r.Modifier.


If the field is not volatile then we are not guaranteed to correctly see 
the constructed LangReflectAccess instance. Without volatile (or without 
additional use of unconditional sync in the accessor) we do not have a 
happens-before relationship between the construction of the LRA 
instance, and the setting of the field.



Initialization of other access objects are not guarded by any
explicit synchronization, however, since similar load/store barriers
will be in effect by ensuring the class that constructs the object has
been initialized. So I think you could remove the explicit
synchronization.


We are not guaranteed to hit the class initialization checks that would 
guarantee the necessary ordering.


David
-


I'm not sure why LangReflectAccess has not moved in with other
SharedSecrets. Perhaps this is just an artefact of having been around
for a long time, but maybe that'd cause some cyclic bootstrap
dependency. Either way it's nice to see it align to use the same
pattern.

Thanks!
/Claes

On 2019-07-17 10:00, Kazunori Ogata wrote:

Hi Aleskey,

Thank you for your comment.

I forgot to mention one thing.  I verified that all accesses to
langReflectioAccess calls the accessor "langReflectAccess()".  Since this
variable is modified once from null to non-null, I think the write in the
setter is guaranteed to be visible in the getter by putting the
synchronized block in langReflectAccess().

Should I put comments about this assumption?  Actually, in JDK11 there 
are

some lines that do not call the getter, so backport needs to fix them,
too.


Regards,
Ogata


Aleksey Shipilev  wrote on 2019/07/17 16:49:08:


From: Aleksey Shipilev 
To: Kazunori Ogata , core-libs-dev@openjdk.java.net
Date: 2019/07/17 16:49
Subject: [EXTERNAL] Re: RFR: JDK-8227831: Avoid using volatile for

write-

once, read-many class field

On 7/17/19 8:48 AM, Kazunori Ogata wrote:

Webrev: http://cr.openjdk.java.net/~ogatak/8227831/webrev/


Note this is generally not safe: it introduces data race on
langReflectAccess field access. It has
to be proved that everything that implements LangReflectAccess interface



makes this data race
benign: e.g. all fields that are accessed via those implementation are
final, read once, etc. And
briefly looking at that, I am not sure it is the case for the actual
accessor generators.

I'd cautiously leave "volatile" here.

--
Thanks,
-Aleksey

[attachment "signature.asc" deleted by Kazunori Ogata/Japan/IBM]

Re: (13) RFR (S): 8227055: Minor edits to launcher help text

[David Holmes]

> I'm fine if you want to move ahead with this patch.

Thanks Mandy - much appreciated! We can continue this refinement for 14.

I have filed JDK-8227229 to deprecate and remove Xdebug - for history see:

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

BTW thanks for the reference to JEP-293, I wasn't aware of it. Would be 
nice to expand it to give more guidelines on handling deprecation etc.


Cheers,
David
-

On 4/07/2019 3:36 am, Mandy Chung wrote:



On 7/2/19 11:10 PM, David Holmes wrote:

Hi Mandy,

Thanks for taking a look.

On 3/07/2019 8:57 am, Mandy Chung wrote:

On 7/2/19 3:44 PM, David Holmes wrote:

webrev: http://cr.openjdk.java.net/~dholmes/8227055/webrev/
bug: https://bugs.openjdk.java.net/browse/JDK-8227055

The launcher help text needs some minor updates/corrections
- -verbose needs more info
- -Xdebug needs to say it does nothing


Should this be removed instead?


There is some strange history/status with this flag. I expect the flag 
and thus documentation should be removed, but that would be a seperate 
issue for 14 (or later). For 13 I'm just aligning the help text with 
the manpage description.




I would propose  to take -Xdebug out from the launcher help message and 
man page but -Xdebug is still accepted but do nothing.



- -Xloggc is deprecated and replaced by -Xlog:gc:filename


I think it can just list -Xlog:gc (the replacement) and no need
to mention the deprecated option in the launcher help message.


Again this aligns with the manpage which does list it as deprecated 
and its replacement. Otherwise the launcher help doesn't document 
specific -Xlog usages.




-Xloggc was deprecated since 9 (or earlier [1]).  I think documenting 
the recommended option instead is the right thing to do.  I think it 
worths the discussion and includes some guideline in JEP 293 (Guidelines 
for JDK CLI) [2] w.r.t. deprecated options in the help message vs man page.


I'm fine if you want to move ahead with this patch.


- -Xshare:on needs warning about use

Reference to Mac OS X should be macOS.

The --disable-@files is documented as both a standard and extra option.



I think this is intended to be an extra option.


This is from Alan:

"I'm not sure how `-disable-@files` ended up in both the `-help` and 
`-X` output, it probably make sense to drop it from the `-X` output. 
That way, `@argument` and `-disable-@files` are listed together, not 
in separate usage pages."


(The bug explains how it ended up in both places - and yes it started 
as a -X option.)




Hmm... Alan and Jon are both on vacation.  We can follow up this 
separately.


Mandy
[1] https://bugs.openjdk.java.net/browse/JDK-8170636
[2] https://openjdk.java.net/jeps/293

Re: (13) RFR (S): 8227055: Minor edits to launcher help text

[David Holmes]

Hi Mandy,

Thanks for taking a look.

On 3/07/2019 8:57 am, Mandy Chung wrote:

On 7/2/19 3:44 PM, David Holmes wrote:

webrev: http://cr.openjdk.java.net/~dholmes/8227055/webrev/
bug: https://bugs.openjdk.java.net/browse/JDK-8227055

The launcher help text needs some minor updates/corrections
- -verbose needs more info
- -Xdebug needs to say it does nothing


Should this be removed instead?


There is some strange history/status with this flag. I expect the flag 
and thus documentation should be removed, but that would be a seperate 
issue for 14 (or later). For 13 I'm just aligning the help text with the 
manpage description.



- -Xloggc is deprecated and replaced by -Xlog:gc:filename


I think it can just list -Xlog:gc (the replacement) and no need
to mention the deprecated option in the launcher help message.


Again this aligns with the manpage which does list it as deprecated and 
its replacement. Otherwise the launcher help doesn't document specific 
-Xlog usages.



- -Xshare:on needs warning about use

Reference to Mac OS X should be macOS.

The --disable-@files is documented as both a standard and extra option.



I think this is intended to be an extra option.


This is from Alan:

"I'm not sure how `-disable-@files` ended up in both the `-help` and 
`-X` output, it probably make sense to drop it from the `-X` output. 
That way, `@argument` and `-disable-@files` are listed together, not in 
separate usage pages."


(The bug explains how it ended up in both places - and yes it started as 
a -X option.)


Thanks,
David
-


Otherwise, looks okay.

Mandy


This aligns the help info with the Java manpage info.

Thanks,
David

(13) RFR (S): 8227055: Minor edits to launcher help text

[David Holmes]

webrev: http://cr.openjdk.java.net/~dholmes/8227055/webrev/
bug: https://bugs.openjdk.java.net/browse/JDK-8227055

The launcher help text needs some minor updates/corrections
- -verbose needs more info
- -Xdebug needs to say it does nothing
- -Xloggc is deprecated and replaced by -Xlog:gc:filename
- -Xshare:on needs warning about use

Reference to Mac OS X should be macOS.

The --disable-@files is documented as both a standard and extra option.

This aligns the help info with the Java manpage info.

Thanks,
David

Re: core-libs-dev Digest, Vol 146, Issue 92

[David Holmes]

Hi Prakhar,

On 22/06/2019 1:28 am, Prakhar Makhija wrote:

Topic: OR operator represented by ||


That should be the subject of your email - not a reply to a digest.


Query: The expression evaluation of the operands, of OR operator, does it
happen in parallel, when Java code runs, in the current versions?


No, there is no parallel evaluation of anything in Java. It would be 
wrong to do so in this case as:


"The conditional-or operator || operator is like | (§15.22.2), but 
evaluates its righthand operand only if the value of its left-hand 
operand is false."


David

Re: 8226242 : Diagnostic output for posix_spawn failure

[David Holmes]

Hi Roger,

On 18/06/2019 6:00 am, Roger Riggs wrote:

Hi,

Updated:
http://cr.openjdk.java.net/~rriggs/webrev-spawn-diag-8225192-3/


+ if (WIFEXITED(status)) {
+ snprintf(ebuf, sizeof ebuf,
+ "Failed to exec spawn helper: pid: %d, exit value: %d",
+ pid, WEXITSTATUS(status));

WIFEXITED returns non-zero when the process terminates normally, so that 
need not be an error AFAICS.


David



On 6/17/19 2:20 PM, Martin Buchholz wrote:



On Mon, Jun 17, 2019 at 10:47 AM Thomas Stüfe > wrote:


    Hi Roger,

    I think this is fine.

    Could you please also print out WIFEXITED and WIFSIGNALLED? Since
    I am not
    sure if the WEXITSTATUS contains valid info if WIFEXITED is 0, or 
just

    random noise. Same for WTERMSIG.

I separated the causes, both exits are abnormal since the exec didn't' 
do what was expected.
Only an exit status of 0 is 'normal' and in this case, that is not what 
expected.


Regards, Roger



    Alternatively:
    if (WIFEXITED) print("exited normaly with %d", WEXITSTATUS)
    else if (WIFSTOPPED) print("terminated with signal %d", WTERMSIG)


thanks for doing this.

I also think it's best to have different detail messages for normal 
termination and death by signal, as some shells do.


I might put the exception throwing code into a separate function.

Re: Review Request JDK-8222448: java/lang/reflect/PublicMethods/PublicMethodsTest.java times out

[David Holmes]

Hi Mandy,

Functional fix looks good, but layout and indentation appears off in the 
diff.


Thanks to Alan for the time spent investigating this!

Thanks,
David

On 4/06/2019 1:02 pm, Mandy Chung wrote:

test/jdk/java/lang/reflect/PublicMethods/PublicMethodsTest.java time out
in certain configuration e.g. fastdebug -Xcomp.  Setting the empty class
path significantly improves the execution time as it eliminates opening
and scanning of the JAR files on the class path.  Alan has also 
experimented

it on Windows machine.

diff --git 
a/test/jdk/java/lang/reflect/PublicMethods/PublicMethodsTest.java 
b/test/jdk/java/lang/reflect/PublicMethods/PublicMethodsTest.java

--- a/test/jdk/java/lang/reflect/PublicMethods/PublicMethodsTest.java
+++ b/test/jdk/java/lang/reflect/PublicMethods/PublicMethodsTest.java
@@ -222,6 +222,11 @@
  StandardJavaFileManager standardJavaFileManager =
  javac.getStandardFileManager(errorsCollector, Locale.ROOT,
Charset.forName("UTF-8"));
+    try {
+ standardJavaFileManager.setLocation(StandardLocation.CLASS_PATH, 
List.of());

+    } catch (IOException e) {
+    throw new UncheckedIOException(e);
+    }
  TestFileManager testFileManager = new TestFileManager(
  standardJavaFileManager, source);


Mandy

Re: Thread stack size issue related to glibc TLS bug

[David Holmes]

Hi Jiangli,

On 31/05/2019 1:23 am, Jiangli Zhou wrote:

Hi David,

This is a link to __pthread_get_minstack that I find in the public
domain: https://code.woboq.org/userspace/glibc/nptl/nptl-init.c.html.
It has copyright of 2002-2019, so it's probably the latest version.

size_t
__pthread_get_minstack (const pthread_attr_t *attr)
{
  return GLRO(dl_pagesize) + __static_tls_size + PTHREAD_STACK_MIN;
}

The PTHREAD_STACK_MIN appears to be 4-pages for the glibc version
(2.24) that I'm linking with. The dl_pagesize is 1-page.

We could go with the suggestion that you brought up earlier by only
adding the min_stack_size to the current thread stack size if it is
certain percent of the stack size. With the added dl_pagesize and
PTHREAD_STACK_MIN, 25% probably is reasonable? I've made changes
(including the percent suggestion) on top the existing patch and also
added a jtreg test based on the test case attached in JDK-8130425. On
JDK 13, the test could fail with different symptoms (segfault or hang)
depending on the TLS sizes without the fix.


We will need to do a CSR request for this behaviour change. I'm not sure 
if 25% may be reasonable or not. I would have opted for something 
smaller though as a starting point - say 10%. [Update: just saw the bug 
comment you added :) ]. So if the TLS is going to steal more than 10% of 
the requested stack then we add on the TLS size. Do we have anything to 
use as a guide here? How much TLS does that profiler use? Makes me think 
the percentage itself needs to be tunable via a flag ... that would also 
allow for it to be disabled completely.


Thanks,
David
-


BTW, I noticed that Florian had already made the comment change in
glibc for __pthread_get_minstack. Thanks!

Best regards,
Jiangli

On Wed, May 29, 2019 at 10:42 PM David Holmes  wrote:


Hi Florian,

On 25/05/2019 6:50 am, Florian Weimer wrote:

* Jiangli Zhou:


Hi Florian,

On Fri, May 24, 2019 at 2:46 AM Florian Weimer  wrote:


* Jiangli Zhou:


[3] change: http://cr.openjdk.java.net/~jiangli/tls_size/webrev/
(contributed by Jeremy Manson)


_dl_get_tls_static_info is an internal symbol (it carries a
GLIBC_PRIVATE symbol version).  Its implementation can change at any
time.  Please do not do this.


Point taken. Thanks.

It appears that asan is already carrying the same type of fix:

https://github.com/gcc-mirror/gcc/blob/master/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cc

As the issue has not been able to be addressed in the glibc layer, all
the others have to workaround it (and possibly by using the glibc
private APIs, _dl_get_tls_static_info or __pthread_get_minstack). That
would cause more dependencies on the private APIs.


_dl_get_tls_static_info changed ABI on i386 in glibc 2.27 already, so
you really don't want to use that (in case someone backported that
cleanup to an earlier version, although that might be bit unlikely).

The sanitizers are special and have a much shorter shelf life than the
OpenJDK binaries.


One alternative (besides fixing glibc) may be making
_dl_get_tls_static_info public. Would that be possible?


For __pthread_get_minstack, I can add a comment to the glibc sources
that if the ABI changes (or if TLS allocations are no longer considered
part of the stack), we should rename the function.  Then, as long as you
use a weak reference or dlsym (the latter is preferable for the sack of
RPM-based distributions which require special steps to reference
GLIBC_PRIVATE symbols directly), old binaries would keep working if we
make further changes.

Since _dl_get_tls_static_info already changed ABI once, I really can't
recommend its use.  Especially if you can work with
__pthread_get_minstack instead.


Can you explain for me what value this __pthread_get_minstack is defined
to return? Will it accommodate any required TLS plus some other glibc
specific overhead? How much memory are we talking about?


The value of PTHREAD_STACK_MIN is rather problematic on x86-64 (for the
reasons I explained earlier), but it's not likely we are going to change
the value of the constant any time soon.  It's more likely that we are
going to work around the AVX-512 register file issue by providing
exactly four usable stack pages with PTHREAD_STACK_MIN, and not less
than two as we did until recently.  So you can assume that it's indeed
possible to use PTHREAD_STACK_MIN and sysconf (_SC_PAGESIZE) to compute
the static TLS area size.


Sorry can you elaborate on that calculation please?

Thanks,
David
-


Thanks,
Florian

Re: RFR: 8219149: ProcessTools.ProcessBuilder should print timing info for subprocesses

[David Holmes]

Hi Kim,

This seems reasonable to me.

Thanks,
David

On 31/05/2019 7:04 am, Kim Barrett wrote:

On May 30, 2019, at 3:58 PM, Roger Riggs  wrote:

Hi Kim,

To ensure you see some messages in the case of timeouts, it would be
useful to call System.out.flush() after printing the message in logProcess().


Good idea; added.

full: http://cr.openjdk.java.net/~kbarrett/8219149/open.01/
incr: http://cr.openjdk.java.net/~kbarrett/8219149/open.01.inc/


I'm ok with the timestamps, as is, though the Duration might be useful in
some cases.


As I discussed with Roger, I want to keep the change simple for now,
as we're not yet sure where to expend effort looking deeper.  I think
comparing timestamps in just a few cases will tell us a lot (possibly
that we're looking in entirely the wrong place, in which case we might
just undo this change).

Re: Thread stack size issue related to glibc TLS bug

[David Holmes]

Hi Florian,

On 25/05/2019 6:50 am, Florian Weimer wrote:

* Jiangli Zhou:


Hi Florian,

On Fri, May 24, 2019 at 2:46 AM Florian Weimer  wrote:


* Jiangli Zhou:


[3] change: http://cr.openjdk.java.net/~jiangli/tls_size/webrev/
(contributed by Jeremy Manson)


_dl_get_tls_static_info is an internal symbol (it carries a
GLIBC_PRIVATE symbol version).  Its implementation can change at any
time.  Please do not do this.


Point taken. Thanks.

It appears that asan is already carrying the same type of fix:

https://github.com/gcc-mirror/gcc/blob/master/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cc

As the issue has not been able to be addressed in the glibc layer, all
the others have to workaround it (and possibly by using the glibc
private APIs, _dl_get_tls_static_info or __pthread_get_minstack). That
would cause more dependencies on the private APIs.


_dl_get_tls_static_info changed ABI on i386 in glibc 2.27 already, so
you really don't want to use that (in case someone backported that
cleanup to an earlier version, although that might be bit unlikely).

The sanitizers are special and have a much shorter shelf life than the
OpenJDK binaries.


One alternative (besides fixing glibc) may be making
_dl_get_tls_static_info public. Would that be possible?


For __pthread_get_minstack, I can add a comment to the glibc sources
that if the ABI changes (or if TLS allocations are no longer considered
part of the stack), we should rename the function.  Then, as long as you
use a weak reference or dlsym (the latter is preferable for the sack of
RPM-based distributions which require special steps to reference
GLIBC_PRIVATE symbols directly), old binaries would keep working if we
make further changes.

Since _dl_get_tls_static_info already changed ABI once, I really can't
recommend its use.  Especially if you can work with
__pthread_get_minstack instead.


Can you explain for me what value this __pthread_get_minstack is defined 
to return? Will it accommodate any required TLS plus some other glibc 
specific overhead? How much memory are we talking about?



The value of PTHREAD_STACK_MIN is rather problematic on x86-64 (for the
reasons I explained earlier), but it's not likely we are going to change
the value of the constant any time soon.  It's more likely that we are
going to work around the AVX-512 register file issue by providing
exactly four usable stack pages with PTHREAD_STACK_MIN, and not less
than two as we did until recently.  So you can assume that it's indeed
possible to use PTHREAD_STACK_MIN and sysconf (_SC_PAGESIZE) to compute
the static TLS area size.


Sorry can you elaborate on that calculation please?

Thanks,
David
-


Thanks,
Florian

Re: Thread stack size issue related to glibc TLS bug

[David Holmes]

Hi Florian,

On 24/05/2019 8:13 pm, Florian Weimer wrote:

* David Holmes:


My thoughts haven't really changed since 2015 - and sadly neither has
there been any change in glibc in that time. Nor, to my recollection,
have there been any other reported issues with this.


The issue gets occasionally reported by people who use small stacks with
large initial-exec TLS consumers (such as jemalloc).  On the glibc side,
we aren't entirely sure what to do about this.  We have recently tweaked
the stack size computation, so that in many cases, threads now receive
an additional page.  This was necessary to work around a kernel/hardware
change where context switches started to push substantially more data on
the stack than before, and minimal stack sizes did not work anymore on
x86-64 (leading to ntpd crashing during startup, among other things).

The main concern is that for workloads with carefully tuned stack sizes,
revamping the stack size computation so that TLS is no longer
effectively allocated on the stack might result in address space
exhaustion.  (This should only be a concern on 32-bit architectures.)

Even if we changed this today (or had changed it in 2015), it would take
a long time for the change to end up with end users, so it's unclear how
much help it would be.


If it had been fixed in 2012 it wouldn't be an issue today. If it gets 
fixed today then it may not be an issue in 2025. If it is not fixed then 
it will always be an issue. Stealing the TLS space out of the stack 
requested by the user is just not a reasonable thing to do IMHO.



Maybe OpenJDK can add a property specifying a stack size reserve, and
htis number is added to all stack size requests?  This will at least
allow users to work around the issue locally.


This would be a low-impact workaround, though as Jiangli points out it 
is a bit hard on the end-user as they first have to hit the problem, 
then recognize what it is, then realize there's a potential solution and 
then determine the right magic number to use. Better than nothing but 
not ideal.


Further follow up coming in response to your later email.

Cheers,
David
-



If we change the accounting in glibc, we will have to add a similar
tunable on the glibc side, too.

Thanks,
Florian

Re: RFR: 8219149: ProcessTools.ProcessBuilder should print timing info for subprocesses

[David Holmes]

Adding back hotspot-dev. I don't think Kim saw this.

David

On 30/05/2019 4:04 am, Roger Riggs wrote:

Hi Kim,

In the normal output less output is cleaner.
It would be more useful to have the Duration of the wait and end time of 
the wait.

(It saves the reader doing subtraction of times).

+  Instant start = Instant.now();
+  boolean aborted = true;
+  try {
+  int result = p.waitFor();
+  aborted = false;
+  return result;
+  } finally {
+  Duration dur = Duration.between(start, Instant.now());
+  if (aborted) {
+  logProgress("Waiting for completion FAILED after: " + 
dur);

+  } else {
+  logProgress("Waiting for completion finished after: " 
+ dur);

+  }
+  }

Or something like that.

Roger


On 05/29/2019 01:24 PM, Kim Barrett wrote:

[I’m not completely sure where this RFR should be sent, but core-libs-dev
and hotspot-dev seems likely to get reasonable coverage of those who
might care.]

Please review this change to the test library to add some "logging"
output to tests that spawn a child process to perform the test and
then analyze that child's output.  We are seeing occasional timeouts
in such tests whose cause is hard to pin down.  It's not clear whether
the excess time is in the child or instead some problem in the testing
framework.  The new logging output provides timestamps for (1) the
start of output collection from the child, (2) the start of waiting
for the child to terminate, and (3) the child's termination.  This
should be enough to determine whether the child is taking too long,
and hint at where (e.g. termination or not).

CR:
https://bugs.openjdk.java.net/browse/JDK-8219149

Webrev:
http://cr.openjdk.java.net/~kbarrett/8219149/open.00/

Testing:
Local hs-tier1 and verified the expected logging output is present.
mach5 tier1-3 to ensure there aren't any "obvious" unexpected problems
caused by the new logging.  (It seems unlikely, but...)

Re: RFR 8220238 : Enhancing j.l.Runtime/System::gc specification with an explicit 'no guarantee' statement

[David Holmes]

Hi Roger,

I think it is important that the "best effort" be kept as per this 
version. It may not be a directly testable property but it does capture 
intent regarding quality-of-implementation IMO.


Thanks,
David

On 30/05/2019 5:25 am, Roger Riggs wrote:

Hi,

ok, thanks for the comments.

Any other comments on:
"* Runs the garbage collector in the Java Virtual Machine.
* 
* Calling this method suggests that the Java Virtual Machine
* expend effort toward recycling unused objects in order to
* make the memory they currently occupy available for reuse
* by the Java Virtual Machine.
* When control returns from the method call, the Java Virtual Machine
* has made a best effort to reclaim space from all discarded objects.
* There is no guarantee that this effort will recycle any particular
* number of unused objects, reclaim any particular amount of space,
* or complete at any particular time, if at all.
"

Issue:
https://bugs.openjdk.java.net/browse/JDK-8220238

CSR:
https://bugs.openjdk.java.net/browse/JDK-8224760

Webrev:
   http://cr.openjdk.java.net/~rriggs/webrev-gc-8220238-1/

Thanks, Roger

On 05/29/2019 01:38 PM, Aleksey Shipilev wrote:

On 5/29/19 5:06 PM, Roger Riggs wrote:
The language is hard to construct, in part because of the innovations 
in gc technology that have

resulted such a wide range of behaviors and timing.

I'll restore the sentence "When control returns"... but I don't think 
it carries any
definite requirement on an implementation. It is benign since 'best 
effort' is undefined

and depends on the unspecified intentions of the implementation(s).
"When control returns" does mean things for me (GC implementor). We 
specifically handle blocking
callers on System.gc() calls in Shenandoah (and partially in Epsilon). 
I would not be surprised if

JCK has tests for that as well.

I think "best effort" is orthogonal to blocking behavior.

-Aleksey

Re: Thread stack size issue related to glibc TLS bug

[David Holmes]

Hi Jiangli,

On 24/05/2019 9:21 am, Jiangli Zhou wrote:

Hi David (and others),

There was a discussion [1] (between you, Jeremy, Martin and others)
back in 2015 regarding a stack size issue caused by a glibc bug
related to TLS (Thread local storage) [2]. The issue was manifested as
a StackOverflowError with the reported test in JDK-8130425 [0] when
large TLS size is used. A workaround was introduced with
-Djdk.lang.processReaperUseDefaultStackSize. Based on the glibc
discussion thread [2], Rust implemented a fix by taking into account
of the TLS size. From one of the comments in the OpenJDK discussion
archive [3], looks like you considered similar fix could be applied
for JVM. I talked to Jeremy about sharing his fix for this particular
issue today. The fix appears to be a more general solution than the
processReaperUseDefaultStackSize workaround. It has been tested/used
for server years and seems to be stable. The link to the changeset is
listed below. Please let me know your thoughts on taking the change in
OpenJDK.


My thoughts haven't really changed since 2015 - and sadly neither has 
there been any change in glibc in that time. Nor, to my recollection, 
have there been any other reported issues with this.


If this were to be taken into hotspot then I think it has to be opt-in 
via a flag so that it doesn't make sudden and unexpected differences in 
the number of threads an application can create. It may also be worth 
considering, from the bugzilla discussion, only adding in the TLS size 
if it is greater than a certain percentage of the stack size being 
requested. That would limit the impact to threads with small stacks 
without forcing every thread to have to grow by the TLS size.


But I'd want to know how often this is actually needed. As Andrew Haley 
said in the original discussion thread "I think we're rather looking at 
abuse of TLS here.".


And I'd need to understand better what versions of glibc this would work 
for (and how they relate to current distros).


Cheers,
David


[0] JDK bug: https://bugs.openjdk.java.net/browse/JDK-8130425
[1] OpenJDK discussion archive:
http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-December/037558.html
[2] glibc discussion archive:
http://sourceware.org/bugzilla/show_bug.cgi?id=11787
[3] change: http://cr.openjdk.java.net/~jiangli/tls_size/webrev/
(contributed by Jeremy Manson)

The #ifdef __GLIBC__ in the change could be removed as os_linux.cpp
already makes assumption about the use of glibc.

Best regards,
Jiangli

Re: RFR: 8224656: Problem list java/security/SecureClassLoader/DefineClass.java until JDK-8224635 is fixed

[David Holmes]

Looks good! Thanks Daniel.

David

On 23/05/2019 7:32 pm, Daniel Fuchs wrote:

Hi,

Please find a patch below that temporarily problem lists
java/security/SecureClassLoader/DefineClass.java
on linux and windows until JDK-8224635 [1] is fixed:

8224656: Problem list java/security/SecureClassLoader/DefineClass.java
  until JDK-8224635 is fixed
https://bugs.openjdk.java.net/browse/JDK-8224656

webrev:
http://cr.openjdk.java.net/~dfuchs/webrev_8224656/webrev.00/

best regards

-- daniel

diff --git a/test/jdk/ProblemList.txt b/test/jdk/ProblemList.txt
--- a/test/jdk/ProblemList.txt
+++ b/test/jdk/ProblemList.txt
@@ -664,6 +664,7 @@
  sun/security/provider/KeyStore/DKSTest.sh 8180266 windows-all

  sun/security/pkcs11/KeyStore/SecretKeysBasic.sh 8209398 generic-all
+java/security/SecureClassLoader/DefineClass.java    8224635 
windows-all,linux-all



 

Re: RFR: 8215156: Deprecate the -Xfuture option

[David Holmes]

Looks good.

Thanks,
David

On 23/05/2019 12:44 pm, Henry Jen wrote:



On May 22, 2019, at 7:04 PM, David Holmes  wrote:

On 23/05/2019 11:56 am, Henry Jen wrote:

Thanks for the quick review,

On May 22, 2019, at 6:34 PM, David Holmes  wrote:

Hi Henry,

On 23/05/2019 11:07 am, Henry Jen wrote:

Hi,
Please review a webrev[1] to deprecate the -Xfuture option per CSR-8224524[2].


src/hotspot/share/Xusage.txt

Ignoring the usefulness, or otherwise of this file, the entry for Xfuture should not be 
deleted (that happens when an option is actually removed) but just have 
"(deprecated)" added to it.


I was wondering if I should prefix (deprecated) or append to the content. It 
make the line over the 80 so I decided just remove it since this document is 
not official we don’t want to advocate that option any more.
So, we have couple options,
  -Xfuture  enable strictest checks, anticipating future default
(deprecated)
  -Xrs  reduce use of OS signals by Java/VM (see documentation)


I prefer option 1.



OK, updated[1].

[1] https://cr.openjdk.java.net/~henryjen/jdk13/8215156/open/1/webrev/

Cheers,
Henry

Re: RFR: 8215156: Deprecate the -Xfuture option

[David Holmes]

On 23/05/2019 11:56 am, Henry Jen wrote:

Thanks for the quick review,


On May 22, 2019, at 6:34 PM, David Holmes  wrote:

Hi Henry,

On 23/05/2019 11:07 am, Henry Jen wrote:

Hi,
Please review a webrev[1] to deprecate the -Xfuture option per CSR-8224524[2].


src/hotspot/share/Xusage.txt

Ignoring the usefulness, or otherwise of this file, the entry for Xfuture should not be 
deleted (that happens when an option is actually removed) but just have 
"(deprecated)" added to it.



I was wondering if I should prefix (deprecated) or append to the content. It 
make the line over the 80 so I decided just remove it since this document is 
not official we don’t want to advocate that option any more.

So, we have couple options,


  -Xfuture  enable strictest checks, anticipating future default
(deprecated)
  -Xrs  reduce use of OS signals by Java/VM (see documentation)


I prefer option 1.


or

 -Xfuture  enable strictest checks (deprecated)

or

 -Xfuture  (deprecated)
   enable strictest checks, anticipating future default

or keep in one line don’t mind the 80 width.

Let me know which do we prefer.


---

In hotspot we include the version of the JDK that deprecated the option:

"Option %s was deprecated in version %s and will likely be removed in a future 
release."

Perhaps you could do the same for the launcher?



During the CSR discussion, we asked Dr Deprecator’s opinion and decided on
“XXX is deprecated and may be removed in a future release.”

It was suggested to change JVM message to match.


Okay.

Thanks,
David


Cheers,
Henry


Otherwise seems fine.

Thanks,
David
-


Cheers,
Henry
  [1] https://cr.openjdk.java.net/~henryjen/jdk13/8215156/open/webrev/
[2] https://bugs.openjdk.java.net/browse/JDK-8224524

Re: RFR: 8215156: Deprecate the -Xfuture option

[David Holmes]

Hi Henry,

On 23/05/2019 11:07 am, Henry Jen wrote:

Hi,

Please review a webrev[1] to deprecate the -Xfuture option per CSR-8224524[2].


src/hotspot/share/Xusage.txt

Ignoring the usefulness, or otherwise of this file, the entry for 
Xfuture should not be deleted (that happens when an option is actually 
removed) but just have "(deprecated)" added to it.


---

In hotspot we include the version of the JDK that deprecated the option:

"Option %s was deprecated in version %s and will likely be removed in a 
future release."


Perhaps you could do the same for the launcher?

Otherwise seems fine.

Thanks,
David
-


Cheers,
Henry
  
[1] https://cr.openjdk.java.net/~henryjen/jdk13/8215156/open/webrev/

[2] https://bugs.openjdk.java.net/browse/JDK-8224524

Re: RFR: 8224629: Unnecessary indirection in LambdaToMethod

[David Holmes]

Hi Alan,

This javac change should be reviewed on compiler-dev not core-libs-dev.

Thanks,
David

On 23/05/2019 10:42 am, Alan Malloy wrote:

Hello. Please review this patch to eliminate an unnecessary upcast and
method call in LambdaToMethod.

bug: https://bugs.openjdk.java.net/browse/JDK-8224629
webrev: http://cr.openjdk.java.net/~cushon/8224629/webrev.00/

Testing: All of jtreg:test/langtools/tools/javac pass locally. No new test
added, as no behavior change is intended.

Thanks,
 Alan

Re: RFR: 8218997: Xusage text, man help, etc doesn't mention -Xlog option.

[David Holmes]

That works for me. :)

Thanks,
David

On 22/05/2019 9:40 am, Henry Jen wrote:

Good suggestion, didn’t know about that page. I took what’s in Xusage.txt. 
Updated as following,

diff -r cd3c74c0 
src/java.base/share/classes/sun/launcher/resources/launcher.properties
--- a/src/java.base/share/classes/sun/launcher/resources/launcher.properties
Tue May 21 15:08:50 2019 -0700
+++ b/src/java.base/share/classes/sun/launcher/resources/launcher.properties
Tue May 21 16:38:42 2019 -0700
@@ -138,6 +138,9 @@
  \-Xinternalversion\n\
  \  displays more detailed JVM version information than 
the\n\
  \  -version option\n\
+\-Xlog:  Configure or enable logging with the Java Virtual\n\
+\  Machine (JVM) unified logging framework. Use 
-Xlog:help\n\
+\  for details.\n\
  \-Xloggc:log GC status to a file with time stamps\n\
  \-Xmixed   mixed mode execution (default)\n\
  \-Xmnsets the initial and maximum size (in bytes) of the 
heap\n\
diff -r cd3c74c0 src/java.base/share/man/java.1
--- a/src/java.base/share/man/java.1Tue May 21 15:08:50 2019 -0700
+++ b/src/java.base/share/man/java.1Tue May 21 16:38:42 2019 -0700
@@ -732,6 +732,11 @@
  option, and then exits\&.
  .RE
  .PP
+\-Xlog:\fIopts\fR
+.RS 4
+Configure or enable logging with the Java Virtual Machine (JVM) unified 
logging framework. Use \fB\-Xlog:help\fR for details.
+.RE
+.PP
  \-Xloggc:\fIfilename\fR
  .RS 4
  Sets the file to which verbose GC events information should be redirected for 
logging\&. The information written to this file is similar to the output of


Cheers,
Henry



On May 21, 2019, at 4:11 PM, David Holmes  wrote:

Hi Henry,

On 22/05/2019 8:41 am, Henry Jen wrote:

Hi,
Please review a trivial patch that add some hints about how to use -Xlog in 
java help and man page.
diff -r cd3c74c0 
src/java.base/share/classes/sun/launcher/resources/launcher.properties
--- a/src/java.base/share/classes/sun/launcher/resources/launcher.properties
Tue May 21 15:08:50 2019 -0700
+++ b/src/java.base/share/classes/sun/launcher/resources/launcher.properties
Tue May 21 15:31:52 2019 -0700
@@ -138,6 +138,7 @@
  \-Xinternalversion\n\
  \  displays more detailed JVM version information than 
the\n\
  \  -version option\n\
+\-Xlog:  control JVM logging, use -Xlog:help for details


I would suggest using the same words as used in the online tool reference page 
[1]:

Configure or enable logging with the Java Virtual Machine (JVM) unified logging 
framework. Use -Xlog:help for details.

Ditto for the man page.

Cheers,
David

[1] 
https://docs.oracle.com/en/java/javase/12/tools/java.html#GUID-3B1CE181-CD30-4178-9602-230B800D4FAE


  \-Xloggc:log GC status to a file with time stamps\n\
  \-Xmixed   mixed mode execution (default)\n\
  \-Xmnsets the initial and maximum size (in bytes) of the 
heap\n\
diff -r cd3c74c0 src/java.base/share/man/java.1
--- a/src/java.base/share/man/java.1Tue May 21 15:08:50 2019 -0700
+++ b/src/java.base/share/man/java.1Tue May 21 15:31:52 2019 -0700
@@ -732,6 +732,11 @@
  option, and then exits\&.
  .RE
  .PP
+\-Xlog:\fIopts\fR
+.RS 4
+Control JVM logging, use \fB\-Xlog:help\fR for details.
+.RE
+.PP
  \-Xloggc:\fIfilename\fR
  .RS 4
  Sets the file to which verbose GC events information should be redirected for 
logging\&. The information written to this file is similar to the output of
Cheers,
Henry

Re: RFR: 8218997: Xusage text, man help, etc doesn't mention -Xlog option.

[David Holmes]

Hi Henry,

On 22/05/2019 8:41 am, Henry Jen wrote:

Hi,

Please review a trivial patch that add some hints about how to use -Xlog in 
java help and man page.

diff -r cd3c74c0 
src/java.base/share/classes/sun/launcher/resources/launcher.properties
--- a/src/java.base/share/classes/sun/launcher/resources/launcher.properties
Tue May 21 15:08:50 2019 -0700
+++ b/src/java.base/share/classes/sun/launcher/resources/launcher.properties
Tue May 21 15:31:52 2019 -0700
@@ -138,6 +138,7 @@
  \-Xinternalversion\n\
  \  displays more detailed JVM version information than 
the\n\
  \  -version option\n\
+\-Xlog:  control JVM logging, use -Xlog:help for details


I would suggest using the same words as used in the online tool 
reference page [1]:


Configure or enable logging with the Java Virtual Machine (JVM) unified 
logging framework. Use -Xlog:help for details.


Ditto for the man page.

Cheers,
David

[1] 
https://docs.oracle.com/en/java/javase/12/tools/java.html#GUID-3B1CE181-CD30-4178-9602-230B800D4FAE



  \-Xloggc:log GC status to a file with time stamps\n\
  \-Xmixed   mixed mode execution (default)\n\
  \-Xmnsets the initial and maximum size (in bytes) of the 
heap\n\
diff -r cd3c74c0 src/java.base/share/man/java.1
--- a/src/java.base/share/man/java.1Tue May 21 15:08:50 2019 -0700
+++ b/src/java.base/share/man/java.1Tue May 21 15:31:52 2019 -0700
@@ -732,6 +732,11 @@
  option, and then exits\&.
  .RE
  .PP
+\-Xlog:\fIopts\fR
+.RS 4
+Control JVM logging, use \fB\-Xlog:help\fR for details.
+.RE
+.PP
  \-Xloggc:\fIfilename\fR
  .RS 4
  Sets the file to which verbose GC events information should be redirected for 
logging\&. The information written to this file is similar to the output of

Cheers,
Henry

Re: RFR8220166 : Performance regression in deserialization

[David Holmes]

Hi Roger,

On 16/05/2019 6:32 am, Roger Riggs wrote:
Please review a change in the synchronization during the creation of an 
ObjectInputStream.
Currently, a synchronized block is used to initialize the streams filter 
is read the global serial filter
which becomes a bottleneck under high concurrency.  Since the value only 
ever changes from null to non-null
once, the synchronization is not needed.  Changing the field to volatile 
and removing

the synchronization on read should alleviate the contention.


That looks fine to me. Even if the value could change repeatedly you 
don't need the sync on the read, as long as its volatile.


Cheers,
David
-


Issue:
   https://bugs.openjdk.java.net/browse/JDK-8220166

Webrev:
   http://cr.openjdk.java.net/~rriggs/webrev-fix-8220166/index.html

Thanks, Roger

Re: RFR: 8223553: Fix code constructs that do not compile with the Eclipse Java Compiler

[David Holmes]

Hi Christoph,

I'm very reluctant to see changes like this that the compiler folk have 
not determined are actually incorrect. That said ...


On 15/05/2019 7:03 am, Langer, Christoph wrote:

Thanks Daniel.

Can anybody help reviewing the changes to:
src/java.base/share/classes/java/lang/invoke/MethodHandles.java


The introduction of the intermediate local variable seems harmless 
(though why it should be necessary is another matter).



src/java.base/share/classes/java/util/concurrent/ConcurrentSkipListMap.java


As you note this should be ok'd by jsr166 folk so I've cc'd Martin 
Buccholz. I dislike seeing a raw type introduced here though.



and
src/java.management/share/classes/java/lang/management/ManagementFactory.java ?


Introducing an unchecked cast seems very crude. I'd want the core-libs 
stream experts to comment on this.


Cheers,
David



Thanks
Christoph


-Original Message-
From: Daniel Fuchs 
Sent: Dienstag, 14. Mai 2019 18:04
To: Langer, Christoph ; core-libs-dev ; net-dev 
Cc: compiler-...@openjdk.java.net
Subject: Re: RFR: 8223553: Fix code constructs that do not compile with the
Eclipse Java Compiler

Hi Christoph,

That looks much better, thanks!
(but still not commenting on the other changes ;-))

best regards,

-- daniel

On 14/05/2019 13:57, Langer, Christoph wrote:

Hi Daniel,


unfortunately, your proposed solution does not work with javac. I get

this

in the build:

Oh darn. I should have double checked.
Can we at least reduce the scope of the @SuppressedWarnings by
introducing a private method that just has the return call?


Sure, what about this one:

http://cr.openjdk.java.net/~clanger/webrevs/8223553.2/ ?


Thanks
Christoph

Re: RFR: 8222533 - jtreg test jdk/internal/platform/cgroup/TestCgroupMetrics.java fails on SLES12.3 linux ppc64le machine

[David Holmes]

Hi Bob,

This fix seems quite reasonable.

Thanks,
David

On 8/05/2019 6:25 am, Bob Vandette wrote:

Please review this simple fix for a TestCgroupMetrics.java test failure.

--- a/src/java.base/linux/classes/jdk/internal/platform/cgroupv1/SubSystem.java
+++ b/src/java.base/linux/classes/jdk/internal/platform/cgroupv1/SubSystem.java
@@ -108,7 +108,7 @@
  try {
  List lines = 
Files.readAllLines(Paths.get(subsystem.path(), param));
  for (String line: lines) {
-if (line.contains(match)) {
+if (line.startsWith(match)) {
  retval = conversion.apply(line);
  break;
  }

Under docker we typically only see a single block I/O device so
the test passed since both lines containing “Total” are the same value.

8:16 Read 4452352
8:16 Write 0
8:16 Sync 0
8:16 Async 4452352
8:16 Total 4452352
Total 4452352

It’s possible and likely that there will be multiple devices causing failures
since the test and the Metrics API are not examining the same lines.

249:0 Read 10477568
249:0 Write 294431895552
249:0 Sync 17292476416
249:0 Async 277149896704
249:0 Total 294442373120 <——— The API was returning this
8:16 Read 19017216
8:16 Write 326780178432
8:16 Sync 17398915072
8:16 Async 309400280576
8:16 Total 326799195648
8:0 Read 27092992
8:0 Write 31070281728
8:0 Sync 10728873984
8:0 Async 20368500736
8:0 Total 31097374720
Total 652338943488 <—— Test test was comparing to this

We are after the line that “startsWith” “Total”.

Bob.

Re: RFR: jsr166 integration 2019-05

[David Holmes]

Hi Martin,

On 30/04/2019 4:00 am, Martin Buchholz wrote:

https://cr.openjdk.java.net/~martin/webrevs/jdk/jsr166-integration/overview.html

8222930: ConcurrentSkipListMap.clone() shares size variable between
original and clone
https://cr.openjdk.java.net/~martin/webrevs/jdk/jsr166-integration/ConcurrentSkipListMap.clone/index.html
https://bugs.openjdk.java.net/browse/JDK-8222930
A real bug fix!
Apologies to Adam for re-routing through jsr166 CVS.
Adam, that loop is now written in Adam style.

8221120: CopyOnWriteArrayList.set should always have volatile write
semantics
https://cr.openjdk.java.net/~martin/webrevs/jdk/jsr166-integration/CopyOnWriteArrayList.set/index.html
https://bugs.openjdk.java.net/browse/JDK-8221120

8221892: ThreadPoolExecutor: Thread.isAlive() is not equivalent to not
being startable
https://cr.openjdk.java.net/~martin/webrevs/jdk/jsr166-integration/isAlive/index.html
https://bugs.openjdk.java.net/browse/JDK-8221892


That one looks good to me.

Thanks,
David
-


8220248: fix headings in java.util.concurrent
https://cr.openjdk.java.net/~martin/webrevs/jdk/jsr166-integration/h3/index.html
https://bugs.openjdk.java.net/browse/JDK-8220248

8203662: remove increment of modCount from ArrayList and Vector replaceAll()
https://cr.openjdk.java.net/~martin/webrevs/jdk/jsr166-integration/replaceAll/index.html
https://bugs.openjdk.java.net/browse/JDK-8203662
This one remains in limbo.
Some impartial openjdk steward should cast a tiebreaker vote or appoint
someone impartial to cast a tiebreaker vote.

8219138: Miscellaneous changes imported from jsr166 CVS 2019-05
https://cr.openjdk.java.net/~martin/webrevs/jdk/jsr166-integration/miscellaneous/index.html

Re: RFR(S): 8222518: Remove unnecessary caching of Parker object in java.lang.Thread

[David Holmes]

On 26/04/2019 7:25 pm, Alan Bateman wrote:

On 26/04/2019 06:32, David Holmes wrote:
I pushed this today based on Dan and Robbin's reviews, but realized 
just after the act that I should have waited for any feedback from 
core-libs - apologies about that. If there are concerns I will roll it 
back.
I don't think there are any concerns, it is of course very welcome to 
remove a field from Thread.


Thanks Alan!

David


-Alan

Re: RFR(S): 8222518: Remove unnecessary caching of Parker object in java.lang.Thread

[David Holmes]

I pushed this today based on Dan and Robbin's reviews, but realized just 
after the act that I should have waited for any feedback from core-libs 
- apologies about that. If there are concerns I will roll it back.


Thanks,
David
-

On 26/04/2019 2:57 pm, David Holmes wrote:

Thanks Dan! Extraneous ; culled.

David

On 25/04/2019 1:16 am, Daniel D. Daugherty wrote:

On 4/24/19 3:12 AM, David Holmes wrote:

Bug: https://bugs.openjdk.java.net/browse/JDK-8222518
webrev: http://cr.openjdk.java.net/~dholmes/8222518/webrev/


src/hotspot/share/classfile/javaClasses.cpp
 L1629:   macro(_park_blocker_offset,  k, "parkBlocker", 
object_signature, false);
 Line ends with a ';' and the previous last line did not. When 
the
         THREAD_FIELDS_DO macro is called, it is already followed by a 
';':


 L1635:   THREAD_FIELDS_DO(FIELD_COMPUTE_OFFSET);
 L1640:   THREAD_FIELDS_DO(FIELD_SERIALIZE_OFFSET);

src/hotspot/share/classfile/javaClasses.hpp
 No comments.

src/hotspot/share/prims/unsafe.cpp
 No comments.

src/java.base/share/classes/java/lang/Thread.java
 No comments.

Thumbs up.  I don't need to see another webrev if you choose to remove
the ';' on L1629.

Dan



The original implementation of Unsafe.unpark simply extracted the 
JavaThread reference from the java.lang.Thread oop and if non-null 
extracted the Parker instance from it and invoked unpark. This was 
racy however as the native JavaThread could terminate at any time and 
deallocate the Parker.


That logic was fixed by JDK-6271298 which used of combination of 
type-stable-memory "event" objects for the Parker, along with use of 
the Threads_lock to obtain the initial reference to the Parker (from 
a JavaThread guaranteed to be alive), together with caching the 
native Parker pointer in a field of java.lang.Thread. Even though the 
native thread may have terminated the Parker was still valid (even if 
associated with a different thread) and the unpark at worst was a 
spurious wakeup for that other thread.


When JDK-8167108 introduced Thread Safe-Memory-Reclaimation (SMR) the 
logic was updated to always use the safe mechanism - we grab a 
ThreadsListHandle then check the cached field, else lookup the native 
thread to see if it is alive and locate the Parker instance that way.
With SMR the caching of the Parker pointer no longer serves any 
purpose - we no longer have a lock-free use-the-cache path versus a 
lock-using populate-the-cache path. With SMR we've already"paid" for 
the ability to ensure the native thread can't terminate regardless of 
whether we lookup the field from the java.lang.Thread or the 
JavaThread. So we can simplify the code and save a little footprint 
by removing the cache from java.lang.Thread:


    /*
 * JVM-private state that persists after native thread termination.
 */
    private long nativeParkEventPointer;

and the supporting code from unsafe.cpp and javaClass.*pp in the JVM.

I considered restoring the fast-path use of the cache without 
recourse to Thread-SMR but performance measurements failed to show 
any benefit in doing. See bug report for details.


Thanks,
David

Re: RFR(S): 8222518: Remove unnecessary caching of Parker object in java.lang.Thread

[David Holmes]

Hi Robbin,

On 25/04/2019 5:53 pm, Robbin Ehn wrote:

Hi David,

Looks good.


Thanks for the review.


Just a question:
It seems like we could just hold the ThreadsList over p->unpark() and 
not rely on TSM ?


Yes now it is done this way we could do the unpark while holding the TLH 
and avoid relying on TSM.


Not sure in how many places we do rely on it, but it would be nice to 
remove TSM for parkers.


TSM for Parkers was introduced by JDK-6271298 (there's a typo in the 
comment in park.hpp that transposes the last 2 numbers of the bug) so I 
think this is the only usage that relies on it.


The exiting thread would set parker to NULL before removing itself from 
the threadslist and free it when it's off.


I don't think we need that complexity. It should suffice change:

JavaThread::~JavaThread() {

  // JSR166 -- return the parker to the free list
  Parker::Release(_parker);
  _parker = NULL;

to do "delete _parker" instead.

I'll file a RFE for that.

Thanks,
David




Thanks, Robbin

On 4/24/19 9:12 AM, David Holmes wrote:

Bug: https://bugs.openjdk.java.net/browse/JDK-8222518
webrev: http://cr.openjdk.java.net/~dholmes/8222518/webrev/

The original implementation of Unsafe.unpark simply extracted the 
JavaThread reference from the java.lang.Thread oop and if non-null 
extracted the Parker instance from it and invoked unpark. This was 
racy however as the native JavaThread could terminate at any time and 
deallocate the Parker.


That logic was fixed by JDK-6271298 which used of combination of 
type-stable-memory "event" objects for the Parker, along with use of 
the Threads_lock to obtain the initial reference to the Parker (from a 
JavaThread guaranteed to be alive), together with caching the native 
Parker pointer in a field of java.lang.Thread. Even though the native 
thread may have terminated the Parker was still valid (even if 
associated with a different thread) and the unpark at worst was a 
spurious wakeup for that other thread.


When JDK-8167108 introduced Thread Safe-Memory-Reclaimation (SMR) the 
logic was updated to always use the safe mechanism - we grab a 
ThreadsListHandle then check the cached field, else lookup the native 
thread to see if it is alive and locate the Parker instance that way.
With SMR the caching of the Parker pointer no longer serves any 
purpose - we no longer have a lock-free use-the-cache path versus a 
lock-using populate-the-cache path. With SMR we've already"paid" for 
the ability to ensure the native thread can't terminate regardless of 
whether we lookup the field from the java.lang.Thread or the 
JavaThread. So we can simplify the code and save a little footprint by 
removing the cache from java.lang.Thread:


 /*
  * JVM-private state that persists after native thread termination.
  */
 private long nativeParkEventPointer;

and the supporting code from unsafe.cpp and javaClass.*pp in the JVM.

I considered restoring the fast-path use of the cache without recourse 
to Thread-SMR but performance measurements failed to show any benefit 
in doing. See bug report for details.


Thanks,
David

Re: RFR(S): 8222518: Remove unnecessary caching of Parker object in java.lang.Thread

[David Holmes]

Thanks Dan! Extraneous ; culled.

David

On 25/04/2019 1:16 am, Daniel D. Daugherty wrote:

On 4/24/19 3:12 AM, David Holmes wrote:

Bug: https://bugs.openjdk.java.net/browse/JDK-8222518
webrev: http://cr.openjdk.java.net/~dholmes/8222518/webrev/


src/hotspot/share/classfile/javaClasses.cpp
     L1629:   macro(_park_blocker_offset,  k, "parkBlocker", 
object_signature, false);

     Line ends with a ';' and the previous last line did not. When the
         THREAD_FIELDS_DO macro is called, it is already followed by a ';':

     L1635:   THREAD_FIELDS_DO(FIELD_COMPUTE_OFFSET);
     L1640:   THREAD_FIELDS_DO(FIELD_SERIALIZE_OFFSET);

src/hotspot/share/classfile/javaClasses.hpp
     No comments.

src/hotspot/share/prims/unsafe.cpp
     No comments.

src/java.base/share/classes/java/lang/Thread.java
     No comments.

Thumbs up.  I don't need to see another webrev if you choose to remove
the ';' on L1629.

Dan



The original implementation of Unsafe.unpark simply extracted the 
JavaThread reference from the java.lang.Thread oop and if non-null 
extracted the Parker instance from it and invoked unpark. This was 
racy however as the native JavaThread could terminate at any time and 
deallocate the Parker.


That logic was fixed by JDK-6271298 which used of combination of 
type-stable-memory "event" objects for the Parker, along with use of 
the Threads_lock to obtain the initial reference to the Parker (from a 
JavaThread guaranteed to be alive), together with caching the native 
Parker pointer in a field of java.lang.Thread. Even though the native 
thread may have terminated the Parker was still valid (even if 
associated with a different thread) and the unpark at worst was a 
spurious wakeup for that other thread.


When JDK-8167108 introduced Thread Safe-Memory-Reclaimation (SMR) the 
logic was updated to always use the safe mechanism - we grab a 
ThreadsListHandle then check the cached field, else lookup the native 
thread to see if it is alive and locate the Parker instance that way.
With SMR the caching of the Parker pointer no longer serves any 
purpose - we no longer have a lock-free use-the-cache path versus a 
lock-using populate-the-cache path. With SMR we've already"paid" for 
the ability to ensure the native thread can't terminate regardless of 
whether we lookup the field from the java.lang.Thread or the 
JavaThread. So we can simplify the code and save a little footprint by 
removing the cache from java.lang.Thread:


    /*
 * JVM-private state that persists after native thread termination.
 */
    private long nativeParkEventPointer;

and the supporting code from unsafe.cpp and javaClass.*pp in the JVM.

I considered restoring the fast-path use of the cache without recourse 
to Thread-SMR but performance measurements failed to show any benefit 
in doing. See bug report for details.


Thanks,
David

RFR(S): 8222518: Remove unnecessary caching of Parker object in java.lang.Thread

[David Holmes]

Bug: https://bugs.openjdk.java.net/browse/JDK-8222518
webrev: http://cr.openjdk.java.net/~dholmes/8222518/webrev/

The original implementation of Unsafe.unpark simply extracted the 
JavaThread reference from the java.lang.Thread oop and if non-null 
extracted the Parker instance from it and invoked unpark. This was racy 
however as the native JavaThread could terminate at any time and 
deallocate the Parker.


That logic was fixed by JDK-6271298 which used of combination of 
type-stable-memory "event" objects for the Parker, along with use of the 
Threads_lock to obtain the initial reference to the Parker (from a 
JavaThread guaranteed to be alive), together with caching the native 
Parker pointer in a field of java.lang.Thread. Even though the native 
thread may have terminated the Parker was still valid (even if 
associated with a different thread) and the unpark at worst was a 
spurious wakeup for that other thread.


When JDK-8167108 introduced Thread Safe-Memory-Reclaimation (SMR) the 
logic was updated to always use the safe mechanism - we grab a 
ThreadsListHandle then check the cached field, else lookup the native 
thread to see if it is alive and locate the Parker instance that way.
With SMR the caching of the Parker pointer no longer serves any purpose 
- we no longer have a lock-free use-the-cache path versus a lock-using 
populate-the-cache path. With SMR we've already"paid" for the ability to 
ensure the native thread can't terminate regardless of whether we lookup 
the field from the java.lang.Thread or the JavaThread. So we can 
simplify the code and save a little footprint by removing the cache from 
java.lang.Thread:


/*
 * JVM-private state that persists after native thread termination.
 */
private long nativeParkEventPointer;

and the supporting code from unsafe.cpp and javaClass.*pp in the JVM.

I considered restoring the fast-path use of the cache without recourse 
to Thread-SMR but performance measurements failed to show any benefit in 
doing. See bug report for details.


Thanks,
David

Re: [8u-dev] RFR (S): JDK-8194653: Deadlock involving FileSystems.getDefault and System.loadLibrary call

[David Holmes]

Hi Ryan,

First with regard to the mailing list for the RFR it is not the bug 
filing category that determines this (though there is usually a 
correlation) but the location of the files where you actually made 
changes. In this case, in the current form, as hotspot changes, it 
should have been reviewed on hotspot-runtime-dev. But I don't want this 
to be hotspot changes. :)


On 24/04/2019 12:47 am, Sciampacone, Ryan wrote:


Alan / David,

I agree this could have also been a class library only solution.  Looking at 
the problem I saw a few things:

- Just above the patched code in create_vm() there is similar code for a JSR 
292 deadlock problem where classes are pre-loaded as a solution.  I felt this 
matched the type of fix I was making here.
- There is plenty of other class pre loading / initialization done in 
create_vm()


The VM controls the basic bootstrapping initialization process** to get 
the core of the core libraries initialized in the right order so that 
the VM can execute the initialization code in a sane fashion. JSR-292 
code is tightly connected to the VM, so are String, Class, and a bunch 
of others like exceptions. The FileSystem code is not part of this core 
and the VM doesn't need to know about it. This is a library problem that 
can and should be fixed in the library code.


** With the introduction of the module system a lot of this was pushed 
back into the Java library code itself.



- If there was a need to put an early switch on the pre-load of this class, I 
felt this be more natural to wrap that here


Not sure what you mean by "early switch", if you mean a flag to control 
whether or not to do the eager-initialization then a Java property could 
be used for that. Though I'd argue against making this optional unless 
there is a significant penalty for always doing the early init - is there?


Thanks,
David


- The problem doesn't exist at jdk9+ because the classes causing this problem get loaded and 
initialized as part of the initialization sequence "naturally" (ie: I see no indication 
that they are loaded to solve this specific problem).  I agree that jdk8 is unlikely to have 
dramatic changes in initialization moving forward, but putting it as part of (to me) the more 
streamlined create_vm() function made sense from a "safety" perspective as it felt more 
controlled and unlikely to be perturbed by any other change in the initialization code flow (class 
library or vm).

To follow on this last couple of points, as noted in the defect (and Brian Burkhalter 
also observes through running the reproducer test), this doesn't happen at jdk9+ because 
initialization has changed and the class got loaded "for free".  The reproducer 
test certainly applies to 13 (it's a valid check) but you can also eyeball the classes 
loaded and see the effect.

On 4/22/19, 11:42 PM, "Alan Bateman"  wrote:

 On 22/04/2019 23:11, David Holmes wrote:
 > Hi Ryan,
 >
 > On 23/04/2019 7:04 am, Sciampacone, Ryan wrote:
 >>
 >> Bug: https://bugs.openjdk.java.net/browse/JDK-8194653
 >> Webrev: http://cr.openjdk.java.net/~phh/8194653/webrev.00/
 >
 > Why does this need to be pushed to the VM? Why not do the
 > pre-loading/initializing at the Java level?
 Ryan - do you have a test case that demonstrates the issue with the main
 line (jdk/jdk)? I think we need that in order to study this issue a bit
 more closely and see what options are available. I see Paul has pasted
 in a stack trace from January and I think it would be interesting to see
 what command line options were used. I don't have a good sense yet how
 this might be fixed but it may need work in FilePermission and/or
 initializing the default file system provider at a different time during
 the startup.
 
 -Alan
 

Re: [8u-dev] RFR (S): JDK-8194653: Deadlock involving FileSystems.getDefault and System.loadLibrary call

[David Holmes]

PS. Given you have implemented this in the VM you're asking for a review 
on the wrong mailing list.


David

On 23/04/2019 7:04 am, Sciampacone, Ryan wrote:


Bug: https://bugs.openjdk.java.net/browse/JDK-8194653
Webrev: http://cr.openjdk.java.net/~phh/8194653/webrev.00/

Eliminates a race condition on call to java.nio.file.FileSystems::getDefault() where 
two threads (one inside a loadLibrary() call) could cause a deadlock if they both 
referenced this code path.  Priming the code path during startup eliminates the 
deadlock ( and loadLibrary0 are single threaded).  Note the JVM is at 
the end of initialization and the getDefault() call is safe to make.  This does 
result in a few extra classes being loaded before the user class is touched, but this 
path was almost inevitably being called once the user class is invoked.

The general problem of multithreaded class initialization and JNI library 
loading remains – this patch addresses a defect that is seen out in the field 
(the defect contains at least 2 public cases, anecdotally I have seen others).

This is JDK8 only – as discussed in the defect JDK9 and beyond address this 
problem with a different startup sequence.

Note that Oracle appears to have picked up this fix (or a version of it) for 
their 8u repo.

Paul Hohensee (phh) has agreed to sponsor the fix.

Re: [8u-dev] RFR (S): JDK-8194653: Deadlock involving FileSystems.getDefault and System.loadLibrary call

[David Holmes]

Hi Ryan,

On 23/04/2019 7:04 am, Sciampacone, Ryan wrote:


Bug: https://bugs.openjdk.java.net/browse/JDK-8194653
Webrev: http://cr.openjdk.java.net/~phh/8194653/webrev.00/


Why does this need to be pushed to the VM? Why not do the 
pre-loading/initializing at the Java level?


Cheers,
David
-


Eliminates a race condition on call to java.nio.file.FileSystems::getDefault() where 
two threads (one inside a loadLibrary() call) could cause a deadlock if they both 
referenced this code path.  Priming the code path during startup eliminates the 
deadlock ( and loadLibrary0 are single threaded).  Note the JVM is at 
the end of initialization and the getDefault() call is safe to make.  This does 
result in a few extra classes being loaded before the user class is touched, but this 
path was almost inevitably being called once the user class is invoked.

The general problem of multithreaded class initialization and JNI library 
loading remains – this patch addresses a defect that is seen out in the field 
(the defect contains at least 2 public cases, anecdotally I have seen others).

This is JDK8 only – as discussed in the defect JDK9 and beyond address this 
problem with a different startup sequence.

Note that Oracle appears to have picked up this fix (or a version of it) for 
their 8u repo.

Paul Hohensee (phh) has agreed to sponsor the fix.

Re: [PING2] RFR: 8217338: [Containers] Improve systemd slice memory limit support

[David Holmes]

Hi Severin,

I took a look at this (again**) and although I'm not at all familiar 
with the actual cgroup facilities the changes seem reasonable in that 
they only look for a hierarchical memory limit if the initial limit is 
"unlimited".


So you can add me as a reviewer.

Thanks,
David

** I didn't previously review in the hope someone more cgroup 
knowledgeable would do so.


On 18/04/2019 3:32 am, Severin Gehwolf wrote:

Ping?

On Tue, 2019-04-09 at 11:33 +0200, Severin Gehwolf wrote:

Hi,

Could I get another reviewer for this, please? Bob Vandette already reviewed it.

Thank you!

Cheers,
Severin

On Tue, 2019-04-02 at 13:48 +0200, Severin Gehwolf wrote:

Could I get a second review, please?

Thanks,
Severin

On Thu, 2019-03-28 at 11:37 -0400, Bob Vandette wrote:

Sorry for the delay.  The update looks good.

Bob.



On Mar 25, 2019, at 1:30 PM, Severin Gehwolf  wrote:

On Fri, 2019-03-22 at 14:25 -0400, Bob Vandette wrote:

Could you maybe combine subsystem_file_contents with 
subsystem_file_line_contents
by adding an additional argument?


Done: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8217338/05/webrev/

Thanks,
Severin

Re: RFR: 8222334: java -Xss0 triggers StackOverflowError

[David Holmes]

Thanks Patrick! Changes pushed.

David

On 16/04/2019 8:44 pm, Patrick Zhang OS wrote:

Done.  http://cr.openjdk.java.net/~qpzhang/8222334/webrev.05

Regards
Patrick

-Original Message-
From: David Holmes 
Sent: Tuesday, April 16, 2019 6:34 PM
To: Patrick Zhang OS 
Cc: core-libs-dev 
Subject: Re: RFR: 8222334: java -Xss0 triggers StackOverflowError

Hi Patrick,

On 16/04/2019 7:42 pm, Patrick Zhang OS wrote:

Hi David,
Please see my updates, the two '0' size test cases. I have run them with jtreg 
on jdk13 + linux + x86/aarch64 systems respectively, all passed.
http://cr.openjdk.java.net/~qpzhang/8222334/webrev.04


Thanks. Please update copyright years. Also instead of this comment:

It can verify the issue fixed in 8222334.

Just add 8222334 to the @bug line.

Thanks,
David


Regards
Patrick

-Original Message-
From: core-libs-dev  On Behalf
Of Patrick Zhang OS
Sent: Tuesday, April 16, 2019 4:23 PM
To: David Holmes 
Cc: core-libs-dev 
Subject: RE: RFR: 8222334: java -Xss0 triggers StackOverflowError

Sure I will add this, and fix the intention mentioned by Alan.

Regards
Patrick

-Original Message-
From: David Holmes 
Sent: Tuesday, April 16, 2019 4:17 PM
To: Patrick Zhang OS 
Cc: Alan Bateman ; core-libs-dev

Subject: Re: RFR: 8222334: java -Xss0 triggers StackOverflowError

Patrick,

Sorry should have picked up on this earlier. Can you please update the 
following two tests to add a test for '0' as appropriate:

./jdk/tools/launcher/TooSmallStackSize.java
./hotspot/jtreg/runtime/Thread/TooSmallStackSize.java

Thanks,
David

On 16/04/2019 5:47 pm, David Holmes wrote:

On 16/04/2019 5:40 pm, Alan Bateman wrote:

On 15/04/2019 08:48, David Holmes wrote:

On 15/04/2019 5:34 pm, Patrick Zhang OS wrote:

Removed it.
http://cr.openjdk.java.net/~qpzhang/8222334/webrev.03/jdk.changese
t

By the way, could you please sponsor to push it once approved?
thanks in advance.


Sure - if the core-libs person who also reviews doesn't volunteer
(hint hint ;-) )

This looks okay to me too, I think we should fix the intention in
ContinueInNewThread while we are there so it matches the rest of the
file.


Thanks Alan! I'll fix the indent before pushing.

David
-


-Alan

Re: RFR: 8222334: java -Xss0 triggers StackOverflowError

[David Holmes]

Hi Patrick,

On 16/04/2019 7:42 pm, Patrick Zhang OS wrote:

Hi David,
Please see my updates, the two '0' size test cases. I have run them with jtreg 
on jdk13 + linux + x86/aarch64 systems respectively, all passed.
http://cr.openjdk.java.net/~qpzhang/8222334/webrev.04


Thanks. Please update copyright years. Also instead of this comment:

It can verify the issue fixed in 8222334.

Just add 8222334 to the @bug line.

Thanks,
David


Regards
Patrick

-Original Message-
From: core-libs-dev  On Behalf Of 
Patrick Zhang OS
Sent: Tuesday, April 16, 2019 4:23 PM
To: David Holmes 
Cc: core-libs-dev 
Subject: RE: RFR: 8222334: java -Xss0 triggers StackOverflowError

Sure I will add this, and fix the intention mentioned by Alan.

Regards
Patrick

-Original Message-
From: David Holmes 
Sent: Tuesday, April 16, 2019 4:17 PM
To: Patrick Zhang OS 
Cc: Alan Bateman ; core-libs-dev 

Subject: Re: RFR: 8222334: java -Xss0 triggers StackOverflowError

Patrick,

Sorry should have picked up on this earlier. Can you please update the 
following two tests to add a test for '0' as appropriate:

./jdk/tools/launcher/TooSmallStackSize.java
./hotspot/jtreg/runtime/Thread/TooSmallStackSize.java

Thanks,
David

On 16/04/2019 5:47 pm, David Holmes wrote:

On 16/04/2019 5:40 pm, Alan Bateman wrote:

On 15/04/2019 08:48, David Holmes wrote:

On 15/04/2019 5:34 pm, Patrick Zhang OS wrote:

Removed it.
http://cr.openjdk.java.net/~qpzhang/8222334/webrev.03/jdk.changeset

By the way, could you please sponsor to push it once approved?
thanks in advance.


Sure - if the core-libs person who also reviews doesn't volunteer
(hint hint ;-) )

This looks okay to me too, I think we should fix the intention in
ContinueInNewThread while we are there so it matches the rest of the
file.


Thanks Alan! I'll fix the indent before pushing.

David
-


-Alan

Re: RFR: 8222334: java -Xss0 triggers StackOverflowError

[David Holmes]

Patrick,

Sorry should have picked up on this earlier. Can you please update the 
following two tests to add a test for '0' as appropriate:


./jdk/tools/launcher/TooSmallStackSize.java
./hotspot/jtreg/runtime/Thread/TooSmallStackSize.java

Thanks,
David

On 16/04/2019 5:47 pm, David Holmes wrote:

On 16/04/2019 5:40 pm, Alan Bateman wrote:

On 15/04/2019 08:48, David Holmes wrote:

On 15/04/2019 5:34 pm, Patrick Zhang OS wrote:
Removed it. 
http://cr.openjdk.java.net/~qpzhang/8222334/webrev.03/jdk.changeset


By the way, could you please sponsor to push it once approved? 
thanks in advance.


Sure - if the core-libs person who also reviews doesn't volunteer 
(hint hint ;-) )
This looks okay to me too, I think we should fix the intention in 
ContinueInNewThread while we are there so it matches the rest of the 
file.


Thanks Alan! I'll fix the indent before pushing.

David
-


-Alan

Re: RFR: 8222334: java -Xss0 triggers StackOverflowError

[David Holmes]

On 16/04/2019 5:40 pm, Alan Bateman wrote:

On 15/04/2019 08:48, David Holmes wrote:

On 15/04/2019 5:34 pm, Patrick Zhang OS wrote:
Removed it. 
http://cr.openjdk.java.net/~qpzhang/8222334/webrev.03/jdk.changeset


By the way, could you please sponsor to push it once approved? thanks 
in advance.


Sure - if the core-libs person who also reviews doesn't volunteer 
(hint hint ;-) )
This looks okay to me too, I think we should fix the intention in 
ContinueInNewThread while we are there so it matches the rest of the file.


Thanks Alan! I'll fix the indent before pushing.

David
-


-Alan

Re: SoftReference incorrect javadoc?

[David Holmes]

Hi Michael,

Re-directing to core-libs-dev and hotspot-gc-dev.

Thanks,
David

On 16/04/2019 12:14 pm, Michael Pollmeier wrote:

Quoting
https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/lang/ref/SoftReference.html



All soft references to softly-reachable objects are guaranteed to have

been cleared before the virtual machine throws an OutOfMemoryError

That statement was true when soft references were first introduced in
java 1.2, but from java 1.3.1 the jvm property
`-XX:SoftRefLRUPolicyMSPerMB` was introduced.
It defaults to 1000 (milliseconds), meaning that if there’s only 10MB
available heap, the garbage collector will free references that have
been used more than 10s ago. I.e. everything else (including young
softly reachable objects) will *not* be freed, leading to an
OutOfMemoryError, contradicting the above quoted 'guarantee'.

That's also the behaviour I observed on various JREs. Would you agree,
i.e. should I propose an updated doc?

Cheers
Michael

Re: RFR: 8222334: java -Xss0 triggers StackOverflowError

[David Holmes]

On 15/04/2019 5:34 pm, Patrick Zhang OS wrote:

Removed it. http://cr.openjdk.java.net/~qpzhang/8222334/webrev.03/jdk.changeset

By the way, could you please sponsor to push it once approved? thanks in 
advance.


Sure - if the core-libs person who also reviews doesn't volunteer (hint 
hint ;-) )


Cheers,
David


Regards
Patrick

-Original Message-
From: David Holmes 
Sent: Monday, April 15, 2019 2:33 PM
To: Patrick Zhang OS ; core-libs-dev 

Subject: Re: RFR: 8222334: java -Xss0 triggers StackOverflowError

Hi Patrick,

On 15/04/2019 3:42 pm, Patrick Zhang OS wrote:

Hi David,

Many thanks, I integrated your updates into the new patch.


Thanks. My only further comment is to not have:

   947  * See JDK-8222334 for details

Cross references from code to bug reports should be very rare and I don't think 
this one warrants it. No need to see updated webrev in that case.

Cheers,
David




I think STACK_SIZE_MINIMUM is an empirical value, NOT suitable for 'all' platforms, at least not 
that safe, for example, a tricky experiment is: create the initial thread with 320K, and have later 
VM inner threads created with 448K, on my aarch64 system, StackOverflowError would be thrown. 
Fortunately this probably would not occur in real cases, as -Xss60k, -Xss320k, etc. can be stopped 
by these if-clauses.  I changed "all platforms" to "most platforms".
"only used for windows" might ambiguously mean "GetDefaultJavaVMInitArgs returns 0 for windows 
only" or "only windows supports 0". I updated it as "for example, Windows"


http://cr.openjdk.java.net/~qpzhang/8222334/webrev.02/

Regards
Patrick

-Original Message-
From: David Holmes 
Sent: Monday, April 15, 2019 6:55 AM
To: Patrick Zhang OS ; core-libs-dev

Subject: Re: RFR: 8222334: java -Xss0 triggers StackOverflowError

Hi Patrick,

Please see:

http://cr.openjdk.java.net/~dholmes/8222334/webrev/

for my suggested updates to the commentary. Note that GetDefaultJavaVMInitArgs returns 
the build-time default stack sizes and so will only return 0 (for "use the system 
default") on Windows. It is not affected by -XX:ThreadStackSize=n as that only gets 
processed when the JVM is actually loaded.

Thanks,
David

On 12/04/2019 6:11 pm, David Holmes wrote:

Hi Patrick,

First apologies that it took me so long to get my head around this.
:) Let me summarise the problem as I see it.

The launcher specifies no particular semantics for -Xss0, to it 0 is
just a very small size. However the VM maps -Xss to
-XX:ThreadStackSize and for it 0 means "use the platform default stack size".

The launcher examines -Xss because it needs to use it to define the
stacksize for the initial thread created to launch the VM.

The VM examines -Xss to see what stacksize to use for subsequently
created threads and it treats 0 as 'use the platform default' and it
otherwise checks the value against some hardcoded minimums and
reports an error if it is too small.

The initial thread that loads the VM needs sufficient stack to be
able to process things to the point where it can determine that the
requested stacksize is too small and report the error. The value of
the minimum stack is hardcoded into the launcher, as
STACK_SIZE_MINIMUM (64KB). If the -Xss value is less than that then it gets set 
to that.

If no -Xss is specified then the launcher asks the VM for a
reasonable value to use for the stacksize of the initial thread (typically 1MB).

The problem arises with -Xss0 because this causes the launcher to set
an initial thread stacksize of STACK_SIZE_MINIMUM, but the VM sees
this as "use the default" and so does not reject it and tries to
continue with VM initialization. That can't succeed as we only have a
tiny STACK_SIZE_MINIMUM stack and so we get StackOverflowError (or
fail an assert in debug builds).

So the solution, as Patrick proposes, is to treat -Xss0 in the
launcher as-if -Xss has not been set and so use the VM suggested
default for the initial thread's stacksize.

So I agree with the functional change here, but have some alternate
suggestions for additional commentary. Unfortunately I have to step
away at the moment (its Friday night) so will send that later - sorry.

Thanks,
David

On 12/04/2019 5:51 pm, Patrick Zhang OS wrote:

Moved this to core-libs-dev for review, thanks.

Dropped and bcc'ed jdk-dev and jdk-updates-dev.

Regards
Patrick

-Original Message-
From: David Holmes 
Sent: Friday, April 12, 2019 3:43 PM
To: Patrick Zhang OS ;
jdk-...@openjdk.java.net
Cc: jdk-updates-...@openjdk.java.net
Subject: Re: RFR: 8222334: java -Xss0 triggers StackOverflowError

Hi Patrick,

Please takes this to core-libs-dev for review.

Thanks,
David

On 12/04/2019 5:24 pm, Patrick Zhang OS wrote:

Hi,

Please review this patch.

The problem is that the launcher does a check on the input -Xss and
ensure it >=64K for the initial thread, while vm has another
function to determin

Re: RFR: 8222334: java -Xss0 triggers StackOverflowError

[David Holmes]

Hi Patrick,

On 15/04/2019 3:42 pm, Patrick Zhang OS wrote:

Hi David,

Many thanks, I integrated your updates into the new patch.


Thanks. My only further comment is to not have:

 947  * See JDK-8222334 for details

Cross references from code to bug reports should be very rare and I 
don't think this one warrants it. No need to see updated webrev in that 
case.


Cheers,
David




I think STACK_SIZE_MINIMUM is an empirical value, NOT suitable for 'all' platforms, at least not 
that safe, for example, a tricky experiment is: create the initial thread with 320K, and have later 
VM inner threads created with 448K, on my aarch64 system, StackOverflowError would be thrown. 
Fortunately this probably would not occur in real cases, as -Xss60k, -Xss320k, etc. can be stopped 
by these if-clauses.  I changed "all platforms" to "most platforms".
"only used for windows" might ambiguously mean "GetDefaultJavaVMInitArgs returns 0 for windows 
only" or "only windows supports 0". I updated it as "for example, Windows"


http://cr.openjdk.java.net/~qpzhang/8222334/webrev.02/

Regards
Patrick

-Original Message-
From: David Holmes 
Sent: Monday, April 15, 2019 6:55 AM
To: Patrick Zhang OS ; core-libs-dev 

Subject: Re: RFR: 8222334: java -Xss0 triggers StackOverflowError

Hi Patrick,

Please see:

http://cr.openjdk.java.net/~dholmes/8222334/webrev/

for my suggested updates to the commentary. Note that GetDefaultJavaVMInitArgs returns 
the build-time default stack sizes and so will only return 0 (for "use the system 
default") on Windows. It is not affected by -XX:ThreadStackSize=n as that only gets 
processed when the JVM is actually loaded.

Thanks,
David

On 12/04/2019 6:11 pm, David Holmes wrote:

Hi Patrick,

First apologies that it took me so long to get my head around this. :)
Let me summarise the problem as I see it.

The launcher specifies no particular semantics for -Xss0, to it 0 is
just a very small size. However the VM maps -Xss to
-XX:ThreadStackSize and for it 0 means "use the platform default stack size".

The launcher examines -Xss because it needs to use it to define the
stacksize for the initial thread created to launch the VM.

The VM examines -Xss to see what stacksize to use for subsequently
created threads and it treats 0 as 'use the platform default' and it
otherwise checks the value against some hardcoded minimums and reports
an error if it is too small.

The initial thread that loads the VM needs sufficient stack to be able
to process things to the point where it can determine that the
requested stacksize is too small and report the error. The value of
the minimum stack is hardcoded into the launcher, as
STACK_SIZE_MINIMUM (64KB). If the -Xss value is less than that then it gets set 
to that.

If no -Xss is specified then the launcher asks the VM for a reasonable
value to use for the stacksize of the initial thread (typically 1MB).

The problem arises with -Xss0 because this causes the launcher to set
an initial thread stacksize of STACK_SIZE_MINIMUM, but the VM sees
this as "use the default" and so does not reject it and tries to
continue with VM initialization. That can't succeed as we only have a
tiny STACK_SIZE_MINIMUM stack and so we get StackOverflowError (or
fail an assert in debug builds).

So the solution, as Patrick proposes, is to treat -Xss0 in the
launcher as-if -Xss has not been set and so use the VM suggested
default for the initial thread's stacksize.

So I agree with the functional change here, but have some alternate
suggestions for additional commentary. Unfortunately I have to step
away at the moment (its Friday night) so will send that later - sorry.

Thanks,
David

On 12/04/2019 5:51 pm, Patrick Zhang OS wrote:

Moved this to core-libs-dev for review, thanks.

Dropped and bcc'ed jdk-dev and jdk-updates-dev.

Regards
Patrick

-Original Message-
From: David Holmes 
Sent: Friday, April 12, 2019 3:43 PM
To: Patrick Zhang OS ;
jdk-...@openjdk.java.net
Cc: jdk-updates-...@openjdk.java.net
Subject: Re: RFR: 8222334: java -Xss0 triggers StackOverflowError

Hi Patrick,

Please takes this to core-libs-dev for review.

Thanks,
David

On 12/04/2019 5:24 pm, Patrick Zhang OS wrote:

Hi,

Please review this patch.

The problem is that the launcher does a check on the input -Xss and
ensure it >=64K for the initial thread, while vm has another
function to determine whether the input stack size is big enough to
future threads, such as cgc_thread, vm_thread, java_thead etc.
However if -Xss0, the initial thread is created with stack size 64K,
while others use hotspot/system default sizes, which would trigger
StackOverflowError. We could either fine tune the threshold 64K to
be a bigger one, or have the initial thread created with system
defaults that may be what the user expects. This patch chooses the
second solution, to avoid potential side-effect of the first.

This can b

Re: RFR: 8222334: java -Xss0 triggers StackOverflowError

[David Holmes]

Hi Patrick,

Please see:

http://cr.openjdk.java.net/~dholmes/8222334/webrev/

for my suggested updates to the commentary. Note that 
GetDefaultJavaVMInitArgs returns the build-time default stack sizes and 
so will only return 0 (for "use the system default") on Windows. It is 
not affected by -XX:ThreadStackSize=n as that only gets processed when 
the JVM is actually loaded.


Thanks,
David

On 12/04/2019 6:11 pm, David Holmes wrote:

Hi Patrick,

First apologies that it took me so long to get my head around this. :) 
Let me summarise the problem as I see it.


The launcher specifies no particular semantics for -Xss0, to it 0 is 
just a very small size. However the VM maps -Xss to -XX:ThreadStackSize 
and for it 0 means "use the platform default stack size".


The launcher examines -Xss because it needs to use it to define the 
stacksize for the initial thread created to launch the VM.


The VM examines -Xss to see what stacksize to use for subsequently 
created threads and it treats 0 as 'use the platform default' and it 
otherwise checks the value against some hardcoded minimums and reports 
an error if it is too small.


The initial thread that loads the VM needs sufficient stack to be able 
to process things to the point where it can determine that the requested 
stacksize is too small and report the error. The value of the minimum 
stack is hardcoded into the launcher, as STACK_SIZE_MINIMUM (64KB). If 
the -Xss value is less than that then it gets set to that.


If no -Xss is specified then the launcher asks the VM for a reasonable 
value to use for the stacksize of the initial thread (typically 1MB).


The problem arises with -Xss0 because this causes the launcher to set an 
initial thread stacksize of STACK_SIZE_MINIMUM, but the VM sees this as 
"use the default" and so does not reject it and tries to continue with 
VM initialization. That can't succeed as we only have a tiny 
STACK_SIZE_MINIMUM stack and so we get StackOverflowError (or fail an 
assert in debug builds).


So the solution, as Patrick proposes, is to treat -Xss0 in the launcher 
as-if -Xss has not been set and so use the VM suggested default for the 
initial thread's stacksize.


So I agree with the functional change here, but have some alternate 
suggestions for additional commentary. Unfortunately I have to step away 
at the moment (its Friday night) so will send that later - sorry.


Thanks,
David

On 12/04/2019 5:51 pm, Patrick Zhang OS wrote:

Moved this to core-libs-dev for review, thanks.

Dropped and bcc'ed jdk-dev and jdk-updates-dev.

Regards
Patrick

-----Original Message-
From: David Holmes 
Sent: Friday, April 12, 2019 3:43 PM
To: Patrick Zhang OS ; 
jdk-...@openjdk.java.net

Cc: jdk-updates-...@openjdk.java.net
Subject: Re: RFR: 8222334: java -Xss0 triggers StackOverflowError

Hi Patrick,

Please takes this to core-libs-dev for review.

Thanks,
David

On 12/04/2019 5:24 pm, Patrick Zhang OS wrote:

Hi,

Please review this patch.

The problem is that the launcher does a check on the input -Xss and
ensure it >=64K for the initial thread, while vm has another function
to determine whether the input stack size is big enough to future
threads, such as cgc_thread, vm_thread, java_thead etc. However if
-Xss0, the initial thread is created with stack size 64K, while others
use hotspot/system default sizes, which would trigger
StackOverflowError. We could either fine tune the threshold 64K to be
a bigger one, or have the initial thread created with system defaults
that may be what the user expects. This patch chooses the second
solution, to avoid potential side-effect of the first.

This can be reproduced with 10, 11, 12 too, so I cc'ed 
jdk-updates-dev here.


More details please refer to the ticket.

JBS: https://bugs.openjdk.java.net/browse/JDK-8222334

Webrev: http://cr.openjdk.java.net/~qpzhang/8222334/webrev.01/

Thanks for David's comments in Jira.

Regards

Patrick

Re: RFR: 8222334: java -Xss0 triggers StackOverflowError

[David Holmes]

Hi Patrick,

First apologies that it took me so long to get my head around this. :) 
Let me summarise the problem as I see it.


The launcher specifies no particular semantics for -Xss0, to it 0 is 
just a very small size. However the VM maps -Xss to -XX:ThreadStackSize 
and for it 0 means "use the platform default stack size".


The launcher examines -Xss because it needs to use it to define the 
stacksize for the initial thread created to launch the VM.


The VM examines -Xss to see what stacksize to use for subsequently 
created threads and it treats 0 as 'use the platform default' and it 
otherwise checks the value against some hardcoded minimums and reports 
an error if it is too small.


The initial thread that loads the VM needs sufficient stack to be able 
to process things to the point where it can determine that the requested 
stacksize is too small and report the error. The value of the minimum 
stack is hardcoded into the launcher, as STACK_SIZE_MINIMUM (64KB). If 
the -Xss value is less than that then it gets set to that.


If no -Xss is specified then the launcher asks the VM for a reasonable 
value to use for the stacksize of the initial thread (typically 1MB).


The problem arises with -Xss0 because this causes the launcher to set an 
initial thread stacksize of STACK_SIZE_MINIMUM, but the VM sees this as 
"use the default" and so does not reject it and tries to continue with 
VM initialization. That can't succeed as we only have a tiny 
STACK_SIZE_MINIMUM stack and so we get StackOverflowError (or fail an 
assert in debug builds).


So the solution, as Patrick proposes, is to treat -Xss0 in the launcher 
as-if -Xss has not been set and so use the VM suggested default for the 
initial thread's stacksize.


So I agree with the functional change here, but have some alternate 
suggestions for additional commentary. Unfortunately I have to step away 
at the moment (its Friday night) so will send that later - sorry.


Thanks,
David

On 12/04/2019 5:51 pm, Patrick Zhang OS wrote:

Moved this to core-libs-dev for review, thanks.

Dropped and bcc'ed jdk-dev and jdk-updates-dev.

Regards
Patrick

-Original Message-
From: David Holmes 
Sent: Friday, April 12, 2019 3:43 PM
To: Patrick Zhang OS ; jdk-...@openjdk.java.net
Cc: jdk-updates-...@openjdk.java.net
Subject: Re: RFR: 8222334: java -Xss0 triggers StackOverflowError

Hi Patrick,

Please takes this to core-libs-dev for review.

Thanks,
David

On 12/04/2019 5:24 pm, Patrick Zhang OS wrote:

Hi,

Please review this patch.

The problem is that the launcher does a check on the input -Xss and
ensure it >=64K for the initial thread, while vm has another function
to determine whether the input stack size is big enough to future
threads, such as cgc_thread, vm_thread, java_thead etc. However if
-Xss0, the initial thread is created with stack size 64K, while others
use hotspot/system default sizes, which would trigger
StackOverflowError. We could either fine tune the threshold 64K to be
a bigger one, or have the initial thread created with system defaults
that may be what the user expects. This patch chooses the second
solution, to avoid potential side-effect of the first.

This can be reproduced with 10, 11, 12 too, so I cc'ed jdk-updates-dev here.

More details please refer to the ticket.

JBS: https://bugs.openjdk.java.net/browse/JDK-8222334

Webrev: http://cr.openjdk.java.net/~qpzhang/8222334/webrev.01/

Thanks for David's comments in Jira.

Regards

Patrick

Re: RFR (XS) 8222111: exeCallerAccessTest.c fails to build: control reaches end of non-void function

[David Holmes]

+1

Strange that an old gcc complains but the newer ones that add more and 
more warnings each time, let this slip through :(


Arguably all the exit(-1) in the main method should just be return -1 
instead - but that's a different matter.


Thanks,
David

On 8/04/2019 10:08 pm, Alan Bateman wrote:

On 08/04/2019 12:54, Aleksey Shipilev wrote:

Bug:
   https://bugs.openjdk.java.net/browse/JDK-8222111

Fix:

diff -r 0d7fb7f07134 
test/jdk/java/lang/reflect/exeCallerAccessTest/exeCallerAccessTest.c
--- 
a/test/jdk/java/lang/reflect/exeCallerAccessTest/exeCallerAccessTest.c  
Mon Apr 08 06:56:37

2019 +0100
+++ 
b/test/jdk/java/lang/reflect/exeCallerAccessTest/exeCallerAccessTest.c  
Mon Apr 08 13:53:51

2019 +0200
@@ -94,4 +94,5 @@

  (*jvm)->DestroyJavaVM(jvm);
+    return 0;
  }

This test seems to having trouble bedding in. I don't know which gcc 
version it falls foul of the above but the patch looks okay to me.


-Alan

Re: (1-line) Review Request: 8222078: test/jdk/java/lang/reflect/exeCallerAccessTest/exeCallerAccessTest.c build fails after 8221530

[David Holmes]

Looks good.

Thanks,
David

On 7/04/2019 6:12 pm, Mandy Chung wrote:

It needs another fix for windows build:

diff --git a/make/test/JtregNativeJdk.gmk b/make/test/JtregNativeJdk.gmk
--- a/make/test/JtregNativeJdk.gmk
+++ b/make/test/JtregNativeJdk.gmk
@@ -61,6 +61,7 @@
    BUILD_JDK_JTREG_LIBRARIES_LIBS_libstringPlatformChars := $(WIN_LIB_JAVA)
    WIN_LIB_JLI := $(SUPPORT_OUTPUTDIR)/native/java.base/libjli/jli.lib
    BUILD_JDK_JTREG_EXECUTABLES_LIBS_exeJliLaunchTest := $(WIN_LIB_JLI)
+  BUILD_JDK_JTREG_EXECUTABLES_LIBS_exeCallerAccessTest := jvm.lib
  else
    BUILD_JDK_JTREG_LIBRARIES_LIBS_libstringPlatformChars := -ljava
    BUILD_JDK_JTREG_LIBRARIES_LIBS_libDirectIO := -ljava
@@ -70,10 +71,9 @@
  BUILD_JDK_JTREG_LIBRARIES_LIBS_libInheritedChannel := -ljava 
-lsocket -lnsl

    endif
    BUILD_JDK_JTREG_EXECUTABLES_LIBS_exeJliLaunchTest := -ljli
+  BUILD_JDK_JTREG_EXECUTABLES_LIBS_exeCallerAccessTest := -ljvm
  endif

-BUILD_JDK_JTREG_EXECUTABLES_LIBS_exeCallerAccessTest := -ljvm
-
  ifeq ($(call isTargetOs, macosx), true)
    BUILD_JDK_JTREG_LIBRARIES_CFLAGS_libTestMainKeyWindow := -ObjC
    BUILD_JDK_JTREG_LIBRARIES_LIBS_libTestMainKeyWindow := -framework 
JavaVM \


Mandy

On 4/7/19 10:47 AM, David Holmes wrote:

Looks good.

Thanks,
David

On 7/04/2019 9:37 am, Mandy Chung wrote:

A simple test fix.  The test causes the build failure.

Thanks
Mandy

diff --git 
a/test/jdk/java/lang/reflect/exeCallerAccessTest/exeCallerAccessTest.c b/test/jdk/java/lang/reflect/exeCallerAccessTest/exeCallerAccessTest.c 

--- 
a/test/jdk/java/lang/reflect/exeCallerAccessTest/exeCallerAccessTest.c
+++ 
b/test/jdk/java/lang/reflect/exeCallerAccessTest/exeCallerAccessTest.c

@@ -34,6 +34,7 @@
  static jmethodID mid_Field_get;

  int getField(JNIEnv *env, char* declaringClass_name, char* 
field_name);

+int checkAndClearIllegalAccessExceptionThrown(JNIEnv *env);

  int main(int argc, char** args) {
  JavaVM *jvm;

Re: (1-line) Review Request: 8222078: test/jdk/java/lang/reflect/exeCallerAccessTest/exeCallerAccessTest.c build fails after 8221530

[David Holmes]

Looks good.

Thanks,
David

On 7/04/2019 9:37 am, Mandy Chung wrote:

A simple test fix.  The test causes the build failure.

Thanks
Mandy

diff --git 
a/test/jdk/java/lang/reflect/exeCallerAccessTest/exeCallerAccessTest.c 
b/test/jdk/java/lang/reflect/exeCallerAccessTest/exeCallerAccessTest.c

--- a/test/jdk/java/lang/reflect/exeCallerAccessTest/exeCallerAccessTest.c
+++ b/test/jdk/java/lang/reflect/exeCallerAccessTest/exeCallerAccessTest.c
@@ -34,6 +34,7 @@
  static jmethodID mid_Field_get;

  int getField(JNIEnv *env, char* declaringClass_name, char* field_name);
+int checkAndClearIllegalAccessExceptionThrown(JNIEnv *env);

  int main(int argc, char** args) {
  JavaVM *jvm;

Re: RFR: 8221477: Inject os/cpu-specific constants into Unsafe from JVM

[David Holmes]

Hi Andrew,

This all looks good to me - thanks for making the changes.

Two nits:
- BE was barely acceptable when used like a local variable, but now I 
think BIG_ENDIAN would be better.
- If you don't use static import you can name the UnsafeConstants fields 
the obvious way without clashing with public fields in Unsafe.


Feel free to ignore those nits. If you do make a change no need to see 
another webrev.


Thanks,
David

On 5/04/2019 1:19 am, Andrew Dinn wrote:

New webrev is now available. Re-reviews welcome.

JIRA:   https://bugs.openjdk.java.net/browse/JDK-8221477
Webrev: http://cr.openjdk.java.net/~adinn/8221477/webrev.03

This version should address all comments from Thomas, David and Coleen.

Testing
Tier1 test passed.
Submit test passed.

regards,


Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander

Re: Robot.mouseMove() in macOS Mojave

[David Holmes]

Hi Kusti,

The folks on swing-dev or awt-dev are probably the ones most likely to 
be able to assist.


Cheers,
David

On 4/04/2019 3:51 pm, Kustaa Nyholm wrote:

Hi,


I've got an application where zooming with the scroll wheel on the mouse will 
also center the mouse on the screen (well window actually).
This has worked since day one for some 15 years.

The mouse centring is implemented with Robot.mouseMove() method.

This morning I received a message from a beta tester that my latest build fails 
to center the cursor on macOS Mojave.

I've not investigated this further except to check that it works for me on High 
Sierra.

So I'm canvassing here to see if this is a know issue or information to the 
contrary.

cheers Kusti

Re: RFR: 8221477: Inject os/cpu-specific constants into Unsafe from JVM

[David Holmes]

Follow up ...

On 1/04/2019 2:27 pm, David Holmes wrote:

Hi Andrew,

On 29/03/2019 8:40 pm, Andrew Dinn wrote:

Hi David,

Thanks very much for reviewing this patch.

On 29/03/2019 01:25, David Holmes wrote:

This seems fine in general but I have a few queries on some details:

src/hotspot/share/classfile/javaClasses.hpp

 f(java_lang_Thread) \
+   f(jdk_internal_misc_UnsafeConstants) \
 f(java_lang_ThreadGroup) \

Is there a reason this needs to be shoved in there? Similarly with
src/hotspot/share/classfile/systemDictionary.hpp:

    do_klass(Thread_klass,
java_lang_Thread  ) \
+   do_klass(UnsafeConstants_klass,
jdk_internal_misc_UnsafeConstants ) \
 do_klass(ThreadGroup_klass,
java_lang_ThreadGroup ) \

?


I'm not sure what you are asking here. Are you talking about the
positioning of these entries? If so then the reason for choosing those
positions was because they match the position of the corresponding class
initialization calls in the file thread.cpp. As to that choice ... see
below.


Yes I'm asking about the positioning ...


src/hotspot/share/runtime/thread.cpp

 main_thread->set_threadObj(thread_object);
+
+   // initialize the hardware-specific constants needed by Unsafe
+   initialize_class(vmSymbols::jdk_internal_misc_UnsafeConstants(),
CHECK);
+   jdk_internal_misc_UnsafeConstants::set_unsafe_constants();
+
 // Set thread status to running since main thread has
 // been started and running.
 java_lang_Thread::set_thread_status(thread_object,

That seems a very odd place to insert the new initialization. I can't
see any reason you'd need to split the Thread object initialization like
that. ??


Well, yes, indeed :-). I was not sure where this init needed to go so I
simply put it in as early as I thought was safe. Clearly, it is an
interloper into intitialise_java_lang_classes() but I was not sure where
else was more appropriate.

I don't think it matters too much where this init happens so long as it
is early enough to precede any clinit dependencies on Unsafe (see
below). I'm very happy to take advice on this (indeed I was hoping/
expecting it would be brought up at review) and also on where the
entries in the headers would best be placed.


More generally exactly when do you need to initialize this new class by
and how does the initialization order change before/after this fix? (I'm
expecting only to see the new class inserted where needed without any
other perturbations.)


As I said, I put the class initialization in at this early point simply
to guarantee that the constants are available before Unsafe or any class
which might recursively initialize Unsafe could end up needing them. I
am sure they could move later and also earlier, although the latter
would probably not make any sense. The important thing is that they
don't really have any hard dependencies on other class inits apart,
perhaps, from 'magic' classes like Object, Class etc which need to exist
in order to init /any/ other class.


I did some analysis of the class loading and initialization sequence and 
added my suggestions to bug report. In summary loading seems somewhat 
immaterial so I suggest:


- javaClasses.hpp: Add UnsafeConstants at the end of 
BASIC_JAVA_CLASSES_DO_PART2

- systemDictionary.hpp: Add UnsafeConstants immediately before Unsafe

Then for init:
- thread.cpp: initialize UnsafeConstants immediately after j.l.Module

It would be desirable to detect if we happen to execute the  
earlier (by accident) so I suggest adding a "not initialized" assertion 
prior to your code calling initialize_class. Actually that might be a 
useful addition to the initialize_class method, as if it fires it means 
we're not initializing in the expected order ... I'll run a little 
adding that ...


I meant to say "run a little test". Turns out you can't put the 
assertion in initialize_class as the initialization can vary for some of 
the exception classes (at least) depending on VM flags used (e.g. -Xrs, 
and I think certain logging options).


Thanks,
David



Thanks,
David

P.S. This missing space in javaClasses.cpp was reported by Thomas but 
hasn't been fixed yet:


4026 }else {



I deliberately factored these constants out of Unsafe into a separate
all static class UnsafeConstants so as to decouple this init from any
current or future dependencies on Unsafe (and also to make them more
clearly visible). Since the fields only hold os/hw specific constants
they can be set any time after VM_Version/CPU-specific init and
OS-specific init has completed.


src/hotspot/share/classfile/vmSymbols.hpp

+   template(big_endian_name,   "BIG_ENDIAN")
    \
+   template(use_unaligned_access_name,
"UNALIGNED_ACCESS")    \

Nit: There's an extra space before "UNALIGNED...


Thanks. Thomas Stuefe al

Re: RFR: 8221477: Inject os/cpu-specific constants into Unsafe from JVM

[David Holmes]

Hi Andrew,

On 29/03/2019 8:40 pm, Andrew Dinn wrote:

Hi David,

Thanks very much for reviewing this patch.

On 29/03/2019 01:25, David Holmes wrote:

This seems fine in general but I have a few queries on some details:

src/hotspot/share/classfile/javaClasses.hpp

     f(java_lang_Thread) \
+   f(jdk_internal_misc_UnsafeConstants) \
     f(java_lang_ThreadGroup) \

Is there a reason this needs to be shoved in there? Similarly with
src/hotspot/share/classfile/systemDictionary.hpp:

    do_klass(Thread_klass,
java_lang_Thread  ) \
+   do_klass(UnsafeConstants_klass,
jdk_internal_misc_UnsafeConstants ) \
     do_klass(ThreadGroup_klass,
java_lang_ThreadGroup ) \

?


I'm not sure what you are asking here. Are you talking about the
positioning of these entries? If so then the reason for choosing those
positions was because they match the position of the corresponding class
initialization calls in the file thread.cpp. As to that choice ... see
below.


Yes I'm asking about the positioning ...


src/hotspot/share/runtime/thread.cpp

     main_thread->set_threadObj(thread_object);
+
+   // initialize the hardware-specific constants needed by Unsafe
+   initialize_class(vmSymbols::jdk_internal_misc_UnsafeConstants(),
CHECK);
+   jdk_internal_misc_UnsafeConstants::set_unsafe_constants();
+
     // Set thread status to running since main thread has
     // been started and running.
     java_lang_Thread::set_thread_status(thread_object,

That seems a very odd place to insert the new initialization. I can't
see any reason you'd need to split the Thread object initialization like
that. ??


Well, yes, indeed :-). I was not sure where this init needed to go so I
simply put it in as early as I thought was safe. Clearly, it is an
interloper into intitialise_java_lang_classes() but I was not sure where
else was more appropriate.

I don't think it matters too much where this init happens so long as it
is early enough to precede any clinit dependencies on Unsafe (see
below). I'm very happy to take advice on this (indeed I was hoping/
expecting it would be brought up at review) and also on where the
entries in the headers would best be placed.


More generally exactly when do you need to initialize this new class by
and how does the initialization order change before/after this fix? (I'm
expecting only to see the new class inserted where needed without any
other perturbations.)


As I said, I put the class initialization in at this early point simply
to guarantee that the constants are available before Unsafe or any class
which might recursively initialize Unsafe could end up needing them. I
am sure they could move later and also earlier, although the latter
would probably not make any sense. The important thing is that they
don't really have any hard dependencies on other class inits apart,
perhaps, from 'magic' classes like Object, Class etc which need to exist
in order to init /any/ other class.


I did some analysis of the class loading and initialization sequence and 
added my suggestions to bug report. In summary loading seems somewhat 
immaterial so I suggest:


- javaClasses.hpp: Add UnsafeConstants at the end of 
BASIC_JAVA_CLASSES_DO_PART2

- systemDictionary.hpp: Add UnsafeConstants immediately before Unsafe

Then for init:
- thread.cpp: initialize UnsafeConstants immediately after j.l.Module

It would be desirable to detect if we happen to execute the  
earlier (by accident) so I suggest adding a "not initialized" assertion 
prior to your code calling initialize_class. Actually that might be a 
useful addition to the initialize_class method, as if it fires it means 
we're not initializing in the expected order ... I'll run a little 
adding that ...


Thanks,
David

P.S. This missing space in javaClasses.cpp was reported by Thomas but 
hasn't been fixed yet:


4026 }else {



I deliberately factored these constants out of Unsafe into a separate
all static class UnsafeConstants so as to decouple this init from any
current or future dependencies on Unsafe (and also to make them more
clearly visible). Since the fields only hold os/hw specific constants
they can be set any time after VM_Version/CPU-specific init and
OS-specific init has completed.


src/hotspot/share/classfile/vmSymbols.hpp

+   template(big_endian_name,   "BIG_ENDIAN")
    \
+   template(use_unaligned_access_name,
"UNALIGNED_ACCESS")    \

Nit: There's an extra space before "UNALIGNED...


Thanks. Thomas Stuefe already spotted that and I have updated the webrev
in place to fix it.


src/java.base/share/classes/jdk/internal/misc/UnsafeConstants.java

31  * package-protected  ...

s/protected/private/


Thanks, will correct it.


  37  * The JVM injects values into all the static fields of this class
  ...
  43  * hardware-specific constants 

Re: RFR: 8221477: Inject os/cpu-specific constants into Unsafe from JVM

[David Holmes]

Hi Andrew,

This seems fine in general but I have a few queries on some details:

src/hotspot/share/classfile/javaClasses.hpp

f(java_lang_Thread) \
+   f(jdk_internal_misc_UnsafeConstants) \
f(java_lang_ThreadGroup) \

Is there a reason this needs to be shoved in there? Similarly with 
src/hotspot/share/classfile/systemDictionary.hpp:


   do_klass(Thread_klass, 
java_lang_Thread  ) \
+   do_klass(UnsafeConstants_klass, 
jdk_internal_misc_UnsafeConstants ) \
do_klass(ThreadGroup_klass, 
java_lang_ThreadGroup ) \


?

---

src/hotspot/share/runtime/thread.cpp

main_thread->set_threadObj(thread_object);
+
+   // initialize the hardware-specific constants needed by Unsafe
+   initialize_class(vmSymbols::jdk_internal_misc_UnsafeConstants(), CHECK);
+   jdk_internal_misc_UnsafeConstants::set_unsafe_constants();
+
// Set thread status to running since main thread has
// been started and running.
java_lang_Thread::set_thread_status(thread_object,

That seems a very odd place to insert the new initialization. I can't 
see any reason you'd need to split the Thread object initialization like 
that. ??


More generally exactly when do you need to initialize this new class by 
and how does the initialization order change before/after this fix? (I'm 
expecting only to see the new class inserted where needed without any 
other perturbations.)


---

src/hotspot/share/classfile/vmSymbols.hpp

+   template(big_endian_name,   "BIG_ENDIAN") 
   \
+   template(use_unaligned_access_name, 
"UNALIGNED_ACCESS")\


Nit: There's an extra space before "UNALIGNED...

---

src/java.base/share/classes/jdk/internal/misc/UnsafeConstants.java

31  * package-protected  ...

s/protected/private/

 37  * The JVM injects values into all the static fields of this class
 ...
 43  * hardware-specific constants installed by the JVM.

I get this gist of this note but still found it rather difficult to 
intepret. There are I think two key points:


1. The fields must not be constant variables, hence the need for the 
static initialization block.

2. The true value of each field is injected by the VM.

How about:

* The JVM injects hardware-specific values into all the static fields
* of this class during JVM initialization. The static initialization
* block exists to prevent the fields from being considered constant
* variables, so the field values will be not be compiled directly into
* any class that uses them.

60  * @implNote
61  * The actual value for this field is injected by JVM. A static
62  * initialization block is used to set the value here to
63  * communicate that this static final field is not statically
64  * foldable, and to avoid any possible circular dependency during
65  * vm initialization.

I think all you need for each field is just:

* @implNote
* The actual value for this field is injected by _the_ JVM.

85  * flag whose value ...
92  * flag whose value ...

s/flag/Flag/

Thanks,
David

On 29/03/2019 2:56 am, Andrew Dinn wrote:

On 28/03/2019 15:22, Thomas Stüfe wrote:

 The second of those was actually intended to be iff. This is a common
 abbreviation used by English/US mathematicians and logicians to write
 'if and only if' (it is also sometimes written as <=>). Since you didn't
 recognize it I guess I really need to write it out in full.


Oh, don't worry on my account. I am not a native speaker nor a
mathematician. You could leave iff and add [sic] to make everyone
curious and start googling "iff" :)

I changed it nevertheless. It would be remiss to presume readers need be
conversant with elliptical, English logico-mathematical parlance (I'm
explaining this in as plain English as I can ... no, wait! ;-).

Anyway, I addressed this and your other concerns in a new webrev.

   JIRA:   https://bugs.openjdk.java.net/browse/JDK-8221477
   Webrev: http://cr.openjdk.java.net/~adinn/8221477/webrev.02

Input from a second reviewer would be welcome ...

regards,


Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander

Re: RFC: Make test failed because of the locale LANG

[David Holmes]

On 29/03/2019 1:59 am, Naoto Sato wrote:

Hi,

I don't think there is any *official* rule for the regression tests to 
succeed in any locale, but if the test case is locale sensitive such as 
in this case, I'd suggest it should correctly specify the locale 
beforehand, or quit gracefully in general.


For this specific case, I'd suggest not set the environment variable (as 
it won't work with Windows), using runtime system properties 
(user.language/user.country) would help.


Which specific case are you referring to? :)

Given there are well over a thousand failures in total I think we need 
to set a reasonable initial locale as part of the test setup. Many of 
the hotspot tests check actual and expected output and we, in general, 
have no idea what kinds of output may be subject to locale specific changes.


Cheers,
David


Naoto

On 3/27/19 10:47 PM, David Holmes wrote:

Hi Jing,

On 28/03/2019 3:22 pm, Jing Tian wrote:

Hi,

When I am doing the 'make test'.If the local LANG is set as 
'zh_CN.UTF-8', Test cases will have a lot of error messages.


==
Test summary
==
    TEST  TOTAL  PASS 
FAIL ERROR

 >> jtreg:test/hotspot/jtreg:tier1 1373 1371 2 0 <<
 >> jtreg:test/jdk:tier1   1867 1860 7 0 <<
 >> jtreg:test/langtools:tier1 3922 2470 
1450 2 <<
    jtreg:test/nashorn:tier1  0 0 
0 0
    jtreg:test/jaxp:tier1 0 0 
0 0

 >> jtreg:test/jdk:tier2   3334 3305 29 0 <<
 >> jtreg:test/langtools:tier2 11 9 2 0 <<
    jtreg:test/nashorn:tier2 35 35 0 0
 >> jtreg:test/jaxp:tier2   438 437 1 0 <<
 >> jtreg:test/jdk:tier3   1104 1068 36 0 <<
    jtreg:test/langtools:tier3    0 0 
0 0
    jtreg:test/nashorn:tier3  0 0 
0 0
    jtreg:test/jaxp:tier3 0 0 
0 0

==


Given most of these are not hotspot issues and given the number of 
failures, I think this is something that would need to be discussed 
much more broadly. So I've cc'd core-libs-dev and compiler-dev. I 
recall there was a very recent discussion regarding some tests failing 
for the same reason, but don't recall the outcome.


David
-


On the same machine,when i set LANG=en_US.UTF-8.

==
Test summary
==
    TEST  TOTAL  PASS 
FAIL ERROR

 >> jtreg:test/hotspot/jtreg:tier1 1388 1386 2 0 <<
 >> jtreg:test/jdk:tier1   1867 1843 19 5 <<
    jtreg:test/langtools:tier1 3920 3920 0 0
    jtreg:test/nashorn:tier1  0 0 
0 0
    jtreg:test/jaxp:tier1 0 0 
0 0

 >> jtreg:test/jdk:tier2   3328 3290 31 7 <<
    jtreg:test/langtools:tier2   11 11 0 0
    jtreg:test/nashorn:tier2 35 35 0 0
    jtreg:test/jaxp:tier2   438 438 0 0
 >> jtreg:test/jdk:tier3   1104 1067 37 0 <<
    jtreg:test/langtools:tier3    0 0 
0 0
    jtreg:test/nashorn:tier3  0 0 
0 0
    jtreg:test/jaxp:tier3 0 0 
0 0



By comparison we can find, lots of(1000+) langtools tests will get 
fail, and other(about 30+, exclude some X11 and sanity that


result problem) test cases will also fail because of local LANG.


such as in the test/hotspot/jtreg/compiler/c2/Test8062950.java,

shouldContain("Error: Could not find or load main class " + CLASSNAME)


When in the zh_CN.UTF-8 environment, because some of the output 
information is changed to Chinese by some properties file,


the English cannot be matched, which will result in an fail.

When I set  LANG=en_US/LC_ALL=C, this test will pass.


I think there are some possible solutions.


1.we set LC_ALL=C/LANG=EN_us in the Runtests.gmk, but this 
modification is more violent because he will affect all test cases.



2.We modify each individual test，E.g 
test/hotspot/jtreg/compiler/c2/Test8062950.java


diff -r 0421d49b6217 test/hotspot/jtreg/compiler/c2/Test8062950.java
  package compiler.c2;
  import jdk.test.lib.process.ProcessTools;
+import java.util.Map;
+import jdk.test.lib.process.OutputAnalyzer;

  public class Test8062950 {
  private static final String CLASSNAME = "DoesNo

Re: RFC: Make test failed because of the locale LANG

[David Holmes]

Hi Jing,

On 28/03/2019 3:22 pm, Jing Tian wrote:

Hi,

When I am doing the 'make test'.If the local LANG is set as 
'zh_CN.UTF-8', Test cases will have a lot of error messages.


==
Test summary
==
    TEST  TOTAL  PASS FAIL 
ERROR

 >> jtreg:test/hotspot/jtreg:tier1 1373 1371 2 0 <<
 >> jtreg:test/jdk:tier1   1867 1860 7 0 <<
 >> jtreg:test/langtools:tier1 3922 2470 
1450 2 <<

    jtreg:test/nashorn:tier1  0 0 0 0
    jtreg:test/jaxp:tier1 0 0 0 0
 >> jtreg:test/jdk:tier2   3334 3305 29 
0 <<

 >> jtreg:test/langtools:tier2 11 9 2 0 <<
    jtreg:test/nashorn:tier2 35 35 0 0
 >> jtreg:test/jaxp:tier2   438 437 1 0 <<
 >> jtreg:test/jdk:tier3   1104 1068 36 
0 <<

    jtreg:test/langtools:tier3    0 0 0 0
    jtreg:test/nashorn:tier3  0 0 0 0
    jtreg:test/jaxp:tier3 0 0 0 0
==


Given most of these are not hotspot issues and given the number of 
failures, I think this is something that would need to be discussed much 
more broadly. So I've cc'd core-libs-dev and compiler-dev. I recall 
there was a very recent discussion regarding some tests failing for the 
same reason, but don't recall the outcome.


David
-


On the same machine,when i set LANG=en_US.UTF-8.

==
Test summary
==
    TEST  TOTAL  PASS FAIL 
ERROR

 >> jtreg:test/hotspot/jtreg:tier1 1388 1386 2 0 <<
 >> jtreg:test/jdk:tier1   1867 1843 19 
5 <<

    jtreg:test/langtools:tier1 3920 3920 0 0
    jtreg:test/nashorn:tier1  0 0 0 0
    jtreg:test/jaxp:tier1 0 0 0 0
 >> jtreg:test/jdk:tier2   3328 3290 31 
7 <<

    jtreg:test/langtools:tier2   11 11 0 0
    jtreg:test/nashorn:tier2 35 35 0 0
    jtreg:test/jaxp:tier2   438 438 0 0
 >> jtreg:test/jdk:tier3   1104 1067 37 
0 <<

    jtreg:test/langtools:tier3    0 0 0 0
    jtreg:test/nashorn:tier3  0 0 0 0
    jtreg:test/jaxp:tier3 0 0 0 0


By comparison we can find, lots of(1000+) langtools tests will get fail, 
and other(about 30+, exclude some X11 and sanity that


result problem) test cases will also fail because of local LANG.


such as in the test/hotspot/jtreg/compiler/c2/Test8062950.java,

shouldContain("Error: Could not find or load main class " + CLASSNAME)


When in the zh_CN.UTF-8 environment, because some of the output 
information is changed to Chinese by some properties file,


the English cannot be matched, which will result in an fail.

When I set  LANG=en_US/LC_ALL=C, this test will pass.


I think there are some possible solutions.


1.we set LC_ALL=C/LANG=EN_us in the Runtests.gmk, but this modification 
is more violent because he will affect all test cases.



2.We modify each individual test，E.g 
test/hotspot/jtreg/compiler/c2/Test8062950.java


diff -r 0421d49b6217 test/hotspot/jtreg/compiler/c2/Test8062950.java
  package compiler.c2;
  import jdk.test.lib.process.ProcessTools;
+import java.util.Map;
+import jdk.test.lib.process.OutputAnalyzer;

  public class Test8062950 {
  private static final String CLASSNAME = "DoesNotExist";
  public static void main(String[] args) throws Exception {
- ProcessTools.executeTestJvm("-Xcomp",
- "-XX:-TieredCompilation",
- "-XX:-UseOptoBiasInlining",
- CLASSNAME)
- .shouldHaveExitValue(1)
-    .shouldContain("Error: Could not find or load main 
class " + CLASSNAME)

-    .shouldNotContain("A fatal error has been detected")
-    .shouldNotContain("Internal Error")
-    .shouldNotContain("HotSpot Virtual Machine Error");
+    final ProcessBuilder pb = 
ProcessTools.createJavaProcessBuilder(true,

+ "-Xcomp",
+ "-XX:-TieredCompilation",
+ "-XX:-UseOptoBiasInlining",
+ CLASSNAME);
+    final Map env = pb.environment();
+    env.put("LC_ALL", "en_US.UTF-8");
+    OutputAnalyzer output = new OutputAnalyzer(pb.start());
+ output.shouldHaveExitValue(1);
+    output.shouldContain("Error: Could not find or load main class 
" + CLASSNAME);

+    output.shouldNotContain("A fatal error has 

Re: Review Request: 8221530: Caller sensitive methods not handling caller = null when invoked by JNI code with no java frames on stack

[David Holmes]

Hi Mandy,

This sounds quite reasonable to me. If there is no calling context then 
only public entities of publicly accessible packages should be 
accessible. JNI itself does not apply access checks so JNI code should 
be using direct field access, and not core reflection, if it needs to 
bypass access checks.


David

On 28/03/2019 9:17 am, Mandy Chung wrote:

This is to fix a regression introduced in JDK 12 by JDK-8206240.
This impacts only native application that creates JVM via JNI
and also invokes Field::getField (or other reflection API) via
JNI that triggers reflection access check against the caller class.
The regression happens only when there is no caller frame, i.e.
when a native thread attaches to JVM e.g. custom launcher.

It's unclear why the native code invokes Field::getField via JNI
to get the value of `java.lang.Integer::TYPE`.  Alternatively it could
call GetFieldID to get jfieldID of that field and then GetObjectField
if this has to be done in JNI (perhaps bypass encapsulation?).

There is no clear semantics what reflection should behave when there
is no caller frame.   Previously, JNI call to access 
`java.lang.Integer.TYPE`

field happens to work since it is called very early at runtime and
AccessibleObject::cache is null and happens that cache == caller == null.

The proposed fix is to perform proper access check.  When there is no
caller frame, it only allows to access to public members of a public type
in an unconditional exported API package.

If a native thread attaches to the VM and attempts to access a non-public
member or a public member in a non-exported type e.g. 
jdk.internal.misc.Unsafe,

it will throw IAE whereas the access check succeeds in JDK 11. It is
illegal to access a non-exported type.   I think the compatibility risk
should be low as this only happens to a native code creating its VM and
calling reflection to access a member in a non-exported type. There is
no change to the behavior of JNI GetFieldID and GetObjectField.

Webrev:
http://cr.openjdk.java.net/~mchung/jdk13/webrevs/8221530/webrev.00/

I will create a CSR.

Thanks
Mandy

Re: RFR (XS) 8221400: [TESTBUG] java/lang/String/StringRepeat.java requests too much heap

[David Holmes]

On 26/03/2019 11:56 pm, Aleksey Shipilev wrote:

On 3/26/19 2:28 PM, Alan Bateman wrote:

Also, are we not doing "[TESTBUG]" prefixes in
synopsis anymore? I see you changed it in JIRA.


The "testbug" label is used to tag test only issues, encoding in the bug 
description/commit message
isn't really reliable. Yes, some areas have used "[TESTBUG]", there was a time 
when some people put
a "TEST-BUG" prefix. There isn't a JDK-wide convention (at least not to my 
knowledge).


Understood, dropped "[TESTBUG]" from the other issue I have around.


Given labels are not evident in emails I much prefer to see TESTBUG in 
the bug synopsis so it's evident in the RFR.


David
-


Thanks,
-Aleksey

Re: ClassCastException: cannot assign SerializedLambda to field with ClassLoader

[David Holmes]

Hi Seth,

I think we've both been chasing red herrings :) The classloader is not 
relevant. I can run the test with everything loaded as normal by the app 
loader and it still gets the ClassCastException. Searching JBS it seems 
that serialization of lambdas is broken - ref:


https://bugs.openjdk.java.net/browse/JDK-8154236
https://bugs.openjdk.java.net/browse/JDK-8174865
https://bugs.openjdk.java.net/browse/JDK-8174864

though I'm not clear if your testcase falls under the same 
categorization as above.


David
-

On 26/03/2019 2:08 pm, David Holmes wrote:

On 26/03/2019 2:03 pm, seth lytle wrote:
it's a nested class, but it's loaded with the same class loader as the 
toplevel. which is what i thought you thought was the problem when


Sorry, hard to see all the diffs inside the email. Not sure what 
relevance splitting into two files has given the main change was to load 
the enclosing class in the new loader rather than the nested class.


Anyway, yes this gets rid of the "nested class in a different 
classloader" problem.


I'll take a look and see what's getting generated under the hood.

David
-


you wrote:
 > this test loads a nested type into a different classloader to its 
enclosing type


and:
 > it is a requirement that all nest members be defined in the same
 > package - which implies the same classloader




On Mon, Mar 25, 2019 at 11:40 PM David Holmes <mailto:david.hol...@oracle.com>> wrote:


    On 26/03/2019 1:06 pm, seth lytle wrote:
 > i still get the same stack trace after breaking the example out
    into two
 > files. does that sufficiently address the nesting issue ?

    ??

      public static class MyCode ...

    is still a nested class.

    David

 > file: SerializedLambdaTest
 > package test;
 >
 > import java.io.ByteArrayInputStream;
 > import java.io.ByteArrayOutputStream;
 > import java.io.IOException;
 > import java.io.ObjectInputStream;
 > import java.io.ObjectOutputStream;
 > import java.io.Serializable;
 >
 > // from: https://bugs.openjdk.java.net/browse/JDK-8008770
 > public class SerializedLambdaTest implements Runnable {
 >      public void run() { new MyCode().run(); }
 >      public interface SerializableRunnable extends
    Runnable,Serializable {
 >      }
 >      public static class MyCode implements SerializableRunnable {
 >          SerializableRunnable runnable2 = ()
 >                  -> {
 >              System.out.println("HELLO"+this.getClass());
 >          };
 >          private byte[] serialize(Object o) {
 >              ByteArrayOutputStream baos;
 >              try (
 >                       ObjectOutputStream oos
 >                      = new ObjectOutputStream(baos = new
 > ByteArrayOutputStream())) {
 >                  oos.writeObject(o);
 >              } catch (IOException e) {
 >                  throw new RuntimeException(e);
 >              }
 >              return baos.toByteArray();
 >          }
 >          private  T deserialize(byte[] bytes) {
 >              try (
 >                       ObjectInputStream ois
 >                      = new ObjectInputStream(new
 > ByteArrayInputStream(bytes))) {
 >                  return (T) ois.readObject();
 >              } catch (IOException|ClassNotFoundException e) {
 >                  throw new RuntimeException(e);
 >              }
 >          }
 >          @Override
 >          public void run() {
 >              System.out.println("                this: "+this);
 >              SerializableRunnable deSerializedThis
 >                      = deserialize(serialize(this));
 >              System.out.println("    deSerializedThis: "
 >                      +deSerializedThis);
 >
 >              SerializableRunnable runnable = runnable2;
 >              System.out.println("            runnable: 
"+runnable);

 >              SerializableRunnable deSerializedRunnable
 >                      = deserialize(serialize(runnable));
 >              System.out.println("deSerializedRunnable: "
 >                      +deSerializedRunnable);
 >          }
 >      }
 > }
 >
 >
 > file: MyClassLoader
 > package test;
 >
 > import java.io.IOException;
 > import java.io.InputStream;
 >
 > class MyClassLoader extends ClassLoader {
 >      MyClassLoader(ClassLoader parent) {
 >          super(parent);
 >      }
 >      @Override
 >      protected Class loadClass(String name,boolean resol

Re: ClassCastException: cannot assign SerializedLambda to field with ClassLoader

[David Holmes]

On 26/03/2019 2:03 pm, seth lytle wrote:
it's a nested class, but it's loaded with the same class loader as the 
toplevel. which is what i thought you thought was the problem when


Sorry, hard to see all the diffs inside the email. Not sure what 
relevance splitting into two files has given the main change was to load 
the enclosing class in the new loader rather than the nested class.


Anyway, yes this gets rid of the "nested class in a different 
classloader" problem.


I'll take a look and see what's getting generated under the hood.

David
-


you wrote:
 > this test loads a nested type into a different classloader to its 
enclosing type


and:
 > it is a requirement that all nest members be defined in the same
 > package - which implies the same classloader




On Mon, Mar 25, 2019 at 11:40 PM David Holmes <mailto:david.hol...@oracle.com>> wrote:


On 26/03/2019 1:06 pm, seth lytle wrote:
 > i still get the same stack trace after breaking the example out
into two
 > files. does that sufficiently address the nesting issue ?

??

      public static class MyCode ...

is still a nested class.

David

 > file: SerializedLambdaTest
 > package test;
 >
 > import java.io.ByteArrayInputStream;
 > import java.io.ByteArrayOutputStream;
 > import java.io.IOException;
 > import java.io.ObjectInputStream;
 > import java.io.ObjectOutputStream;
 > import java.io.Serializable;
 >
 > // from: https://bugs.openjdk.java.net/browse/JDK-8008770
 > public class SerializedLambdaTest implements Runnable {
 >      public void run() { new MyCode().run(); }
 >      public interface SerializableRunnable extends
Runnable,Serializable {
 >      }
 >      public static class MyCode implements SerializableRunnable {
 >          SerializableRunnable runnable2 = ()
 >                  -> {
 >              System.out.println("HELLO"+this.getClass());
 >          };
 >          private byte[] serialize(Object o) {
 >              ByteArrayOutputStream baos;
 >              try (
 >                       ObjectOutputStream oos
 >                      = new ObjectOutputStream(baos = new
 > ByteArrayOutputStream())) {
 >                  oos.writeObject(o);
 >              } catch (IOException e) {
 >                  throw new RuntimeException(e);
 >              }
 >              return baos.toByteArray();
 >          }
 >          private  T deserialize(byte[] bytes) {
 >              try (
 >                       ObjectInputStream ois
 >                      = new ObjectInputStream(new
 > ByteArrayInputStream(bytes))) {
 >                  return (T) ois.readObject();
 >              } catch (IOException|ClassNotFoundException e) {
 >                  throw new RuntimeException(e);
 >              }
 >          }
 >          @Override
 >          public void run() {
 >              System.out.println("                this: "+this);
 >              SerializableRunnable deSerializedThis
 >                      = deserialize(serialize(this));
 >              System.out.println("    deSerializedThis: "
 >                      +deSerializedThis);
 >
 >              SerializableRunnable runnable = runnable2;
 >              System.out.println("            runnable: "+runnable);
 >              SerializableRunnable deSerializedRunnable
 >                      = deserialize(serialize(runnable));
 >              System.out.println("deSerializedRunnable: "
 >                      +deSerializedRunnable);
 >          }
 >      }
 > }
 >
 >
 > file: MyClassLoader
 > package test;
 >
 > import java.io.IOException;
 > import java.io.InputStream;
 >
 > class MyClassLoader extends ClassLoader {
 >      MyClassLoader(ClassLoader parent) {
 >          super(parent);
 >      }
 >      @Override
 >      protected Class loadClass(String name,boolean resolve)
throws
 > ClassNotFoundException {
 >          if (name.startsWith("test."))
 >              synchronized (getClassLoadingLock(name)) {
 >                  Class c = findLoadedClass(name);
 >                  if (c==null) c = findClass(name);
 >                  if (resolve) resolveClass(c);
 >                  return c;
 >              }
 >          else return super.loadClass(name,resolve);
 >      }
 >      @Override
 >      protected Class findClass(Strin

Re: ClassCastException: cannot assign SerializedLambda to field with ClassLoader

[David Holmes]

On 26/03/2019 1:06 pm, seth lytle wrote:
i still get the same stack trace after breaking the example out into two 
files. does that sufficiently address the nesting issue ?


??

public static class MyCode ...

is still a nested class.

David


file: SerializedLambdaTest
package test;

import java.io.ByteArrayInputStream;
import java.io.ByteArrayOutputStream;
import java.io.IOException;
import java.io.ObjectInputStream;
import java.io.ObjectOutputStream;
import java.io.Serializable;

// from: https://bugs.openjdk.java.net/browse/JDK-8008770
public class SerializedLambdaTest implements Runnable {
     public void run() { new MyCode().run(); }
     public interface SerializableRunnable extends Runnable,Serializable {
     }
     public static class MyCode implements SerializableRunnable {
         SerializableRunnable runnable2 = ()
                 -> {
             System.out.println("HELLO"+this.getClass());
         };
         private byte[] serialize(Object o) {
             ByteArrayOutputStream baos;
             try (
                      ObjectOutputStream oos
                     = new ObjectOutputStream(baos = new 
ByteArrayOutputStream())) {

                 oos.writeObject(o);
             } catch (IOException e) {
                 throw new RuntimeException(e);
             }
             return baos.toByteArray();
         }
         private  T deserialize(byte[] bytes) {
             try (
                      ObjectInputStream ois
                     = new ObjectInputStream(new 
ByteArrayInputStream(bytes))) {

                 return (T) ois.readObject();
             } catch (IOException|ClassNotFoundException e) {
                 throw new RuntimeException(e);
             }
         }
         @Override
         public void run() {
             System.out.println("                this: "+this);
             SerializableRunnable deSerializedThis
                     = deserialize(serialize(this));
             System.out.println("    deSerializedThis: "
                     +deSerializedThis);

             SerializableRunnable runnable = runnable2;
             System.out.println("            runnable: "+runnable);
             SerializableRunnable deSerializedRunnable
                     = deserialize(serialize(runnable));
             System.out.println("deSerializedRunnable: "
                     +deSerializedRunnable);
         }
     }
}


file: MyClassLoader
package test;

import java.io.IOException;
import java.io.InputStream;

class MyClassLoader extends ClassLoader {
     MyClassLoader(ClassLoader parent) {
         super(parent);
     }
     @Override
     protected Class loadClass(String name,boolean resolve) throws 
ClassNotFoundException {

         if (name.startsWith("test."))
             synchronized (getClassLoadingLock(name)) {
                 Class c = findLoadedClass(name);
                 if (c==null) c = findClass(name);
                 if (resolve) resolveClass(c);
                 return c;
             }
         else return super.loadClass(name,resolve);
     }
     @Override
     protected Class findClass(String name) throws 
ClassNotFoundException {

         String path = name.replace('.','/').concat(".class");
         try (final InputStream is = getResourceAsStream(path)) {
             if (is!=null) {
                 byte[] bytes = is.readAllBytes();
                 return defineClass(name,bytes,0,bytes.length);
             }
             else throw new ClassNotFoundException(name);
         } catch (IOException e) {
             throw new ClassNotFoundException(name,e);
         }
     }
     public static void main(String[] args) throws Exception {
         ClassLoader myCl = new MyClassLoader(
                 MyClassLoader.class.getClassLoader()
         );
         Class myCodeClass = Class.forName(
                 "test.SerializedLambdaTest",
                 true,
                 myCl
         );
         Runnable myCode = (Runnable) myCodeClass.newInstance();
         myCode.run();
     }
}




On Mon, Mar 25, 2019 at 10:34 PM David Holmes <mailto:david.hol...@oracle.com>> wrote:


Hi Seth,

On 26/03/2019 12:16 pm, seth lytle wrote:
 > i haven't changed the assignment from the original test case
(which was
 > accepted as valid at the time). i haven't gone through it in
depth, but
 > assume that it's ok and used it since people are already familiar
with
 > it. it appears to me that that example uses reflection to
allocate an
 > instance using a definitive class loader and then uses a runnable
for
 > the serialization/deserialization cycle
 >
 > the class cast exception happens not at the example level, but deep
 > inside `readObject` when it's doing an internal assignment
 >
 > perhaps my choice of words "into a new classlo

Re: ClassCastException: cannot assign SerializedLambda to field with ClassLoader

[David Holmes]

Hi Seth,

On 26/03/2019 12:16 pm, seth lytle wrote:
i haven't changed the assignment from the original test case (which was 
accepted as valid at the time). i haven't gone through it in depth, but 
assume that it's ok and used it since people are already familiar with 
it. it appears to me that that example uses reflection to allocate an 
instance using a definitive class loader and then uses a runnable for 
the serialization/deserialization cycle


the class cast exception happens not at the example level, but deep 
inside `readObject` when it's doing an internal assignment


perhaps my choice of words "into a new classloader" was insufficient or 
misleading. in both examples that i've come up with so far, the class 
loader calls defineClass directly without first delegating to super (for 
a subset of the classes). so "into a definitive classloader" might have 
been a better choice


I think the basic problem is that this test loads a nested type into a 
different classloader to its enclosing type. I can easily imagine this 
messing up the code that gets generated to implement the lambda 
expression - but I'd need to examine that code in detail to see exactly 
why (others may know this more readily than I do). It's unclear to me 
whether this would be considered a bug or a "don't do that" situation. 
As of JDK 11, nested types define "nests" at the VM level (see JEP-181) 
and it is a requirement that all nest members be defined in the same 
package - which implies the same classloader.


David







On Mon, Mar 25, 2019 at 9:34 PM David Holmes <mailto:david.hol...@oracle.com>> wrote:


Hi Seth,

On 26/03/2019 11:22 am, seth lytle wrote:
 > if a lambda is a field and captures `this`, and it's deserialized
into a
 > new class loader, it throws a ClassCastException.

Not sure I follow. If you load a class into a different classloader
then
you get a different type. It might appear the same to you but it is a
distinct type, so you can't assign across different instances loaded by
different classloaders.

Cheers,
David
-

 > i came across this bug
 > independently while writing a test, but then found an old openjdk
bug with
 > a similar issue and tweaked the test case (source below). my version
 > differs only in
 > 1. moved the lambda to a field
 > 2. reference `this` in it
 > 3. uses readAllBytes instead of the internal method
 > 4. formatting
 >
 > running with java 11 or 12 (or java 8 using a substitute for
readAllBytes)
 > results in:
 >                  this: test.SerializedLambdaTest$MyCode@8efb846
 >      deSerializedThis: test.SerializedLambdaTest$MyCode@2b71fc7e
 >              runnable:
 >
test.SerializedLambdaTest$MyCode$$Lambda$1/0x000801188440@37bba400
 > Exception in thread "main" java.lang.ClassCastException: cannot
assign
 > instance of java.lang.invoke.SerializedLambda to field
 > test.SerializedLambdaTest$MyCode.runnable2 of type
 > test.SerializedLambdaTest$SerializableRunnable in instance of
 > test.SerializedLambdaTest$MyCode
 > at
 > java.base/java.io

<http://java.io>.ObjectStreamClass$FieldReflector.setObjFieldValues(ObjectStreamClass.java:2190)
 > at
 > java.base/java.io

<http://java.io>.ObjectStreamClass$FieldReflector.checkObjectFieldValueTypes(ObjectStreamClass.java:2153)
 > at
 > java.base/java.io

<http://java.io>.ObjectStreamClass.checkObjFieldValueTypes(ObjectStreamClass.java:1407)
 > at
 > java.base/java.io

<http://java.io>.ObjectInputStream.defaultCheckFieldValues(ObjectInputStream.java:2371)
 > at
 > java.base/java.io

<http://java.io>.ObjectInputStream.readSerialData(ObjectInputStream.java:2278)
 > at
 > java.base/java.io

<http://java.io>.ObjectInputStream.readOrdinaryObject(ObjectInputStream.java:2087)
 > at
 > java.base/java.io
<http://java.io>.ObjectInputStream.readObject0(ObjectInputStream.java:1594)
 > at
 > java.base/java.io
<http://java.io>.ObjectInputStream.readArray(ObjectInputStream.java:1993)
 > at
 > java.base/java.io
<http://java.io>.ObjectInputStream.readObject0(ObjectInputStream.java:1588)
 > at
 > java.base/java.io

<http://java.io>.ObjectInputStream.defaultReadFields(ObjectInputStream.java:2355)
 > at
 > java.base/java.io

<http://java.io>.ObjectInputStream.readSerialData(ObjectInputStream.java:2249)
 > at
 > java.base/java.io

<http://java.io>.ObjectInputStream.readOrdinaryObject(ObjectInputStream.java:2087)
 > at
 > java.base/java.io
<http

Re: ClassCastException: cannot assign SerializedLambda to field with ClassLoader

[David Holmes]

Hi Seth,

On 26/03/2019 11:22 am, seth lytle wrote:

if a lambda is a field and captures `this`, and it's deserialized into a
new class loader, it throws a ClassCastException. 


Not sure I follow. If you load a class into a different classloader then 
you get a different type. It might appear the same to you but it is a 
distinct type, so you can't assign across different instances loaded by 
different classloaders.


Cheers,
David
-


i came across this bug
independently while writing a test, but then found an old openjdk bug with
a similar issue and tweaked the test case (source below). my version
differs only in
1. moved the lambda to a field
2. reference `this` in it
3. uses readAllBytes instead of the internal method
4. formatting

running with java 11 or 12 (or java 8 using a substitute for readAllBytes)
results in:
 this: test.SerializedLambdaTest$MyCode@8efb846
 deSerializedThis: test.SerializedLambdaTest$MyCode@2b71fc7e
 runnable:
test.SerializedLambdaTest$MyCode$$Lambda$1/0x000801188440@37bba400
Exception in thread "main" java.lang.ClassCastException: cannot assign
instance of java.lang.invoke.SerializedLambda to field
test.SerializedLambdaTest$MyCode.runnable2 of type
test.SerializedLambdaTest$SerializableRunnable in instance of
test.SerializedLambdaTest$MyCode
at
java.base/java.io.ObjectStreamClass$FieldReflector.setObjFieldValues(ObjectStreamClass.java:2190)
at
java.base/java.io.ObjectStreamClass$FieldReflector.checkObjectFieldValueTypes(ObjectStreamClass.java:2153)
at
java.base/java.io.ObjectStreamClass.checkObjFieldValueTypes(ObjectStreamClass.java:1407)
at
java.base/java.io.ObjectInputStream.defaultCheckFieldValues(ObjectInputStream.java:2371)
at
java.base/java.io.ObjectInputStream.readSerialData(ObjectInputStream.java:2278)
at
java.base/java.io.ObjectInputStream.readOrdinaryObject(ObjectInputStream.java:2087)
at
java.base/java.io.ObjectInputStream.readObject0(ObjectInputStream.java:1594)
at
java.base/java.io.ObjectInputStream.readArray(ObjectInputStream.java:1993)
at
java.base/java.io.ObjectInputStream.readObject0(ObjectInputStream.java:1588)
at
java.base/java.io.ObjectInputStream.defaultReadFields(ObjectInputStream.java:2355)
at
java.base/java.io.ObjectInputStream.readSerialData(ObjectInputStream.java:2249)
at
java.base/java.io.ObjectInputStream.readOrdinaryObject(ObjectInputStream.java:2087)
at
java.base/java.io.ObjectInputStream.readObject0(ObjectInputStream.java:1594)
at
java.base/java.io.ObjectInputStream.readObject(ObjectInputStream.java:430)
at
test.SerializedLambdaTest$MyCode.deserialize(SerializedLambdaTest.java:35)
at test.SerializedLambdaTest$MyCode.run(SerializedLambdaTest.java:51)
at test.SerializedLambdaTest.main(SerializedLambdaTest.java:66)



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


package test;

import java.io.ByteArrayInputStream;
import java.io.ByteArrayOutputStream;
import java.io.IOException;
import java.io.InputStream;
import java.io.ObjectInputStream;
import java.io.ObjectOutputStream;
import java.io.Serializable;

// from: https://bugs.openjdk.java.net/browse/JDK-8008770
public class SerializedLambdaTest {
 public interface SerializableRunnable extends Runnable,Serializable {
 }
 public static class MyCode implements SerializableRunnable {
 SerializableRunnable runnable2 = ()
 -> {
 System.out.println("HELLO"+this.getClass());
 };
 private byte[] serialize(Object o) {
 ByteArrayOutputStream baos;
 try (
  ObjectOutputStream oos
 = new ObjectOutputStream(baos = new
ByteArrayOutputStream())) {
 oos.writeObject(o);
 } catch (IOException e) {
 throw new RuntimeException(e);
 }
 return baos.toByteArray();
 }
 private  T deserialize(byte[] bytes) {
 try (
  ObjectInputStream ois
 = new ObjectInputStream(new
ByteArrayInputStream(bytes))) {
 return (T) ois.readObject();
 } catch (IOException|ClassNotFoundException e) {
 throw new RuntimeException(e);
 }
 }
 @Override
 public void run() {
 System.out.println("this: "+this);
 SerializableRunnable deSerializedThis
 = deserialize(serialize(this));
 System.out.println("deSerializedThis: "
 +deSerializedThis);

 SerializableRunnable runnable = runnable2;
 System.out.println("runnable: "+runnable);
 SerializableRunnable deSerializedRunnable
 = deserialize(serialize(runnable));
 System.out.println("deSerializedRunnable: "
 +deSerializedRunnable);
 }
 }
 public static void main(String[] args) throws Exception {
 ClassLoader 

Re: RFR(xs): 8221375: Windows 32bit build (VS2017) broken

[David Holmes]

Hi Thomas,

A few queries, comments and concerns ...

On 25/03/2019 6:58 am, Thomas Stüfe wrote:

Hi all,

After a long time I tried to build a Windows 32bit VM (VS2017) and failed;


I'm somewhat surprised as I thought someone was actively doing Windows 
32-bit builds out there, plus there are shared code changes that should 
also have been caught by non-Windows 32-bit builds. :(



multiple errors and warnings. Lets reverse the bitrot:

cr:
http://cr.openjdk.java.net/~stuefe/webrevs/8221375--windows-32bit-build-(vs2017)-broken-in-many-places/webrev.00/webrev/

Issue: https://bugs.openjdk.java.net/browse/JDK-8221375

Most of the fixes are trivial: Calling convention mismatches (awt DTRACE
callbacks), printf specifiers etc.

Had to supress a warning in os_windows_x86.cpp - I was surprised by this
since this did not look 32bit specifc, do we disable warnings on Windows
64bit builds?


What version of VS2017? We use VS2017 15.9.6 and we don't disable warnings.


The error I had in vmStructs.cpp was a bit weird: compiler complained about
an assignment of an enum value defined like this:

hash_mask_in_place   = (address_word)hash_mask << hash_shift

to an uint64_t variable, complaining about narrowing. I did not find out
what his problem was. In the end, I decided to add an explicit cast to
GENERATE_VM_LONG_CONSTANT_ENTRY(name) (see vmStructs.hpp).


Not at all sure that's the right fix. In markOop.hpp we see that value 
gets special treatment on Windows-x64:


#ifndef _WIN64
 ,hash_mask   = right_n_bits(hash_bits),
 hash_mask_in_place   = (address_word)hash_mask << hash_shift
#endif
  };

  // Alignment of JavaThread pointers encoded in object header required 
by biased locking

  enum { biased_lock_alignment= 2 << (epoch_shift + epoch_bits)
  };

#ifdef _WIN64
// These values are too big for Win64
const static uintptr_t hash_mask = right_n_bits(hash_bits);
const static uintptr_t hash_mask_in_place  =
(address_word)hash_mask << hash_shift;
#endif

perhaps something special is needed for Windows-x86. I'm unclear how the 
values can be "too big" ??




With this patch we can build with warnings enabled on 32bit and 64bit
windows.

Since patch fixes both hotspot and JDK parts, I'm sending it to hs-dev and
core-libs-dev.


Don't see anything that actually comes under core-libs-dev. Looks like 
one net-dev, one awt-dev and one security-dev. Sorry.


Cheers,
David
-


Thanks, Thomas

Re: RFR 8220684 : Process.waitFor(long, TimeUnit) can return false for a process that exited within the timeout

[David Holmes]

On 15/03/2019 5:03 pm, Ivan Gerasimov wrote:

Thank you David!


On 3/14/19 11:01 PM, David Holmes wrote:

Hi Ivan,

On 15/03/2019 12:02 pm, Ivan Gerasimov wrote:

Hi David!


On 3/14/19 5:28 PM, David Holmes wrote:

Hi Ivan,

On 15/03/2019 5:49 am, Ivan Gerasimov wrote:

Hello!

The default implementation of Process.waitFor(long, TimeUnit) does 
not check if the process has exited after the last portion of the 
timeout has expired.


Please clarify. There is always a race between detecting a timeout 
and the process actually terminating. If the process actually 
terminates while you're deciding to report a timeout that seems 
just  an acceptable possibility. No matter what you do the process 
could terminate just after you decided to report the timeout.




Current code for waitFor(...) is

  212 do {
  213 try {
  214 exitValue();
  215 return true;
  216 } catch(IllegalThreadStateException ex) {
  217 if (rem > 0)
  218 Thread.sleep(
  219 Math.min(TimeUnit.NANOSECONDS.toMillis(rem) + 1, 100));
  220 }
  221 rem = unit.toNanos(timeout) - (System.nanoTime() - 
startTime);

  222 } while (rem > 0);
  223 return false;

So, if the loop has just processed the last 100 ms portion of the 
timeout, it ends right away, without checking if the process has 
exited during this period.


Ah I see. Yes there should be a final check after what will be the 
last sleep.


Not a big deal of course, but it is more accurate to check if the 
process has exited at the *end* of the specified timeout, and not 100 
milliseconds before the end.


Agreed.

I share Pavel's concern about the implicit overflow in calculating a 
deadline, rather than comparing elapsed time with the timeout. There 
has been some effort in core-libs to remove assumptions about how 
overflows are handled.


We only check sign of the remainingNanos, which is initially strictly 
positive timeout.  Over time it gets decreased by elapsed time.
The elapsed time is the difference between System.nanoTime() measured at 
line 217 and 213, so it is always non-negative (well, strictly positive, 
as there's sleep in between).


I don't really see a possibility of overflow for remainingNanos here.

The deadline may overflow, and it is totally fine, as we don't care 
about its sign.


It's deadline that I was flagging.


Am I missing something?


Just a code cleanliness issue. I had thought, but now can't find, these 
kinds of overflowing calculations were being cleaned up in core-libs. 
But perhaps I'm just mis-remembering.


Cheers,
David



With kind regards,
Ivan


Thanks,
David
-


With kind regards,
Ivan



David
-

JDK has two implementations of Process (for Unix and Windows) and 
they both override waitFor(), so it's not an issue for them.


Still, it is better to provide a more accurate default implementation.

I'm not quite certain the regression test needs to be included in 
the fix.  The test does demonstrate the issue with the unfixed JDK 
and passed Okay on all tested platforms in Mach5. Yet, I suspect 
the test can still show false negative results, as there are no 
guaranties that even such simple application as `true` will finish 
in 100 ms.
I can tag the test as @ignored with a comment, or simply remove it 
from the fix.


BUGURL: https://bugs.openjdk.java.net/browse/JDK-8220684
WEBREV: http://cr.openjdk.java.net/~igerasim/8220684/00/webrev/

Thanks in advance for reviewing!

Re: RFR 6307456 : UnixFileSystem_md.c use of chmod() and access() should handle EINTR signal appropriately (unix)

[David Holmes]

Hi Ivan,

On 15/03/2019 11:42 am, Ivan Gerasimov wrote:

Thank you David!


On 3/14/19 4:48 PM, David Holmes wrote:

Hi Ivan,

This is an "ancient" bug that you are fixing. I don't think it is valid.

On 15/03/2019 3:29 am, Ivan Gerasimov wrote:

Hello!

Not all the man pages agree that chmod, access and statvfs64 can be 
interrupted, but at least on some platforms they are allowed to fail 
with EINTR:  chmod(2) on MacOS, access(2) on Solaris and statvfs(3) 
on Linux.


So, it would be more accurate to wrap these up into a RESTARTABLE loop.


When Java threads are created, or native threads attach to the VM to 
become Java threads, they all get a very specific signal-mask to block 
most (non synchronous) signals. The signals that we install handlers 
for in the VM are also configured with SA_RESTART. So unless 
specifically specified as not honouring SA_RESTART we should not need 
the RESTARTABLE loop.



But isn't it possible to install a custom signal handler through JNI, 
omitting SA_RESTART flag?


It's possible - then you also have to update signal masks. But yes possible.



So I'm not clear exactly what signals we need to be guarding against 
here, or whether this indicates some kind of (historic) mismatch 
between the library and VM code?


grep shows that RESTARTABLE macro and its variants are used throughout 
hotspot and jdk code.


Yes and on closer examination you will find a lot of inconsistencies. 
RESTARTABLE goes back a long way and many of the I/O APIs have switched 
locations over the years. Stuff was copied from the HPI layer into 
hotspot, then back to the JDK and sometimes things were copied with 
RESTARTABLE and sometimes not; and sometimes ports were written that 
copied RESTARTABLE and sometimes not; and sometime new APIs were added 
that were RESTARTABLE and sometimes not. All in all a bit of a mess.


For example here are some uses of write in JDK libs:

./share/native/libzip/zlib/gzwrite.c:writ = 
write(state->fd, state->x.next, put);
./unix/native/libnio/ch/IOUtil.c:return convertReturnVal(env, 
write(fd, , 1), JNI_FALSE);
./unix/native/libnio/ch/FileDispatcherImpl.c:return 
convertReturnVal(env, write(fd, buf, len), JNI_FALSE);
./unix/native/libnio/fs/UnixCopyFile.c: 
RESTARTABLE(write((int)dst, bufp, len), n);
./unix/native/libnio/fs/UnixNativeDispatcher.c: 
RESTARTABLE(write((int)fd, bufp, (size_t)nbytes), n)
./unix/native/libjava/ProcessImpl_md.c:write(c->childenv[1], (char 
*), sizeof(magic)); // magic number first
./unix/native/libjava/io_util_md.c:RESTARTABLE(write(fd, buf, len), 
result);


A mix of RESTARTABLE and not.

If it were possible to guarantee that no syscalls are ever interrupted, 
it would surely be much cleaner to remove all these wrappers and loops.


There is no guarantee as you note - someone could install their own 
handler for SIGUSR1 (not used by the VM) for example and not use 
SA_RESTART and cause unexpected EINTR.


But that problem could arise today in many different places not just the 
couple you are changing here.


So it comes down to a basic question of signal handling philosophy: do 
we expect/require SA_RESTART to always be used, or do we always guard 
against EINTR? The Go folk had a similar choice:


https://github.com/golang/go/issues/20400

We're kind of in a messy undecided state. We use SA_RESTART ourselves 
but don't document its need for others to use, nor do we enforce its use 
even through libjsig (for signal chaining). We use RESTARTABLE in some 
places but not in others.


So yeah feel free to make these changes, just realize they are only one 
part of a larger problem (if we intend to allow no SA_RESTART).


Cheers,
David


With kind regards,
Ivan


Thanks,
David
-

Also, java_io_UnixFileSystem_list was tiny-optimized to avoid 
unnecessary reallocation.


Would you please help review the fix?

BUGURL: https://bugs.openjdk.java.net/browse/JDK-6307456
WEBREV: http://cr.openjdk.java.net/~igerasim/6307456/00/webrev/

Thanks in advance!

Re: RFR 8220684 : Process.waitFor(long, TimeUnit) can return false for a process that exited within the timeout

[David Holmes]

Hi Ivan,

On 15/03/2019 12:02 pm, Ivan Gerasimov wrote:

Hi David!


On 3/14/19 5:28 PM, David Holmes wrote:

Hi Ivan,

On 15/03/2019 5:49 am, Ivan Gerasimov wrote:

Hello!

The default implementation of Process.waitFor(long, TimeUnit) does 
not check if the process has exited after the last portion of the 
timeout has expired.


Please clarify. There is always a race between detecting a timeout and 
the process actually terminating. If the process actually terminates 
while you're deciding to report a timeout that seems just  an 
acceptable possibility. No matter what you do the process could 
terminate just after you decided to report the timeout.




Current code for waitFor(...) is

  212 do {
  213 try {
  214 exitValue();
  215 return true;
  216 } catch(IllegalThreadStateException ex) {
  217 if (rem > 0)
  218 Thread.sleep(
  219 Math.min(TimeUnit.NANOSECONDS.toMillis(rem) + 1, 100));
  220 }
  221 rem = unit.toNanos(timeout) - (System.nanoTime() - 
startTime);

  222 } while (rem > 0);
  223 return false;

So, if the loop has just processed the last 100 ms portion of the 
timeout, it ends right away, without checking if the process has exited 
during this period.


Ah I see. Yes there should be a final check after what will be the last 
sleep.


Not a big deal of course, but it is more accurate to check if the 
process has exited at the *end* of the specified timeout, and not 100 
milliseconds before the end.


Agreed.

I share Pavel's concern about the implicit overflow in calculating a 
deadline, rather than comparing elapsed time with the timeout. There has 
been some effort in core-libs to remove assumptions about how overflows 
are handled.


Thanks,
David
-


With kind regards,
Ivan



David
-

JDK has two implementations of Process (for Unix and Windows) and 
they both override waitFor(), so it's not an issue for them.


Still, it is better to provide a more accurate default implementation.

I'm not quite certain the regression test needs to be included in the 
fix.  The test does demonstrate the issue with the unfixed JDK and 
passed Okay on all tested platforms in Mach5. Yet, I suspect the test 
can still show false negative results, as there are no guaranties 
that even such simple application as `true` will finish in 100 ms.
I can tag the test as @ignored with a comment, or simply remove it 
from the fix.


BUGURL: https://bugs.openjdk.java.net/browse/JDK-8220684
WEBREV: http://cr.openjdk.java.net/~igerasim/8220684/00/webrev/

Thanks in advance for reviewing!

Re: RFR(S) : 8219139 : move hotspot tests from test/jdk/vm

[David Holmes]

Hi Igor,

This all seems fine to me.

Thanks,
David

On 15/03/2019 7:38 am, Igor Ignatyev wrote:

Hi Misha,

thanks for your suggestions, I have moved all runtime tests into 
subdirectories. here is the updated webrev: 
http://cr.openjdk.java.net/~iignatyev//8219139/webrev.01/index.html


Thanks,
-- Igor

On Mar 4, 2019, at 1:57 PM, mikhailo.seledt...@oracle.com 
<mailto:mikhailo.seledt...@oracle.com> wrote:


Hi Igor,

  Sorry it took a while to get back to you on this one. See my comment 
below



On 2/22/19 10:35 AM,mikhailo.seledt...@oracle.com 
<mailto:mikhailo.seledt...@oracle.com>wrote:
Overall the change looks good; thank you Igor for doing this. I have 
couple of comments:


- I am in favor of the approach where we move tests first under 
corresponding sub-component directories in
test/hotspot sub-tree, and then figure out whether to keep them. Even 
though in general I am in favor
of removing tests that are obsolete or of questionable value, this 
requires time, consideration and discussions.
Hence, I recommend filing an RFE for that, which can be addressed 
after the migration.


- Runtime normally avoids tests directly in test/hotspot/jtreg/runtime
The tests should go into underlying sub-directories which best match 
functional area or topic of that test.
In most cases you did it in your change, but there are several tests 
that your change places directly under

test/hotspot/jtreg/runtime/:

ExplicitArithmeticCheck.java
MonitorCacheMaybeExpand_DeadLock.java
ReflectStackOverflow.java
ShiftTest.java - David commented this can go under compiler (a jit test)
WideStrictInline.java
I have looked at the tests in more detail, and here are my 
recommendation of placements:

    ExplicitArithmeticCheck
    This test checks that ArithmeticException is thrown when 
appropriate

    I would recommend placing it under runtime/ErrorHandling
    MonitorCacheMaybeExpand_DeadLock
    Existing folder: runtime/Thread (it does have a locking test)
    Or, alternatively, create a new folder: 'locking' or 'monitors'
    ReflectStackOverflow
    Uses recursive reflection attempting to provoke stack overflow
    Can go under: runtime/reflect
    WideStrictInline:
    checks for correct FP inlining by the interpreter
    I could not find existing sections; perhaps create 'interpreter'
    folder under 'runtime'

Thank you,
Misha


Since we plan (as discussed) to follow up this work with an RFE to 
review and consider removal of old and
not-that-useful tests, you could place them under 'misc' for now. 
Alternatively, find the best match

or create new sub-directories under runtime/ if necessary.


Thank you,
Misha


On 2/21/19 11:53 AM, Igor Ignatyev wrote:


On Feb 21, 2019, at 12:11 AM, David Holmes <mailto:david.hol...@oracle.com>> wrote:


Hi Igor,

On 21/02/2019 3:19 pm, Igor Ignatyev wrote:

http://cr.openjdk.java.net/~iignatyev//8219139/webrev.00/index.html

40 lines changed: 17 ins; 2 del; 21 mod;

Hi all,
could you please review this small patch which moves tests from 
test/jdk/vm?
I find some of these tests - the runtime ones at least - of 
extremely dubious value. They either cover basic functionality that 
is already well covered, or are regression tests for bugs in code 
that hasn't existed for many many years!
as I wrote in another related email: "one of the reason I'm 
proposing this move is exactly questionable value of these tests, I 
want to believe that having these tests in hotspot/ test directories 
will bring more attention to them from corresponding component teams 
and hence they will be removed/reworked/re-whatever faster and 
better. and I also believe that one of the reason we got 
duplications exactly because these tests were located in jdk test 
suite."



BTW:

test/hotspot/jtreg/runtime/ShiftTest.java

is actually a jit test according to the test comment.

sure, I will move it to hotspot/compiler.
there are 16 tests in test/jdk/vm directory. all but 
JniInvocationTest are hotspot tests, so they are moved to 
test/hotspot test suite; JniInvocationTest is a launcher test
No its a JNI invocation API test - nothing to do with our launcher. 
It belongs in runtime/jni. But we already have tests in runtime 
that use the JNI invocation API so this test adds no new coverage.
this is actually was my first reaction, and I even have the webrev 
which moves it to runtime/jni, but then I looked at the associated 
bug and it is filed against tools/launcher. and I even got a false 
(as I know by now) memory that I saw JLI_ method being called from 
the test. there is actually another test 
(dk/tools/launcher/exeJliLaunchTest.c) associated w/ this bug which 
calls JLI_Launch. anyhow, I'll move this test to hotspot/runtime/jni.


I really think the value of these tests needs to be examined before 
they are brought over.
I'd prefer to have follow-up RFEs/tasks, b/c the longer we keep 
jdk/vm directory the more tests can end up there a