Re: RFR(S): 8186902: jcmd GC.run should not be blocked by DisableExplicitGC

[Mikael Gerdin]

Hi Kevin,

On 2017-08-29 13:41, Kevin Walls wrote:

Hi,

This is a small review request for:

8186902: jcmd GC.run should not be blocked by DisableExplicitGC
https://bugs.openjdk.java.net/browse/JDK-8186902

jcmd GC.run to invoke GC fails if -XX:+DisableExplicitGC is set: this 
seems like a mistake as it is obstructive for a live app that needs a 
GC, and was started with -XX:+DisableExplicitGC.


hg diff pasted below, simply removes the DisableExplicitGC if and 
un-indents the existing call to collect().


Builds and manually tests OK, GC occurs in response to jcmd GC.run even 
if java was started with -XX:+DisableExplicitGC


Previous email on this: 
http://mail.openjdk.java.net/pipermail/serviceability-dev/2017-August/021748.html 



Many thanks
Kevin


bash-4.2$ cd hotspot
bash-4.2$ hg diff
diff -r a20f0fa4c426 src/share/vm/services/diagnosticCommand.cpp
--- a/src/share/vm/services/diagnosticCommand.cpp   Mon Aug 28 
23:46:22 2017 +
+++ b/src/share/vm/services/diagnosticCommand.cpp   Tue Aug 29 
02:55:56 2017 -0700

@@ -414,11 +414,7 @@
  }

  void SystemGCDCmd::execute(DCmdSource source, TRAPS) {
-  if (!DisableExplicitGC) {
-Universe::heap()->collect(GCCause::_dcmd_gc_run);
-  } else {
-output()->print_cr("Explicit GC is disabled, no GC has been 
performed.");

-  }
+  Universe::heap()->collect(GCCause::_dcmd_gc_run);
  }

  void RunFinalizationDCmd::execute(DCmdSource source, TRAPS) {
bash-4.2$




Looks fine.
/Mikael




On 29/08/2017 10:14, Kevin Walls wrote:


Hi Mikael, thanks yes, it could be a separate cmd GC.runForce... 
However I was thinking if you can get as far as having your jcmd 
executed, you really _do_ want to run that collection.  Whatever 
behaviour you were protecting against when you chose the command-line 
arguments, you would only ever want to override if you run the jcmd to 
invoke GC... 8-)


I'll convert this to a review request for removing that check, will 
post that shortly.  This would be changing the behaviour, but I don't 
think it contradicts anything we document, and we seem to have added 
the check without documenting it.


Thanks
Kevin


On 28/08/2017 16:01, Mikael Gerdin wrote:

Hi Kevin,

On 2017-08-22 16:38, Kevin Walls wrote:

Hi,

jcmd GC.run to invoke GC fails if -XX:+DisableExplicitGC is set: 
this seems like a mistake?


This behaviour is obstructive for a live app that _needs_ a GC, and 
was started with -XX:+DisableExplicitGC.


DisableExplicitGC to protect from Java code calling System.gc 
frequently makes sense, but if I can attach and run a dcmd, I should 
have permission to inspect and maintain the JVM, including invoking 
a GC. (This is as the user who owns the process and can kill it off.)


The behaviour (checking DisableExplicitGC in SystemGCDCmd::execute) 
comes in with:


https://bugs.openjdk.java.net/browse/JDK-8004095
8004095: Add support for JMX interface to Diagnostic Framework and 
Commands


The JMX relation I suppose suggests we didn't want JMX to override 
DisableExplicitGC by way of using a jcmd/DCmd.


But also, we now have:
8072913: [REDO] GCCause should distinguish jcmd GC.run from System.gc()
Summary: GCCause which is caused by GC.run diagnostic command should 
be different from System.gc() .


..at least the causes are distinct.

I don't think we document this clearly.  Our comment in globals.hpp 
is ""Ignore calls to System.gc()".  I don't think we say anywhere 
that jcmd is subject to being disabled by the flag.


Interested to hear any reason in favour of the current behaviour! If 
there's nothing, I'll log a bug and ask for review of the change to 
remove it...


There were some discussions earlier around this area and I came up 
with the idea of having a "force" option to the GC.run command to 
override DisableExplicitGC.
The comments in globals.hpp are a notoriously bad spec for the flags 
since they are only ever present in debug builds of the JVM.


Thanks
/Mikael



Thanks
Kevin

Re: jcmd GC.run to invoke GC fails if -XX:+DisableExplicitGC is set.

[Mikael Gerdin]

Hi Kevin,

On 2017-08-29 11:14, Kevin Walls wrote:


Hi Mikael, thanks yes, it could be a separate cmd GC.runForce... However 
I was thinking if you can get as far as having your jcmd executed, you 
really _do_ want to run that collection.  Whatever behaviour you were 
protecting against when you chose the command-line arguments, you would 
only ever want to override if you run the jcmd to invoke GC... 8-)


I was more thinking of
jcmd GC.run force=1



I'll convert this to a review request for removing that check, will post 
that shortly.  This would be changing the behaviour, but I don't think 
it contradicts anything we document, and we seem to have added the check 
without documenting it.


Sounds good.
/Mikael



Thanks
Kevin


On 28/08/2017 16:01, Mikael Gerdin wrote:

Hi Kevin,

On 2017-08-22 16:38, Kevin Walls wrote:

Hi,

jcmd GC.run to invoke GC fails if -XX:+DisableExplicitGC is set: this 
seems like a mistake?


This behaviour is obstructive for a live app that _needs_ a GC, and 
was started with -XX:+DisableExplicitGC.


DisableExplicitGC to protect from Java code calling System.gc 
frequently makes sense, but if I can attach and run a dcmd, I should 
have permission to inspect and maintain the JVM, including invoking a 
GC. (This is as the user who owns the process and can kill it off.)


The behaviour (checking DisableExplicitGC in SystemGCDCmd::execute) 
comes in with:


https://bugs.openjdk.java.net/browse/JDK-8004095
8004095: Add support for JMX interface to Diagnostic Framework and 
Commands


The JMX relation I suppose suggests we didn't want JMX to override 
DisableExplicitGC by way of using a jcmd/DCmd.


But also, we now have:
8072913: [REDO] GCCause should distinguish jcmd GC.run from System.gc()
Summary: GCCause which is caused by GC.run diagnostic command should 
be different from System.gc() .


..at least the causes are distinct.

I don't think we document this clearly.  Our comment in globals.hpp 
is ""Ignore calls to System.gc()".  I don't think we say anywhere 
that jcmd is subject to being disabled by the flag.


Interested to hear any reason in favour of the current behaviour!  If 
there's nothing, I'll log a bug and ask for review of the change to 
remove it...


There were some discussions earlier around this area and I came up 
with the idea of having a "force" option to the GC.run command to 
override DisableExplicitGC.
The comments in globals.hpp are a notoriously bad spec for the flags 
since they are only ever present in debug builds of the JVM.


Thanks
/Mikael



Thanks
Kevin

Re: jcmd GC.run to invoke GC fails if -XX:+DisableExplicitGC is set.

[Mikael Gerdin]

Hi Kevin,

On 2017-08-22 16:38, Kevin Walls wrote:

Hi,

jcmd GC.run to invoke GC fails if -XX:+DisableExplicitGC is set: this 
seems like a mistake?


This behaviour is obstructive for a live app that _needs_ a GC, and was 
started with -XX:+DisableExplicitGC.


DisableExplicitGC to protect from Java code calling System.gc frequently 
makes sense, but if I can attach and run a dcmd, I should have 
permission to inspect and maintain the JVM, including invoking a GC. 
(This is as the user who owns the process and can kill it off.)


The behaviour (checking DisableExplicitGC in SystemGCDCmd::execute) 
comes in with:


https://bugs.openjdk.java.net/browse/JDK-8004095
8004095: Add support for JMX interface to Diagnostic Framework and Commands

The JMX relation I suppose suggests we didn't want JMX to override 
DisableExplicitGC by way of using a jcmd/DCmd.


But also, we now have:
8072913: [REDO] GCCause should distinguish jcmd GC.run from System.gc()
Summary: GCCause which is caused by GC.run diagnostic command should be 
different from System.gc() .


..at least the causes are distinct.

I don't think we document this clearly.  Our comment in globals.hpp is 
""Ignore calls to System.gc()".  I don't think we say anywhere that jcmd 
is subject to being disabled by the flag.


Interested to hear any reason in favour of the current behaviour!  If 
there's nothing, I'll log a bug and ask for review of the change to 
remove it...


There were some discussions earlier around this area and I came up with 
the idea of having a "force" option to the GC.run command to override 
DisableExplicitGC.
The comments in globals.hpp are a notoriously bad spec for the flags 
since they are only ever present in debug builds of the JVM.


Thanks
/Mikael



Thanks
Kevin

Re: RFR: 8154728: JvmtiExport::add_default_read_edges hits a guarantee

[Mikael Gerdin]

Hi Robbin,

On 2016-04-20 09:40, Robbin Ehn wrote:

Hi all,

Please review.

We are missing a cr on a log stream, which leads to this guarantee.

Bug: https://bugs.openjdk.java.net/browse/JDK-8154728

Thanks !

/Robbin

diff -r 857efca82258 src/share/vm/prims/jvmtiExport.cpp
--- a/src/share/vm/prims/jvmtiExport.cppSun Apr 17 19:15:52 2016 -0700
+++ b/src/share/vm/prims/jvmtiExport.cppWed Apr 20 09:37:01 2016 +0200
@@ -428,6 +428,7 @@
  LogTarget(Trace, jvmti) log;
  LogStreamCHeap log_stream(log);
  java_lang_Throwable::print(PENDING_EXCEPTION, &log_stream);
+log_stream.cr();
  CLEAR_PENDING_EXCEPTION;
  return;
}



Looks good.

/Mikael

Re: RFR[9u-dev]: 8054326: Confusing message in "Current rem set statistics"

[Mikael Gerdin]

Hi,

this is GC code and the rewview should go to hotspot-gc-dev.

I'm redirecting this mail there.

/Mikael

On 2016-04-06 08:15, Cheleswer Sahu wrote:

Hi,



This fix is waiting for review, can somebody please review this change.



Regards,

Cheleswer



From: Cheleswer Sahu
Sent: Friday, April 01, 2016 6:01 PM
To: hotspot-runtime-...@openjdk.java.net; serviceability-dev@openjdk.java.net
Cc: Mattis Castegren
Subject: RFR[9u-dev]: 8054326: Confusing message in "Current rem set statistics"



Hi,



Please review the code changes for 
https://bugs.openjdk.java.net/browse/JDK-8054326.



Webrev Link: http://cr.openjdk.java.net/~csahu/8054326/



Bug Brief: In output of remset statistics "Max"  is printing as 0k, which is 
confusing for user.



Problem Identified : "Max" value is rounded to KB, which result  in 0k, if the 
value is less than 1024B.



Solution Proposed:  If the value for "Max" is less than 1 KB (1024 B),  print 
the value in bytes (i.e. 1023B.) else

print the value in KB.







Regards,

Cheleswer

Re: (S) RFR: 8150506: Remove unused locks

[Mikael Gerdin]

Hi David,

On 2016-02-24 08:03, David Holmes wrote:

I stumbled across the fact that the following locks are no longer being
used in the VM:

Runtime:
  - Interrupt_lock
  - ProfileVM_lock
  - ObjAllocPost_lock

Serviceability:
  -JvmtiPendingEvent_lock

GC:
- CMark_lock
- CMRegionStack_lock


GC Lock removal looks good.
/Mikael



so unless there are objections I will remove them. A reviewer from each
area would be appreciated.

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

webrev: http://cr.openjdk.java.net/~dholmes/8150506/webrev/

Thanks,
David

Re: [PATCH resend2] Skip the null pointer check before calling realloc

[Mikael Gerdin]

Dmitry,

On 2015-12-18 14:11, Dmitry Samersoff wrote:

Alex,

Unfortunately, webrev.zip attachment is removed from the e-mail.

Could you put it somewhere and send me a link?


http://mail.openjdk.java.net/pipermail/serviceability-dev/2015-December/018519.html

I got the attachment just fine, maybe there is something specific to 
your setup which disallows you from receiving it?


/Mikael




Sorry for inconvenience.

-Dmitry


On 2015-12-18 00:04, Alex Henrie wrote:

An attachment in the email has been found to contain executable code and has 
been removed.

File removed : webrev.zip, 
zip,html,list,c,c,,html,html,html,html,html,html,html,html,patch,html,html,patch,js

--
2015-12-17 2:17 GMT-07:00 Dmitry Samersoff :

I'll sponsor the push, but please:

1. fix realloc issue as it suggested by Dan and me.

2. prepare a webrev (I'll put it under my ojdk account to
/sponsorship/alexhenrie)


Here you go.

-Alex

Re: [RFR] (M) 8143608: Don't 64-bit align start of InstanceKlass vtable, itable, and nonstatic_oopmap on 32-bit systems

[Mikael Gerdin]

Hi Chris,

On 2015-12-16 08:40, Chris Plummer wrote:

Hi Mikael,

On 12/15/15 1:56 AM, Mikael Gerdin wrote:

Hi Chris,

sorry for the late reply.

On 2015-12-10 22:31, Chris Plummer wrote:

Hi Mikael,

See comments inline below:

On 12/9/15 8:48 AM, Mikael Gerdin wrote:

On 2015-12-08 20:14, Chris Plummer wrote:

[Adding serviceability-dev@openjdk.java.net]

Hi Mikael,

Thanks for pointing this out. I'll look into it some more. Are
there any
tests that should be failing as a result of this? I get the feeling
no,
since I see other issues here that existed before my change. For
example, this code is not returning the proper size if the class is
anonymous or is an interface. It needs to add 1 extra word in that
case.
See size() in instanceKlass.hpp.

Another difference from the VM code is alignObjectSize() is being used
by getSize(), but headerSize is set using alignObjectOffset(). The VM
code uses align_object_offset in both cases.

   // Align the object size.
   public static long alignObjectSize(long size) {
 return VM.getVM().alignUp(size,
VM.getVM().getMinObjAlignmentInBytes());
   }

   // All vm's align longs, so pad out certain offsets.
   public static long alignObjectOffset(long offset) {
 return VM.getVM().alignUp(offset, VM.getVM().getBytesPerLong());
   }

So the difference here is in the use of getMinObjAlignmentInBytes (not
what the VM does) vs getBytesPerLong (what the VM uses):

   public int getObjectAlignmentInBytes() {
 if (objectAlignmentInBytes == 0) {
 Flag flag = getCommandLineFlag("ObjectAlignmentInBytes");
 objectAlignmentInBytes = (flag == null) ? 8 :
(int)flag.getIntx();
 }
 return objectAlignmentInBytes;
   }

So this seems wrong for use in any InstanceKlass size or embedded
field
offset calculation. It is probably a remnant of when class metadata
was
stored in the java heap, and the size of InstanceKlass had to be
padded
out to the minimum heap object alignment. At least it is harmless if
ObjectAlignmentInBytes is not set, and if set it is only supported for
64-bit:

   lp64_product(intx, ObjectAlignmentInBytes,
8, \
   "Default object alignment in bytes, 8 is
minimum")\
   range(8,
256) \
constraint(ObjectAlignmentInBytesConstraintFunc,AtParse) \

I'll get these cleaned up, but it sure would be nice if there was a
way
to reliably test it.


I'd say that it's quite possible to test it!
Create a whitebox.cpp entry point for determining the size of a class.

Ok, but I instead decided to use jcmd with "GC.class_stats KlassBytes"


Sounds good.



Run a java program calling the entry point and printing the
InstanceKlass size as computed by calling InstanceKlass::size() on a
set of pre-determined set of complex and simple classes (vtables,
itables, anonymous, etc.)

For now I just did this from the command line to do some quick checking.
No test written.

Have the java program wait after it's finished printing.

Launch the SA agent and attach it to the process.
Through several layers of magic, execute the incantation:

mgerdin@mgerdin03:~/work/repos/hg/jdk9/hs-rt-work/hotspot$
../build/linux-x86_64-normal-server-slowdebug/images/jdk/bin/java
sun.jvm.hotspot.CLHSDB 6211
Attaching to process 6211, please wait...
hsdb> jseval
"sapkg.utilities.SystemDictionaryHelper.findInstanceKlass('java/lang/Object').getSize();"


472

Ok. I did this to get sizes as SA sees them. They were not just wrong
for existing JDK, but in most cases off by a large margin. I did some
investigating.  This is InstanceKlass.getSize()

   public long getSize() {
 return Oop.alignObjectSize(getHeaderSize() +
Oop.alignObjectOffset(getVtableLen()) +
Oop.alignObjectOffset(getItableLen()) +
Oop.alignObjectOffset(getNonstaticOopMapSize()));
   }

The problem is that getHeaderSize() returns the size in bytes, but the
others all return the size in words. They need to be multiplied by the
word size to get the right value since the caller of getSize() expects
the result to be in bytes.


Oh, that seems like a familiar problem :)



So we have multiple problems with SA with respect to the
InstanceKlass.getSize() support:

  * Alignment issues introduced by me.
  * Some minor other issues like using alignObjectSize when it's not
needed, and not taking into account extra fields for interfaces and
anonymous classes.
  * Not multiplying the vtable, itable, and oopMapSize by the number of
bytes in a word.
  * No test available.

I'm not too sure how to proceed here w.r.t. my CR. I'm inclined to just
make the SA adjustments needed to take into consideration the alignment
changes I've made to InstanceKlass, and file a CRs for the rest (one for
the existing bugs and one for the lack of any test).


It would be good if you could at least fix the obvious word size
scaling 

Re: [RFR] (M) 8143608: Don't 64-bit align start of InstanceKlass vtable, itable, and nonstatic_oopmap on 32-bit systems

[Mikael Gerdin]

Hi Chris,

sorry for the late reply.

On 2015-12-10 22:31, Chris Plummer wrote:

Hi Mikael,

See comments inline below:

On 12/9/15 8:48 AM, Mikael Gerdin wrote:

On 2015-12-08 20:14, Chris Plummer wrote:

[Adding serviceability-dev@openjdk.java.net]

Hi Mikael,

Thanks for pointing this out. I'll look into it some more. Are there any
tests that should be failing as a result of this? I get the feeling no,
since I see other issues here that existed before my change. For
example, this code is not returning the proper size if the class is
anonymous or is an interface. It needs to add 1 extra word in that case.
See size() in instanceKlass.hpp.

Another difference from the VM code is alignObjectSize() is being used
by getSize(), but headerSize is set using alignObjectOffset(). The VM
code uses align_object_offset in both cases.

   // Align the object size.
   public static long alignObjectSize(long size) {
 return VM.getVM().alignUp(size,
VM.getVM().getMinObjAlignmentInBytes());
   }

   // All vm's align longs, so pad out certain offsets.
   public static long alignObjectOffset(long offset) {
 return VM.getVM().alignUp(offset, VM.getVM().getBytesPerLong());
   }

So the difference here is in the use of getMinObjAlignmentInBytes (not
what the VM does) vs getBytesPerLong (what the VM uses):

   public int getObjectAlignmentInBytes() {
 if (objectAlignmentInBytes == 0) {
 Flag flag = getCommandLineFlag("ObjectAlignmentInBytes");
 objectAlignmentInBytes = (flag == null) ? 8 :
(int)flag.getIntx();
 }
 return objectAlignmentInBytes;
   }

So this seems wrong for use in any InstanceKlass size or embedded field
offset calculation. It is probably a remnant of when class metadata was
stored in the java heap, and the size of InstanceKlass had to be padded
out to the minimum heap object alignment. At least it is harmless if
ObjectAlignmentInBytes is not set, and if set it is only supported for
64-bit:

   lp64_product(intx, ObjectAlignmentInBytes,
8, \
   "Default object alignment in bytes, 8 is
minimum")\
   range(8,
256) \
constraint(ObjectAlignmentInBytesConstraintFunc,AtParse) \

I'll get these cleaned up, but it sure would be nice if there was a way
to reliably test it.


I'd say that it's quite possible to test it!
Create a whitebox.cpp entry point for determining the size of a class.

Ok, but I instead decided to use jcmd with "GC.class_stats KlassBytes"


Sounds good.



Run a java program calling the entry point and printing the
InstanceKlass size as computed by calling InstanceKlass::size() on a
set of pre-determined set of complex and simple classes (vtables,
itables, anonymous, etc.)

For now I just did this from the command line to do some quick checking.
No test written.

Have the java program wait after it's finished printing.

Launch the SA agent and attach it to the process.
Through several layers of magic, execute the incantation:

mgerdin@mgerdin03:~/work/repos/hg/jdk9/hs-rt-work/hotspot$
../build/linux-x86_64-normal-server-slowdebug/images/jdk/bin/java
sun.jvm.hotspot.CLHSDB 6211
Attaching to process 6211, please wait...
hsdb> jseval
"sapkg.utilities.SystemDictionaryHelper.findInstanceKlass('java/lang/Object').getSize();"

472

Ok. I did this to get sizes as SA sees them. They were not just wrong
for existing JDK, but in most cases off by a large margin. I did some
investigating.  This is InstanceKlass.getSize()

   public long getSize() {
 return Oop.alignObjectSize(getHeaderSize() +
Oop.alignObjectOffset(getVtableLen()) +
Oop.alignObjectOffset(getItableLen()) +
Oop.alignObjectOffset(getNonstaticOopMapSize()));
   }

The problem is that getHeaderSize() returns the size in bytes, but the
others all return the size in words. They need to be multiplied by the
word size to get the right value since the caller of getSize() expects
the result to be in bytes.


Oh, that seems like a familiar problem :)



So we have multiple problems with SA with respect to the
InstanceKlass.getSize() support:

  * Alignment issues introduced by me.
  * Some minor other issues like using alignObjectSize when it's not
needed, and not taking into account extra fields for interfaces and
anonymous classes.
  * Not multiplying the vtable, itable, and oopMapSize by the number of
bytes in a word.
  * No test available.

I'm not too sure how to proceed here w.r.t. my CR. I'm inclined to just
make the SA adjustments needed to take into consideration the alignment
changes I've made to InstanceKlass, and file a CRs for the rest (one for
the existing bugs and one for the lack of any test).


It would be good if you could at least fix the obvious word size scaling 
bug, filing follow ups on the extra alignObjectSize and missing extra 
fiel

Re: [RFR] (M) 8143608: Don't 64-bit align start of InstanceKlass vtable, itable, and nonstatic_oopmap on 32-bit systems

[Mikael Gerdin]

On 2015-12-08 20:14, Chris Plummer wrote:

[Adding serviceability-dev@openjdk.java.net]

Hi Mikael,

Thanks for pointing this out. I'll look into it some more. Are there any
tests that should be failing as a result of this? I get the feeling no,
since I see other issues here that existed before my change. For
example, this code is not returning the proper size if the class is
anonymous or is an interface. It needs to add 1 extra word in that case.
See size() in instanceKlass.hpp.

Another difference from the VM code is alignObjectSize() is being used
by getSize(), but headerSize is set using alignObjectOffset(). The VM
code uses align_object_offset in both cases.

   // Align the object size.
   public static long alignObjectSize(long size) {
 return VM.getVM().alignUp(size,
VM.getVM().getMinObjAlignmentInBytes());
   }

   // All vm's align longs, so pad out certain offsets.
   public static long alignObjectOffset(long offset) {
 return VM.getVM().alignUp(offset, VM.getVM().getBytesPerLong());
   }

So the difference here is in the use of getMinObjAlignmentInBytes (not
what the VM does) vs getBytesPerLong (what the VM uses):

   public int getObjectAlignmentInBytes() {
 if (objectAlignmentInBytes == 0) {
 Flag flag = getCommandLineFlag("ObjectAlignmentInBytes");
 objectAlignmentInBytes = (flag == null) ? 8 : (int)flag.getIntx();
 }
 return objectAlignmentInBytes;
   }

So this seems wrong for use in any InstanceKlass size or embedded field
offset calculation. It is probably a remnant of when class metadata was
stored in the java heap, and the size of InstanceKlass had to be padded
out to the minimum heap object alignment. At least it is harmless if
ObjectAlignmentInBytes is not set, and if set it is only supported for
64-bit:

   lp64_product(intx, ObjectAlignmentInBytes,
8, \
   "Default object alignment in bytes, 8 is
minimum")\
   range(8,
256) \
constraint(ObjectAlignmentInBytesConstraintFunc,AtParse) \

I'll get these cleaned up, but it sure would be nice if there was a way
to reliably test it.


I'd say that it's quite possible to test it!
Create a whitebox.cpp entry point for determining the size of a class.

Run a java program calling the entry point and printing the 
InstanceKlass size as computed by calling InstanceKlass::size() on a set 
of pre-determined set of complex and simple classes (vtables, itables, 
anonymous, etc.)

Have the java program wait after it's finished printing.

Launch the SA agent and attach it to the process.
Through several layers of magic, execute the incantation:

mgerdin@mgerdin03:~/work/repos/hg/jdk9/hs-rt-work/hotspot$ 
../build/linux-x86_64-normal-server-slowdebug/images/jdk/bin/java 
sun.jvm.hotspot.CLHSDB 6211

Attaching to process 6211, please wait...
hsdb> jseval 
"sapkg.utilities.SystemDictionaryHelper.findInstanceKlass('java/lang/Object').getSize();"

472

You could also create a javascript source file in the test directory 
which performs the appropriate calls and do

hsdb> jsload /path/to/file.js
hsdb> jseval "doit()"

where
file.js:
doit = function() {
 sd = sapkg.utilities.SystemDictionaryHelper;
 do_klass = function(name) {
 writeln(sd.findInstanceKlass(name).getSize());
 }

 do_klass('java/lang/Object');
 do_klass('java/lang/Class');
}


The only problem is that the test can't reliably execute on unconfigured 
os x setups because of OS level security requirements for attaching to 
processes.


After detaching HSDB with the "quit" command you tell the debugged java 
process to terminate, for example by printing some string on its stdin.


Easy, right? :)

/Mikael



thanks,

Chris

On 12/8/15 1:54 AM, Mikael Gerdin wrote:

Hi Chris,

Not a review but I'm fairly sure that you need to update the
serviceability agent to reflect these changes, see for example:

public long getSize() {
  return Oop.alignObjectSize(
getHeaderSize() +
Oop.alignObjectOffset(getVtableLen()) +
Oop.alignObjectOffset(getItableLen()) +
Oop.alignObjectOffset(getNonstaticOopMapSize()));
  }

in InstanceKlass.java

/Mikael

On 2015-12-04 23:02, Chris Plummer wrote:

Hello,

Please review the following:

https://bugs.openjdk.java.net/browse/JDK-8143608
http://cr.openjdk.java.net/~cjplummer/8143608/webrev.00/webrev.hotspot/

A bit of background would help. The InstanceKlass object has a number of
variable length fields that are laid out after the declared fields. When
an InstanceKlass object is allocated, extra memory is allocated for it
to leave room for these fields. The first three of these fields are
vtable, itable, and nonstatic_oopmap. They are all arrays of HeapWord
sized values, which means void* size, which means they only need 32-bit
alignment on 32-bit systems. However, they have always been 64-

Re: RFR(M, v11): JDK-8059036 : Implement Diagnostic Commands for heap and finalizerinfo

[Mikael Gerdin]

On 2015-06-02 23:04, Dmitry Samersoff wrote:

Mikael,

The reason of placing get_filed_offset_by_name to the oop.inline is that
hotspot widely duplicate this code.

Symbol is used to identify a field within klass, not a klass them self
so I think it's OK to have it tied to the oopDesc.


Ok, I didn't relize that both the symbols were used for the field, sorry 
about that.

I still strongly feel that the method should be on InstanceKlass though.
/Mikael



-Dmitry

On 2015-06-02 15:06, Mikael Gerdin wrote:

Hi Dmitry,

On 2015-06-02 13:12, Dmitry Samersoff wrote:

Staffan,


Instead of hardcoding the field offsets, you can use
InstanceKlass::find_field and fieldDescriptor::offset to find the
offset at runtime.


Done. Please, see

http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.11

I put the function int get_filed_offset_by_name(Symbol*,Symbol*) to
oop.inline.hpp leaving a room for further cleanup because I found couple
of places in hotspot that implements mostly similar functionality.


Sorry for this sort of drive-by review, but I really don't think
get_field_offset_by_name should be in the oop class. If anywhere it
should be on InstanceKlass, and using Symbol* to identify a Klass* seems
incorrect since the same symbol can refer to different classes in
different class loader contexts.

/Mikael



-Dmitry

On 2015-06-01 10:18, Staffan Larsen wrote:

Dmitry,

Instead of hardcoding the field offsets, you can use
InstanceKlass::find_field and fieldDescriptor::offset to find the
offset at runtime.

Thanks,
/Staffan


On 31 maj 2015, at 13:43, Dmitry Samersoff
 wrote:

Everyone,

Please take a look at new version of the fix.

http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.10/

Changes (to previous version) are in
Finalizer.java and diagnosticCommand.cpp

This version copy data from Map.Entry<> to array of
FinalizerHistogramEntry instances then,
VM prints content of this array.

-Dmitry


On 2015-05-28 21:06, Mandy Chung wrote:


On 05/28/2015 07:35 AM, Peter Levart wrote:

Hi Mandy,

On 05/27/2015 03:32 PM, Mandy Chung wrote:

Taking it further - is it simpler to return String[] of all
classnames including the duplicated ones and have the VM do the
count?  Are you concerned with the size of the String[]?


Yes, the histogram is much smaller than the list of all instances.
There can be millions of instances waiting in finalizer queue, but
only a few distinct classes.


Do you happen to know what libraries are the major contributors to
these
millions of finalizers?

It has been strongly recommended to avoid finalizers (see Effective
Java
Item 7).   I'm not surprised that existing code is still using
finalizers while we should really encourage them to migrate away
from it.


What could be done in Java to simplify things in native code but
still
not format the output is to convert the array of Map.Entry(s) into an
Object[] array of alternating {String, int[], String, int[],  }

Would this simplify native code for the price of a little extra work
in Java? The sizes of old and new arrays are not big (# of distinct
classes of finalizable objects in the queue).


I also prefer writing in Java and writing less native code (much
simplified).  It's an interface that we have to agree upon and keep it
simple.  Having the returned Object[] as alternate String and int[]
is a
reasonable compromise.

ReferenceQueue.java - you can move @SuppressWarning from the method to
just the field variable "rn"
  @SuppressWarnings("unchecked")
  Reference rn = r.next;

Finalizer.java
  It's better to rename printFinalizationQueue as it inspects the
finalizer reference queue (maybe getFinalizerHistogram?).  Can you
move
this method to the end of this class and add the comment saying that
this is invoked by VM for jcmd -finalizerinfo support and @return to
describe the returned content.  I also think you can remove
@SuppressWarnings for this method.

Mandy



--
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(M, v11): JDK-8059036 : Implement Diagnostic Commands for heap and finalizerinfo

[Mikael Gerdin]

Hi Dmitry,

On 2015-06-02 13:12, Dmitry Samersoff wrote:

Staffan,


Instead of hardcoding the field offsets, you can use
InstanceKlass::find_field and fieldDescriptor::offset to find the
offset at runtime.


Done. Please, see

http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.11

I put the function int get_filed_offset_by_name(Symbol*,Symbol*) to
oop.inline.hpp leaving a room for further cleanup because I found couple
of places in hotspot that implements mostly similar functionality.


Sorry for this sort of drive-by review, but I really don't think 
get_field_offset_by_name should be in the oop class. If anywhere it 
should be on InstanceKlass, and using Symbol* to identify a Klass* seems 
incorrect since the same symbol can refer to different classes in 
different class loader contexts.


/Mikael



-Dmitry

On 2015-06-01 10:18, Staffan Larsen wrote:

Dmitry,

Instead of hardcoding the field offsets, you can use InstanceKlass::find_field 
and fieldDescriptor::offset to find the offset at runtime.

Thanks,
/Staffan


On 31 maj 2015, at 13:43, Dmitry Samersoff  wrote:

Everyone,

Please take a look at new version of the fix.

http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.10/

Changes (to previous version) are in
Finalizer.java and diagnosticCommand.cpp

This version copy data from Map.Entry<> to array of
FinalizerHistogramEntry instances then,
VM prints content of this array.

-Dmitry


On 2015-05-28 21:06, Mandy Chung wrote:


On 05/28/2015 07:35 AM, Peter Levart wrote:

Hi Mandy,

On 05/27/2015 03:32 PM, Mandy Chung wrote:

Taking it further - is it simpler to return String[] of all
classnames including the duplicated ones and have the VM do the
count?  Are you concerned with the size of the String[]?


Yes, the histogram is much smaller than the list of all instances.
There can be millions of instances waiting in finalizer queue, but
only a few distinct classes.


Do you happen to know what libraries are the major contributors to these
millions of finalizers?

It has been strongly recommended to avoid finalizers (see Effective Java
Item 7).   I'm not surprised that existing code is still using
finalizers while we should really encourage them to migrate away from it.


What could be done in Java to simplify things in native code but still
not format the output is to convert the array of Map.Entry(s) into an
Object[] array of alternating {String, int[], String, int[],  }

Would this simplify native code for the price of a little extra work
in Java? The sizes of old and new arrays are not big (# of distinct
classes of finalizable objects in the queue).


I also prefer writing in Java and writing less native code (much
simplified).  It's an interface that we have to agree upon and keep it
simple.  Having the returned Object[] as alternate String and int[] is a
reasonable compromise.

ReferenceQueue.java - you can move @SuppressWarning from the method to
just the field variable "rn"
 @SuppressWarnings("unchecked")
 Reference rn = r.next;

Finalizer.java
 It's better to rename printFinalizationQueue as it inspects the
finalizer reference queue (maybe getFinalizerHistogram?).  Can you move
this method to the end of this class and add the comment saying that
this is invoked by VM for jcmd -finalizerinfo support and @return to
describe the returned content.  I also think you can remove
@SuppressWarnings for this method.

Mandy



--
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: [urgent] RFR: 8081470 com/sun/jdi tests are failing with "Error. failed to clean up files after test" with jtreg 4.1 b12

[Mikael Gerdin]

Hi Staffan,

On 2015-05-29 10:53, Staffan Larsen wrote:

Can I have a fast review of the following change that is currently blocking 
hotspot pushes in jprt. jtreg 4.1b12 adds stricter checking of @library tags. 
Some com/sun/jdi tests have @library clauses that are not needed.

I do not intend to wait 24 hours before pushing this...

webrev: http://cr.openjdk.java.net/~sla/8081470/webrev.00/ 



Looks good, Reviewed.

/Mikael


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


Thanks,
/Staffan

Re: RFR: JDK-8072913: [REDO] GCCause should distinguish jcmd GC.run from System.gc()

[Mikael Gerdin]

Hi,

On 2015-03-11 14:13, Yasumasa Suenaga wrote:

Hi all,


So I think we can remove _jvmti_force_gc from is_user_requested_gc()
and add _dcmd_gc_run
to it.


I've uploaded new webrev, and I've applied it to new patch.
Could you review it?

http://cr.openjdk.java.net/~ysuenaga/JDK-8072913/webrev.01/


Sorry for the delay, I've been pretty busy lately.
Does anyone have time to look at this change?

/Mikael



I also updated jtreg testcase.
It works fine in my environment.


Thanks,

Yasumasa


On 2015/02/14 22:10, Yasumasa Suenaga wrote:

Hi Mikael,


I'd prefer if you could add a GCCause::is_system_gc_equivalent()
which returns true for some set of GCCause enum values, such as
_java_lang_system_gc and _dcmd_gc_run


Can I add _dcmd_gc_run to GCCause::is_user_requested_gc() ?
This function is used with GCCause::is_serviceability_requested_gc() .
CMSCollector::is_external_interruption() and
AdaptiveSizePolicy::check_gc_overhead_limit()

is_user_requested_gc() and is_serviceability_requested_gc() checkes
_jvmti_force_gc
is selected.
So I think we can remove _jvmti_force_gc from is_user_requested_gc()
and add _dcmd_gc_run
to it.


A "grep" for _java_lang_system_gc should yield more places where
updates may be necessary.


We can use GCCause::is_user_requested_gc() if the proposal in above is
accepted.


Thanks

Yasumasa



On 2015/02/13 21:33, Mikael Gerdin wrote:

Hi Yasumasa,

On 2015-02-11 15:02, Yasumasa Suenaga wrote:

Hi all,

I've committed JDK-8068589 to add new GCCause - Diagnostic Command.
However, it has been backouted because test is failed [1] and it is
not considered
about concurrent GC: -XX:+ExplicitGCInvokesConcurrent [2].

I've created patch for this enhancement.
Could you review it?

http://cr.openjdk.java.net/~ysuenaga/JDK-8072913/webrev.00/


I'd prefer if you could add a GCCause::is_system_gc_equivalent()
which returns true for some set of GCCause enum values, such as
_java_lang_system_gc and _dcmd_gc_run

Given that the documentation of the GC.run command is:
"GC.run
Call java.lang.System.gc().

Impact: Medium: Depends on Java heap size and content.

Syntax: GC.run"

I interpret the documentation that the GC is supposed to be (for all
intents and purposes) equivalent to the application invoking
System.gc().

This would also require updates to other places where we refer to the
_java_lang_system_gc GCCause, such as UseAdaptiveSizePolicyWithSystemGC

A "grep" for _java_lang_system_gc should yield more places where
updates may be necessary.

/Mikael




I'm jdk9 committer, but I'm not employee at Oracle.
So I need a Sponsor.


Thanks,

Yasumasa


[1]
http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2015-February/011957.html

[2]
http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2015-February/011962.html

Re: RFR: 8073688: Infinite loop reading types during jmap attach.

[Mikael Gerdin]

On 2015-03-03 14:57, Kevin Walls wrote:


Hi,

Yes that's true - the original problem is in 7u.  We'll have it around
for a while yet so I would like to get this change there to protect us.

While we can't reproduce the same thing on 9 today, there's still a
while loop that will never terminate if we read a zero for the stride
size, for example.  I think it's worth protecting ourselves from that
kind of problem.


That's probably a good idea to fix then.

I'll leave the rest for people who are qualified to review the code :)

/Mikael



Thanks
Kevin

On 03/03/2015 13:51, Mikael Gerdin wrote:

Kevin,

On 2015-03-03 14:15, Kevin Walls wrote:

Hi,

This is a review request for a way to make the SA tools protect
themselves from infinite loops during initialisation.

Attaching jmap (for example) to a JVM can fail, infinitely writing an
error - and filling a disk if being logged to a file.  This reproduces
on a Solaris package based install, but not with other distribution
bundles.  In those packages, there's a link JDK/jre/lib/sparc/libjvm ->
client/libjvm.so.  If a server/libjvm.so is loaded and running, we see
libverify.so pull in client/libjvm.so, as it finds the symlink in its
$ORIGIN, in preference to finding the already loaded libjvm.so.


There is only server/libjvm.so on Solaris since JDK 8, so I assume
that this RFR is for JDK 7u only. The late stage of the JDK 7u updates
project should perhaps be considered when looking at this change.

/Mikael



Symbol lookup in the SA is fooled by having multiple libjvm.so loaded.
There are various things wrong with that.  Protection against zero
strides through the type arrays and a maximum count for duplicated types
will protect us from a few possible problems.

I'll also work a bug for the "install" repo where we create that
symlink, but the tools need to protect themselves from this kind of
problem.

Testing was manual.

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

webrev
http://cr.openjdk.java.net/~kevinw/8073688/webrev.00/

Thanks
Kevin

Re: RFR: 8073688: Infinite loop reading types during jmap attach.

[Mikael Gerdin]

Kevin,

On 2015-03-03 14:15, Kevin Walls wrote:

Hi,

This is a review request for a way to make the SA tools protect
themselves from infinite loops during initialisation.

Attaching jmap (for example) to a JVM can fail, infinitely writing an
error - and filling a disk if being logged to a file.  This reproduces
on a Solaris package based install, but not with other distribution
bundles.  In those packages, there's a link JDK/jre/lib/sparc/libjvm ->
client/libjvm.so.  If a server/libjvm.so is loaded and running, we see
libverify.so pull in client/libjvm.so, as it finds the symlink in its
$ORIGIN, in preference to finding the already loaded libjvm.so.


There is only server/libjvm.so on Solaris since JDK 8, so I assume that 
this RFR is for JDK 7u only. The late stage of the JDK 7u updates 
project should perhaps be considered when looking at this change.


/Mikael



Symbol lookup in the SA is fooled by having multiple libjvm.so loaded.
There are various things wrong with that.  Protection against zero
strides through the type arrays and a maximum count for duplicated types
will protect us from a few possible problems.

I'll also work a bug for the "install" repo where we create that
symlink, but the tools need to protect themselves from this kind of
problem.

Testing was manual.

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

webrev
http://cr.openjdk.java.net/~kevinw/8073688/webrev.00/

Thanks
Kevin

Re: DTraceAllocProbes bug in interpreter

[Mikael Gerdin]

Hi Jungwoo,

The DTrace probes are part of the serviceability support, bcc:ing gc-dev 
and redirecting to serviceability-dev.


/Mikael

On 2015-02-14 02:21, Jungwoo Ha wrote:

I am not sure if DTraceAllocProbes is a live code, but I think this is a
bug.
dtrace_object_alloc has changed to receive size argument in JDK8.
This code only passes oop to dtrace_object_alloc. RDX has the size but
it needs to be saved.
I don't use DTrace, but it looks broken. If someone sponsor this is a
right fix, I'll create a webrev.

diff -r f06c7b654d63 src/cpu/x86/vm/templateTable_x86_32.cpp
--- a/src/cpu/x86/vm/templateTable_x86_32.cppThu Jul 31 09:58:53 2014 +0100
+++ b/src/cpu/x86/vm/templateTable_x86_32.cppFri Feb 13 17:14:57 2015 -0800
@@ -3288,6 +3288,7 @@
  // The object is initialized before the header.  If the object size is
  // zero, go directly to the header initialization.
  __ bind(initialize_object);
+__ push(rdx);  // save object size
  __ decrement(rdx, sizeof(oopDesc));
  __ jcc(Assembler::zero, initialize_header);
@@ -3328,13 +3329,14 @@
__ pop(rcx);   // get saved klass back in the register.
  }
  __ store_klass(rax, rcx);  // klass
+__ pop(rdx);  // restore object size to rdx
  {
SkipIfEqual skip_if(_masm, &DTraceAllocProbes, 0);
// Trigger dtrace event for fastpath
__ push(atos);
__ call_VM_leaf(
-   CAST_FROM_FN_PTR(address,
SharedRuntime::dtrace_object_alloc), rax);
+   CAST_FROM_FN_PTR(address,
SharedRuntime::dtrace_object_alloc), rax, rdx);
__ pop(atos);
  }
diff -r f06c7b654d63 src/cpu/x86/vm/templateTable_x86_64.cpp
--- a/src/cpu/x86/vm/templateTable_x86_64.cppThu Jul 31 09:58:53 2014 +0100
+++ b/src/cpu/x86/vm/templateTable_x86_64.cppFri Feb 13 17:14:57 2015 -0800
@@ -3352,6 +3352,7 @@
  // The object is initialized before the header.  If the object size is
  // zero, go directly to the header initialization.
  __ bind(initialize_object);
+__ movl(rbx, rdx);  // save object size to rbx
  __ decrementl(rdx, sizeof(oopDesc));
  __ jcc(Assembler::zero, initialize_header);
@@ -3386,7 +3387,7 @@
// Trigger dtrace event for fastpath
__ push(atos); // save the return value
__ call_VM_leaf(
-   CAST_FROM_FN_PTR(address,
SharedRuntime::dtrace_object_alloc), rax);
+   CAST_FROM_FN_PTR(address,
SharedRuntime::dtrace_object_alloc), rax, rbx);
__ pop(atos); // restore the return value
  }

Re: RFR: JDK-8072913: [REDO] GCCause should distinguish jcmd GC.run from System.gc()

[Mikael Gerdin]

Hi Yasumasa,

On 2015-02-11 15:02, Yasumasa Suenaga wrote:

Hi all,

I've committed JDK-8068589 to add new GCCause - Diagnostic Command.
However, it has been backouted because test is failed [1] and it is not 
considered
about concurrent GC: -XX:+ExplicitGCInvokesConcurrent [2].

I've created patch for this enhancement.
Could you review it?

http://cr.openjdk.java.net/~ysuenaga/JDK-8072913/webrev.00/


I'd prefer if you could add a GCCause::is_system_gc_equivalent() which 
returns true for some set of GCCause enum values, such as 
_java_lang_system_gc and _dcmd_gc_run


Given that the documentation of the GC.run command is:
"GC.run
Call java.lang.System.gc().

Impact: Medium: Depends on Java heap size and content.

Syntax: GC.run"

I interpret the documentation that the GC is supposed to be (for all 
intents and purposes) equivalent to the application invoking System.gc().


This would also require updates to other places where we refer to the 
_java_lang_system_gc GCCause, such as UseAdaptiveSizePolicyWithSystemGC


A "grep" for _java_lang_system_gc should yield more places where updates 
may be necessary.


/Mikael




I'm jdk9 committer, but I'm not employee at Oracle.
So I need a Sponsor.


Thanks,

Yasumasa


[1] 
http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2015-February/011957.html
[2] 
http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2015-February/011962.html

Re: A hotspot patch for stack profiling (frame pointer)

[Mikael Gerdin]

Maynard,

On 2014-12-08 16:05, Maynard Johnson wrote:

On 12/05/2014 05:09 PM, Brendan Gregg wrote:

G'Day Volker,

On Fri, Dec 5, 2014 at 11:22 AM, Volker Simonis
 wrote:

Hi Brendan,

I'm still not understanding who is taking the actual stack traces (let
alone the symbols) in your examples. Is this done by 'perf' itself
based only on the frame pointer?


perf is walking the frame pointers.

Volker, to be specific, the perf profiling tool has a user space part and a
kernel space part. The collection of stack traces is done by the kernel.
When a user-specified event (or series of events) occur, the process
being profiled is interrupted and the sampled information (which can
optionally include a full stack trace) is made available to the user space
perf tool to be saved to a file for future post-profiling processing.

During the profiling phase, the perf tool collects information about the
profiled process's memory mappings, which allows for this address-to-symbol.
resolution, It's in the post-profiling phase where the sampled instruction,
along with its associated stack trace, are resolved to the appropriate symbol
(i.e., function/method) in a specific binary file (e.g., library, exectuable).

And if the VM creates a /tmp/perf-.map file to save information about
JITed methods, the perf's post-profiling tool will find it and use it to
correlate sampled addresses it collected from the VM's executable anonymous
memory mappings to the method names.


I seem to recall reading about perf having support for DWARF debug info.

If the VM (or a JVM/TI agent) could create DWARF debug symbols, could 
that be used to convey information about inlined functions and stack 
unwinding without frame pointers?
I realize that emitting DWARF debug symbols for generated code is not a 
trivial undertaking but since perf is running sampling in the kernel and 
we can't disable inlining that seems to be one of the few ways we can 
get complete stack traces.


There would be several other advantages to having DWARF symbols for 
generated code, GDB can use them when debugging the JVM for example.


An alternate approach could be to extend the information in 
perf-.map to have more detailed PC ranges with information about 
which functions are inlined. A lot of that information is available in 
the VM but not necessarily exposed via the tool APIs


/Mikael



-Maynard


A JVMTI agent, perf-map-agent, is providing a map file for symbol
translation under /tmp/perf-PID.map. Linux perf already hunts for such
a file when doing symbol translation.



As I wrote before, this is pretty hard to get right for a JVM, but
there are good approximations. Have you looked at the 'jstack' tool
which is part of the JDK? If you run it on a Java process, it will
give you exact stack traces with full inlining information. However
this only works at safepoints so it is probably not suitable for
profiling with performance counters.


Right, jstack works, and I get full correct stacks. I do really want
to take stacks at any moment: not just CPU samples, but when tracing
kernel TCP events, or PMC cache miss profiling, etc. perf can already
do many advanced tracing and profiling activities. I just needed the
Java stacks for context.


But you can also use 'jstack -F
-m' which gives you a 'best effort' mixed Java/C++ stacaktrace (most
of the time even with inlined Java frames. This is probably the best
you can get when interrupting a running JVM at an arbitrary point in
time. As you mentioned in one of your blogs, the VM can be in the
C-Library or even in the kernel at that time which don't preserve the
frame pointer either. So it will be already hard to even walk up to
the first Java frame.


Well, the JVMs I'm looking at are already built with
-fno-omit-frame-pointer (which is good). I edited hotspot to preserve
it as well.

Here's before I changed hotspot:

http://www.brendangregg.com/FlameGraphs/cpu-mixedmode-nofp.svg

Yes, most stacks are clearly broken.

After changing hotspot:

http://www.brendangregg.com/FlameGraphs/cpu-mixedmode-vertx.svg

It's looking pretty good. If you look carefully on the far left and
right, there are 0.8% stacks in read() and write() directly from java,
which may well be broken (unless a java thread is calling these
directly; there could also be some gcc inlining going on). Even if
they are broken, I can see 98% of my profile. Plus, I'd be interested
to know what exactly is reusing the frame pointer, so we could fix
that too.

The Java stacks themselves are also about a third as deep as they
should be, due to inlining.



But nevertheless, if the output of 'jstack -F -m' is "good enough" for
your purpose, you can implement something similar in 'perf' or a
helper library of 'perf' and be happy (I don't actually know how perf
takes stack traces but I suppose there may some kind of callback
mechanism for walking unknown frames). This is actually not so hard.
I've recently implemented a "print_native_stack()" function within
hotspot 

Re: Fwd: Disastrous bug when running jinfo and jmap

[Mikael Gerdin]

Hi,

This is the expected behavior for jmap and jinfo. If you call jstack 
with the "-F" flag you will see the same behavior.


The reason for this is that jmap, jinfo and jstack -F all attach to your 
target JVM as a debugger and read the memory from the process. That 
needs to be done when the target process is in a frozen state.


/Mikael

On 2014-09-02 11:08, tobe wrote:

When I run jinfo or jmap to any Java process, it will "suspend" the Java
process. It's 100% reproduced for the long running processes.

Here're the detailed steps:

1. Pick a Java process which is running over 25 days(It's wired because
this doesn't work for new processes).
2. Run ps to check the state of the process, should be "Sl" which is
expected.
3. Run jinfo or jmap to this process(BTY, jstack doesn't have this issue).
4. Run ps to check the state of the process. This time it changes to "Tl"
which means STOPPED and the process doesn't response any requests.

Here's the output of our process:

[work@hadoop ~]$ ps aux |grep "qktst" |grep "RegionServer"
work 36663  0.1  1.7 24157828 1150820 ?Sl   Aug06  72:54
/opt/soft/jdk/bin/java -cp
/home/work/app/hbase/qktst-qk/regionserver/:/home/work/app/hbase/qktst-qk/regionserver/package//:/home/work/app/hbase/qktst-qk/regionserver/package//lib/*:/home/work/app/hbase/qktst-qk/regionserver/package//*
-Djava.library.path=:/home/work/app/hbase/qktst-qk/regionserver/package/lib/native/:/home/work/app/hbase/qktst-qk/regionserver/package/lib/native/Linux-amd64-64
-Xbootclasspath/p:/home/work/app/hbase/qktst-qk/regionserver/package/lib/hadoop-security-2.0.0-mdh1.1.0.jar
-Xmx10240m -Xms10240m -Xmn1024m -XX:MaxDirectMemorySize=1024m
-XX:MaxPermSize=512m
-Xloggc:/home/work/app/hbase/qktst-qk/regionserver/stdout/regionserver_gc_20140806-211157.log
-Xss256k -XX:PermSize=64m -XX:+HeapDumpOnOutOfMemoryError
-XX:HeapDumpPath=/home/work/app/hbase/qktst-qk/regionserver/log
-XX:+PrintGCApplicationStoppedTime -XX:+UseConcMarkSweepGC -verbose:gc
-XX:+PrintGCDetails -XX:+PrintGCDateStamps -XX:SurvivorRatio=6
-XX:+UseCMSCompactAtFullCollection -XX:CMSInitiatingOccupancyFraction=75
-XX:+UseCMSInitiatingOccupancyOnly -XX:+CMSParallelRemarkEnabled
-XX:+UseNUMA -XX:+CMSClassUnloadingEnabled
-XX:CMSMaxAbortablePrecleanTime=1 -XX:TargetSurvivorRatio=80
-XX:+UseGCLogFileRotation -XX:NumberOfGCLogFiles=100 -XX:GCLogFileSize=128m
-XX:CMSWaitDuration=2000 -XX:+CMSScavengeBeforeRemark
-XX:+PrintPromotionFailure -XX:ConcGCThreads=16 -XX:ParallelGCThreads=16
-XX:PretenureSizeThreshold=2097088 -XX:+CMSConcurrentMTEnabled
-XX:+ExplicitGCInvokesConcurrent -XX:+SafepointTimeout
-XX:MonitorBound=16384 -XX:-UseBiasedLocking -XX:MaxTenuringThreshold=3
-Dproc_regionserver
-Djava.security.auth.login.config=/home/work/app/hbase/qktst-qk/regionserver/jaas.conf
-Djava.net.preferIPv4Stack=true
-Dhbase.log.dir=/home/work/app/hbase/qktst-qk/regionserver/log
-Dhbase.pid=36663 -Dhbase.cluster=qktst-qk -Dhbase.log.level=debug
-Dhbase.policy.file=hbase-policy.xml
-Dhbase.home.dir=/home/work/app/hbase/qktst-qk/regionserver/package
-Djava.security.krb5.conf=/home/work/app/hbase/qktst-qk/regionserver/krb5.conf
-Dhbase.id.str=work org.apache.hadoop.hbase.regionserver.HRegionServer start
[work@hadoop ~]$ jinfo 36663 > tobe.jinfo
Attaching to process ID 36663, please wait...
Debugger attached successfully.
Server compiler detected.
JVM version is 20.12-b01
[work@hadoop ~]$ ps aux |grep "qktst" |grep "RegionServer"
work 36663  0.1  1.7 24157828 1151008 ?Tl   Aug06  72:54
/opt/soft/jdk/bin/java -cp
/home/work/app/hbase/qktst-qk/regionserver/:/home/work/app/hbase/qktst-qk/regionserver/package//:/home/work/app/hbase/qktst-qk/regionserver/package//lib/*:/home/work/app/hbase/qktst-qk/regionserver/package//*
-Djava.library.path=:/home/work/app/hbase/qktst-qk/regionserver/package/lib/native/:/home/work/app/hbase/qktst-qk/regionserver/package/lib/native/Linux-amd64-64
-Xbootclasspath/p:/home/work/app/hbase/qktst-qk/regionserver/package/lib/hadoop-security-2.0.0-mdh1.1.0.jar
-Xmx10240m -Xms10240m -Xmn1024m -XX:MaxDirectMemorySize=1024m
-XX:MaxPermSize=512m
-Xloggc:/home/work/app/hbase/qktst-qk/regionserver/stdout/regionserver_gc_20140806-211157.log
-Xss256k -XX:PermSize=64m -XX:+HeapDumpOnOutOfMemoryError
-XX:HeapDumpPath=/home/work/app/hbase/qktst-qk/regionserver/log
-XX:+PrintGCApplicationStoppedTime -XX:+UseConcMarkSweepGC -verbose:gc
-XX:+PrintGCDetails -XX:+PrintGCDateStamps -XX:SurvivorRatio=6
-XX:+UseCMSCompactAtFullCollection -XX:CMSInitiatingOccupancyFraction=75
-XX:+UseCMSInitiatingOccupancyOnly -XX:+CMSParallelRemarkEnabled
-XX:+UseNUMA -XX:+CMSClassUnloadingEnabled
-XX:CMSMaxAbortablePrecleanTime=1 -XX:TargetSurvivorRatio=80
-XX:+UseGCLogFileRotation -XX:NumberOfGCLogFiles=100 -XX:GCLogFileSize=128m
-XX:CMSWaitDuration=2000 -XX:+CMSScavengeBeforeRemark
-XX:+PrintPromotionFailure -XX:ConcGCThreads=16 -XX:ParallelGCThreads=16
-XX:PretenureSizeThreshold=2097088 -XX:+CMSConcurrentMTEnabled
-XX:+ExplicitGCInvokesConcurrent -XX:

Re: RFR: 8035939: java/lang/management/MemoryMXBean/MemoryManagement.java timed out on Linux-amd64

[Mikael Gerdin]

Hi Stefan,

On 2014-08-19 12:25, Stefan Karlsson wrote:

Hi all,

Please review this patch harden two MemoryMXBean tests. These tests
cause intermittent test failures when the allocated objects are put in
the young gen instead of the old gen. The proposed fix/workaround is to
lower the young gen size and assert that the allocated objects are large
enough to not fit in the young gen.

http://cr.openjdk.java.net/~stefank/8035939/webrev.00/


Looks good.

/Mikael


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

thanks,
StefanK

Re: RFR (backport of): 8028391 - Make the Min/MaxHeapFreeRatio flags manageable

[Mikael Gerdin]

Hi Jesper,

On Tuesday 04 February 2014 10.17.16 Jesper Wilhelmsson wrote:
> Hi,
> 
> The patch from jdk9/hs-gc to make MinHeapFreeRatio and MaxHeapFreeRatio
> manageable flags and supported in ParallelGC applied cleanly to
> jdk8u/hs-dev.
> 
> hsx/jdk7u60 required some manual labor to patch since the argument parsing
> code had moved around, but the relevant logic was untouched so I didn't
> have to change anything in the new code. For 7 I omitted some of the minor
> cleanups that was done in the original patch.
> 
> Webrev for 7: http://cr.openjdk.java.net/~jwilhelm/8028391/webrev.7u60/

The 7u60 backport looks good to me.

> Webrev for 8: http://cr.openjdk.java.net/~jwilhelm/8028391/webrev.8u20/
> 
> (The 8u20 webrev is identical with the final version for 9.)

In that case, you don't need to ask for reviews for the 8u20 backport, you can 
just go ahead and push it.

/Mikael

> 
> So kindly approve this change for backport to jdk8u20 and jdk7u60.
> 
> The bugs:
> jdk9: https://bugs.openjdk.java.net/browse/JDK-8028391
> jdk8: https://bugs.openjdk.java.net/browse/JDK-8033209
> jdk7: https://bugs.openjdk.java.net/browse/JDK-8028720
> 
> Reviews in this thread:
> http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2014-January/009187.ht
> ml
> 
> The jdk9 changeset:
> http://hg.openjdk.java.net/jdk9/hs-gc/hotspot/rev/44315152d434
> 
> Thanks,
> /Jesper

Re: RFR: 8028391 - Make the Min/MaxHeapFreeRatio flags manageable

[Mikael Gerdin]

Hi Jesper,

On Monday 27 January 2014 21.46.01 Jesper Wilhelmsson wrote:
> Staffan, Bengt, Mikael,
> 
> Thanks for the reviews!
> 
> I have made the changes you have suggested and a new webrev is available at:
> 
> http://cr.openjdk.java.net/~jwilhelm/8028391/webrev.5/
> 
> I agree with your assessment that it would be good to implement a generic
> way to verify manageable flags. I think it is a separate change though so I
> will not attack that problem in this change.
> 
> As Mikael wrote in his review we have talked offline about the changes and
> how to make them more correct and readable. Thanks Mikael for the input!

The calculations are much easier to follow now, thanks.

I think the changes are good.

/Mikael

> 
> More comments inline.
> 
> Bengt Rutisson skrev 22/1/14 11:21 AM:
> > Hi Jesper,
> > 
> > The calculation in
> > PSAdaptiveSizePolicy::calculated_old_free_size_in_bytes()> 
> > looks wrong to me. I would have expected this:
> >86 // free = (live*ratio) / (1-ratio)
> >87 size_t max_free = (size_t)((heap->old_gen()->used_in_bytes() *
> > 
> > mhfr_as_percent) / (1.0 - mhfr_as_percent));
> > 
> > to be something like this:
> >   size_t max_free = heap->old_gen()->capacity_in_bytes() *
> >   mhfr_as_percent;
> 
> The suggested formula above will calculate how much free memory there can be
> based on the current old gen size. What I want to achieve in the code is to
> calculate how much free memory there can be based on the amount of live
> data in the old generation. I have talked to Bengt offline and he agrees
> that the code is doing what I want it to. I have rewritten the code and
> added more comments to explain the formula.
> 
> > (A minor naming thing is that mhfr_as_percent is actually not a percent
> > but a ratio or fraction. Just like you write in the comment.)
> 
> Right. Fixed.
> 
> > We also don't seem to take MinHeapFreeRatio into account. Should we do
> > that?
> We should. Good catch! I have added support for MinHeapFreeRatio both here
> and in psScavenge.cpp.
> 
> > I think it should be possible to write a internal VM test or a whitebox
> > test for the calculated_old_free_size_in_bytes() to verify that it
> > produces the correct results.
> 
> I've added an internal test to verify the new code.
> 
> > Speaking of testing. There is already a test called
> > test/gc/arguments/TestHeapFreeRatio.java. That test seems to pass with the
> > ParallelGC already before your changes. I think that means that the test
> > is not strict enough. Could you update that test or add a new test to
> > make sure that your changes are tested?
> 
> TestHeapFreeRatio only verifies that the VM gives correct error messages for
> the -Xminf and -Xmaxf flags. Since HotSpot usually don't complain about
> flags that don't affect the chosen GC, there is no error given about
> ParallelGC not implementing the heap ratio flags. The code I change is not
> tested by this test. Dmitry Fazunenko has developed a test for the new
> feature which I have used while developing. This test will be pushed once
> the feature is in place.
> > I also agree with Staffan that the methods is_within() and is_min() make
> > it
> > harder to read the code.
> 
> Yes, me to...
> I removed them.
> 
> Thanks,
> /Jesper
> 
> > Thanks,
> > Bengt
> > 
> > On 2014-01-22 09:40, Staffan Larsen wrote:
> >> Jesper,
> >> 
> >> This looks ok from a serviceability perspective. Long term we should
> >> probably have a more pluggable way to verify values of manageable flags
> >> so we can avoid some of the duplication.
> >> 
> >> I have a slight problem with is_within() and is_min() in that it is not
> >> obvious from the call site if the min and max values are inclusive or not
> >> - it was very obvious before.
> >> 
> >> /Staffan
> >> 
> >> 
> >> On 21 jan 2014, at 22:49, Jesper Wilhelmsson
> >> >> 
> >> wrote:
> >>> Hi,
> >>> 
> >>> Could I have a few reviews of this change?
> >>> 
> >>> Summary:
> >>> To allow applications a more fine grained control over the GC over time,
> >>> we'll make the flags MinHeapFreeRatio and MaxHeapFreeRatio manageable.
> >>> 
> >>> The initial request that lead up to this change involved ParallelGC
> >>> which is notoriously unwilling to shrink the heap. Since ParallelGC
> >>> didn't support the heap free ratio flags, this change also includes
> >>> implementing support for these flags in ParallelGC.
> >>> 
> >>> Changes have also been made to the argument parsing, attach listener and
> >>> the management API to verify the flag values when set through the
> >>> different interfaces.
> >>> 
> >>> Webrev: http://cr.openjdk.java.net/~jwilhelm/8028391/webrev.4/
> >>> 
> >>> Bug: https://bugs.openjdk.java.net/browse/JDK-8028391
> >>> 
> >>> The plan is to push this to 9 and then backport to 8 and 7.
> >>> 
> >>> Thanks!
> >>> /Jesper

Re: RFR: 8028391 - Make the Min/MaxHeapFreeRatio flags manageable

[Mikael Gerdin]

Hi Jesper,

On Tuesday 21 January 2014 22.49.19 Jesper Wilhelmsson wrote:
> Hi,
> 
> Could I have a few reviews of this change?
> 
> Summary:
> To allow applications a more fine grained control over the GC over time,
> we'll make the flags MinHeapFreeRatio and MaxHeapFreeRatio manageable.
> 
> The initial request that lead up to this change involved ParallelGC which is
> notoriously unwilling to shrink the heap. Since ParallelGC didn't support
> the heap free ratio flags, this change also includes implementing support
> for these flags in ParallelGC.
> 
> Changes have also been made to the argument parsing, attach listener and the
> management API to verify the flag values when set through the different
> interfaces.
> 
> Webrev: http://cr.openjdk.java.net/~jwilhelm/8028391/webrev.4/

In psAdaptiveSizePolicy.cpp:
  83 ParallelScavengeHeap* heap = (ParallelScavengeHeap*)Universe::heap();
You can use ParallelScavengeHeap::heap() here instead.

I had an offline discussion with Jesper with notebooks and formulas to figure 
out how Min/MaxHeapFreeRatio should be used and how to change the psScavenge 
change.

I have a suggestion for how to do verification as well:

if you add a   
bool Flag::is_equal(void* flagptr) const { return _addr == flagptr; }

you can have a sort of generic verify_flag function with only a pointer check 
and some ifs instead of doing string compares:

template 
bool CommandLineFlags::verify_flag(Flag* flag, T value) {
  if (flag->is_equal(&MinHeapFreeRatio)) {
if (is_percentage(value)) {

}

if (value <= MaxHeapFreeRatio) {

}
  }

  if (flag->is_equal(&MaxHeapFreeRatio)) { ... }

  return something;
}

Another, perhaps too complex, approach would be to trade the _type char* for a 
vtable pointer for Flags and have BooleanFlag, FloatFlag, etc. and make the 
_type field based on virtual dispatch.
Then each flag which needed verification could override a virtual verify() 
function.

I'm not going to insist on changing this but we really need to fix this if we 
add one more manageable flag which needs bounds checking or some other type of 
verification.

/Mikael

> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8028391
> 
> The plan is to push this to 9 and then backport to 8 and 7.
> 
> Thanks!
> /Jesper

Re: RFR: JDK-8029395 SA: jstack throws WrongTypeException

[Mikael Gerdin]

Staffan,

On Monday 02 December 2013 17.30.00 Staffan Larsen wrote:
> The problem here happens when SA wants to walk every object in the heap, to
> do that it needs to figure out what parts of the heap are in active use
> (the "live regions"). It gets the list of GC spaces as a start. It then
> maps out the TLABs in these spaces so that it will not walk un-allocated
> memory in the TLABs.
> 
> In this case, it misses one (or more) active TLABs and so tries to walk
> memory that is part of a TLAB, but that has not been allocated to an object
> yet. In a fast debug build this will be filled with 0xbaadbabe and while
> dereferencing this memory it fails with a WrongTypeException. Sometimes it
> will also fail with an UnmappedAddressException, but these exceptions are
> ignored in this part of SA (for some reason).
> 
> The TLAB that SA misses is one in a compiler thread. The code in SA does:
> 
> if (VM.getVM().getUseTLAB()) {
>   for (JavaThread thread = VM.getVM().getThreads().first(); thread !=
> null; thread = thread.next()) { if (thread.isJavaThread()) {
>   ThreadLocalAllocBuffer tlab = thread.tlab();
>   
> 
> The problem is that thread.isJavaThread() will return false for
> CompilerThread (and some others) although they can have TLABs that we need
> to look at. The solution is to remove that check.
> 
> I’ve left some debugging code in place in the code that I think can be
> useful for other problems. The real fix is just two lines of code.
> 
> webrev: http://cr.openjdk.java.net/~sla/8029395/webrev.00/
> bug: https://bugs.openjdk.java.net/browse/JDK-8029395

The fix looks good to me, good catch finding that isJavaThread() in the SA is 
lying.

I had a quick look through the other uses of isJavaThread() and some of them 
seem to require that the thread has a Java SP. Not enough to warrant not using 
a "isRealJavaThread()" or whatever to signal that it's in fact lying about 
being a Java thread, but I guess there's a lot of ugly code in the SA :(

/Mikael


> 
> Thanks,
> /Staffan

Re: Review quest for JDK-7067973: test/java/lang/management/MemoryMXBean/CollectionUsageThreshold.java hanging intermittently

[Mikael Gerdin]

On Thursday 28 November 2013 13.33.48 Staffan Larsen wrote:
> On 28 nov 2013, at 11:16, Leonid Mesnik  wrote:
> > Eric, Mandy
> > 
> > Sorry that I looping on very late step. It is not a review just
> > suggestion.
> > We have whitebox API in Hotspot which includes fullGC() method. It could
> > be used to reliably provoke full GC. See example in
> > http://hg.openjdk.java.net/jdk8/tl/hotspot/file/c9f439732b18/test/runtime
> > /interned/SanityTest.java
> > 
> > Should such approach works for you?
> 
> It’s not necessary. System.gc is implemented as a full GC for all of our GC
> implementations. I don’t think there is a problem in relying on that fact.

Förutom om SQE injicerar commandline-flaggor i tester, typ -XX:
+ExplicitGCInvokesConcurrent, eller rentav -XX:+DisableExpicitGC...

tycker inte att ni ska ändra till WBapi-fullgc, men det är nog därifrån Leonid 
kommer.

/m

> > Also please note that your new variant of test fails if any of GC is set
> > explicitly. It is incompatible with GC setting. We set GC's and
> > GC-related options during Promotion/Nightly/PIT in Hotspot/SVC. For us is
> > better if test just works with any GC set externally.
> 
> This is broken (as has been discussed many times). Tests *need* to be able
> to provide their own flags without someone overriding them and still
> expecting the test to work.
> 
> /Staffan
> 
> > Do you need to run it with all GC each time?
> > 
> > Leonid
> > 
> > On 11/28/2013 09:21 AM, Eric Wang wrote:
> >> Hi Mandy,
> >> 
> >> Yes, I have tested and all settings are passed, as you mentioned the test
> >> hangs with -XX:+UseG1GC -XX:+ExplicitGCInvokesConcurrent and default
> >> heap size as no GC happens on Old Gen. That is why to add -Xmx2m and big
> >> object to make sure GC happens.
> >> 
> >> I didn't realized the -Xconcgc is same as -XX:+UseConcMarkSweepGC, i have
> >> updated the webrev:
> >> http://cr.openjdk.java.net/~ewang/JDK-7067973/webrev.00/
> >> 
> >> Thanks,
> >> Eric
> >> 
> >> On 2013/11/27 10:17, Mandy Chung wrote:
> >>> Hi Eric,
> >>> 
> >>> I'll defer this to the serviceability team to sponsor it and also get
> >>> one more review.
> >>> 
> >>> I don't think you need all 7 @runs.  -Xconcgc is equivalent to setting
> >>> -XX:+UseConcMarkSweepGC.  For G1 and CMS, you should use
> >>> -XX:+ExplicitGCInvokesConcurrent so that System.gc will force a GC in
> >>> foreground that you can count the GC reliably. The test wants to get
> >>> notified for each System.gc and if there is any GC caused by allocation
> >>> failure, the test would fail due to the unexpected GC count.  It seems
> >>> that you may run into this issue setting -Xmx2m.
> >>> 
> >>> Have you got the test passed in all settings?   I'm seeing that the test
> >>> hangs with -XX:+UseG1GC -XX:+ExplicitGCInvokesConcurrent without
> >>> -Xmx2m.  Looks like there is no GC in the old gen - I'm not familiar
> >>> with G1 if it allocates the big object in the old gen.  Jarolsav - can
> >>> you help Eric diagnose this issue?  I recalled you ran into something
> >>> like that before - maybe Staffan?
> >>> 
> >>> thanks
> >>> Mandy
> >>> 
> >>> On 11/25/2013 8:53 PM, Eric Wang wrote:
>  Hi Mandy,
>  
>  1. for L34-40, executing tests with 7 settings is trying to cover more
>  cases (normal cases and special cases), especially last 3 settings, as
>  found that the test hung if using vm option
>  "-XX:+ExplicitGCInvokesConcurrent" with one of 3 options -XX:+UseG1GC,
>  -XX:+UseConcMarkSweepGC or -Xconcgc
>  
>  2. for L61, that is right, the test has been updated. please review.
>  http://cr.openjdk.java.net/~ewang/JDK-7067973/webrev.00/
>  
>  Thanks,
>  Eric
>  
>  On 2013/11/26 8:37, Mandy Chung wrote:
> > Hi Eric,
> > 
> > On 11/24/2013 7:41 PM, Eric Wang wrote:
> >> Hi Mandy & All,
> >> 
> >> Sorry for late!
> >> The webrev below is just finished based on the comments from peers,
> >> please help to review.
> >> http://cr.openjdk.java.net/~ewang/JDK-7067973/webrev.00/
> > 
> > Thanks for the patch that looks okay.  Some comments:
> > 
> > L34-40: can you explain why you want to run all 7 settings?  I would
> > expect one for each collector. L61: I think the static checker
> > variable is meant to be a local variable (and looks like "pools" and
> > "managers" don't need to be static variable).
> > 
> > Mandy
> > 
> >> Thanks,
> >> Eric
> >> 
> >> On 2013/11/15 10:55, Mandy Chung wrote:
> >>> Hi Eric,
> >>> 
> >>> On 11/14/2013 6:16 PM, Eric Wang wrote:
>  Hi Everyone,
>  
>  I'm working on the bug
>  https://bugs.openjdk.java.net/browse/JDK-7067973.
>  
>  It is a test bug as the test doesn't guarantee memory allocated
>  from the Old Gen, if the used memory is zero and doesn't cross the
>  threshold, no notification is sent, so both the main thread and
> >>

Re: jstack and the target output

[Mikael Gerdin]

On Friday 22 November 2013 12.59.42 Piotr Bzdyl wrote:
> After checking the strace output I notice that the issue was caused by the
> file mode for files created on vboxsf mount. By default (when automount
> option is used in virtualbox configuration) it creates .attach_pid
> with file mode 0770. When I manually mounted shared folder specifying
> fmode=660 jstack worked as expected.
> 
> Thank you for pointing me to the right path to narrow down the issue.
> 
> BTW, where I could find the JVM code responsible for handling SIGQUIT
> signal to see what it does that file mode matters?

http://hg.openjdk.java.net/hsx/hotspot-gc/hotspot/file/tip/src/os/linux/vm/attachListener_linux.cpp

and

http://hg.openjdk.java.net/hsx/hotspot-gc/hotspot/file/tip/src/share/vm/runtime/os.cpp
(see signal_thread_entry and its call to AttachListener::is_init_trigger()

/Mikael


> 
> On Fri, Nov 22, 2013 at 10:30 AM, Piotr Bzdyl  wrote:
> > Thank you Mikael for the detailed information - I learned a lot about how
> > SA tools attache to the target process.
> > 
> > When I was running the test you suggested I noticed one "detail" in my
> > setup which turned out to be the cause of my problems: as I mentioned
> > previously I was running my tests inside a virtual machine (namely
> > VirtualBox). The issue was that the cwd dir of my target process was on
> > the
> > mounted shared directory (vboxsf filesystem) in VirtualBox VM and it was
> > causing the problems. I still have to go through the strace outputs to
> > find
> > out if it is because of the access rights of because how vboxsf file
> > system
> > driver works (attached the output).
> > 
> > On Fri, Nov 22, 2013 at 9:51 AM, Mikael Gerdin 
wrote:
> >> Piotr,
> >> 
> >> On Friday 22 November 2013 09.21.56 Piotr Bzdyl wrote:
> >> > Is there any other information I could collect to help troubleshoot
> >> > this
> >> > problem? How could I check if VM runs with attach disabled?
> >> 
> >> The comments in
> >> 
> >> http://hg.openjdk.java.net/jdk8/jdk8/jdk/file/tip/src/solaris/classes/sun
> >> /tools/attach/LinuxVirtualMachine.java describe the handshaking involved
> >> in dynamically enabling the attach mechanism
> >> on Linux.
> >> 
> >> The mechanism is basically:
> >> * The attacher checks for the existence of the socket file
> >> /tmp/.java_pid${PID}
> >> * If that does not exist it creates a file .attach_pid${PID} in the
> >> target
> >> process' working directory and sends a SIGQUIT (kill -QUIT) to that
> >> process.
> >> * The SIGQUIT handler notices the .attach_pid${PID} file and creates the
> >> socket at /tmp/.java_pid${PID}
> >> * The attacher notices the socket file and unlinks the .attach_pid${PID}
> >> file.
> >> 
> >> You can force the target process to always create the socket file at
> >> startup
> >> by using the -XX:+StartAttachListener command line flag.
> >> 
> >> To further troubleshoot the problem I suggest that you try strace:ing the
> >> target process and jstack with the following flags enabled:
> >> 
> >> (target process)
> >> $ strace -f -e trace=accept,stat,socket,bind $JAVA_HOME/bin/java
> >> donothing
> >> 
> >> (jstack process)
> >> $ strace -f -e trace=open,connect,stat,unlink,write,kill
> >> $JAVA_HOME/bin/jstack
> >> 
> >> 
> >> You should get something similar to the following:
> >> (target process)
> >> (...)
> >> [pid 12602] --- SIGQUIT {si_signo=SIGQUIT, si_code=SI_USER, si_pid=12687,
> >> si_uid=726} ---
> >> [pid 12630] stat(".attach_pid12602", {st_mode=S_IFREG|0644, st_size=0,
> >> ...}) =
> >> 0
> >> Process 12733 attached
> >> [pid 12733] socket(PF_LOCAL, SOCK_STREAM, 0) = 5
> >> [pid 12733] bind(5, {sa_family=AF_LOCAL,
> >> sun_path="/tmp/.java_pid12602.tmp"},
> >> 110) = 0
> >> [pid 12733] accept(5, {sa_family=AF_LOCAL, NULL}, [2]) = 6
> >> 
> >> (jstack process)
> >> [pid 12688] stat("/localhome/java/jdk-8-ea-bin-
> >> b112/jre/lib/amd64/libattach.so", {st_mode=S_IFREG|0755, st_size=16029,
> >> ...})
> >> = 0
> >> [pid 12688] open("/localhome/java/jdk-8-ea-bin-
> >> b112/jre/lib/amd64/libattach.so", O_RDONLY) = 7
> >> [pid 12688] open("/localhome/java/jdk-8-ea-bin-
> >> b112/jre/lib/amd64/libattach.so", O_RDONLY|O_CLOEXEC) = 7
> >

Re: jstack and the target output

[Mikael Gerdin]

Piotr,

On Friday 22 November 2013 09.21.56 Piotr Bzdyl wrote:
> Is there any other information I could collect to help troubleshoot this
> problem? How could I check if VM runs with attach disabled?

The comments in 
http://hg.openjdk.java.net/jdk8/jdk8/jdk/file/tip/src/solaris/classes/sun/tools/attach/LinuxVirtualMachine.java
describe the handshaking involved in dynamically enabling the attach mechanism 
on Linux.

The mechanism is basically:
* The attacher checks for the existence of the socket file 
/tmp/.java_pid${PID}
* If that does not exist it creates a file .attach_pid${PID} in the target 
process' working directory and sends a SIGQUIT (kill -QUIT) to that process.
* The SIGQUIT handler notices the .attach_pid${PID} file and creates the 
socket at /tmp/.java_pid${PID}
* The attacher notices the socket file and unlinks the .attach_pid${PID} file.

You can force the target process to always create the socket file at startup 
by using the -XX:+StartAttachListener command line flag.

To further troubleshoot the problem I suggest that you try strace:ing the 
target process and jstack with the following flags enabled:

(target process) 
$ strace -f -e trace=accept,stat,socket,bind $JAVA_HOME/bin/java donothing

(jstack process)
$ strace -f -e trace=open,connect,stat,unlink,write,kill $JAVA_HOME/bin/jstack 


You should get something similar to the following:
(target process)
(...)
[pid 12602] --- SIGQUIT {si_signo=SIGQUIT, si_code=SI_USER, si_pid=12687, 
si_uid=726} ---
[pid 12630] stat(".attach_pid12602", {st_mode=S_IFREG|0644, st_size=0, ...}) = 
0
Process 12733 attached
[pid 12733] socket(PF_LOCAL, SOCK_STREAM, 0) = 5
[pid 12733] bind(5, {sa_family=AF_LOCAL, sun_path="/tmp/.java_pid12602.tmp"}, 
110) = 0
[pid 12733] accept(5, {sa_family=AF_LOCAL, NULL}, [2]) = 6

(jstack process)
[pid 12688] stat("/localhome/java/jdk-8-ea-bin-
b112/jre/lib/amd64/libattach.so", {st_mode=S_IFREG|0755, st_size=16029, ...}) 
= 0
[pid 12688] open("/localhome/java/jdk-8-ea-bin-
b112/jre/lib/amd64/libattach.so", O_RDONLY) = 7
[pid 12688] open("/localhome/java/jdk-8-ea-bin-
b112/jre/lib/amd64/libattach.so", O_RDONLY|O_CLOEXEC) = 7
[pid 12688] stat("/tmp/.java_pid12602", 0x7fe53183e3a0) = -1 ENOENT (No such 
file or directory)
[pid 12688] open("/proc/12602/cwd/.attach_pid12602", O_RDWR|O_CREAT|O_EXCL, 
0666) = 7
[pid 12688] kill(12602, SIGQUIT)= 0
[pid 12688] stat("/tmp/.java_pid12602", {st_mode=S_IFSOCK|0600, st_size=0, 
...}) = 0
[pid 12688] unlink("/proc/12602/cwd/.attach_pid12602") = 0
[pid 12688] stat("/tmp/.java_pid12602", {st_mode=S_IFSOCK|0600, st_size=0, 
...}) = 0
[pid 12688] connect(7, {sa_family=AF_LOCAL, sun_path="/tmp/.java_pid12602"}, 
110) = 0
[pid 12688] connect(7, {sa_family=AF_LOCAL, sun_path="/tmp/.java_pid12602"}, 
110) = 0
[pid 12688] write(7, "1", 1)= 1
[pid 12688] write(7, "\0", 1)   = 1
[pid 12688] write(7, "threaddump", 10)  = 10
[pid 12688] write(7, "\0", 1)   = 1

/Mikael

> 
> I noticed the same behavior when trying to connect to the process using
> JConsole: JConsole displays the sample app in the list and when I try to
> connect it finally displays an error that it cannot connect whereas the
> sample app prints thread dump on its console.
> 
> On Thu, Nov 21, 2013 at 7:18 PM, Alan Bateman 
wrote:
> > On 21/11/2013 14:47, David Holmes wrote:
> >> I can recreate this using the minimal VM, which doesn't support
> >> libattach. So it definitely seems like something is going wrong in the
> >> attach process, but I can't say what. Not sure what debugging hooks we
> >> have
> >> for the attach mechanism ??
> > 
> > The first attach involves signaling the target VM to start up the attach
> > mechanism. So for the minimal VM or a VM running with attach disabled (or
> > as Staffan pointed out, running with a different tmp directory) then the
> > target VM gets a SIGQUIT and just prints the thread dump. It's hard to say
> > what Piotr is running into but I think this at least explains it for the
> > minimal VM.
> > 
> > -Alan

hg: hsx/hotspot-rt/hotspot: 2 new changesets

[mikael . gerdin]

Changeset: 9d8b29a0548c
Author:mgerdin
Date:  2013-11-08 16:48 +0100
URL:   http://hg.openjdk.java.net/hsx/hotspot-rt/hotspot/rev/9d8b29a0548c

8027237: New tests on ReservedSpace/VirtualSpace classes
Summary: Three tests added: 1) test stressing VirtualSpace by resizing it 
constantly 2) test running unit tests in several threads 3) test checking 
protected area in ReservedHeapSpace class
Reviewed-by: stefank, zgu
Contributed-by: aleksey.timof...@oracle.com

! src/share/vm/prims/whitebox.cpp
+ test/runtime/memory/ReadFromNoaccessArea.java
+ test/runtime/memory/RunUnitTestsConcurrently.java
+ test/runtime/memory/StressVirtualSpaceResize.java
! test/testlibrary/whitebox/sun/hotspot/WhiteBox.java

Changeset: 19f8a5d7600b
Author:mgerdin
Date:  2013-11-08 23:49 +
URL:   http://hg.openjdk.java.net/hsx/hotspot-rt/hotspot/rev/19f8a5d7600b

Merge

hg: hsx/hotspot-rt/hotspot: 8027252: Crash in interpreter because get_unsigned_2_byte_index_at_bcp reads 4 bytes

[mikael . gerdin]

Changeset: ea79ab313e98
Author:mgerdin
Date:  2013-10-30 15:35 +0100
URL:   http://hg.openjdk.java.net/hsx/hotspot-rt/hotspot/rev/ea79ab313e98

8027252: Crash in interpreter because get_unsigned_2_byte_index_at_bcp reads 4 
bytes
Summary: Use 2-byte loads to load indexes from the byte code stream to avoid 
out of bounds reads.
Reviewed-by: coleenp, sspitsyn

! src/cpu/x86/vm/interp_masm_x86_32.cpp
! src/cpu/x86/vm/interp_masm_x86_64.cpp
! src/cpu/x86/vm/templateTable_x86_32.cpp
! src/cpu/x86/vm/templateTable_x86_64.cpp

Re: RFR: 8025834: NPE in Parallel Scavenge with -XX:+CheckUnhandledOops (round 2)

[Mikael Gerdin]

Erik,

On Monday 21 October 2013 11.26.51 Erik Helin wrote:
> Hi all,
> 
> this is the second version of a patch for JDK-8025834.
> 
> Background:
> In JvmtiEnv::SetBreakpoint and JvmtiEnv::ClearBreakpoint, we create a
> JvmtiBreakpoint on the stack. This JvmtiBreakpoint contains an
> "unhandled" oop to the class loader of the Method's class.
> 
> A pointer to the JvmtiBreakpoint allocated on the stack will be passed
> to VM_ChangeBreakpoint via JvmtiBreakpoints::set/clear. The
> VM_ChangeBreakpoint class has an oops_do method that will visit the oop
> in the JvmtiBreakpoint that was allocated on the stack.
> 
> Since no GC can occur before the JvmtiBreakpoint has been passed to the
> VM_ChangeBreakpoint (which is then passed to VMThread::execute), there
> is no need to have a Handle for the _class_loader oop. Once the
> VM_ChangeBreakpoint is on the VMThread's list for being executed, the
> oop will be found via VMThread::oops_do.
> 
> The oop can't be changed to a Handle because JvmtiBreakpoint:s are also
> allocated on the heap, and Handles are only used for oops allocated on
> the stack.
> 
> This patch therefore registers the oop as "unhandled" to tell the
> unhandled oop verifier to (rightfully) ignore it.
> 
> Webrev:
> http://cr.openjdk.java.net/~ehelin/8025834/round-2/webrev.00/

The change looks good to me. 

/Mikael

> 
> Bug:
> https://bugs.openjdk.java.net/browse/JDK-8025834
> 
> As I explained in a previous email, there are additional issues in the
> JvmtiEnv::SetBreakpoint and JvmtiEnv::ClearBreapoints methods:
> - https://bugs.openjdk.java.net/browse/JDK-8026946
> - https://bugs.openjdk.java.net/browse/JDK-8026948
> This change does not intend to fix these issues.
> 
> Thanks,
> Erik

Re: RFR: 8025834: NPE in Parallel Scavenge with -XX:+CheckUnhandledOops

[Mikael Gerdin]

Erik,

(it's not necessary to cross-post between hotspot-dev and 
hotspot-gc-dev, so I removed hotspot-gc from the CC list)


On 2013-10-16 18:09, Erik Helin wrote:

Hi all,

this patch fixes an issue where an oop in JvmtiBreakpoint,
JvmtiBreakpoint::_class_loader, was found by the unhandled oop detector.

Instead of registering the oop as an unhandled oop, which would have
worked, I decided to wrap the oop in a handle as long as it is on the
stack.

A JvmtiBreakpoint is created on the stack by the two methods
JvmtiEnv::SetBreakpoint and JvmtiEnv::ClearBreakpoint. This
JvmtiBreakpoint is only created to carry the Method*, jlocation and oop
to a VM operation, VM_ChangeBreakpoints. VM_ChangeBreakpoints will, when
at a safepoint, allocate a new JvmtiBreakpoint on the native heap, copy
the values from the stack allocated JvmtiBreakpoint and then place/clear the
newly alloacted JvmtiBreakpoint in
JvmtiCurrentBreakpoints::_jvmti_breakpoints.

I have updated to the code to check that the public constructor is only
used to allocate JvmtiBreakpoints on the stack (to warn a future
programmer if he/she decides to allocate one on the heap). The
class_loader oop is now wrapped in a Handle for stack allocated
JvmtiBreakpoints.

Due to the stack allocated JvmtiBreakpoint having the oop in a handle,
the oops_do method of VM_ChangeBreakpoints can be removed. This also
makes the oop in the handle safe for use after the VM_ChangeBreakpoint
operation is finished.

The unhandled oop in the JvmtiBreakpoint allocated on the heap will be
visited by the GC via jvmtiExport::oops_do ->
JvmtiCurrentBreakpoints::oops_do -> JvmtiBreakpoints::oops_do ->
GrowableCache::oops_do -> JvmtiBreakpoint::oops_do, since it is being
added to.

I've also removed some dead code to simplify the change:
- GrowableCache::insert
- JvmtiBreakpoint::copy
- JvmtiBreakpoint::lessThan
- GrowableElement::lessThan

Finally, I also formatted JvmtiEnv::ClearBreakpoint and
Jvmti::SetBreakpoint exactly the same to highlight that they share all
code except one line. Unfortunately, I was not able to remove this code
duplication in a good way.

Webrev:
http://cr.openjdk.java.net/~ehelin/8025834/webrev.00/


jvmtiImpl.hpp:
Since clone() uses unhandled oops, and is only supposed to be used by 
the VM operation, would it make sense to 
assert(SafepointSynchronize::is_at_safepoint())?


196   GrowableElement *clone(){
 197 return new JvmtiBreakpoint(_method, _bci, _class_loader_handle);

jvmtiImpl.cpp:
No comments.

jvmtiEnv.cpp:
Have you verified that the generated JVMTI entry point contains a 
ResourceMark or is it just not needed?

-  ResourceMark rm;
+  HandleMark hm;

Otherwise the code change looks good.


One thing that you didn't describe here, but which was related to the 
bug (which we discussed) was the fact that the old code tried to "do the 
right thing" WRT CheckUnhandledOops, but it incorrectly added a Method*:


thread->allow_unhandled_oop((oop*)&_method);

We should take care to find other such places where we try to put a 
non-oop in allow_unhandled_oop(), perhaps checking is_oop_or_null in the 
unhandled oops code.


/Mikael



Testing:
- JPRT
- The four tests that were failing are now passing

Thanks,
Erik

Re: RFR (S): 8025925: jmap fails with "field _length not found in type HeapRegionSeq"

[Mikael Gerdin]

Thomas,

On 15 October 2013 16:08:52 Thomas Schatzl  wrote:

Hi,

On Tue, 2013-10-15 at 15:04 +0200, Mikael Gerdin wrote:
> Thomas,
> On Tuesday 15 October 2013 12.53.25 Thomas Schatzl wrote:
> > Hi all,
> > >   can I have reviews for the following change? It updates the SA agent
> > that got out of date after the changes for JDK-7163191 where the
> > HeapRegionSeq class has been refactored.
> > > The changes mirror the changes in the C++ code of that revision, adding
> > a G1HeapRegionTable java class, and the associated modifications in the
> > HeapRegionSeq class.
> > > There have been no interface changes to the HeapRegionSeq class to avoid
> > breakage of any tools depending on it.
> > > Webrev:
> > http://cr.openjdk.java.net/~tschatzl/8025925/webrev/
> Why did you change the copyright header format?
> "2011, 2013" is the format we should use, where 2011 is the first year 
and 2013 is the year when it was last modified.


Okay, sorry. Fixed. Not sure what I thought when changing that.

> Otherwise I think the changes look good. Do you know if any part of the 
SA code base actually uses this class?


Which one? HeapRegionSeq or the new G1HeapRegionTable class?
HeapRegionSeq is used by the G1CollectedHeap class for the n_regions()
and heap iteration during heap dump and liveness analysis it seems. In
the change, the G1HeapRegionTable replaced the _regions field of
HeapRegionSeq. I tried to follow what I thought was the style of the
other code, i.e. try to stay close to the C++ object hierarchy.

The new HeapRegionSeq mostly forwards requests to it to the new
G1HeapRegionTable class. I could hide the latter (i.e. make it package
private) if wanted and even move it into the HeapRegionSeq.java file
(and remove the new file).


Ok, in that case I'm fine with the code change as-is.
I don't need to see the copyright year fix.

Ship it,
/Mikael



I do not mind either way.

Thanks,
Thomas

Re: RFR (S): 8025925: jmap fails with "field _length not found in type HeapRegionSeq"

[Mikael Gerdin]

Thomas,

On Tuesday 15 October 2013 12.53.25 Thomas Schatzl wrote:
> Hi all,
> 
>   can I have reviews for the following change? It updates the SA agent
> that got out of date after the changes for JDK-7163191 where the
> HeapRegionSeq class has been refactored.
> 
> The changes mirror the changes in the C++ code of that revision, adding
> a G1HeapRegionTable java class, and the associated modifications in the
> HeapRegionSeq class.
> 
> There have been no interface changes to the HeapRegionSeq class to avoid
> breakage of any tools depending on it.
> 
> Webrev:
> http://cr.openjdk.java.net/~tschatzl/8025925/webrev/

Why did you change the copyright header format?
"2011, 2013" is the format we should use, where 2011 is the first year and 
2013 is the year when it was last modified.

Otherwise I think the changes look good. Do you know if any part of the SA 
code base actually uses this class?

/Mikael

> 
> Testing:
> Connecting an SA-agent with the change on a running java program; test
> case in bug report
> 
> (I added the serviceability-dev mailing list as additional recipients as
> the change also concerns this part of the VM)
> 
> Thanks,
>   Thomas

Re: RFR (S): 8016845: SA is unable to use hsdis on windows

[Mikael Gerdin]

Fredrik,

On 09/20/2013 04:29 PM, Fredrik Arvidsson wrote:

Please help me review this:

Bug: https://bugs.openjdk.java.net/browse/JDK-8016845
Webrev: http://cr.openjdk.java.net/~allwin/farvidss/8016845/webrev.00/



the change looks good to me.



Small change was made in the sa.make file for windows to compile and
link sadis.c in to sawindbg.dll providing JNI entrypoints needed to call
the hsdis disassembler from SADB on windows. A small change in
agent/src/share/classes/sun/jvm/hotspot/asm/Disassembler.java to have
the correct library name when loading library depending if running on 32
or 64 bit platform.


Thanks for fixing this!

/Mikael



To test this you have to build the hsdis library yourself, since it cant
be bundled due to license issues. Instructions can be found here:
http://dropzone.nfshost.com/hsdis.htm (hint, I have pre-built binaries).
When running disassembling in HSDB (using the 'dis' command in the
console) the hsdis-amd64.dll/hsdis-i386.dll must be in the /bin
directory of the JRE/JDK used.

Cheers
/Fredrik

Re: RFR (S): JDK-8024423: JVMTI: GetLoadedClasses doesn't enumerate anonymous classes

[Mikael Gerdin]

Fredrik,

On 09/30/2013 01:45 PM, Fredrik Arvidsson wrote:

Hi

Please help me to review the changes below:

Jira case: https://bugs.openjdk.java.net/browse/JDK-8024423
Webrev: http://cr.openjdk.java.net/~allwin/farvidss/8024423/webrev.00/


In this review I still have an outstanding unanswered question about the
need to synchronize the access to the:

ClassLoaderDataGraph::classes_do(&JvmtiGetLoadedClassesClosure::increment);
and
ClassLoaderDataGraph::classes_do(&JvmtiGetLoadedClassesClosure::add);


It seems to me that the synchronization is only needed to make sure that 
we get a correct count for the _list array. New Klass*'s are pushed on 
the top of the _klasses list so that should not interfere with walking 
the Klass links.


One approach would be to do only one walk over the ClassLoaderDataGraph 
and use a growing datastructure instead of allocating one big linear 
array. I don't see any obvious requirement for a random-access 
datastructure so you could probably just put a Stack there and 
push the mirrors on the stack, get the stack size and put them on 
result_list in the correct order.


You can also make JvmtiGetLoadedClassesClosure a KlassClosure and get 
rid of the thread-local get_this/set_this since ClassLoaderDataGraph 
provides an interface for a KlassClosure as well as a callback function 
pointer.


/Mikael



calls in
http://cr.openjdk.java.net/~allwin/farvidss/8024423/webrev.00/src/share/vm/prims/jvmtiGetLoadedClasses.cpp.udiff.html
  


Please give me advise on this.

There will be a review soon regarding re-enabling the tests that failed because 
of the above bug, see:
https://bugs.openjdk.java.net/browse/JDK-8025576

Cheers
/Fredrik

hg: hsx/hotspot-rt/hotspot: 8023956: Provide a work-around to broken Linux 32 bit "Exec Shield" using CS for NX emulation (crashing with SI_KERNEL)

[mikael . gerdin]

Changeset: 899ecf76b570
Author:dsimms
Date:  2013-09-25 13:58 +0200
URL:   http://hg.openjdk.java.net/hsx/hotspot-rt/hotspot/rev/899ecf76b570

8023956: Provide a work-around to broken Linux 32 bit "Exec Shield" using CS 
for NX emulation (crashing with SI_KERNEL)
Summary: Execute some code at a high virtual address value, and keep mapped
Reviewed-by: coleenp, zgu

! src/os/linux/vm/os_linux.cpp
! src/os_cpu/linux_x86/vm/os_linux_x86.cpp
! src/os_cpu/linux_x86/vm/os_linux_x86.hpp

hg: hsx/hotspot-rt/hotspot: 2 new changesets

[mikael . gerdin]

Changeset: 1b03bed31241
Author:allwin
Date:  2013-09-17 17:16 +0200
URL:   http://hg.openjdk.java.net/hsx/hotspot-rt/hotspot/rev/1b03bed31241

7196151: ParserTest SEGv on solaris
Reviewed-by: sla, coleenp, ctornqvi, dsamersoff

! src/share/vm/services/diagnosticArgument.cpp

Changeset: e5a25e4ae509
Author:mgerdin
Date:  2013-09-20 10:34 -0700
URL:   http://hg.openjdk.java.net/hsx/hotspot-rt/hotspot/rev/e5a25e4ae509

Merge

Re: jstack can't attach to JVM (OS X)

[Mikael Gerdin]

On 2013-08-27 10:26, Staffan Larsen wrote:


On 26 aug 2013, at 20:19, Martin Traverso mailto:mtrave...@gmail.com>> wrote:


Great! Thanks for the explanation.

Just curious, why does running jstack at least once before the code
reaches that method call prevent the problem from happening at all?


No idea. :(  Can't figure it out.


I think SIGQUIT is only used to initiate the attach mechanism. If the 
JVM can catch the SIGQUIT before the verification error has occurred 
then the attach mechanism will have been initialized and subsequent 
jstack/jcmd commands can be processed.


/Mikael



/Staffan



Martin


On Mon, Aug 26, 2013 at 5:31 AM, Staffan Larsen
mailto:staffan.lar...@oracle.com>> wrote:

Here is what's happening:

- The VM sets the signal mask for all threads (except the internal
VMThread) to mask out SIGQUIT using pthread_sigmask(). So the
process will still respond to SIGQUIT, but only one thread.
- The verifier code calls setjmp() to save the calling context. On
OS X this also saves the signal mask.
- The example code causes a verification error somewhere which
causes the verifier to call longjmp().
- longjmp will restore the signal mask using sigprocmask() which
sets the signal mask for the _process_.
- Now the process has a signal mask that masks out SIGQUIT and
attach stops working.

This only happens on OS X because setjmp/longjmp does not save and
restore the signal mask on other platforms. There are functions
_setjmp/_longjmp on OS X to skip the signal mask save/restore and
I think this is what we need to use in the verifier (also need to
check other uses of setjmp/longjmp in the JDK).

I have filed bug 8023720 for this.

Thanks again for the report and reproducer.

/Staffan




On 23 aug 2013, at 14:44, Staffan Larsen
mailto:staffan.lar...@oracle.com>> wrote:


Thanks for the excellent bugreport!

I've had a quick look today but haven't yet figured out what the
problem is. What happens is that somehow the process becomes
immune to the SIGQUIT that jstack sends to it when it wants to
attach. Instead of running jstack, one can also do "kill -QUIT
" and with this code no thread stacks are printed (as they
normally will be).

I'll continue looking,
/Staffan

On 23 aug 2013, at 07:20, Martin Traverso mailto:mtrave...@gmail.com>> wrote:


I have a program that causes jstack and jcmd to fail to attach
to the JVM on OS X. Unfortunately, it uses a third-party
library, so I'm not sure what's the magic incantation that
causes this to happen.

I wrote a small test case using this library that reproduces the
problem. You can find the code here:
https://github.com/martint/jstackissue.

I've seen the problem on both 1.7.0_25-b15 and 1.8.0-ea-b100 on
OS X, but not on Java 6. It doesn't seem to happen on Linux, either.

One interesting data point is that if I run jstack before the
code makes the call to the library, it works, and continues to
work even after that call is complete. On the other hand, if run
jstack for the first time *after* the call to the library, it
won't work at all.

Any ideas?

Thanks!
Martin

hg: hsx/hotspot-rt/hotspot: 8022683: JNI GetStringUTFChars should return NULL on allocation failure not abort the VM

[mikael . gerdin]

Changeset: faf2631b9334
Author:dsimms
Date:  2013-08-26 09:33 +0200
URL:   http://hg.openjdk.java.net/hsx/hotspot-rt/hotspot/rev/faf2631b9334

8022683: JNI GetStringUTFChars should return NULL on allocation failure not 
abort the VM
Summary: Return NULL on OOM from GetStringChars, GetStringUTFChars and 
GetArrayElements family of functions.
Reviewed-by: dholmes, coleenp

! src/share/vm/memory/allocation.hpp
! src/share/vm/prims/jni.cpp

Re: RR(S): 8020943: Memory leak when GCNotifier uses create_from_platform_dependent_str()

[Mikael Gerdin]

Kevin,

On 07/31/2013 11:45 AM, Kevin Walls wrote:

Hi,

I'd like to get a review on this small change:

http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8020943
https://jbs.oracle.com/bugs/browse/JDK-8020943

webrev
http://cr.openjdk.java.net/~kevinw/8020943/webrev.00/


Looks good to me.

/Mikael



It turns out there's a leak in the gc notifier: reproduce by attaching
e.g. JConsole and watching, if there is frequent GC the number of
apparently unowned String objects that can't get collect continually
increases.

In the notifier, the method it calls to create String objects involves a
JNI call that leaves a Handle behind and doesn't get cleared.   There is
a simpler method to call, there is no need for all that work, as we are
dealing with a small set of simple strings in the JVM being converted,
to describe the collection type, cause, ...

Thanks
Kevin

Re: RR(XS) 8019396: SA-JDI: OSThread class initialization throws an exception

[Mikael Gerdin]

David,

On 2013-07-23 12:31, David Holmes wrote:

Hi Dmitry,

Looks okay.

Minor nit:

! return (int)threadIdField.getJInt(addr);

The cast should not be need as getJInt returns int.

Aside: this looks like a major bug in BasicField to me:

  public long getJLong(Address addr) ...

A jlong is 64-bits but a long may be 32-bits!


But isn't that a Java land "long"? That's the definition of jlong :)

/Mikael



David

On 23/07/2013 7:39 PM, Dmitry Samersoff wrote:

Hi Everybody,

Please, review the fix.

http://cr.openjdk.java.net/~dsamersoff/JDK-8019396.SA-JDI/webrev.01/


Method sun.jvm.hotspot.runtime.OSThread.initialize throws a
sun.jvm.hotspot.types.WrongTypeException with message: field
"_thread_id" in type OSThread is not of type jint, but instead of type
unsigned OSThread::thread_id_t.

After fixing an exception test still fails, because of wrong value used
for JVMTI_THREAD_STATE_WAITING, fixed it as well.

Testing:

nsk/sajdi/ThreadReference/status/status002

-Dmitry

hg: hsx/hotspot-rt/hotspot: 6671508: JNI GetPrimitiveArrayCritical should not be callable on object arrays

[mikael . gerdin]

Changeset: 39deebbc90b3
Author:mgerdin
Date:  2013-07-16 07:33 +0200
URL:   http://hg.openjdk.java.net/hsx/hotspot-rt/hotspot/rev/39deebbc90b3

6671508: JNI GetPrimitiveArrayCritical should not be callable on object arrays
Summary: Checked JNI now reports error for Get/ReleasePrimitiveArrayCritical on 
object arrays
Reviewed-by: dholmes, acorn
Contributed-by: david.si...@oracle.com

! src/share/vm/prims/jniCheck.cpp

Re: RFR 7162400: Intermittent java.io.IOException: Bad file number during HotSpotVirtualMachine.executeCommand

[Mikael Gerdin]

On 07/09/2013 05:25 PM, Peter Allwin wrote:

Mikael,

That's a good point, unfortunately attach uses os::get_temp_directory which
is hardcoded to use /tmp. We could add a whitebox API to allow us to
override this but now we're on the border to noreg-hard land again IMO.

Any other opinions on this?


Can you use the "-XX:+PauseAtStartup" vm flag it will create a 
vm.paused. file in the current work directory. You could extract 
the pid, touch the correct attach file in /tmp and then remove the 
vm.paused to let the VM resume.


I didn't check if PauseAtStartup stops the VM early enough though.

An alternate, even more hacky approach is to do something like (in bash):
(bash -c 'echo $$; touch /tmp/.java_pid$$; exec java -version')
Where you can extract the pid of the subshell process with $$ and then 
exec into the java launcher and keep the same pid (at least on Linux, 
unsure about the Solaris launcher).


/Mikael




Thanks!

/peter


-Original Message-
From: Mikael Gerdin [mailto:mikael.ger...@oracle.com]
Sent: Tuesday, July 9, 2013 2:49 PM
To: Peter Allwin
Cc: serguei.spit...@oracle.com; daniel.daughe...@oracle.com;
serviceability-dev@openjdk.java.net; hotspot-runtime-
d...@openjdk.java.net
Subject: Re: RFR 7162400: Intermittent java.io.IOException: Bad file

number

during HotSpotVirtualMachine.executeCommand

Peter,

On 2013-07-09 14:25, Peter Allwin wrote:

Hello!

It is reproducible by letting the test create .java_pid* files for all
possible process id's on the system, setting correct access flags,
launching the target VM and attempting to connect. There are some
caveats though but it should be doable.

I'll convert the repro script to JTREG and add it to the webrev.


It's probably not a good idea to have a test which taints the system with

stale

.java_pid* files.
If the test execution times out and the script isn't allowed to clean up I
imagine that other subsequent executions could fail.
Is there a way to tell the attach api to use a specific directory so you

won't

need to taint /tmp?

/Mikael



Thanks for the reviews!

/peter

*From:*serguei.spit...@oracle.com [mailto:serguei.spit...@oracle.com]
*Sent:* Tuesday, July 9, 2013 1:26 AM
*To:* daniel.daughe...@oracle.com
*Cc:* Peter Allwin; serviceability-dev@openjdk.java.net;
hotspot-runtime-...@openjdk.java.net
*Subject:* Re: RFR 7162400: Intermittent java.io.IOException: Bad file
number during HotSpotVirtualMachine.executeCommand

Ok, thanks!

Peter, did you manage to reproduce this issue with your script?
If so, then, please, include it into the bug report and remove the
"noreg-sqe" label.

It is Ok if you did not reproduce it, though.

Thanks,
Serguei


On 7/8/13 4:20 PM, Daniel D. Daugherty wrote:

 I definitely don't insist... :-)

 BTW, I noticed this in Peter's e-mail:

 > Testing:
 > JPRT, reproducing script on Solaris, Linux.

 so maybe Peter already has this covered with "reproducing script"...

 Dan

 On 7/8/13 5:07 PM, serguei.spit...@oracle.com
 <mailto:serguei.spit...@oracle.com> wrote:

 Dan,

 Dan, thank you for the recommendation.
 But I'm still not sure it is a right thing to do.
 Even though, there are multiple test cases associated with this
 bug they
 can not be used to verify that fix because an additional

condition

 must be present as well.
 This condition is a presence of stale door file which is not
 that easy to reproduce.

 However, if you insist then I can change the lable to the
 "noreg-sqe"
 with the corresponding comment.

 Thanks,
 Serguei


 On 7/8/13 3:46 PM, Daniel D. Daugherty wrote:

 Serguei,

 There are a number of existing tests associated with this
 bug. I don't
 think that 'noreg-hard' is the right label. I think
 'noreg-sqe' is
 the right one:

 noreg-sqe
  Change can be verified by running an existing SQE test
 suite; the bug
  should identify the suite and the specific test

case(s).


 Dan

 On 7/8/13 12:59 PM, serguei.spit...@oracle.com
 <mailto:serguei.spit...@oracle.com> wrote:

 Peter,

 I've added the label "noreg-hard" with the comment to
 the report.
 It is not easy to reproduce the issue and demonstrate
 the fix in a regression test.

 Thanks,
 Serguei


 On 7/8/13 11:36 AM, serguei.spit...@oracle.com
 <mailto:serguei.spit...@oracle.com> wrote:

 Hi Peter,

 The fix looks good.

 Thanks,
   

Re: RFR 7162400: Intermittent java.io.IOException: Bad file number during HotSpotVirtualMachine.executeCommand

[Mikael Gerdin]

Peter,

On 2013-07-09 14:25, Peter Allwin wrote:

Hello!

It is reproducible by letting the test create .java_pid* files for all
possible process id’s on the system, setting correct access flags,
launching the target VM and attempting to connect. There are some
caveats though but it should be doable.

I’ll convert the repro script to JTREG and add it to the webrev.


It's probably not a good idea to have a test which taints the system 
with stale .java_pid* files.
If the test execution times out and the script isn't allowed to clean up 
I imagine that other subsequent executions could fail.
Is there a way to tell the attach api to use a specific directory so you 
won't need to taint /tmp?


/Mikael



Thanks for the reviews!

/peter

*From:*serguei.spit...@oracle.com [mailto:serguei.spit...@oracle.com]
*Sent:* Tuesday, July 9, 2013 1:26 AM
*To:* daniel.daughe...@oracle.com
*Cc:* Peter Allwin; serviceability-dev@openjdk.java.net;
hotspot-runtime-...@openjdk.java.net
*Subject:* Re: RFR 7162400: Intermittent java.io.IOException: Bad file
number during HotSpotVirtualMachine.executeCommand

Ok, thanks!

Peter, did you manage to reproduce this issue with your script?
If so, then, please, include it into the bug report and remove the
"noreg-sqe" label.

It is Ok if you did not reproduce it, though.

Thanks,
Serguei


On 7/8/13 4:20 PM, Daniel D. Daugherty wrote:

I definitely don't insist... :-)

BTW, I noticed this in Peter's e-mail:

> Testing:
> JPRT, reproducing script on Solaris, Linux.

so maybe Peter already has this covered with "reproducing script"...

Dan

On 7/8/13 5:07 PM, serguei.spit...@oracle.com
 wrote:

Dan,

Dan, thank you for the recommendation.
But I'm still not sure it is a right thing to do.
Even though, there are multiple test cases associated with this
bug they
can not be used to verify that fix because an additional condition
must be present as well.
This condition is a presence of stale door file which is not
that easy to reproduce.

However, if you insist then I can change the lable to the
"noreg-sqe"
with the corresponding comment.

Thanks,
Serguei


On 7/8/13 3:46 PM, Daniel D. Daugherty wrote:

Serguei,

There are a number of existing tests associated with this
bug. I don't
think that 'noreg-hard' is the right label. I think
'noreg-sqe' is
the right one:

noreg-sqe
 Change can be verified by running an existing SQE test
suite; the bug
 should identify the suite and the specific test case(s).

Dan

On 7/8/13 12:59 PM, serguei.spit...@oracle.com
 wrote:

Peter,

I've added the label "noreg-hard" with the comment to
the report.
It is not easy to reproduce the issue and demonstrate
the fix in a regression test.

Thanks,
Serguei


On 7/8/13 11:36 AM, serguei.spit...@oracle.com
 wrote:

Hi Peter,

The fix looks good.

Thanks,
Serguei

On 7/8/13 6:54 AM, Peter Allwin wrote:

Hello!

Looking for reviews of this change:

http://cr.openjdk.java.net/~allwin/7162400/webrev.01/



For CR:

http://bugs.sun.com/view_bug.do?bug_id=7162400

https://jbs.oracle.com/bugs/browse/JDK-7162400

Summary:

This change addresses an issue in the Attach API
on Solaris, Linux and BSD where an attaching
application can receive IOExceptions such as
“Bad file number” (Solaris), “Connection
refused” (Linux/BSD), or “well-known file is not
secure”.

The attach process uses a file in the temporary
directory as a door (Solaris) or domain socket
(Linux,BSD) to communicate with the VM. In
certain circumstances stale files can be left in
the file system which can cause the attaching
application to believe that the VM is ready to
receive a connection when it’s not. With this
change the stale file will be removed during VM
startup.

Note that there is

Re: RFR: 8013590: NPG: Add a memory pool MXBean for Metaspace

[Mikael Gerdin]

Erik,

On 2013-06-11 17:27, Erik Helin wrote:

Hi Mikael and Jon,

thanks for reviewing!

I have updated the webrev. The changes from webrev.00 are:
- Only exposing the compressed class space memory pool if compressed
   classes are used.
- Renamed the name of the pool to "Compressed Class Space" (was
   "Compressed Klass Space", note the 'K').
- Renamed the variable _cks_pool to _compressed_class_pool.
- Using max_uintx instead of _undefined_size.
- Updated the test and the rest of the code to take these new change
   into account.

Please see new webrev at:
http://cr.openjdk.java.net/~ehelin/8013590/webrev.01/


This looks much better, ship it!

/Mikael



On 2013-06-04, Jon Masamitsu wrote:

http://cr.openjdk.java.net/~ehelin/8013590/webrev.00/test/gc/metaspace/TestMetaspaceMemoryPool.java.html

The functions assertEquals(), assertTrue(), and assertDefined() seem
useful.  Is there
a more global place where they can be put so other can use them?


I agree that assertTrue and assertEquals should be move to a separate
file, but I think that assertDefined is mostly usable for tests dealing
with MXMemoryBeans (since defined is not always the same as "differ from
-1").

I would like to do these changes as a separate change. Jon, what do you
about that?

Thanks,
Erik

On 2013-05-31, Mikael Gerdin wrote:

(merging the gc-dev and svc-dev threads)

Hi Erik,

On 2013-05-29 10:44, Erik Helin wrote:

Hi all,

this want sent to hotspot-gc-...@openjdk.java.net, sending to
serviceability-dev@openjdk.java.net as well since the change is about
memory pools.

This change adds two memory pools for metaspace, one for Metaspace and
one for compressed klass space. The memory pool for compressed klass
space will only have valus that differ from 0 or -1 (undefined) if
compressed klass pointers are used.

Question: Should I use an empty pool when compressed klass pointers are
*not* used or should I just not expose the pool?

This change also adds a manager for the pools: Metaspace Manager.

I have also added a test that checks that the metaspace manager is
present and that the two pools are present. The test also verifies that
the compressed klass space pool act correct according to the
UseCompressedKlass flag.

The last time I added metaspace memory pools, it triggered some
unforeseen bugs:
- Two asserts in jmm_GetMemoryUsage that asserted that a memory pool was
   either of heap type or had an undefined init/max size.
- The jdk/test/java/lang/management/MemoryMXBean/MemoryTest.java failed
- The service lock was taken out of order with the metaspace locks

These bugs have all been fixed:
- The asserts have been removed since they are no longer true
- The test has been updated but is not part of this change since it is a
   JDK change
- This change does not make use of functions requiring the metaspace
   lock. I had to remove some verification code in free_chunks_total to
   ensure this.

Webrev:
http://cr.openjdk.java.net/~ehelin/8013590/webrev.00/


Overall I think your fix looks good but I disagree with your choice
of exposing an EmptyCompressedKlassSpacePool for the case of
-UseCompressedClassPointers.

Given the dynamic nature of the MemoryManagerMXBeans and
MemoryPoolMXBeans it seems more true to the intentions of the API to
ony expose a MemoryPool if it contains interesting information.

Generally I don't think users of the management APIs can (or should)
depend on the exact set of MemoryManagerMXBeans and
MemoryPoolMXBeans
available in a given VM instance.

/Mikael



Testing:
- One new jtreg test
- JPRT
- All the tests that failed in nighly testing last time now pass

Thanks,
Erik

Re: RFR: 8013590: NPG: Add a memory pool MXBean for Metaspace

[Mikael Gerdin]

(merging the gc-dev and svc-dev threads)

Hi Erik,

On 2013-05-29 10:44, Erik Helin wrote:

Hi all,

this want sent to hotspot-gc-...@openjdk.java.net, sending to
serviceability-dev@openjdk.java.net as well since the change is about
memory pools.

This change adds two memory pools for metaspace, one for Metaspace and
one for compressed klass space. The memory pool for compressed klass
space will only have valus that differ from 0 or -1 (undefined) if
compressed klass pointers are used.

Question: Should I use an empty pool when compressed klass pointers are
*not* used or should I just not expose the pool?

This change also adds a manager for the pools: Metaspace Manager.

I have also added a test that checks that the metaspace manager is
present and that the two pools are present. The test also verifies that
the compressed klass space pool act correct according to the
UseCompressedKlass flag.

The last time I added metaspace memory pools, it triggered some
unforeseen bugs:
- Two asserts in jmm_GetMemoryUsage that asserted that a memory pool was
   either of heap type or had an undefined init/max size.
- The jdk/test/java/lang/management/MemoryMXBean/MemoryTest.java failed
- The service lock was taken out of order with the metaspace locks

These bugs have all been fixed:
- The asserts have been removed since they are no longer true
- The test has been updated but is not part of this change since it is a
   JDK change
- This change does not make use of functions requiring the metaspace
   lock. I had to remove some verification code in free_chunks_total to
   ensure this.

Webrev:
http://cr.openjdk.java.net/~ehelin/8013590/webrev.00/


Overall I think your fix looks good but I disagree with your choice of 
exposing an EmptyCompressedKlassSpacePool for the case of 
-UseCompressedClassPointers.


Given the dynamic nature of the MemoryManagerMXBeans and 
MemoryPoolMXBeans it seems more true to the intentions of the API to ony 
expose a MemoryPool if it contains interesting information.


Generally I don't think users of the management APIs can (or should) 
depend on the exact set of MemoryManagerMXBeans and MemoryPoolMXBeans

available in a given VM instance.

/Mikael



Testing:
- One new jtreg test
- JPRT
- All the tests that failed in nighly testing last time now pass

Thanks,
Erik

Re: Review Request (S) 8013945: CMS fatal error: must own lock MemberNameTable_lock

[Mikael Gerdin]

Serguei,

On 2013-05-24 11:29, serguei.spit...@oracle.com wrote:

Please, review the fix for:
   bug: http://bugs.sun.com/view_bug.do?bug_id=8013945
   jbs: https://jbs.oracle.com/bugs/browse/JDK-8013945

Open webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2013/hotspot/8013945-JVMTI-JSR292.1/


I agree with the semantic change but I'm not that fond of the duplication.
Instead of duplicating the code you could have a:

MutexLockerEx ml( ( SafepointSynchronize::is_at_safepoint() ? NULL : 
MemberNameTable_lock ), Mutex::_no_safepoint_check_flag );


Or maybe

Mutex* mutex_or_null = SafepointSynchronize::is_at_safepoint() ? NULL : 
MemberNameTable_lock;

MutexLockerEx ml(mutex_or_null, Mutex::_no_safepoint_check_flag);

MutexLockerEx already has a null-check in both constructor and destructor.

/Mikael




Summary:
   CMS calls InstanceKlass::release_C_heap_structures() concurrently.
   The "delete mnt" needs to take MemberNameTable_lock if
!SafepointSynchronize::is_at_safepoint().

Testing:
   The vm/mlvm and Nashorn tests, the tests listed in the bug report

Thanks,
Serguei

hg: hsx/hotspot-rt/hotspot: 8009763: Add WB test for String.intern()

[mikael . gerdin]

Changeset: ede380e13960
Author:mgerdin
Date:  2013-04-02 11:28 +0200
URL:   http://hg.openjdk.java.net/hsx/hotspot-rt/hotspot/rev/ede380e13960

8009763: Add WB test for String.intern()
Summary: Add convenience method in StringTable, add WhiteBox method and simple 
sanity test
Reviewed-by: mgerdin, zgu
Contributed-by: leonid.mes...@oracle.com

! src/share/vm/classfile/symbolTable.cpp
! src/share/vm/classfile/symbolTable.hpp
! src/share/vm/prims/whitebox.cpp
+ test/runtime/interned/SanityTest.java
! test/testlibrary/whitebox/sun/hotspot/WhiteBox.java

hg: hsx/hotspot-rt/hotspot: 8006753: fix failed for JDK-8002415 White box testing API for HotSpot

[mikael . gerdin]

Changeset: 1b0dc9f87e75
Author:mgerdin
Date:  2013-02-19 18:45 +0100
URL:   http://hg.openjdk.java.net/hsx/hotspot-rt/hotspot/rev/1b0dc9f87e75

8006753: fix failed for JDK-8002415 White box testing API for HotSpot
Summary: Modify WhiteBoxAPI to use interface classes from test/testlibrary 
instead, add ClassFileInstaller to resolve the boot class path issue
Reviewed-by: ctornqvi, dsamersoff, coleenp, kvn

! make/Makefile
! make/bsd/makefiles/defs.make
! make/bsd/makefiles/vm.make
- make/bsd/makefiles/wb.make
! make/linux/makefiles/defs.make
! make/linux/makefiles/vm.make
- make/linux/makefiles/wb.make
! make/solaris/makefiles/defs.make
! make/solaris/makefiles/vm.make
- make/solaris/makefiles/wb.make
! make/windows/makefiles/debug.make
! make/windows/makefiles/defs.make
! make/windows/makefiles/fastdebug.make
! make/windows/makefiles/product.make
- make/windows/makefiles/wb.make
- src/share/tools/whitebox/sun/hotspot/WhiteBox.java
- src/share/tools/whitebox/sun/hotspot/parser/DiagnosticCommand.java
! src/share/vm/runtime/arguments.cpp
! test/compiler/whitebox/DeoptimizeAllTest.java
! test/compiler/whitebox/DeoptimizeMethodTest.java
! test/compiler/whitebox/IsMethodCompilableTest.java
! test/compiler/whitebox/MakeMethodNotCompilableTest.java
! test/compiler/whitebox/SetDontInlineMethodTest.java
! test/runtime/NMT/AllocTestType.java
! test/runtime/NMT/PrintNMTStatistics.java
! test/runtime/NMT/SummarySanityCheck.java
! test/sanity/WBApi.java
! test/serviceability/ParserTest.java
+ test/testlibrary/ClassFileInstaller.java
+ test/testlibrary/whitebox/sun/hotspot/WhiteBox.java
+ test/testlibrary/whitebox/sun/hotspot/parser/DiagnosticCommand.java

Re: 答复: Errors when use "universe" command in CLHSDB

[Mikael Gerdin]

  }
> 
> // large block
> 
> -  BinaryTreeDictionary bfbd = (BinaryTreeDictionary)
> VMObjectFactory.newObject(BinaryTreeDictionary.class,
> 
> +  AFLBinaryTreeDictionary bfbd = (AFLBinaryTreeDictionary)
> VMObjectFactory.newObject(AFLBinaryTreeDictionary.class,
> 
>   
>dictionaryField.getValue(addr));
> 
> size += bfbd.size();
> 
> diff -r b14da2e6f2dc -r 8652e04889a4
> agent/src/share/classes/sun/jvm/hotspot/memory/FreeList.java
> 
> ---
> a/agent/src/share/classes/sun/jvm/hotspot/memory/FreeList.java
> Thu Jan 17 13:40:31 2013 -0500
> 
> +++
> b/agent/src/share/classes/sun/jvm/hotspot/memory/FreeList.java 
> Fri Jan 18 09:56:06 2013 +0800
> 
> @@ -41,7 +41,7 @@
> 
>  }
> 
>  private static synchronized void initialize(TypeDataBase db) {
> 
> -  Type type = db.lookupType("FreeList");
> 
> +  Type type = db.lookupType("FreeList");
> 
> sizeField = type.getCIntegerField("_size");
> 
> countField = type.getCIntegerField("_count");
> 
> headerSize = type.getSize();
> 
> diff -r b14da2e6f2dc -r 8652e04889a4
> src/share/vm/gc_implementation/concurrentMarkSweep/vmStructs_cms.hpp
> 
> ---
> a/src/share/vm/gc_implementation/concurrentMarkSweep/vmStructs_cms.hpp 
> Thu Jan 17 13:40:31 2013 -0500
> 
> +++
> b/src/share/vm/gc_implementation/concurrentMarkSweep/vmStructs_cms.hpp
> 
> Fri Jan 18 09:56:06 2013 +0800
> 
> @@ -43,7 +43,8 @@
> 
> nonstatic_field(LinearAllocBlock, 
>_word_size,   
> size_t)\
> 
> nonstatic_field(AFLBinaryTreeDictionary,
> _total_size,  
> size_t)\
> 
> nonstatic_field(CompactibleFreeListSpace,   
> _indexedFreeList[0],  
> FreeList)   \
> 
> -  nonstatic_field(CompactibleFreeListSpace,   
> _smallLinearAllocBlock,LinearAllocBlock)
> 
> +  nonstatic_field(CompactibleFreeListSpace,   
> _smallLinearAllocBlock,   
> LinearAllocBlock)  \
> 
> +  nonstatic_field(CompactibleFreeListSpace,   
> _dictionary,  
> FreeBlockDictionary*)
> 
>   #define
> VM_TYPES_CMS(declare_type,\
> 
> Regards,
> 
> Yunda
> 
> 
> 
> 
> This email (including any attachments) is confidential and may be
> legally privileged. If you received this email in error, please
> delete it immediately and do not copy it or use it for any purpose
> or disclose its contents to any other person. Thank you.
> 
> 本电邮(包括任何附件)可能含有机密资料并受法律保护。如您不是正确的收件
> 人，请您立即删除本邮件。请不要将本电邮进行复制并用作任何其他用途、或
> 透露本邮件之内容。谢谢。
> 
> 
> 
> 
> This email (including any attachments) is confidential and may be 
> legally privileged. If you received this email in error, please delete 
> it immediately and do not copy it or use it for any purpose or disclose 
> its contents to any other person. Thank you.
> 
> 本电邮(包括任何附件)可能含有机密资料并受法律保护。如您不是正确的收件人， 
> 请您立即删除本邮件。请不要将本电邮进行复制并用作任何其他用途、或透露本邮 
> 件之内容。谢谢。

-- 
Mikael Gerdin
Java SE VM SQE Stockholm

Re: Review request (S) 7195557 NPG: Unexpected number of memory pools

[Mikael Gerdin]

(adding serviceability-dev again)
Jon

On 2012-09-06 17:05, Jon Masamitsu wrote:

Mikael,

Does the code in CollectionUsageThreshold.java
happen to work if perm is the last memory pool
in the list and the test

  139 if (result.size() == numMemoryPools) {
  140 break;
  141 }

exits the loop having never seen perm (so not incrementing
numMemoryPools?


Good point. I'll have to look at this tomorrow. Unfortunately this 
version of the fix has already been pushed so if we need to fix this 
I'll open a new CR.


/Mikael



Jon

On 09/05/12 23:54, Mikael Gerdin wrote:

Somehow serviceability-dev was dropped from the CC list.
I uploaded a new webrev since I thought Bengt had a point about being
consistent.
We talked about where to integrate the fix during the perm gen removal
meeting and decided that we'd like to push this through jdk8/tl/jdk
since we're not in a rush for the bug fix, it's already marked as a
known failure.

Can someone from the jdk8 project please Review this fix?
Thanks
/Mikael

On 2012-09-05 12:38, Mikael Gerdin wrote:

Bengt,

On 2012-09-05 10:52, Bengt Rutisson wrote:


Hi Mikael,

I think this looks good.

One minor thing. You kind of solved the same problem in two different
ways. In CollectionUsageThreshold.java you reduce the expected number
and then increase it if you find a perm gen pool. In
MemoryMXBean/MemoryTest.java you left the expected value unchanged and
only reduce it if we do not find a perm gen pool.

I think it would be cleaner to use the same approach for both tests and
I think I prefer the way you did it in CollectionUsageThreshold.java
since the "normal" case from now on should be that there is no perm
gen.


Good point, I agree that it looks strange, I've updated the MemoryTest
changes to act in a similar way to CollectionUsageThreshold.

I've uploaded a new webrev with the updated version:
http://cr.openjdk.java.net/~mgerdin/7195557/webrev.1

/Mikael



Bengt

On 2012-09-05 10:13, Mikael Gerdin wrote:

Hi,

With the perm gen removal changes the number of memory pools exported
by the VM has changed. This causes the tests
java/lang/management/MemoryMXBean/MemoryTest.java and
java/lang/management/MemoryMXBean/CollectionUsageThreshold.java to
fail.

My suggested fix is to modify the tests to work with VMs both with and
without perm gen memory pools.

CR link:
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7195557
Webrev:
http://cr.openjdk.java.net/~mgerdin/7195557/webrev

I'd also like to request that this fix be allowed to be pushed to
hsx/hotspot-gc/jdk instead of 8-tl/jdk so we can get rid of these test
failures in the hotspot testing before we integrate perm removal into
jdk8.
With that I'll need someone to sponsor the push since I'm not a
Committer.

Thanks
/Mikael









--
Mikael Gerdin
Java SE VM SQE Stockholm

Re: Review request (S) 7195557 NPG: Unexpected number of memory pools

[Mikael Gerdin]

Thanks for the reviews,
I'll try to get someone here in Stockholm to push this to jdk8 for me.

/Mikael

On 2012-09-06 11:04, David Holmes wrote:

On 6/09/2012 5:07 PM, Staffan Larsen wrote:

Looks good to me, too.


Me too.


Not a Reviewer, either.


I am :)

David
-


/Staffan

On 6 sep 2012, at 08:58, Bengt Rutisson
wrote:



Thanks, Mikael! I think this fix looks good also when sent out to the
serviceability mailing list :-)

I am not a JDK reviewer...

Bengt

On 2012-09-06 08:54, Mikael Gerdin wrote:

Somehow serviceability-dev was dropped from the CC list.
I uploaded a new webrev since I thought Bengt had a point about
being consistent.
We talked about where to integrate the fix during the perm gen
removal meeting and decided that we'd like to push this through
jdk8/tl/jdk
since we're not in a rush for the bug fix, it's already marked as a
known failure.

Can someone from the jdk8 project please Review this fix?
Thanks
/Mikael

On 2012-09-05 12:38, Mikael Gerdin wrote:

Bengt,

On 2012-09-05 10:52, Bengt Rutisson wrote:


Hi Mikael,

I think this looks good.

One minor thing. You kind of solved the same problem in two different
ways. In CollectionUsageThreshold.java you reduce the expected number
and then increase it if you find a perm gen pool. In
MemoryMXBean/MemoryTest.java you left the expected value unchanged
and
only reduce it if we do not find a perm gen pool.

I think it would be cleaner to use the same approach for both
tests and
I think I prefer the way you did it in CollectionUsageThreshold.java
since the "normal" case from now on should be that there is no
perm gen.


Good point, I agree that it looks strange, I've updated the MemoryTest
changes to act in a similar way to CollectionUsageThreshold.

I've uploaded a new webrev with the updated version:
http://cr.openjdk.java.net/~mgerdin/7195557/webrev.1

/Mikael



Bengt

On 2012-09-05 10:13, Mikael Gerdin wrote:

Hi,

With the perm gen removal changes the number of memory pools
exported
by the VM has changed. This causes the tests
java/lang/management/MemoryMXBean/MemoryTest.java and
java/lang/management/MemoryMXBean/CollectionUsageThreshold.java
to fail.

My suggested fix is to modify the tests to work with VMs both
with and
without perm gen memory pools.

CR link:
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7195557
Webrev:
http://cr.openjdk.java.net/~mgerdin/7195557/webrev

I'd also like to request that this fix be allowed to be pushed to
hsx/hotspot-gc/jdk instead of 8-tl/jdk so we can get rid of these
test
failures in the hotspot testing before we integrate perm removal
into
jdk8.
With that I'll need someone to sponsor the push since I'm not a
Committer.

Thanks
/Mikael













--
Mikael Gerdin
Java SE VM SQE Stockholm

Re: Review request (S) 7195557 NPG: Unexpected number of memory pools

[Mikael Gerdin]

Somehow serviceability-dev was dropped from the CC list.
I uploaded a new webrev since I thought Bengt had a point about being 
consistent.
We talked about where to integrate the fix during the perm gen removal 
meeting and decided that we'd like to push this through jdk8/tl/jdk
since we're not in a rush for the bug fix, it's already marked as a 
known failure.


Can someone from the jdk8 project please Review this fix?
Thanks
/Mikael

On 2012-09-05 12:38, Mikael Gerdin wrote:

Bengt,

On 2012-09-05 10:52, Bengt Rutisson wrote:


Hi Mikael,

I think this looks good.

One minor thing. You kind of solved the same problem in two different
ways. In CollectionUsageThreshold.java you reduce the expected number
and then increase it if you find a perm gen pool. In
MemoryMXBean/MemoryTest.java you left the expected value unchanged and
only reduce it if we do not find a perm gen pool.

I think it would be cleaner to use the same approach for both tests and
I think I prefer the way you did it in CollectionUsageThreshold.java
since the "normal" case from now on should be that there is no perm gen.


Good point, I agree that it looks strange, I've updated the MemoryTest
changes to act in a similar way to CollectionUsageThreshold.

I've uploaded a new webrev with the updated version:
http://cr.openjdk.java.net/~mgerdin/7195557/webrev.1

/Mikael



Bengt

On 2012-09-05 10:13, Mikael Gerdin wrote:

Hi,

With the perm gen removal changes the number of memory pools exported
by the VM has changed. This causes the tests
java/lang/management/MemoryMXBean/MemoryTest.java and
java/lang/management/MemoryMXBean/CollectionUsageThreshold.java to fail.

My suggested fix is to modify the tests to work with VMs both with and
without perm gen memory pools.

CR link:
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7195557
Webrev:
http://cr.openjdk.java.net/~mgerdin/7195557/webrev

I'd also like to request that this fix be allowed to be pushed to
hsx/hotspot-gc/jdk instead of 8-tl/jdk so we can get rid of these test
failures in the hotspot testing before we integrate perm removal into
jdk8.
With that I'll need someone to sponsor the push since I'm not a
Committer.

Thanks
/Mikael







--
Mikael Gerdin
Java SE VM SQE Stockholm

Re: Review request (S) 7195557 NPG: Unexpected number of memory pools

[Mikael Gerdin]

Alan,

On 2012-09-05 11:00, Alan Bateman wrote:

On 05/09/2012 09:13, Mikael Gerdin wrote:

Hi,

With the perm gen removal changes the number of memory pools exported
by the VM has changed. This causes the tests
java/lang/management/MemoryMXBean/MemoryTest.java and
java/lang/management/MemoryMXBean/CollectionUsageThreshold.java to fail.

My suggested fix is to modify the tests to work with VMs both with and
without perm gen memory pools.

CR link:
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7195557
Webrev:
http://cr.openjdk.java.net/~mgerdin/7195557/webrev

I'd also like to request that this fix be allowed to be pushed to
hsx/hotspot-gc/jdk instead of 8-tl/jdk so we can get rid of these test
failures in the hotspot testing before we integrate perm removal into
jdk8.
With that I'll need someone to sponsor the push since I'm not a
Committer.

Thanks
/Mikael


It should okay to push this via the hsx/hotspot-gc forest, assuming of
course that the tests in its jdk repository are actually used for the
testing that you mention. One thing to know is that
test/java/lang/management/MemoryMXBean/CollectionUsageThreshold.java is
on the exclude list (see jdk/test/ProblemList.txt) so the test should
not be run (so in Oracle this means it is excluded in JPRT, in testing
of jdk8/tl, etc.). If you are happy that the test is now stable then it
would be good to remove it, but I think you might want to check 7067973
in case that issue and failure mode still exists.


Good point about looking up where the java/lang/management tests for 
hotspot-gc actually come from. It appears that we pick up the sources 
from the latest promoted build of jdk8 for those tests. So there's 
really no point in pushing the fix to hotspot-gc.

I guess I'll need someone to push this through 8-tl then when it's reviewed.

The nightly testing done on hotspot ignores ProblemList.txt so it's 
still run in our testing. I looked at the recent test execution history 
of CollectionUsageThreshold and I see some timeouts but only when 
running -XX:+UseG1GC and -XX:+ExplicitGCInvokesConcurrent


/Mikael



-Alan.


--
Mikael Gerdin
Java SE VM SQE Stockholm

Review request (S) 7195557 NPG: Unexpected number of memory pools

[Mikael Gerdin]

Hi,

With the perm gen removal changes the number of memory pools exported by 
the VM has changed. This causes the tests 
java/lang/management/MemoryMXBean/MemoryTest.java and 
java/lang/management/MemoryMXBean/CollectionUsageThreshold.java to fail.


My suggested fix is to modify the tests to work with VMs both with and 
without perm gen memory pools.


CR link:
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7195557
Webrev:
http://cr.openjdk.java.net/~mgerdin/7195557/webrev

I'd also like to request that this fix be allowed to be pushed to 
hsx/hotspot-gc/jdk instead of 8-tl/jdk so we can get rid of these test 
failures in the hotspot testing before we integrate perm removal into jdk8.

With that I'll need someone to sponsor the push since I'm not a Committer.

Thanks
/Mikael

--
Mikael Gerdin
Java SE VM SQE Stockholm

Re: RFR (L): Adding core file parsing on Mac OS X to SA

[Mikael Gerdin]

Staffan,

I looked through test makefile changes and the white box test, the 
changes seem alright but I'm not convinced that using a white box test 
for this is the best approach.
There are some existing tests for the error handler and hs_err creation 
in test_error_handler in debug.cpp (look for use of the ErrorHandlerTest 
flag)


I realize that this would not work for your test since you use jstack to 
verify that the SA works and ErrorHandlerTest crashes the VM before any 
Java frames are on the stack. But it would be great if we could re-use 
the existing code to force the VM to crash instead of duplicating it.


Anyway, I won't object to the changes in their current state (and I'm 
not a Committer, so don't listen to me).


/mg

On 2012-05-08 13:11, Staffan Larsen wrote:

The Serviceability Agent currently has an outage on Mac OS X compared to
other platforms in that we cannot read core files. We plan to address
this for 7u6 by incorporating code from a separate tool called "kjdb"
developed at Oracle. Kjdb is a cross-platform core-file debugger written
completely in Java that currently works on ELF (solaris, linux) and
Mach-O (mac) core files. Since it is written in Java it can read core
files from a different system than the system you are running kjdb on,
which is very useful.

The alternative to incorporating the kjdb technology would have been to
implement this in C as is done for the other platforms, but reusing
existing code saves us some trouble. The kjdb debugger backend will
automatically be enabled when you are opening a Mach-O core file (or if
you explicitly set the -Duse.kjdb property when starting SA).

The risks with this is that this is a large addition of relatively new
code to SA (around 100 files). It also means that debugging a live
process or a core file on OS X will use different debugger backends
which may have different behavior and bugs.

This change also adds a basic test that creates a core file and uses SA
to print the thread dumps in the core. This test runs on all platforms,
not just Mac OS X. Other than that test, some level of manual testing
has been done with various core files.

Please see the webrev at: http://cr.openjdk.java.net/~sla/kjdb/webrev.01/

There are quite a number of added files in this webrev, so it's probably
best to concentrate on the changes in the existing files.

Thanks,
/Staffan

Re: RFR (and sponsor): 7148488: Whitebox tests for the Diagnostic Framework Parser

[Mikael Gerdin]

Hi,

just a small nitpick in parserTests.cpp:
41: const char* lookup_diagnosticArgumentEnum(const char* field_name, 
oop object)

and
52: void fill_in_parser(DCmdParser* parser, oop argument)
should probably be static (in the C sense).

Otherwise it looks good.

/Mikael


On 2012-03-15 13:45, Nils Loodin wrote:

Hey all!

Here's an implementation of a nice way of doing parser testing from a
jtreg-test, through the whitebox testing framework.

This patch makes it easy to do parser testing (which will be necessary
if we want to change it with any sort of confidence in the future) and
partly to show off what can be possible to do with the whitebox testing api.

In the added JTREG test, parser testing now works like this from java:

//test that we can parse without exceptions
wb.parseCommandLine("myIntArgument=10", args);

//test that the value get's parsed to what we want
parse("myIntArgument", "10", "myIntArgument=10", args);

//test that illegal command lines gives exception and aren't silently broken
shouldFail("myLongArgument=12m", args); //<-- should fail, doesn't

http://cr.openjdk.java.net/~nloodin/7148488/webrev.00/

Regards,
Nils Loodin

Re: SA frontends

[Mikael Gerdin]

Hi Staffan

On 2012-02-22 09:26, Staffan Larsen wrote:

I'm trying to figure out what the different front ends to SA are and how they 
differ. So far I have identified the following:

HSDB - basic UI
CLHSDB - command line version of HSDB
BugSpot - improvement over HSDB ??
JSDB - JavaScript command line
SOQL - Simple Object Query Language command line

Have I missed any?


I think that several of the command-line "servicability" tools use the 
SA code when working on core files.

Looking in the agent/src/share/classes/sun/jvm/hotspot/tools/
there are classes for jmap, jinfo and jstack.

/Mikael



What are your experiences with these? Which ones are most used? Should we 
maintain all of them? Can we consolidate them into one? Specifically: what is 
the difference between HSDB and BugSpot?

I'd be happy for all and any feedback,
/Staffan

Re: Request for review (M): 7132070: Use a mach_port_t as the OSThread thread_id rather than pthread_t on BSD/OSX

[Mikael Gerdin]

Hi Staffan,

It looks like you're adding Mac-specific stuff like thread_t and calls 
to ::mach_thread_self() inside _ALLBSD_SOURCE #ifdefs, are you sure this 
won't break BSD builds?
Does the OSX compiler define _ALLBSD_SOURCE or is that for 
(free|net|open)bsd?
It's too bad we don't do regular builds on any of the BSDs, otherwise 
this would have been easier to figure out.


/Mikael


On 2012-02-15 11:29, Staffan Larsen wrote:

Please review the following change:

Bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7132070

Webrev: http://cr.openjdk.java.net/~sla/7132070/webrev.00/

This changes the value returned by OSThread::thread_id() and
os::current_thread_id() on macosx to return the mach thread_t instead of
pthread_t. There is a separate method OSThread:pthread_id() that returns
the pthread_t.

The reason for this change is both that JFR would like a 4 byte value
for thread id, and that SA requires access to the thread_t.

Thanks,
/Staffan

Re: Request for Review (XS): 7143760 Memory leak in GarbageCollectionNotifications

[Mikael Gerdin]

Hi Fred,

I ran the repro on a build with your fix applied and it appears that the leak 
is fixed. Thanks for finding it so quick :)

/Mikael

On Friday 10 February 2012 10.27.30 Frederic Parain wrote:
> Here's a small fix (one line) for CR 7143760  Memory leak in
> GarbageCollectionNotifications
> 
> There's a missing HandleMark at the beginning of the
> GCNotifier::sendNotificatin() method. Without this HandleMark, all
> handles used when creating GC notifications are kept alive causing a
> double leak: in the Java heap and in the thread local handle area of the
> service thread.
> 
> Here's the CR:
> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7143760
> (Warning, the changeset referenced in the CR is not the
> one containing the original bug).
> 
> Here's the webrev:
> http://cr.openjdk.java.net/~fparain/7143760/webrev.00/
> 
> Thanks,
> 
> Fred