Re: [RFR]8215622: Add dump to file support for jmap histo

[臧琳]

Dear all,
 I have found there is problem for handling the "filepath" and it cause 
test "java/util/logging/TestLoggerWeakRefLeak.java" fail, I have identified the 
root cause, please wait for the new update.
Thanks!

BRs,
Lin

From: 臧琳
Sent: Friday, January 25, 2019 9:00:19 AM
To: Hohensee, Paul; JC Beyler
Cc: serviceability-dev@openjdk.java.net
Subject: Re: [RFR]8215622: Add dump to file support for jmap histo

Dear All,
May I get more review about this webrev?
http://cr.openjdk.java.net/~xiaofeya/8215622/webrev.04/

Thanks!
BRs,
Lin

From: 臧琳
Sent: Tuesday, January 22, 2019 2:18:06 PM
To: Hohensee, Paul; JC Beyler
Cc: serviceability-dev@openjdk.java.net
Subject: Re: [RFR]8215622: Add dump to file support for jmap histo

Hi Paul,
Thanks a lot for your review. I have refined it based on your comments.
http://cr.openjdk.java.net/~xiaofeya/8215622/webrev.04/
Would you like to help review it again? Thanks

BRs,
Lin

From: Hohensee, Paul 
Sent: Friday, January 18, 2019 6:11:14 AM
To: 臧琳; JC Beyler
Cc: serviceability-dev@openjdk.java.net
Subject: Re: [RFR]8215622: Add dump to file support for jmap histo

Just a few small things.

In attachListener.cpp, line 278, is the static_cast needed? fileStream is a 
subclass of outputStream, so it should be ok to pass to the 
VM_GC_Heap_Inspection constructor, but maybe there's some C++ arcana I don't 
know about.

In attachListener.cpp, line 275, change "Fail to" to "Failed to".

In JMap.java, line 286, change  use \"all\""touse \"all\"."to 
match line 277.

Thanks,

Paul

On 1/11/19, 1:39 AM, "臧琳"  wrote:

Hi Jc, Paul and All,
 Here is webrev03 
http://cr.openjdk.java.net/~xiaofeya/8215622/webrev.03/
 would you like to help review?

Thanks,
Lin

From: 臧琳
Sent: Friday, January 11, 2019 10:25:27 AM
To: JC Beyler
Cc: Hohensee, Paul; serviceability-dev@openjdk.java.net
Subject: 答复: [RFR]8215622: Add dump to file support for jmap histo

Hi Jc,
 Thanks a lot. it is not a necessary change, I forget to omit it:)

BRs,
Lin

发件人: JC Beyler 
发送时间: 2019年1月11日 0:58:22
收件人: 臧琳
抄送: Hohensee, Paul; serviceability-dev@openjdk.java.net
主题: Re: [RFR]8215622: Add dump to file support for jmap histo

Hi Lin,

Small nit:

http://cr.openjdk.java.net/~xiaofeya/8215622/webrev.02/src/java.base/share/native/libjli/java.c.udiff.html

needs a copyright update,
Jc


On Wed, Jan 9, 2019 at 7:48 PM 臧琳 mailto:zangl...@jd.com>> 
wrote:
Dear All,
   I have updated the refined webrev at 
http://cr.openjdk.java.net/~xiaofeya/8215622/webrev.02/
   Would you like to help review? Thanks!

BRs,
Lin
From: 臧琳
Sent: Wednesday, January 9, 2019 11:00 AM
To: 'JC Beyler' mailto:jcbey...@google.com>>
Cc: Hohensee, Paul mailto:hohen...@amazon.com>>; 
serviceability-dev@openjdk.java.net
Subject: RE: [RFR]8215622: Add dump to file support for jmap histo

Dear JC,
   Thanks to point it out, I processed the “-file=” case in JMap.java 
but forgot to do it in attachListener.cpp. I will do it in next webrev.

Cheers,
Lin

From: JC Beyler mailto:jcbey...@google.com>>
Sent: Wednesday, January 9, 2019 10:51 AM
To: 臧琳 mailto:zangl...@jd.com>>
Cc: Hohensee, Paul mailto:hohen...@amazon.com>>; 
serviceability-dev@openjdk.java.net
Subject: Re: [RFR]8215622: Add dump to file support for jmap histo

Hi Lin,

Inlined as well :-)

On Tue, Jan 8, 2019 at 6:23 PM 臧琳 mailto:zangl...@jd.com>> 
wrote:
Dear JC,
 Thanks for your comments, I inlined my comments here:

http://cr.openjdk.java.net/~xiaofeya/8215622/webrev.01/src/hotspot/share/services/attachListener.cpp.udiff.html
  - Should we do like the rest of the file and declare variables when 
needed instead of doing them all at the start?
--- (Lin) I will do that in next webrev.
  - Should the method return JNI_ERR if the file cannot be created (because 
if not, then why fail if no file is passed at all?)
   --- (Lin) The logic is that when user use “-file=”, the file 
must be created successful, otherwise the “-file=” not work, which break user’s 
expection, so it fail here. If user not specify “-file=”, it indicate that user 
not expect to dump to file, so the outputStream will be used. Do you think it 
is reasonable?


No that is reasonable BUT your code currently allows the user to do 
"--file="; in this absurd case, your code prints out "No dump file specified" 
and just continues. Why not make that fail as well?

The bigger issue I see is the passing of NULL for a filename, why do we not 
do thi

12 RFR(M) 8195635: [Graal] nsk/jvmti/unit/ForceEarlyReturn/earlyretbase crashes with assertion "compilation level out of bounds"

[dean . long]

http://cr.openjdk.java.net/~dlong/8195635/webrev.5/
https://bugs.openjdk.java.net/browse/JDK-8195635

Please see the bug report for all the gory details.  Here's the short 
version:


If we allow any safepoint to be a suspend point, we run into trouble 
with PopFrame and ForceEarlyReturn, which reasonably expect the top 
frame not to change between the suspend and when the 
PopFrame/ForceEarlyReturn is executed.  Normally this is not an issue, 
but certain safepoints cause problems, when we are about to call a new 
Java method.  In particular, if we safepoint and suspend in 
JavaCallWrapper, the top frame will still be the caller, but when we 
execute the PopFrame/ForceEarlyReturn we will be in the callee.


The solution this patch takes is to block suspend around troublesome VM 
code using a new "allow_suspend" thread flag.  This means 
JavaThread::java_suspend can't just ask the VMThread to safepoint and be 
done.  Instead it has wait and allow threads to roll forward to an 
allowed suspend point.


dl

Re: 12 RFR(M) 8195635: [Graal] nsk/jvmti/unit/ForceEarlyReturn/earlyretbase crashes with assertion "compilation level out of bounds"

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

  
  
Hi Dean,
  
  I like this approach in general, and think, it is the best to
  take.
  (I already mentioned in the bug report comments).
  
  But there is some risk of regressions as the fix has pretty wide
  impact.
  
  If I understand correctly, this fix with
SuspendableMark helper objects
  defines the non-suspendable
  points (where suspend is not allowed) and also
  explicit suspendable points (where suspend is allowed to happen).
  By default (in contexts where there is no SuspendableMark)
  suspend is allowed.
  
  One concern is if there can be some execution contexts with nested
  opposite SuspendableMark's.
I wonder, if we could add some asserted traps to make sure there
are no such cases.
Otherwise, this mechanism can fail to always work correctly.
  
  Also, there is a couple of spots in this fix which I don't fully
  understand.
  
  One place is in the java_suspend() and java_suspend_self():
    
http://cr.openjdk.java.net/~dlong/8195635/webrev.5/src/hotspot/share/runtime/thread.cpp.frames.html
  
  The fix adds some comments but they are not enough.
  For instance, the use of
ThreadSuspendClosure
  and Handshake::execute() is not clear
in the newly added fragment (replaced the use of VM_ThreadSuspend helper object):
  
  2342   if (this == JavaThread::current()) {
2343 return;
2344   }
2345 
2346   // In the past, forcing a safepoint here was enough, but with
2347   // allow_suspend(), not all safepoints are suspend points.
2348   if (thread_state() == _thread_in_Java) {
2349 ThreadSuspendClosure tsc;
2350 Handshake::execute(&tsc, this);
2351   } else {
2352 MutexLockerEx ml(SR_lock(), Mutex::_no_safepoint_check_flag);
2353 // Wait for self-suspend or resume.
2354 // Not all interesting transitions perform a notify, so wait with
2355 // timeout.
2356 SR_lock()->wait(!Mutex::_no_safepoint_check_flag, 10);
2357   }
2358   }
  
  Another place is:
   
http://cr.openjdk.java.net/~dlong/8195635/webrev.5/src/hotspot/share/prims/jvmtiEnv.cpp.frames.html
  
  It is not easy to understand the consequences of change in the
  JvmtiEnv::SuspendThreadList().
  
  So, we need our Suspend/Resume experts (Dan and David) to look at
  this fix.
  
  
  I also have a couple of minor comments.
  
 http://cr.openjdk.java.net/~dlong/8195635/webrev.5/src/hotspot/share/prims/jvmtiEnv.cpp.frames.html
  
    Now we have three spots with the same checks:
   936 static jvmtiError suspendThread_request(JavaThread* java_thread) {
 937   // don't allow hidden thread suspend request.
 938   if (java_thread->is_hidden_from_external_view()) {
 939 return (JVMTI_ERROR_NONE);
 940   }
 . . .
 955 static jvmtiError suspendThread_complete(JavaThread* java_thread) {
 956   // don't allow hidden thread suspend request.
 957   if (java_thread->is_hidden_from_external_view()) {
 958 return (JVMTI_ERROR_NONE);
 959   }
 . . .
 970 JvmtiEnv::SuspendThread(JavaThread* java_thread) {
 971   // don't allow hidden thread suspend request.
 972   if (java_thread->is_hidden_from_external_view()) {
 973 return (JVMTI_ERROR_NONE);
 974   }


   At least, the checks in the suspendThread_request() and
suspendThread_complete()
   can be moved into the SuspendThreadList()
loops to make them more clear.


Also, the SuspendableMark helper objects for non-suspendable
points are named differently.
In the jvmciRuntime.cpp it is named 'nosuspend'.
In the sharedRuntime.cpp they are named 'block_suspend'.
This naming can be unified.


Otherwise, it is very good fix to have.
Thank you a lot for taking care about this hard-to-fix issue!

Thanks,
Serguei
  
  
  On 1/28/19 17:13, dean.l...@oracle.com wrote:

http://cr.openjdk.java.net/~dlong/8195635/webrev.5/
  
  https://bugs.openjdk.java.net/browse/JDK-8195635
  
  
  Please see the bug report for all the gory details.  Here's the
  short version:
  
  
  If we allow any safepoint to be a suspend point, we run into
  trouble with PopFrame and ForceEarlyReturn, which reasonably
  expect the top frame not to change between the suspend and when
  the PopFrame/ForceEarlyReturn is executed.  Normally this is not
  an issue, but certain safepoints cause problems, when we are about
  to call a new Java method.  In particular, if we safepoint and
  suspend in JavaCallWrapper, the top frame will still be the
  caller, but when we execute the PopFrame/ForceEarlyReturn we will
  be in the callee.
  
  
  The solution this patch takes is to block sus