Re: RFR : JDK-8192953 - sun/management/jmxremote/bootstrap/*.sh tests fail with error : revokeall.exe: Permission denied
Apart from removing revokeall.exe, all the shell scripts must be converted to Java programs. I have raised JDK-8205972 to track that effort. Harsha On Wednesday 27 June 2018 10:38 PM, mandy chung wrote: What refactoring are you thinking about about? It should be straight-forward to write an utility in java to replace revokeall.exe. As it has been a long-standing testing reliability issue and this is a test-only bug, you have time to fix in 11. Also, your fix does not work if "open" directory does not exist. Mandy On 6/27/18 9:28 AM, Harsha Wardhana B wrote: Since the tests are failing in every CI run, we have the option to push this fix or quarantine the tests. Refactoring the tests takes more than a week of effort and tests will have to be quarantined till then. I guess pushing this fix is the right thing to do now. Harsha On Wednesday 27 June 2018 09:52 PM, mandy chung wrote: I think the right thing to do is to bite the bullet and fix the test properly. In addition, this fix does not seem to work if there is no "open" directory. Mandy On 6/27/18 9:03 AM, Harsha Wardhana B wrote: That will be done subsequently and tracked under a different bug. Don't you think pushing this fix is better than quarantining the tests? Harsha On Wednesday 27 June 2018 08:50 PM, mandy chung wrote: I would suggest to take the time and replace it with java.nio.file API and remove revokeall.exe sooner rather than later. Mandy On 6/26/18 7:09 AM, Harsha Wardhana B wrote: Hi All, Please find the fix for the bug, https://bugs.openjdk.java.net/browse/JDK-8192953 having webrev at, http://cr.openjdk.java.net/~hb/8192953/webrev.00/ The fix grants execute permission for revokeall.exe. The paths in the shell sciprt had to be converted to cygwin paths (/cygwin/c/... ) from windows path (C:/...). Using windows path was causing strange behavior in cygwin. revokeall.exe should be removed and the above tests need to be refactored to use java.nio.Acl* APIs. That plan is in the near future, and the current fix needs to go in to stop consistent failures in Mach5. Please review the above patch and provide feedback if any. Thanks Harsha
Re: RFR(XS): 8205719: Windows Java_sun_tools_attach_VirtualMachineImpl_enqueue() method should include exitCode in exception message
On 6/27/18 6:26 PM, Daniel D. Daugherty wrote: On 6/26/18 3:29 PM, Chris Plummer wrote: Hello, Please review the following: https://bugs.openjdk.java.net/browse/JDK-8205719 http://cr.openjdk.java.net/~cjplummer/8205719/webrev.00/ src/jdk.attach/windows/native/libattach/VirtualMachineImpl.c No comments. Thumbs up. Thanks! Dan I am adding some debugging code to help track down the root cause of JDK-8199811. thanks, Chris
Re: RFR(XS): 8205719: Windows Java_sun_tools_attach_VirtualMachineImpl_enqueue() method should include exitCode in exception message
On 6/26/18 3:29 PM, Chris Plummer wrote: Hello, Please review the following: https://bugs.openjdk.java.net/browse/JDK-8205719 http://cr.openjdk.java.net/~cjplummer/8205719/webrev.00/ src/jdk.attach/windows/native/libattach/VirtualMachineImpl.c No comments. Thumbs up. Dan I am adding some debugging code to help track down the root cause of JDK-8199811. thanks, Chris
Re: RFR(XS): 8205719: Windows Java_sun_tools_attach_VirtualMachineImpl_enqueue() method should include exitCode in exception message
Ping! One last request for a second reviewer. I'd really like to get this pushed before the cutoff. It's a p4, so I can't push it to 11 after the cutoff. The changes are trivial. thanks, Chris On 6/27/18 11:41 AM, Chris Plummer wrote: Thanks, Serguei! Can I get a second reviewer please? thanks, Chris On 6/26/18 1:25 PM, serguei.spit...@oracle.com wrote: Hi Chris, It looks good to me. Thanks, Serguei On 6/26/18 12:29, Chris Plummer wrote: Hello, Please review the following: https://bugs.openjdk.java.net/browse/JDK-8205719 http://cr.openjdk.java.net/~cjplummer/8205719/webrev.00/ I am adding some debugging code to help track down the root cause of JDK-8199811. thanks, Chris
Re: RFR: JDK-8205681: [TEST_BUG] vmTestbase/nsk/jvmti/Allocate/alloc001/TestDescription.java fails with exit code 98
+1 On 6/27/18 10:19 AM, serguei.spit...@oracle.com wrote: Hi Alex, It looks good. Thanks, Serguei On 6/27/18 08:27, Alex Menkov wrote: Hi all, please review simple fix for https://bugs.openjdk.java.net/browse/JDK-8205681 webrev: http://cr.openjdk.java.net/~amenkov/open_test_env/webrev/ The test was recently open-sourced and path to file in the open repository assumes open repo is cloned into directory named "open". For openjdk-only build root directory can be named differently. The test is now in open repo, so the path can be simplified. --alex
Re: RFR(XS): 8205719: Windows Java_sun_tools_attach_VirtualMachineImpl_enqueue() method should include exitCode in exception message
Thanks, Serguei! Can I get a second reviewer please? thanks, Chris On 6/26/18 1:25 PM, serguei.spit...@oracle.com wrote: Hi Chris, It looks good to me. Thanks, Serguei On 6/26/18 12:29, Chris Plummer wrote: Hello, Please review the following: https://bugs.openjdk.java.net/browse/JDK-8205719 http://cr.openjdk.java.net/~cjplummer/8205719/webrev.00/ I am adding some debugging code to help track down the root cause of JDK-8199811. thanks, Chris
Re: RFR: JDK-8205681: [TEST_BUG] vmTestbase/nsk/jvmti/Allocate/alloc001/TestDescription.java fails with exit code 98
Hi Alex, It looks good. Thanks, Serguei On 6/27/18 08:27, Alex Menkov wrote: Hi all, please review simple fix for https://bugs.openjdk.java.net/browse/JDK-8205681 webrev: http://cr.openjdk.java.net/~amenkov/open_test_env/webrev/ The test was recently open-sourced and path to file in the open repository assumes open repo is cloned into directory named "open". For openjdk-only build root directory can be named differently. The test is now in open repo, so the path can be simplified. --alex
Re: RFR : JDK-8192953 - sun/management/jmxremote/bootstrap/*.sh tests fail with error : revokeall.exe: Permission denied
What refactoring are you thinking about about? It should be straight-forward to write an utility in java to replace revokeall.exe. As it has been a long-standing testing reliability issue and this is a test-only bug, you have time to fix in 11. Also, your fix does not work if "open" directory does not exist. Mandy On 6/27/18 9:28 AM, Harsha Wardhana B wrote: Since the tests are failing in every CI run, we have the option to push this fix or quarantine the tests. Refactoring the tests takes more than a week of effort and tests will have to be quarantined till then. I guess pushing this fix is the right thing to do now. Harsha On Wednesday 27 June 2018 09:52 PM, mandy chung wrote: I think the right thing to do is to bite the bullet and fix the test properly. In addition, this fix does not seem to work if there is no "open" directory. Mandy On 6/27/18 9:03 AM, Harsha Wardhana B wrote: That will be done subsequently and tracked under a different bug. Don't you think pushing this fix is better than quarantining the tests? Harsha On Wednesday 27 June 2018 08:50 PM, mandy chung wrote: I would suggest to take the time and replace it with java.nio.file API and remove revokeall.exe sooner rather than later. Mandy On 6/26/18 7:09 AM, Harsha Wardhana B wrote: Hi All, Please find the fix for the bug, https://bugs.openjdk.java.net/browse/JDK-8192953 having webrev at, http://cr.openjdk.java.net/~hb/8192953/webrev.00/ The fix grants execute permission for revokeall.exe. The paths in the shell sciprt had to be converted to cygwin paths (/cygwin/c/... ) from windows path (C:/...). Using windows path was causing strange behavior in cygwin. revokeall.exe should be removed and the above tests need to be refactored to use java.nio.Acl* APIs. That plan is in the near future, and the current fix needs to go in to stop consistent failures in Mach5. Please review the above patch and provide feedback if any. Thanks Harsha
Re: RFR : JDK-8192953 - sun/management/jmxremote/bootstrap/*.sh tests fail with error : revokeall.exe: Permission denied
Since the tests are failing in every CI run, we have the option to push this fix or quarantine the tests. Refactoring the tests takes more than a week of effort and tests will have to be quarantined till then. I guess pushing this fix is the right thing to do now. Harsha On Wednesday 27 June 2018 09:52 PM, mandy chung wrote: I think the right thing to do is to bite the bullet and fix the test properly. In addition, this fix does not seem to work if there is no "open" directory. Mandy On 6/27/18 9:03 AM, Harsha Wardhana B wrote: That will be done subsequently and tracked under a different bug. Don't you think pushing this fix is better than quarantining the tests? Harsha On Wednesday 27 June 2018 08:50 PM, mandy chung wrote: I would suggest to take the time and replace it with java.nio.file API and remove revokeall.exe sooner rather than later. Mandy On 6/26/18 7:09 AM, Harsha Wardhana B wrote: Hi All, Please find the fix for the bug, https://bugs.openjdk.java.net/browse/JDK-8192953 having webrev at, http://cr.openjdk.java.net/~hb/8192953/webrev.00/ The fix grants execute permission for revokeall.exe. The paths in the shell sciprt had to be converted to cygwin paths (/cygwin/c/... ) from windows path (C:/...). Using windows path was causing strange behavior in cygwin. revokeall.exe should be removed and the above tests need to be refactored to use java.nio.Acl* APIs. That plan is in the near future, and the current fix needs to go in to stop consistent failures in Mach5. Please review the above patch and provide feedback if any. Thanks Harsha
Re: RFR : JDK-8192953 - sun/management/jmxremote/bootstrap/*.sh tests fail with error : revokeall.exe: Permission denied
I think the right thing to do is to bite the bullet and fix the test properly. In addition, this fix does not seem to work if there is no "open" directory. Mandy On 6/27/18 9:03 AM, Harsha Wardhana B wrote: That will be done subsequently and tracked under a different bug. Don't you think pushing this fix is better than quarantining the tests? Harsha On Wednesday 27 June 2018 08:50 PM, mandy chung wrote: I would suggest to take the time and replace it with java.nio.file API and remove revokeall.exe sooner rather than later. Mandy On 6/26/18 7:09 AM, Harsha Wardhana B wrote: Hi All, Please find the fix for the bug, https://bugs.openjdk.java.net/browse/JDK-8192953 having webrev at, http://cr.openjdk.java.net/~hb/8192953/webrev.00/ The fix grants execute permission for revokeall.exe. The paths in the shell sciprt had to be converted to cygwin paths (/cygwin/c/... ) from windows path (C:/...). Using windows path was causing strange behavior in cygwin. revokeall.exe should be removed and the above tests need to be refactored to use java.nio.Acl* APIs. That plan is in the near future, and the current fix needs to go in to stop consistent failures in Mach5. Please review the above patch and provide feedback if any. Thanks Harsha
Re: RFR : JDK-8192953 - sun/management/jmxremote/bootstrap/*.sh tests fail with error : revokeall.exe: Permission denied
That will be done subsequently and tracked under a different bug. Don't you think pushing this fix is better than quarantining the tests? Harsha On Wednesday 27 June 2018 08:50 PM, mandy chung wrote: I would suggest to take the time and replace it with java.nio.file API and remove revokeall.exe sooner rather than later. Mandy On 6/26/18 7:09 AM, Harsha Wardhana B wrote: Hi All, Please find the fix for the bug, https://bugs.openjdk.java.net/browse/JDK-8192953 having webrev at, http://cr.openjdk.java.net/~hb/8192953/webrev.00/ The fix grants execute permission for revokeall.exe. The paths in the shell sciprt had to be converted to cygwin paths (/cygwin/c/... ) from windows path (C:/...). Using windows path was causing strange behavior in cygwin. revokeall.exe should be removed and the above tests need to be refactored to use java.nio.Acl* APIs. That plan is in the near future, and the current fix needs to go in to stop consistent failures in Mach5. Please review the above patch and provide feedback if any. Thanks Harsha
RFR: JDK-8205681: [TEST_BUG] vmTestbase/nsk/jvmti/Allocate/alloc001/TestDescription.java fails with exit code 98
Hi all, please review simple fix for https://bugs.openjdk.java.net/browse/JDK-8205681 webrev: http://cr.openjdk.java.net/~amenkov/open_test_env/webrev/ The test was recently open-sourced and path to file in the open repository assumes open repo is cloned into directory named "open". For openjdk-only build root directory can be named differently. The test is now in open repo, so the path can be simplified. --alex
Re: RFR : JDK-8192953 - sun/management/jmxremote/bootstrap/*.sh tests fail with error : revokeall.exe: Permission denied
I would suggest to take the time and replace it with java.nio.file API and remove revokeall.exe sooner rather than later. Mandy On 6/26/18 7:09 AM, Harsha Wardhana B wrote: Hi All, Please find the fix for the bug, https://bugs.openjdk.java.net/browse/JDK-8192953 having webrev at, http://cr.openjdk.java.net/~hb/8192953/webrev.00/ The fix grants execute permission for revokeall.exe. The paths in the shell sciprt had to be converted to cygwin paths (/cygwin/c/... ) from windows path (C:/...). Using windows path was causing strange behavior in cygwin. revokeall.exe should be removed and the above tests need to be refactored to use java.nio.Acl* APIs. That plan is in the near future, and the current fix needs to go in to stop consistent failures in Mach5. Please review the above patch and provide feedback if any. Thanks Harsha
Re: RFR(XXXS): 8205648 fix for 8205195 breaks secondary error handling
Thanks! And thanks for catching the bug itself.
Dan
On 6/26/18 11:43 PM, David Holmes wrote:
Looks good Dan!
Thanks,
David
On 26/06/2018 11:06 PM, Daniel D. Daugherty wrote:
Greetings,
For this one liner review, I'd love to hear from David H., Thomas Stüfe,
and Serguei Spitsyn.
David Holmes spotted a locking problem with my fix for the following
bug:
JDK-8205195 NestedThreadsListHandleInErrorHandlingTest fails
because
hs_err doesn't contain _nested_thread_list_max
https://bugs.openjdk.java.net/browse/JDK-8205195
Webrev URL:
http://cr.openjdk.java.net/~dcubed/8205195-webrev/0-for-jdk-jdk/
The fix for the issue is a one liner that only grabs the Threads_lock
when the caller does not already own it:
$ hg diff
diff -r 5698cf4e50f1 src/hotspot/share/utilities/vmError.cpp
--- a/src/hotspot/share/utilities/vmError.cpp Fri Jun 22 12:15:16
2018 -0400
+++ b/src/hotspot/share/utilities/vmError.cpp Mon Jun 25 21:20:52
2018 -0400
@@ -1703,7 +1703,7 @@
// from racing with Threads::add() or Threads::remove() as we
// generate the hs_err_pid file. This makes our ErrorHandling tests
// more stable.
- MutexLockerEx ml(Threads_lock, Mutex::_no_safepoint_check_flag);
+ MutexLockerEx ml(Threads_lock->owned_by_self() ? NULL :
Threads_lock, Mutex::_no_safepoint_check_flag);
switch (how) {
case 1: vmassert(str == NULL, "expected null"); break;
Figuring out a way to detect the failure mode in order to test the
fix was much more involved than the fix itself. Gory details are
in the bug:
JDK-8205648 fix for 8205195 breaks secondary error handling
https://bugs.openjdk.java.net/browse/JDK-8205648
No webrev since the one liner diff is shown above. This fix was tested
with the test/hotspot/jtreg/runtime/ErrorHandling tests on Linux-X64
and with a special version of ErrorHandling/SecondaryErrorTest.java
that is documented in JDK-8205648.
Thanks, in advance, for any comments, questions or suggestions.
Dan
Re: RFR(s): jcmd VM.classloaders should fold similar loaders
On Wed, Jun 27, 2018 at 2:21 PM, David Holmes wrote: > Hi Thomas, > > > On 27/06/2018 10:13 PM, Thomas Stüfe wrote: >> >> Hi David, >> >> On Mon, Jun 25, 2018 at 7:48 AM, David Holmes >> wrote: >>> >>> Hi Thomas, >>> >>> >>> On 23/06/2018 5:10 AM, Thomas Stüfe wrote: Resent with the correct subject, sorry. ..Thomas On Fri, Jun 22, 2018 at 9:08 PM, Thomas Stüfe wrote: > > > Hi all, > > may I have reviews for this small enhancement to the jcmd > VM.classloader diagnostic command: > > https://bugs.openjdk.java.net/browse/JDK-8205531 > > > http://cr.openjdk.java.net/~stuefe/webrevs/8205531-vm.classloader-tree-folding/webrev.00/webrev/ > > > VM.classloaders prints a tree of class loaders. This tree can grow a > lot and become unwieldy, especially with a lot of similar loaders. One > prime example is the DelegatingClassLoader. It would be helpful were > all these loaders: > > 13114: > +-- > | > +-- "platform", > jdk.internal.loader.ClassLoaders$PlatformClassLoader > | > +-- "app", > jdk.internal.loader.ClassLoaders$AppClassLoader > | > +-- test3.internals.InMemoryClassLoader > | > +-- > jdk.internal.reflect.DelegatingClassLoader > | > +-- > jdk.internal.reflect.DelegatingClassLoader > | > +-- > jdk.internal.reflect.DelegatingClassLoader > | > +-- > jdk.internal.reflect.DelegatingClassLoader > | > +-- > jdk.internal.reflect.DelegatingClassLoader > | > .. repeat 1495 times > >folded into one: > > 13114: > +-- > | > +-- "platform", > jdk.internal.loader.ClassLoaders$PlatformClassLoader > | > +-- "app", > jdk.internal.loader.ClassLoaders$AppClassLoader > | > +-- test3.internals.InMemoryClassLoader > | > +-- > jdk.internal.reflect.DelegatingClassLoader > (+ 1499 more) >>> >>> >>> >>> I don't quite understand that. These are different instances aren't they >>> - >>> potentially uniquely named? So if we gave all loaders a default name >>> (like >>> we do threads) they would all show up expanded again - right? >>> >> >> This patch will fold loaders loaded by the same parent, having the >> same class and the same or no name. >> >> Example: >> >> 25401: >> +-- >>| >>+-- "platform", >> jdk.internal.loader.ClassLoaders$PlatformClassLoader >>| | >>| +-- "app", jdk.internal.loader.ClassLoaders$AppClassLoader >>| >>+-- "small-loader", test3.internals.InMemoryClassLoader (+ 1101 >> more) >> >> Note that I compare addresses of _Symbol_. So, loaders which have the >> _same_ default name still fold, see example above. > > > Right. So in the case of DelegatingClassloader, today this change will fold > them all into one because they are unnamed. Yes. > If tomorrow we give them a > unique name then they will no longer fold. Yes. > There's no substantive difference > between the two cases that I can see so why should they present differently? There is, they are now named :) The whole point of this exercise is to make the output of VM.classloaders better parsable for human brains without omitting information: If class and name are identical, I can mentally expand ["small-loader", test3.internals.InMemoryClassLoader (+ 1101 more)] to 1102 lines of ["small-loader", test3.internals.InMemoryClassLoader]. The moment someone starts giving his classloaders unique names, this makes no sense anymore, since the name carries information I do not want to hide. > I guess I'm not sure the name is actually significant here. Hm. So, you would ignore the name and just fold the same class? And then what, write "various" as name for the 1101 differently named loaders? As I wrote, I did not want to omit information. > > What happens when some of these similar loaders have distinct child loaders? > How will things be grouped and displayed then? No folding is done in that case. Thanks, Thomas > > Thanks, > David > > >> Thanks, Thomas >> >> >>> typo: >>> >>> ! // we we add a >>> >>> Not a review as I don't know any of the logic being modified. >>> >>> David >>> >>> > > > Original idea by Bernd Eckenfels, see > > > http://mail.openjdk.java.net/pipermail/serviceability-dev/2018-May/023824.html > > Thank you, Thomas
Re: RFR(s): jcmd VM.classloaders should fold similar loaders
Hi Thomas, On 27/06/2018 10:13 PM, Thomas Stüfe wrote: Hi David, On Mon, Jun 25, 2018 at 7:48 AM, David Holmes wrote: Hi Thomas, On 23/06/2018 5:10 AM, Thomas Stüfe wrote: Resent with the correct subject, sorry. ..Thomas On Fri, Jun 22, 2018 at 9:08 PM, Thomas Stüfe wrote: Hi all, may I have reviews for this small enhancement to the jcmd VM.classloader diagnostic command: https://bugs.openjdk.java.net/browse/JDK-8205531 http://cr.openjdk.java.net/~stuefe/webrevs/8205531-vm.classloader-tree-folding/webrev.00/webrev/ VM.classloaders prints a tree of class loaders. This tree can grow a lot and become unwieldy, especially with a lot of similar loaders. One prime example is the DelegatingClassLoader. It would be helpful were all these loaders: 13114: +-- | +-- "platform", jdk.internal.loader.ClassLoaders$PlatformClassLoader | +-- "app", jdk.internal.loader.ClassLoaders$AppClassLoader | +-- test3.internals.InMemoryClassLoader | +-- jdk.internal.reflect.DelegatingClassLoader | +-- jdk.internal.reflect.DelegatingClassLoader | +-- jdk.internal.reflect.DelegatingClassLoader | +-- jdk.internal.reflect.DelegatingClassLoader | +-- jdk.internal.reflect.DelegatingClassLoader | .. repeat 1495 times folded into one: 13114: +-- | +-- "platform", jdk.internal.loader.ClassLoaders$PlatformClassLoader | +-- "app", jdk.internal.loader.ClassLoaders$AppClassLoader | +-- test3.internals.InMemoryClassLoader | +-- jdk.internal.reflect.DelegatingClassLoader (+ 1499 more) I don't quite understand that. These are different instances aren't they - potentially uniquely named? So if we gave all loaders a default name (like we do threads) they would all show up expanded again - right? This patch will fold loaders loaded by the same parent, having the same class and the same or no name. Example: 25401: +-- | +-- "platform", jdk.internal.loader.ClassLoaders$PlatformClassLoader | | | +-- "app", jdk.internal.loader.ClassLoaders$AppClassLoader | +-- "small-loader", test3.internals.InMemoryClassLoader (+ 1101 more) Note that I compare addresses of _Symbol_. So, loaders which have the _same_ default name still fold, see example above. Right. So in the case of DelegatingClassloader, today this change will fold them all into one because they are unnamed. If tomorrow we give them a unique name then they will no longer fold. There's no substantive difference between the two cases that I can see so why should they present differently? I guess I'm not sure the name is actually significant here. What happens when some of these similar loaders have distinct child loaders? How will things be grouped and displayed then? Thanks, David Thanks, Thomas typo: ! // we we add a Not a review as I don't know any of the logic being modified. David Original idea by Bernd Eckenfels, see http://mail.openjdk.java.net/pipermail/serviceability-dev/2018-May/023824.html Thank you, Thomas
Re: RFR(s): jcmd VM.classloaders should fold similar loaders
Hi David, On Mon, Jun 25, 2018 at 7:48 AM, David Holmes wrote: > Hi Thomas, > > > On 23/06/2018 5:10 AM, Thomas Stüfe wrote: >> >> Resent with the correct subject, sorry. >> >> ..Thomas >> >> On Fri, Jun 22, 2018 at 9:08 PM, Thomas Stüfe >> wrote: >>> >>> Hi all, >>> >>> may I have reviews for this small enhancement to the jcmd >>> VM.classloader diagnostic command: >>> >>> https://bugs.openjdk.java.net/browse/JDK-8205531 >>> >>> http://cr.openjdk.java.net/~stuefe/webrevs/8205531-vm.classloader-tree-folding/webrev.00/webrev/ >>> >>> >>> VM.classloaders prints a tree of class loaders. This tree can grow a >>> lot and become unwieldy, especially with a lot of similar loaders. One >>> prime example is the DelegatingClassLoader. It would be helpful were >>> all these loaders: >>> >>> 13114: >>> +-- >>>| >>>+-- "platform", >>> jdk.internal.loader.ClassLoaders$PlatformClassLoader >>> | >>> +-- "app", jdk.internal.loader.ClassLoaders$AppClassLoader >>>| >>>+-- test3.internals.InMemoryClassLoader >>> | >>> +-- jdk.internal.reflect.DelegatingClassLoader >>> | >>> +-- jdk.internal.reflect.DelegatingClassLoader >>> | >>> +-- jdk.internal.reflect.DelegatingClassLoader >>> | >>> +-- jdk.internal.reflect.DelegatingClassLoader >>> | >>> +-- jdk.internal.reflect.DelegatingClassLoader >>> | >>> .. repeat 1495 times >>> >>> folded into one: >>> >>> 13114: >>> +-- >>>| >>>+-- "platform", >>> jdk.internal.loader.ClassLoaders$PlatformClassLoader >>> | >>> +-- "app", jdk.internal.loader.ClassLoaders$AppClassLoader >>>| >>>+-- test3.internals.InMemoryClassLoader >>> | >>> +-- jdk.internal.reflect.DelegatingClassLoader >>> (+ 1499 more) > > > I don't quite understand that. These are different instances aren't they - > potentially uniquely named? So if we gave all loaders a default name (like > we do threads) they would all show up expanded again - right? > This patch will fold loaders loaded by the same parent, having the same class and the same or no name. Example: 25401: +-- | +-- "platform", jdk.internal.loader.ClassLoaders$PlatformClassLoader | | | +-- "app", jdk.internal.loader.ClassLoaders$AppClassLoader | +-- "small-loader", test3.internals.InMemoryClassLoader (+ 1101 more) Note that I compare addresses of _Symbol_. So, loaders which have the _same_ default name still fold, see example above. Thanks, Thomas > typo: > > ! // we we add a > > Not a review as I don't know any of the logic being modified. > > David > > >>> >>> >>> Original idea by Bernd Eckenfels, see >>> >>> http://mail.openjdk.java.net/pipermail/serviceability-dev/2018-May/023824.html >>> >>> Thank you, Thomas
Re: RFR: JDK-8189429: SA: MacOSX: Replace the deprecated PT_ATTACH with PT_ATTACHEXC
Thank you very much, David. I will do the test-repeat run of the tests (after a temp fix to enable OSX runs on Mach5 (JDK-8199700)). Thanks, Jini. On 6/27/2018 12:02 PM, David Holmes wrote: Hi Jini, I took a look ... that's about all I can say :) I know that you and Sharath have worked through this in detail over an extended period of time, so I'm okay to add my Reviewed stamp to it. About the only thing I'd suggest, if not already done, is to do a mach5 run only on OSX with --test-repeat 10, to try to check that we are okay on a range of OSX machines. Thanks, David On 26/06/2018 5:25 PM, Jini George wrote: Ping - Gentle reminder ! Thanks, Jini. On 6/25/2018 3:41 PM, Jini George wrote: Thank you, Sharath. May I have a Reviewer to take a look at the MacosxDebuggerLocal code? Thanks, Jini. On 6/25/2018 1:52 PM, Sharath Ballal wrote: Hi Jini, Changes in MacosxDebuggerLocal.m looks good. Thanks, Sharath -Original Message- From: Jini George Sent: Sunday, June 24, 2018 11:07 PM To: Erik Joelsson; serviceability-dev@openjdk.java.net; build-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net Subject: Re: RFR: JDK-8189429: SA: MacOSX: Replace the deprecated PT_ATTACH with PT_ATTACHEXC Hi Erik, Thank you very much for looking into this. I have addressed your comments. The latest webrev is at: http://cr.openjdk.java.net/~jgeorge/8189429/webrev.06/ Thank you, Jini. On 6/23/2018 3:31 AM, Erik Joelsson wrote: Hello Jini, In general this looks pretty good, but it's also breaking some new ground as it's adding generation of native source in the java gensrc step. Mixing native code with the java code that the genrcs targets and gensrc output directories are meant for seems ok for now, but may cause trouble in the future. I'm going to accept it for now though. In Gensrc-jdk.hotspot.agent.gmk: Please put the new macosx stuff in its own section (as delimited by the 80x # lines) and put that whole thing inside a conditional for macosx. Also please break line 47 (for recipe lines, indent with tab and 4 additional spaces for continuation [1]). In Lib-jdk.hotspot.agent.gmk: I believe adding extra src should be enough as that will implicitly add the same directories as header dirs by default. At least that's the intention. /Erik [1] http://openjdk.java.net/groups/build/doc/code-conventions.html On 2018-06-22 11:11, Jini George wrote: Hi all, [Including build-dev also since this includes build related changes]. Requesting reviews for: https://bugs.openjdk.java.net/browse/JDK-8189429 (SA: MacOSX: Replace the deprecated PT_ATTACH with PT_ATTACHEXC) Webrev: http://cr.openjdk.java.net/~jgeorge/8189429/webrev.04/ This is the follow-up solution for the temporary restoration of PT_ATTACH to fix https://bugs.openjdk.java.net/browse/JDK-8184042 (several serviceability/sa tests timed out on MacOS X), which was done in Oct 2017. The mails related to this are at: http://mail.openjdk.java.net/pipermail/serviceability-dev/2017-August /021702.html Changes as compared to the patch sent last year (http://cr.openjdk.java.net/~jgeorge/8184042/webrev.00/): * Addressed the review comments which were provided by Poonam, Dan, Dmitry. * A major change as compared to what was done last year is that the MIG generated files have been included as a part of the JDK build process. * The test case which was provided in the patch last year is no longer required since we have ClhsdbAttach.java testing the same functionality as a part of the SA testsuite now. * Other than that, some minor improvements have been done wrt error handling. The steps for the proposed solution have been provided in JBS. Testing: ALL the SA tests pass on MacOSX and the other Mach5 platforms. Thanks to Sharath, Robin, Gerard and Dan for looking into the changes and providing valuable comments. Thanks, Jini.
Re: RFR: JDK-8189429: SA: MacOSX: Replace the deprecated PT_ATTACH with PT_ATTACHEXC
Hi Jini, I took a look ... that's about all I can say :) I know that you and Sharath have worked through this in detail over an extended period of time, so I'm okay to add my Reviewed stamp to it. About the only thing I'd suggest, if not already done, is to do a mach5 run only on OSX with --test-repeat 10, to try to check that we are okay on a range of OSX machines. Thanks, David On 26/06/2018 5:25 PM, Jini George wrote: Ping - Gentle reminder ! Thanks, Jini. On 6/25/2018 3:41 PM, Jini George wrote: Thank you, Sharath. May I have a Reviewer to take a look at the MacosxDebuggerLocal code? Thanks, Jini. On 6/25/2018 1:52 PM, Sharath Ballal wrote: Hi Jini, Changes in MacosxDebuggerLocal.m looks good. Thanks, Sharath -Original Message- From: Jini George Sent: Sunday, June 24, 2018 11:07 PM To: Erik Joelsson; serviceability-dev@openjdk.java.net; build-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net Subject: Re: RFR: JDK-8189429: SA: MacOSX: Replace the deprecated PT_ATTACH with PT_ATTACHEXC Hi Erik, Thank you very much for looking into this. I have addressed your comments. The latest webrev is at: http://cr.openjdk.java.net/~jgeorge/8189429/webrev.06/ Thank you, Jini. On 6/23/2018 3:31 AM, Erik Joelsson wrote: Hello Jini, In general this looks pretty good, but it's also breaking some new ground as it's adding generation of native source in the java gensrc step. Mixing native code with the java code that the genrcs targets and gensrc output directories are meant for seems ok for now, but may cause trouble in the future. I'm going to accept it for now though. In Gensrc-jdk.hotspot.agent.gmk: Please put the new macosx stuff in its own section (as delimited by the 80x # lines) and put that whole thing inside a conditional for macosx. Also please break line 47 (for recipe lines, indent with tab and 4 additional spaces for continuation [1]). In Lib-jdk.hotspot.agent.gmk: I believe adding extra src should be enough as that will implicitly add the same directories as header dirs by default. At least that's the intention. /Erik [1] http://openjdk.java.net/groups/build/doc/code-conventions.html On 2018-06-22 11:11, Jini George wrote: Hi all, [Including build-dev also since this includes build related changes]. Requesting reviews for: https://bugs.openjdk.java.net/browse/JDK-8189429 (SA: MacOSX: Replace the deprecated PT_ATTACH with PT_ATTACHEXC) Webrev: http://cr.openjdk.java.net/~jgeorge/8189429/webrev.04/ This is the follow-up solution for the temporary restoration of PT_ATTACH to fix https://bugs.openjdk.java.net/browse/JDK-8184042 (several serviceability/sa tests timed out on MacOS X), which was done in Oct 2017. The mails related to this are at: http://mail.openjdk.java.net/pipermail/serviceability-dev/2017-August /021702.html Changes as compared to the patch sent last year (http://cr.openjdk.java.net/~jgeorge/8184042/webrev.00/): * Addressed the review comments which were provided by Poonam, Dan, Dmitry. * A major change as compared to what was done last year is that the MIG generated files have been included as a part of the JDK build process. * The test case which was provided in the patch last year is no longer required since we have ClhsdbAttach.java testing the same functionality as a part of the SA testsuite now. * Other than that, some minor improvements have been done wrt error handling. The steps for the proposed solution have been provided in JBS. Testing: ALL the SA tests pass on MacOSX and the other Mach5 platforms. Thanks to Sharath, Robin, Gerard and Dan for looking into the changes and providing valuable comments. Thanks, Jini.
