Re: RFR(XS): 8218947: jdb threads command should print threadID in decimal, not hex

[David Holmes]

Hi Chris,

On 14/02/2019 4:54 pm, Chris Plummer wrote:

Hi David,

I can update the man page. Can you point me to where it is in the 
source? I haven't been able to find it. It seems to be out of date in 
other ways also. 


Will send pointer separately as it's not part of OpenJDK.


This is what the output looks like (with my fix).

main[1] threads
Group system:
   (java.lang.ref.Reference$ReferenceHandler)908 Reference Handler running
   (java.lang.ref.Finalizer$FinalizerThread)909  Finalizer cond. waiting
   (java.lang.Thread)910 Signal Dispatcher running
Group main:
   (java.lang.Thread)1   main running (at 
breakpoint)

Group InnocuousThreadGroup:
   (jdk.internal.misc.InnocuousThread)911    Common-Cleaner cond. 
waiting


Note there is no index printed as the docs suggest. Also, the docs don't 
even mention having the threadID printed.


Okay probably need a separate bug to update the man page then.

I found this change very useful while using jdb to debug the JDWP debug 
agent. If I don't push it, it will likely just stay as an uncommitted 
change in my local repo indefinitely since I continue to find use for it.


As for the status of jdb, it is supported and we do actively fix it. 
It's greatest use is probably in testing JDI, since a large number of 
JDI tests do so using jdb. Also, I've been using it a lot lately to test 
loom debugging support I've been adding to the JDWP debug agent.


I don't know about needing a CSR request. The rules for needing one 
elude me. I consider you the expert in that area. :)


Well this isn't changing a programming interface just the UI, so unless 
people use it with some kind of screen scraping I can't see there being 
a compatibility issue.


Cheers,
David
-


thanks,

Chris

On 2/13/19 8:53 PM, David Holmes wrote:



pps. You'll also need to update the man page as it has an example 
showing the hex:


 threads
  List the threads that are currently running. For each 
thread, its name and current status are printed and
  an index that can be used in other commands. In this 
example, the thread index is 4, the thread is an
  instance of java.lang.Thread, the thread name is main, 
and it is currently running.


  4. (java.lang.Thread)0x1 main  running

Is this change really worth bothering with? What's the status of jdb 
(I was surprised to seem this is all an example!) ? Does this need a 
CSR request?


Cheers,
David
On 14/02/2019 2:36 pm, David Holmes wrote:

PS.

  return MessageOutput.format("object description and hex id",

Need to change the message too!

David

On 14/02/2019 2:32 pm, David Holmes wrote:

Hi Chris,

On 14/02/2019 1:37 pm, Chris Plummer wrote:

Hi,

Please review the following:

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


Are there times you may want to correlate those thread ids with ones 
in other logs/tools that are in hex?


That aside:

java.lang.Long.toString

you don't need the java.lang part (it's always implicitly imported).

Thanks,
David
-


Tested by running the following on all supported platforms:

open/test/hotspot/jtreg/vmTestbase/nsk/jdb
open/test/jdk/com/sun/jdi

thanks,

Chris

Re: RFR: 8218731: SA: Use concrete class the as return type of VMObjectFactory.newObject

[Erik Österlund]

Hi Stefan,

This looks good and trivial.

Thanks,
/Erik

On 2019-02-11 08:47, Stefan Karlsson wrote:

Hi all,

I propose this simple change to use the concrete class as the return 
type of VMObjectFactory.newObject.


https://cr.openjdk.java.net/~stefank/8218731/webrev.01
https://bugs.openjdk.java.net/browse/JDK-8218731

This allows us to specify the class only once when calling newObject. 
For example, now we can use:
return VMObjectFactory.newObject(ZPhysicalMemoryManager.class, 
physicalAddr);


Instead of having to write:
    return 
(ZPhysicalMemoryManager)VMObjectFactory.newObject(ZPhysicalMemoryManager.class, 
physicalAddr);


Thanks,
StefanK

Re: RFR: 8218732: SA: Resolves ZPageAllocator::_physical incorrectly

[Erik Österlund]

Hi Stefan,

Given that the remark from Jini is fixed, this looks good. I don't need 
another webrev.


Thanks,
/Erik

On 2019-02-11 19:09, Stefan Karlsson wrote:

Hi Jini,

On 2019-02-11 19:00, Jini George wrote:

Hi Stefan,

Looks good to me overall. One point though.

physicalFieldOffset = type.getAddressField("_physical").getOffset();

Wrt the line above, is there any reason due to which getAddressField() 
instead of getField() was used ? getAddressField() is typically used 
when the field to be read in is a pointer. I feel getField() might be 
more appropriate in this situation.


No, this is just an oversight. I have the same issue in my later 
OopHandle patch. I'll change this.


Thanks for reviewing,
StefanK



Thanks,
Jini.

On 2/11/2019 1:23 PM, Stefan Karlsson wrote:

Hi all,

Please review this small patch to resolve ZPageAllocator::_physical 
as a value object instead of a pointer.


https://cr.openjdk.java.net/~stefank/8218732/webrev.01/
https://bugs.openjdk.java.net/browse/JDK-8218732

This fixes the Heap Parameters view.

Thanks,
StefanK

Re: RFR: 8218733: SA: CollectedHeap provides broken implementation for used() and capacity()

[Erik Österlund]

Hi Stefan,

Looks good.

Thanks,
/Erik

On 2019-02-11 09:13, Stefan Karlsson wrote:

Hi all,

Please review this patch to remove the broken implementation of 
CollectedHeap used() and capacity() and instead force all GCs to provide 
their own implementations.


https://cr.openjdk.java.net/~stefank/8218733/webrev.01/
https://bugs.openjdk.java.net/browse/JDK-8218733

This was found while running 
serviceability/sa/TestHeapDumpForLargeArray.java on an experimental 
implementation of heap dumping in ZGC.


ZGC didn't provide a ZCollectedHeap.used() function and 
CollectedHeap.used() was used instead at:

 // Check weather we should dump the heap as segments
 useSegmentedHeapDump = vm.getUniverse().heap().used() > 
HPROF_SEGMENTED_HEAP_DUMP_THRESHOLD;


Because of this we incorrectly did not use segmented heap dumps, and 
therefore overflowed later in the code


Aleksey and Roman,

Could you verify that the implementation for Epsilon is correct? I also 
haven't implemented capacity for Shenandoah, as the information isn't 
trivially available in the ShenandoahHeap SA class. Do you want to fix 
it as part of this patch, or should I create a separate RFE for Shenandoah?


Thanks,
StefanK

Re: RFR: 8218734: SA: Incorrect and raw loads of OopHandles

[Erik Österlund]

Hi Stefan,

Looks good. One thing though...

At VMOopHandle.java line 38:

* Remove weird double indentation
* Remove weird double semicolon
* Use Field instead of AddressField

I don't think I need another webrev.

Thanks,
/Erik

On 2019-02-11 09:39, Stefan Karlsson wrote:

Hi all,

Please review this patch to fix the resolving of oops inside the (VM) 
OopHandles.


https://cr.openjdk.java.net/~stefank/8218734/webrev.01/
https://bugs.openjdk.java.net/browse/JDK-8218734

Before this patch the OopHandle::_obj was assumed to be located at 
offset 0, and the SA resolved OopHandle (Klass::_java_mirror and 
ClassLoaderData::_class_loader) without the required load barrier for 
ZGC. I've added a new class VMOopHandle (The SA already has a 
OopHandle), which handles the resolving (load or load + barrier).


The OopHandle type is grouped under the Oops section in vmStructs.cpp. 
It's not an oop, so I added a newline to separate it out from the rest. 
Any suggestion of a better location in that file?


Tested with the jtreg tests in serviceability/sa + patches to make ZGC 
usable with the SA's hprof dumping.


Thanks,
StefanK

Re: RFR: 8218743: SA: Add support for large bitmaps

[Erik Österlund]

Hi Stefan,

Looks good.

Thanks,
/Erik

On 2019-02-11 13:36, Stefan Karlsson wrote:

Hi all,

Please review this patch to add support for large bitmaps in the SA.

http://cr.openjdk.java.net/~stefank/8218743/webrev.01/
https://bugs.openjdk.java.net/browse/JDK-8218743

I've added a minimal interface (at, atPut, clear) that uses longs 
instead of ints, and changed MarkBits to use this interface.


I've implemented a segmented bitmap that, currently, all GCs use when 
asked for a bitmap in MarkBits. Later ZGC will provide its own 
discontiguous bitmap, implementing the new interface, but that will be 
sent out as a separate RFE.


I've tested this with a temporary patch that lowers the segment size and 
implements a jtreg test:

http://cr.openjdk.java.net/~stefank/8218743/webrev.test.01

I ran through the all the tests in serviceability/sa with both these 
patches.


Thanks,
StefanK

Re: RFR: 8218731: SA: Use concrete class the as return type of VMObjectFactory.newObject

[Stefan Karlsson]

Thanks, Erik.

Stefank

On 2019-02-14 09:19, Erik Österlund wrote:

Hi Stefan,

This looks good and trivial.

Thanks,
/Erik

On 2019-02-11 08:47, Stefan Karlsson wrote:

Hi all,

I propose this simple change to use the concrete class as the return 
type of VMObjectFactory.newObject.


https://cr.openjdk.java.net/~stefank/8218731/webrev.01
https://bugs.openjdk.java.net/browse/JDK-8218731

This allows us to specify the class only once when calling newObject. 
For example, now we can use:
return VMObjectFactory.newObject(ZPhysicalMemoryManager.class, 
physicalAddr);


Instead of having to write:
    return 
(ZPhysicalMemoryManager)VMObjectFactory.newObject(ZPhysicalMemoryManager.class, 
physicalAddr);


Thanks,
StefanK

Re: RFR: 8218732: SA: Resolves ZPageAllocator::_physical incorrectly

[Stefan Karlsson]

Thanks, Erik.

Stefank

On 2019-02-14 09:26, Erik Österlund wrote:

Hi Stefan,

Given that the remark from Jini is fixed, this looks good. I don't need 
another webrev.


Thanks,
/Erik

On 2019-02-11 19:09, Stefan Karlsson wrote:

Hi Jini,

On 2019-02-11 19:00, Jini George wrote:

Hi Stefan,

Looks good to me overall. One point though.

physicalFieldOffset = type.getAddressField("_physical").getOffset();

Wrt the line above, is there any reason due to which 
getAddressField() instead of getField() was used ? getAddressField() 
is typically used when the field to be read in is a pointer. I feel 
getField() might be more appropriate in this situation.


No, this is just an oversight. I have the same issue in my later 
OopHandle patch. I'll change this.


Thanks for reviewing,
StefanK



Thanks,
Jini.

On 2/11/2019 1:23 PM, Stefan Karlsson wrote:

Hi all,

Please review this small patch to resolve ZPageAllocator::_physical 
as a value object instead of a pointer.


https://cr.openjdk.java.net/~stefank/8218732/webrev.01/
https://bugs.openjdk.java.net/browse/JDK-8218732

This fixes the Heap Parameters view.

Thanks,
StefanK

Re: RFR: 8218734: SA: Incorrect and raw loads of OopHandles

[Stefan Karlsson]

On 2019-02-14 10:01, Erik Österlund wrote:

Hi Stefan,

Looks good. One thing though...

At VMOopHandle.java line 38:

* Remove weird double indentation
* Remove weird double semicolon
* Use Field instead of AddressField


I'll fix.



I don't think I need another webrev.


Thanks for reviewing.

StefanK



Thanks,
/Erik

On 2019-02-11 09:39, Stefan Karlsson wrote:

Hi all,

Please review this patch to fix the resolving of oops inside the (VM) 
OopHandles.


https://cr.openjdk.java.net/~stefank/8218734/webrev.01/
https://bugs.openjdk.java.net/browse/JDK-8218734

Before this patch the OopHandle::_obj was assumed to be located at 
offset 0, and the SA resolved OopHandle (Klass::_java_mirror and 
ClassLoaderData::_class_loader) without the required load barrier for 
ZGC. I've added a new class VMOopHandle (The SA already has a 
OopHandle), which handles the resolving (load or load + barrier).


The OopHandle type is grouped under the Oops section in vmStructs.cpp. 
It's not an oop, so I added a newline to separate it out from the 
rest. Any suggestion of a better location in that file?


Tested with the jtreg tests in serviceability/sa + patches to make ZGC 
usable with the SA's hprof dumping.


Thanks,
StefanK

Re: RFR: 8218733: SA: CollectedHeap provides broken implementation for used() and capacity()

[Stefan Karlsson]

Thanks, Erik.

Stefank

On 2019-02-14 09:38, Erik Österlund wrote:

Hi Stefan,

Looks good.

Thanks,
/Erik

On 2019-02-11 09:13, Stefan Karlsson wrote:

Hi all,

Please review this patch to remove the broken implementation of 
CollectedHeap used() and capacity() and instead force all GCs to 
provide their own implementations.


https://cr.openjdk.java.net/~stefank/8218733/webrev.01/
https://bugs.openjdk.java.net/browse/JDK-8218733

This was found while running 
serviceability/sa/TestHeapDumpForLargeArray.java on an experimental 
implementation of heap dumping in ZGC.


ZGC didn't provide a ZCollectedHeap.used() function and 
CollectedHeap.used() was used instead at:

 // Check weather we should dump the heap as segments
 useSegmentedHeapDump = vm.getUniverse().heap().used() > 
HPROF_SEGMENTED_HEAP_DUMP_THRESHOLD;


Because of this we incorrectly did not use segmented heap dumps, and 
therefore overflowed later in the code


Aleksey and Roman,

Could you verify that the implementation for Epsilon is correct? I 
also haven't implemented capacity for Shenandoah, as the information 
isn't trivially available in the ShenandoahHeap SA class. Do you want 
to fix it as part of this patch, or should I create a separate RFE for 
Shenandoah?


Thanks,
StefanK

Re: RFR: 8218743: SA: Add support for large bitmaps

[Stefan Karlsson]

Thanks, Erik.

Stefank

On 2019-02-14 10:11, Erik Österlund wrote:

Hi Stefan,

Looks good.

Thanks,
/Erik

On 2019-02-11 13:36, Stefan Karlsson wrote:

Hi all,

Please review this patch to add support for large bitmaps in the SA.

http://cr.openjdk.java.net/~stefank/8218743/webrev.01/
https://bugs.openjdk.java.net/browse/JDK-8218743

I've added a minimal interface (at, atPut, clear) that uses longs 
instead of ints, and changed MarkBits to use this interface.


I've implemented a segmented bitmap that, currently, all GCs use when 
asked for a bitmap in MarkBits. Later ZGC will provide its own 
discontiguous bitmap, implementing the new interface, but that will be 
sent out as a separate RFE.


I've tested this with a temporary patch that lowers the segment size 
and implements a jtreg test:

http://cr.openjdk.java.net/~stefank/8218743/webrev.test.01

I ran through the all the tests in serviceability/sa with both these 
patches.


Thanks,
StefanK

Re: RFR: 8218746: SA: Implement discontiguous bitmap for ZGC

[Erik Österlund]

Hi Stefan,

At ZExternalBitMap line 22:
* Maybe add a bounds check for offsets < 0 as well.

At ZExternalBitMap line 45:
* Remove weird whitespace in argument list.
* Consider inverting the condition to early exit. Don't mind if you 
choose to do that or not.


Otherwise looks good. Don't need another webrev.

Thanks,
/Erik

On 2019-02-11 14:55, Stefan Karlsson wrote:

Hi all,

Please review this patch to implement a discontiuous bitmap for ZGC in 
the SA.


http://cr.openjdk.java.net/~stefank/8218746/webrev.01/
https://bugs.openjdk.java.net/browse/JDK-8218746

ZGC uses a 16TB virtual memory address range, so the normal scheme to 
map a bitmap over the entire address range uses too much memory. This 
patch lazily creates smaller bitmaps per ZPage, when at() or atPut() is 
called.


This patch builds upon JDK-8218743.

Tested with serviceability/sa and manually running Reverse Object 
Analysis in the SA.


Thanks,
StefanK

Re: RFR: 8218746: SA: Implement discontiguous bitmap for ZGC

[Stefan Karlsson]

On 2019-02-14 10:22, Erik Österlund wrote:

Hi Stefan,

At ZExternalBitMap line 22:
* Maybe add a bounds check for offsets < 0 as well.

At ZExternalBitMap line 45:
* Remove weird whitespace in argument list.
* Consider inverting the condition to early exit. Don't mind if you 
choose to do that or not.


Otherwise looks good. Don't need another webrev.


I'll fix. Thanks for reviewing.

StefanK



Thanks,
/Erik

On 2019-02-11 14:55, Stefan Karlsson wrote:

Hi all,

Please review this patch to implement a discontiuous bitmap for ZGC in 
the SA.


http://cr.openjdk.java.net/~stefank/8218746/webrev.01/
https://bugs.openjdk.java.net/browse/JDK-8218746

ZGC uses a 16TB virtual memory address range, so the normal scheme to 
map a bitmap over the entire address range uses too much memory. This 
patch lazily creates smaller bitmaps per ZPage, when at() or atPut() 
is called.


This patch builds upon JDK-8218743.

Tested with serviceability/sa and manually running Reverse Object 
Analysis in the SA.


Thanks,
StefanK

Re: RFR/C: 8218922: SA: Enable best-effort implementation of live regions iteration for ZGC

[Erik Österlund]

Hi Stefan,

I think this makes things better. I like the cleanups and I like the 
partial support for heap parsing. I think we should bring this in, and 
it looks good to me.


Of course we can always improve this further at some point in the future 
to deal with broken Klass pointers better.


Thanks,
/Erik

On 2019-02-13 15:52, Stefan Karlsson wrote:

Hi all,

Please review / comment on this patch to enable a best-effort live heap 
region iteration implementation in ZGC.


http://cr.openjdk.java.net/~stefank/8218922/webrev.01/
https://bugs.openjdk.java.net/browse/JDK-8218922

The SA has functionally that relies on live heap region information from 
the GCs. This is problematic when dead objects are left in the heap, and 
their classes have been unloaded.


Because of this ZGC has so far not implemented this feature. However, we 
could provide a best-effort implementation that most likely will work if 
classes are not unloaded (or class unloading is turned off), and 
otherwise might fail to fully parse and report all live heap regions.


For active, non-relocating pages the patch simply returns [start, top) 
and for pages being actively relocated, it reports regions containing 
the non-forwarded objects, the other objects are either dead or could be 
found in one of the to-regions.


With this patch I'm able to get heap histograms with ZGC.

Maybe this is enough to enable a bit more SA debugging capabilities when 
running with ZGC? What do you think, should we bring in this change?


To be able to implement this more cleanly I've restructured the live 
region collection, and pushed GC specific code into the specific GCs. 
There are some extra usage of generics to make the code a bit easier to 
read and develop.


Thanks,
StefanK

Re: RFR/C: 8218922: SA: Enable best-effort implementation of live regions iteration for ZGC

[Stefan Karlsson]

Thanks for taking a look at this!

StefanK

On 2019-02-14 10:58, Erik Österlund wrote:

Hi Stefan,

I think this makes things better. I like the cleanups and I like the 
partial support for heap parsing. I think we should bring this in, and 
it looks good to me.


Of course we can always improve this further at some point in the future 
to deal with broken Klass pointers better.


Thanks,
/Erik

On 2019-02-13 15:52, Stefan Karlsson wrote:

Hi all,

Please review / comment on this patch to enable a best-effort live 
heap region iteration implementation in ZGC.


http://cr.openjdk.java.net/~stefank/8218922/webrev.01/
https://bugs.openjdk.java.net/browse/JDK-8218922

The SA has functionally that relies on live heap region information 
from the GCs. This is problematic when dead objects are left in the 
heap, and their classes have been unloaded.


Because of this ZGC has so far not implemented this feature. However, 
we could provide a best-effort implementation that most likely will 
work if classes are not unloaded (or class unloading is turned off), 
and otherwise might fail to fully parse and report all live heap regions.


For active, non-relocating pages the patch simply returns [start, top) 
and for pages being actively relocated, it reports regions containing 
the non-forwarded objects, the other objects are either dead or could 
be found in one of the to-regions.


With this patch I'm able to get heap histograms with ZGC.

Maybe this is enough to enable a bit more SA debugging capabilities 
when running with ZGC? What do you think, should we bring in this change?


To be able to implement this more cleanly I've restructured the live 
region collection, and pushed GC specific code into the specific GCs. 
There are some extra usage of generics to make the code a bit easier 
to read and develop.


Thanks,
StefanK

Re: RFR(S): 8218941: jdb should support a dbgtrace command that acts the same as the dbgtrace command line option

[gary.ad...@oracle.com]

Do you need placeholder entries in the translated TTYResources message 
files?


On 2/13/19 9:43 PM, Chris Plummer wrote:

Hi,

Please review the following:

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

Tested by running the following on all supported platforms:

open/test/hotspot/jtreg/vmTestbase/nsk/jdb
open/test/jdk/com/sun/jdi

thanks,

Chris

Re: RFR/C: 8218922: SA: Enable best-effort implementation of live regions iteration for ZGC

[Stefan Karlsson]

If this patch is accepted we also need this patch to turn on the SA 
hprof implementation:

http://cr.openjdk.java.net/~stefank/8218970/webrev.01/

StefanK

On 2019-02-13 15:52, Stefan Karlsson wrote:

Hi all,

Please review / comment on this patch to enable a best-effort live heap 
region iteration implementation in ZGC.


http://cr.openjdk.java.net/~stefank/8218922/webrev.01/
https://bugs.openjdk.java.net/browse/JDK-8218922

The SA has functionally that relies on live heap region information from 
the GCs. This is problematic when dead objects are left in the heap, and 
their classes have been unloaded.


Because of this ZGC has so far not implemented this feature. However, we 
could provide a best-effort implementation that most likely will work if 
classes are not unloaded (or class unloading is turned off), and 
otherwise might fail to fully parse and report all live heap regions.


For active, non-relocating pages the patch simply returns [start, top) 
and for pages being actively relocated, it reports regions containing 
the non-forwarded objects, the other objects are either dead or could be 
found in one of the to-regions.


With this patch I'm able to get heap histograms with ZGC.

Maybe this is enough to enable a bit more SA debugging capabilities when 
running with ZGC? What do you think, should we bring in this change?


To be able to implement this more cleanly I've restructured the live 
region collection, and pushed GC specific code into the specific GCs. 
There are some extra usage of generics to make the code a bit easier to 
read and develop.


Thanks,
StefanK

Re: RFR/C: 8218922: SA: Enable best-effort implementation of live regions iteration for ZGC

[Yasumasa Suenaga]

Hi Stefan,


Maybe this is enough to enable a bit more SA debugging capabilities when
running with ZGC? What do you think, should we bring in this change?


I think it should be brought this in.
I filed same issue as JDK-8207843, but I've not yet worked.
So I will close it as duplicate of your change.



To be able to implement this more cleanly I've restructured the live
region collection, and pushed GC specific code into the specific GCs.
There are some extra usage of generics to make the code a bit easier to
read and develop.


IMHO refactoring for live region collection and ZGC related changes should
be separated. What do you think?


Your change looks good to me.
BTW, did you check `jhsdb jmap --binaryheap` with this change?


Thanks,

Yasumasa



On 2019/02/13 23:52, Stefan Karlsson wrote:

Hi all,

Please review / comment on this patch to enable a best-effort live heap
region iteration implementation in ZGC.

http://cr.openjdk.java.net/~stefank/8218922/webrev.01/
https://bugs.openjdk.java.net/browse/JDK-8218922

The SA has functionally that relies on live heap region information from
the GCs. This is problematic when dead objects are left in the heap, and
their classes have been unloaded.

Because of this ZGC has so far not implemented this feature. However, we
could provide a best-effort implementation that most likely will work if
classes are not unloaded (or class unloading is turned off), and
otherwise might fail to fully parse and report all live heap regions.

For active, non-relocating pages the patch simply returns [start, top)
and for pages being actively relocated, it reports regions containing
the non-forwarded objects, the other objects are either dead or could be
found in one of the to-regions.

With this patch I'm able to get heap histograms with ZGC.

Maybe this is enough to enable a bit more SA debugging capabilities when
running with ZGC? What do you think, should we bring in this change?

To be able to implement this more cleanly I've restructured the live
region collection, and pushed GC specific code into the specific GCs.
There are some extra usage of generics to make the code a bit easier to
read and develop.

Thanks,
StefanK

Re: RFR/C: 8218922: SA: Enable best-effort implementation of live regions iteration for ZGC

[Stefan Karlsson]

Hi Yasumasa,

On 2019-02-14 14:11, Yasumasa Suenaga wrote:

Hi Stefan,


Maybe this is enough to enable a bit more SA debugging capabilities when
running with ZGC? What do you think, should we bring in this change?


I think it should be brought this in.
I filed same issue as JDK-8207843, but I've not yet worked.
So I will close it as duplicate of your change.



To be able to implement this more cleanly I've restructured the live
region collection, and pushed GC specific code into the specific GCs.
There are some extra usage of generics to make the code a bit easier to
read and develop.


IMHO refactoring for live region collection and ZGC related changes should
be separated. What do you think?


Yes. I think that would be good. I'll separate this out into two changes.




Your change looks good to me.
BTW, did you check `jhsdb jmap --binaryheap` with this change?



Yes, when testing this I ran all tests in serviceability/sa and manually 
tested the command above.


While testing this I also had this patch applied to enable SA hprof for ZGC:
http://cr.openjdk.java.net/~stefank/8218970/webrev.01/

And used this patch to turn off the ZGC filtering in the tests:
http://cr.openjdk.java.net/~stefank/8218978/webrev.01/

I'm currently rerunning the tests to see that the latest changes didn't 
break anything.


Thanks,
StefanK


Thanks,

Yasumasa



On 2019/02/13 23:52, Stefan Karlsson wrote:

Hi all,

Please review / comment on this patch to enable a best-effort live heap
region iteration implementation in ZGC.

http://cr.openjdk.java.net/~stefank/8218922/webrev.01/
https://bugs.openjdk.java.net/browse/JDK-8218922

The SA has functionally that relies on live heap region information from
the GCs. This is problematic when dead objects are left in the heap, and
their classes have been unloaded.

Because of this ZGC has so far not implemented this feature. However, we
could provide a best-effort implementation that most likely will work if
classes are not unloaded (or class unloading is turned off), and
otherwise might fail to fully parse and report all live heap regions.

For active, non-relocating pages the patch simply returns [start, top)
and for pages being actively relocated, it reports regions containing
the non-forwarded objects, the other objects are either dead or could be
found in one of the to-regions.

With this patch I'm able to get heap histograms with ZGC.

Maybe this is enough to enable a bit more SA debugging capabilities when
running with ZGC? What do you think, should we bring in this change?

To be able to implement this more cleanly I've restructured the live
region collection, and pushed GC specific code into the specific GCs.
There are some extra usage of generics to make the code a bit easier to
read and develop.

Thanks,
StefanK

Re: RFR(S): 8218941: jdb should support a dbgtrace command that acts the same as the dbgtrace command line option

[Chris Plummer]

Do you mean for the non-english locales? If I understood correctly what 
I was told a while back, we don't localize anymore, so it shouldn't be 
necessary.


Chris

On 2/14/19 2:11 AM, gary.ad...@oracle.com wrote:
Do you need placeholder entries in the translated TTYResources message 
files?


On 2/13/19 9:43 PM, Chris Plummer wrote:

Hi,

Please review the following:

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

Tested by running the following on all supported platforms:

open/test/hotspot/jtreg/vmTestbase/nsk/jdb
open/test/jdk/com/sun/jdi

thanks,

Chris

Re: RFR: 8218746: SA: Implement discontiguous bitmap for ZGC

[Jini George]

Hi Stefan,

Looks good to me. Nit: Could you please modify the copyright year? And 
don't we have to add the copyright message to the new file 
ZExternalBitMap.java ?


Thanks!
Jini.

On 2/11/2019 7:25 PM, Stefan Karlsson wrote:

Hi all,

Please review this patch to implement a discontiuous bitmap for ZGC in 
the SA.


http://cr.openjdk.java.net/~stefank/8218746/webrev.01/
https://bugs.openjdk.java.net/browse/JDK-8218746

ZGC uses a 16TB virtual memory address range, so the normal scheme to 
map a bitmap over the entire address range uses too much memory. This 
patch lazily creates smaller bitmaps per ZPage, when at() or atPut() is 
called.


This patch builds upon JDK-8218743.

Tested with serviceability/sa and manually running Reverse Object 
Analysis in the SA.


Thanks,
StefanK

RFR: JDK-8149461: jmap kills process if non-java pid is specified in the command line

[Gary Adams]

There is an old reported problem that using jmap on a process that is 
not running

Java could cause the process to terminate. This is due to the SIGQUIT used
when attaching to the process.

It is a fairly simple operation to validate that the pid matches one of 
the known

running Java processes using VirtualMachine.list().

  Issue: https://bugs.openjdk.java.net/browse/JDK-8149461

Proposed fix:

diff --git a/src/jdk.jcmd/share/classes/sun/tools/jmap/JMap.java 
b/src/jdk.jcmd/share/classes/sun/tools/jmap/JMap.java

--- a/src/jdk.jcmd/share/classes/sun/tools/jmap/JMap.java
+++ b/src/jdk.jcmd/share/classes/sun/tools/jmap/JMap.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2005, 2018, Oracle and/or its affiliates. All rights 
reserved.
+ * Copyright (c) 2005, 2019, Oracle and/or its affiliates. All rights 
reserved.

  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -30,6 +30,7 @@
 import java.io.InputStream;
 import java.io.UnsupportedEncodingException;
 import java.util.Collection;
+import java.util.List;

 import com.sun.tools.attach.VirtualMachine;
 import com.sun.tools.attach.VirtualMachineDescriptor;
@@ -125,6 +126,10 @@
 private static void executeCommandForPid(String pid, String 
command, Object ... args)

 throws AttachNotSupportedException, IOException,
UnsupportedEncodingException {
+// Before attaching, confirm that pid is a known Java process
+if (!checkJavaPid(pid)) {
+throw new AttachNotSupportedException();
+}
 VirtualMachine vm = VirtualMachine.attach(pid);

 // Cast to HotSpotVirtualMachine as this is an
@@ -196,6 +201,19 @@
 executeCommandForPid(pid, "dumpheap", filename, liveopt);
 }

+// Check that pid matches a known running Java process
+static boolean checkJavaPid(String pid) {
+List l = VirtualMachine.list();
+boolean found = false;
+for (VirtualMachineDescriptor vmd: l) {
+if (vmd.id().equals(pid)) {
+found = true;
+break;
+}
+}
+return found;
+}
+
 private static void checkForUnsupportedOptions(String[] args) {
 // Check arguments for -F, -m, and non-numeric value
 // and warn the user that SA is not supported anymore

Re: RFR: 8218746: SA: Implement discontiguous bitmap for ZGC

[Stefan Karlsson]

On 2019-02-14 17:04, Jini George wrote:

Hi Stefan,

Looks good to me. Nit: Could you please modify the copyright year? And 
don't we have to add the copyright message to the new file 
ZExternalBitMap.java ?


Thanks. I've gone through all my SA patches and updated the copyright year.

StefanK


Thanks!
Jini.

On 2/11/2019 7:25 PM, Stefan Karlsson wrote:

Hi all,

Please review this patch to implement a discontiuous bitmap for ZGC 
in the SA.


http://cr.openjdk.java.net/~stefank/8218746/webrev.01/
https://bugs.openjdk.java.net/browse/JDK-8218746

ZGC uses a 16TB virtual memory address range, so the normal scheme to 
map a bitmap over the entire address range uses too much memory. This 
patch lazily creates smaller bitmaps per ZPage, when at() or atPut() 
is called.


This patch builds upon JDK-8218743.

Tested with serviceability/sa and manually running Reverse Object 
Analysis in the SA.


Thanks,
StefanK

RFR: JDK-8219002: Some comments and error messages refer to VMDisconnectException

[Gary Adams]

Trivial cleanup of a misspelled exception :
  VMDisconnectException
  VMDisconnectedException

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

Re: RFR: JDK-8219002: Some comments and error messages refer to VMDisconnectException

[Chris Plummer]

Looks good.

Chris

On 2/14/19 9:00 AM, Gary Adams wrote:

Trivial cleanup of a misspelled exception :
  VMDisconnectException
  VMDisconnectedException

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

Re: RFR: JDK-8149461: jmap kills process if non-java pid is specified in the command line

[Chris Plummer]

Hi Gary,

What about the other tools that attach to a user specified process. Do 
they also have this issue?


thanks,

Chris

On 2/14/19 8:35 AM, Gary Adams wrote:
There is an old reported problem that using jmap on a process that is 
not running
Java could cause the process to terminate. This is due to the SIGQUIT 
used

when attaching to the process.

It is a fairly simple operation to validate that the pid matches one 
of the known

running Java processes using VirtualMachine.list().

  Issue: https://bugs.openjdk.java.net/browse/JDK-8149461

Proposed fix:

diff --git a/src/jdk.jcmd/share/classes/sun/tools/jmap/JMap.java 
b/src/jdk.jcmd/share/classes/sun/tools/jmap/JMap.java

--- a/src/jdk.jcmd/share/classes/sun/tools/jmap/JMap.java
+++ b/src/jdk.jcmd/share/classes/sun/tools/jmap/JMap.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2005, 2018, Oracle and/or its affiliates. All rights 
reserved.
+ * Copyright (c) 2005, 2019, Oracle and/or its affiliates. All rights 
reserved.

  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -30,6 +30,7 @@
 import java.io.InputStream;
 import java.io.UnsupportedEncodingException;
 import java.util.Collection;
+import java.util.List;

 import com.sun.tools.attach.VirtualMachine;
 import com.sun.tools.attach.VirtualMachineDescriptor;
@@ -125,6 +126,10 @@
 private static void executeCommandForPid(String pid, String 
command, Object ... args)

 throws AttachNotSupportedException, IOException,
    UnsupportedEncodingException {
+    // Before attaching, confirm that pid is a known Java process
+    if (!checkJavaPid(pid)) {
+    throw new AttachNotSupportedException();
+    }
 VirtualMachine vm = VirtualMachine.attach(pid);

 // Cast to HotSpotVirtualMachine as this is an
@@ -196,6 +201,19 @@
 executeCommandForPid(pid, "dumpheap", filename, liveopt);
 }

+    // Check that pid matches a known running Java process
+    static boolean checkJavaPid(String pid) {
+    List l = VirtualMachine.list();
+    boolean found = false;
+    for (VirtualMachineDescriptor vmd: l) {
+    if (vmd.id().equals(pid)) {
+    found = true;
+    break;
+    }
+    }
+    return found;
+    }
+
 private static void checkForUnsupportedOptions(String[] args) {
 // Check arguments for -F, -m, and non-numeric value
 // and warn the user that SA is not supported anymore

Re: RFR/C: 8218922: SA: Enable best-effort implementation of live regions iteration for ZGC

[Stefan Karlsson]

Hi again,

I've separated the live regions iteration refactoring into this patch:
https://cr.openjdk.java.net/~stefank/8219003/webrev.01/

And use this RFE for the ZGC specific parts:
https://cr.openjdk.java.net/~stefank/8218922/webrev.02/

Thanks,
StefanK

On 2019-02-14 14:39, Stefan Karlsson wrote:

Hi Yasumasa,

On 2019-02-14 14:11, Yasumasa Suenaga wrote:

Hi Stefan,

Maybe this is enough to enable a bit more SA debugging capabilities 
when

running with ZGC? What do you think, should we bring in this change?


I think it should be brought this in.
I filed same issue as JDK-8207843, but I've not yet worked.
So I will close it as duplicate of your change.



To be able to implement this more cleanly I've restructured the live
region collection, and pushed GC specific code into the specific GCs.
There are some extra usage of generics to make the code a bit easier to
read and develop.


IMHO refactoring for live region collection and ZGC related changes 
should

be separated. What do you think?


Yes. I think that would be good. I'll separate this out into two changes.




Your change looks good to me.
BTW, did you check `jhsdb jmap --binaryheap` with this change?



Yes, when testing this I ran all tests in serviceability/sa and 
manually tested the command above.


While testing this I also had this patch applied to enable SA hprof 
for ZGC:

http://cr.openjdk.java.net/~stefank/8218970/webrev.01/

And used this patch to turn off the ZGC filtering in the tests:
http://cr.openjdk.java.net/~stefank/8218978/webrev.01/

I'm currently rerunning the tests to see that the latest changes 
didn't break anything.


Thanks,
StefanK


Thanks,

Yasumasa



On 2019/02/13 23:52, Stefan Karlsson wrote:

Hi all,

Please review / comment on this patch to enable a best-effort live heap
region iteration implementation in ZGC.

http://cr.openjdk.java.net/~stefank/8218922/webrev.01/
https://bugs.openjdk.java.net/browse/JDK-8218922

The SA has functionally that relies on live heap region information 
from
the GCs. This is problematic when dead objects are left in the heap, 
and

their classes have been unloaded.

Because of this ZGC has so far not implemented this feature. 
However, we
could provide a best-effort implementation that most likely will 
work if

classes are not unloaded (or class unloading is turned off), and
otherwise might fail to fully parse and report all live heap regions.

For active, non-relocating pages the patch simply returns [start, top)
and for pages being actively relocated, it reports regions containing
the non-forwarded objects, the other objects are either dead or 
could be

found in one of the to-regions.

With this patch I'm able to get heap histograms with ZGC.

Maybe this is enough to enable a bit more SA debugging capabilities 
when

running with ZGC? What do you think, should we bring in this change?

To be able to implement this more cleanly I've restructured the live
region collection, and pushed GC specific code into the specific GCs.
There are some extra usage of generics to make the code a bit easier to
read and develop.

Thanks,
StefanK

Re: RFR: 8218734: SA: Incorrect and raw loads of OopHandles

[Jini George]

Hi Stefan,

Other than the getAddressField to getField change and the comments by 
Erik, I just have a couple of nits to add.


1. This line in VMOopHandle.java can be removed:

31 import sun.jvm.hotspot.runtime.VMObjectFactory;

2. The copyright year for some of the files need updation.

This looks good to me otherwise.

Thanks,
Jini.


On 2/11/2019 2:09 PM, Stefan Karlsson wrote:

Hi all,

Please review this patch to fix the resolving of oops inside the (VM) 
OopHandles.


https://cr.openjdk.java.net/~stefank/8218734/webrev.01/
https://bugs.openjdk.java.net/browse/JDK-8218734

Before this patch the OopHandle::_obj was assumed to be located at 
offset 0, and the SA resolved OopHandle (Klass::_java_mirror and 
ClassLoaderData::_class_loader) without the required load barrier for 
ZGC. I've added a new class VMOopHandle (The SA already has a 
OopHandle), which handles the resolving (load or load + barrier).


The OopHandle type is grouped under the Oops section in vmStructs.cpp. 
It's not an oop, so I added a newline to separate it out from the rest. 
Any suggestion of a better location in that file?


Tested with the jtreg tests in serviceability/sa + patches to make ZGC 
usable with the SA's hprof dumping.


Thanks,
StefanK

Re: RFR: JDK-8219002: Some comments and error messages refer to VMDisconnectException

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

+1

Thanks,
Serguei


On 2/14/19 09:07, Chris Plummer wrote:

Looks good.

Chris

On 2/14/19 9:00 AM, Gary Adams wrote:

Trivial cleanup of a misspelled exception :
  VMDisconnectException
  VMDisconnectedException

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

Re: RFR: 8218734: SA: Incorrect and raw loads of OopHandles

[Stefan Karlsson]

Hi Jini,

On 2019-02-14 18:11, Jini George wrote:

Hi Stefan,

Other than the getAddressField to getField change and the comments by 
Erik, I just have a couple of nits to add.


1. This line in VMOopHandle.java can be removed:

31 import sun.jvm.hotspot.runtime.VMObjectFactory;


Done.



2. The copyright year for some of the files need updation.


Sure.



This looks good to me otherwise.


Thanks for reviewing.

StefanK



Thanks,
Jini.


On 2/11/2019 2:09 PM, Stefan Karlsson wrote:

Hi all,

Please review this patch to fix the resolving of oops inside the (VM) 
OopHandles.


https://cr.openjdk.java.net/~stefank/8218734/webrev.01/
https://bugs.openjdk.java.net/browse/JDK-8218734

Before this patch the OopHandle::_obj was assumed to be located at 
offset 0, and the SA resolved OopHandle (Klass::_java_mirror and 
ClassLoaderData::_class_loader) without the required load barrier for 
ZGC. I've added a new class VMOopHandle (The SA already has a 
OopHandle), which handles the resolving (load or load + barrier).


The OopHandle type is grouped under the Oops section in 
vmStructs.cpp. It's not an oop, so I added a newline to separate it 
out from the rest. Any suggestion of a better location in that file?


Tested with the jtreg tests in serviceability/sa + patches to make 
ZGC usable with the SA's hprof dumping.


Thanks,
StefanK

Re: RFR(S): 8218941: jdb should support a dbgtrace command that acts the same as the dbgtrace command line option

[Alex Menkov]

+1

--alex

On 02/13/2019 23:07, serguei.spit...@oracle.com wrote:

Hi Chris,

It looks good to me.

Thanks,
Serguei


On 2/13/19 18:43, Chris Plummer wrote:

Hi,

Please review the following:

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

Tested by running the following on all supported platforms:

open/test/hotspot/jtreg/vmTestbase/nsk/jdb
open/test/jdk/com/sun/jdi

thanks,

Chris

Re: RFR(T) : 8209455 : [error-prone] JdkObsolete in jdk.management.agent

[Igor Ignatyev]

Thanks, JC!

Alan, does webrev.01 look good to you?

-- Igor

> On Feb 13, 2019, at 2:42 PM, Jean Christophe Beyler  
> wrote:
> 
> Hi Igor,
> 
> Looks good to me :)
> Jc
> 
> On Wed, Feb 13, 2019 at 11:24 AM Igor Ignatyev  > wrote:
> Hi Alan,
> 
> actually, String::join is enough in this case, uploaded webrev.01 -- 
> http://cr.openjdk.java.net/~iignatyev//8209455/webrev.01 
>  
> 
> Thanks,
> -- Igor
> 
>> On Feb 12, 2019, at 11:04 PM, Alan Bateman > > wrote:
>> 
>> On 13/02/2019 00:01, Igor Ignatyev wrote:
>>> http://cr.openjdk.java.net/~iignatyev//8209455/webrev.00 
>>> 
 2 lines changed: 0 ins; 0 del; 2 mod;
>>> 
>>> Hi all,
>>> 
>>> could you please review this trivial cleanup which replaces StringBuffer by 
>>> StringBuilder in jdk.management.agent module?
>>> 
>>> webrev: http://cr.openjdk.java.net/~iignatyev/8209455/webrev.00/ 
>>> 
>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8209455 
>>> 
>>> 
>> Looks okay but StringJoiner might be better here.
>> 
>> -Alan
> 
> 
> 
> -- 
> 
> Thanks,
> Jc

Re: RFR: JDK-8149461: jmap kills process if non-java pid is specified in the command line

[Gary Adams]

The following commands present a similar kill process behavior:
   jcmd
   jinfo
   jmap
   jstack

The following commands do not attach.
   jstat sun.jvmstat.monitor.MonitorException "not found"
   jps no pid arguments

This update moves the checkJavaPid method into the 
common/ProcessArgumentsMatcher.java

and applies the check before the pid is used for an attach operation.

  Revised webrev: http://cr.openjdk.java.net/~gadams/8149461/webrev.01/

On 2/14/19, 12:07 PM, Chris Plummer wrote:

Hi Gary,

What about the other tools that attach to a user specified process. Do 
they also have this issue?


thanks,

Chris

On 2/14/19 8:35 AM, Gary Adams wrote:
There is an old reported problem that using jmap on a process that is 
not running
Java could cause the process to terminate. This is due to the SIGQUIT 
used

when attaching to the process.

It is a fairly simple operation to validate that the pid matches one 
of the known

running Java processes using VirtualMachine.list().

  Issue: https://bugs.openjdk.java.net/browse/JDK-8149461

Proposed fix:

diff --git a/src/jdk.jcmd/share/classes/sun/tools/jmap/JMap.java 
b/src/jdk.jcmd/share/classes/sun/tools/jmap/JMap.java

--- a/src/jdk.jcmd/share/classes/sun/tools/jmap/JMap.java
+++ b/src/jdk.jcmd/share/classes/sun/tools/jmap/JMap.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2005, 2018, Oracle and/or its affiliates. All 
rights reserved.
+ * Copyright (c) 2005, 2019, Oracle and/or its affiliates. All 
rights reserved.

  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -30,6 +30,7 @@
 import java.io.InputStream;
 import java.io.UnsupportedEncodingException;
 import java.util.Collection;
+import java.util.List;

 import com.sun.tools.attach.VirtualMachine;
 import com.sun.tools.attach.VirtualMachineDescriptor;
@@ -125,6 +126,10 @@
 private static void executeCommandForPid(String pid, String 
command, Object ... args)

 throws AttachNotSupportedException, IOException,
UnsupportedEncodingException {
+// Before attaching, confirm that pid is a known Java process
+if (!checkJavaPid(pid)) {
+throw new AttachNotSupportedException();
+}
 VirtualMachine vm = VirtualMachine.attach(pid);

 // Cast to HotSpotVirtualMachine as this is an
@@ -196,6 +201,19 @@
 executeCommandForPid(pid, "dumpheap", filename, liveopt);
 }

+// Check that pid matches a known running Java process
+static boolean checkJavaPid(String pid) {
+List l = VirtualMachine.list();
+boolean found = false;
+for (VirtualMachineDescriptor vmd: l) {
+if (vmd.id().equals(pid)) {
+found = true;
+break;
+}
+}
+return found;
+}
+
 private static void checkForUnsupportedOptions(String[] args) {
 // Check arguments for -F, -m, and non-numeric value
 // and warn the user that SA is not supported anymore

Re: RFR: JDK-8149461: jmap kills process if non-java pid is specified in the command line

[Thomas Stüfe]

Hi Gary,

thanks for taking this on. I have occasionally killed a foreign process
with jcmd and it is a bit embarrassing :)

I think your patch is okay, but I wonder whether a check directly in
libattach would be simpler and cover all uses of the attach framework.

Such a test could be e.g. simply checking /proc//cmdline whether it is
a java launcher. Or even better - since this would not work with a custom
launcher - check /proc//maps for a mapping of a libjvm.so.

This could even be done in native coding, right before sending SIGQUIT.

Cheers, Thomas








On Thu, Feb 14, 2019 at 8:11 PM Gary Adams  wrote:

> The following commands present a similar kill process behavior:
> jcmd
> jinfo
> jmap
> jstack
>
> The following commands do not attach.
> jstat sun.jvmstat.monitor.MonitorException "not found"
> jps no pid arguments
>
> This update moves the checkJavaPid method into the
> common/ProcessArgumentsMatcher.java
> and applies the check before the pid is used for an attach operation.
>
>Revised webrev: http://cr.openjdk.java.net/~gadams/8149461/webrev.01/
>
> On 2/14/19, 12:07 PM, Chris Plummer wrote:
> > Hi Gary,
> >
> > What about the other tools that attach to a user specified process. Do
> > they also have this issue?
> >
> > thanks,
> >
> > Chris
> >
> > On 2/14/19 8:35 AM, Gary Adams wrote:
> >> There is an old reported problem that using jmap on a process that is
> >> not running
> >> Java could cause the process to terminate. This is due to the SIGQUIT
> >> used
> >> when attaching to the process.
> >>
> >> It is a fairly simple operation to validate that the pid matches one
> >> of the known
> >> running Java processes using VirtualMachine.list().
> >>
> >>   Issue: https://bugs.openjdk.java.net/browse/JDK-8149461
> >>
> >> Proposed fix:
> >>
> >> diff --git a/src/jdk.jcmd/share/classes/sun/tools/jmap/JMap.java
> >> b/src/jdk.jcmd/share/classes/sun/tools/jmap/JMap.java
> >> --- a/src/jdk.jcmd/share/classes/sun/tools/jmap/JMap.java
> >> +++ b/src/jdk.jcmd/share/classes/sun/tools/jmap/JMap.java
> >> @@ -1,5 +1,5 @@
> >>  /*
> >> - * Copyright (c) 2005, 2018, Oracle and/or its affiliates. All
> >> rights reserved.
> >> + * Copyright (c) 2005, 2019, Oracle and/or its affiliates. All
> >> rights reserved.
> >>   * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
> >>   *
> >>   * This code is free software; you can redistribute it and/or modify it
> >> @@ -30,6 +30,7 @@
> >>  import java.io.InputStream;
> >>  import java.io.UnsupportedEncodingException;
> >>  import java.util.Collection;
> >> +import java.util.List;
> >>
> >>  import com.sun.tools.attach.VirtualMachine;
> >>  import com.sun.tools.attach.VirtualMachineDescriptor;
> >> @@ -125,6 +126,10 @@
> >>  private static void executeCommandForPid(String pid, String
> >> command, Object ... args)
> >>  throws AttachNotSupportedException, IOException,
> >> UnsupportedEncodingException {
> >> +// Before attaching, confirm that pid is a known Java process
> >> +if (!checkJavaPid(pid)) {
> >> +throw new AttachNotSupportedException();
> >> +}
> >>  VirtualMachine vm = VirtualMachine.attach(pid);
> >>
> >>  // Cast to HotSpotVirtualMachine as this is an
> >> @@ -196,6 +201,19 @@
> >>  executeCommandForPid(pid, "dumpheap", filename, liveopt);
> >>  }
> >>
> >> +// Check that pid matches a known running Java process
> >> +static boolean checkJavaPid(String pid) {
> >> +List l = VirtualMachine.list();
> >> +boolean found = false;
> >> +for (VirtualMachineDescriptor vmd: l) {
> >> +if (vmd.id().equals(pid)) {
> >> +found = true;
> >> +break;
> >> +}
> >> +}
> >> +return found;
> >> +}
> >> +
> >>  private static void checkForUnsupportedOptions(String[] args) {
> >>  // Check arguments for -F, -m, and non-numeric value
> >>  // and warn the user that SA is not supported anymore
> >
> >
>
>

Re: RFR(T) : 8209455 : [error-prone] JdkObsolete in jdk.management.agent

[Alan Bateman]

On 14/02/2019 19:02, Igor Ignatyev wrote:

Thanks, JC!

Alan, does webrev.01 look good to you?


Yes, this looks much nicer.

-Alan

Re: RFR: JDK-8149461: jmap kills process if non-java pid is specified in the command line

[Chris Plummer]

Looks good to me. You could also take on Thomas' suggestion, but I'm not 
sure if there are any common use cases that it would be protecting us 
from accidentally killing a process (other than the ones you are already 
fixing).


Chris

On 2/14/19 11:12 AM, Gary Adams wrote:

The following commands present a similar kill process behavior:
   jcmd
   jinfo
   jmap
   jstack

The following commands do not attach.
   jstat sun.jvmstat.monitor.MonitorException "not found"
   jps no pid arguments

This update moves the checkJavaPid method into the 
common/ProcessArgumentsMatcher.java

and applies the check before the pid is used for an attach operation.

  Revised webrev: http://cr.openjdk.java.net/~gadams/8149461/webrev.01/

On 2/14/19, 12:07 PM, Chris Plummer wrote:

Hi Gary,

What about the other tools that attach to a user specified process. 
Do they also have this issue?


thanks,

Chris

On 2/14/19 8:35 AM, Gary Adams wrote:
There is an old reported problem that using jmap on a process that 
is not running
Java could cause the process to terminate. This is due to the 
SIGQUIT used

when attaching to the process.

It is a fairly simple operation to validate that the pid matches one 
of the known

running Java processes using VirtualMachine.list().

  Issue: https://bugs.openjdk.java.net/browse/JDK-8149461

Proposed fix:

diff --git a/src/jdk.jcmd/share/classes/sun/tools/jmap/JMap.java 
b/src/jdk.jcmd/share/classes/sun/tools/jmap/JMap.java

--- a/src/jdk.jcmd/share/classes/sun/tools/jmap/JMap.java
+++ b/src/jdk.jcmd/share/classes/sun/tools/jmap/JMap.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2005, 2018, Oracle and/or its affiliates. All 
rights reserved.
+ * Copyright (c) 2005, 2019, Oracle and/or its affiliates. All 
rights reserved.

  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or 
modify it

@@ -30,6 +30,7 @@
 import java.io.InputStream;
 import java.io.UnsupportedEncodingException;
 import java.util.Collection;
+import java.util.List;

 import com.sun.tools.attach.VirtualMachine;
 import com.sun.tools.attach.VirtualMachineDescriptor;
@@ -125,6 +126,10 @@
 private static void executeCommandForPid(String pid, String 
command, Object ... args)

 throws AttachNotSupportedException, IOException,
    UnsupportedEncodingException {
+    // Before attaching, confirm that pid is a known Java process
+    if (!checkJavaPid(pid)) {
+    throw new AttachNotSupportedException();
+    }
 VirtualMachine vm = VirtualMachine.attach(pid);

 // Cast to HotSpotVirtualMachine as this is an
@@ -196,6 +201,19 @@
 executeCommandForPid(pid, "dumpheap", filename, liveopt);
 }

+    // Check that pid matches a known running Java process
+    static boolean checkJavaPid(String pid) {
+    List l = VirtualMachine.list();
+    boolean found = false;
+    for (VirtualMachineDescriptor vmd: l) {
+    if (vmd.id().equals(pid)) {
+    found = true;
+    break;
+    }
+    }
+    return found;
+    }
+
 private static void checkForUnsupportedOptions(String[] args) {
 // Check arguments for -F, -m, and non-numeric value
 // and warn the user that SA is not supported anymore

Re: RFR/C: 8218922: SA: Enable best-effort implementation of live regions iteration for ZGC

[Yasumasa Suenaga]

Hi Stefan,

Both changes look good to me!


Thanks,

Yasumasa

2019年2月15日(金) 2:12 Stefan Karlsson :
>
> Hi again,
>
> I've separated the live regions iteration refactoring into this patch:
> https://cr.openjdk.java.net/~stefank/8219003/webrev.01/
>
> And use this RFE for the ZGC specific parts:
> https://cr.openjdk.java.net/~stefank/8218922/webrev.02/
>
> Thanks,
> StefanK
>
> On 2019-02-14 14:39, Stefan Karlsson wrote:
> > Hi Yasumasa,
> >
> > On 2019-02-14 14:11, Yasumasa Suenaga wrote:
> >> Hi Stefan,
> >>
> >>> Maybe this is enough to enable a bit more SA debugging capabilities
> >>> when
> >>> running with ZGC? What do you think, should we bring in this change?
> >>
> >> I think it should be brought this in.
> >> I filed same issue as JDK-8207843, but I've not yet worked.
> >> So I will close it as duplicate of your change.
> >>
> >>
> >>> To be able to implement this more cleanly I've restructured the live
> >>> region collection, and pushed GC specific code into the specific GCs.
> >>> There are some extra usage of generics to make the code a bit easier to
> >>> read and develop.
> >>
> >> IMHO refactoring for live region collection and ZGC related changes
> >> should
> >> be separated. What do you think?
> >
> > Yes. I think that would be good. I'll separate this out into two changes.
> >
> >>
> >>
> >> Your change looks good to me.
> >> BTW, did you check `jhsdb jmap --binaryheap` with this change?
> >>
> >
> > Yes, when testing this I ran all tests in serviceability/sa and
> > manually tested the command above.
> >
> > While testing this I also had this patch applied to enable SA hprof
> > for ZGC:
> > http://cr.openjdk.java.net/~stefank/8218970/webrev.01/
> >
> > And used this patch to turn off the ZGC filtering in the tests:
> > http://cr.openjdk.java.net/~stefank/8218978/webrev.01/
> >
> > I'm currently rerunning the tests to see that the latest changes
> > didn't break anything.
> >
> > Thanks,
> > StefanK
> >>
> >> Thanks,
> >>
> >> Yasumasa
> >>
> >>
> >>
> >> On 2019/02/13 23:52, Stefan Karlsson wrote:
> >>> Hi all,
> >>>
> >>> Please review / comment on this patch to enable a best-effort live heap
> >>> region iteration implementation in ZGC.
> >>>
> >>> http://cr.openjdk.java.net/~stefank/8218922/webrev.01/
> >>> https://bugs.openjdk.java.net/browse/JDK-8218922
> >>>
> >>> The SA has functionally that relies on live heap region information
> >>> from
> >>> the GCs. This is problematic when dead objects are left in the heap,
> >>> and
> >>> their classes have been unloaded.
> >>>
> >>> Because of this ZGC has so far not implemented this feature.
> >>> However, we
> >>> could provide a best-effort implementation that most likely will
> >>> work if
> >>> classes are not unloaded (or class unloading is turned off), and
> >>> otherwise might fail to fully parse and report all live heap regions.
> >>>
> >>> For active, non-relocating pages the patch simply returns [start, top)
> >>> and for pages being actively relocated, it reports regions containing
> >>> the non-forwarded objects, the other objects are either dead or
> >>> could be
> >>> found in one of the to-regions.
> >>>
> >>> With this patch I'm able to get heap histograms with ZGC.
> >>>
> >>> Maybe this is enough to enable a bit more SA debugging capabilities
> >>> when
> >>> running with ZGC? What do you think, should we bring in this change?
> >>>
> >>> To be able to implement this more cleanly I've restructured the live
> >>> region collection, and pushed GC specific code into the specific GCs.
> >>> There are some extra usage of generics to make the code a bit easier to
> >>> read and develop.
> >>>
> >>> Thanks,
> >>> StefanK
> >>>
>

Re: RFR: JDK-8149461: jmap kills process if non-java pid is specified in the command line

[David Holmes]

Gary,

What is the overhead of doing the validation? How do we list VMs? Do we 
need to examine every running process to get the list of VMs? Wouldn't 
it be better to check the given process is a VM rather than checking all 
potential VM processes?


I think there is an onus of responsibility on the user to provide a 
correct pid here, rather than trying to make this foolproof.


Thanks,
David

On 15/02/2019 5:12 am, Gary Adams wrote:

The following commands present a similar kill process behavior:
    jcmd
    jinfo
    jmap
    jstack

The following commands do not attach.
    jstat sun.jvmstat.monitor.MonitorException "not found"
    jps no pid arguments

This update moves the checkJavaPid method into the 
common/ProcessArgumentsMatcher.java

and applies the check before the pid is used for an attach operation.

   Revised webrev: http://cr.openjdk.java.net/~gadams/8149461/webrev.01/

On 2/14/19, 12:07 PM, Chris Plummer wrote:

Hi Gary,

What about the other tools that attach to a user specified process. Do 
they also have this issue?


thanks,

Chris

On 2/14/19 8:35 AM, Gary Adams wrote:
There is an old reported problem that using jmap on a process that is 
not running
Java could cause the process to terminate. This is due to the SIGQUIT 
used

when attaching to the process.

It is a fairly simple operation to validate that the pid matches one 
of the known

running Java processes using VirtualMachine.list().

  Issue: https://bugs.openjdk.java.net/browse/JDK-8149461

Proposed fix:

diff --git a/src/jdk.jcmd/share/classes/sun/tools/jmap/JMap.java 
b/src/jdk.jcmd/share/classes/sun/tools/jmap/JMap.java

--- a/src/jdk.jcmd/share/classes/sun/tools/jmap/JMap.java
+++ b/src/jdk.jcmd/share/classes/sun/tools/jmap/JMap.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2005, 2018, Oracle and/or its affiliates. All 
rights reserved.
+ * Copyright (c) 2005, 2019, Oracle and/or its affiliates. All 
rights reserved.

  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -30,6 +30,7 @@
 import java.io.InputStream;
 import java.io.UnsupportedEncodingException;
 import java.util.Collection;
+import java.util.List;

 import com.sun.tools.attach.VirtualMachine;
 import com.sun.tools.attach.VirtualMachineDescriptor;
@@ -125,6 +126,10 @@
 private static void executeCommandForPid(String pid, String 
command, Object ... args)

 throws AttachNotSupportedException, IOException,
    UnsupportedEncodingException {
+    // Before attaching, confirm that pid is a known Java process
+    if (!checkJavaPid(pid)) {
+    throw new AttachNotSupportedException();
+    }
 VirtualMachine vm = VirtualMachine.attach(pid);

 // Cast to HotSpotVirtualMachine as this is an
@@ -196,6 +201,19 @@
 executeCommandForPid(pid, "dumpheap", filename, liveopt);
 }

+    // Check that pid matches a known running Java process
+    static boolean checkJavaPid(String pid) {
+    List l = VirtualMachine.list();
+    boolean found = false;
+    for (VirtualMachineDescriptor vmd: l) {
+    if (vmd.id().equals(pid)) {
+    found = true;
+    break;
+    }
+    }
+    return found;
+    }
+
 private static void checkForUnsupportedOptions(String[] args) {
 // Check arguments for -F, -m, and non-numeric value
 // and warn the user that SA is not supported anymore

jstack mixed mode and jhsdb stack mixed mode can not get expected result(stack trace is incomplete) on latest ubuntu or fedora or RHEL 8 beta

[Shuai Gao]

Hello,

 

I tried “jstack -m PID”in latest openjdk 8 64-bit and “jhsdb jstack –mixed –pid 
PID” in latest openjdk 11 64-bit on latest Ubuntu(18.04) and latest fedora (29) 
and RHEL 8 Beta.

The stack traces returned are always incomplete like following output:

 

- 1479 -

"DestroyJavaVM" #17 prio=5 tid=0x00bae000 nid=0x5c7 waiting on 
condition [0x]

   java.lang.Thread.State: RUNNABLE

   JavaThread state: _thread_blocked

0x7f839224f9f3  __pthread_cond_wait + 0x243

- 1480 -

- 1481 -

"Reference Handler" #2 daemon prio=10 tid=0x00c36000 nid=0x5c9 waiting 
on condition [0x7f8376d15000]

   java.lang.Thread.State: RUNNABLE

   JavaThread state: _thread_blocked

0x7f839224f9f3  __pthread_cond_wait + 0x243

- 1482 -

"Finalizer" #3 daemon prio=8 tid=0x00c3a000 nid=0x5ca in Object.wait() 
[0x7f8376c14000]

   java.lang.Thread.State: WAITING (on object monitor)

   JavaThread state: _thread_blocked

0x7f839224f9f3  __pthread_cond_wait + 0x243

- 1483 -

"Signal Dispatcher" #4 daemon prio=9 tid=0x00c56800 nid=0x5cb runnable 
[0x]

   java.lang.Thread.State: RUNNABLE

   JavaThread state: _thread_blocked

0x7f83922526d6  do_futex_wait.constprop.1 + 0x36

 

The issue existed even after I disable the selinux. It can not be reproduced on 
oracle linux 7.4.

The kernel version on Ubuntu or fedora is 4.15. the kernel version on RHEL 8 
Beta is 4.18. the kernel version on Oracle linux 7.4 is 3.1.

I can not find any existed bug for now.

 

 

Is there any clue about this issue?

 

Any reply will be appreciated.

 

Best Regards

Gao Shuai from GCS