Re: RFR(L): 8215624: add parallel heap inspection support for jmap histo(G1)(Internet mail)

[臧琳]

Hi Stefan, 
  >>  - Adding Atomic::load/store.
  >>  - Removing the time measurement in the run_task. I renamed G1's function 
  >>  to run_task_timed. If we need this outside of G1, we can rethink the API 
  >>  at that point.
   >>  - ZGC style cleanups
   Thanks for revising the patch,  they are all good to me, and I have made a 
tiny change based on it: 
   http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_04/ 
   http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_04-delta/
  it reduce the scope of mutex in ParHeapInspectTask, and delete unnecessary 
comments.
 
BRs,
Lin

On 2020/4/27, 4:34 PM, "Stefan Karlsson"  wrote:

Hi Lin,

On 2020-04-26 05:10, linzang(臧琳) wrote:
> Hi Stefan and Paul，
>  I have made a new patch based on your comments and Stefan's Poc code:
>  Webrev: 
http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_03/
>  Delta(based on Stefan's change:) : 
http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_03-delta/webrev_03-delta/

Thanks for providing a delta patch. It makes it much easier to look at, 
and more likely for reviewers to continue reviewing.

I'm going to continue focusing on the GC parts, and leave the rest to 
others to review.

> 
>  And Here are main changed I made and want to discuss with you:
>  1.  changed"parallelThreadNum=" to "parallel=" for jmap -histo 
options.
>  2.  Add logic to test where parallelHeapInspection is fail, in 
heapInspection.cpp
>This is because the parHeapInspectTask create thread local 
KlassInfoTable in it's work() method, and this may fail because of native OOM, 
in this case, the parallel should fail and serial heap inspection can be tried.
>One more thing I want discuss with you is about the member 
"_success" of parHeapInspectTask, when native OOM happenes, it is set to false. 
And since this "set" operation can be conducted in multiple threads, should it 
be atomic ops?  IMO, this is not necessary because "_success" can only be set 
to false, and there is no way to change it from back to true after the 
ParHeapInspectTask instance is created, so it is save to be non-atomic, do you 
agree with that?

In these situations you should be using the Atomic::load/store 
primitives. We're moving toward a later C++ standard were data races are 
considered undefined behavior.

> 3. make CollectedHeap::run_task() be an abstract virtual func, so 
that every subclass of collectedHeap should support it, so later implementation 
of new collectedHeap will not miss the "parallel" features.
>   The problem I want to discuss with you is about epsilonHeap and 
SerialHeap, as they may not need parallel heap iteration, so I only make 
task->work(0), in case the run_task() is invoked someway in future. Another way 
is to left run_task()  unimplemented, which one do you think is better?

I don't have a strong opinion about this.

  And also please help take a look at the zHeap, as there is a class 
zTask that wrap the abstractGangTask, and the collectedHeap::run_task() 
only accept  AbstraceGangTask* as argument, so I made a delegate class 
to adapt it , please see src/hotspot/share/gc/z/zHeap.cpp.
> 
>There maybe other better ways to sovle the above problems, welcome 
for any comments, Thanks!

I've created a few cleanups and changes on top of your latest patch:

https://cr.openjdk.java.net/~stefank/8215624/webrev.02.delta
https://cr.openjdk.java.net/~stefank/8215624/webrev.02

- Adding Atomic::load/store.
- Removing the time measurement in the run_task. I renamed G1's function 
to run_task_timed. If we need this outside of G1, we can rethink the API 
at that point.
- ZGC style cleanups

Thanks,
StefanK

> 
> BRs,
> Lin
> 
> On 2020/4/23, 11:08 AM, "linzang(臧琳)"  wrote:
> 
>  Thanks Paul! I agree with using "parallel", will make the update in 
next patch, Thanks for help update the CSR.
> 
>  BRs,
>  Lin
> 
>  On 2020/4/23, 4:42 AM, "Hohensee, Paul"  wrote:
> 
>  For the interface, I'd use "parallel" instead of 
"parallelThreadNum". All the other options are lower case, and it's a lot 
easier to type "parallel". I took the liberty of updating the CSR. If you're ok 
with it, you might want to change variable names and such, plus of course 
JMap.usage.
> 
>  Thanks,
>  Paul
> 
>  On 4/22/20, 2:29 AM, "serviceability-dev on behalf of 
linzang(臧琳)"  wrote:
> 
>  Dear Stefan,
> 
>  Thanks a lot! I agree with you to decouple the heap 
inspection code with GC's.
>  I will start  from your POC code, may discuss with 
you later.
> 
> 
>  BRs,
>  Lin
> 
>

Re: RFR(T) : 8243946 : serviceability/sa tests fail after JDK-8243928

[Igor Ignatyev]

thanks, i'll push it as soon as I get full test results (assuming there are no 
new failures).

-- Igor

> On Apr 27, 2020, at 10:21 PM, serguei.spit...@oracle.com wrote:
> 
> This update looks good too.
> 
> Thanks,
> Serguei
> 
> 
> On 4/27/20 22:12, Igor Ignatyev wrote:
>> Thanks Mikael,
>> 
>> there is, thought, one more test failing after 8243928 -- 
>> serviceability/jvmti/CanGenerateAllClassHook/CanGenerateAllClassHook.java. 
>> the fix is pretty much the same revert part of 8243928:
>> 
>>> diff -r 67cbcf4f 
>>> test/hotspot/jtreg/serviceability/jvmti/CanGenerateAllClassHook/CanGenerateAllClassHook.java
>>> --- 
>>> a/test/hotspot/jtreg/serviceability/jvmti/CanGenerateAllClassHook/CanGenerateAllClassHook.java
>>>   Mon Apr 27 20:06:22 2020 -0700
>>> +++ 
>>> b/test/hotspot/jtreg/serviceability/jvmti/CanGenerateAllClassHook/CanGenerateAllClassHook.java
>>>   Mon Apr 27 22:11:31 2020 -0700
>>> @@ -30,7 +30,7 @@
>>>   * @requires vm.cds
>>>   * @library /test/lib
>>>   * @compile CanGenerateAllClassHook.java
>>> - * @run driver/native CanGenerateAllClassHook
>>> + * @run main/native CanGenerateAllClassHook
>>>   */
>>> 
>> 
>> -- Igor
>> 
>>> On Apr 27, 2020, at 10:10 PM, Mikael Vidstedt  
>>> wrote:
>>> 
>>> 
>>> Looks good, thanks for the quick turnaround!
>>> 
>>> Cheers,
>>> Mikael
>>> 
 On Apr 27, 2020, at 10:02 PM, Igor Ignatyev  
 wrote:
 
 http://cr.openjdk.java.net/~iignatyev//8243946/webrev.00
> 2 lines changed: 0 ins; 0 del; 2 mod;
 Hi all,
 
 (so now it's my time to apology for inconvenience)
 
 could you please review this small follow-up fix/partial revert of 
 8243928[1]? serviceability/sa/TestCpoolForInvokeDynamic.java  and 
 TestDefaultMethods.java test use non-exported API so they can't be run in 
 driver mode (b/c jtreg use vanilla JVM for driver code, meaning even 
 exports from @modules tags are ignored)
 
 JBS: https://bugs.openjdk.java.net/browse/JDK-8243946
 testing: test/hotspot/jtreg/serviceability (in progress)
 webrev: http://cr.openjdk.java.net/~iignatyev//8243946/webrev.00
 
 [1]  https://bugs.openjdk.java.net/browse/JDK-8243928
 
 Thanks,
 -- Igor
> 

Re: RFR(T) : 8243946 : serviceability/sa tests fail after JDK-8243928

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

This update looks good too.

Thanks,
Serguei


On 4/27/20 22:12, Igor Ignatyev wrote:

Thanks Mikael,

there is, thought, one more test failing after 8243928 -- 
serviceability/jvmti/CanGenerateAllClassHook/CanGenerateAllClassHook.java. the 
fix is pretty much the same revert part of 8243928:


diff -r 67cbcf4f 
test/hotspot/jtreg/serviceability/jvmti/CanGenerateAllClassHook/CanGenerateAllClassHook.java
--- 
a/test/hotspot/jtreg/serviceability/jvmti/CanGenerateAllClassHook/CanGenerateAllClassHook.java
  Mon Apr 27 20:06:22 2020 -0700
+++ 
b/test/hotspot/jtreg/serviceability/jvmti/CanGenerateAllClassHook/CanGenerateAllClassHook.java
  Mon Apr 27 22:11:31 2020 -0700
@@ -30,7 +30,7 @@
   * @requires vm.cds
   * @library /test/lib
   * @compile CanGenerateAllClassHook.java
- * @run driver/native CanGenerateAllClassHook
+ * @run main/native CanGenerateAllClassHook
   */



-- Igor


On Apr 27, 2020, at 10:10 PM, Mikael Vidstedt  
wrote:


Looks good, thanks for the quick turnaround!

Cheers,
Mikael


On Apr 27, 2020, at 10:02 PM, Igor Ignatyev  wrote:

http://cr.openjdk.java.net/~iignatyev//8243946/webrev.00

2 lines changed: 0 ins; 0 del; 2 mod;

Hi all,

(so now it's my time to apology for inconvenience)

could you please review this small follow-up fix/partial revert of 8243928[1]? 
serviceability/sa/TestCpoolForInvokeDynamic.java  and TestDefaultMethods.java 
test use non-exported API so they can't be run in driver mode (b/c jtreg use 
vanilla JVM for driver code, meaning even exports from @modules tags are 
ignored)

JBS: https://bugs.openjdk.java.net/browse/JDK-8243946
testing: test/hotspot/jtreg/serviceability (in progress)
webrev: http://cr.openjdk.java.net/~iignatyev//8243946/webrev.00

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

Thanks,
-- Igor

Re: RFR(T) : 8243946 : serviceability/sa tests fail after JDK-8243928

[Igor Ignatyev]

Hi Serguei,

thanks for review,  no I haven't yet as I found another problem w/ 8243928.

-- Igor

> On Apr 27, 2020, at 10:14 PM, serguei.spit...@oracle.com wrote:
> 
> Hi Igor,
> 
> Looks good but you have already pushed. :)
> 
> Thanks,
> Serguei
> 
> 
> On 4/27/20 22:10, Mikael Vidstedt wrote:
>> Looks good, thanks for the quick turnaround!
>> 
>> Cheers,
>> Mikael
>> 
>>> On Apr 27, 2020, at 10:02 PM, Igor Ignatyev  
>>> wrote:
>>> 
>>> http://cr.openjdk.java.net/~iignatyev//8243946/webrev.00
 2 lines changed: 0 ins; 0 del; 2 mod;
>>> Hi all,
>>> 
>>> (so now it's my time to apology for inconvenience)
>>> 
>>> could you please review this small follow-up fix/partial revert of 
>>> 8243928[1]? serviceability/sa/TestCpoolForInvokeDynamic.java  and 
>>> TestDefaultMethods.java test use non-exported API so they can't be run in 
>>> driver mode (b/c jtreg use vanilla JVM for driver code, meaning even 
>>> exports from @modules tags are ignored)
>>> 
>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8243946
>>> testing: test/hotspot/jtreg/serviceability (in progress)
>>> webrev: http://cr.openjdk.java.net/~iignatyev//8243946/webrev.00
>>> 
>>> [1]  https://bugs.openjdk.java.net/browse/JDK-8243928
>>> 
>>> Thanks,
>>> -- Igor
> 

Re: RFR(T) : 8243946 : serviceability/sa tests fail after JDK-8243928

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

Hi Igor,

Looks good but you have already pushed. :)

Thanks,
Serguei


On 4/27/20 22:10, Mikael Vidstedt wrote:

Looks good, thanks for the quick turnaround!

Cheers,
Mikael


On Apr 27, 2020, at 10:02 PM, Igor Ignatyev  wrote:

http://cr.openjdk.java.net/~iignatyev//8243946/webrev.00

2 lines changed: 0 ins; 0 del; 2 mod;

Hi all,

(so now it's my time to apology for inconvenience)

could you please review this small follow-up fix/partial revert of 8243928[1]? 
serviceability/sa/TestCpoolForInvokeDynamic.java  and TestDefaultMethods.java 
test use non-exported API so they can't be run in driver mode (b/c jtreg use 
vanilla JVM for driver code, meaning even exports from @modules tags are 
ignored)

JBS: https://bugs.openjdk.java.net/browse/JDK-8243946
testing: test/hotspot/jtreg/serviceability (in progress)
webrev: http://cr.openjdk.java.net/~iignatyev//8243946/webrev.00

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

Thanks,
-- Igor

Re: RFR(T) : 8243946 : serviceability/sa tests fail after JDK-8243928

[Igor Ignatyev]

Thanks Mikael,

there is, thought, one more test failing after 8243928 -- 
serviceability/jvmti/CanGenerateAllClassHook/CanGenerateAllClassHook.java. the 
fix is pretty much the same revert part of 8243928:

> diff -r 67cbcf4f 
> test/hotspot/jtreg/serviceability/jvmti/CanGenerateAllClassHook/CanGenerateAllClassHook.java
> --- 
> a/test/hotspot/jtreg/serviceability/jvmti/CanGenerateAllClassHook/CanGenerateAllClassHook.java
>   Mon Apr 27 20:06:22 2020 -0700
> +++ 
> b/test/hotspot/jtreg/serviceability/jvmti/CanGenerateAllClassHook/CanGenerateAllClassHook.java
>   Mon Apr 27 22:11:31 2020 -0700
> @@ -30,7 +30,7 @@
>   * @requires vm.cds
>   * @library /test/lib
>   * @compile CanGenerateAllClassHook.java
> - * @run driver/native CanGenerateAllClassHook
> + * @run main/native CanGenerateAllClassHook
>   */
> 


-- Igor

> On Apr 27, 2020, at 10:10 PM, Mikael Vidstedt  
> wrote:
> 
> 
> Looks good, thanks for the quick turnaround!
> 
> Cheers,
> Mikael
> 
>> On Apr 27, 2020, at 10:02 PM, Igor Ignatyev  wrote:
>> 
>> http://cr.openjdk.java.net/~iignatyev//8243946/webrev.00
>>> 2 lines changed: 0 ins; 0 del; 2 mod;
>> 
>> Hi all,
>> 
>> (so now it's my time to apology for inconvenience)
>> 
>> could you please review this small follow-up fix/partial revert of 
>> 8243928[1]? serviceability/sa/TestCpoolForInvokeDynamic.java  and 
>> TestDefaultMethods.java test use non-exported API so they can't be run in 
>> driver mode (b/c jtreg use vanilla JVM for driver code, meaning even exports 
>> from @modules tags are ignored)
>> 
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8243946
>> testing: test/hotspot/jtreg/serviceability (in progress)
>> webrev: http://cr.openjdk.java.net/~iignatyev//8243946/webrev.00
>> 
>> [1]  https://bugs.openjdk.java.net/browse/JDK-8243928
>> 
>> Thanks,
>> -- Igor
> 

Re: RFR(T) : 8243946 : serviceability/sa tests fail after JDK-8243928

[David Holmes]

Looks good and trivial.

Thanks,
David

On 28/04/2020 3:02 pm, Igor Ignatyev wrote:

http://cr.openjdk.java.net/~iignatyev//8243946/webrev.00

2 lines changed: 0 ins; 0 del; 2 mod;


Hi all,

(so now it's my time to apology for inconvenience)

could you please review this small follow-up fix/partial revert of 8243928[1]? 
serviceability/sa/TestCpoolForInvokeDynamic.java  and TestDefaultMethods.java 
test use non-exported API so they can't be run in driver mode (b/c jtreg use 
vanilla JVM for driver code, meaning even exports from @modules tags are 
ignored)

JBS: https://bugs.openjdk.java.net/browse/JDK-8243946
testing: test/hotspot/jtreg/serviceability (in progress)
webrev: http://cr.openjdk.java.net/~iignatyev//8243946/webrev.00

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

Thanks,
-- Igor

Re: RFR(T) : 8243946 : serviceability/sa tests fail after JDK-8243928

[Mikael Vidstedt]

Looks good, thanks for the quick turnaround!

Cheers,
Mikael

> On Apr 27, 2020, at 10:02 PM, Igor Ignatyev  wrote:
> 
> http://cr.openjdk.java.net/~iignatyev//8243946/webrev.00
>> 2 lines changed: 0 ins; 0 del; 2 mod;
> 
> Hi all,
> 
> (so now it's my time to apology for inconvenience)
> 
> could you please review this small follow-up fix/partial revert of 
> 8243928[1]? serviceability/sa/TestCpoolForInvokeDynamic.java  and 
> TestDefaultMethods.java test use non-exported API so they can't be run in 
> driver mode (b/c jtreg use vanilla JVM for driver code, meaning even exports 
> from @modules tags are ignored)
> 
> JBS: https://bugs.openjdk.java.net/browse/JDK-8243946
> testing: test/hotspot/jtreg/serviceability (in progress)
> webrev: http://cr.openjdk.java.net/~iignatyev//8243946/webrev.00
> 
> [1]  https://bugs.openjdk.java.net/browse/JDK-8243928
> 
> Thanks,
> -- Igor

RFR(T) : 8243946 : serviceability/sa tests fail after JDK-8243928

[Igor Ignatyev]

http://cr.openjdk.java.net/~iignatyev//8243946/webrev.00
> 2 lines changed: 0 ins; 0 del; 2 mod;

Hi all,

(so now it's my time to apology for inconvenience)

could you please review this small follow-up fix/partial revert of 8243928[1]? 
serviceability/sa/TestCpoolForInvokeDynamic.java  and TestDefaultMethods.java 
test use non-exported API so they can't be run in driver mode (b/c jtreg use 
vanilla JVM for driver code, meaning even exports from @modules tags are 
ignored)

JBS: https://bugs.openjdk.java.net/browse/JDK-8243946
testing: test/hotspot/jtreg/serviceability (in progress)
webrev: http://cr.openjdk.java.net/~iignatyev//8243946/webrev.00

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

Thanks,
-- Igor

Re: trivial RFR(XS): 8243941: build issue introduced with the push of 8242237

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

  
  
Pushed.
  Sorry for the inconvenience.
  
  Thanks,
  Serguei
  
  
  On 4/27/20 20:55, Igor Ignatyev wrote:

sure,
  and thank you, Serguei, for handling this so promptly!
  

  -- Igor

  
On Apr 27, 2020, at 8:54 PM, serguei.spit...@oracle.com
  wrote:


  
Thanks, Igor!
  Will push it after build-tier1 job is completed.
  
  Thanks,
  Serguei
  
  On 4/27/20 20:53, Igor Ignatyev wrote:

Hi Serguei,
  
  
  LGTM,
  
  
  -- Igor

  
On Apr 27, 2020, at 8:52 PM, serguei.spit...@oracle.com
  wrote:


   Please, review small patch
to fix the build regression caused by
fix of 8242237.

Patch:

diff --git
a/test/hotspot/jtreg/serviceability/jvmti/HiddenClass/libHiddenClassSigTest.cpp
b/test/hotspot/jtreg/serviceability/jvmti/HiddenClass/libHiddenClassSigTest.cpp
---
a/test/hotspot/jtreg/serviceability/jvmti/HiddenClass/libHiddenClassSigTest.cpp
+++
b/test/hotspot/jtreg/serviceability/jvmti/HiddenClass/libHiddenClassSigTest.cpp
@@ -242,7 +242,7 @@
 
 /* Process a CLASS_LOAD or
aClassPrepare event. */
 static void
process_class_event(jvmtiEnv* jvmti,
JNIEnv* jni, jclass klass,
-    int*
event_count_ptr, const char* event_name)
{
+    jint*
event_count_ptr, const char* event_name)
{
   char* sig = NULL;
   char* gsig = NULL;
   jvmtiError err;


Summary:
  Windows Compiler reports errors when a
parameter is declared as 'int*' but
jint* is actually used in the calls:

./open/test/hotspot/jtreg/serviceability/jvmti/HiddenClass/libHiddenClassSigTest.cpp(271):
error C2664: 'void
process_class_event(jvmtiEnv *,JNIEnv
*,jclass,int *,const char *)': cannot
convert argument 4 from 'jint *' to 'int
*' 
./open/test/hotspot/jtreg/serviceability/jvmti/HiddenClass/libHiddenClassSigTest.cpp(271):
note: Types pointed to are unrelated;
conversion requires reinterpret_cast,
C-style cast or function-style cast


Testing:
  A mach5 job build-tier1 submitted.

Thanks,
serguei
  

  


  


  

  


  

  


  

Re: trivial RFR(XS): 8243941: build issue introduced with the push of 8242237

[Igor Ignatyev]

sure, and thank you, Serguei, for handling this so promptly!

-- Igor

> On Apr 27, 2020, at 8:54 PM, serguei.spit...@oracle.com wrote:
> 
> Thanks, Igor!
> Will push it after build-tier1 job is completed.
> 
> Thanks,
> Serguei
> 
> On 4/27/20 20:53, Igor Ignatyev wrote:
>> Hi Serguei,
>> 
>> LGTM,
>> 
>> -- Igor
>> 
>>> On Apr 27, 2020, at 8:52 PM, serguei.spit...@oracle.com 
>>>  wrote:
>>> 
>>> Please, review small patch to fix the build regression caused by fix of 
>>> 8242237.
>>> 
>>> Patch:
>>> 
>>> diff --git 
>>> a/test/hotspot/jtreg/serviceability/jvmti/HiddenClass/libHiddenClassSigTest.cpp
>>>  
>>> b/test/hotspot/jtreg/serviceability/jvmti/HiddenClass/libHiddenClassSigTest.cpp
>>> --- 
>>> a/test/hotspot/jtreg/serviceability/jvmti/HiddenClass/libHiddenClassSigTest.cpp
>>> +++ 
>>> b/test/hotspot/jtreg/serviceability/jvmti/HiddenClass/libHiddenClassSigTest.cpp
>>> @@ -242,7 +242,7 @@
>>>  
>>>  /* Process a CLASS_LOAD or aClassPrepare event. */
>>>  static void process_class_event(jvmtiEnv* jvmti, JNIEnv* jni, jclass klass,
>>> -int* event_count_ptr, const char* 
>>> event_name) {
>>> +jint* event_count_ptr, const char* 
>>> event_name) {
>>>char* sig = NULL;
>>>char* gsig = NULL;
>>>jvmtiError err;
>>> 
>>> 
>>> Summary:
>>>   Windows Compiler reports errors when a parameter is declared as 'int*' 
>>> but jint* is actually used in the calls:
>>> 
>>> ./open/test/hotspot/jtreg/serviceability/jvmti/HiddenClass/libHiddenClassSigTest.cpp(271):
>>>  error C2664: 'void process_class_event(jvmtiEnv *,JNIEnv *,jclass,int 
>>> *,const char *)': cannot convert argument 4 from 'jint *' to 'int *' 
>>> ./open/test/hotspot/jtreg/serviceability/jvmti/HiddenClass/libHiddenClassSigTest.cpp(271):
>>>  note: Types pointed to are unrelated; conversion requires 
>>> reinterpret_cast, C-style cast or function-style cast
>>> 
>>> 
>>> Testing:
>>>   A mach5 job build-tier1 submitted.
>>> 
>>> Thanks,
>>> serguei
>> 
> 

Re: trivial RFR(XS): 8243941: build issue introduced with the push of 8242237

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

  
  
Thanks, Igor!
  Will push it after build-tier1 job is completed.
  
  Thanks,
  Serguei
  
  On 4/27/20 20:53, Igor Ignatyev wrote:

Hi
  Serguei,
  
  
  LGTM,
  
  
  -- Igor

  
On Apr 27, 2020, at 8:52 PM, serguei.spit...@oracle.com
  wrote:


   Please, review small patch to fix the build
regression caused by fix of 8242237.

Patch:

diff --git
a/test/hotspot/jtreg/serviceability/jvmti/HiddenClass/libHiddenClassSigTest.cpp
b/test/hotspot/jtreg/serviceability/jvmti/HiddenClass/libHiddenClassSigTest.cpp
---
a/test/hotspot/jtreg/serviceability/jvmti/HiddenClass/libHiddenClassSigTest.cpp
+++
b/test/hotspot/jtreg/serviceability/jvmti/HiddenClass/libHiddenClassSigTest.cpp
@@ -242,7 +242,7 @@
 
 /* Process a CLASS_LOAD or aClassPrepare event. */
 static void process_class_event(jvmtiEnv* jvmti,
JNIEnv* jni, jclass klass,
-    int* event_count_ptr,
const char* event_name) {
+    jint* event_count_ptr,
const char* event_name) {
   char* sig = NULL;
   char* gsig = NULL;
   jvmtiError err;


Summary:
  Windows Compiler reports errors when a parameter is
declared as 'int*' but jint* is actually used in the
calls:

./open/test/hotspot/jtreg/serviceability/jvmti/HiddenClass/libHiddenClassSigTest.cpp(271):
error C2664: 'void process_class_event(jvmtiEnv *,JNIEnv
*,jclass,int *,const char *)': cannot convert argument 4
from 'jint *' to 'int *' 
./open/test/hotspot/jtreg/serviceability/jvmti/HiddenClass/libHiddenClassSigTest.cpp(271):
note: Types pointed to are unrelated; conversion
requires reinterpret_cast, C-style cast or
function-style cast


Testing:
  A mach5 job build-tier1 submitted.

Thanks,
serguei
  

  


  


  

trivial RFR(XS): 8243941: build issue introduced with the push of 8242237

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

  
  
Please, review small patch to fix the build
  regression caused by fix of 8242237.
  
  Patch:
  
  diff --git
a/test/hotspot/jtreg/serviceability/jvmti/HiddenClass/libHiddenClassSigTest.cpp
b/test/hotspot/jtreg/serviceability/jvmti/HiddenClass/libHiddenClassSigTest.cpp
  ---
a/test/hotspot/jtreg/serviceability/jvmti/HiddenClass/libHiddenClassSigTest.cpp
  +++
b/test/hotspot/jtreg/serviceability/jvmti/HiddenClass/libHiddenClassSigTest.cpp
  @@ -242,7 +242,7 @@
   
   /* Process a CLASS_LOAD or aClassPrepare event. */
   static void process_class_event(jvmtiEnv* jvmti, JNIEnv* jni,
  jclass klass,
  -    int* event_count_ptr, const char*
  event_name) {
  +    jint* event_count_ptr, const
  char* event_name) {
     char* sig = NULL;
     char* gsig = NULL;
     jvmtiError err;
  
  
  Summary:
    Windows Compiler reports errors when a parameter is declared as 'int*' but jint*
  is actually used in the calls:

./open/test/hotspot/jtreg/serviceability/jvmti/HiddenClass/libHiddenClassSigTest.cpp(271):
  error C2664: 'void process_class_event(jvmtiEnv *,JNIEnv
  *,jclass,int *,const char *)': cannot convert argument 4 from
  'jint *' to 'int *'
  
./open/test/hotspot/jtreg/serviceability/jvmti/HiddenClass/libHiddenClassSigTest.cpp(271):
  note: Types pointed to are unrelated; conversion requires
  reinterpret_cast, C-style cast or function-style cast
  
  
  Testing:
    A mach5 job build-tier1 submitted.
  
  Thanks,
  serguei

  

Re: trivial RFR(XS): 8243941: build issue introduced with the push of 8242237

[Igor Ignatyev]

Hi Serguei,

LGTM,

-- Igor

> On Apr 27, 2020, at 8:52 PM, serguei.spit...@oracle.com wrote:
> 
> Please, review small patch to fix the build regression caused by fix of 
> 8242237.
> 
> Patch:
> 
> diff --git 
> a/test/hotspot/jtreg/serviceability/jvmti/HiddenClass/libHiddenClassSigTest.cpp
>  
> b/test/hotspot/jtreg/serviceability/jvmti/HiddenClass/libHiddenClassSigTest.cpp
> --- 
> a/test/hotspot/jtreg/serviceability/jvmti/HiddenClass/libHiddenClassSigTest.cpp
> +++ 
> b/test/hotspot/jtreg/serviceability/jvmti/HiddenClass/libHiddenClassSigTest.cpp
> @@ -242,7 +242,7 @@
>  
>  /* Process a CLASS_LOAD or aClassPrepare event. */
>  static void process_class_event(jvmtiEnv* jvmti, JNIEnv* jni, jclass klass,
> -int* event_count_ptr, const char* 
> event_name) {
> +jint* event_count_ptr, const char* 
> event_name) {
>char* sig = NULL;
>char* gsig = NULL;
>jvmtiError err;
> 
> 
> Summary:
>   Windows Compiler reports errors when a parameter is declared as 'int*' but 
> jint* is actually used in the calls:
> 
> ./open/test/hotspot/jtreg/serviceability/jvmti/HiddenClass/libHiddenClassSigTest.cpp(271):
>  error C2664: 'void process_class_event(jvmtiEnv *,JNIEnv *,jclass,int 
> *,const char *)': cannot convert argument 4 from 'jint *' to 'int *' 
> ./open/test/hotspot/jtreg/serviceability/jvmti/HiddenClass/libHiddenClassSigTest.cpp(271):
>  note: Types pointed to are unrelated; conversion requires reinterpret_cast, 
> C-style cast or function-style cast
> 
> 
> Testing:
>   A mach5 job build-tier1 submitted.
> 
> Thanks,
> serguei

Re: RFR(T) : 8243928 : several svc tests can be run in driver mode

[Igor Ignatyev]

Thanks Alex, pushed.

-- Igor

> On Apr 27, 2020, at 7:36 PM, Alex Menkov  wrote:
> 
> Hi Igor,
> 
> LGTM
> 
> --alex
> 
> On 04/27/2020 16:51, Igor Ignatyev wrote:
>> http://cr.openjdk.java.net/~iignatyev//8243928/webrev.00
>>> 16 lines changed: 4 ins; 0 del; 12 mod;
>> Hi all,
>> could you please review the trivial patch which updates 15 svc tests to use 
>> a driver mode?
>> from JBS:
>>> the "main" classes of the tests listed below fork new JVMs and check their 
>>> outputs, so there is no need to run them w/ external vm flags.<...>
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8243928
>> webrev: http://cr.openjdk.java.net/~iignatyev//8243928/webrev.00
>> Thanks,
>> -- Igor
>>  

Re: RFR(T) : 8243928 : several svc tests can be run in driver mode

[Alex Menkov]

Hi Igor,

LGTM

--alex

On 04/27/2020 16:51, Igor Ignatyev wrote:

http://cr.openjdk.java.net/~iignatyev//8243928/webrev.00

16 lines changed: 4 ins; 0 del; 12 mod;



Hi all,

could you please review the trivial patch which updates 15 svc tests to use a 
driver mode?
from JBS:

the "main" classes of the tests listed below fork new JVMs and check their outputs, 
so there is no need to run them w/ external vm flags.<...>


JBS: https://bugs.openjdk.java.net/browse/JDK-8243928
webrev: http://cr.openjdk.java.net/~iignatyev//8243928/webrev.00


Thanks,
-- Igor
  

Re: RFR(M) 8243500: SA: Incorrect BCI and Line Number with jstack if the top frame is in the interpreter (BSD and Windows)

[Alex Menkov]

Hi Chris,

The fix looks good.

--alex

On 04/27/2020 12:17, Chris Plummer wrote:

Ping! Please help review if you can.

thanks,

Chris

On 4/24/20 12:44 AM, Chris Plummer wrote:

Hello,

Please review the following:

https://bugs.openjdk.java.net/browse/JDK-8243500
http://cr.openjdk.java.net/~cjplummer/8243500/webrev.00/index.html

A couple years ago JDK-8214226 fixed an issue on Linux-x64 with SA 
stack dumps not properly displaying the correct line number for the 
topmost frame if it was interpreted. The issue was that SA was always 
relying on frame->bcp when in fact the BCP is kept in R13, and only 
flushed to frame->bcp when needed as a scratch register. So this means 
that SA was in most cases grabbing a stale value from frame->bcp.


The fix for JDK-8214226 was mostly made in X86Frame.java to support 
using the BCP register for the topmost frame instead using frame->bcp. 
This fix actually had a bug in it that was causing the "illegal bci" 
failures we've been seeing. There is already a separate webrev and RFR 
out for that:


https://bugs.openjdk.java.net/browse/JDK-8231634
http://cr.openjdk.java.net/~cjplummer/8231634/webrev.00/index.html

What this RFR addresses is the fact that part of the fix for 
JDK-8214226 was in LinuxAMD64JavaThreadPDAccess.java, but the same 
changes were never made to WindowsAMD64JavaThreadPDAccess.java or 
BsdAMD64JavaThreadPDAccess.java. This fix addresses those two ports. 
Here's the CR and changeset for reference:


https://bugs.openjdk.java.net/browse/JDK-8214226
http://hg.openjdk.java.net/jdk/jdk/rev/9a73a4e4011f

The changes for the fix are pretty trivial. The more complicated part 
is the test I added that will reproduce the issue 100% of the time on 
platforms where SA does not properly check the BCP register. For this 
reason I've used @requires to limit running this test on just those 
platforms I know have the support in place. The test has pretty good 
comments on how it works, so I won't go into details here.


thanks,

Chris

Re: RFR (L): 8241807: JDWP needs update for hidden classes

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

  
  
Hi Leonid,
  
  Updated webrev version with the suggested refactoring is:
 
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/valhalla-jdi-events.5/
  

  

  

  


  

  

  
  

  Please, see my comments below.
  
  On 4/24/20 13:20, Leonid Mesnik wrote:


  Hi
  I have several comments about coding and desing. These tests
might be used as examples for new JDI tests based on this
framework so I think it would be better to polish them.


Yes, as we agreed it'd be nice to create good examples, a base for a
future test development in the JDI area.
I also wanted to polish these tests as much as possible, so thanks
for helping with it.
These are good suggestions from you below.
One problem is that these tests will differ in style from the rest
of the NSK JDI tests but it has to be okay.



  General comment:
  
  1. You often check results (not null, exactly one result). 
  
  It makes sense to use jdk.test.lib.Asserts for such
verification. Just to reduce number of code and fail with
standard exceptions.


Good suggestion - fixed now.


  2. Wildcard imports like 'import com.sun.jdi.*;' are not
recommended.


Good suggestion - fixed.
The list of imports became big but I see no problem with that.


  3. I recommend to catch Throwable instead of Exception
especially in non-main threads. Just to ensure that nothing is
missed.
  


God suggestion - fixed, at least, in places where it makes sense to
do.


   
  4. nsk.share.Failure is subclass of RuntimeException so you
don't need to add it to throws. Now sometimes it is added,
sometimes not. It looks inconsistent.
  


Good suggestion - fixed.
In fact, I did not like the use of Failure as it creates this
inconsistency, so I tried to get rid of it.


   
  5. There are lot of code duplication in debugger and debugge
classes. Does it make a sense to merge it into base classes?
  


I was thinking about this too but was reluctant to do so, as there
are just two tests.
But it has to be beneficial to move shared code to the JDI testing
framework.
I've just made some initial preparation by separating common parts
DebuggeeBase and DebuggerBase.
Also, it seems to be natural to move the HiddenClass and
EventHandler classes to their own files.
Then I've decided to combine both tests into one.
All refactoring still makes sense to have.


   
  http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/valhalla-jdi-events.4/test/hotspot/jtreg/vmTestbase/nsk/jdi/HiddenClass/ClassPrepareEvent/classPrepEvent001.java.html
  6. The getField can't return null, it verifies result. So check
in line 191-193 is not needed.
   190 Field field = getField(hcRefType, "hcField");
 191 if (field == null) {
 192 throw new Failure("TEST FAILED: cannot enable ModificationWhatchpointRequest: hcField not found");
 193 }



Good catch - fixed.


  
  
  7. I would recommend to move all
initialization from run in constructor and make most of variable
'final' it helps to ensure that they are initialized.
  
 297 public int run(final String args[], final PrintStream out) {
 298 ArgumentHandler argHandler = new ArgumentHandler(args);
 299 
 300 log = new Log(out, argHandler);
 301 eventTimeout = argHandler.getWaitTime() * 60 * 1000; // milliseconds
 302 launchDebuggee(argHandler);
  


Right comment in general, but I did not go with a constructor and
final fields.
Instead, I've just reorganized this a little bit.
There is a little base to refactor to the constructor because the
field "log" needs to be static and timeout can be local.



   
  8. Pair looks inconsistent. start
saves handler as class field while join use as parameter.
  
  
305 startEventHandler();
  
  
330 joinEventHandler(eventHandler);


  
  I think that something like  
  
  
EventHandler eventHandler = startEventHandler();
joinEventHandler(eventHandler);
  
  looks better and restrict usage of
eventHandler. You also migt move methods startEventHandler into
EventHandler itself.


Good suggestion - fixed. Moved both methods to the EventHandler
class.


  
  
  9. I suggest to make setHCRefType
private and rename 

Re: Fwd: RFR(S) 8231634: SA stack walking fails with "illegal bci"

[Alex Menkov]

Hi Chris,

great analysis.
LGTM++

--alex

On 04/27/2020 14:52, serguei.spit...@oracle.com wrote:

Hi Chris,

The fix looks reasonable.

Thanks,
Serguei


On 4/27/20 12:16, Chris Plummer wrote:
Ping! Please help review if you can. There's also an RFR out for 
8243500, which is related.


thanks,

Chris

 Forwarded Message 
Subject:RFR(S) 8231634: SA stack walking fails with "illegal bci"
Date:   Thu, 23 Apr 2020 14:35:49 -0700
From:   Chris Plummer 
To: serviceability-dev 



Hello,

Please help review the following:

https://bugs.openjdk.java.net/browse/JDK-8231634
http://cr.openjdk.java.net/~cjplummer/8231634/webrev.00/index.html

The fix is fairly simple: Don't use R13 as the BCP if it does not 
point into the methods actual bytecodes. Instead rely on the BCP value 
flushed to the frame. Details below are mostly there to help you 
understand the big picture of what is going on.


First a bit a background on what the live_bcp field is in the 
X86Frame. BCP is "bytecode pointer". It points to the current bytecode 
being executed. I'm mostly just talking about interpreter frames here, 
but I think for compiled frames it points to the current native 
instruction being executed. For all but the current frame, frame->bcp 
reliably contains the BCP. For the current frame you normally need to 
look in a register for an up to date BCP. On x86_64 that's R13. When 
trying to find and create the current frame for a thread, eventually 
you end up in LinuxAMD64JavaThreadPDAccess.getCurrentFrameGuess(), 
which does:


  // pass the value of R13 which contains the bcp for the top 
level frame

  Address bcp = context.getRegisterAsAddress(AMD64ThreadContext.R13);
  return new X86Frame(guesser.getSP(), guesser.getFP(), 
guesser.getPC(), null, bcp <-- live_bcp field);


So in this case the X86Frame is constructed with R13 stored in the 
live_bcp field. For any frame other than the current frame, the 
live_bcp field will be NULL.


And one other bit of clarification before moving on to the bug. You'll 
see BCX in code. For example, INTERPRETER_FRAME_BCX_OFFSET. BCX == 
BCP. I'm not sure why BCX was ever chosen. I was tempted to rename it 
to BCP, but it's been cloned to all the other SA CPU ports, so I think 
it's best to just leave it.


Now on to the bug. Sometimes the interpreter uses R13 as a scratch 
register, so it does not always contain the BCP. This is why sometimes 
tests will see an "illegal bci" assert from SA. It's because r13 is 
currently not pointing to the BCP of the current frame, and thus can't 
be converted to a BCI.


What this fix does take advantage of the fact that when r13 is used as 
a scratch register, it is first flushed to frame->bcp. So with this 
fix R13 is only used as the BCP if it points within the Method's 
bytecodes. If it does not, then SA will use the BCP flushed to 
frame->bcp. The reason we don't always used frame->bcp is because it 
is not kept up to date as the interpreter moves between bytecodes, but 
we know it is up to date if R13 is currently being used as a scratch 
register. So in other words, an invalid R13 implies that frame->bcp is 
up to date.


And if you find yourself thinking "X86Frame.java supports is for both 
32-bit and 64-bit x86, but R13 does not exists on 32-bit.", your 
right. A couple of years ago the concept of live_bcp was introduced to 
fix JDK-8214226 [1]. It only added support for fixing 64-bit and 
ignored 32-bit. So you'll never see live_bcp getting set for 32-bit, 
and thus JDK-8214226 is still broken on 32-bit, but it also means 
32-bit is not impacted by the bug I'm fixing here. So the fix for 
JDK-8214226 is why you see R13 references in the X86Frame.java, but 
the code will still work for any platform that sets up live_bcd 
properly, even if it's not actually R13.


...and one more thing. 8214226 [1] actually introduced this "illegal 
bci" bug. However 8214226 [1] was only ever applied to LinuxAMD64. 
Never for BSD or Windows. This means two things: (1) the "illegal bci" 
bug never turned up on these platforms, and (2) 8214226 is still 
broken on these platform, meaning the bci returned by 
getInterpreterFrameBCI() is likely often stale, meaning stack traces 
will not show the proper line number for the current frame. It seems 
we don't have a test to catch this. I've filed 8243500 [2] to cover 
fixing 8214226 on BSD and Windows.


[1] https://bugs.openjdk.java.net/browse/JDK-8214226
[2] https://bugs.openjdk.java.net/browse/JDK-8243500

thanks,

Chris

Re: RFR: JDK-8242522: Minor LingeredApp improvements

[Alex Menkov]

updated webrev:
http://cr.openjdk.java.net/~amenkov/jdk15/LingeredApp_improve/webrev.2/

Moved getStdoutAsList to OutputBuffer.

--alex

On 04/24/2020 20:38, Chris Plummer wrote:

On 4/24/20 6:52 PM, Alex Menkov wrote:

Hi Chris,

On 04/24/2020 15:42, Chris Plummer wrote:

Hi Alex,

Overall it looks good, although I'm not so sure I agree with removing 
getAppOutput(). It's a generally useful utility API. Seems it is 
better off in LingeredApp.java rather than in the one test that 
currently uses it.


To me the method just adds noise to the class.
If you think it can be useful for some other tests, I'd move it to to 
OutputBuffer (make it default method which uses 
OutputBuffer.getStdout()):


default public List getStdOutAsList()
I think that's a good idea. It looks like what tests are commonly doing 
now is using String.split():


     String[] appLines   = 
app.getOutput().getStdout().split("\\R");


I'm not sure of the pros and cons of producing a String[] this way vs a 
List using getStdOutAsList(). Maybe both have their uses. But 
one thing for sure is the split() code is a lot more readable. One 
reason I prefer getStdOutAsList() being placed in a library or utility 
class is because TBH I find Streams code very hard to read, and 
combining with chained Reader code doesn't help any, so I just assume it 
be in a library that I'm less apt to look at rather than in the test 
where I'm bound to find my eyes glazing over as I'm drawn in to figure 
it out.


Chris


--alex



thanks,

Chris

On 4/24/20 3:17 PM, Alex Menkov wrote:

Hi all,

Please review the fix for
https://bugs.openjdk.java.net/browse/JDK-8242522
webrev:
http://cr.openjdk.java.net/~amenkov/jdk15/LingeredApp_improve/webrev/

The fix contains minor fixes/improvements for LingeredApp and tests 
which use it. See jira for details.


Tested all tests which use LingeredApp.

--alex

RFR(T) : 8243929 : use @requires in serviceability/attach/AttachWithStalePidFile.java test

[Igor Ignatyev]

http://cr.openjdk.java.net/~iignatyev//8243929/webrev.00
> 7 lines changed: 1 ins; 6 del; 0 mod;

Hi all,

could you please review this trivial patch which updates 
AttachWithStalePidFile.java test to use @requires?
from JBS:
> serviceability/attach/AttachWithStalePidFile.java test can be run on windows 
> and checks platform before executing any actual testing code. the modern 
> faster and cleaner way to do it is using @requires.

JBS: https://bugs.openjdk.java.net/browse/JDK-8243929
webrev: http://cr.openjdk.java.net/~iignatyev//8243929/webrev.00

Thanks,
-- Igor
 

RFR(T) : 8243928 : several svc tests can be run in driver mode

[Igor Ignatyev]

http://cr.openjdk.java.net/~iignatyev//8243928/webrev.00
> 16 lines changed: 4 ins; 0 del; 12 mod; 


Hi all,

could you please review the trivial patch which updates 15 svc tests to use a 
driver mode?
from JBS:
> the "main" classes of the tests listed below fork new JVMs and check their 
> outputs, so there is no need to run them w/ external vm flags.<...>

JBS: https://bugs.openjdk.java.net/browse/JDK-8243928
webrev: http://cr.openjdk.java.net/~iignatyev//8243928/webrev.00


Thanks,
-- Igor
 

Re: Fwd: RFR(S) 8231634: SA stack walking fails with "illegal bci"

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

  
  
Hi Chris,
  
  The fix looks reasonable.
  
  Thanks,
  Serguei
  
  
  On 4/27/20 12:16, Chris Plummer wrote:

 Ping!
  Please help review if you can. There's also an RFR out for
  8243500, which is related.
  
  thanks,
  
  Chris
  
 Forwarded Message 

  

  Subject: 
  RFR(S) 8231634: SA stack walking fails with "illegal
bci"


  Date: 
  Thu, 23 Apr 2020 14:35:49 -0700


  From: 
  Chris Plummer 


  To: 
  serviceability-dev 

  



Hello,

Please help review the following:

https://bugs.openjdk.java.net/browse/JDK-8231634
http://cr.openjdk.java.net/~cjplummer/8231634/webrev.00/index.html

The fix is fairly simple: Don't use R13 as the BCP if it does
not point into the methods actual bytecodes. Instead rely on the
BCP value flushed to the frame. Details below are mostly there
to help you understand the big picture of what is going on.

First a bit a background on what the live_bcp field is in the
X86Frame. BCP is "bytecode pointer". It points to the current
bytecode being executed. I'm mostly just talking about
interpreter frames here, but I think for compiled frames it
points to the current native instruction being executed. For all
but the current frame, frame->bcp reliably contains the BCP.
For the current frame you normally need to look in a register
for an up to date BCP. On x86_64 that's R13. When trying to find
and create the current frame for a thread, eventually you end up
in LinuxAMD64JavaThreadPDAccess.getCurrentFrameGuess(), which
does:

  // pass the value of R13 which contains the bcp for the
top level frame
  Address bcp =
context.getRegisterAsAddress(AMD64ThreadContext.R13);
  return new X86Frame(guesser.getSP(), guesser.getFP(),
guesser.getPC(), null, bcp <-- live_bcp field);

So in this case the X86Frame is constructed with R13 stored in
the live_bcp field. For any frame other than the current frame,
the live_bcp field will be NULL.

And one other bit of clarification before moving on to the bug.
You'll see BCX in code. For example,
INTERPRETER_FRAME_BCX_OFFSET. BCX == BCP. I'm not sure why BCX
was ever chosen. I was tempted to rename it to BCP, but it's
been cloned to all the other SA CPU ports, so I think it's best
to just leave it.

Now on to the bug. Sometimes the interpreter uses R13 as a
scratch register, so it does not always contain the BCP. This is
why sometimes tests will see an "illegal bci" assert from SA.
It's because r13 is currently not pointing to the BCP of the
current frame, and thus can't be converted to a BCI.

What this fix does take advantage of the fact that when r13 is
used as a scratch register, it is first flushed to
frame->bcp. So with this fix R13 is only used as the BCP if
it points within the Method's bytecodes. If it does not, then SA
will use the BCP flushed to frame->bcp. The reason we don't
always used frame->bcp is because it is not kept up to date
as the interpreter moves between bytecodes, but we know it is up
to date if R13 is currently being used as a scratch register. So
in other words, an invalid R13 implies that frame->bcp is up
to date.

And if you find yourself thinking "X86Frame.java supports is for
both 32-bit and 64-bit x86, but R13 does not exists on 32-bit.",
your right. A couple of years ago the concept of live_bcp was
introduced to fix JDK-8214226 [1]. It only added support for
fixing 64-bit and ignored 32-bit. So you'll never see live_bcp
getting set for 32-bit, and thus JDK-8214226 is still broken on
32-bit, but it also means 32-bit is not impacted by the bug I'm
fixing here. So the fix for JDK-8214226 is why you see R13
references in the X86Frame.java, but the code will still work
for any platform that sets up live_bcd properly, even if it's
not actually R13.

...and one more thing. 8214226 [1] actually introduced this
"illegal bci" bug. However 8214226 [1] was only ever applied to
LinuxAMD64. Never for BSD or Windows. This means two things: (1)
the "illegal bci" bug never turned up on these platforms, and
(2) 8214226 is still broken on these platform, meaning the bci

Re: RFR (S): 8242237: Improve JVM TI HiddenClasses tests

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

Thanks a lot, Alex!
Serguei


On 4/27/20 14:24, Alex Menkov wrote:

Hi Serguei,

LGTM++

--alex

On 04/24/2020 15:20, serguei.spit...@oracle.com wrote:

Hi Leonid,

Thank you for review and suggestion.
Will fix it.

Thanks,
Serguei


On 4/24/20 15:11, Leonid Mesnik wrote:

Looks good.

The small nit (optional). New function
  287 static void JNICALL
  288 ClassPrepare(jvmtiEnv* jvmti, JNIEnv* jni, jthread thread, 
jclass klass) {


is very similar to ClassLoad, and verification of signature might be 
moved into common function.


Leonid

On Apr 24, 2020, at 11:31 AM, serguei.spit...@oracle.com 
 wrote:


Please, review a fix for the sub-task:
https://bugs.openjdk.java.net/browse/JDK-8242237

Webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-hidden-test-update.1/ 



Summary:
  The test update includes:
   - interface and method renaming: Test => HCInterf, test() => 
hcMethod()

   - readClassFile replaced with Files.readAllBytes
   - added ClassPrepare event
   - removed unneeded capability can_get_source_file_name
   - added more comments

Testing:
  Tested locally on Linux, mach5 test run is in progress:
https://mach5.us.oracle.com/mdash/jobs/sspitsyn-HiddenClass-jvmti-test-20200424-1829-10485216 



Thanks,
Serguei

Re: RFR (S): 8242237: Improve JVM TI HiddenClasses tests

[Alex Menkov]

Hi Serguei,

LGTM++

--alex

On 04/24/2020 15:20, serguei.spit...@oracle.com wrote:

Hi Leonid,

Thank you for review and suggestion.
Will fix it.

Thanks,
Serguei


On 4/24/20 15:11, Leonid Mesnik wrote:

Looks good.

The small nit (optional). New function
  287 static void JNICALL
  288 ClassPrepare(jvmtiEnv* jvmti, JNIEnv* jni, jthread thread, jclass klass) {

is very similar to ClassLoad, and verification of signature might be 
moved into common function.


Leonid

On Apr 24, 2020, at 11:31 AM, serguei.spit...@oracle.com 
 wrote:


Please, review a fix for the sub-task:
https://bugs.openjdk.java.net/browse/JDK-8242237

Webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-hidden-test-update.1/

Summary:
  The test update includes:
   - interface and method renaming: Test => HCInterf, test() => 
hcMethod()

   - readClassFile replaced with Files.readAllBytes
   - added ClassPrepare event
   - removed unneeded capability can_get_source_file_name
   - added more comments

Testing:
  Tested locally on Linux, mach5 test run is in progress:
https://mach5.us.oracle.com/mdash/jobs/sspitsyn-HiddenClass-jvmti-test-20200424-1829-10485216

Thanks,
Serguei

Re: RFR: 8242009: Review setting test.java/vm.opts in jcmd/jhsdb and debugger in serviceability tests

[Chris Plummer]

Hi Daniil,

Overall it looks good. A few comments below.

Can you add a comment to TestSysProps.java indicating the reason for 
checking if the line starts with "[".


In JDKToolLauncher you have an extra empty line:

 112  * Any platform specific arguments required for running the 
tool are

 113  * automatically added.
 114  *
 115  *
 116  * @param args

In OutputAnalyzer.java, I wonder if all these matching APIs you updated 
should by default just include the version output in their filtering.


As for the added time to execute, I would suggest possibly stripping 
-Xcomp from the few outliers, and I would mostly focus on how much 
longer it takes, not how many times longer. For example, increasing from 
10 seconds to 40 seconds is not a big deal. Increasing from 10 minutes 
to 20 minutes is.


SADebugDTest creates 8 tool processes so, that's probably the reason for 
the big increase, although I'm not sure why it is kind of slow in the 
first place. It does nothing more on each iteration than launch "jhsdb 
debugd", which will connect to the debuggee, and then is killed off. 
Maybe there is something slow with connecting, especial with RMI.


thanks,

Chris

On 4/27/20 12:07 PM, Daniil Titov wrote:

Please review the change [1] that ensures that VM and test options are 
forwarded to
j*-tools when they are launched from serviceability/sa tests.

The tests that expect an empty output  were corrected to ignore the product 
version printed
in the error stream since in some  tiers the tests are run with ' -showversion' 
VM option (tier3).

In test serviceability/sa/TestSysProps.java the code that counts the system 
properties  was  corrected
to ignore the debug output when the test is run with " -Xlog:cds=debug" option 
(tier4).

Testing:  Mach5 tests for tier1 - tier7 passed.

I also run the test with -XComp at Mach5 linux-x64-debug builds before and 
after the changes
and for  the most of the tests the  overhead is about 2 times although for
serviceability/sa/sadebugd/SADebugDTest.java it spikes up to 5 times.  Probably 
at least for some tests
it makes to filter out some properties (e.g. -Xcomp) before forwarding them to 
j*-tools.

serviceability/sa/sadebugd/SADebugDTest.java, before : 2m 23s ,  after:11m 07s
serviceability/sa/sadebugd/TestJmapCore.java,  before : 42s ,  after:1m 09s
serviceability/sa/TestSysProps.java, before : 36s ,  after: 1m 27s


[1] http://cr.openjdk.java.net/~dtitov/8242009/webrev.01
[2] https://bugs.openjdk.java.net/browse/JDK-8242009

Thank you,
Daniil

Re: RFR: 8242239: [Graal] javax/management/generified/GenericTest.java fails: FAILED: queryMBeans sets same

[Daniil Titov]

Thank you, Chris and Serguei, for reviewing this change.

I will fix typos before pushing in the repository.

Best regards,
Daniil

On 4/27/20, 12:12 PM, "Chris Plummer"  wrote:

Hi Daniil,

   83 // names2, and names3 will have different size. Repeat 
the test in this case.

Should be "sizes". There are a few instances of this in the comments 
that need to be changed.

Looks good otherwise. I don't need to see another webrev.

thanks,

Chris

On 4/25/20 9:11 AM, Daniil Titov wrote:
> Hi Serguei,
>
> Please review a new version of the  webrev [1] that changes the condition
> as you suggested. Please note then in both cases we need break from the 
loop : the case when
> it is a first attempt and the conditions are met and  the case when it is 
a second attempt.
>
>
> [1] http://cr.openjdk.java.net/~dtitov/8242239/webrev.03
> [2] https://bugs.openjdk.java.net/browse/JDK-8242239
>
> Thank you,
> Daniil
>
>
> From: "serguei.spit...@oracle.com" 
> Date: Saturday, April 25, 2020 at 12:06 AM
> To: Daniil Titov , Chris Plummer 
, serviceability-dev 

> Subject: Re: RFR: 8242239: [Graal] 
javax/management/generified/GenericTest.java fails: FAILED: queryMBeans sets 
same
>
> Hi Daniil,
>
> Thank you for the update.
>
> On 4/24/20 22:22, Daniil Titov wrote:
> Hi Chris and Serguei,
>
> Please review a new version of the fix [1] that makes the tests  try to 
repeat the check only once.
>
> Regarding the question Serguei asked:
>
>   97 while(true) {
> 113 if (mbeanCount * 2 == counterCount || isSecondAttempt) {
>   114 assertEquals(mbeanCount * 2, counterCount);
> I doubt, the assert is really needed.
> we need the assert here to make the test fail in case if it is a second 
attempt and the conditions are not met.
>
> A space is still needed in "while(true)."
>   111 if (mbeanCount * 2 == counterCount || isSecondAttempt) {
>   112 assertEquals(mbeanCount * 2, counterCount);
>   113 break;
>   114 }
> The code above is a little confusing as we have to logically separate the 
assert and break cases.
> Would something like below be more straightforward?:
>   if (mbeanCount * 2 == counterCount) {
>   break;
>   }
>   if (isSecondAttempt) {
>   assertEquals(mbeanCount * 2 != counterCount);
>   }
>
> Otherwise, the update looks good.
>
> Thanks,
> Serguei
>
>
> [1] http://cr.openjdk.java.net/~dtitov/8242239/webrev.02/
> [2] https://bugs.openjdk.java.net/browse/JDK-8242239
>
> Thank you,
> Daniil
>
>
> From: serviceability-dev 
mailto:serviceability-dev-boun...@openjdk.java.net on behalf of Daniil Titov 
mailto:daniil.x.ti...@oracle.com
> Date: Friday, April 24, 2020 at 2:08 PM
> To: Chris Plummer mailto:chris.plum...@oracle.com, 
mailto:serguei.spit...@oracle.com mailto:serguei.spit...@oracle.com, 
serviceability-dev mailto:serviceability-dev@openjdk.java.net
> Subject: Re: RFR: 8242239: [Graal] 
javax/management/generified/GenericTest.java fails: FAILED: queryMBeans sets 
same
>
> Hi Chris,
>   
> I agree with you. I will change the test to retry just once.
>   
> Thank you,
> Daniil
>   
>   
> From: Chris Plummer mailto:chris.plum...@oracle.com
> Date: Friday, April 24, 2020 at 1:27 PM
> To: Daniil Titov mailto:daniil.x.ti...@oracle.com, 
mailto:serguei.spit...@oracle.com mailto:serguei.spit...@oracle.com, 
serviceability-dev mailto:serviceability-dev@openjdk.java.net
> Subject: Re: RFR: 8242239: [Graal] 
javax/management/generified/GenericTest.java fails: FAILED: queryMBeans sets 
same
>   
> Hi Daniil,
>
> I think a retry count more in line with our current expectations would 
make more sense since it is deterministic how many retries are might actually 
be needed. You just used a large number in case more “lazy-registered” mbeans 
are added in the future, but if this is the case then maybe even 10 is not 
enough.
>
> thanks,
>
> Chris
>
> On 4/24/20 12:36 PM, Daniil Titov wrote:
> Hi Serguei and Chris,
>   
> Thank you for your comments.
>   
> I wanted to make the fix  more generic  and not limit it just to Graal 
management bean. In this case we could expect that after the first retry is 
triggered by Graal MBean registration some other “lazy-registered” MBean got 
registered and the test might fail. To avoid this we need to allow test to make 
at least several retry attempts before failing.  If number 10 looks as too high 
we could make it 5 or even 3. This should not affect the test run-time unless 
the test starts failing for some other reason than we expect.  In the new 

Re: RFR(M) 8243500: SA: Incorrect BCI and Line Number with jstack if the top frame is in the interpreter (BSD and Windows)

[Chris Plummer]

Ping! Please help review if you can.

thanks,

Chris

On 4/24/20 12:44 AM, Chris Plummer wrote:

Hello,

Please review the following:

https://bugs.openjdk.java.net/browse/JDK-8243500
http://cr.openjdk.java.net/~cjplummer/8243500/webrev.00/index.html

A couple years ago JDK-8214226 fixed an issue on Linux-x64 with SA 
stack dumps not properly displaying the correct line number for the 
topmost frame if it was interpreted. The issue was that SA was always 
relying on frame->bcp when in fact the BCP is kept in R13, and only 
flushed to frame->bcp when needed as a scratch register. So this means 
that SA was in most cases grabbing a stale value from frame->bcp.


The fix for JDK-8214226 was mostly made in X86Frame.java to support 
using the BCP register for the topmost frame instead using frame->bcp. 
This fix actually had a bug in it that was causing the "illegal bci" 
failures we've been seeing. There is already a separate webrev and RFR 
out for that:


https://bugs.openjdk.java.net/browse/JDK-8231634
http://cr.openjdk.java.net/~cjplummer/8231634/webrev.00/index.html

What this RFR addresses is the fact that part of the fix for 
JDK-8214226 was in LinuxAMD64JavaThreadPDAccess.java, but the same 
changes were never made to WindowsAMD64JavaThreadPDAccess.java or 
BsdAMD64JavaThreadPDAccess.java. This fix addresses those two ports. 
Here's the CR and changeset for reference:


https://bugs.openjdk.java.net/browse/JDK-8214226
http://hg.openjdk.java.net/jdk/jdk/rev/9a73a4e4011f

The changes for the fix are pretty trivial. The more complicated part 
is the test I added that will reproduce the issue 100% of the time on 
platforms where SA does not properly check the BCP register. For this 
reason I've used @requires to limit running this test on just those 
platforms I know have the support in place. The test has pretty good 
comments on how it works, so I won't go into details here.


thanks,

Chris

Fwd: RFR(S) 8231634: SA stack walking fails with "illegal bci"

[Chris Plummer]

  
  
Ping! Please help review if you can. There's also an RFR out for
8243500, which is related.

thanks,

Chris

   Forwarded Message 
  

  
Subject:

RFR(S) 8231634: SA stack walking fails with "illegal
  bci"
  
  
Date: 
Thu, 23 Apr 2020 14:35:49 -0700
  
  
From: 
Chris Plummer 
  
  
To: 
serviceability-dev
  
  

  
  
  
  Hello,
  
  Please help review the following:
  
  https://bugs.openjdk.java.net/browse/JDK-8231634
  http://cr.openjdk.java.net/~cjplummer/8231634/webrev.00/index.html
  
  The fix is fairly simple: Don't use R13 as the BCP if it does not
  point into the methods actual bytecodes. Instead rely on the BCP
  value flushed to the frame. Details below are mostly there to help
  you understand the big picture of what is going on.
  
  First a bit a background on what the live_bcp field is in the
  X86Frame. BCP is "bytecode pointer". It points to the current
  bytecode being executed. I'm mostly just talking about interpreter
  frames here, but I think for compiled frames it points to the
  current native instruction being executed. For all but the current
  frame, frame->bcp reliably contains the BCP. For the current
  frame you normally need to look in a register for an up to date
  BCP. On x86_64 that's R13. When trying to find and create the
  current frame for a thread, eventually you end up in
  LinuxAMD64JavaThreadPDAccess.getCurrentFrameGuess(), which does:
  
    // pass the value of R13 which contains the bcp for the top
  level frame
    Address bcp =
  context.getRegisterAsAddress(AMD64ThreadContext.R13);
    return new X86Frame(guesser.getSP(), guesser.getFP(),
  guesser.getPC(), null, bcp <-- live_bcp field);
  
  So in this case the X86Frame is constructed with R13 stored in the
  live_bcp field. For any frame other than the current frame, the
  live_bcp field will be NULL.
  
  And one other bit of clarification before moving on to the bug.
  You'll see BCX in code. For example, INTERPRETER_FRAME_BCX_OFFSET.
  BCX == BCP. I'm not sure why BCX was ever chosen. I was tempted to
  rename it to BCP, but it's been cloned to all the other SA CPU
  ports, so I think it's best to just leave it.
  
  Now on to the bug. Sometimes the interpreter uses R13 as a scratch
  register, so it does not always contain the BCP. This is why
  sometimes tests will see an "illegal bci" assert from SA. It's
  because r13 is currently not pointing to the BCP of the current
  frame, and thus can't be converted to a BCI.
  
  What this fix does take advantage of the fact that when r13 is
  used as a scratch register, it is first flushed to frame->bcp.
  So with this fix R13 is only used as the BCP if it points within
  the Method's bytecodes. If it does not, then SA will use the BCP
  flushed to frame->bcp. The reason we don't always used
  frame->bcp is because it is not kept up to date as the
  interpreter moves between bytecodes, but we know it is up to date
  if R13 is currently being used as a scratch register. So in other
  words, an invalid R13 implies that frame->bcp is up to date.
  
  And if you find yourself thinking "X86Frame.java supports is for
  both 32-bit and 64-bit x86, but R13 does not exists on 32-bit.",
  your right. A couple of years ago the concept of live_bcp was
  introduced to fix JDK-8214226 [1]. It only added support for
  fixing 64-bit and ignored 32-bit. So you'll never see live_bcp
  getting set for 32-bit, and thus JDK-8214226 is still broken on
  32-bit, but it also means 32-bit is not impacted by the bug I'm
  fixing here. So the fix for JDK-8214226 is why you see R13
  references in the X86Frame.java, but the code will still work for
  any platform that sets up live_bcd properly, even if it's not
  actually R13.
  
  ...and one more thing. 8214226 [1] actually introduced this
  "illegal bci" bug. However 8214226 [1] was only ever applied to
  LinuxAMD64. Never for BSD or Windows. This means two things: (1)
  the "illegal bci" bug never turned up on these platforms, and (2)
  8214226 is still broken on these platform, meaning the bci
  returned by getInterpreterFrameBCI() is likely often stale,
  meaning stack traces will not show the proper line number for the
  current frame. It seems we don't have a test to catch this. I've
  filed 8243500 [2] to cover fixing 8214226 on BSD and Windows.
  
  [1] https://bugs.openjdk.java.net/browse/JDK-8214226
  [2] 

Re: RFR: 8242239: [Graal] javax/management/generified/GenericTest.java fails: FAILED: queryMBeans sets same

[Chris Plummer]

Hi Daniil,

  83 // names2, and names3 will have different size. Repeat 
the test in this case.


Should be "sizes". There are a few instances of this in the comments 
that need to be changed.


Looks good otherwise. I don't need to see another webrev.

thanks,

Chris

On 4/25/20 9:11 AM, Daniil Titov wrote:

Hi Serguei,

Please review a new version of the  webrev [1] that changes the condition
as you suggested. Please note then in both cases we need break from the loop : 
the case when
it is a first attempt and the conditions are met and  the case when it is a 
second attempt.


[1] http://cr.openjdk.java.net/~dtitov/8242239/webrev.03
[2] https://bugs.openjdk.java.net/browse/JDK-8242239

Thank you,
Daniil


From: "serguei.spit...@oracle.com" 
Date: Saturday, April 25, 2020 at 12:06 AM
To: Daniil Titov , Chris Plummer 
, serviceability-dev 
Subject: Re: RFR: 8242239: [Graal] javax/management/generified/GenericTest.java 
fails: FAILED: queryMBeans sets same

Hi Daniil,

Thank you for the update.

On 4/24/20 22:22, Daniil Titov wrote:
Hi Chris and Serguei,

Please review a new version of the fix [1] that makes the tests  try to repeat 
the check only once.

Regarding the question Serguei asked:

  97 while(true) {
113 if (mbeanCount * 2 == counterCount || isSecondAttempt) {
  114 assertEquals(mbeanCount * 2, counterCount);
I doubt, the assert is really needed.
we need the assert here to make the test fail in case if it is a second attempt 
and the conditions are not met.

A space is still needed in "while(true)."
  111 if (mbeanCount * 2 == counterCount || isSecondAttempt) {
  112 assertEquals(mbeanCount * 2, counterCount);
  113 break;
  114 }
The code above is a little confusing as we have to logically separate the 
assert and break cases.
Would something like below be more straightforward?:
  if (mbeanCount * 2 == counterCount) {
  break;
  }
  if (isSecondAttempt) {
          assertEquals(mbeanCount * 2 != counterCount);
  }

Otherwise, the update looks good.

Thanks,
Serguei


[1] http://cr.openjdk.java.net/~dtitov/8242239/webrev.02/
[2] https://bugs.openjdk.java.net/browse/JDK-8242239

Thank you,
Daniil


From: serviceability-dev mailto:serviceability-dev-boun...@openjdk.java.net on 
behalf of Daniil Titov mailto:daniil.x.ti...@oracle.com
Date: Friday, April 24, 2020 at 2:08 PM
To: Chris Plummer mailto:chris.plum...@oracle.com, 
mailto:serguei.spit...@oracle.com mailto:serguei.spit...@oracle.com, 
serviceability-dev mailto:serviceability-dev@openjdk.java.net
Subject: Re: RFR: 8242239: [Graal] javax/management/generified/GenericTest.java 
fails: FAILED: queryMBeans sets same

Hi Chris,
  
I agree with you. I will change the test to retry just once.
  
Thank you,

Daniil
  
  
From: Chris Plummer mailto:chris.plum...@oracle.com

Date: Friday, April 24, 2020 at 1:27 PM
To: Daniil Titov mailto:daniil.x.ti...@oracle.com, 
mailto:serguei.spit...@oracle.com mailto:serguei.spit...@oracle.com, 
serviceability-dev mailto:serviceability-dev@openjdk.java.net
Subject: Re: RFR: 8242239: [Graal] javax/management/generified/GenericTest.java 
fails: FAILED: queryMBeans sets same
  
Hi Daniil,


I think a retry count more in line with our current expectations would make 
more sense since it is deterministic how many retries are might actually be 
needed. You just used a large number in case more “lazy-registered” mbeans are 
added in the future, but if this is the case then maybe even 10 is not enough.

thanks,

Chris

On 4/24/20 12:36 PM, Daniil Titov wrote:
Hi Serguei and Chris,
  
Thank you for your comments.
  
I wanted to make the fix  more generic  and not limit it just to Graal management bean. In this case we could expect that after the first retry is triggered by Graal MBean registration some other “lazy-registered” MBean got registered and the test might fail. To avoid this we need to allow test to make at least several retry attempts before failing.  If number 10 looks as too high we could make it 5 or even 3. This should not affect the test run-time unless the test starts failing for some other reason than we expect.  In the new webrev I will also correct  the iteration counting (as Chris mentioned) and fix typos.
  
Thanks again,

Daniil
  
From: mailto:serguei.spit...@oracle.com mailto:serguei.spit...@oracle.com

Date: Friday, April 24, 2020 at 11:48 AM
To: Chris Plummer mailto:chris.plum...@oracle.com, Daniil Titov 
mailto:daniil.x.ti...@oracle.com, serviceability-dev 
mailto:serviceability-dev@openjdk.java.net
Subject: Re: RFR: 8242239: [Graal] javax/management/generified/GenericTest.java 
fails: FAILED: queryMBeans sets same
  
Hi Daniil,


Besides what Chris is pointed out the fix looks good.

Minor:
   97 while(true) {
  113 if(mbeanCount * 2 == counterCount || retryCounter++ > 
MAX_RETRY_ATTEMPT) {
  114 assertEquals(mbeanCount * 2, 

RFR: 8242009: Review setting test.java/vm.opts in jcmd/jhsdb and debugger in serviceability tests

[Daniil Titov]

Please review the change [1] that ensures that VM and test options are 
forwarded to 
j*-tools when they are launched from serviceability/sa tests.

The tests that expect an empty output  were corrected to ignore the product 
version printed
in the error stream since in some  tiers the tests are run with ' -showversion' 
VM option (tier3).

In test serviceability/sa/TestSysProps.java the code that counts the system 
properties  was  corrected
to ignore the debug output when the test is run with " -Xlog:cds=debug" option 
(tier4).

Testing:  Mach5 tests for tier1 - tier7 passed.

I also run the test with -XComp at Mach5 linux-x64-debug builds before and 
after the changes 
and for  the most of the tests the  overhead is about 2 times although for 
serviceability/sa/sadebugd/SADebugDTest.java it spikes up to 5 times.  Probably 
at least for some tests
it makes to filter out some properties (e.g. -Xcomp) before forwarding them to 
j*-tools.

serviceability/sa/sadebugd/SADebugDTest.java, before : 2m 23s ,  after:11m 07s
serviceability/sa/sadebugd/TestJmapCore.java,  before : 42s ,  after:1m 09s
serviceability/sa/TestSysProps.java, before : 36s ,  after: 1m 27s


[1] http://cr.openjdk.java.net/~dtitov/8242009/webrev.01
[2] https://bugs.openjdk.java.net/browse/JDK-8242009 

Thank you,
Daniil

Re: RFR(S/T) : 8243568 : serviceability/logging/TestLogRotation.java uses 'test.java.opts' and not 'test.vm.opts'

[Stefan Karlsson]

Looks good.

StefanK

On 2020-04-25 00:30, Igor Ignatyev wrote:

http://cr.openjdk.java.net/~iignatyev//8243568/webrev.00

8 lines changed: 0 ins; 6 del; 2 mod;

Hi all,

could you please review this small and trivial patch which updates 
serviceability/logging/TestLogRotation.java test to pass both 'test.java.opts' 
and not 'test.vm.opts' ? to do that, the patch removes the custom logic of 
handling test.java.opts and just uses 
ProcessTools.createJavaProcessBuilder(boolean addTestVmAndJavaOptions=true, 
String...), which prepends values of both properties.
from JBS:

serviceability/logging/TestLogRotation.java test use 'test.java.opts' to get 
external vm flags, yet actually external flags are split b/w 'test.java.opts' 
and 'test.vm.opts' and in the majority of cases all external flags are set 
listed in 'test.vm.opts' so the test should be updated to read both.

JBS: https://bugs.openjdk.java.net/browse/JDK-8243568
webrev: http://cr.openjdk.java.net/~iignatyev//8243568/webrev.00

Thanks,
-- Igor

RE: RFR(S) 8238585: Use handshake for JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make compiled methods on stack not_entrant

[Reingruber, Richard]

Hi David,

> Not a review but some general commentary ...

That's welcome.

> On 25/04/2020 2:08 am, Reingruber, Richard wrote:
> > Hi Yasumasa, Patricio,
> > 
>  I will send review request to replace VM_SetFramePop to handshake in 
>  early next week in JDK-8242427.
>  Does it help you? I think it gives you to remove workaround.
> >>>
> >>> I think it would not help that much. Note that when replacing 
> >>> VM_SetFramePop with a direct handshake
> >>> you could not just execute VM_EnterInterpOnlyMode as a nested vm 
> >>> operation [1]. So you would have to
> >>> change/replace VM_EnterInterpOnlyMode and I would have to adapt to these 
> >>> changes.
> > 
> >> Thanks for your information.
> >> I tested my patch with both vmTestbase/nsk/jvmti/PopFrame and 
> >> vmTestbase/nsk/jvmti/NotifyFramePop.
> >> I will modify and will test it after yours.
> > 
> > Thanks :)
> > 
> >>> Also my first impression was that it won't be that easy from a 
> >>> synchronization point of view to
> >>> replace VM_SetFramePop with a direct handshake. E.g. 
> >>> VM_SetFramePop::doit() indirectly calls
> >>> JvmtiEventController::set_frame_pop(JvmtiEnvThreadState *ets, 
> >>> JvmtiFramePop fpop) where
> >>> JvmtiThreadState_lock is acquired with safepoint check, if not at 
> >>> safepoint. It's not directly clear
> >>> to me, how this has to be handled.
> > 
> >> I think JvmtiEventController::set_frame_pop() should hold 
> >> JvmtiThreadState_lock because it affects other JVMTI operation especially 
> >> FramePop event.
> > 
> > Yes. To me it is unclear what synchronization is necessary, if it is called 
> > during a handshake. And
> > also I'm unsure if a thread should do safepoint checks while executing a 
> > handshake.

> I'm growing increasingly concerned that use of direct handshakes to 
> replace VM operations needs a much greater examination for correctness 
> than might initially be thought. I see a number of issues:

I agree. I'll address your concerns in the context of this review thread for 
JDK-8238585 below.

In addition I would suggest to take the general part of the discussion to a 
dedicated thread or to
the review thread for JDK-8242427. I would like to keep this thread closer to 
its subject.

> First, the VMThread executes (most) VM operations with a clean stack in 
> a clean state, so it has lots of room to work. If we now execute the 
> same logic in a JavaThread then we risk hitting stackoverflows if 
> nothing else. But we are also now executing code in a JavaThread and so 
> we have to be sure that code is not going to act differently (in a bad 
> way) if executed by a JavaThread rather than the VMThread. For example, 
> may it be possible that if executing in the VMThread we defer some 
> activity that might require execution of Java code, or else hand it off 
> to one of the service threads? If we execute that code directly in the 
> current JavaThread instead we may not be in a valid state (e.g. consider 
> re-entrancy to various subsystems that is not allowed).

It is not too complex, what EnterInterpOnlyModeClosure::do_thread() is doing. I 
already added a
paragraph to the JBS-Item [1] explaining why the direct handshake is sufficient 
from a
synchronization point of view.

Furthermore the stack is walked and the return pc of compiled frames is 
replaced with the address of
the deopt handler.

I can't see why this cannot be done with a direct handshake. Something very 
similar is already done
in JavaThread::deoptimize_marked_methods() which is executed as part of an 
ordinary handshake.

The demand on stack-space should be very modest. I would not expect a higher 
risk for stackoverflow.

> Second, we have this question mark over what happens if the operation 
> hits further safepoint or handshake polls/checks? Are there constraints 
> on what is allowed here? How can we recognise this problem may exist and 
> so deal with it?

The thread in EnterInterpOnlyModeClosure::do_thread() can't become 
safepoint/handshake safe. I
tested locally test/hotspot/jtreg:vmTestbase_nsk_jvmti with a 
NoSafepointVerifier.

> Third, while we are generally considering what appear to be 
> single-thread operations, which should be amenable to a direct 
> handshake, we also have to be careful that some of the code involved 
> doesn't already expect/assume we are at a safepoint - e.g. a VM op may 
> not need to take a lock where a direct handshake might!

See again my arguments in the JBS item [1].

Thanks,
Richard.

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

-Original Message-
From: David Holmes  
Sent: Montag, 27. April 2020 07:16
To: Reingruber, Richard ; Yasumasa Suenaga 
; Patricio Chilano 
; serguei.spit...@oracle.com; Vladimir 
Ivanov ; serviceability-dev@openjdk.java.net; 
hotspot-compiler-...@openjdk.java.net; hotspot-...@openjdk.java.net; 
hotspot-runtime-...@openjdk.java.net; hotspot-gc-...@openjdk.java.net
Subject: Re: RFR(S) 8238585: Use handshake for 

Re: RFR(S) 8238585: Use handshake for JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make compiled methods on stack not_entrant

[David Holmes]

Hi all,

Not a review but some general commentary ...

On 25/04/2020 2:08 am, Reingruber, Richard wrote:

Hi Yasumasa, Patricio,


I will send review request to replace VM_SetFramePop to handshake in early next 
week in JDK-8242427.
Does it help you? I think it gives you to remove workaround.


I think it would not help that much. Note that when replacing VM_SetFramePop 
with a direct handshake
you could not just execute VM_EnterInterpOnlyMode as a nested vm operation [1]. 
So you would have to
change/replace VM_EnterInterpOnlyMode and I would have to adapt to these 
changes.



Thanks for your information.
I tested my patch with both vmTestbase/nsk/jvmti/PopFrame and 
vmTestbase/nsk/jvmti/NotifyFramePop.
I will modify and will test it after yours.


Thanks :)


Also my first impression was that it won't be that easy from a synchronization 
point of view to
replace VM_SetFramePop with a direct handshake. E.g. VM_SetFramePop::doit() 
indirectly calls
JvmtiEventController::set_frame_pop(JvmtiEnvThreadState *ets, JvmtiFramePop 
fpop) where
JvmtiThreadState_lock is acquired with safepoint check, if not at safepoint. 
It's not directly clear
to me, how this has to be handled.



I think JvmtiEventController::set_frame_pop() should hold JvmtiThreadState_lock 
because it affects other JVMTI operation especially FramePop event.


Yes. To me it is unclear what synchronization is necessary, if it is called 
during a handshake. And
also I'm unsure if a thread should do safepoint checks while executing a 
handshake.


I'm growing increasingly concerned that use of direct handshakes to 
replace VM operations needs a much greater examination for correctness 
than might initially be thought. I see a number of issues:


First, the VMThread executes (most) VM operations with a clean stack in 
a clean state, so it has lots of room to work. If we now execute the 
same logic in a JavaThread then we risk hitting stackoverflows if 
nothing else. But we are also now executing code in a JavaThread and so 
we have to be sure that code is not going to act differently (in a bad 
way) if executed by a JavaThread rather than the VMThread. For example, 
may it be possible that if executing in the VMThread we defer some 
activity that might require execution of Java code, or else hand it off 
to one of the service threads? If we execute that code directly in the 
current JavaThread instead we may not be in a valid state (e.g. consider 
re-entrancy to various subsystems that is not allowed).


Second, we have this question mark over what happens if the operation 
hits further safepoint or handshake polls/checks? Are there constraints 
on what is allowed here? How can we recognise this problem may exist and 
so deal with it?


Third, while we are generally considering what appear to be 
single-thread operations, which should be amenable to a direct 
handshake, we also have to be careful that some of the code involved 
doesn't already expect/assume we are at a safepoint - e.g. a VM op may 
not need to take a lock where a direct handshake might!


Cheers,
David
-


@Patricio, coming back to my question [1]:

In the example you gave in your answer [2]: the java thread would execute a vm 
operation during a
direct handshake operation, while the VMThread is actually in the middle of a 
VM_HandshakeAllThreads
operation, waiting to handshake the same handshakee: why can't the VMThread 
just proceed? The
handshakee would be safepoint safe, wouldn't it?

Thanks, Richard.

[1] 
https://bugs.openjdk.java.net/browse/JDK-8230594?focusedCommentId=14301677=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14301677

[2] 
https://bugs.openjdk.java.net/browse/JDK-8230594?focusedCommentId=14301763=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14301763

-Original Message-
From: Yasumasa Suenaga 
Sent: Freitag, 24. April 2020 17:23
To: Reingruber, Richard ; Patricio Chilano 
; serguei.spit...@oracle.com; Vladimir Ivanov 
; serviceability-dev@openjdk.java.net; 
hotspot-compiler-...@openjdk.java.net; hotspot-...@openjdk.java.net; 
hotspot-runtime-...@openjdk.java.net; hotspot-gc-...@openjdk.java.net
Subject: Re: RFR(S) 8238585: Use handshake for 
JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make compiled 
methods on stack not_entrant

Hi Richard,

On 2020/04/24 23:44, Reingruber, Richard wrote:

Hi Yasumasa,


I will send review request to replace VM_SetFramePop to handshake in early next 
week in JDK-8242427.
Does it help you? I think it gives you to remove workaround.


I think it would not help that much. Note that when replacing VM_SetFramePop 
with a direct handshake
you could not just execute VM_EnterInterpOnlyMode as a nested vm operation [1]. 
So you would have to
change/replace VM_EnterInterpOnlyMode and I would have to adapt to these 
changes.


Thanks for your information.
I tested my patch with both vmTestbase/nsk/jvmti/PopFrame and 

Re: RFR(L): 8215624: add parallel heap inspection support for jmap histo(G1)(Internet mail)

[Stefan Karlsson]

Hi Lin,

On 2020-04-26 05:10, linzang(臧琳) wrote:

Hi Stefan and Paul，
 I have made a new patch based on your comments and Stefan's Poc code:
 Webrev: http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_03/
 Delta(based on Stefan's change:) : 
http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_03-delta/webrev_03-delta/


Thanks for providing a delta patch. It makes it much easier to look at, 
and more likely for reviewers to continue reviewing.


I'm going to continue focusing on the GC parts, and leave the rest to 
others to review.




 And Here are main changed I made and want to discuss with you:
 1.  changed"parallelThreadNum=" to "parallel=" for jmap -histo options.
 2.  Add logic to test where parallelHeapInspection is fail, in 
heapInspection.cpp
   This is because the parHeapInspectTask create thread local 
KlassInfoTable in it's work() method, and this may fail because of native OOM, 
in this case, the parallel should fail and serial heap inspection can be tried.
   One more thing I want discuss with you is about the member "_success" of 
parHeapInspectTask, when native OOM happenes, it is set to false. And since this "set" operation 
can be conducted in multiple threads, should it be atomic ops?  IMO, this is not necessary because 
"_success" can only be set to false, and there is no way to change it from back to true after the 
ParHeapInspectTask instance is created, so it is save to be non-atomic, do you agree with that?


In these situations you should be using the Atomic::load/store 
primitives. We're moving toward a later C++ standard were data races are 
considered undefined behavior.



3. make CollectedHeap::run_task() be an abstract virtual func, so that every subclass 
of collectedHeap should support it, so later implementation of new collectedHeap will not 
miss the "parallel" features.
  The problem I want to discuss with you is about epsilonHeap and 
SerialHeap, as they may not need parallel heap iteration, so I only make 
task->work(0), in case the run_task() is invoked someway in future. Another way 
is to left run_task()  unimplemented, which one do you think is better?


I don't have a strong opinion about this.

 And also please help take a look at the zHeap, as there is a class 
zTask that wrap the abstractGangTask, and the collectedHeap::run_task() 
only accept  AbstraceGangTask* as argument, so I made a delegate class 
to adapt it , please see src/hotspot/share/gc/z/zHeap.cpp.


   There maybe other better ways to sovle the above problems, welcome for 
any comments, Thanks!


I've created a few cleanups and changes on top of your latest patch:

https://cr.openjdk.java.net/~stefank/8215624/webrev.02.delta
https://cr.openjdk.java.net/~stefank/8215624/webrev.02

- Adding Atomic::load/store.
- Removing the time measurement in the run_task. I renamed G1's function 
to run_task_timed. If we need this outside of G1, we can rethink the API 
at that point.

- ZGC style cleanups

Thanks,
StefanK



BRs,
Lin

On 2020/4/23, 11:08 AM, "linzang(臧琳)"  wrote:

 Thanks Paul! I agree with using "parallel", will make the update in next 
patch, Thanks for help update the CSR.

 BRs,
 Lin

 On 2020/4/23, 4:42 AM, "Hohensee, Paul"  wrote:

 For the interface, I'd use "parallel" instead of "parallelThreadNum". All the 
other options are lower case, and it's a lot easier to type "parallel". I took the liberty of 
updating the CSR. If you're ok with it, you might want to change variable names and such, plus of course 
JMap.usage.

 Thanks,
 Paul

 On 4/22/20, 2:29 AM, "serviceability-dev on behalf of linzang(臧琳)" 
 wrote:

 Dear Stefan,

 Thanks a lot! I agree with you to decouple the heap 
inspection code with GC's.
 I will start  from your POC code, may discuss with you 
later.


 BRs,
 Lin

 On 2020/4/22, 5:14 PM, "Stefan Karlsson" 
 wrote:

 Hi Lin,

 I took a look at this earlier and saw that the heap inspection 
code is
 strongly coupled with the CollectedHeap and G1CollectedHeap. 
I'd prefer
 if we'd abstract this away, so that the GCs only provide a 
"parallel
 object iteration" interface, and the heap inspection code is 
kept elsewhere.

 I started experimenting with doing that, but other 
higher-priority (to
 me) tasks have had to take precedence.

 I've uploaded my work-in-progress / proof-of-concept:
   https://cr.openjdk.java.net/~stefank/8215624/webrev.01.delta/
   https://cr.openjdk.java.net/~stefank/8215624/webrev.01/

 The current code doesn't handle the lifecycle (deletion) of the
 ParallelObjectIterators. There's also code left unimplemented 
in around