Re: RFR: 8227269: Slow class loading when running JVM in debug mode

[Roman Kennke]

That was in the previous implementation: I got a condition wrong in the
table lookup (as noted by Serguei), and this prevented any
class-unload-events from getting out. I have fixed this, but found other
problems in that implementation (deadlocks and a crash).

 The current implementation has none of these problems: we don't need
table-lookups - we simply pass-through the signatures, and locking is
much simpler and in particular we don't need a lock around the JVMTI
call (SetTag) which was the cause of the deadlock.

Does that answer your questions?

Thanks,
Roman

> Hi Roman,
> 
> It passed all my testing. I think before you push Serguei has a question
> regarding an issue you brought up a while back. You mentioned that you
> weren't getting some events, and suddenly started seeing them. We were
> discussing it today and it was unclear if this was an issue you were
> seeing before your changes, and your changes resolved it, or it was
> initially caused by an earlier version of your changes, and you later
> fixed it. We just want to better understand what this issue was and how
> it was fixed.
> 
> thanks,
> 
> Chris
> 
> On 3/25/20 3:22 PM, Roman Kennke wrote:
>> The new job finished, its ID is:
>>
>>   mach5-one-rkennke-JDK-8227269-2-20200325-2027-9716289
>>
>> Thank you,
>> Roman
>>
>>
>>> Yes, please submit a new job. I'll start my testing once I see that the
>>> builds are done.
>>>
>>> Chris
>>>
>>> On 3/25/20 12:59 PM, Roman Kennke wrote:
 Hi Chris,

 Apparently we can get into classTrack_reset() before calling
 activate(),
 and we're seeing a null deletedSignatureBag. A simple NULL-check around
 the cleaning routine fixes the problem for me.

 http://cr.openjdk.java.net/~rkennke/JDK-8227269/webrev.08/

 Should I post another submit-repo job with that fix?

 Thanks,
 Roman


> Hi Roman,
>
> com/sun/jdi/JdwpAllowTest.java crashed on many runs:
>
> Stack: [0x7fbb790f9000,0x7fbb791fa000],
> sp=0x7fbb791f8af0,  free space=1022k
> Native frames: (J=compiled Java code, A=aot compiled Java code,
> j=interpreted, Vv=VM code, C=native code)
> C  [libjdwp.so+0xdb71]  bagEnumerateOver+0x11
> C  [libjdwp.so+0xe365]  classTrack_reset+0x25
> C  [libjdwp.so+0xfca1]  debugInit_reset+0x71
> C  [libjdwp.so+0x12e0d]  debugLoop_run+0x38d
> C  [libjdwp.so+0x25700]  acceptThread+0x80
> V  [libjvm.so+0xf4b5a7]  JvmtiAgentThread::call_start_function()+0x1c7
> V  [libjvm.so+0x15215c6]  JavaThread::thread_main_inner()+0x226
> V  [libjvm.so+0x1527736]  Thread::call_run()+0xf6
> V  [libjvm.so+0x1250ade]  thread_native_entry(Thread*)+0x10e
>
>
> This happened during a test task run of open/test/jdk/:jdk_jdi. There
> doesn't seem to be anything magic on the command line that might be
> triggering. Pretty much I see it with all the various VM configs we
> test.
>
> I'm also seeing crashes in the following tests, but not as often:
>
> serviceability/jvmti/ModuleAwareAgents/ThreadStart/MAAThreadStart.java
> vmTestbase/nsk/jdwp/VirtualMachine/Version/version002/TestDescription.java
>
>
> vmTestbase/nsk/jdwp/VirtualMachine/ReleaseEvents/releaseevents002/TestDescription.java
>
>
> vmTestbase/nsk/jdwp/VirtualMachine/HoldEvents/holdevents002/TestDescription.java
>
>
> vmTestbase/nsk/jdwp/VirtualMachine/Dispose/dispose001/TestDescription.java
>
>
>
> thanks,
>
> Chris
>
>
> On 3/25/20 11:37 AM, Roman Kennke wrote:
>> Hi Chris,
>>
>>> Regarding the new assert:
>>>
>>>    105 if (gdata && gdata->assertOn) {
>>>    106 // Check this is not already tagged.
>>>    107 jlong tag;
>>>    108 error = JVMTI_FUNC_PTR(trackingEnv, GetTag)(env,
>>> klass, &tag);
>>>    109 if (error != JVMTI_ERROR_NONE) {
>>>    110 EXIT_ERROR(error, "Unable to GetTag with class
>>> trackingEnv");
>>>    111 }
>>>    112 JDI_ASSERT(tag == NOT_TAGGED);
>>>    113 }
>>>
>>> I think you should remove the gdata check. gdata should never be
>>> NULL
>>> when you get to this code. If it is ever NULL then there's a bug,
>>> and
>>> the check will hide the bug.
>> Ok, will remove this.
>>
>>> Regarding testing, after you do the submit repo testing let me know
>>> the
>>> jobID and I'll do additional testing on it.
>> I did the submit repo earlier today, and it came back green:
>>
>> mach5-one-rkennke-JDK-8227269-2-20200325-1421-9706762
>>
>> Thanks,
>> Roman
>>
>>> thanks,
>>>
>>> Chris
>>>
>>> On 3/25/20 6:00 AM, Roman Kennke wrote:
 Hi Sergei,

> The fix looks pretty clean now.
> I also like new name of the lock.:)
 Thank you!

> Just one comment bel

Re: RFR: 8196751: Add jhsdb option to specify debug server RMI connector port

[Daniil Titov]

Hi Yasumasa and Serguei,

Thank you for reviewing this change.

Best regards,
--Daniil

On 3/25/20, 1:01 PM, "serguei.spit...@oracle.com"  
wrote:

Hi Daniil,

On 3/24/20 10:00, Daniil Titov wrote:
> Hi Serguei,
>
>> It looks like you removed the last call site of DebugServer.main.
> Yes. It is correct.
>
>> Do we need to remove the DebugServer.java as well?
> I was considering this but since it is a public class I think it needs to 
be deprecated first. I also think that it would be better to do in a separate 
issue
> since a  CSR for deprecation needs to be filed for that.  If you agree I 
will create a new issue for that.

I'm okay to separate this.

Thanks,
Serguei

>
> Thanks,
> Daniil
>
>
> On 3/23/20, 11:56 PM, "serguei.spit...@oracle.com" 
 wrote:
>
>  Hi Daniil,
>  
>  It looks pretty good in general.
>  
>  It looks like you removed the last call site of DebugServer.main.
>  Do we need to remove the DebugServer.java as well?
>  
>  Thanks,
>  Serguei
>  
>  
>  On 3/22/20 15:29, Daniil Titov wrote:
>  > Hi Yasumasa, Serguei and Alex,
>  >
>  > Please review a new version of the webrev that merges 
SADebugDTest.java  with changes  done in  [2].
>  >
>  > Also the CRS [3] and the help message for debug server in 
SALauncher.java were updated to specify that  '--hostname'
>  > option could be a hostname or an IPv4/IPv6 address.
>  >
>  >   >  Ok, but I think it might be more simply with TestLibrary.
>  >   >   For example, can you use TestLibrary::getUnusedRandomPort ? 
It is used in test/jdk/java/rmi/testlibrary/RMID.java .
>  >
>  > TestLibrary:: getUnusedRandomPort() doesn't allow to specify what 
ports are reserved and it uses some hardcoded port range [FIXED_PORT_MIN, 
FIXED_PORT_MAX] as reserved ports. Besides,  
test/jdk/java/rmi/testlibrary/TestLibrary.java class cannot be directly used in 
test/hotspot/jtreg/serviceability/* tests (it doesn't compile).
>  >
>  > Nevertheless, to simplify the test itself I moved 
findUnreservedFreePort(int .. reservedPorts) from SADebugTest.java to 
jdk.test.lib.Utils in /test/lib.
>  >
>  > Testing: Mach5 tier1-tier3 tests (that include 
serviceability/sa/sadebugd tests) succeeded.
>  >
>  > [1] http://cr.openjdk.java.net/~dtitov/8196751/webrev.04/
>  > [2] https://bugs.openjdk.java.net/browse/JDK-8238268
>  > [3] https://bugs.openjdk.java.net/browse/JDK-8239831
>  >
>  > Thank you,
>  > Daniil
>  >
>  > On 3/13/20, 7:23 PM, "Yasumasa Suenaga"  
wrote:
>  >
>  >  Hi Daniil,
>  >
>  >  On 2020/03/14 7:05, Daniil Titov wrote:
>  >  > Hi Yasumasa, Serguei and Alex,
>  >  >
>  >  > Please review a new version of the webrev that includes the 
changes Yasumasa suggested.
>  >  >
>  >  >> Shutdown hook is already registered in c'tor of 
HotSpotAgent.
>  >  >> It works same as shutdownServer(), so I think shutdown 
hook at SALauncher is not needed.
>  >  >
>  >  > The shutdown hook registered in the HotSpotAgent c'tor only 
works for non-servers, so we still need a
>  >  > the shutdown hook for remote server being added in 
SALauncher. I changed it to use  the lambda expression.
>  >  >
>  >  > 101 public HotSpotAgent() {
>  >  >   102 // for non-server add shutdown hook to 
clean-up debugger in case
>  >  >   103 // of forced exit. For remote server, 
shutdown hook is added by
>  >  >   104 // DebugServer.
>  >  >   105 Runtime.getRuntime().addShutdownHook(new 
java.lang.Thread(
>  >  >   106 new Runnable() {
>  >  >   107 public void run() {
>  >  >   108 synchronized (HotSpotAgent.this) {
>  >  >   109 if (!isServer) {
>  >  >   110 detach();
>  >  >   111 }
>  >  >   112 }
>  >  >   113 }
>  >  >   114 }));
>  >  >   115 }
>  >
>  >  I missed it, thanks!
>  >
>  >
>  >  >>> Hmm... I think port check (already in use) is not 
needed because test/hotspot/jtreg/serviceability/sa/sadebugd/TEST.properties 
contains
>  >  >>> `exclusiveAccess.dirs=.` to avoid concurrent execution
>  >  > As I understand exclusiveAccess.dirs prevents only the 
tests located in this directory from being run simulta

Re: RFR: 8227269: Slow class loading when running JVM in debug mode

[Chris Plummer]

Hi Roman,

Yes. Thank you.

Chris

On 3/26/20 1:44 AM, Roman Kennke wrote:

That was in the previous implementation: I got a condition wrong in the
table lookup (as noted by Serguei), and this prevented any
class-unload-events from getting out. I have fixed this, but found other
problems in that implementation (deadlocks and a crash).

  The current implementation has none of these problems: we don't need
table-lookups - we simply pass-through the signatures, and locking is
much simpler and in particular we don't need a lock around the JVMTI
call (SetTag) which was the cause of the deadlock.

Does that answer your questions?

Thanks,
Roman


Hi Roman,

It passed all my testing. I think before you push Serguei has a question
regarding an issue you brought up a while back. You mentioned that you
weren't getting some events, and suddenly started seeing them. We were
discussing it today and it was unclear if this was an issue you were
seeing before your changes, and your changes resolved it, or it was
initially caused by an earlier version of your changes, and you later
fixed it. We just want to better understand what this issue was and how
it was fixed.

thanks,

Chris

On 3/25/20 3:22 PM, Roman Kennke wrote:

The new job finished, its ID is:

   mach5-one-rkennke-JDK-8227269-2-20200325-2027-9716289

Thank you,
Roman



Yes, please submit a new job. I'll start my testing once I see that the
builds are done.

Chris

On 3/25/20 12:59 PM, Roman Kennke wrote:

Hi Chris,

Apparently we can get into classTrack_reset() before calling
activate(),
and we're seeing a null deletedSignatureBag. A simple NULL-check around
the cleaning routine fixes the problem for me.

http://cr.openjdk.java.net/~rkennke/JDK-8227269/webrev.08/

Should I post another submit-repo job with that fix?

Thanks,
Roman



Hi Roman,

com/sun/jdi/JdwpAllowTest.java crashed on many runs:

Stack: [0x7fbb790f9000,0x7fbb791fa000],
sp=0x7fbb791f8af0,  free space=1022k
Native frames: (J=compiled Java code, A=aot compiled Java code,
j=interpreted, Vv=VM code, C=native code)
C  [libjdwp.so+0xdb71]  bagEnumerateOver+0x11
C  [libjdwp.so+0xe365]  classTrack_reset+0x25
C  [libjdwp.so+0xfca1]  debugInit_reset+0x71
C  [libjdwp.so+0x12e0d]  debugLoop_run+0x38d
C  [libjdwp.so+0x25700]  acceptThread+0x80
V  [libjvm.so+0xf4b5a7]  JvmtiAgentThread::call_start_function()+0x1c7
V  [libjvm.so+0x15215c6]  JavaThread::thread_main_inner()+0x226
V  [libjvm.so+0x1527736]  Thread::call_run()+0xf6
V  [libjvm.so+0x1250ade]  thread_native_entry(Thread*)+0x10e


This happened during a test task run of open/test/jdk/:jdk_jdi. There
doesn't seem to be anything magic on the command line that might be
triggering. Pretty much I see it with all the various VM configs we
test.

I'm also seeing crashes in the following tests, but not as often:

serviceability/jvmti/ModuleAwareAgents/ThreadStart/MAAThreadStart.java
vmTestbase/nsk/jdwp/VirtualMachine/Version/version002/TestDescription.java


vmTestbase/nsk/jdwp/VirtualMachine/ReleaseEvents/releaseevents002/TestDescription.java


vmTestbase/nsk/jdwp/VirtualMachine/HoldEvents/holdevents002/TestDescription.java


vmTestbase/nsk/jdwp/VirtualMachine/Dispose/dispose001/TestDescription.java



thanks,

Chris


On 3/25/20 11:37 AM, Roman Kennke wrote:

Hi Chris,


Regarding the new assert:

    105 if (gdata && gdata->assertOn) {
    106 // Check this is not already tagged.
    107 jlong tag;
    108 error = JVMTI_FUNC_PTR(trackingEnv, GetTag)(env,
klass, &tag);
    109 if (error != JVMTI_ERROR_NONE) {
    110 EXIT_ERROR(error, "Unable to GetTag with class
trackingEnv");
    111 }
    112 JDI_ASSERT(tag == NOT_TAGGED);
    113 }

I think you should remove the gdata check. gdata should never be
NULL
when you get to this code. If it is ever NULL then there's a bug,
and
the check will hide the bug.

Ok, will remove this.


Regarding testing, after you do the submit repo testing let me know
the
jobID and I'll do additional testing on it.

I did the submit repo earlier today, and it came back green:

mach5-one-rkennke-JDK-8227269-2-20200325-1421-9706762

Thanks,
Roman


thanks,

Chris

On 3/25/20 6:00 AM, Roman Kennke wrote:

Hi Sergei,


The fix looks pretty clean now.
I also like new name of the lock.:)

Thank you!


Just one comment below.

http://cr.openjdk.java.net/~rkennke/JDK-8227269/webrev.06/src/jdk.jdwp.agent/share/native/libjdwp/classTrack.c.frames.html




110 if (tag != 0l) {
111 return; // Already added
     112 }

 It is better to use a named constant or macro instead.
 Also, it'd be nice to add a short comment about this value is.

As I replied to Chris earlier, this whole block can be turned
into an
assert. I also made a constant for the value 0, which should be
pretty
much self-explaining.

http://cr.openjdk.java.net/~rkennke/JDK-8227269/webrev.07/


How do you test the fix?

I am using a manual test that is provided in this bug report:
https:

Re: RFR: 8240956: SEGV in DwarfParser::process_dwarf after JDK-8234624

[Kevin Walls]

Hi Yasumasa,

Oops, didn't catch this - I also had done some manual testing and in 
mach5 but clearly not enough.


Generally I think this looks good.

"lastFrame" can mean last as in final, or last as in previous. "last" is 
one of those annoying English words.  Here it means final, if we get an 
Exception during processDwarf, use this to flag that we should return 
null from sender().  "finalFrame" would be clearer to me, anything else 
probably gets more verbose than you wanted.


Yes I like having the limit on the while loop in process_dwarf(), always 
worried how sane the information is that we are parsing through.


Thanks!
Kevin


On 24/03/2020 23:47, Yasumasa Suenaga wrote:

Thanks Serguei!

I will push it when I get second reviewer.


Yasumasa


On 2020/03/25 1:39, serguei.spit...@oracle.com wrote:

Hi Yasumasa,

I'm okay with this update.
My mach5 test run for this patch is passed.

Thanks,
Serguei


On 3/23/20 17:08, Yasumasa Suenaga wrote:

Hi Serguei,

Thanks for your comment!
I uploaded new webrev:

  http://cr.openjdk.java.net/~ysuenaga/JDK-8240956/webrev.04/

Also I pushed it to submit repo:

  http://hg.openjdk.java.net/jdk/submit/rev/fade6a949bd1

On 2020/03/24 7:39, serguei.spit...@oracle.com wrote:

Hi Yasumasa,

The mach5 tier5 testing looks good.
The serviceability/sa/ClhsdbPstack.java is failed without fix and 
is not failed with it.


Thanks,
Serguei


On 3/23/20 10:18, serguei.spit...@oracle.com wrote:

Hi Yasumasa,

I looked at you changes.
It is hard to understand if this fully solves the issue.

http://cr.openjdk.java.net/~ysuenaga/JDK-8240956/webrev.03/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/debugger/linux/amd64/LinuxAMD64CFrame.java.frames.html 



@@ -34,10 +34,11 @@
   public static LinuxAMD64CFrame getTopFrame(LinuxDebugger 
dbg, Address rip, ThreadContext context) {

    Address libptr = dbg.findLibPtrByAddress(rip);
    Address cfa = 
context.getRegisterAsAddress(AMD64ThreadContext.RBP);

    DwarfParser dwarf = null;
+ boolean unsupportedDwarf = false;
      if (libptr != null) { // Native frame
  try {
    dwarf = new DwarfParser(libptr);
    dwarf.processDwarf(rip);
-- 


@@ -45,24 +46,33 @@
   !dwarf.isBPOffsetAvailable())
  ? 
context.getRegisterAsAddress(AMD64ThreadContext.RBP)
  : 
context.getRegisterAsAddress(dwarf.getCFARegister())

.addOffsetTo(dwarf.getCFAOffset());
  } catch (DebuggerException e) {
- // Bail out to Java frame case
+ if (dwarf != null) {
+ // DWARF processing should succeed when the frame is native
+ // but it might fail if CIE has language personality routine
+ // and/or LSDA.
+ dwarf = null;
+ unsupportedDwarf = true;
+ } else {
+ throw e;
+ }
  }
    }
      return (cfa == null) ? null
- : new LinuxAMD64CFrame(dbg, cfa, rip, dwarf);
+ : new LinuxAMD64CFrame(dbg, cfa, rip, dwarf, !unsupportedDwarf);
 }

@@ -121,13 +131,25 @@
   }
     return isValidFrame(nextCFA, context) ? nextCFA : null;
 }
  - private DwarfParser getNextDwarf(Address nextPC) {
- DwarfParser nextDwarf = null;
+ @Override
+ public CFrame sender(ThreadProxy thread) {
+ if (!possibleNext) {
+ return null;
+ }
+
+ ThreadContext context = thread.getContext();
+
+ Address nextPC = getNextPC(dwarf != null);
+ if (nextPC == null) {
+ return null;
+ }
  + DwarfParser nextDwarf = null;
+ boolean unsupportedDwarf = false;
   if ((dwarf != null) && dwarf.isIn(nextPC)) {
 nextDwarf = dwarf;
   } else {
 Address libptr = dbg.findLibPtrByAddress(nextPC);
 if (libptr != null) {

Re: RFR: 8240956: SEGV in DwarfParser::process_dwarf after JDK-8234624

[serguei.spit...@oracle.com]

Hi Kevin,

Nice catch with the name "lastFrame".
I was also confused when reviewed this but did not come up with 
something better.


Thanks,
Serguei

On 3/26/20 10:40, Kevin Walls wrote:

Hi Yasumasa,

Oops, didn't catch this - I also had done some manual testing and in 
mach5 but clearly not enough.


Generally I think this looks good.

"lastFrame" can mean last as in final, or last as in previous. "last" 
is one of those annoying English words.  Here it means final, if we 
get an Exception during processDwarf, use this to flag that we should 
return null from sender().  "finalFrame" would be clearer to me, 
anything else probably gets more verbose than you wanted.


Yes I like having the limit on the while loop in process_dwarf(), 
always worried how sane the information is that we are parsing through.


Thanks!
Kevin


On 24/03/2020 23:47, Yasumasa Suenaga wrote:

Thanks Serguei!

I will push it when I get second reviewer.


Yasumasa


On 2020/03/25 1:39, serguei.spit...@oracle.com wrote:

Hi Yasumasa,

I'm okay with this update.
My mach5 test run for this patch is passed.

Thanks,
Serguei


On 3/23/20 17:08, Yasumasa Suenaga wrote:

Hi Serguei,

Thanks for your comment!
I uploaded new webrev:

http://cr.openjdk.java.net/~ysuenaga/JDK-8240956/webrev.04/

Also I pushed it to submit repo:

  http://hg.openjdk.java.net/jdk/submit/rev/fade6a949bd1

On 2020/03/24 7:39, serguei.spit...@oracle.com wrote:

Hi Yasumasa,

The mach5 tier5 testing looks good.
The serviceability/sa/ClhsdbPstack.java is failed without fix and 
is not failed with it.


Thanks,
Serguei


On 3/23/20 10:18, serguei.spit...@oracle.com wrote:

Hi Yasumasa,

I looked at you changes.
It is hard to understand if this fully solves the issue.

http://cr.openjdk.java.net/~ysuenaga/JDK-8240956/webrev.03/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/debugger/linux/amd64/LinuxAMD64CFrame.java.frames.html 



@@ -34,10 +34,11 @@
   public static LinuxAMD64CFrame getTopFrame(LinuxDebugger 
dbg, Address rip, ThreadContext context) {

    Address libptr = dbg.findLibPtrByAddress(rip);
    Address cfa = 
context.getRegisterAsAddress(AMD64ThreadContext.RBP);

    DwarfParser dwarf = null;
+ boolean unsupportedDwarf = false;
      if (libptr != null) { // Native frame
  try {
    dwarf = new DwarfParser(libptr);
    dwarf.processDwarf(rip);
-- 


@@ -45,24 +46,33 @@
   !dwarf.isBPOffsetAvailable())
  ? 
context.getRegisterAsAddress(AMD64ThreadContext.RBP)
  : 
context.getRegisterAsAddress(dwarf.getCFARegister())

.addOffsetTo(dwarf.getCFAOffset());
  } catch (DebuggerException e) {
- // Bail out to Java frame case
+ if (dwarf != null) {
+ // DWARF processing should succeed when the frame is native
+ // but it might fail if CIE has language personality routine
+ // and/or LSDA.
+ dwarf = null;
+ unsupportedDwarf = true;
+ } else {
+ throw e;
+ }
  }
    }
      return (cfa == null) ? null
- : new LinuxAMD64CFrame(dbg, cfa, rip, dwarf);
+ : new LinuxAMD64CFrame(dbg, cfa, rip, dwarf, !unsupportedDwarf);
 }

@@ -121,13 +131,25 @@
   }
     return isValidFrame(nextCFA, context) ? nextCFA : null;
 }
  - private DwarfParser getNextDwarf(Address nextPC) {
- DwarfParser nextDwarf = null;
+ @Override
+ public CFrame sender(ThreadProxy thread) {
+ if (!possibleNext) {
+ return null;
+ }
+
+ ThreadContext context = thread.getContext();
+
+ Address nextPC = getNextPC(dwarf != null);
+ if (nextPC == null) {
+ return null;
+ }
  + DwarfParser nextDwarf = null;
+ boolean unsupportedDwarf = false;
   if ((dwarf != null) && dwarf.isIn(nextPC)) {
 nextDwarf = dwarf;
   } else {
 Address libptr = dbg.findLibPtrByAddress(nextPC);
 if (libptr != null) {
-

RFR(XS) 8241696: ProblemList gc/metaspace/CompressedClassSpaceSizeInJmapHeap.java due to JDK-8241293

[Chris Plummer]

Hello,

Please review the following:

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

diff --git a/test/hotspot/jtreg/ProblemList.txt 
b/test/hotspot/jtreg/ProblemList.txt

--- a/test/hotspot/jtreg/ProblemList.txt
+++ b/test/hotspot/jtreg/ProblemList.txt
@@ -85,7 +85,7 @@
 gc/stress/gclocker/TestGCLockerWithParallel.java 8180622 generic-all
 gc/stress/gclocker/TestGCLockerWithG1.java 8180622 generic-all
 gc/stress/TestJNIBlockFullGC/TestJNIBlockFullGC.java 8192647 generic-all
-gc/metaspace/CompressedClassSpaceSizeInJmapHeap.java 8193639 solaris-all
+gc/metaspace/CompressedClassSpaceSizeInJmapHeap.java 8193639,8241293 
solaris-all,macosx-x64


thanks,

Chris

Re: RFR(XS) 8241696: ProblemList gc/metaspace/CompressedClassSpaceSizeInJmapHeap.java due to JDK-8241293

[Christian Tornqvist]

Hi Chris,

Looks good, thanks for fixing this.

Thanks,
Christian

> On Mar 26, 2020, at 1:27 PM, Chris Plummer  wrote:
> 
> Hello,
> 
> Please review the following:
> 
> https://bugs.openjdk.java.net/browse/JDK-8241696
> 
> diff --git a/test/hotspot/jtreg/ProblemList.txt 
> b/test/hotspot/jtreg/ProblemList.txt
> --- a/test/hotspot/jtreg/ProblemList.txt
> +++ b/test/hotspot/jtreg/ProblemList.txt
> @@ -85,7 +85,7 @@
>  gc/stress/gclocker/TestGCLockerWithParallel.java 8180622 generic-all
>  gc/stress/gclocker/TestGCLockerWithG1.java 8180622 generic-all
>  gc/stress/TestJNIBlockFullGC/TestJNIBlockFullGC.java 8192647 generic-all
> -gc/metaspace/CompressedClassSpaceSizeInJmapHeap.java 8193639 solaris-all
> +gc/metaspace/CompressedClassSpaceSizeInJmapHeap.java 8193639,8241293 
> solaris-all,macosx-x64
> 
> thanks,
> 
> Chris
> 

Re: RFR(XS) 8241696: ProblemList gc/metaspace/CompressedClassSpaceSizeInJmapHeap.java due to JDK-8241293

[Daniel D. Daugherty]

Thumbs up. This is a trivial review, but you didn't qualify it as such
so now you have a second review.

Dan


On 3/26/20 4:41 PM, Christian Tornqvist wrote:

Hi Chris,

Looks good, thanks for fixing this.

Thanks,
Christian


On Mar 26, 2020, at 1:27 PM, Chris Plummer  wrote:

Hello,

Please review the following:

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

diff --git a/test/hotspot/jtreg/ProblemList.txt 
b/test/hotspot/jtreg/ProblemList.txt
--- a/test/hotspot/jtreg/ProblemList.txt
+++ b/test/hotspot/jtreg/ProblemList.txt
@@ -85,7 +85,7 @@
  gc/stress/gclocker/TestGCLockerWithParallel.java 8180622 generic-all
  gc/stress/gclocker/TestGCLockerWithG1.java 8180622 generic-all
  gc/stress/TestJNIBlockFullGC/TestJNIBlockFullGC.java 8192647 generic-all
-gc/metaspace/CompressedClassSpaceSizeInJmapHeap.java 8193639 solaris-all
+gc/metaspace/CompressedClassSpaceSizeInJmapHeap.java 8193639,8241293 
solaris-all,macosx-x64

thanks,

Chris

RFR: 8241456: ThreadRunner shouldn't use Wicket for threads starting synchronization

[Leonid Mesnik]

Replying with correct summary.

Leonid

On 3/23/20 8:55 PM, Leonid Mesnik wrote:

Hi

Could you please review following fix which update ThreadsRunner to use 
AtomicInteger/spinOnWait instead of Wicket to synchronize starting of stress 
test threads.

Failing tests allocated all memory by earlier started threads before 
Lock.unlock is called in the latest threads. So thread might get an OOME 
exception while trying to release lock and/or get into inconsistent state.

The bug was introduced by https://bugs.openjdk.java.net/browse/JDK-8241123 

The Atomic works fine for stress test finishing sync. I just didn't expect that 
tests might OOME while releasing start lock.
Verified that tests now don't fail with -Xcomp -server -XX:-TieredCompilation 
-XX:-UseCompressedOops.

webrev: http://cr.openjdk.java.net/~lmesnik/8241456/webrev.00/ 

bug: https://bugs.openjdk.java.net/browse/JDK-8241456 


Leonid

Re: RFR(XS) 8241696: ProblemList gc/metaspace/CompressedClassSpaceSizeInJmapHeap.java due to JDK-8241293

[Chris Plummer]

Thanks!

On 3/26/20 1:55 PM, Daniel D. Daugherty wrote:

Thumbs up. This is a trivial review, but you didn't qualify it as such
so now you have a second review.

Dan


On 3/26/20 4:41 PM, Christian Tornqvist wrote:

Hi Chris,

Looks good, thanks for fixing this.

Thanks,
Christian

On Mar 26, 2020, at 1:27 PM, Chris Plummer 
 wrote:


Hello,

Please review the following:

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

diff --git a/test/hotspot/jtreg/ProblemList.txt 
b/test/hotspot/jtreg/ProblemList.txt

--- a/test/hotspot/jtreg/ProblemList.txt
+++ b/test/hotspot/jtreg/ProblemList.txt
@@ -85,7 +85,7 @@
  gc/stress/gclocker/TestGCLockerWithParallel.java 8180622 generic-all
  gc/stress/gclocker/TestGCLockerWithG1.java 8180622 generic-all
  gc/stress/TestJNIBlockFullGC/TestJNIBlockFullGC.java 8192647 
generic-all
-gc/metaspace/CompressedClassSpaceSizeInJmapHeap.java 8193639 
solaris-all
+gc/metaspace/CompressedClassSpaceSizeInJmapHeap.java 
8193639,8241293 solaris-all,macosx-x64


thanks,

Chris

Re: RFR: 8241456: ThreadRunner shouldn't use Wicket for threads starting synchronization

[David Holmes]

Hi Leonid,

On 27/03/2020 7:39 am, Leonid Mesnik wrote:

Replying with correct summary.

Leonid

On 3/23/20 8:55 PM, Leonid Mesnik wrote:

Hi

Could you please review following fix which update ThreadsRunner to 
use AtomicInteger/spinOnWait instead of Wicket to synchronize starting 
of stress test threads.


Failing tests allocated all memory by earlier started threads before 
Lock.unlock is called in the latest threads. So thread might get an 
OOME exception while trying to release lock and/or get into 
inconsistent state.


You have a bug in Wicket:

+try {
+lock.lock();
...
+} finally {
+lock.unlock();

The lock() has to go outside the try block. That is why you were getting 
IllegalMonitorStateExceptions when the lock() threw OOME.


But the OOME itself is still a problem as it means you can't use any 
proper synchronizer. I don't like seeing the spin-loops but in this code 
you may have no choice if memory may already be exhausted.


David
-




The bug was introduced by 
https://bugs.openjdk.java.net/browse/JDK-8241123 

The Atomic works fine for stress test finishing sync. I just didn't 
expect that tests might OOME while releasing start lock.
Verified that tests now don't fail with -Xcomp -server 
-XX:-TieredCompilation -XX:-UseCompressedOops.


webrev: http://cr.openjdk.java.net/~lmesnik/8241456/webrev.00/ 

bug: https://bugs.openjdk.java.net/browse/JDK-8241456 



Leonid

Re: RFR: 8241456: ThreadRunner shouldn't use Wicket for threads starting synchronization

[Leonid Mesnik]

On 3/26/20 4:06 PM, David Holmes wrote:

Hi Leonid,

On 27/03/2020 7:39 am, Leonid Mesnik wrote:

Replying with correct summary.

Leonid

On 3/23/20 8:55 PM, Leonid Mesnik wrote:

Hi

Could you please review following fix which update ThreadsRunner to 
use AtomicInteger/spinOnWait instead of Wicket to synchronize 
starting of stress test threads.


Failing tests allocated all memory by earlier started threads before 
Lock.unlock is called in the latest threads. So thread might get an 
OOME exception while trying to release lock and/or get into 
inconsistent state.


You have a bug in Wicket:

+    try {
+    lock.lock();
...
+    } finally {
+    lock.unlock();

The lock() has to go outside the try block. That is why you were 
getting IllegalMonitorStateExceptions when the lock() threw OOME.
Thanks for explanation. But anyway, as I understand locks use memory and 
might be inconsistent if OOME happened.


But the OOME itself is still a problem as it means you can't use any 
proper synchronizer. I don't like seeing the spin-loops but in this 
code you may have no choice if memory may already be exhausted.


It should be really short spin-loop, test only start thread during this 
loop and don't do anything more. Also, it is done only once for all 
stress test. The goal is to start thread completely before heap is 
exhausted.


Leonid



David
-




The bug was introduced by 
https://bugs.openjdk.java.net/browse/JDK-8241123 

The Atomic works fine for stress test finishing sync. I just didn't 
expect that tests might OOME while releasing start lock.
Verified that tests now don't fail with -Xcomp -server 
-XX:-TieredCompilation -XX:-UseCompressedOops.


webrev: http://cr.openjdk.java.net/~lmesnik/8241456/webrev.00/ 

bug: https://bugs.openjdk.java.net/browse/JDK-8241456 



Leonid

Re: RFR: 8241456: ThreadRunner shouldn't use Wicket for threads starting synchronization

[David Holmes]

On 27/03/2020 9:16 am, Leonid Mesnik wrote:


On 3/26/20 4:06 PM, David Holmes wrote:

Hi Leonid,

On 27/03/2020 7:39 am, Leonid Mesnik wrote:

Replying with correct summary.

Leonid

On 3/23/20 8:55 PM, Leonid Mesnik wrote:

Hi

Could you please review following fix which update ThreadsRunner to 
use AtomicInteger/spinOnWait instead of Wicket to synchronize 
starting of stress test threads.


Failing tests allocated all memory by earlier started threads before 
Lock.unlock is called in the latest threads. So thread might get an 
OOME exception while trying to release lock and/or get into 
inconsistent state.


You have a bug in Wicket:

+    try {
+    lock.lock();
...
+    } finally {
+    lock.unlock();

The lock() has to go outside the try block. That is why you were 
getting IllegalMonitorStateExceptions when the lock() threw OOME.
Thanks for explanation. But anyway, as I understand locks use memory and 
might be inconsistent if OOME happened.


They use memory and so lock() can throw OOME, but they are never 
inconsistent.




But the OOME itself is still a problem as it means you can't use any 
proper synchronizer. I don't like seeing the spin-loops but in this 
code you may have no choice if memory may already be exhausted.


It should be really short spin-loop, test only start thread during this 
loop and don't do anything more. Also, it is done only once for all 
stress test. The goal is to start thread completely before heap is 
exhausted.


Okay. I'm somewhat dubious about making these changes in mainline now 
just to support loom. I don't see why we need to care about pinning 
threads in this kind of situation.


David


Leonid



David
-




The bug was introduced by 
https://bugs.openjdk.java.net/browse/JDK-8241123 

The Atomic works fine for stress test finishing sync. I just didn't 
expect that tests might OOME while releasing start lock.
Verified that tests now don't fail with -Xcomp -server 
-XX:-TieredCompilation -XX:-UseCompressedOops.


webrev: http://cr.openjdk.java.net/~lmesnik/8241456/webrev.00/ 

bug: https://bugs.openjdk.java.net/browse/JDK-8241456 



Leonid

Re: RFR: 8241585: Remove unused _recursion_counter facility from PerfTraceTime

[David Holmes]

Hi Claes,

Adding serviceability as they are the consumers of this IIUC.

On 27/03/2020 3:40 am, Claes Redestad wrote:

Hi,

PerfTraceTime::_recursion_counter is unused, and removing it
gets rid of some branchy (but well-predicted) code in paths that is
somewhat startup sensitive.


Okay.


http://cr.openjdk.java.net/~redestad/8241585/open.00/

Also added some trace logging to determine the number of perf
data counter or each type along with a tune-up to exactly match
the defaults.


Okay so can you change the bug synopsis and description to cover this 
more general cleanup and tuneup please.


I'm never very clear on the uses of these PerfCounters. It seems SUN_NS 
is unused after this change. The references to jvmstat seem no longer 
correct - these are read via jstat ?



Testing: tier1+2


I think serviceability testing is mainly in tier3.

Thanks,
David
-



Thanks!

/Claes

Re: RFR: 8241456: ThreadRunner shouldn't use Wicket for threads starting synchronization

[Leonid Mesnik]

On 3/26/20 4:29 PM, David Holmes wrote:

On 27/03/2020 9:16 am, Leonid Mesnik wrote:


On 3/26/20 4:06 PM, David Holmes wrote:

Hi Leonid,

On 27/03/2020 7:39 am, Leonid Mesnik wrote:

Replying with correct summary.

Leonid

On 3/23/20 8:55 PM, Leonid Mesnik wrote:

Hi

Could you please review following fix which update ThreadsRunner 
to use AtomicInteger/spinOnWait instead of Wicket to synchronize 
starting of stress test threads.


Failing tests allocated all memory by earlier started threads 
before Lock.unlock is called in the latest threads. So thread 
might get an OOME exception while trying to release lock and/or 
get into inconsistent state.


You have a bug in Wicket:

+    try {
+    lock.lock();
...
+    } finally {
+    lock.unlock();

The lock() has to go outside the try block. That is why you were 
getting IllegalMonitorStateExceptions when the lock() threw OOME.
Thanks for explanation. But anyway, as I understand locks use memory 
and might be inconsistent if OOME happened.


They use memory and so lock() can throw OOME, but they are never 
inconsistent.

Ok, I will move lock.lock() outside of try {}. Thanks for explanation.




But the OOME itself is still a problem as it means you can't use any 
proper synchronizer. I don't like seeing the spin-loops but in this 
code you may have no choice if memory may already be exhausted.


It should be really short spin-loop, test only start thread during 
this loop and don't do anything more. Also, it is done only once for 
all stress test. The goal is to start thread completely before heap 
is exhausted.


Okay. I'm somewhat dubious about making these changes in mainline now 
just to support loom. I don't see why we need to care about pinning 
threads in this kind of situation.


The idea is to add some nsk/share stress tests for virtual threads. 
Basically, there are the same tests as existing (gc, sysdict) but 
running in virtual threads. And these tests are going to be executed 
after loom is integrated. And I want to keep the difference as small as 
possible between mainline and loom.


Leonid



David


Leonid



David
-




The bug was introduced by 
https://bugs.openjdk.java.net/browse/JDK-8241123 

The Atomic works fine for stress test finishing sync. I just 
didn't expect that tests might OOME while releasing start lock.
Verified that tests now don't fail with -Xcomp -server 
-XX:-TieredCompilation -XX:-UseCompressedOops.


webrev: http://cr.openjdk.java.net/~lmesnik/8241456/webrev.00/ 

bug: https://bugs.openjdk.java.net/browse/JDK-8241456 



Leonid

Re: RFR: 8241585: Remove unused _recursion_counter facility from PerfTraceTime

[Chris Plummer]

On 3/26/20 4:36 PM, David Holmes wrote:

Hi Claes,

Adding serviceability as they are the consumers of this IIUC.

On 27/03/2020 3:40 am, Claes Redestad wrote:

Hi,

PerfTraceTime::_recursion_counter is unused, and removing it
gets rid of some branchy (but well-predicted) code in paths that is
somewhat startup sensitive.


Okay.


http://cr.openjdk.java.net/~redestad/8241585/open.00/

Also added some trace logging to determine the number of perf
data counter or each type along with a tune-up to exactly match
the defaults.


Okay so can you change the bug synopsis and description to cover this 
more general cleanup and tuneup please.


I'm never very clear on the uses of these PerfCounters. It seems 
SUN_NS is unused after this change. The references to jvmstat seem no 
longer correct - these are read via jstat ?

jstat uses jvmstat.

Chris



Testing: tier1+2


I think serviceability testing is mainly in tier3.

Thanks,
David
-



Thanks!

/Claes

Re: RFR: 8241585: Remove unused _recursion_counter facility from PerfTraceTime

[David Holmes]

On 27/03/2020 9:46 am, Chris Plummer wrote:

On 3/26/20 4:36 PM, David Holmes wrote:

Hi Claes,

Adding serviceability as they are the consumers of this IIUC.

On 27/03/2020 3:40 am, Claes Redestad wrote:

Hi,

PerfTraceTime::_recursion_counter is unused, and removing it
gets rid of some branchy (but well-predicted) code in paths that is
somewhat startup sensitive.


Okay.


http://cr.openjdk.java.net/~redestad/8241585/open.00/

Also added some trace logging to determine the number of perf
data counter or each type along with a tune-up to exactly match
the defaults.


Okay so can you change the bug synopsis and description to cover this 
more general cleanup and tuneup please.


I'm never very clear on the uses of these PerfCounters. It seems 
SUN_NS is unused after this change. The references to jvmstat seem no 
longer correct - these are read via jstat ?

jstat uses jvmstat.


Thanks Chris, I was grepping C++ code not realizing jvmstat is a Java API.

David


Chris



Testing: tier1+2


I think serviceability testing is mainly in tier3.

Thanks,
David
-



Thanks!

/Claes

Review Request: 8238358: Implementation of JEP 371: Hidden Classes

[Mandy Chung]

Please review the implementation of JEP 371: Hidden Classes. The main 
changes are in core-libs and hotspot runtime area.  Small changes are 
made in javac, VM compiler (intrinsification of Class::isHiddenClass), 
JFR, JDI, and jcmd.  CSR [1]has been reviewed and is in the finalized 
state (see specdiff and javadoc below for reference).


Webrev:
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.03

Hidden class is created via `Lookup::defineHiddenClass`. From JVM's point
of view, a hidden class is a normal class except the following:

- A hidden class has no initiating class loader and is not registered in 
any dictionary.
- A hidden class has a name containing an illegal character 
`Class::getName` returns `p.Foo/0x1234` whereas `GetClassSignature` 
returns "Lp/Foo.0x1234;".
- A hidden class is not modifiable, i.e. cannot be redefined or 
retransformed. JVM TI IsModifableClass returns false on a hidden.
- Final fields in a hidden class is "final".  The value of final fields 
cannot be overriden via reflection.  setAccessible(true) can still be 
called on reflected objects representing final fields in a hidden class 
and its access check will be suppressed but only have read-access (i.e. 
can do Field::getXXX but not setXXX).


Brief summary of this patch:

1. A new Lookup::defineHiddenClass method is the API to create a hidden 
class.
2. A new Lookup.ClassOption enum class defines NESTMATE and STRONG 
option that

   can be specified when creating a hidden class.
3. A new Class::isHiddenClass method tests if a class is a hidden class.
4. Field::setXXX method will throw IAE on a final field of a hidden class
   regardless of the value of the accessible flag.
5. JVM_LookupDefineClass is the new JVM entry point for Lookup::defineClass
   and defineHiddenClass to create a class from the given bytes.
6. ClassLoaderData implementation is not changed.  There is one primary CLD
   that holds the classes strongly referenced by its defining loader.  
There

   can be zero or more additional CLDs - one per weak class.
7. Nest host determination is updated per revised JVMS 5.4.4. Access control
   check no longer throws LinkageError but instead it will throw IAE with
   a clear message if a class fails to resolve/validate the nest host 
declared

   in NestHost/NestMembers attribute.
8. JFR, jcmd, JDI are updated to support hidden classes.
9. update javac LambdaToMethod as lambda proxy starts using nestmates
   and generate a bridge method to desuger a method reference to a 
protected

   method in its supertype in a different package

This patch also updates StringConcatFactory, LambdaMetaFactory, and 
LambdaForms
to use hidden classes.  The webrev includes changes in nashorn to hidden 
class

and I will update the webrev if JEP 372 removes it any time soon.

We uncovered a bug in Lookup::defineClass spec throws LinkageError and 
intends

to have the newly created class linked.  However, the implementation in 14
does not link the class.  A separate CSR [2] proposes to update the
implementation to match the spec.  This patch fixes the implementation.

The spec update on JVM TI, JDI and Instrumentation will be done as
a separate RFE [3].  This patch includes new tests for JVM TI and
java.instrument that validates how the existing APIs work for hidden 
classes.


javadoc/specdiff
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/api/
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/specdiff/

JVMS 5.4.4 change:
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/Draft-JVMS-HiddenClasses.pdf

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

Thanks
Mandy
[1] https://bugs.openjdk.java.net/browse/JDK-8238359
[2] https://bugs.openjdk.java.net/browse/JDK-8240338
[3] https://bugs.openjdk.java.net/browse/JDK-8230502

Re: RFR: 8240956: SEGV in DwarfParser::process_dwarf after JDK-8234624

[Yasumasa Suenaga]

Thanks Kevin and Serguei! and sorry for my English...

I uploaded new webrev:

  http://cr.openjdk.java.net/~ysuenaga/JDK-8240956/webrev.05/

Diff from webrev.04 is here:

  http://hg.openjdk.java.net/jdk/submit/rev/d5f400d70e94


Thanks,

Yasumasa


On 2020/03/27 2:53, serguei.spit...@oracle.com wrote:

Hi Kevin,

Nice catch with the name "lastFrame".
I was also confused when reviewed this but did not come up with something 
better.

Thanks,
Serguei

On 3/26/20 10:40, Kevin Walls wrote:

Hi Yasumasa,

Oops, didn't catch this - I also had done some manual testing and in mach5 but 
clearly not enough.

Generally I think this looks good.

"lastFrame" can mean last as in final, or last as in previous. "last" is one of those 
annoying English words.  Here it means final, if we get an Exception during processDwarf, use this to flag 
that we should return null from sender().  "finalFrame" would be clearer to me, anything else 
probably gets more verbose than you wanted.

Yes I like having the limit on the while loop in process_dwarf(), always 
worried how sane the information is that we are parsing through.

Thanks!
Kevin


On 24/03/2020 23:47, Yasumasa Suenaga wrote:

Thanks Serguei!

I will push it when I get second reviewer.


Yasumasa


On 2020/03/25 1:39, serguei.spit...@oracle.com wrote:

Hi Yasumasa,

I'm okay with this update.
My mach5 test run for this patch is passed.

Thanks,
Serguei


On 3/23/20 17:08, Yasumasa Suenaga wrote:

Hi Serguei,

Thanks for your comment!
I uploaded new webrev:

http://cr.openjdk.java.net/~ysuenaga/JDK-8240956/webrev.04/

Also I pushed it to submit repo:

  http://hg.openjdk.java.net/jdk/submit/rev/fade6a949bd1

On 2020/03/24 7:39, serguei.spit...@oracle.com wrote:

Hi Yasumasa,

The mach5 tier5 testing looks good.
The serviceability/sa/ClhsdbPstack.java is failed without fix and is not failed 
with it.

Thanks,
Serguei


On 3/23/20 10:18, serguei.spit...@oracle.com wrote:

Hi Yasumasa,

I looked at you changes.
It is hard to understand if this fully solves the issue.

http://cr.openjdk.java.net/~ysuenaga/JDK-8240956/webrev.03/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/debugger/linux/amd64/LinuxAMD64CFrame.java.frames.html

@@ -34,10 +34,11 @@
   public static LinuxAMD64CFrame getTopFrame(LinuxDebugger dbg, Address 
rip, ThreadContext context) {
    Address libptr = dbg.findLibPtrByAddress(rip);
    Address cfa = context.getRegisterAsAddress(AMD64ThreadContext.RBP);
    DwarfParser dwarf = null;
+ boolean unsupportedDwarf = false;
      if (libptr != null) { // Native frame
  try {
    dwarf = new DwarfParser(libptr);
    dwarf.processDwarf(rip);
-- 


@@ -45,24 +46,33 @@
   !dwarf.isBPOffsetAvailable())
  ? context.getRegisterAsAddress(AMD64ThreadContext.RBP)
  : context.getRegisterAsAddress(dwarf.getCFARegister())
.addOffsetTo(dwarf.getCFAOffset());
  } catch (DebuggerException e) {
- // Bail out to Java frame case
+ if (dwarf != null) {
+ // DWARF processing should succeed when the frame is native
+ // but it might fail if CIE has language personality routine
+ // and/or LSDA.
+ dwarf = null;
+ unsupportedDwarf = true;
+ } else {
+ throw e;
+ }
  }
    }
      return (cfa == null) ? null
- : new LinuxAMD64CFrame(dbg, cfa, rip, dwarf);
+ : new LinuxAMD64CFrame(dbg, cfa, rip, dwarf, !unsupportedDwarf);
 }

@@ -121,13 +131,25 @@
   }
     return isValidFrame(nextCFA, context) ? nextCFA : null;
 }
  - private DwarfParser getNextDwarf(Address nextPC) {
- DwarfParser nextDwarf = null;
+ @Override
+ public CFrame sender(ThreadProxy thread) {
+ if (!possibleNext) {
+ return null;
+ }
+
+ ThreadContext context = thread.getContext();
+
+ Address nextPC = getNextPC(dwarf != null);
+ if (nextPC == null) {
+ return null;
+ }
  + DwarfParser nextDwarf = null;
+ boolean unsupportedDwarf = false;
   if ((dwarf != null) && dwarf.isIn(nextPC)) {
 nextDwarf = dwa

Re: RFR: 8241585: Remove unused _recursion_counter facility from PerfTraceTime

[Claes Redestad]

On 2020-03-27 00:36, David Holmes wrote:




Okay so can you change the bug synopsis and description to cover this 
more general cleanup and tuneup please.


I filed an addendum RFE and will add this RFE bug id to the single
changeset push:
https://bugs.openjdk.java.net/browse/JDK-8241705



I'm never very clear on the uses of these PerfCounters. It seems SUN_NS 
is unused after this change. The references to jvmstat seem no longer 
correct - these are read via jstat ?


The general confusion about PerfData/-Counters and what they're for is
why I'm trying to untangle this. Generally I think we should pull the
plug on it, but the perfdata shared file is tangled up with
functionality to detect running JVMs used by jcmd etc, so it might
take a few iterations to get there.

/Claes

Re: RFR: 8227269: Slow class loading when running JVM in debug mode

[serguei.spit...@oracle.com]

Hi Roman,

Yes. Thank you for the explanation.

Thanks,
Serguei

On 3/26/20 01:44, Roman Kennke wrote:

That was in the previous implementation: I got a condition wrong in the
table lookup (as noted by Serguei), and this prevented any
class-unload-events from getting out. I have fixed this, but found other
problems in that implementation (deadlocks and a crash).

  The current implementation has none of these problems: we don't need
table-lookups - we simply pass-through the signatures, and locking is
much simpler and in particular we don't need a lock around the JVMTI
call (SetTag) which was the cause of the deadlock.

Does that answer your questions?

Thanks,
Roman


Hi Roman,

It passed all my testing. I think before you push Serguei has a question
regarding an issue you brought up a while back. You mentioned that you
weren't getting some events, and suddenly started seeing them. We were
discussing it today and it was unclear if this was an issue you were
seeing before your changes, and your changes resolved it, or it was
initially caused by an earlier version of your changes, and you later
fixed it. We just want to better understand what this issue was and how
it was fixed.

thanks,

Chris

On 3/25/20 3:22 PM, Roman Kennke wrote:

The new job finished, its ID is:

   mach5-one-rkennke-JDK-8227269-2-20200325-2027-9716289

Thank you,
Roman



Yes, please submit a new job. I'll start my testing once I see that the
builds are done.

Chris

On 3/25/20 12:59 PM, Roman Kennke wrote:

Hi Chris,

Apparently we can get into classTrack_reset() before calling
activate(),
and we're seeing a null deletedSignatureBag. A simple NULL-check around
the cleaning routine fixes the problem for me.

http://cr.openjdk.java.net/~rkennke/JDK-8227269/webrev.08/

Should I post another submit-repo job with that fix?

Thanks,
Roman



Hi Roman,

com/sun/jdi/JdwpAllowTest.java crashed on many runs:

Stack: [0x7fbb790f9000,0x7fbb791fa000],
sp=0x7fbb791f8af0,  free space=1022k
Native frames: (J=compiled Java code, A=aot compiled Java code,
j=interpreted, Vv=VM code, C=native code)
C  [libjdwp.so+0xdb71]  bagEnumerateOver+0x11
C  [libjdwp.so+0xe365]  classTrack_reset+0x25
C  [libjdwp.so+0xfca1]  debugInit_reset+0x71
C  [libjdwp.so+0x12e0d]  debugLoop_run+0x38d
C  [libjdwp.so+0x25700]  acceptThread+0x80
V  [libjvm.so+0xf4b5a7]  JvmtiAgentThread::call_start_function()+0x1c7
V  [libjvm.so+0x15215c6]  JavaThread::thread_main_inner()+0x226
V  [libjvm.so+0x1527736]  Thread::call_run()+0xf6
V  [libjvm.so+0x1250ade]  thread_native_entry(Thread*)+0x10e


This happened during a test task run of open/test/jdk/:jdk_jdi. There
doesn't seem to be anything magic on the command line that might be
triggering. Pretty much I see it with all the various VM configs we
test.

I'm also seeing crashes in the following tests, but not as often:

serviceability/jvmti/ModuleAwareAgents/ThreadStart/MAAThreadStart.java
vmTestbase/nsk/jdwp/VirtualMachine/Version/version002/TestDescription.java


vmTestbase/nsk/jdwp/VirtualMachine/ReleaseEvents/releaseevents002/TestDescription.java


vmTestbase/nsk/jdwp/VirtualMachine/HoldEvents/holdevents002/TestDescription.java


vmTestbase/nsk/jdwp/VirtualMachine/Dispose/dispose001/TestDescription.java



thanks,

Chris


On 3/25/20 11:37 AM, Roman Kennke wrote:

Hi Chris,


Regarding the new assert:

    105 if (gdata && gdata->assertOn) {
    106 // Check this is not already tagged.
    107 jlong tag;
    108 error = JVMTI_FUNC_PTR(trackingEnv, GetTag)(env,
klass, &tag);
    109 if (error != JVMTI_ERROR_NONE) {
    110 EXIT_ERROR(error, "Unable to GetTag with class
trackingEnv");
    111 }
    112 JDI_ASSERT(tag == NOT_TAGGED);
    113 }

I think you should remove the gdata check. gdata should never be
NULL
when you get to this code. If it is ever NULL then there's a bug,
and
the check will hide the bug.

Ok, will remove this.


Regarding testing, after you do the submit repo testing let me know
the
jobID and I'll do additional testing on it.

I did the submit repo earlier today, and it came back green:

mach5-one-rkennke-JDK-8227269-2-20200325-1421-9706762

Thanks,
Roman


thanks,

Chris

On 3/25/20 6:00 AM, Roman Kennke wrote:

Hi Sergei,


The fix looks pretty clean now.
I also like new name of the lock.:)

Thank you!


Just one comment below.

http://cr.openjdk.java.net/~rkennke/JDK-8227269/webrev.06/src/jdk.jdwp.agent/share/native/libjdwp/classTrack.c.frames.html




110 if (tag != 0l) {
111 return; // Already added
     112 }

 It is better to use a named constant or macro instead.
 Also, it'd be nice to add a short comment about this value is.

As I replied to Chris earlier, this whole block can be turned
into an
assert. I also made a constant for the value 0, which should be
pretty
much self-explaining.

http://cr.openjdk.java.net/~rkennke/JDK-8227269/webrev.07/


How do you test the fix?

I am using a manual test that is provide

Re: RFR: 8227269: Slow class loading when running JVM in debug mode

[serguei.spit...@oracle.com]

Hi Roman,

I'm okay with fix.

Thanks,
Serguei


On 3/26/20 17:15, serguei.spit...@oracle.com wrote:

Hi Roman,

Yes. Thank you for the explanation.

Thanks,
Serguei

On 3/26/20 01:44, Roman Kennke wrote:

That was in the previous implementation: I got a condition wrong in the
table lookup (as noted by Serguei), and this prevented any
class-unload-events from getting out. I have fixed this, but found other
problems in that implementation (deadlocks and a crash).

  The current implementation has none of these problems: we don't need
table-lookups - we simply pass-through the signatures, and locking is
much simpler and in particular we don't need a lock around the JVMTI
call (SetTag) which was the cause of the deadlock.

Does that answer your questions?

Thanks,
Roman


Hi Roman,

It passed all my testing. I think before you push Serguei has a 
question

regarding an issue you brought up a while back. You mentioned that you
weren't getting some events, and suddenly started seeing them. We were
discussing it today and it was unclear if this was an issue you were
seeing before your changes, and your changes resolved it, or it was
initially caused by an earlier version of your changes, and you later
fixed it. We just want to better understand what this issue was and how
it was fixed.

thanks,

Chris

On 3/25/20 3:22 PM, Roman Kennke wrote:

The new job finished, its ID is:

   mach5-one-rkennke-JDK-8227269-2-20200325-2027-9716289

Thank you,
Roman


Yes, please submit a new job. I'll start my testing once I see 
that the

builds are done.

Chris

On 3/25/20 12:59 PM, Roman Kennke wrote:

Hi Chris,

Apparently we can get into classTrack_reset() before calling
activate(),
and we're seeing a null deletedSignatureBag. A simple NULL-check 
around

the cleaning routine fixes the problem for me.

http://cr.openjdk.java.net/~rkennke/JDK-8227269/webrev.08/

Should I post another submit-repo job with that fix?

Thanks,
Roman



Hi Roman,

com/sun/jdi/JdwpAllowTest.java crashed on many runs:

Stack: [0x7fbb790f9000,0x7fbb791fa000],
sp=0x7fbb791f8af0,  free space=1022k
Native frames: (J=compiled Java code, A=aot compiled Java code,
j=interpreted, Vv=VM code, C=native code)
C  [libjdwp.so+0xdb71]  bagEnumerateOver+0x11
C  [libjdwp.so+0xe365]  classTrack_reset+0x25
C  [libjdwp.so+0xfca1]  debugInit_reset+0x71
C  [libjdwp.so+0x12e0d]  debugLoop_run+0x38d
C  [libjdwp.so+0x25700]  acceptThread+0x80
V  [libjvm.so+0xf4b5a7] 
JvmtiAgentThread::call_start_function()+0x1c7

V  [libjvm.so+0x15215c6] JavaThread::thread_main_inner()+0x226
V  [libjvm.so+0x1527736]  Thread::call_run()+0xf6
V  [libjvm.so+0x1250ade] thread_native_entry(Thread*)+0x10e


This happened during a test task run of open/test/jdk/:jdk_jdi. 
There

doesn't seem to be anything magic on the command line that might be
triggering. Pretty much I see it with all the various VM configs we
test.

I'm also seeing crashes in the following tests, but not as often:

serviceability/jvmti/ModuleAwareAgents/ThreadStart/MAAThreadStart.java 

vmTestbase/nsk/jdwp/VirtualMachine/Version/version002/TestDescription.java 




vmTestbase/nsk/jdwp/VirtualMachine/ReleaseEvents/releaseevents002/TestDescription.java 




vmTestbase/nsk/jdwp/VirtualMachine/HoldEvents/holdevents002/TestDescription.java 




vmTestbase/nsk/jdwp/VirtualMachine/Dispose/dispose001/TestDescription.java 





thanks,

Chris


On 3/25/20 11:37 AM, Roman Kennke wrote:

Hi Chris,


Regarding the new assert:

105 if (gdata && gdata->assertOn) {
106 // Check this is not already tagged.
107 jlong tag;
108 error = JVMTI_FUNC_PTR(trackingEnv, GetTag)(env,
klass, &tag);
109 if (error != JVMTI_ERROR_NONE) {
110 EXIT_ERROR(error, "Unable to GetTag with 
class

trackingEnv");
111 }
112 JDI_ASSERT(tag == NOT_TAGGED);
113 }

I think you should remove the gdata check. gdata should never be
NULL
when you get to this code. If it is ever NULL then there's a bug,
and
the check will hide the bug.

Ok, will remove this.

Regarding testing, after you do the submit repo testing let me 
know

the
jobID and I'll do additional testing on it.

I did the submit repo earlier today, and it came back green:

mach5-one-rkennke-JDK-8227269-2-20200325-1421-9706762

Thanks,
Roman


thanks,

Chris

On 3/25/20 6:00 AM, Roman Kennke wrote:

Hi Sergei,


The fix looks pretty clean now.
I also like new name of the lock.:)

Thank you!


Just one comment below.

http://cr.openjdk.java.net/~rkennke/JDK-8227269/webrev.06/src/jdk.jdwp.agent/share/native/libjdwp/classTrack.c.frames.html 






110 if (tag != 0l) {
111 return; // Already added
 112 }

 It is better to use a named constant or macro instead.
 Also, it'd be nice to add a short comment about this 
value is.

As I replied to Chris earlier, this whole block can be turned
into an
assert. I also made a constant for the value 0, which should be
pretty
much self-explaining.

Re: RFR: 8241585: Remove unused _recursion_counter facility from PerfTraceTime

[David Holmes]

Hi Claes,

On 27/03/2020 10:11 am, Claes Redestad wrote:



On 2020-03-27 00:36, David Holmes wrote:




Okay so can you change the bug synopsis and description to cover this 
more general cleanup and tuneup please.


I filed an addendum RFE and will add this RFE bug id to the single
changeset push:
https://bugs.openjdk.java.net/browse/JDK-8241705


That works too :) Thanks.



I'm never very clear on the uses of these PerfCounters. It seems 
SUN_NS is unused after this change. The references to jvmstat seem no 
longer correct - these are read via jstat ?


The general confusion about PerfData/-Counters and what they're for is
why I'm trying to untangle this. Generally I think we should pull the
plug on it, but the perfdata shared file is tangled up with
functionality to detect running JVMs used by jcmd etc, so it might
take a few iterations to get there.


Yeah they confuse me. Which makes it hard to see what impact your 
changes may have.


Hopefully serviceability folk are more familiar with how things hook 
together.


Thanks,
David


/Claes

Re: RFR: 8240956: SEGV in DwarfParser::process_dwarf after JDK-8234624

[Yasumasa Suenaga]

All tests on submit repo has been passed. 
(mach5-one-ysuenaga-JDK-8240956-3-20200327-0003-9753265)

Yasumasa

On 2020/03/27 9:07, Yasumasa Suenaga wrote:

Thanks Kevin and Serguei! and sorry for my English...

I uploaded new webrev:

   http://cr.openjdk.java.net/~ysuenaga/JDK-8240956/webrev.05/

Diff from webrev.04 is here:

   http://hg.openjdk.java.net/jdk/submit/rev/d5f400d70e94


Thanks,

Yasumasa


On 2020/03/27 2:53, serguei.spit...@oracle.com wrote:

Hi Kevin,

Nice catch with the name "lastFrame".
I was also confused when reviewed this but did not come up with something 
better.

Thanks,
Serguei

On 3/26/20 10:40, Kevin Walls wrote:

Hi Yasumasa,

Oops, didn't catch this - I also had done some manual testing and in mach5 but 
clearly not enough.

Generally I think this looks good.

"lastFrame" can mean last as in final, or last as in previous. "last" is one of those 
annoying English words.  Here it means final, if we get an Exception during processDwarf, use this to flag 
that we should return null from sender().  "finalFrame" would be clearer to me, anything else 
probably gets more verbose than you wanted.

Yes I like having the limit on the while loop in process_dwarf(), always 
worried how sane the information is that we are parsing through.

Thanks!
Kevin


On 24/03/2020 23:47, Yasumasa Suenaga wrote:

Thanks Serguei!

I will push it when I get second reviewer.


Yasumasa


On 2020/03/25 1:39, serguei.spit...@oracle.com wrote:

Hi Yasumasa,

I'm okay with this update.
My mach5 test run for this patch is passed.

Thanks,
Serguei


On 3/23/20 17:08, Yasumasa Suenaga wrote:

Hi Serguei,

Thanks for your comment!
I uploaded new webrev:

http://cr.openjdk.java.net/~ysuenaga/JDK-8240956/webrev.04/

Also I pushed it to submit repo:

  http://hg.openjdk.java.net/jdk/submit/rev/fade6a949bd1

On 2020/03/24 7:39, serguei.spit...@oracle.com wrote:

Hi Yasumasa,

The mach5 tier5 testing looks good.
The serviceability/sa/ClhsdbPstack.java is failed without fix and is not failed 
with it.

Thanks,
Serguei


On 3/23/20 10:18, serguei.spit...@oracle.com wrote:

Hi Yasumasa,

I looked at you changes.
It is hard to understand if this fully solves the issue.

http://cr.openjdk.java.net/~ysuenaga/JDK-8240956/webrev.03/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/debugger/linux/amd64/LinuxAMD64CFrame.java.frames.html

@@ -34,10 +34,11 @@
   public static LinuxAMD64CFrame getTopFrame(LinuxDebugger dbg, Address 
rip, ThreadContext context) {
    Address libptr = dbg.findLibPtrByAddress(rip);
    Address cfa = context.getRegisterAsAddress(AMD64ThreadContext.RBP);
    DwarfParser dwarf = null;
+ boolean unsupportedDwarf = false;
      if (libptr != null) { // Native frame
  try {
    dwarf = new DwarfParser(libptr);
    dwarf.processDwarf(rip);
-- 


@@ -45,24 +46,33 @@
   !dwarf.isBPOffsetAvailable())
  ? context.getRegisterAsAddress(AMD64ThreadContext.RBP)
  : context.getRegisterAsAddress(dwarf.getCFARegister())
.addOffsetTo(dwarf.getCFAOffset());
  } catch (DebuggerException e) {
- // Bail out to Java frame case
+ if (dwarf != null) {
+ // DWARF processing should succeed when the frame is native
+ // but it might fail if CIE has language personality routine
+ // and/or LSDA.
+ dwarf = null;
+ unsupportedDwarf = true;
+ } else {
+ throw e;
+ }
  }
    }
      return (cfa == null) ? null
- : new LinuxAMD64CFrame(dbg, cfa, rip, dwarf);
+ : new LinuxAMD64CFrame(dbg, cfa, rip, dwarf, !unsupportedDwarf);
 }

@@ -121,13 +131,25 @@
   }
     return isValidFrame(nextCFA, context) ? nextCFA : null;
 }
  - private DwarfParser getNextDwarf(Address nextPC) {
- DwarfParser nextDwarf = null;
+ @Override
+ public CFrame sender(ThreadProxy thread) {
+ if (!possibleNext) {
+ return null;
+ }
+
+ ThreadContext context = thread.getContext();
+
+ Address nextPC = getNextPC(dwarf != null);
+ if (nextPC == null) {
+ retur