Re: RFR: 8219414: SA: jhsdb jsnap throws UnmappedAddressException with core generated by gcore

[Yasumasa Suenaga]

Hi Jini,

Thank you for your comment.

I uploaded new webrev.
Could you review again?

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

diff from webrev.00 is here:
  http://hg.openjdk.java.net/jdk/submit/rev/bb58c9d41381


Thanks,

Yasumasa


2019年2月21日(木) 12:30 Jini George :
>
> Your changes look good to me overall, Yasumasa. A couple of points though.
>
> * Since you are making modifications to have DumpPrivateMappingsInCore
> independent of CDS, in os::abort(), you would also need change the
> following lines:
> 1361 #if INCLUDE_CDS
> 1362 if (UseSharedSpaces && DumpPrivateMappingsInCore) {
> 1363   ClassLoader::close_jrt_image();
> 1364 }
> 1365 #endif
>
> to
>
> 1361
> 1362 if (DumpPrivateMappingsInCore) {
> 1363   ClassLoader::close_jrt_image();
> 1364 }
>
> But this might cause the zero build to fail due to the call to
> ClassLoader::close_jrt_image()
> (https://bugs.openjdk.java.net/browse/JDK-8215342). So, it might be
> better to guard the above lines with #ifndef ZERO to have:
>
> 1361 #ifndef ZERO
> 1362 if (DumpPrivateMappingsInCore) {
> 1363   ClassLoader::close_jrt_image();
> 1364 }
> 1365 #endif
>
>
> * Also, one nit:
>
> http://cr.openjdk.java.net/~ysuenaga/JDK-8219414/webrev.00/src/hotspot/os/linux/os_linux.cpp.html
>
> - Could you please remove or modify the comment at line 3436 since
> set_coredump_filter() is not restricted to include only largepages now?
>
> Thank you!
> Jini.
>
>
> On 2/21/2019 7:43 AM, Yasumasa Suenaga wrote:
> > Thanks Chris!
> >
> >
> > Yasumasa
> >
> >
> > 2019年2月21日(木) 11:08 Chris Plummer  > >:
> >
> > Ok. The changes look fine to me then.
> >
> > thanks,
> >
> > Chris
> >
> > On 2/20/19 6:02 PM, Yasumasa Suenaga wrote:
> >  > Hi Chris,
> >  >
> >  > I grep'ed with "MAP_SHARED" on jdk/src.
> >  > Shared memory is used in ZGC (ZBackingFile and
> > ZPhysicalMemoryBacking)
> >  > and in FileChannel::map at least.
> >  >
> >  > I think we have memory footprint concern about them, but shared
> > memory
> >  > should be dumped.
> >  > If we did not get them, we cannot analyze ZGC related behavior and
> >  > other code which uses shared memory.
> >  > (We can pass FD to os::reserve_memory - it will use shared memory if
> >  > FD is passed.)
> >  >
> >  > Thus I want to introduce `DumpSharedMappingsInCore` for dumping
> > shared
> >  > memory and set it to true by default.
> >  >
> >  >
> >  > Thanks,
> >  >
> >  > Yasumasa
> >  >
> >  >
> >  > 2019年2月21日(木) 3:10 Chris Plummer  > >:
> >  >> [adding runtime]
> >  >>
> >  >> Hi Yasumasa,
> >  >>
> >  >> Overall looks good. Just a couple of questions.
> >  >>
> >  >> Do we have the same footprint concerns with the shared mappings
> > as we
> >  >> did with the private mappings? If not, possibly it doesn't need an
> >  >> option and should always be enabled.
> >  >>
> >  >> thanks,
> >  >>
> >  >> Chris
> >  >>
> >  >> On 2/20/19 12:03 AM, Yasumasa Suenaga wrote:
> >  >>> Hi all,
> >  >>>
> >  >>> Please review this webrev:
> >  >>>
> >  >>> JBS: https://bugs.openjdk.java.net/browse/JDK-8219414
> >  >>> webrev:
> > http://cr.openjdk.java.net/~ysuenaga/JDK-8219414/webrev.00/
> >  >>>
> >  >>> I tried to get PerfCounter values via `jhsdb jsnap` from core image
> >  >>> which is generated by `gcore` (provided by GDB), but I encountered
> >  >>> UnmappedAddressException.
> >  >>>
> >  >>> It is caused by `generate-core-file` on GDB regards
> > `coredump_filter` on procfs.
> >  >>>
> >  >>> https://sourceware.org/gdb/onlinedocs/gdb/Core-File-Generation.html
> >  >>>
> >  >>> JDK-8200613 introduced `DumpPrivateMappingsInCore` for CDS. I
> > want to
> >  >>> introduce `DumpSharedMappingsInCore` for shared memory mapping.
> >  >>>
> >  >>> Currently `DumpPrivateMappingsInCore` affects when
> > `UseSharedSpaces` is enabled.
> >  >>> I want `DumpPrivateMappingsInCore` to affect independently in this
> >  >>> change because file-backed private memory which is not CDS might be
> >  >>> useful in the future.
> >  >>>
> >  >>>
> >  >>> Thanks,
> >  >>>
> >  >>> Yasumasa
> >  >>
> >
> >

Re: JDK-8219143: jdb should support breakpoint thread filters

[Chris Plummer]

Please review the updated webrev:

http://cr.openjdk.java.net/~cjplummer/8219143/webrev.01/

The syntax is now:

    stop [go|thread] [threadid ]  

I filed JDK-8219507 to cover BreakpointSpec.toString() improvements, and 
other possible improvements to the output when listing breakpoints.


I did a minor fix in TTYResources.java to address a bug from a recent 
JDK-8218941 commit that added the dbgtrace command. There was missing 
newline. I've added a note to JDK-8218941 indicating that it is being 
fixed here.


I've added a test. Below is a log of the debug session output that might 
help with reading the source of the test. The test creates 3 threads 
that all execute the same code, and verifies that the breakpoint set 
using the threadid option only stops on the one specified thread.


thanks,

Chris

[jdb] Set uncaught java.lang.Throwable
[jdb] Set deferred uncaught java.lang.Throwable
[jdb] Initializing jdb ...
[jdb]
[jdb] VM Started: > No frames on the current call stack
[jdb]
[jdb] main[1]
> stop in JdbStopThreadidTestTarg.brkMethod
[jdb] Deferring breakpoint JdbStopThreadidTestTarg.brkMethod.
[jdb] It will be set after the class is loaded.
[jdb] main[1]
> run
[jdb] > Set deferred breakpoint JdbStopThreadidTestTarg.brkMethod
[jdb]
[jdb] Breakpoint hit: "thread=main", 
JdbStopThreadidTestTarg.brkMethod(), line=80 bci=0

[jdb] 80    }
[jdb]
[jdb] main[1]
> threads
[jdb] Group system:
[jdb]   (java.lang.ref.Reference$ReferenceHandler)367 Reference Handler 
running
[jdb]   (java.lang.ref.Finalizer$FinalizerThread)368 Finalizer 
cond. waiting
[jdb]   (java.lang.Thread)369 Signal Dispatcher 
running

[jdb] Group main:
[jdb]   (java.lang.Thread)1 main  running (at breakpoint)
[jdb]   (JdbStopThreadidTestTarg$MyThread)482 MYTHREAD-1    waiting 
in a monitor
[jdb]   (JdbStopThreadidTestTarg$MyThread)483 MYTHREAD-2    waiting 
in a monitor
[jdb]   (JdbStopThreadidTestTarg$MyThread)484 MYTHREAD-3    waiting 
in a monitor

[jdb] Group InnocuousThreadGroup:
[jdb]   (jdk.internal.misc.InnocuousThread)416 Common-Cleaner    cond. 
waiting

[jdb] main[1]
> stop threadid 483 in JdbStopThreadidTestTarg$MyThread.brkMethod
[jdb] Set breakpoint JdbStopThreadidTestTarg$MyThread.brkMethod
[jdb] main[1]
> cont
[jdb] >
[jdb] Breakpoint hit: "thread=MYTHREAD-2", 
JdbStopThreadidTestTarg$MyThread.brkMethod(), line=101 bci=0

[jdb] 101    }
[jdb]
[jdb] MYTHREAD-2[1]
> cont
[jdb] >
[jdb] The application exited
[jdb]


On 2/20/19 2:23 PM, Chris Plummer wrote:

On 2/20/19 2:00 PM, Alex Menkov wrote:

Hi Chris,

- New threadid param breaks at/in clause - this doesn't look good.
Maybe it would be better to put threadid before in/at:
stop [go|thread] [threadid ]  

Ok. I'll try moving it.

This also would make commandStop/parseBreakpointSpec logic more clear:
commandStop handles go/thread/threadid, parseBreakpointSpec handles 
position (part starting from in/at);


- do you think BreakpointSpec toString should contain info about the 
thread (i.e. to specified in the breakpoint list)?;
I had added this at one point, alone with also adding the suspend 
policy. You then see both of these when you use "stop in/at" or 
"clear" without any arguments (it lists the breakpoints in this case). 
I was a little worried that the extra text might break something, so I 
left it out. Note also that currently the text given when you list 
breakpoints is exactly the text you need to enter when using the clear 
command, so that would no longer be true if I added any more text. 
However, I did envision a better version of this that not only include 
the suspend policy and threadid, but also a breakpoint index, allowing 
you to more easily clear one after listing it. Maybe I could add an 
RFE for this.


- it would be nice to add a test for the new threadid functionality.


I've been working on one today. It's near completion.

Chris



--alex

On 02/19/2019 21:57, Chris Plummer wrote:

Hello,

Please review the following:

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

Normally when a breakpoint is set in jdb, it is set globally (all 
threads). JDI supports the ability to have the breakpoint be 
automatically filtered so it will only be delivered on a specified 
thread and ignored on all other threads. This change allows making 
use of that JDI feature from jdb. So instead of something like:


   stop at Foo:23

You can now do:

   stop at threadid 7 Foo:23

Where 7 is the threadID for the thread as seen in the output of the 
"threads" command. The format of the stop command is now:


    stop [go|thread]  [threadid ] 

It is still fully backwards compatible.

As part of this change I also cleaned up the "stop" command parsing 
and error handling. It was kind of a mess, and was near impossible 
to add the "threadid" option until after I did much of the cleanup. 
I've also cleaned up the help output a lot, and added help for 

Re: RFR: 8219414: SA: jhsdb jsnap throws UnmappedAddressException with core generated by gcore

[Jini George]

Your changes look good to me overall, Yasumasa. A couple of points though.

* Since you are making modifications to have DumpPrivateMappingsInCore 
independent of CDS, in os::abort(), you would also need change the 
following lines:

1361 #if INCLUDE_CDS
1362 if (UseSharedSpaces && DumpPrivateMappingsInCore) {
1363   ClassLoader::close_jrt_image();
1364 }
1365 #endif

to

1361
1362 if (DumpPrivateMappingsInCore) {
1363   ClassLoader::close_jrt_image();
1364 }

But this might cause the zero build to fail due to the call to 
ClassLoader::close_jrt_image() 
(https://bugs.openjdk.java.net/browse/JDK-8215342). So, it might be 
better to guard the above lines with #ifndef ZERO to have:


1361 #ifndef ZERO
1362 if (DumpPrivateMappingsInCore) {
1363   ClassLoader::close_jrt_image();
1364 }
1365 #endif


* Also, one nit:

http://cr.openjdk.java.net/~ysuenaga/JDK-8219414/webrev.00/src/hotspot/os/linux/os_linux.cpp.html

- Could you please remove or modify the comment at line 3436 since 
set_coredump_filter() is not restricted to include only largepages now?


Thank you!
Jini.


On 2/21/2019 7:43 AM, Yasumasa Suenaga wrote:

Thanks Chris!


Yasumasa


2019年2月21日(木) 11:08 Chris Plummer >:


Ok. The changes look fine to me then.

thanks,

Chris

On 2/20/19 6:02 PM, Yasumasa Suenaga wrote:
 > Hi Chris,
 >
 > I grep'ed with "MAP_SHARED" on jdk/src.
 > Shared memory is used in ZGC (ZBackingFile and
ZPhysicalMemoryBacking)
 > and in FileChannel::map at least.
 >
 > I think we have memory footprint concern about them, but shared
memory
 > should be dumped.
 > If we did not get them, we cannot analyze ZGC related behavior and
 > other code which uses shared memory.
 > (We can pass FD to os::reserve_memory - it will use shared memory if
 > FD is passed.)
 >
 > Thus I want to introduce `DumpSharedMappingsInCore` for dumping
shared
 > memory and set it to true by default.
 >
 >
 > Thanks,
 >
 > Yasumasa
 >
 >
 > 2019年2月21日(木) 3:10 Chris Plummer mailto:chris.plum...@oracle.com>>:
 >> [adding runtime]
 >>
 >> Hi Yasumasa,
 >>
 >> Overall looks good. Just a couple of questions.
 >>
 >> Do we have the same footprint concerns with the shared mappings
as we
 >> did with the private mappings? If not, possibly it doesn't need an
 >> option and should always be enabled.
 >>
 >> thanks,
 >>
 >> Chris
 >>
 >> On 2/20/19 12:03 AM, Yasumasa Suenaga wrote:
 >>> Hi all,
 >>>
 >>> Please review this webrev:
 >>>
 >>>     JBS: https://bugs.openjdk.java.net/browse/JDK-8219414
 >>>     webrev:
http://cr.openjdk.java.net/~ysuenaga/JDK-8219414/webrev.00/
 >>>
 >>> I tried to get PerfCounter values via `jhsdb jsnap` from core image
 >>> which is generated by `gcore` (provided by GDB), but I encountered
 >>> UnmappedAddressException.
 >>>
 >>> It is caused by `generate-core-file` on GDB regards
`coredump_filter` on procfs.
 >>>
 >>> https://sourceware.org/gdb/onlinedocs/gdb/Core-File-Generation.html
 >>>
 >>> JDK-8200613 introduced `DumpPrivateMappingsInCore` for CDS. I
want to
 >>> introduce `DumpSharedMappingsInCore` for shared memory mapping.
 >>>
 >>> Currently `DumpPrivateMappingsInCore` affects when
`UseSharedSpaces` is enabled.
 >>> I want `DumpPrivateMappingsInCore` to affect independently in this
 >>> change because file-backed private memory which is not CDS might be
 >>> useful in the future.
 >>>
 >>>
 >>> Thanks,
 >>>
 >>> Yasumasa
 >>

Re: RFR: 8219414: SA: jhsdb jsnap throws UnmappedAddressException with core generated by gcore

[Yasumasa Suenaga]

Thanks Chris!


Yasumasa


2019年2月21日(木) 11:08 Chris Plummer :

> Ok. The changes look fine to me then.
>
> thanks,
>
> Chris
>
> On 2/20/19 6:02 PM, Yasumasa Suenaga wrote:
> > Hi Chris,
> >
> > I grep'ed with "MAP_SHARED" on jdk/src.
> > Shared memory is used in ZGC (ZBackingFile and ZPhysicalMemoryBacking)
> > and in FileChannel::map at least.
> >
> > I think we have memory footprint concern about them, but shared memory
> > should be dumped.
> > If we did not get them, we cannot analyze ZGC related behavior and
> > other code which uses shared memory.
> > (We can pass FD to os::reserve_memory - it will use shared memory if
> > FD is passed.)
> >
> > Thus I want to introduce `DumpSharedMappingsInCore` for dumping shared
> > memory and set it to true by default.
> >
> >
> > Thanks,
> >
> > Yasumasa
> >
> >
> > 2019年2月21日(木) 3:10 Chris Plummer :
> >> [adding runtime]
> >>
> >> Hi Yasumasa,
> >>
> >> Overall looks good. Just a couple of questions.
> >>
> >> Do we have the same footprint concerns with the shared mappings as we
> >> did with the private mappings? If not, possibly it doesn't need an
> >> option and should always be enabled.
> >>
> >> thanks,
> >>
> >> Chris
> >>
> >> On 2/20/19 12:03 AM, Yasumasa Suenaga wrote:
> >>> Hi all,
> >>>
> >>> Please review this webrev:
> >>>
> >>> JBS: https://bugs.openjdk.java.net/browse/JDK-8219414
> >>> webrev:
> http://cr.openjdk.java.net/~ysuenaga/JDK-8219414/webrev.00/
> >>>
> >>> I tried to get PerfCounter values via `jhsdb jsnap` from core image
> >>> which is generated by `gcore` (provided by GDB), but I encountered
> >>> UnmappedAddressException.
> >>>
> >>> It is caused by `generate-core-file` on GDB regards `coredump_filter`
> on procfs.
> >>>
> >>>
> https://sourceware.org/gdb/onlinedocs/gdb/Core-File-Generation.html
> >>>
> >>> JDK-8200613 introduced `DumpPrivateMappingsInCore` for CDS. I want to
> >>> introduce `DumpSharedMappingsInCore` for shared memory mapping.
> >>>
> >>> Currently `DumpPrivateMappingsInCore` affects when `UseSharedSpaces`
> is enabled.
> >>> I want `DumpPrivateMappingsInCore` to affect independently in this
> >>> change because file-backed private memory which is not CDS might be
> >>> useful in the future.
> >>>
> >>>
> >>> Thanks,
> >>>
> >>> Yasumasa
> >>
>
>
>

Re: RFR: 8219414: SA: jhsdb jsnap throws UnmappedAddressException with core generated by gcore

[Chris Plummer]

Ok. The changes look fine to me then.

thanks,

Chris

On 2/20/19 6:02 PM, Yasumasa Suenaga wrote:

Hi Chris,

I grep'ed with "MAP_SHARED" on jdk/src.
Shared memory is used in ZGC (ZBackingFile and ZPhysicalMemoryBacking)
and in FileChannel::map at least.

I think we have memory footprint concern about them, but shared memory
should be dumped.
If we did not get them, we cannot analyze ZGC related behavior and
other code which uses shared memory.
(We can pass FD to os::reserve_memory - it will use shared memory if
FD is passed.)

Thus I want to introduce `DumpSharedMappingsInCore` for dumping shared
memory and set it to true by default.


Thanks,

Yasumasa


2019年2月21日(木) 3:10 Chris Plummer :

[adding runtime]

Hi Yasumasa,

Overall looks good. Just a couple of questions.

Do we have the same footprint concerns with the shared mappings as we
did with the private mappings? If not, possibly it doesn't need an
option and should always be enabled.

thanks,

Chris

On 2/20/19 12:03 AM, Yasumasa Suenaga wrote:

Hi all,

Please review this webrev:

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

I tried to get PerfCounter values via `jhsdb jsnap` from core image
which is generated by `gcore` (provided by GDB), but I encountered
UnmappedAddressException.

It is caused by `generate-core-file` on GDB regards `coredump_filter` on procfs.

https://sourceware.org/gdb/onlinedocs/gdb/Core-File-Generation.html

JDK-8200613 introduced `DumpPrivateMappingsInCore` for CDS. I want to
introduce `DumpSharedMappingsInCore` for shared memory mapping.

Currently `DumpPrivateMappingsInCore` affects when `UseSharedSpaces` is enabled.
I want `DumpPrivateMappingsInCore` to affect independently in this
change because file-backed private memory which is not CDS might be
useful in the future.


Thanks,

Yasumasa

Re: RFR: 8219414: SA: jhsdb jsnap throws UnmappedAddressException with core generated by gcore

[Yasumasa Suenaga]

Hi Chris,

I grep'ed with "MAP_SHARED" on jdk/src.
Shared memory is used in ZGC (ZBackingFile and ZPhysicalMemoryBacking)
and in FileChannel::map at least.

I think we have memory footprint concern about them, but shared memory
should be dumped.
If we did not get them, we cannot analyze ZGC related behavior and
other code which uses shared memory.
(We can pass FD to os::reserve_memory - it will use shared memory if
FD is passed.)

Thus I want to introduce `DumpSharedMappingsInCore` for dumping shared
memory and set it to true by default.


Thanks,

Yasumasa


2019年2月21日(木) 3:10 Chris Plummer :
>
> [adding runtime]
>
> Hi Yasumasa,
>
> Overall looks good. Just a couple of questions.
>
> Do we have the same footprint concerns with the shared mappings as we
> did with the private mappings? If not, possibly it doesn't need an
> option and should always be enabled.
>
> thanks,
>
> Chris
>
> On 2/20/19 12:03 AM, Yasumasa Suenaga wrote:
> > Hi all,
> >
> > Please review this webrev:
> >
> >JBS: https://bugs.openjdk.java.net/browse/JDK-8219414
> >webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8219414/webrev.00/
> >
> > I tried to get PerfCounter values via `jhsdb jsnap` from core image
> > which is generated by `gcore` (provided by GDB), but I encountered
> > UnmappedAddressException.
> >
> > It is caused by `generate-core-file` on GDB regards `coredump_filter` on 
> > procfs.
> >
> >https://sourceware.org/gdb/onlinedocs/gdb/Core-File-Generation.html
> >
> > JDK-8200613 introduced `DumpPrivateMappingsInCore` for CDS. I want to
> > introduce `DumpSharedMappingsInCore` for shared memory mapping.
> >
> > Currently `DumpPrivateMappingsInCore` affects when `UseSharedSpaces` is 
> > enabled.
> > I want `DumpPrivateMappingsInCore` to affect independently in this
> > change because file-backed private memory which is not CDS might be
> > useful in the future.
> >
> >
> > Thanks,
> >
> > Yasumasa
>
>

Re: RFR (M) 8078725: method adjustments can be done just once for all classes involved into redefinition

[coleen . phillimore]

Serguei, Thank you for reviewing!

On 2/20/19 8:38 PM, serguei.spit...@oracle.com wrote:

Hi Coleen,

It looks great to me!
The overhead does not grow quadratically anymore with the number of  
redefined classes.

Also, several spots in code are simplified with this change.

One minor comment:

http://cr.openjdk.java.net/~coleenp/2019/8078725.01/webrev/src/hotspot/share/prims/jvmtiRedefineClasses.hpp.frames.html
518 // to fix up these pointers and clean MethodData out
   Dot is missed at the end of comment.



Fixed.



One question:

http://cr.openjdk.java.net/~coleenp/2019/8078725.01/webrev/src/hotspot/share/prims/jvmtiRedefineClasses.cpp.udiff.html
- // Fix the vtable embedded in the_class and subclasses of the_class,
- // if one exists. We discard scratch_class and we don't keep an
- // InstanceKlass around to hold obsolete methods so we don't have
- // any other InstanceKlass embedded vtables to update. The vtable
- // holds the Method*s for virtual (but not final) methods.
- // Default methods, or concrete methods in interfaces are stored
- // in the vtable, so if an interface changes we need to check
- // adjust_method_entries() for every InstanceKlass, which will also
- // adjust the default method vtable indices.
- // We also need to adjust any default method entries that are
- // not yet in the vtable, because the vtable setup is in progress.
- // This must be done after we adjust the default_methods and
- // default_vtable_indices for methods already in the vtable.
- // If redefining Unsafe, walk all the vtables looking for entries.
- if (ik->vtable_length() > 0 && (_the_class->is_interface()
- || _the_class == SystemDictionary::internal_Unsafe_klass()
- || ik->is_subtype_of(_the_class))) {
- // ik->vtable() creates a wrapper object; rm cleans it up
+ // Adjust all vtables, default methods and itables, to clean out old 
methods.

ResourceMark rm(_thread);
-
- ik->vtable().adjust_method_entries(the_class, &trace_name_printed);
- ik->adjust_default_methods(the_class, &trace_name_printed);
+ if (ik->vtable_length() > 0) {
+ ik->vtable().adjust_method_entries(&trace_name_printed);
+ ik->adjust_default_methods(&trace_name_printed);
  }
  
- // If the current class has an itable and we are either redefining an

- // interface or if the current class is a subclass of the_class, then
- // we potentially have to fix the itable. If we are redefining an
- // interface, then we have to call adjust_method_entries() for
- // every InstanceKlass that has an itable since there isn't a
- // subclass relationship between an interface and an InstanceKlass.
- // If redefining Unsafe, walk all the itables looking for entries.
- if (ik->itable_length() > 0 && (_the_class->is_interface()
- || _the_class == SystemDictionary::internal_Unsafe_klass()
- || ik->is_subclass_of(_the_class))) {
- ResourceMark rm(_thread);
- ik->itable().adjust_method_entries(the_class, &trace_name_printed);
+ if (ik->itable_length() > 0) {
+ ik->itable().adjust_method_entries(&trace_name_printed);
  }
  It is not clear what was the motivation to remove comments
  and a part of conditions above:
- && (_the_class->is_interface()
- || _the_class == SystemDictionary::internal_Unsafe_klass()
- || ik->is_subtype_of(_the_class))



The old code could skip adjusting entries for vtables and itables 
depending on the class that it was redefined, whereas the new code 
doesn't walk the CLDG per class.  There is no _the_class anymore. So the 
whole function is simplified to unconditionally go through the itables 
and vtables.  If we wanted to optimize further, we could add these 
conditions based on "are any of the classes redefined an interface, 
etc".  I didn't think it was worth the complication.


Most of the comments are explaining why we may or may not have to visit 
the metadata.  I didn't think scavenging bits of the comments would be 
particularly helpful to the reader.


Thanks,
Coleen


Thanks,
Serguei


On 2/20/19 07:10, coleen.phillim...@oracle.com wrote:
Summary: walk all classes at the end of redefinition and adjust 
method entries and clean MethodData


This improves performance for redefining multiple classes at once. 
See CR for more information.


Tested with redefinition tests in repository, and hs tier1-3.

make test TEST=open/test/hotspot/jtreg/vmTestbase/nsk/jvmti >&jvmti.out
make test TEST=open/test/hotspot/jtreg/vmTestbase/nsk/jdi >&jdi.out
make test TEST=open/test/hotspot/jtreg/runtime/RedefineTests 
>&redefine.out

make test TEST=open/test/hotspot/jtreg/runtime/logging >&logging.out
make test TEST=open/test/jdk/java/lang/instrument >&instrument.out
make test TEST=open/test/jdk/com/sun/jdi >&jtreg.jdi.out

open webrev at 
http://cr.openjdk.java.net/~coleenp/2019/8078725.01/webrev

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

Thanks,
Coleen

Re: RFR (M) 8078725: method adjustments can be done just once for all classes involved into redefinition

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

  
  
Hi Coleen,
  
  It looks great to me!
  The overhead does not grow quadratically anymore with the number
  of  redefined classes.
  Also, several spots in code are simplified with this change.
  
  One minor comment:
  
http://cr.openjdk.java.net/~coleenp/2019/8078725.01/webrev/src/hotspot/share/prims/jvmtiRedefineClasses.hpp.frames.html
   518   // to fix up these pointers and clean MethodData out
     Dot is missed at the end of comment.
  
  
  One question:
  
http://cr.openjdk.java.net/~coleenp/2019/8078725.01/webrev/src/hotspot/share/prims/jvmtiRedefineClasses.cpp.udiff.html
  -// Fix the vtable embedded in the_class and subclasses of the_class,
-// if one exists. We discard scratch_class and we don't keep an
-// InstanceKlass around to hold obsolete methods so we don't have
-// any other InstanceKlass embedded vtables to update. The vtable
-// holds the Method*s for virtual (but not final) methods.
-// Default methods, or concrete methods in interfaces are stored
-// in the vtable, so if an interface changes we need to check
-// adjust_method_entries() for every InstanceKlass, which will also
-// adjust the default method vtable indices.
-// We also need to adjust any default method entries that are
-// not yet in the vtable, because the vtable setup is in progress.
-// This must be done after we adjust the default_methods and
-// default_vtable_indices for methods already in the vtable.
-// If redefining Unsafe, walk all the vtables looking for entries.
-if (ik->vtable_length() > 0 && (_the_class->is_interface()
-|| _the_class == SystemDictionary::internal_Unsafe_klass()
-|| ik->is_subtype_of(_the_class))) {
-  // ik->vtable() creates a wrapper object; rm cleans it up
+// Adjust all vtables, default methods and itables, to clean out old methods.
   ResourceMark rm(_thread);
-
-  ik->vtable().adjust_method_entries(the_class, &trace_name_printed);
-  ik->adjust_default_methods(the_class, &trace_name_printed);
+if (ik->vtable_length() > 0) {
+  ik->vtable().adjust_method_entries(&trace_name_printed);
+  ik->adjust_default_methods(&trace_name_printed);
 }
 
-// If the current class has an itable and we are either redefining an
-// interface or if the current class is a subclass of the_class, then
-// we potentially have to fix the itable. If we are redefining an
-// interface, then we have to call adjust_method_entries() for
-// every InstanceKlass that has an itable since there isn't a
-// subclass relationship between an interface and an InstanceKlass.
-// If redefining Unsafe, walk all the itables looking for entries.
-if (ik->itable_length() > 0 && (_the_class->is_interface()
-|| _the_class == SystemDictionary::internal_Unsafe_klass()
-|| ik->is_subclass_of(_the_class))) {
-  ResourceMark rm(_thread);
-  ik->itable().adjust_method_entries(the_class, &trace_name_printed);
+if (ik->itable_length() > 0) {
+  ik->itable().adjust_method_entries(&trace_name_printed);
 }
    It is not clear what was the motivation to remove comments
    and a part of conditions above:
  -&& (_the_class->is_interface()
-|| _the_class == SystemDictionary::internal_Unsafe_klass()
-|| ik->is_subtype_of(_the_class))
  
  Thanks,
  Serguei
  
  
  On 2/20/19 07:10, coleen.phillim...@oracle.com wrote:

Summary:
  walk all classes at the end of redefinition and adjust method
  entries and clean MethodData
  
  
  This improves performance for redefining multiple classes at once.
  See CR for more information.
  
  
  Tested with redefinition tests in repository, and hs tier1-3.
  
  
  make test TEST=open/test/hotspot/jtreg/vmTestbase/nsk/jvmti
  >&jvmti.out
  
  make test TEST=open/test/hotspot/jtreg/vmTestbase/nsk/jdi
  >&jdi.out
  
  make test TEST=open/test/hotspot/jtreg/runtime/RedefineTests
  >&redefine.out
  
  make test TEST=open/test/hotspot/jtreg/runtime/logging
  >&logging.out
  
  make test TEST=open/test/jdk/java/lang/instrument
  >&instrument.out
  
  make test TEST=open/test/jdk/com/sun/jdi >&jtreg.jdi.out
  
  
  open webrev at
  http://cr.openjdk.java.net/~coleenp/2019/8078725.01/webrev
  
  bug link https://bugs.openjdk.java.net/browse/JDK-8078725
  
  
  Thanks,
  
  Coleen
  


  

Re: RFR(S) 8218751 Do not store original classfiles inside the CDS archive

[Claes Redestad]

Hi,

in my opinion it's way too easy to add flags compared to how hard they
are to get rid of, so I agree with postponing the addition of the flag
until it's requested based on a real need.

Thanks!

/Claes

On 2019-02-20 23:17, serguei.spit...@oracle.com wrote:

Hi Ioi,


Sorry that I missed that this discussion moved to the runtime-dev only.

In fact, I like the first webrev. It makes sense to safe on CDS footprint.
What about to postpone adding option EnableOptionalDataSharedSpace until 
real customers request it?

We need to understand real use cases first.

In general, I'm not convinced there is a real performance problem at 
startup

when CDS is used with native or java agents which enable CFLH events.
At least, it seems to be a rare use case, and it is not performance 
critical.


Thanks,
Serguei


On 2/18/19 22:58, Ioi Lam wrote:

Hi Jiangli,

I think you're right that apps that use CFLH but rarely redefine 
classes might see a slight speed up with the OD space. I don't know 
how prevalent such uses cases are, but I won't object to making the OD 
space optional.


So, I added a new flag to enable the OD space. Instead of the name you 
suggested, I used EnableOptionalDataSharedSpace so that it's related 
to other XXXSharedSpace flags. I also added a sanity test case, and 
fixed a Misplaced ResourceMark in klassFactory.cpp


http://cr.openjdk.java.net/~iklam/jdk13/8218751-dont-store-classfiles-in-cds.v02/ 

http://cr.openjdk.java.net/~iklam/jdk13/8218751-dont-store-classfiles-in-cds.v02-delta/ 



Please let me know what you think.

Thanks
- Ioi

On 2/18/19 8:12 PM, Jiangli Zhou wrote:


On Mon, Feb 18, 2019 at 11:33 AM Ioi Lam > wrote:




    On 2/15/19 7:11 PM, Jiangli Zhou wrote:

    On Fri, Feb 15, 2019 at 6:56 PM Ioi Lam mailto:ioi@oracle.com>> wrote:

    On 2/15/19 4:54 PM, Jiangli Zhou wrote:

    Hi,

    To answer your use case question, one of the case reported
    last year in OpenJDK was JRebel (please go back to the
    hotspot-dev mail list 2018 Oct. archive). That is an
    existing example as I've tried to point out in my earlier 
email.



    First of all, if you read the email carefully, CDS was not
    even in their usual test matrix. Also, if you're modifying
    classes on the fly, you're slowing down start-up big time. A
    few ms spent in decoding the classfile data will be
    completely drown out by the overhead of the classfile parsing
    and patching code in the JVMTI agent.

    So no, JRebel will not start up a few % faster if CDS stores
    the classfile data. It will most likely be not measurable.


    My understanding of the case is that their users generated a CDS
    archive and used it together with JRebel. That's why the issue
    was unnoticed until their users reported. There are use cases out
    there in the community that we simply are not aware of, so it
    would not be wise to assume there is no usage.


    Just to confirm one thing -- you do not dispute my claim that this
    "optimization" has no benefits even for those that actually use
    CDS and CFLH together. Correct?


That's actually the part that I have a different opinion. With the 
proposed change in the current webrev, the performance hit is across 
the board for loading shared classes regardless if there is a class 
being redefined/retransformed, as long as the 
can_generate_all_class_hook_events capability is enabled.



    We have plenty of nice-to-have harmless optimizations in HotSpot
    that probably weren't vigorously validated. However, this is not
    one of them.

    Here we have clear evidence of harm (50% footprint increase), with
    no evidence of benefit (theoretical or real-life). This code was
    checked into HotSpot without proper validation. That was wrong,
    and that's why I am taking it out now.

    It's easy to prove me wrong. Just supply a proper benchmark that
    shows benefits, and I will change my mind. That's the minimal
    standard for an optimization that has harmful side effects. I have
    created JDK-8219255 for tracking this.


Looks like there is a deadlock in the discussion. To summarize and 
help move this forward. Here are your reasons for dropping the 'od' 
space:


  * static footprint increase due to 'od' space
  * no benefit
  * maintenance of the code

Regarding static footprint, the change proposed in the current webrev 
is a short-term and temporary solution. For a long-term solution, the 
original class files can be removed from the final image created by 
jlink when jlink can work with CDS to produce a single runtime image.


On the benefit side, the 'od' space provides both startup speedup and 
runtime memory saving (when can_generate_all_class_hook_events 
capability is enabled), which I've already pointed those out in 
earlier emails.


For maintenance, it is also not an issue with the support from the 
community (I'll for sure to continue con

Re: JDK-8219143: jdb should support breakpoint thread filters

[Chris Plummer]

On 2/20/19 2:00 PM, Alex Menkov wrote:

Hi Chris,

- New threadid param breaks at/in clause - this doesn't look good.
Maybe it would be better to put threadid before in/at:
stop [go|thread] [threadid ]  

Ok. I'll try moving it.

This also would make commandStop/parseBreakpointSpec logic more clear:
commandStop handles go/thread/threadid, parseBreakpointSpec handles 
position (part starting from in/at);


- do you think BreakpointSpec toString should contain info about the 
thread (i.e. to specified in the breakpoint list)?;
I had added this at one point, alone with also adding the suspend 
policy. You then see both of these when you use "stop in/at" or "clear" 
without any arguments (it lists the breakpoints in this case). I was a 
little worried that the extra text might break something, so I left it 
out. Note also that currently the text given when you list breakpoints 
is exactly the text you need to enter when using the clear command, so 
that would no longer be true if I added any more text. However, I did 
envision a better version of this that not only include the suspend 
policy and threadid, but also a breakpoint index, allowing you to more 
easily clear one after listing it. Maybe I could add an RFE for this.


- it would be nice to add a test for the new threadid functionality.


I've been working on one today. It's near completion.

Chris



--alex

On 02/19/2019 21:57, Chris Plummer wrote:

Hello,

Please review the following:

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

Normally when a breakpoint is set in jdb, it is set globally (all 
threads). JDI supports the ability to have the breakpoint be 
automatically filtered so it will only be delivered on a specified 
thread and ignored on all other threads. This change allows making 
use of that JDI feature from jdb. So instead of something like:


   stop at Foo:23

You can now do:

   stop at threadid 7 Foo:23

Where 7 is the threadID for the thread as seen in the output of the 
"threads" command. The format of the stop command is now:


    stop [go|thread]  [threadid ] 

It is still fully backwards compatible.

As part of this change I also cleaned up the "stop" command parsing 
and error handling. It was kind of a mess, and was near impossible to 
add the "threadid" option until after I did much of the cleanup. I've 
also cleaned up the help output a lot, and added help for the "go" 
and "thread" options. One last change was to remove the distinction 
between "stop at" and "stop in", which was suggested already in a 
comment. They can be used interchangeably now. The changes in 
Commands.java are pretty much all just the above cleanup, except for 
the one short section with the 'Handle "threadid" modifier' comment.


thanks,

Chris

Re: RFR(S) 8218751 Do not store original classfiles inside the CDS archive

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

Hi Ioi,


Sorry that I missed that this discussion moved to the runtime-dev only.

In fact, I like the first webrev. It makes sense to safe on CDS footprint.
What about to postpone adding option EnableOptionalDataSharedSpace until 
real customers request it?

We need to understand real use cases first.

In general, I'm not convinced there is a real performance problem at startup
when CDS is used with native or java agents which enable CFLH events.
At least, it seems to be a rare use case, and it is not performance 
critical.


Thanks,
Serguei


On 2/18/19 22:58, Ioi Lam wrote:

Hi Jiangli,

I think you're right that apps that use CFLH but rarely redefine 
classes might see a slight speed up with the OD space. I don't know 
how prevalent such uses cases are, but I won't object to making the OD 
space optional.


So, I added a new flag to enable the OD space. Instead of the name you 
suggested, I used EnableOptionalDataSharedSpace so that it's related 
to other XXXSharedSpace flags. I also added a sanity test case, and 
fixed a Misplaced ResourceMark in klassFactory.cpp


http://cr.openjdk.java.net/~iklam/jdk13/8218751-dont-store-classfiles-in-cds.v02/ 

http://cr.openjdk.java.net/~iklam/jdk13/8218751-dont-store-classfiles-in-cds.v02-delta/ 



Please let me know what you think.

Thanks
- Ioi

On 2/18/19 8:12 PM, Jiangli Zhou wrote:


On Mon, Feb 18, 2019 at 11:33 AM Ioi Lam > wrote:




    On 2/15/19 7:11 PM, Jiangli Zhou wrote:

    On Fri, Feb 15, 2019 at 6:56 PM Ioi Lam mailto:ioi@oracle.com>> wrote:

    On 2/15/19 4:54 PM, Jiangli Zhou wrote:

    Hi,

    To answer your use case question, one of the case reported
    last year in OpenJDK was JRebel (please go back to the
    hotspot-dev mail list 2018 Oct. archive). That is an
    existing example as I've tried to point out in my earlier 
email.



    First of all, if you read the email carefully, CDS was not
    even in their usual test matrix. Also, if you're modifying
    classes on the fly, you're slowing down start-up big time. A
    few ms spent in decoding the classfile data will be
    completely drown out by the overhead of the classfile parsing
    and patching code in the JVMTI agent.

    So no, JRebel will not start up a few % faster if CDS stores
    the classfile data. It will most likely be not measurable.


    My understanding of the case is that their users generated a CDS
    archive and used it together with JRebel. That's why the issue
    was unnoticed until their users reported. There are use cases out
    there in the community that we simply are not aware of, so it
    would not be wise to assume there is no usage.


    Just to confirm one thing -- you do not dispute my claim that this
    "optimization" has no benefits even for those that actually use
    CDS and CFLH together. Correct?


That's actually the part that I have a different opinion. With the 
proposed change in the current webrev, the performance hit is across 
the board for loading shared classes regardless if there is a class 
being redefined/retransformed, as long as the 
can_generate_all_class_hook_events capability is enabled.



    We have plenty of nice-to-have harmless optimizations in HotSpot
    that probably weren't vigorously validated. However, this is not
    one of them.

    Here we have clear evidence of harm (50% footprint increase), with
    no evidence of benefit (theoretical or real-life). This code was
    checked into HotSpot without proper validation. That was wrong,
    and that's why I am taking it out now.

    It's easy to prove me wrong. Just supply a proper benchmark that
    shows benefits, and I will change my mind. That's the minimal
    standard for an optimization that has harmful side effects. I have
    created JDK-8219255 for tracking this.


Looks like there is a deadlock in the discussion. To summarize and 
help move this forward. Here are your reasons for dropping the 'od' 
space:


  * static footprint increase due to 'od' space
  * no benefit
  * maintenance of the code

Regarding static footprint, the change proposed in the current webrev 
is a short-term and temporary solution. For a long-term solution, the 
original class files can be removed from the final image created by 
jlink when jlink can work with CDS to produce a single runtime image.


On the benefit side, the 'od' space provides both startup speedup and 
runtime memory saving (when can_generate_all_class_hook_events 
capability is enabled), which I've already pointed those out in 
earlier emails.


For maintenance, it is also not an issue with the support from the 
community (I'll for sure to continue contributing to it). What's more 
important is to continue keeping CDS/AppCDS moving in the right 
direction by providing more performance & memory benefits and making 
it easier to use.


So I think providing a command-line option, 
-XX:EnableOptionalDataSpace that al

Re: RFR: JDK-4903717: nsk/jdi/ThreadReference/isSuspended/issuspended002 failing

[Alex Menkov]

+1

But could you please remove old "pipe.println("docontinue")" statement 
instead of commenting it out (no new webrev is required)


--alex

On 02/20/2019 09:58, Chris Plummer wrote:

Looks good.

Chris

On 2/20/19 7:57 AM, Gary Adams wrote:
The issuspended002 has been on the ProblemList since the tests were 
moved to
the open repos. The proposed changeset applies the same fix that was 
used in issuspended001.


The current test fails when the debuggee replies with the "docontinue" 
response,
while it is still holding a lock in a syncrhonized block. When the 
debugger then
suspends the vm the debuggee test thread fails to proceed to the 
expected breakpoint.

Testing in progress.

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

Re: JDK-8219143: jdb should support breakpoint thread filters

[Alex Menkov]

Hi Chris,

- New threadid param breaks at/in clause - this doesn't look good.
Maybe it would be better to put threadid before in/at:
stop [go|thread] [threadid ]  
This also would make commandStop/parseBreakpointSpec logic more clear:
commandStop handles go/thread/threadid, parseBreakpointSpec handles 
position (part starting from in/at);


- do you think BreakpointSpec toString should contain info about the 
thread (i.e. to specified in the breakpoint list)?;


- it would be nice to add a test for the new threadid functionality.

--alex

On 02/19/2019 21:57, Chris Plummer wrote:

Hello,

Please review the following:

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

Normally when a breakpoint is set in jdb, it is set globally (all 
threads). JDI supports the ability to have the breakpoint be 
automatically filtered so it will only be delivered on a specified 
thread and ignored on all other threads. This change allows making use 
of that JDI feature from jdb. So instead of something like:


   stop at Foo:23

You can now do:

   stop at threadid 7 Foo:23

Where 7 is the threadID for the thread as seen in the output of the 
"threads" command. The format of the stop command is now:


    stop [go|thread]  [threadid ] 

It is still fully backwards compatible.

As part of this change I also cleaned up the "stop" command parsing and 
error handling. It was kind of a mess, and was near impossible to add 
the "threadid" option until after I did much of the cleanup. I've also 
cleaned up the help output a lot, and added help for the "go" and 
"thread" options. One last change was to remove the distinction between 
"stop at" and "stop in", which was suggested already in a comment. They 
can be used interchangeably now. The changes in Commands.java are pretty 
much all just the above cleanup, except for the one short section with 
the 'Handle "threadid" modifier' comment.


thanks,

Chris

Re: JDK-8219388: Misleading log message "issuspended002a debuggee launched"

[Hohensee, Paul]

Quite a large number of files, indeed. Lgtm.

Paul

On 2/20/19, 9:36 AM, "serviceability-dev on behalf of Chris Plummer" 
 wrote:

I think because of the number of files changed it probably should not be 
considered trivial. For anyone interested in doing a review, just look 
at the patch file. It's pretty easy to skim through.

Chris

On 2/20/19 5:20 AM, Gary Adams wrote:
> Can I have a second reviewer or a confirmation that this is a trivial 
> change
> only needing one reviewer?
>
> On 2/19/19, 6:15 PM, Chris Plummer wrote:
>> Looks good.
>>
>> Chris
>>
>> On 2/19/19 1:52 PM, gary.ad...@oracle.com wrote:
>>> Sorry, my bad, used the wrong list of files when I made the webrev.
>>> Added the location, interrupt and setvalue debuggee references.
>>>
>>>   Webrev: http://cr.openjdk.java.net/~gadams/8219388/webrev.01/
>>>
>>> On 2/19/19 3:01 PM, Chris Plummer wrote:
 Hi Gary,

 On 2/19/19 11:38 AM, Gary Adams wrote:
> On my first pass I went after cleaning up the issuspend002a messages,
> because I'm investigating pulling it off the ProblemList.
>
> I added the setvalue003a misleading log messages in this updated 
> webrev.
 3 of the ones I pointed out below that were not setvalue003a.
> I think the raw "debugee launched" messages are in context of 
> other messages.
> I'd prefer to leave that level of clean to a future effort.
 Ok.
>
>   Webrev: http://cr.openjdk.java.net/~gadams/8219388/webrev.01/

 There's something wrong with your webrev. Look at the end of the 
 index page. Those files showing no diffs are all duplicates, and 
 also show up twice in the patch file. I also don't see the 
 setvalue003a changes in it.

 Chris

>
> On 2/19/19, 1:59 PM, Chris Plummer wrote:
>> Hi Gary,
>>
>> Changes look good, but there are other similar incorrect messages 
>> you might also want to address:
>>
>> ./StackFrame/thisObject/thisobject002.java:158: 
>> log2("setvalue003a debuggee launched");
>> ./StackFrame/thisObject/thisobject001.java:159: 
>> log2("setvalue003a debuggee launched");
>> ./StackFrame/visibleVariableByName/visiblevarbyname001.java:161: 
>> log2("setvalue003a debuggee launched");
>> ./StackFrame/visibleVariableByName/visiblevarbyname002.java:152: 
>> log2("setvalue003a debuggee launched");
>> ./StackFrame/thread/thread001.java:156: log2("location001a 
>> debuggee launched");
>> ./StackFrame/visibleVariables/visiblevariables001.java:156: 
>> log2("setvalue003a debuggee launched");
>> ./StackFrame/visibleVariables/visiblevariables002.java:153: 
>> log2("setvalue003a debuggee launched");
>> ./ThreadReference/interrupt/interrupt001.java:156: 
>> log2("interrupt002a debuggee launched");
>> ./LocalVariable/isVisible/isvisible001.java:162: 
>> log2("setvalue003a debuggee launched");
>> ./ClassType/invokeMethod/invokemethod001.java:162: 
>> log2("location001a debuggee launched");
>> ./Value/type/type002/type002.java:199: log2("setvalue003a 
>> debuggee launched");
>> ./VoidValue/equals/equals001/equals001.java:207: 
>> log2("setvalue003a debuggee launched");
>> ./VoidValue/hashCode/hashcode001/hashcode001.java:207: 
>> log2("setvalue003a debuggee launched");
>>
>> And I noticed a very large number of tests that only have:
>>
>> log2("debuggee launched");
>>
>> But there are so many that if you are to fix them, it should be 
>> done as a separate CR.
>>
>> Chris
>>
>> On 2/19/19 10:19 AM, Gary Adams wrote:
>>> A log message should have been parameterized
>>> with the debuggeeName.
>>>
>>>   Issue: https://bugs.openjdk.java.net/browse/JDK-8219388
>>>   Webrev: http://cr.openjdk.java.net/~gadams/8219388/webrev.00/
>>
>>
>>
>


>>>
>>
>>
>

Re: [RFR]8215622: Add dump to file support for jmap histo

[Hohensee, Paul]

Thanks, Serguei. Pushed.

On 2/19/19, 11:47 AM, "serguei.spit...@oracle.com" 
 wrote:

Hi Paul and Lin,

I'm Okay with the fix.

Thanks,
Serguei


On 2/18/19 2:44 PM, Hohensee, Paul wrote:
> Looks good. Imo "-histo:" should be valid because it's backward 
compatible. Serguei, if you concur I'll push the patch for Lin.
>
> Thanks,
>
> Paul
>
> On 2/17/19, 7:28 PM, "臧琳"  wrote:
>
>  Dear All,
>  Thanks a lot for your reviewing.
>  Here I refined the patch to fix a minor bug that cause 
BasicJMapTest fail with jmap -histo:all.
>  May I ask your help again to review it? 
http://cr.openjdk.java.net/~xiaofeya/8215622/webrev.09/
>  Thanks!
>  
>  
>  Cheers,
>  Lin
>  
>  From: serguei.spit...@oracle.com 
>  Sent: Wednesday, February 13, 2019 2:57:15 PM
>  To: 臧琳; Hohensee, Paul; Joe Darcy; Daniel Fuchs; David Holmes; JC 
Beyler
>  Cc: serviceability-dev@openjdk.java.net
>  Subject: Re: [RFR]8215622: Add dump to file support for jmap histo
>  
>  Hi Lin,
>  
>  The update looks good to me.
>  The only question I have if the option "-histo:" needs to be valid.
>  But I feel it is not that a big problem if it is valid.
>  Let's wait for other opinions.
>  
>  Thanks,
>  Serguei
>  
>  
>  On 2/12/19 21:26, 臧琳 wrote:
>  
>  Dear All,
>  I have uploaded new webrev at 
http://cr.openjdk.java.net/~xiaofeya/8215622/webrev.08/
>  Here are the changes I made:
>  * fixed the grammar issue.
>  * fixed the help info of jmap -histo
>  * added several unit test in BasicJmapTest.java
> Would you like to help review it? Thanks.
>  
>  
>  BRs,
>  Lin
>  
>  From: serguei.spit...@oracle.com 

>  Sent: Wednesday, February 13, 2019 4:00
>  To: Hohensee, Paul; Joe Darcy; Daniel Fuchs; 臧琳; David Holmes; JC 
Beyler
>  Cc: 
serviceability-dev@openjdk.java.net
>  Subject: Re: [RFR]8215622: Add dump to file support for jmap histo
>  
>  On 2/12/19 10:36 AM, Hohensee, Paul wrote:
>  
>  
>  Thanks, Joe and Daniel. Doesn't seem that we need it for this case 
anymore, but it's good to know how to proceed in the future. :)
>  
>  
>  
>  Agreed.
>  
>  Thanks,
>  Serguei
>  
>  
>  
>  
>  Paul
>  
>  On 2/12/19, 9:07 AM, "Joe Darcy" 
 wrote:
>  
>   Yes, if you make yourself the assignee, you can change the 
state of the
>   request (and then change the assignee back to the original 
person).
>  
>   HTH,
>  
>   -Joe
>  
>   On 2/12/2019 7:38 AM, Daniel Fuchs wrote:
>   > Hi Paul,
>   >
>   > On 12/02/2019 16:27, Hohensee, Paul wrote:
>   >> I don't see a way to change the Status from Closed to 
anything else.
>   >> There's no option I can find to do so.
>   >
>   > It may be that only the assignee can see this.
>   > On the closed CSRs assigned to me I can see a "Back to Draft" 
button
>   > next to "More".
>   >
>   > Hope this helps,
>   >
>   > -- daniel
>  
>  
>  
>  
>  
>  
>  
>  
>

JDK-8149460: jmap give unrelated error message if non java process id is specified in the command.

[Gary Adams]

I'd like to close this bug as "cannot reproduce".

The diagnostic output that suggested to use the "-F"
option was removed when the tmtools were decoupled
from the serviceability agent. See JDK-8155091 (May 2016).

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

Re: RFR: 8219414: SA: jhsdb jsnap throws UnmappedAddressException with core generated by gcore

[Chris Plummer]

[adding runtime]

Hi Yasumasa,

Overall looks good. Just a couple of questions.

Do we have the same footprint concerns with the shared mappings as we 
did with the private mappings? If not, possibly it doesn't need an 
option and should always be enabled.


thanks,

Chris

On 2/20/19 12:03 AM, Yasumasa Suenaga wrote:

Hi all,

Please review this webrev:

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

I tried to get PerfCounter values via `jhsdb jsnap` from core image
which is generated by `gcore` (provided by GDB), but I encountered
UnmappedAddressException.

It is caused by `generate-core-file` on GDB regards `coredump_filter` on procfs.

   https://sourceware.org/gdb/onlinedocs/gdb/Core-File-Generation.html

JDK-8200613 introduced `DumpPrivateMappingsInCore` for CDS. I want to
introduce `DumpSharedMappingsInCore` for shared memory mapping.

Currently `DumpPrivateMappingsInCore` affects when `UseSharedSpaces` is enabled.
I want `DumpPrivateMappingsInCore` to affect independently in this
change because file-backed private memory which is not CDS might be
useful in the future.


Thanks,

Yasumasa

Re: RFR: JDK-4903717: nsk/jdi/ThreadReference/isSuspended/issuspended002 failing

[Chris Plummer]

Looks good.

Chris

On 2/20/19 7:57 AM, Gary Adams wrote:
The issuspended002 has been on the ProblemList since the tests were 
moved to
the open repos. The proposed changeset applies the same fix that was 
used in issuspended001.


The current test fails when the debuggee replies with the "docontinue" 
response,
while it is still holding a lock in a syncrhonized block. When the 
debugger then
suspends the vm the debuggee test thread fails to proceed to the 
expected breakpoint.

Testing in progress.

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

Re: RFR: JDK-8219388: Misleading log message "issuspended002a debuggee launched"

[Chris Plummer]

I think because of the number of files changed it probably should not be 
considered trivial. For anyone interested in doing a review, just look 
at the patch file. It's pretty easy to skim through.


Chris

On 2/20/19 5:20 AM, Gary Adams wrote:
Can I have a second reviewer or a confirmation that this is a trivial 
change

only needing one reviewer?

On 2/19/19, 6:15 PM, Chris Plummer wrote:

Looks good.

Chris

On 2/19/19 1:52 PM, gary.ad...@oracle.com wrote:

Sorry, my bad, used the wrong list of files when I made the webrev.
Added the location, interrupt and setvalue debuggee references.

  Webrev: http://cr.openjdk.java.net/~gadams/8219388/webrev.01/

On 2/19/19 3:01 PM, Chris Plummer wrote:

Hi Gary,

On 2/19/19 11:38 AM, Gary Adams wrote:

On my first pass I went after cleaning up the issuspend002a messages,
because I'm investigating pulling it off the ProblemList.

I added the setvalue003a misleading log messages in this updated 
webrev.

3 of the ones I pointed out below that were not setvalue003a.
I think the raw "debugee launched" messages are in context of 
other messages.

I'd prefer to leave that level of clean to a future effort.

Ok.


  Webrev: http://cr.openjdk.java.net/~gadams/8219388/webrev.01/


There's something wrong with your webrev. Look at the end of the 
index page. Those files showing no diffs are all duplicates, and 
also show up twice in the patch file. I also don't see the 
setvalue003a changes in it.


Chris



On 2/19/19, 1:59 PM, Chris Plummer wrote:

Hi Gary,

Changes look good, but there are other similar incorrect messages 
you might also want to address:


./StackFrame/thisObject/thisobject002.java:158: 
log2("setvalue003a debuggee launched");
./StackFrame/thisObject/thisobject001.java:159: 
log2("setvalue003a debuggee launched");
./StackFrame/visibleVariableByName/visiblevarbyname001.java:161: 
log2("setvalue003a debuggee launched");
./StackFrame/visibleVariableByName/visiblevarbyname002.java:152: 
log2("setvalue003a debuggee launched");
./StackFrame/thread/thread001.java:156: log2("location001a 
debuggee launched");
./StackFrame/visibleVariables/visiblevariables001.java:156: 
log2("setvalue003a debuggee launched");
./StackFrame/visibleVariables/visiblevariables002.java:153: 
log2("setvalue003a debuggee launched");
./ThreadReference/interrupt/interrupt001.java:156: 
log2("interrupt002a debuggee launched");
./LocalVariable/isVisible/isvisible001.java:162: 
log2("setvalue003a debuggee launched");
./ClassType/invokeMethod/invokemethod001.java:162: 
log2("location001a debuggee launched");
./Value/type/type002/type002.java:199: log2("setvalue003a 
debuggee launched");
./VoidValue/equals/equals001/equals001.java:207: 
log2("setvalue003a debuggee launched");
./VoidValue/hashCode/hashcode001/hashcode001.java:207: 
log2("setvalue003a debuggee launched");


And I noticed a very large number of tests that only have:

    log2("debuggee launched");

But there are so many that if you are to fix them, it should be 
done as a separate CR.


Chris

On 2/19/19 10:19 AM, Gary Adams wrote:

A log message should have been parameterized
with the debuggeeName.

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

RFR: JDK-4903717: nsk/jdi/ThreadReference/isSuspended/issuspended002 failing

[Gary Adams]

The issuspended002 has been on the ProblemList since the tests were 
moved to
the open repos. The proposed changeset applies the same fix that was 
used in issuspended001.


The current test fails when the debuggee replies with the "docontinue" 
response,
while it is still holding a lock in a syncrhonized block. When the 
debugger then
suspends the vm the debuggee test thread fails to proceed to the 
expected breakpoint.

Testing in progress.

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

RFR (M) 8078725: method adjustments can be done just once for all classes involved into redefinition

[coleen . phillimore]

Summary: walk all classes at the end of redefinition and adjust method 
entries and clean MethodData


This improves performance for redefining multiple classes at once. See 
CR for more information.


Tested with redefinition tests in repository, and hs tier1-3.

make test TEST=open/test/hotspot/jtreg/vmTestbase/nsk/jvmti >&jvmti.out
make test TEST=open/test/hotspot/jtreg/vmTestbase/nsk/jdi >&jdi.out
make test TEST=open/test/hotspot/jtreg/runtime/RedefineTests >&redefine.out
make test TEST=open/test/hotspot/jtreg/runtime/logging >&logging.out
make test TEST=open/test/jdk/java/lang/instrument >&instrument.out
make test TEST=open/test/jdk/com/sun/jdi >&jtreg.jdi.out

open webrev at http://cr.openjdk.java.net/~coleenp/2019/8078725.01/webrev
bug link https://bugs.openjdk.java.net/browse/JDK-8078725

Thanks,
Coleen

Re: RFR: JDK-8219388: Misleading log message "issuspended002a debuggee launched"

[Gary Adams]

Can I have a second reviewer or a confirmation that this is a trivial change
only needing one reviewer?

On 2/19/19, 6:15 PM, Chris Plummer wrote:

Looks good.

Chris

On 2/19/19 1:52 PM, gary.ad...@oracle.com wrote:

Sorry, my bad, used the wrong list of files when I made the webrev.
Added the location, interrupt and setvalue debuggee references.

  Webrev: http://cr.openjdk.java.net/~gadams/8219388/webrev.01/

On 2/19/19 3:01 PM, Chris Plummer wrote:

Hi Gary,

On 2/19/19 11:38 AM, Gary Adams wrote:

On my first pass I went after cleaning up the issuspend002a messages,
because I'm investigating pulling it off the ProblemList.

I added the setvalue003a misleading log messages in this updated 
webrev.

3 of the ones I pointed out below that were not setvalue003a.
I think the raw "debugee launched" messages are in context of other 
messages.

I'd prefer to leave that level of clean to a future effort.

Ok.


  Webrev: http://cr.openjdk.java.net/~gadams/8219388/webrev.01/


There's something wrong with your webrev. Look at the end of the 
index page. Those files showing no diffs are all duplicates, and 
also show up twice in the patch file. I also don't see the 
setvalue003a changes in it.


Chris



On 2/19/19, 1:59 PM, Chris Plummer wrote:

Hi Gary,

Changes look good, but there are other similar incorrect messages 
you might also want to address:


./StackFrame/thisObject/thisobject002.java:158: log2("setvalue003a 
debuggee launched");
./StackFrame/thisObject/thisobject001.java:159: log2("setvalue003a 
debuggee launched");
./StackFrame/visibleVariableByName/visiblevarbyname001.java:161: 
log2("setvalue003a debuggee launched");
./StackFrame/visibleVariableByName/visiblevarbyname002.java:152: 
log2("setvalue003a debuggee launched");
./StackFrame/thread/thread001.java:156: log2("location001a 
debuggee launched");
./StackFrame/visibleVariables/visiblevariables001.java:156: 
log2("setvalue003a debuggee launched");
./StackFrame/visibleVariables/visiblevariables002.java:153: 
log2("setvalue003a debuggee launched");
./ThreadReference/interrupt/interrupt001.java:156: 
log2("interrupt002a debuggee launched");
./LocalVariable/isVisible/isvisible001.java:162: 
log2("setvalue003a debuggee launched");
./ClassType/invokeMethod/invokemethod001.java:162: 
log2("location001a debuggee launched");
./Value/type/type002/type002.java:199: log2("setvalue003a debuggee 
launched");
./VoidValue/equals/equals001/equals001.java:207: 
log2("setvalue003a debuggee launched");
./VoidValue/hashCode/hashcode001/hashcode001.java:207: 
log2("setvalue003a debuggee launched");


And I noticed a very large number of tests that only have:

log2("debuggee launched");

But there are so many that if you are to fix them, it should be 
done as a separate CR.


Chris

On 2/19/19 10:19 AM, Gary Adams wrote:

A log message should have been parameterized
with the debuggeeName.

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

RFR: 8219414: SA: jhsdb jsnap throws UnmappedAddressException with core generated by gcore

[Yasumasa Suenaga]

Hi all,

Please review this webrev:

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

I tried to get PerfCounter values via `jhsdb jsnap` from core image
which is generated by `gcore` (provided by GDB), but I encountered
UnmappedAddressException.

It is caused by `generate-core-file` on GDB regards `coredump_filter` on procfs.

  https://sourceware.org/gdb/onlinedocs/gdb/Core-File-Generation.html

JDK-8200613 introduced `DumpPrivateMappingsInCore` for CDS. I want to
introduce `DumpSharedMappingsInCore` for shared memory mapping.

Currently `DumpPrivateMappingsInCore` affects when `UseSharedSpaces` is enabled.
I want `DumpPrivateMappingsInCore` to affect independently in this
change because file-backed private memory which is not CDS might be
useful in the future.


Thanks,

Yasumasa