Re: RFR 8034168: ThreadMXBean/Locks.java failed, blocked on wrong object

[Jaroslav Bachorik]

On 18.2.2014 18:06, Martin Buchholz wrote:

Not checking any details, but tests that want to wait for a particular
thread state are a good reason to use

volatile boolean flag;
...
while (!flag) Thread.yield();

I prefer calling Thread.yield to sleeping in this special case, in part
because I don't want to rely on the implementation of sleep, while yield is
semantically a no-op.  (Also sleeping 100ms is a long time for a computer)


There were discussions for a similar fix regarding Thread.yield(). The 
concern was that using Thread.yield() in a tight loop might very easily 
lead to starvation on single core machines. Therefore Thread.sleep(10) 
is used to be sure the flag setting thread has actually a chance to 
progress.


-JB-





On Tue, Feb 18, 2014 at 8:22 AM, Jaroslav Bachorik <
jaroslav.bacho...@oracle.com> wrote:


Please, review the following test change.

Issue : https://bugs.openjdk.java.net/browse/JDK-8034168
Webrev: http://cr.openjdk.java.net/~jbachorik/8034168/webrev.00

The test fails because of falsely evaluating the thread being parked as
actually waiting on a monitor. This is because there is no difference in
java thread state for those two situations. The test is using Phaser for
synchronization between the checked and checking thread to make sure an
appropriate code section is entered before performing asserts. Then it
checks the checked thread state and waits till it becomes WAITING.
Unfortunately, when Phaser needs to wait it parks the thread and sets the
thread state to WAITING. From now on the test is in a completely random
state and the result will largely depend on timing - thus failing
intermittently.

The solution is to use an additional volatile variable to prevent falsely
indicating the park() induced WAITING state.

Thanks,

-JB-

Re: RFR(S): 6952105 TEST_BUG: testcase failure, not very often, com/sun/jdi/SuspendThreadTest.java

[shanliang]

I am looking at the old file:
143 while (bkptCount < maxBkpts) {
144 prevBkptCount = bkptCount;

suppose the following execution sequence:
1)   when Line 143 was called by Thread1, we had bkptCount == maxBkpts - 1;

2) bkptCount++ was executed by thread2;

3) Line 144 was called by thread1,

in this case it was sure that the line
   152 failure("failure: test hung");
would be called.

It is good to add:
   synchronized (bkptSignal)
in the fix, but we need to put Line 143 and 144 into synchronization too.

To deal with a spurious wakeup, we might do like this:
   long stopTime = System.currentTimeMillis() + 5000;
   do {
   try {
   bkptSignal.wait(100);
   } catch (InterruptedException e){}
   } while(prevBkptCount == bkptCount && System.currentTimeMillis() 
< stopTime);


Shanliang

David Holmes wrote:

On 18/02/2014 11:03 PM, Staffan Larsen wrote:


On 18 feb 2014, at 13:09, David Holmes  wrote:


Hi Staffan,

If you get a spurious wakeup from wait():

151 try {
152 synchronized (bkptSignal) {
153 bkptSignal.wait(5000);
154 }
155 } catch (InterruptedException ee) {
156 }
157 if (prevBkptCount == bkptCount) {
158 failure("failure: test hung");

you could report failure. But that is far less likely than the 
current problem using sleep.


Right. Adding “continue;” inside the catch(InterruptedException) 
block should guard against that.


No, a spurious wakeup is not an interrupt - the wait() will simply 
return.


David


/Staffan



David

On 18/02/2014 8:19 PM, Staffan Larsen wrote:

Still looking for Reviewer for this change.

Thanks,
/Staffan

On 11 feb 2014, at 15:12, Staffan Larsen 
 wrote:


Updated the test to use proper synchronization and notification 
between threads. Should be more stable and much faster.


bug: https://bugs.openjdk.java.net/browse/JDK-6952105
webrev: http://cr.openjdk.java.net/~sla/6952105/webrev.00/

Thanks,
/Staffan

JDK-8005604, instance size/metaspace data in HPROF

[Vladimir Sitnikov]

Seems like HPROF does not include information on non-heap memory.
It would be nice to have accurate class size (e.g. with @Contended), space
wastage, permgen/metaspace (e.g. constant pool size or whatever is in
C-heap), code cache blobs in the heap dump.

Alexey Shipilёv raised similar question regarding instance size CR 8005604:
HPROF should report the actual instance size [1], [2], however it seems no
progress is made.

Creating brand-new format does not look like an easy solution, and it is
not clear how new features will be added later. It is not clear why new
format would be better in terms of efforts to update tooling.

What do you think if we keep HPROF format completely intact and dump
additional information via synthetic classes and objects?

For instance, to dump precise instance size of java class, we include
"vm.native.synthetic.ClassLayout { int instanceSize; int spaceLoss; }"
class in the generated HPROF file. For each java class we dump "instance"
of this ClassInfo.
This will reveal true instance size, space loss (e.g. alignment,
@contended, etc) to the heap analyzer.
The resulting HPROF will be fully compatible with existing analyzers, so no
significant harm is made.

To associate java.lang.Class and its vm.native metadata, a new synthetic
static reference from java.lang.Class to vm.native.synthetic.ClassInfo is
added.
Another approach is to reference from vm.native to java.lang.Class, however
it will be harder to analyze (more clicks, more complex queries)

Pros:
1) We can dump real instance size or any new information while keeping
HPROF format intact
2) Current tools will parse and display the dump just fine. With moderate
effort tools can be improved to use this "metadata" for calculations (and
proper accounting of "used heap size")
3) Query language of memory analyzers (e.g. map-reduce-javascript in
VirtualVM, OQL/SQL in Eclipse MAT, etc) can leverage this new data. The
approach of synthetic classes does not require to update analyzer for that.
4) More details (e.g. code cache blobs or whatever) can be added in the
similar way to the dump while still keeping forward and backward
compatibility in terms of file format

Cons:
1) I am not sure if iteration over permgen/metaspace/codecache is safe in
terms of crashes (or whatever). It is not good a good idea to crash when
writing HPROF dump (especially during OutOfMemoryError), however I believe
at least basic information (e.g. instance size) should be available in a
safe way.
2) Existing analyzers will charge "heap size" for synthetic
classes/objects. I do not think it is a big deal, however, if we dump
permgen/metaspace/codecache, then the amount of synthetic objects might be
significant enough to confuse non-expecting engineers


1: https://bugs.openjdk.java.net/browse/JDK-8005604
2:
http://mail.openjdk.java.net/pipermail/serviceability-dev/2012-December/007852.html

-- 
Regards,
Vladimir Sitnikov

Re: 8034856/8034857: More gcc warnings

[Mikael Vidstedt]

On 2014-02-18 00:33, Alan Bateman wrote:

On 18/02/2014 03:59, Mikael Vidstedt wrote:


How about:

http://cr.openjdk.java.net/~mikael/webrevs/isspace/webrev.00/webrev/

Cheers,
Mikael

I checked the java.lang.instrument spec and for the Boot-Class-Path 
attribute then it doesn't say any more than "space". It might be worth 
checking the manifest parsing code (parse_manfiest.c) to see how 
continuations are handled as I suspect \r and \n can't appear in the 
attribute value (in which case the check might really only need to be 
for space and \t.


That makes sense, and in fact parse_manifest.c does not even appear to 
allow for \t, so I'm more and more starting to think that a reasonable 
implementation in this context would be:


static int isNormalSpace(int c) { return c == ' '; }

In which case it probably shouldn't even be a separate function to start 
with. I would like to get a second opinion on the implications of only 
checking for ' ' (0x20) though.


If we want to allow both ' ' and \t we should probably call the function 
isblankAscii.


Otherwise replacing isspace is good and your isspaceAscii is likely to 
match the libc isspace (at runtime). This code isn't performance 
sensitive but maybe check space first would be a bit better. Also the 
library native code using 4 space indent rather than hotspot's 2.


Will fix indentation. I seriously doubt that the performance difference 
warrants the more complicated code.


I created JDK-8035054 a few days ago to track this. Thanks for taking 
it as I am busy with a number of other things at the moment.


Always for you, sir! ;)

/Mikael

Re: RFR(S): 6952105 TEST_BUG: testcase failure, not very often, com/sun/jdi/SuspendThreadTest.java

[David Holmes]

On 18/02/2014 11:03 PM, Staffan Larsen wrote:


On 18 feb 2014, at 13:09, David Holmes  wrote:


Hi Staffan,

If you get a spurious wakeup from wait():

151 try {
152 synchronized (bkptSignal) {
153 bkptSignal.wait(5000);
154 }
155 } catch (InterruptedException ee) {
156 }
157 if (prevBkptCount == bkptCount) {
158 failure("failure: test hung");

you could report failure. But that is far less likely than the current problem 
using sleep.


Right. Adding “continue;” inside the catch(InterruptedException) block should 
guard against that.


No, a spurious wakeup is not an interrupt - the wait() will simply return.

David


/Staffan



David

On 18/02/2014 8:19 PM, Staffan Larsen wrote:

Still looking for Reviewer for this change.

Thanks,
/Staffan

On 11 feb 2014, at 15:12, Staffan Larsen  wrote:


Updated the test to use proper synchronization and notification between 
threads. Should be more stable and much faster.

bug: https://bugs.openjdk.java.net/browse/JDK-6952105
webrev: http://cr.openjdk.java.net/~sla/6952105/webrev.00/

Thanks,
/Staffan

RFR 8034168: ThreadMXBean/Locks.java failed, blocked on wrong object

[Jaroslav Bachorik]

Please, review the following test change.

Issue : https://bugs.openjdk.java.net/browse/JDK-8034168
Webrev: http://cr.openjdk.java.net/~jbachorik/8034168/webrev.00

The test fails because of falsely evaluating the thread being parked as 
actually waiting on a monitor. This is because there is no difference in 
java thread state for those two situations. The test is using Phaser for 
synchronization between the checked and checking thread to make sure an 
appropriate code section is entered before performing asserts. Then it 
checks the checked thread state and waits till it becomes WAITING. 
Unfortunately, when Phaser needs to wait it parks the thread and sets 
the thread state to WAITING. From now on the test is in a completely 
random state and the result will largely depend on timing - thus failing 
intermittently.


The solution is to use an additional volatile variable to prevent 
falsely indicating the park() induced WAITING state.


Thanks,

-JB-

Re: RFR(S): 6952105 TEST_BUG: testcase failure, not very often, com/sun/jdi/SuspendThreadTest.java

[Staffan Larsen]

On 18 feb 2014, at 13:09, David Holmes  wrote:

> Hi Staffan,
> 
> If you get a spurious wakeup from wait():
> 
> 151 try {
> 152 synchronized (bkptSignal) {
> 153 bkptSignal.wait(5000);
> 154 }
> 155 } catch (InterruptedException ee) {
> 156 }
> 157 if (prevBkptCount == bkptCount) {
> 158 failure("failure: test hung");
> 
> you could report failure. But that is far less likely than the current 
> problem using sleep.

Right. Adding “continue;” inside the catch(InterruptedException) block should 
guard against that.

/Staffan

> 
> David
> 
> On 18/02/2014 8:19 PM, Staffan Larsen wrote:
>> Still looking for Reviewer for this change.
>> 
>> Thanks,
>> /Staffan
>> 
>> On 11 feb 2014, at 15:12, Staffan Larsen  wrote:
>> 
>>> Updated the test to use proper synchronization and notification between 
>>> threads. Should be more stable and much faster.
>>> 
>>> bug: https://bugs.openjdk.java.net/browse/JDK-6952105
>>> webrev: http://cr.openjdk.java.net/~sla/6952105/webrev.00/
>>> 
>>> Thanks,
>>> /Staffan
>> 

Re: RFR(S): 6952105 TEST_BUG: testcase failure, not very often, com/sun/jdi/SuspendThreadTest.java

[David Holmes]

Hi Staffan,

If you get a spurious wakeup from wait():

 151 try {
 152 synchronized (bkptSignal) {
 153 bkptSignal.wait(5000);
 154 }
 155 } catch (InterruptedException ee) {
 156 }
 157 if (prevBkptCount == bkptCount) {
 158 failure("failure: test hung");

you could report failure. But that is far less likely than the current 
problem using sleep.


David

On 18/02/2014 8:19 PM, Staffan Larsen wrote:

Still looking for Reviewer for this change.

Thanks,
/Staffan

On 11 feb 2014, at 15:12, Staffan Larsen  wrote:


Updated the test to use proper synchronization and notification between 
threads. Should be more stable and much faster.

bug: https://bugs.openjdk.java.net/browse/JDK-6952105
webrev: http://cr.openjdk.java.net/~sla/6952105/webrev.00/

Thanks,
/Staffan

Re: RFR(S): 6952105 TEST_BUG: testcase failure, not very often, com/sun/jdi/SuspendThreadTest.java

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

The fix looks good.

Thanks,
Serguei

On 2/18/14 2:19 AM, Staffan Larsen wrote:

Still looking for Reviewer for this change.

Thanks,
/Staffan

On 11 feb 2014, at 15:12, Staffan Larsen  wrote:


Updated the test to use proper synchronization and notification between 
threads. Should be more stable and much faster.

bug: https://bugs.openjdk.java.net/browse/JDK-6952105
webrev: http://cr.openjdk.java.net/~sla/6952105/webrev.00/

Thanks,
/Staffan

Re: RFR(S): 6952105 TEST_BUG: testcase failure, not very often, com/sun/jdi/SuspendThreadTest.java

[Staffan Larsen]

Still looking for Reviewer for this change.

Thanks,
/Staffan

On 11 feb 2014, at 15:12, Staffan Larsen  wrote:

> Updated the test to use proper synchronization and notification between 
> threads. Should be more stable and much faster.
> 
> bug: https://bugs.openjdk.java.net/browse/JDK-6952105
> webrev: http://cr.openjdk.java.net/~sla/6952105/webrev.00/
> 
> Thanks,
> /Staffan

Re: RFR 4505697: nsk/jdi/ExceptionEvent/_itself_/exevent006 and exevent008 tests fail with InvocationTargetException

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

On 2/17/14 12:04 AM, Jaroslav Bachorik wrote:

On 14.2.2014 23:13, serguei.spit...@oracle.com wrote:

On 2/14/14 12:33 PM, Daniel D. Daugherty wrote:

On 2/14/14 11:46 AM, serguei.spit...@oracle.com wrote:

Jaroslav,

It looks good in general modulo indent comments from Dan.

But I have a doubt that acquiring the JvmtiThreadState_lock is needed
or right thing to do in the JvmtiExport::clear_detected_exception().
It seems, both clear_exception_detected() and
set_exception_detected() are always
called on current thread and so, it has to be safe to do without
acquiring any locks.


My JVM/TI-foo is rusty, but I believe that JvmtiThreadState stuff
can also be queried/modified by other threads so grabbing the
associated lock is a good idea.


The lock synchronization is cooperative.
It does not help much if the lock is not acquired in other places.
I can be wrong, but I've not found yet any place in the code where the
clear_exception_detected() and set_exception_detected() are called
under protection of the JvmtiThreadState_lock.


I copied the locking over from 
"JvmtiExport::cleanup_thread(JavaThread* thread)". That method is also 
supposed to work only with the current thread but acquires the lock 
nonetheless. But if you are sure that the lock is not required I have 
no objections removing it.


I'm suggesting to remove it, as it is not used in other places in the code.
It is going to be confusing if it is used in one place and missed in others.

Thanks,
Serguei



-JB-



Thanks,
Serguei



Dan




And I'm repeating my question about pre-integration testing (Dan is
asking about the same).

Thanks,
Serguei


On 2/14/14 3:07 AM, Jaroslav Bachorik wrote:

This is a round-0 review request.

The reflection code intercepting the exceptions thrown in the
invoked methods does not play nicely with JVMTI (which, in this
case, propagates to JDI).

The reflection code lacks the traditional error handler - therefore,
upon throwing the NumberFormatException, the stack is searched for
appropriate handlers and none are found. This leaves the
"exception_detected" flag set to true while normally it would be
reset to false once the exception is handled. The reflection code
then goes on and wraps the NumberFormatException into
InvocationTargetException and throws it. But, alas, the
"exception_detected" flag is still set to true and no JVMTI
exception event will be sent out.

The proposed solution is to call
thread->jvmti_thread_state()->clear_exception_detected() at the
appropriate places in the reflection code to reset the
"exception_detected" flag and enable the InvocationTargetException
be properly reported over JVMTI.

Issue : https://bugs.openjdk.java.net/browse/JDK-4505697
Webrev: http://cr.openjdk.java.net/~jbachorik/4505697/webrev.00

Thanks!

-JB-

Re: RFR(XS): JDK-4515292: ReferenceType.isStatic() returns true for arrays

[Staffan Larsen]

Looks good!

Thanks,
/Staffan

On 17 feb 2014, at 10:46, Fredrik Arvidsson  
wrote:

> Hi please review this minor JDI fix.
> 
> This bug was found when investigating failing JDI tests. According to the 
> JavaDoc all calls to ReferenceType when it is an array type should return 
> false for isStatic().
> 
> Webrev: http://cr.openjdk.java.net/~farvidsson/4515292/webrev.00/ 
> 
> Jira: https://bugs.openjdk.java.net/browse/JDK-4515292
> 
> Cheers
> /Fredrik

RFR(XS): JDK-8034203: Change JavaDoc for com.sun.jdi.request.EventRequest.setEnabled(boolean val)

[Fredrik Arvidsson]

Hi

Please help me review this small change:

This is a minor change to the JavaDoc in the JDI API. The problem was 
discovered when trying to fix a failing test.


Webrev: 8034203 


Bug: JDK-8034203 
CCC: 8034203 

Thanks
/Fredrik

Re: RFR 4505697: nsk/jdi/ExceptionEvent/_itself_/exevent006 and exevent008 tests fail with InvocationTargetException

[David Holmes]

On 18/02/2014 6:47 PM, Jaroslav Bachorik wrote:

Hi David,

On 18.2.2014 06:28, David Holmes wrote:

Hi Jaroslav,

It seems to me that this issue extends to other places in the VM. In
particular class initialization in instanceKlass.cpp - anywhere that one
exception is "caught" in the VM and then wrapped with, or replaced by,
another exception, will only notify JVMTI of the original exception.


Thanks for pointing this out. Turns out there is another location in
jvm.cpp which needs the same treatment.

BTW, what is your take on the necessity to grab the
JvmtiThreadState_lock before cleaning the detected exception in the
jvmti thread state?


I would need to analyze all of the code that accesses it to determine 
that. My initial thought was that it seemed unnecessary and I did look 
at some of the code which seemed to indicate other threads would only 
access it at a safepoint. But there may be other access points that I'm 
not aware of.


David


-JB-



David

On 14/02/2014 9:07 PM, Jaroslav Bachorik wrote:

This is a round-0 review request.

The reflection code intercepting the exceptions thrown in the invoked
methods does not play nicely with JVMTI (which, in this case, propagates
to JDI).

The reflection code lacks the traditional error handler - therefore,
upon throwing the NumberFormatException, the stack is searched for
appropriate handlers and none are found. This leaves the
"exception_detected" flag set to true while normally it would be reset
to false once the exception is handled. The reflection code then goes on
and wraps the NumberFormatException into InvocationTargetException and
throws it. But, alas, the "exception_detected" flag is still set to true
and no JVMTI exception event will be sent out.

The proposed solution is to call
thread->jvmti_thread_state()->clear_exception_detected() at the
appropriate places in the reflection code to reset the
"exception_detected" flag and enable the InvocationTargetException be
properly reported over JVMTI.

Issue : https://bugs.openjdk.java.net/browse/JDK-4505697
Webrev: http://cr.openjdk.java.net/~jbachorik/4505697/webrev.00

Thanks!

-JB-

Re: 8034856/8034857: More gcc warnings

[Dmitry Samersoff]

Mikael,

> http://cr.openjdk.java.net/~mikael/webrevs/isspace/webrev.00/webrev/

1. I agree that ctypes isspace usually cause more problems than solve
and it's good to have our own version.

2. one of possible implementation is

 #define isspaceASCII(c) (strchr(SPACE_CHARS,c) != NULL)

-Dmitry


On 2014-02-18 07:59, Mikael Vidstedt wrote:
> 
> On 2014-02-17 07:08, Alan Bateman wrote:
>> On 17/02/2014 05:51, Mikael Vidstedt wrote:
>>>
>>> I'm inclined to agree with this. Since the code depends on a specific
>>> behavior of isspace which does not match what the system provided
>>> function does I too think it would be more robust to implement our
>>> own version of it.
>> I completely agree that changing this code to use its own isspace is
>> the right thing, it just seems a bit much for a drive-by fixed to gcc
>> warnings. Do either of you want to take it?
> 
> How about:
> 
> http://cr.openjdk.java.net/~mikael/webrevs/isspace/webrev.00/webrev/
> 
> Cheers,
> Mikael
> 


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.

Re: RFR 4505697: nsk/jdi/ExceptionEvent/_itself_/exevent006 and exevent008 tests fail with InvocationTargetException

[Jaroslav Bachorik]

Hi David,

On 18.2.2014 06:28, David Holmes wrote:

Hi Jaroslav,

It seems to me that this issue extends to other places in the VM. In
particular class initialization in instanceKlass.cpp - anywhere that one
exception is "caught" in the VM and then wrapped with, or replaced by,
another exception, will only notify JVMTI of the original exception.


Thanks for pointing this out. Turns out there is another location in 
jvm.cpp which needs the same treatment.


BTW, what is your take on the necessity to grab the 
JvmtiThreadState_lock before cleaning the detected exception in the 
jvmti thread state?


-JB-



David

On 14/02/2014 9:07 PM, Jaroslav Bachorik wrote:

This is a round-0 review request.

The reflection code intercepting the exceptions thrown in the invoked
methods does not play nicely with JVMTI (which, in this case, propagates
to JDI).

The reflection code lacks the traditional error handler - therefore,
upon throwing the NumberFormatException, the stack is searched for
appropriate handlers and none are found. This leaves the
"exception_detected" flag set to true while normally it would be reset
to false once the exception is handled. The reflection code then goes on
and wraps the NumberFormatException into InvocationTargetException and
throws it. But, alas, the "exception_detected" flag is still set to true
and no JVMTI exception event will be sent out.

The proposed solution is to call
thread->jvmti_thread_state()->clear_exception_detected() at the
appropriate places in the reflection code to reset the
"exception_detected" flag and enable the InvocationTargetException be
properly reported over JVMTI.

Issue : https://bugs.openjdk.java.net/browse/JDK-4505697
Webrev: http://cr.openjdk.java.net/~jbachorik/4505697/webrev.00

Thanks!

-JB-

Re: 8034856/8034857: More gcc warnings

[Alan Bateman]

On 18/02/2014 03:59, Mikael Vidstedt wrote:


How about:

http://cr.openjdk.java.net/~mikael/webrevs/isspace/webrev.00/webrev/

Cheers,
Mikael

I checked the java.lang.instrument spec and for the Boot-Class-Path 
attribute then it doesn't say any more than "space". It might be worth 
checking the manifest parsing code (parse_manfiest.c) to see how 
continuations are handled as I suspect \r and \n can't appear in the 
attribute value (in which case the check might really only need to be 
for space and \t.


Otherwise replacing isspace is good and your isspaceAscii is likely to 
match the libc isspace (at runtime). This code isn't performance 
sensitive but maybe check space first would be a bit better. Also the 
library native code using 4 space indent rather than hotspot's 2.


I created JDK-8035054 a few days ago to track this. Thanks for taking it 
as I am busy with a number of other things at the moment.


-Alan