date:20190320

Re: 8218401: WRONG_PHASE: vmTestbase/nsk/jvmti test crash

2019-03-20 Thread serguei.spit...@oracle.com


Hi Daniil,

You are right, there is a hole for this.

It can be easily fixed if the callbacks check phase at the beginning.
But I don't think this would be simpler than your current fix.
As I already mentioned, I'm Okay with the fix you have now.

I'm still thinking on how to fix this on the JVMTI level.

Thanks,
Serguei


On 3/20/19 16:35, Daniil Titov wrote:

Hi Serguei,

I could be missing something but as I understood the suggested alternative 
approach was  to not introduce the callbacksEnabled  flag and instead just 
unregister other callbacks inside VMDeath callback.  My concern was the case 
when one thread is on line 1302 (it is going to call a class loaded callback), 
while another thread just entered VMDeath callback. Without callbacksEnabled   
flag the first thread will enter the callback and wait on the raw monitor 
acquired by VMDeath callback thread, after that it will continue and call JVMTI 
API that might throw WRONG_PHASE error if by this time the second thread  
already returned from the VMDeath callback and changed the JVMTI phase.

1277void JvmtiExport::post_class_load(JavaThread *thread, Klass* klass) {
   1278   if (JvmtiEnv::get_phase() < JVMTI_PHASE_PRIMORDIAL) {
   1279 return;
   1280   }
   1281   HandleMark hm(thread);
   1282 
   1283   EVT_TRIG_TRACE(JVMTI_EVENT_CLASS_LOAD, ("[%s] Trg Class Load 
triggered",
   1284   JvmtiTrace::safe_get_thread_name(thread)));
   1285   JvmtiThreadState* state = thread->jvmti_thread_state();
   1286   if (state == NULL) {
   1287 return;
   1288   }
   1289   JvmtiEnvThreadStateIterator it(state);
   1290   for (JvmtiEnvThreadState* ets = it.first(); ets != NULL; ets = 
it.next(ets)) {
   1291 if (ets->is_enabled(JVMTI_EVENT_CLASS_LOAD)) {
   1292   JvmtiEnv *env = ets->get_env();
   1293   if (env->phase() == JVMTI_PHASE_PRIMORDIAL) {
   1294 continue;
   1295   }
   1296   EVT_TRACE(JVMTI_EVENT_CLASS_LOAD, ("[%s] Evt Class Load sent %s",
   1297  
JvmtiTrace::safe_get_thread_name(thread),
   1298  klass==NULL? "NULL" : 
klass->external_name() ));
   1299   JvmtiClassEventMark jem(thread, klass);
   1300   JvmtiJavaThreadEventTransition jet(thread);
   1301   jvmtiEventClassLoad callback = env->callbacks()->ClassLoad;
   1302   if (callback != NULL) {
   1303 (*callback)(env->jvmti_external(), jem.jni_env(), 
jem.jni_thread(), jem.jni_class());
   1304   }
   1305 }
   1306   }
   1307 }

Thanks!

--Daniil

On 3/20/19, 3:40 PM, "serguei.spit...@oracle.com"  
wrote:

 Hi Daniil,
 
 No chance for it if you lock a raw monitor in the event callbacks.
 
 Thanks,

 Serguei
 
 On 3/20/19 14:58, Daniil Titov wrote:

 > Hi Serguei,
 >
 > I think that just disabling event notifications inside VMDeath callback 
still leaves a small window for VMDeath callback being called after the classload 
callback is called (or about being called)  but before it enters a raw monitor. 
Thus I decided to follow your original suggestion  and restore volatile modifier 
for callbacksEnabled.  Please review a new version of the patch.
 >
 > Webrev: http://cr.openjdk.java.net/~dtitov/8218401/webrev.03/
 > Bug : https://bugs.openjdk.java.net/browse/JDK-8218401
 >
 > Thanks!
 >
 > Best regards,
 > Daniil
 >
 >
 >
 > From: 
 > Organization: Oracle Corporation
 > Date: Tuesday, March 19, 2019 at 7:17 PM
 > To: Daniil Titov , OpenJDK Serviceability 
, Jean Christophe Beyler 
 > Subject: Re: 8218401: WRONG_PHASE: vmTestbase/nsk/jvmti test crash
 >
 > Hi Daniil,
 >
 > I'd keep the volatile modifier for callbacksEnabled to disable compiler 
optimizations.
 > Otherwise, looks good to me.
 >
 >
 > Another approach could be to disable event notifications in VMDeath 
callback with:
 >SetEventNotificationMode(JVMTI_DISABLE, JVMTI_EVENT_CLASS_LOAD, NULL);
 >SetEventNotificationMode(JVMTI_DISABLE, JVMTI_EVENT_BREAKPOINT, NULL);
 >. . .
 >
 > Thanks,
 > Serguei
 >
 > On 3/18/19 6:58 PM, Daniil Titov wrote:
 > Hi Serguei and JC,
 >
 > Please review a new version of the fix that locks a monitor across the 
callbacks, as Serguei suggested.
 >
 > Webrev: http://cr.openjdk.java.net/~dtitov/8218401/webrev.02/
 > Bug: https://bugs.openjdk.java.net/browse/JDK-8218401
 >
 > Thanks!
 > --Daniil
 >
 >
 > On 3/18/19, 9:47 AM, mailto:serguei.spit...@oracle.com 
mailto:serguei.spit...@oracle.com wrote:
 >
 >  Hi Daniil,
 >
 >  The JVMTI phase can change in the middle of callback work after the
 >  check you added.
 >  I'd suggest to lock a raw monitor across the callbacks to make them 
atomic.
 >
 >  Thank you for taking care about this issue!

Re: 8218401: WRONG_PHASE: vmTestbase/nsk/jvmti test crash

2019-03-20 Thread Daniil Titov

Hi Serguei,

I could be missing something but as I understood the suggested alternative 
approach was  to not introduce the callbacksEnabled  flag and instead just 
unregister other callbacks inside VMDeath callback.  My concern was the case 
when one thread is on line 1302 (it is going to call a class loaded callback), 
while another thread just entered VMDeath callback. Without callbacksEnabled   
flag the first thread will enter the callback and wait on the raw monitor 
acquired by VMDeath callback thread, after that it will continue and call JVMTI 
API that might throw WRONG_PHASE error if by this time the second thread  
already returned from the VMDeath callback and changed the JVMTI phase.

1277void JvmtiExport::post_class_load(JavaThread *thread, Klass* klass) {
  1278if (JvmtiEnv::get_phase() < JVMTI_PHASE_PRIMORDIAL) {
  1279  return;
  1280}
  1281HandleMark hm(thread);
  1282
  1283EVT_TRIG_TRACE(JVMTI_EVENT_CLASS_LOAD, ("[%s] Trg Class Load 
triggered",
  1284JvmtiTrace::safe_get_thread_name(thread)));
  1285JvmtiThreadState* state = thread->jvmti_thread_state();
  1286if (state == NULL) {
  1287  return;
  1288}
  1289JvmtiEnvThreadStateIterator it(state);
  1290for (JvmtiEnvThreadState* ets = it.first(); ets != NULL; ets = 
it.next(ets)) {
  1291  if (ets->is_enabled(JVMTI_EVENT_CLASS_LOAD)) {
  1292JvmtiEnv *env = ets->get_env();
  1293if (env->phase() == JVMTI_PHASE_PRIMORDIAL) {
  1294  continue;
  1295}
  1296EVT_TRACE(JVMTI_EVENT_CLASS_LOAD, ("[%s] Evt Class Load sent %s",
  1297   
JvmtiTrace::safe_get_thread_name(thread),
  1298   klass==NULL? "NULL" : 
klass->external_name() ));
  1299JvmtiClassEventMark jem(thread, klass);
  1300JvmtiJavaThreadEventTransition jet(thread);
  1301jvmtiEventClassLoad callback = env->callbacks()->ClassLoad;
  1302if (callback != NULL) {
  1303  (*callback)(env->jvmti_external(), jem.jni_env(), 
jem.jni_thread(), jem.jni_class());
  1304}
  1305  }
  1306}
  1307  }

Thanks!

--Daniil

On 3/20/19, 3:40 PM, "serguei.spit...@oracle.com"  
wrote:

Hi Daniil,

No chance for it if you lock a raw monitor in the event callbacks.

Thanks,
Serguei

On 3/20/19 14:58, Daniil Titov wrote:
> Hi Serguei,
>
> I think that just disabling event notifications inside VMDeath callback 
still leaves a small window for VMDeath callback being called after the 
classload callback is called (or about being called)  but before it enters a 
raw monitor. Thus I decided to follow your original suggestion  and restore 
volatile modifier for callbacksEnabled.  Please review a new version of the 
patch.
>
> Webrev: http://cr.openjdk.java.net/~dtitov/8218401/webrev.03/
> Bug : https://bugs.openjdk.java.net/browse/JDK-8218401
>
> Thanks!
>
> Best regards,
> Daniil
>
>
>
> From: 
> Organization: Oracle Corporation
> Date: Tuesday, March 19, 2019 at 7:17 PM
> To: Daniil Titov , OpenJDK Serviceability 
, Jean Christophe Beyler 

> Subject: Re: 8218401: WRONG_PHASE: vmTestbase/nsk/jvmti test crash
>
> Hi Daniil,
>
> I'd keep the volatile modifier for callbacksEnabled to disable compiler 
optimizations.
> Otherwise, looks good to me.
>
>
> Another approach could be to disable event notifications in VMDeath 
callback with:
>SetEventNotificationMode(JVMTI_DISABLE, JVMTI_EVENT_CLASS_LOAD, NULL);
>SetEventNotificationMode(JVMTI_DISABLE, JVMTI_EVENT_BREAKPOINT, NULL);
>. . .
>
> Thanks,
> Serguei
>
> On 3/18/19 6:58 PM, Daniil Titov wrote:
> Hi Serguei and JC,
>
> Please review a new version of the fix that locks a monitor across the 
callbacks, as Serguei suggested.
>
> Webrev: http://cr.openjdk.java.net/~dtitov/8218401/webrev.02/
> Bug: https://bugs.openjdk.java.net/browse/JDK-8218401
>
> Thanks!
> --Daniil
>
>
> On 3/18/19, 9:47 AM, mailto:serguei.spit...@oracle.com 
mailto:serguei.spit...@oracle.com wrote:
>
>  Hi Daniil,
>  
>  The JVMTI phase can change in the middle of callback work after the
>  check you added.
>  I'd suggest to lock a raw monitor across the callbacks to make them 
atomic.
>  
>  Thank you for taking care about this issue!
>  
>  Thanks,
>  Serguei
>  
>  
>  
>  On 3/15/19 16:08, Daniil Titov wrote:
>  > Please review the change that fixes 3 tests that intermittently 
fail with JVMTI_ERROR_WRONG_PHASE error.
>  >
>  > The problem here is that the callbacks these tests enable keep 
processing events and perform JVMTI calls after VM is terminated. The fix

Re: 8218401: WRONG_PHASE: vmTestbase/nsk/jvmti test crash

2019-03-20 Thread serguei.spit...@oracle.com

Hi Daniil,

No chance for it if you lock a raw monitor in the event callbacks.

Thanks,
Serguei

On 3/20/19 14:58, Daniil Titov wrote:

Hi Serguei,

I think that just disabling event notifications inside VMDeath callback still 
leaves a small window for VMDeath callback being called after the classload 
callback is called (or about being called)  but before it enters a raw monitor. 
Thus I decided to follow your original suggestion  and restore volatile 
modifier for callbacksEnabled.  Please review a new version of the patch.

Webrev: http://cr.openjdk.java.net/~dtitov/8218401/webrev.03/
Bug : https://bugs.openjdk.java.net/browse/JDK-8218401

Thanks!

Best regards,
Daniil

From: 
Organization: Oracle Corporation
Date: Tuesday, March 19, 2019 at 7:17 PM
To: Daniil Titov , OpenJDK Serviceability 
, Jean Christophe Beyler 
Subject: Re: 8218401: WRONG_PHASE: vmTestbase/nsk/jvmti test crash

Hi Daniil,

I'd keep the volatile modifier for callbacksEnabled to disable compiler 
optimizations.
Otherwise, looks good to me.

Another approach could be to disable event notifications in VMDeath callback 
with:
   SetEventNotificationMode(JVMTI_DISABLE, JVMTI_EVENT_CLASS_LOAD, NULL);
   SetEventNotificationMode(JVMTI_DISABLE, JVMTI_EVENT_BREAKPOINT, NULL);
   . . .

Thanks,
Serguei

On 3/18/19 6:58 PM, Daniil Titov wrote:
Hi Serguei and JC,

Please review a new version of the fix that locks a monitor across the 
callbacks, as Serguei suggested.

Webrev: http://cr.openjdk.java.net/~dtitov/8218401/webrev.02/
Bug: https://bugs.openjdk.java.net/browse/JDK-8218401

Thanks!
--Daniil

On 3/18/19, 9:47 AM, mailto:serguei.spit...@oracle.com 
mailto:serguei.spit...@oracle.com wrote:

 Hi Daniil,

 The JVMTI phase can change in the middle of callback work after the

 check you added.
 I'd suggest to lock a raw monitor across the callbacks to make them atomic.

 Thank you for taking care about this issue!

 Thanks,

 Serguei

 On 3/15/19 16:08, Daniil Titov wrote:

 > Please review the change that fixes 3 tests that intermittently fail 
with JVMTI_ERROR_WRONG_PHASE error.
 >
 > The problem here is that the callbacks these tests enable keep 
processing events and perform JVMTI calls after VM is terminated. The fix makes 
these test listen for VMDeath event and  quick return from the callbacks after 
VMDeath event is received.
 >
 > Webrev: http://cr.openjdk.java.net/~dtitov/8218401/webrev.01/
 > Bug: https://bugs.openjdk.java.net/browse/JDK-8218401
 >
 > Thanks!
 > -Daniil
 >
 >

Re: 8218401: WRONG_PHASE: vmTestbase/nsk/jvmti test crash

2019-03-20 Thread Daniil Titov

Hi Serguei,

I think that just disabling event notifications inside VMDeath callback still 
leaves a small window for VMDeath callback being called after the classload 
callback is called (or about being called)  but before it enters a raw monitor. 
Thus I decided to follow your original suggestion  and restore volatile 
modifier for callbacksEnabled.  Please review a new version of the patch.

Webrev: http://cr.openjdk.java.net/~dtitov/8218401/webrev.03/ 
Bug : https://bugs.openjdk.java.net/browse/JDK-8218401 

Thanks!

Best regards,
Daniil

From: 
Organization: Oracle Corporation
Date: Tuesday, March 19, 2019 at 7:17 PM
To: Daniil Titov , OpenJDK Serviceability 
, Jean Christophe Beyler 

Subject: Re: 8218401: WRONG_PHASE: vmTestbase/nsk/jvmti test crash

Hi Daniil,

I'd keep the volatile modifier for callbacksEnabled to disable compiler 
optimizations.
Otherwise, looks good to me.

Another approach could be to disable event notifications in VMDeath callback 
with:
  SetEventNotificationMode(JVMTI_DISABLE, JVMTI_EVENT_CLASS_LOAD, NULL);
  SetEventNotificationMode(JVMTI_DISABLE, JVMTI_EVENT_BREAKPOINT, NULL);
  . . .

Thanks,
Serguei

On 3/18/19 6:58 PM, Daniil Titov wrote:
Hi Serguei and JC,

Please review a new version of the fix that locks a monitor across the 
callbacks, as Serguei suggested.

Webrev: http://cr.openjdk.java.net/~dtitov/8218401/webrev.02/ 
Bug: https://bugs.openjdk.java.net/browse/JDK-8218401 

Thanks!
--Daniil

On 3/18/19, 9:47 AM, mailto:serguei.spit...@oracle.com 
mailto:serguei.spit...@oracle.com wrote:

Hi Daniil,

The JVMTI phase can change in the middle of callback work after the 
check you added.
I'd suggest to lock a raw monitor across the callbacks to make them atomic.

Thank you for taking care about this issue!

Thanks,
Serguei

On 3/15/19 16:08, Daniil Titov wrote:
> Please review the change that fixes 3 tests that intermittently fail with 
JVMTI_ERROR_WRONG_PHASE error.
>
> The problem here is that the callbacks these tests enable keep processing 
events and perform JVMTI calls after VM is terminated. The fix makes these test 
listen for VMDeath event and  quick return from the callbacks after VMDeath 
event is received.
>
> Webrev: http://cr.openjdk.java.net/~dtitov/8218401/webrev.01/
> Bug: https://bugs.openjdk.java.net/browse/JDK-8218401
>
> Thanks!
> -Daniil
>
>

Re: RFR: JDK-8220295: sun/tools/jps/TestJps.java still timing out

2019-03-20 Thread Gary Adams


I think I have found the source of the dash.
 sun/tools/jstat/RowClosure.java
sets the NaN value to "-".

That should be fine.

It just means in the jstat output tests need to account for the possibility.

...

On 3/19/19, 6:07 PM, Chris Plummer wrote:
The presence of the '-' sounds familiar. I recall reading about it in 
a review. I know I did a review for these tests that impacted the 
output, but I don't see it introducing a '-' anywhere. See 
JDK-8202466. However, that bug references JDK-8199519, which caused 
problems by changing a value in the output from a '-' to a 0:


--- 
a/test/hotspot/jtreg/serviceability/tmtools/jstat/utils/JstatGcCauseResults.java
Wed Apr 25 17:50:32 2018 -0400
+++ 
b/test/hotspot/jtreg/serviceability/tmtools/jstat/utils/JstatGcCauseResults.java
Thu Apr 26 09:45:47 2018 +0900
@@ -74,10 +74,18 @@
  assertThat(GCT>= 0, "Incorrect time value for GCT");
  assertThat(GCT>= YGCT, "GCT<  YGCT (total garbage collection time<  young 
generation garbage collection time)");
  
-int CGC = getIntValue("CGC");

-float CGCT = getFloatValue("CGCT");
-assertThat(CGCT>= 0, "Incorrect time value for CGCT");
+int CGC = 0;
+float CGCT = 0.0f;
+try {
+CGC = getIntValue("CGC");
+} catch (NumberFormatException e) {
+if (!e.getMessage().equals("Unparseable number: \"-\"")) {
+throw e;
+}
+}
  if (CGC>  0) {
+CGCT = getFloatValue("CGCT");
+assertThat(CGCT>= 0, "Incorrect time value for CGCT");
  assertThat(CGCT>  0, "Number of concurrent GC events is " + CGC + ", 
but CGCT is 0");
  }

I'm not sure if any of this is related to what you are seeing.

Chris

On 3/19/19 11:39 AM, Gary Adams wrote:

A quick follow up on the jstat test failures.

On the failed runs the output looks like this :

--messages:(3/127)--
command: shell jstatLineCounts4.sh
reason: User specified action: run shell jstatLineCounts4.sh
elapsed time (seconds): 7.496
--System.out:(13/1261)--
   S0 S1 E  O  M CCSYGC YGCTFGCFGCTCGC  
  CGCT GCT
   0.00   0.00   0.00   0.00  -  -  00.000 00.000 0 
   0.0000.000
   0.00   0.00   0.00   0.00  -  -  00.000 00.000 0 
   0.0000.000
   0.00   0.00   0.00   0.00  -  -  00.000 00.000 0 
   0.0000.000
   0.00   0.00   0.00   0.00  -  -  00.000 00.000 0 
   0.0000.000
   0.00   0.00   0.00   0.00  -  -  00.000 00.000 0 
   0.0000.000
   0.00   0.00   0.00   0.00  -  -  00.000 00.000 0 
   0.0000.000
   0.00   0.00   0.00   0.00  -  28.19  10.571 00.000 0 
   0.0000.571
   0.00 100.00   0.00  14.85  31.29  28.19  10.571 00.000 0 
   0.0000.571
   0.00 100.00   0.00  14.85  31.29  28.19  10.571 00.000 0 
   0.0000.571
   0.00 100.00   0.00  14.85  31.29  28.19  10.571 00.000 0 
   0.0000.571
   S0 S1 E  O  M CCSYGC YGCTFGCFGCTCGC  
  CGCT GCT
   0.00 100.00   0.00  14.85  31.29  28.19  10.571 00.000 0 
   0.0000.571

The awk scripts used to check the output are not aware a dash '-' is 
allowed the metaspace column.

Here is a quick stab to allow the dashes.
Is anyone aware of recent changes in the gcutil output?

diff --git a/test/jdk/sun/tools/jstat/lineCounts3.awk 
b/test/jdk/sun/tools/jstat/lineCounts3.awk

--- a/test/jdk/sun/tools/jstat/lineCounts3.awk
+++ b/test/jdk/sun/tools/jstat/lineCounts3.awk
@@ -23,7 +23,7 @@
 headerlines++;
 }

-/^[ ]*[0-9]+\.[0-9]+[ ]*[0-9]+\.[0-9]+[ ]*[0-9]+\.[0-9]+[ 
]*[0-9]+\.[0-9]+[ ]*[0-9]+\.[0-9]+[ ]*([0-9]+\.[0-9]+)|-[ ]*[0-9]+[ 
]*[0-9]+\.[0-9]+[ ]*[0-9]+[ ]*[0-9]+\.[0-9]+[ ]*[0-9]+[ 
]*[0-9]+\.[0-9]+[ ]*[0-9]+\.[0-9]+$/{
+/^[ ]*[0-9]+\.[0-9]+[ ]*[0-9]+\.[0-9]+[ ]*[0-9]+\.[0-9]+[ 
]*[0-9]+\.[0-9]+[ ]*([0-9]+\.[0-9]+)|-[ ]*([0-9]+\.[0-9]+)|-[ 
]*[0-9]+[ ]*[0-9]+\.[0-9]+[ ]*[0-9]+[ ]*[0-9]+\.[0-9]+[ ]*[0-9]+[ 
]*[0-9]+\.[0-9]+[ ]*[0-9]+\.[0-9]+$/{

 datalines++;
 }

diff --git a/test/jdk/sun/tools/jstat/lineCounts4.awk 
b/test/jdk/sun/tools/jstat/lineCounts4.awk

--- a/test/jdk/sun/tools/jstat/lineCounts4.awk
+++ b/test/jdk/sun/tools/jstat/lineCounts4.awk
@@ -26,7 +26,7 @@
 headerlines++;
 }

-/^[ ]*[0-9]+\.[0-9]+[ ]*[0-9]+\.[0-9]+[ ]*[0-9]+\.[0-9]+[ 
]*[0-9]+\.[0-9]+[ ]*[0-9]+\.[0-9]+[ ]*([0-9]+\.[0-9]+)|-[ ]*[0-9]+[ 
]*[0-9]+\.[0-9]+[ ]*[0-9]+[ ]*[0-9]+\.[0-9]+[ ]*[0-9]+[ 
]*[0-9]+\.[0-9]+[ ]*[0-9]+\.[0-9]+$/{
+/^[ ]*[0-9]+\.[0-9]+[ ]*[0-9]+\.[0-9]+[ ]*[0-9]+\.[0-9]+[ 
]*[0-9]+\.[0-9]+[ ]*([0-9]+\.[0-9]+)|-[ ]*([0-9]+\.[0-9]+)|-[ 
]*[0-9]+[ ]*[0-9]+\.[0-9]+[ ]*[0-9]+[ ]*[0-9]+\.[0-9]+[ ]*[0-9]+[

Re: RFR(XS): 8220707: [TESTBUG] serviceability/sa/TestHeapDumpForLargeArray.java fails with jtreg -vmoption:-Xmx < 8g

2019-03-20 Thread Nick Gasson

Thanks! Do I need to get any more official Reviewers to look at this 
before it's OK to push?

Nick

On 19/03/2019 00:17, Jean Christophe Beyler wrote:

Hi Nick,

Looks good to me :)
Jc

On Mon, Mar 18, 2019 at 3:05 AM Nick Gasson > wrote:

Hi Sharath,

On 15/03/2019 23:32, Sharath Ballal wrote:
 >
 > Fix looks good.  How have you tested it ?

I ran the test using the "make test" target with and without
JTREG="MAX_MEM=512m" on AArch64 and x86. Previously it would fail with
that argument.

Thanks,
Nick

--

Thanks,
Jc

Re: 8218401: WRONG_PHASE: vmTestbase/nsk/jvmti test crash

Re: 8218401: WRONG_PHASE: vmTestbase/nsk/jvmti test crash

Re: 8218401: WRONG_PHASE: vmTestbase/nsk/jvmti test crash

Re: 8218401: WRONG_PHASE: vmTestbase/nsk/jvmti test crash

Re: RFR: JDK-8220295: sun/tools/jps/TestJps.java still timing out

Re: RFR(XS): 8220707: [TESTBUG] serviceability/sa/TestHeapDumpForLargeArray.java fails with jtreg -vmoption:-Xmx < 8g

6 matches

Site Navigation

Mail list logo

Footer information