Re: RFR (S) 8249822: SymbolPropertyTable creates an extra OopHandle per entry

[David Holmes]

On 23/07/2020 6:55 am, coleen.phillim...@oracle.com wrote:
Summary: Add an assert to OopHandle assigment operator to catch leaking 
OopHandles, and fix code accordingly.


There are some jvmtiRedefineClasses.cpp changes here - basically, I 
moved the RedefineVerifyMark and comments into jvmtiRedefineClasses.cpp 
because I needed a Handle.


Ran tier1-6 tests and tier1 on all Oracle platforms.

open webrev at http://cr.openjdk.java.net/~coleenp/2020/8249822.01/webrev
bug link https://bugs.openjdk.java.net/browse/JDK-8249822


This all seems okay. A couple of minor nits

src/hotspot/share/oops/klass.cpp

+ void Klass::replace_java_mirror(oop mirror) { 
_java_mirror.replace(mirror); }


Writing this all in one line is inconsistent with surrounding style.

---

1226 //  a) A reference to the class being redefined (_the_class) and a
1230 //   b) The _java_mirror field from _the_class is copied to the

There's an extra space before b) on line 1230.

Thanks,
David
-



Thanks,
Coleen

Re: RFR (T) 8250042: Clean up methodOop and method_oop names from the code

[David Holmes]

Hi Coleen,

On 24/07/2020 2:58 am, coleen.phillim...@oracle.com wrote:
See bug for more details.  I've been running into these names a lot 
lately.   Many of these names are in JVMTI.


Tested with tier1 on all Oracle platforms and built on non-Oracle 
platforms.


open webrev at http://cr.openjdk.java.net/~coleenp/2020/8250042.01/webrev
bug link https://bugs.openjdk.java.net/browse/JDK-8250042


src/hotspot/cpu/*/*.ad

These still refer to "method oop" and method_oop in a number of places.

src/hotspot/share/adlc/adlparse.cpp

+  frame->_interpreter_method_oop_reg = parse_one_arg("method reg entry");

I guess I'm not understanding the scope of this renaming - why is 
_interpreter_method_oop_reg not renamed as well? Should this (and other 
uses) be parsed as method-(oop-reg) rather than (method-oop)-reg?


Otherwise all okay.

Thanks,
David


Thanks,
Coleen

Re: RFR: 8248362: JVMTI frame operations should use Thread-Local Handshake

[Yasumasa Suenaga]

Thanks Serguei!

Yasumasa


On 2020/07/24 4:31, serguei.spit...@oracle.com wrote:

Hi Yasumasa,

The update looks good.

Thanks,
Serguei


On 7/22/20 20:03, Yasumasa Suenaga wrote:

Hi Serguei,

Thanks for your comment!
I fixed it in new webrev:

  http://cr.openjdk.java.net/~ysuenaga/JDK-8248362/webrev.01/
  Diff from previous webrev: 
http://hg.openjdk.java.net/jdk/submit/rev/df75038b5449


Yasumasa


On 2020/07/23 9:31, serguei.spit...@oracle.com wrote:

Hi Yasumasa,

Looks good.
Just one minor comment.

http://cr.openjdk.java.net/~ysuenaga/JDK-8248362/webrev.00/src/hotspot/share/prims/jvmtiEnv.cpp.udiff.html

  // JVMTI get java stack frame location at safepoint.

Replace: "at safepoint" => "with handshake".

Thanks,
Serguei


On 7/22/20 07:38, Yasumasa Suenaga wrote:

Hi all,

Please review this change:

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

Migrate JVMTI frame operations to Thread-Local Handshakes from VM Operations.

    - VM_GetFrameCount
    - VM_GetFrameLocation

They affects both GetFrameCount() and GetFrameLocation() JVMTI functions.

This change has passed all tests on submit repo 
(mach5-one-ysuenaga-JDK-8248362-20200722-1249-12859056), and 
vmTestbase/nsk/{jdi,jdw,jvmti}, serviceability/{jdwp,jvmti} .


Thanks,

Yasumasa

Re: RFR (T) 8250042: Clean up methodOop and method_oop names from the code

[Chris Plummer]

On 7/23/20 4:17 PM, coleen.phillim...@oracle.com wrote:



On 7/23/20 6:29 PM, Chris Plummer wrote:

Hi Coleen,

The JVMTI changes look fine, although I wonder why in jvmtiEnter.xsl 
you did the rename to "checked_method" instead of just "method". Was 
there a conflict in some cases?


Yes it was quite painful.  The jvmti.xml spec has jmethodID parameters 
to be named "method" and I didn't want to change that.


    
  
    
  The method in which to set the breakpoint
    

I picked checked_method because you get it by calling this below and 
the alternative "m" I think was used somewhere else (or not 
descriptive at all).


+    Method* checked_method = 
Method::checked_resolve_jmethod_id(


Ah, so this is the case of having to deal with a jmethodID and a Method* 
in the same scope. I think this is fine, although it would be nice if 
Serguei also gave his blessing.


thanks,

Chris

Thanks,
Coleen


thanks,

Chris

On 7/23/20 9:58 AM, coleen.phillim...@oracle.com wrote:
See bug for more details.  I've been running into these names a lot 
lately.   Many of these names are in JVMTI.


Tested with tier1 on all Oracle platforms and built on non-Oracle 
platforms.


open webrev at 
http://cr.openjdk.java.net/~coleenp/2020/8250042.01/webrev

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

Thanks,
Coleen

Re: RFR (T) 8250042: Clean up methodOop and method_oop names from the code

[coleen . phillimore]

On 7/23/20 6:29 PM, Chris Plummer wrote:

Hi Coleen,

The JVMTI changes look fine, although I wonder why in jvmtiEnter.xsl 
you did the rename to "checked_method" instead of just "method". Was 
there a conflict in some cases?


Yes it was quite painful.  The jvmti.xml spec has jmethodID parameters 
to be named "method" and I didn't want to change that.


    
  
    
  The method in which to set the breakpoint
    

I picked checked_method because you get it by calling this below and the 
alternative "m" I think was used somewhere else (or not descriptive at all).


+    Method* checked_method = 
Method::checked_resolve_jmethod_id(


Thanks,
Coleen


thanks,

Chris

On 7/23/20 9:58 AM, coleen.phillim...@oracle.com wrote:
See bug for more details.  I've been running into these names a lot 
lately.   Many of these names are in JVMTI.


Tested with tier1 on all Oracle platforms and built on non-Oracle 
platforms.


open webrev at 
http://cr.openjdk.java.net/~coleenp/2020/8250042.01/webrev

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

Thanks,
Coleen

Re: RFR (T) 8250042: Clean up methodOop and method_oop names from the code

[Chris Plummer]

Hi Coleen,

The JVMTI changes look fine, although I wonder why in jvmtiEnter.xsl you 
did the rename to "checked_method" instead of just "method". Was there a 
conflict in some cases?


thanks,

Chris

On 7/23/20 9:58 AM, coleen.phillim...@oracle.com wrote:
See bug for more details.  I've been running into these names a lot 
lately.   Many of these names are in JVMTI.


Tested with tier1 on all Oracle platforms and built on non-Oracle 
platforms.


open webrev at http://cr.openjdk.java.net/~coleenp/2020/8250042.01/webrev
bug link https://bugs.openjdk.java.net/browse/JDK-8250042

Thanks,
Coleen

Re: RFR(S) 8249293: Unsafe stackwalk in VM_GetOrSetLocal::doit_prologue()

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

Hi Richard,

Thank you for filing the CR and taking care about it!
The fix itself looks good to me.
I still need another look at new test.
Could you, please, convert the agent of new test to C++?
It will make it a little bit simpler.

Thanks,
Serguei


On 7/20/20 01:15, Reingruber, Richard wrote:

Hi,

please help review this fix for VM_GetOrSetLocal. It moves the unsafe stackwalk 
from the vm
operation prologue before the safepoint into the doit() method executed at the 
safepoint.

Webrev: http://cr.openjdk.java.net/~rrich/webrevs/8249293/webrev.0/index.html
Bug:https://bugs.openjdk.java.net/browse/JDK-8249293

According to the JVMTI spec on local variable access it is not required to 
suspend the target thread
T [1]. The operation will simply fail with JVMTI_ERROR_NO_MORE_FRAMES if T is 
executing
bytecodes. It will succeed though if T is blocked because of synchronization or 
executing some native
code.

The issue is that in the latter case the stack walk in 
VM_GetOrSetLocal::doit_prologue() to prepare
the access to the local variable is unsafe, because it is done before the 
safepoint and it races
with T returning to execute bytecodes making its stack not walkable. The 
included test shows that
this can crash the VM if T wins the race.

Manual testing:

   - new test 
test/hotspot/jtreg/serviceability/jvmti/GetLocalVariable/GetLocalWithoutSuspendTest.java
   - test/hotspot/jtreg/vmTestbase/nsk/jvmti
   - test/hotspot/jtreg/serviceability/jvmti

Nightly regression tests @SAP: JCK and JTREG, also in Xcomp mode, SPECjvm2008, 
SPECjbb2015,
Renaissance Suite, SAP specific tests with fastdebug and release builds on all 
platforms

Thanks, Richard.

[1] https://docs.oracle.com/en/java/javase/14/docs/specs/jvmti.html#local

Re: RFR(M): 8247515: OSX pc_to_symbol() lookup does not work with core files

[Chris Plummer]

  
  
Just a minor update of some new
  findings (no new code change). The DB hash table being used by
  default will overwrite an existing entry, not duplicate it, and
  this is indeed what was happening. The second entry added was the
  one with a 0 offset. When I enable the R_NOOVERWRITE flag, it
  stops the overwrite and that also fixes the problem, but that's
  only because the entry with offset 0 comes last. The fix I've done
  is better since it avoids the offet 0 entry altogether, so even if
  it came first it would not get used.
  
  thanks,
  
  Chris
  
  On 7/22/20 10:25 PM, Chris Plummer wrote:


  
  Hi Kevin,

Thanks for the review. Unfortunately there was yet another bug
which I have now addressed. Although testing with mach5 went
fine, when I tried with a local build I had issues. SA couldn't
even get past an early part of the core file handling which
involves doing some adjustments related to CDS. It needs to look
up a couple of hotspot symbols by name to do this, and get their
values (such as _SharedBaseAddress). Although the symbol ->
address lookup seemed to work, the values retrieved from the
address were garbage. After some debugging I noticed the 3
symbols being looked up all had the same address. Then I noticed
this address was at offset 0 of the libjvm segment. After a lot
more debugging I found the problem. These symbols were actually
in the symbol table twice, once with a proper offset and once
with an offset of 0. I'm not sure why the ones with an offset of
0 were there (other than they originated in the mach-o symbol
table).

The reason this didn't always happen is because SA takes all the
symbols it finds and adds them to a hash table for fast symbol
-> address lookup. If a symbol is in there twice, it's a
tossup which you'll get. It could change from build to build in
fact. The trigger for my local build was probably how I ran
configure, which likely is not the same as mach5, although I'm 
unsure if this just gave me the unlucky hashing, or if in fact
it resulted in the entries with offset 0. In any case, the fix
is to ignore entries with offset 0. Here's the updated webrev:

http://cr.openjdk.java.net/~cjplummer/8247515/webrev.03/index.html

All the changes since webrev.02 are in build_symtab() in
symtab.c. Besides ignoring entries with offset 0 to fix the bug,
I also did some cleanup. There used to be two loops to iterate
over the symbols. There wasn't really a good reason for this, so
now there is just one. Also, it no longer adds entries with a
file offset 0, an offset into the string section of 0, or an
empty string. By doing this the size of the libjvm symbol table
went from about 240k entries to 90k. Since it was originally
allocated at it's full potential size, it's now reallocate to
the smaller size after symbol table processing is over.

thanks,

Chris

On 7/22/20 2:45 AM, Kevin Walls wrote:
  
  

Thanks Chris, yes looks good, I like that we check the
  library bounds before calling nearest_symbol.
--
  Kevin


On 21/07/2020 21:05, Chris Plummer
  wrote:


  
  Hi Serguei and Kevin,

The webrev has been updated:

http://cr.openjdk.java.net/~cjplummer/8247515/webrev.02/index.html
https://bugs.openjdk.java.net/browse/JDK-8247515

Two issues were addressed:

(1) Code in symbol_for_pc() assumed the caller had first
checked to make sure that the symbol is in a library,
where-as some callers assume NULL will be returned if the
symbol is not in a library. This is the case for pstack for
example, which first blindly does a pc to symbol lookup, and
only if that returns null does it then check if the pc is in
the codecache or interpreter. The logic in symbol_for_pc()
assumed that if the pc was greater than the start address of
the last library in the list, then it must be in that
library. So in stack traces the frames for compiled or
interpreted pc's showed up as the last symbol in the last
library, plus some very large offset. The fix is to now
track the size of libraries so we can do a proper range
check.

(2) There are issues with finding system libraries. See [1]
JDK-8249779. Because of this I disabled support for trying

Re: RFR: JDK-8249550: jdb should use loopback address when not using remote agent

[Chris Plummer]

Hi Alex,

I'm no expert in this area, but the changes appear to do what you 
describe (use the loopback address), so thumbs up.


thanks,

Chris

On 7/21/20 3:04 PM, Alex Menkov wrote:

Hi all,

please review the fix for
https://bugs.openjdk.java.net/browse/JDK-8249550

webrev:
http://cr.openjdk.java.net/~amenkov/jdk16/jdb_loopback/webrev/

some background:
https://bugs.openjdk.java.net/browse/JDK-8041435 made default 
listening on loopback address.
Later https://bugs.openjdk.java.net/browse/JDK-8184770 added handling 
of "*" address to listen on all addresses, but it didn't fixed 
"default" startListening() method (used by jdb through 
SunCommandLineLauncher).


The method called startListening(String localaddress, int port) with 
localaddress == null, but this method for null localladdress starts 
listening on all addresses (i.e. handle null value as "*").
The fix changes it to startListening(String address) which handles 
null address the same way as JDI socket connector does (i.e. listens 
on loopback address only)


--alex

Re: RFR: 8216324: GetClassMethods is confused by the presence of default methods in super interfaces

[Alex Menkov]

+1

--alex

On 07/23/2020 12:28, serguei.spit...@oracle.com wrote:

Hi Daniil,

The update looks good to me.

Thanks,
Serguei


On 7/23/20 11:32, Daniil Titov wrote:

Hi Serguei and Alex,

Please review a new version of the webrev [1]  that includes the 
changes Serguei suggested.  This version also has new test renamed 
from DefaultMethods.java to OverpassMethods.java to avoid warnings 
during the build about conflict with native library for 
runtime/jni/8033445/DefaultMethods.java  test.


[1] http://cr.openjdk.java.net/~dtitov/8216324/webrev.04/

Thank you,
Daniil

From: "serguei.spit...@oracle.com" 
Date: Tuesday, July 21, 2020 at 10:53 PM
To: Daniil Titov , Alex Menkov 
, serviceability-dev 

Subject: Re: RFR: 8216324: GetClassMethods is confused by the presence 
of default methods in super interfaces


Hi Daniil,

Thank you for the update!
It looks good to me.

Just two more minor comments and no need for another webrev you 
address them.

2519   int skipped = 0;  // skip default methods
Saying "overpass methods" would be more precise.
   47 printf("Enabled capability: maintain_original_method_order: 
true\n");
It is better to get rid of ": true" at the end (sorry, I missed this 
in my previous suggestion).

Enabling capability means it is set to true.


Thanks,
Serguei


On 7/21/20 20:25, Daniil Titov wrote:
Hi Serguei and Alex,

Please, review new version of the change [1]  that includes changes as 
Serguei suggested.


114 default void default_method() { } // should never be seen
The comment above is not clear. Should never be seen in what context?
This method should not be included in the list of methods 
GetClassMethods returns for the sub-interface or implementing class.  
I don't think we want to comment each method in the test interfaces 
declared in this test when they should be seen in this test and when 
they should not. This information already exists in getclmthd007.cpp, 
thus I decided to omit this comment.


Please see below the output from the new test.


--messages:(4/215)--
command: main -agentlib:DefaultMethods DefaultMethods
reason: User specified action: run main/othervm/native 
-agentlib:DefaultMethods DefaultMethods

Mode: othervm [/othervm specified]
elapsed time (seconds): 0.241
--configuration:(0/0)--
--System.out:(3/265)--
Reflection getDeclaredMethods returned: [public abstract 
java.lang.String DefaultMethods$Child.method2()]
JVMTI GetClassMethods returned: [public abstract java.lang.String 
DefaultMethods$Child.method2()]

Test passed: Got expected output from JVMTI GetClassMethods!
--System.err:(1/15)--
STATUS:Passed.



--messages:(4/276)--
command: main -agentlib:DefaultMethods=maintain_original_method_order 
DefaultMethods
reason: User specified action: run main/othervm/native 
-agentlib:DefaultMethods=maintain_original_method_order DefaultMethods

Mode: othervm [/othervm specified]
elapsed time (seconds): 0.25
--configuration:(0/0)--
--System.out:(4/322)--
Enabled capability: maintain_original_method_order: true
Reflection getDeclaredMethods returned: [public abstract 
java.lang.String DefaultMethods$Child.method2()]
JVMTI GetClassMethods returned: [public abstract java.lang.String 
DefaultMethods$Child.method2()]

Test passed: Got expected output from JVMTI GetClassMethods!
--System.err:(1/15)--
STATUS:Passed.


[1] http://cr.openjdk.java.net/~dtitov/8216324/webrev.02/

Thanks,
Daniil

From: mailto:serguei.spit...@oracle.com mailto:serguei.spit...@oracle.com
Date: Monday, July 20, 2020 at 6:48 PM
To: Alex Menkov mailto:alexey.men...@oracle.com, Daniil Titov 
mailto:daniil.x.ti...@oracle.com, serviceability-dev 
mailto:serviceability-dev@openjdk.java.net
Subject: Re: RFR: 8216324: GetClassMethods is confused by the presence 
of default methods in super interfaces


Hi Daniil,

The fix looks pretty good to me.

Just minor comments.


http://cr.openjdk.java.net/~dtitov/8216324/webrev.01/src/hotspot/share/prims/jvmtiEnv.cpp.frames.html 

2519   int skipped = 0;  // skip default methods from superinterface 
(see JDK-8216324)


You can say just:  // skip overpass methods
There is probably no need to list the bug number.

2523 // Depending on can_maintain_original_method_order capability
2524 // use the original method ordering indices stored in the 
class, so we can emit

2525 // jmethodIDs in the order they appeared in the class file
2526 // or just copy in any order
Could you, please re-balance the comment a little bit?
Also, the comment has to be terminated with a dot.
Replace: "or just copy in any order" => "or just copy in current order".


http://cr.openjdk.java.net/~dtitov/8216324/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/jvmti/GetClassMethods/getclmthd007.java.frames.html 


  114 default void default_method() { } // should never be seen
The comment above is not clear. Should never be seen in what context?

Re: RFR: 8248362: JVMTI frame operations should use Thread-Local Handshake

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

Hi Yasumasa,

The update looks good.

Thanks,
Serguei


On 7/22/20 20:03, Yasumasa Suenaga wrote:

Hi Serguei,

Thanks for your comment!
I fixed it in new webrev:

  http://cr.openjdk.java.net/~ysuenaga/JDK-8248362/webrev.01/
  Diff from previous webrev: 
http://hg.openjdk.java.net/jdk/submit/rev/df75038b5449



Yasumasa


On 2020/07/23 9:31, serguei.spit...@oracle.com wrote:

Hi Yasumasa,

Looks good.
Just one minor comment.

http://cr.openjdk.java.net/~ysuenaga/JDK-8248362/webrev.00/src/hotspot/share/prims/jvmtiEnv.cpp.udiff.html 



  // JVMTI get java stack frame location at safepoint.

Replace: "at safepoint" => "with handshake".

Thanks,
Serguei


On 7/22/20 07:38, Yasumasa Suenaga wrote:

Hi all,

Please review this change:

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

Migrate JVMTI frame operations to Thread-Local Handshakes from VM 
Operations.


    - VM_GetFrameCount
    - VM_GetFrameLocation

They affects both GetFrameCount() and GetFrameLocation() JVMTI 
functions.


This change has passed all tests on submit repo 
(mach5-one-ysuenaga-JDK-8248362-20200722-1249-12859056), and 
vmTestbase/nsk/{jdi,jdw,jvmti}, serviceability/{jdwp,jvmti} .



Thanks,

Yasumasa

Re: RFR: 8216324: GetClassMethods is confused by the presence of default methods in super interfaces

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

Hi Daniil,

The update looks good to me.

Thanks,
Serguei


On 7/23/20 11:32, Daniil Titov wrote:

Hi Serguei and Alex,

Please review a new version of the webrev [1]  that includes the changes 
Serguei suggested.  This version also has new test renamed from 
DefaultMethods.java to OverpassMethods.java to avoid warnings during the build 
about conflict with native library for runtime/jni/8033445/DefaultMethods.java  
test.

[1] http://cr.openjdk.java.net/~dtitov/8216324/webrev.04/

Thank you,
Daniil

From: "serguei.spit...@oracle.com" 
Date: Tuesday, July 21, 2020 at 10:53 PM
To: Daniil Titov , Alex Menkov , 
serviceability-dev 
Subject: Re: RFR: 8216324: GetClassMethods is confused by the presence of 
default methods in super interfaces

Hi Daniil,

Thank you for the update!
It looks good to me.

Just two more minor comments and no need for another webrev you address them.
2519   int skipped = 0;  // skip default methods
Saying "overpass methods" would be more precise.
   47 printf("Enabled capability: maintain_original_method_order: true\n");
It is better to get rid of ": true" at the end (sorry, I missed this in my 
previous suggestion).
Enabling capability means it is set to true.


Thanks,
Serguei


On 7/21/20 20:25, Daniil Titov wrote:
Hi Serguei and Alex,

Please, review new version of the change [1]  that includes changes as Serguei 
suggested.

114 default void default_method() { } // should never be seen
The comment above is not clear. Should never be seen in what context?
This method should not be included in the list of methods GetClassMethods 
returns for the sub-interface or implementing class.  I don't think we want to 
comment each method in the test interfaces declared in this test when they 
should be seen in this test and when they should not. This information already 
exists in getclmthd007.cpp, thus I decided to omit this comment.

Please see below the output from the new test.


--messages:(4/215)--
command: main -agentlib:DefaultMethods DefaultMethods
reason: User specified action: run main/othervm/native -agentlib:DefaultMethods 
DefaultMethods
Mode: othervm [/othervm specified]
elapsed time (seconds): 0.241
--configuration:(0/0)--
--System.out:(3/265)--
Reflection getDeclaredMethods returned: [public abstract java.lang.String 
DefaultMethods$Child.method2()]
JVMTI GetClassMethods returned: [public abstract java.lang.String 
DefaultMethods$Child.method2()]
Test passed: Got expected output from JVMTI GetClassMethods!
--System.err:(1/15)--
STATUS:Passed.



--messages:(4/276)--
command: main -agentlib:DefaultMethods=maintain_original_method_order 
DefaultMethods
reason: User specified action: run main/othervm/native 
-agentlib:DefaultMethods=maintain_original_method_order DefaultMethods
Mode: othervm [/othervm specified]
elapsed time (seconds): 0.25
--configuration:(0/0)--
--System.out:(4/322)--
Enabled capability: maintain_original_method_order: true
Reflection getDeclaredMethods returned: [public abstract java.lang.String 
DefaultMethods$Child.method2()]
JVMTI GetClassMethods returned: [public abstract java.lang.String 
DefaultMethods$Child.method2()]
Test passed: Got expected output from JVMTI GetClassMethods!
--System.err:(1/15)--
STATUS:Passed.


[1] http://cr.openjdk.java.net/~dtitov/8216324/webrev.02/

Thanks,
Daniil

From: mailto:serguei.spit...@oracle.com mailto:serguei.spit...@oracle.com
Date: Monday, July 20, 2020 at 6:48 PM
To: Alex Menkov mailto:alexey.men...@oracle.com, Daniil Titov 
mailto:daniil.x.ti...@oracle.com, serviceability-dev 
mailto:serviceability-dev@openjdk.java.net
Subject: Re: RFR: 8216324: GetClassMethods is confused by the presence of 
default methods in super interfaces

Hi Daniil,

The fix looks pretty good to me.

Just minor comments.


http://cr.openjdk.java.net/~dtitov/8216324/webrev.01/src/hotspot/share/prims/jvmtiEnv.cpp.frames.html
2519   int skipped = 0;  // skip default methods from superinterface (see 
JDK-8216324)

You can say just:  // skip overpass methods
There is probably no need to list the bug number.

2523 // Depending on can_maintain_original_method_order capability
2524 // use the original method ordering indices stored in the class, so we 
can emit
2525 // jmethodIDs in the order they appeared in the class file
2526 // or just copy in any order
Could you, please re-balance the comment a little bit?
Also, the comment has to be terminated with a dot.
Replace: "or just copy in any order" => "or just copy in current order".


http://cr.openjdk.java.net/~dtitov/8216324/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/jvmti/GetClassMethods/getclmthd007.java.frames.html
  114 default void default_method() { } // should never be seen
The comment above is not clear. Should never be seen in what context?
  117 interface OuterInterface1  extends DefaultInterface {
An extra space before "extends

Re: RFR (S) 8249822: SymbolPropertyTable creates an extra OopHandle per entry

[coleen . phillimore]

Thanks Serguei!
Coleen

On 7/23/20 3:18 PM, serguei.spit...@oracle.com wrote:

Thank you for answering my question, Coleen!
Serguei


On 7/23/20 04:36, coleen.phillim...@oracle.com wrote:



On 7/22/20 8:53 PM, serguei.spit...@oracle.com wrote:

Hi Coleen,

The fix looks good to me.


Thank you for looking at this Serguei.



On 7/22/20 13:55, coleen.phillim...@oracle.com wrote:
Summary: Add an assert to OopHandle assigment operator to catch 
leaking OopHandles, and fix code accordingly.


There are some jvmtiRedefineClasses.cpp changes here - basically, I 
moved the RedefineVerifyMark and comments into 
jvmtiRedefineClasses.cpp because I needed a Handle.


I think, the jvmtiRedefineClasses.cpp is a better place for the 
RedefineVerifyMark.
But could you, please, explain a little bit more why this move was 
necessary?

Why Handle can not be used in the jvmtiThreadState.cpp?


I did have a patch where I moved the constructor and destructor in 
jvmtiThreadState.cpp but thought it made more sense just to move the 
whole class since nothing else would ever use it.   I couldn't put 
the code change in jvmtiThreadState.hpp in place because I'd need to 
include handles.inline.hpp there.  It seems like 
jvmtiRedefineClasses.cpp already included all the files it needed, so 
that's where I moved it.


Thanks,
Coleen




Thanks,
Serguei



Ran tier1-6 tests and tier1 on all Oracle platforms.

open webrev at 
http://cr.openjdk.java.net/~coleenp/2020/8249822.01/webrev

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

Thanks,
Coleen

Re: [15?] RFR (S): 8249192: MonitorInfo stores raw oops across safepoints

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

LGTM++

Thanks,
Serguei

On 7/23/20 08:35, Daniel D. Daugherty wrote:

jdk16:
http://cr.openjdk.java.net/~tschatzl/8249192/webrev.2_to_3/ (diff)

jdk15:
http://cr.openjdk.java.net/~tschatzl/8249192/webrev.jdk15.2_to_3/ (diff) 


The delta webrevs look just fine.

Dan


On 7/23/20 5:39 AM, Thomas Schatzl wrote:

Hi Dan and Serguei,

  thanks for your reviews.

On 22.07.20 19:04, Daniel D. Daugherty wrote:

jdk15:

http://cr.openjdk.java.net/~tschatzl/8249192/webrev.jdk15.2/ (full) 


src/hotspot/share/prims/jvmtiEnvBase.cpp
 old L1029:   ResourceMark rm;
 It's not clear (to me anyway) why this ResourceMark is 
removed.


 Update: I saw the discussion of "ResourceMark rm" in JDK15 
versus
 "ResourceMark rm(current_thread)" in JDK16, but that 
doesn't tell

 me why it was necessary to remove that ResourceMark.


The method that is guarded by this ResourceMark contains the 
necessary ResourceMark itself, so I removed it.




src/hotspot/share/prims/stackwalk.cpp
 L291:     ResourceMark rm;
 L292:     HandleMark hm;
 Since there's a TRAPS parameter, these should be 
'rm(THREAD)' and

 'hm(THREAD)'.

src/hotspot/share/runtime/biasedLocking.cpp
 No comments.

src/hotspot/share/runtime/deoptimization.cpp
 No comments.

src/hotspot/share/runtime/vframe.cpp
 L461:   _lock  = lock;
 nit - extra space before '='.

src/hotspot/share/runtime/vframe.hpp
 L32: #include "runtime/handles.inline.hpp"
 nit - new include is out of order; should be after frame.hpp.

src/hotspot/share/runtime/vframeArray.cpp
 No comments.

src/hotspot/share/runtime/vframe_hp.cpp
 Skipped - no changes.

src/hotspot/share/services/threadService.cpp
 No comments.



All fixed, and incorporating Serguei's changes in the other email as 
well.


jdk16:
http://cr.openjdk.java.net/~tschatzl/8249192/webrev.3/ (full)
http://cr.openjdk.java.net/~tschatzl/8249192/webrev.2_to_3/ (diff)

jdk15:
http://cr.openjdk.java.net/~tschatzl/8249192/webrev.jdk15.3/ (full)
http://cr.openjdk.java.net/~tschatzl/8249192/webrev.jdk15.2_to_3/ (diff)

Note that the jdk15 change will only go into 15.0.2 as discussion 
with the release team showed that the change is too risky for earlier 
releases. See the relevant CR comment for details.


Thanks,
  Thomas

Re: RFR (S) 8249822: SymbolPropertyTable creates an extra OopHandle per entry

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

Thank you for answering my question, Coleen!
Serguei


On 7/23/20 04:36, coleen.phillim...@oracle.com wrote:



On 7/22/20 8:53 PM, serguei.spit...@oracle.com wrote:

Hi Coleen,

The fix looks good to me.


Thank you for looking at this Serguei.



On 7/22/20 13:55, coleen.phillim...@oracle.com wrote:
Summary: Add an assert to OopHandle assigment operator to catch 
leaking OopHandles, and fix code accordingly.


There are some jvmtiRedefineClasses.cpp changes here - basically, I 
moved the RedefineVerifyMark and comments into 
jvmtiRedefineClasses.cpp because I needed a Handle.


I think, the jvmtiRedefineClasses.cpp is a better place for the 
RedefineVerifyMark.
But could you, please, explain a little bit more why this move was 
necessary?

Why Handle can not be used in the jvmtiThreadState.cpp?


I did have a patch where I moved the constructor and destructor in 
jvmtiThreadState.cpp but thought it made more sense just to move the 
whole class since nothing else would ever use it.   I couldn't put the 
code change in jvmtiThreadState.hpp in place because I'd need to 
include handles.inline.hpp there.  It seems like 
jvmtiRedefineClasses.cpp already included all the files it needed, so 
that's where I moved it.


Thanks,
Coleen




Thanks,
Serguei



Ran tier1-6 tests and tier1 on all Oracle platforms.

open webrev at 
http://cr.openjdk.java.net/~coleenp/2020/8249822.01/webrev

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

Thanks,
Coleen

Re: RFR: 8216324: GetClassMethods is confused by the presence of default methods in super interfaces

[Daniil Titov]

Hi Serguei and Alex,

Please review a new version of the webrev [1]  that includes the changes 
Serguei suggested.  This version also has new test renamed from 
DefaultMethods.java to OverpassMethods.java to avoid warnings during the build 
about conflict with native library for runtime/jni/8033445/DefaultMethods.java  
test.

[1] http://cr.openjdk.java.net/~dtitov/8216324/webrev.04/

Thank you,
Daniil

From: "serguei.spit...@oracle.com" 
Date: Tuesday, July 21, 2020 at 10:53 PM
To: Daniil Titov , Alex Menkov 
, serviceability-dev 

Subject: Re: RFR: 8216324: GetClassMethods is confused by the presence of 
default methods in super interfaces

Hi Daniil,

Thank you for the update!
It looks good to me.

Just two more minor comments and no need for another webrev you address them.
2519   int skipped = 0;  // skip default methods
Saying "overpass methods" would be more precise.
  47 printf("Enabled capability: maintain_original_method_order: true\n");
It is better to get rid of ": true" at the end (sorry, I missed this in my 
previous suggestion).
Enabling capability means it is set to true.


Thanks,
Serguei


On 7/21/20 20:25, Daniil Titov wrote:
Hi Serguei and Alex,

Please, review new version of the change [1]  that includes changes as Serguei 
suggested.

114 default void default_method() { } // should never be seen
The comment above is not clear. Should never be seen in what context?
This method should not be included in the list of methods GetClassMethods 
returns for the sub-interface or implementing class.  I don't think we want to 
comment each method in the test interfaces declared in this test when they 
should be seen in this test and when they should not. This information already 
exists in getclmthd007.cpp, thus I decided to omit this comment.

Please see below the output from the new test.


--messages:(4/215)--
command: main -agentlib:DefaultMethods DefaultMethods
reason: User specified action: run main/othervm/native -agentlib:DefaultMethods 
DefaultMethods 
Mode: othervm [/othervm specified]
elapsed time (seconds): 0.241
--configuration:(0/0)--
--System.out:(3/265)--
Reflection getDeclaredMethods returned: [public abstract java.lang.String 
DefaultMethods$Child.method2()]
JVMTI GetClassMethods returned: [public abstract java.lang.String 
DefaultMethods$Child.method2()]
Test passed: Got expected output from JVMTI GetClassMethods!
--System.err:(1/15)--
STATUS:Passed.



--messages:(4/276)--
command: main -agentlib:DefaultMethods=maintain_original_method_order 
DefaultMethods
reason: User specified action: run main/othervm/native 
-agentlib:DefaultMethods=maintain_original_method_order DefaultMethods 
Mode: othervm [/othervm specified]
elapsed time (seconds): 0.25
--configuration:(0/0)--
--System.out:(4/322)--
Enabled capability: maintain_original_method_order: true
Reflection getDeclaredMethods returned: [public abstract java.lang.String 
DefaultMethods$Child.method2()]
JVMTI GetClassMethods returned: [public abstract java.lang.String 
DefaultMethods$Child.method2()]
Test passed: Got expected output from JVMTI GetClassMethods!
--System.err:(1/15)--
STATUS:Passed.


[1] http://cr.openjdk.java.net/~dtitov/8216324/webrev.02/ 

Thanks,
Daniil

From: mailto:serguei.spit...@oracle.com mailto:serguei.spit...@oracle.com
Date: Monday, July 20, 2020 at 6:48 PM
To: Alex Menkov mailto:alexey.men...@oracle.com, Daniil Titov 
mailto:daniil.x.ti...@oracle.com, serviceability-dev 
mailto:serviceability-dev@openjdk.java.net
Subject: Re: RFR: 8216324: GetClassMethods is confused by the presence of 
default methods in super interfaces

Hi Daniil,

The fix looks pretty good to me.

Just minor comments.


http://cr.openjdk.java.net/~dtitov/8216324/webrev.01/src/hotspot/share/prims/jvmtiEnv.cpp.frames.html
2519   int skipped = 0;  // skip default methods from superinterface (see 
JDK-8216324)

You can say just:  // skip overpass methods
There is probably no need to list the bug number.

2523 // Depending on can_maintain_original_method_order capability
2524 // use the original method ordering indices stored in the class, so we 
can emit
2525 // jmethodIDs in the order they appeared in the class file
2526 // or just copy in any order
Could you, please re-balance the comment a little bit?
Also, the comment has to be terminated with a dot.
Replace: "or just copy in any order" => "or just copy in current order".


http://cr.openjdk.java.net/~dtitov/8216324/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/jvmti/GetClassMethods/getclmthd007.java.frames.html
 114 default void default_method() { } // should never be seen
The comment above is not clear. Should never be seen in what context?
 117 interface OuterInterface1  extends DefaultInterface {
An extra space before "extends".


http://cr.openjdk.java.net/~dtitov/8216324/webrev.01/test/hotspot/jtreg/serviceability/jvmti/G

RFR (T) 8250042: Clean up methodOop and method_oop names from the code

[coleen . phillimore]

See bug for more details.  I've been running into these names a lot 
lately.   Many of these names are in JVMTI.


Tested with tier1 on all Oracle platforms and built on non-Oracle platforms.

open webrev at http://cr.openjdk.java.net/~coleenp/2020/8250042.01/webrev
bug link https://bugs.openjdk.java.net/browse/JDK-8250042

Thanks,
Coleen

Re: [15?] RFR (S): 8249192: MonitorInfo stores raw oops across safepoints

[Daniel D. Daugherty]

jdk16:
http://cr.openjdk.java.net/~tschatzl/8249192/webrev.2_to_3/ (diff)

jdk15:
http://cr.openjdk.java.net/~tschatzl/8249192/webrev.jdk15.2_to_3/ (diff) 


The delta webrevs look just fine.

Dan


On 7/23/20 5:39 AM, Thomas Schatzl wrote:

Hi Dan and Serguei,

  thanks for your reviews.

On 22.07.20 19:04, Daniel D. Daugherty wrote:

jdk15:

http://cr.openjdk.java.net/~tschatzl/8249192/webrev.jdk15.2/ (full) 


src/hotspot/share/prims/jvmtiEnvBase.cpp
 old L1029:   ResourceMark rm;
 It's not clear (to me anyway) why this ResourceMark is removed.

 Update: I saw the discussion of "ResourceMark rm" in JDK15 
versus
 "ResourceMark rm(current_thread)" in JDK16, but that doesn't 
tell

 me why it was necessary to remove that ResourceMark.


The method that is guarded by this ResourceMark contains the necessary 
ResourceMark itself, so I removed it.




src/hotspot/share/prims/stackwalk.cpp
 L291:     ResourceMark rm;
 L292:     HandleMark hm;
 Since there's a TRAPS parameter, these should be 
'rm(THREAD)' and

 'hm(THREAD)'.

src/hotspot/share/runtime/biasedLocking.cpp
 No comments.

src/hotspot/share/runtime/deoptimization.cpp
 No comments.

src/hotspot/share/runtime/vframe.cpp
 L461:   _lock  = lock;
 nit - extra space before '='.

src/hotspot/share/runtime/vframe.hpp
 L32: #include "runtime/handles.inline.hpp"
 nit - new include is out of order; should be after frame.hpp.

src/hotspot/share/runtime/vframeArray.cpp
 No comments.

src/hotspot/share/runtime/vframe_hp.cpp
 Skipped - no changes.

src/hotspot/share/services/threadService.cpp
 No comments.



All fixed, and incorporating Serguei's changes in the other email as 
well.


jdk16:
http://cr.openjdk.java.net/~tschatzl/8249192/webrev.3/ (full)
http://cr.openjdk.java.net/~tschatzl/8249192/webrev.2_to_3/ (diff)

jdk15:
http://cr.openjdk.java.net/~tschatzl/8249192/webrev.jdk15.3/ (full)
http://cr.openjdk.java.net/~tschatzl/8249192/webrev.jdk15.2_to_3/ (diff)

Note that the jdk15 change will only go into 15.0.2 as discussion with 
the release team showed that the change is too risky for earlier 
releases. See the relevant CR comment for details.


Thanks,
  Thomas

Re: RFR (S) 8249822: SymbolPropertyTable creates an extra OopHandle per entry

[coleen . phillimore]

Thanks Erik. I added the assert to replace() and reran tier1 and 3 tests.
Coleen

On 7/23/20 10:40 AM, Erik Österlund wrote:

Hi Coleen,

Looks good. One thing though: in the replace() function, assert that 
the obj_ptr != NULL instead of silently doing nothing.


Don't need another webrev.

Thanks,
/Erik

On 2020-07-22 22:55, coleen.phillim...@oracle.com wrote:
Summary: Add an assert to OopHandle assigment operator to catch 
leaking OopHandles, and fix code accordingly.


There are some jvmtiRedefineClasses.cpp changes here - basically, I 
moved the RedefineVerifyMark and comments into 
jvmtiRedefineClasses.cpp because I needed a Handle.


Ran tier1-6 tests and tier1 on all Oracle platforms.

open webrev at 
http://cr.openjdk.java.net/~coleenp/2020/8249822.01/webrev

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

Thanks,
Coleen

Re: RFR: 8248362: JVMTI frame operations should use Thread-Local Handshake

[Daniel D. Daugherty]

On 7/22/20 9:25 PM, David Holmes wrote:

Hi Dan,

I just want to respond to one aspect of your response ...

On 23/07/2020 11:04 am, Daniel D. Daugherty wrote:

On 7/22/20 10:38 AM, Yasumasa Suenaga wrote:

Hi all,

Please review this change:

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


src/hotspot/share/prims/jvmtiEnv.cpp
 L1755 // JVMTI get java stack frame location at safepoint.
 You missed updating this one. Perhaps:

   // JVMTI get java stack frame location via direct 
handshake.


src/hotspot/share/prims/jvmtiEnvBase.cpp
 L1563:   JavaThread* jt = _state->get_thread();
 L1564:   assert(target == jt, "just checking");
 This code is different than the other closures. It might be 
worth
 a comment to explain why. I don't remember why 
VM_GetFrameCount had
 to use the JvmtiThreadState to fetch the JavaThread. Serguei 
might

 remember.

 It could be that we don't need the _state field anymore 
because of
 the way that handshakes work (and provide the JavaThread* to 
the
 do_thread() function). Your choice on whether to deal with 
that as

 part of this fix or following with another RFE.

 Update: Uggg the get_frame_count() function takes 
JvmtiThreadState
 as a parameter. This is very much entangled... sigh... we 
should

 definitely look at untangling this in another RFE...

 old L1565:   ThreadsListHandle tlh;
 new L1565:   if (!jt->is_exiting() && jt->threadObj() != NULL) {

 Please consider add this comment above L1565:
  // ThreadsListHandle is not needed due to direct 
handshake.


 Yes, I see that previous closures were added without that 
comment,
 but when I see "if (!jt->is_exiting() && jt->threadObj() != 
NULL)"
 I immediately wonder where the ThreadsListHandle is... 
Please consider

 adding the comment to the others also.


I understand why, when looking at this change, you would like the 
comment, but outside of the webrev the reference to ThreadsListHandle 
doesn't really have any context. We need a TLH anywhere there is a 
risk the target thread might exit while we're interacting with it, but 
that can't happen in the context of a direct handshake operation with 
the target thread because the TLH exists in our caller. This is the 
new normal for code that executes as part of a direct handshake.


Agreed. I have to adjust to the "new normal".

Dan




David
-


 old L1574:   ThreadsListHandle tlh;
 new L1574:   if (!jt->is_exiting() && jt->threadObj() != NULL) {

 Please consider add this comment above L1574:
  // ThreadsListHandle is not needed due to direct 
handshake.


src/hotspot/share/prims/jvmtiEnvBase.hpp
 L580: // HandshakeClosure to frame location.
 typo - s/to frame/to get frame/

src/hotspot/share/prims/jvmtiThreadState.cpp
 No comments on the changes.

 For the above comments about VM_GetFrameCount, understanding why
 JvmtiThreadState::count_frames() has to exist in JvmtiThreadState
 will go along way towards untangling the mess.

src/hotspot/share/runtime/vmOperations.hpp
 No comments.


Thumbs up. I only have nits above. If you choose to make the above
changes, I do not need to see another webrev.

Dan





Migrate JVMTI frame operations to Thread-Local Handshakes from VM 
Operations.


    - VM_GetFrameCount
    - VM_GetFrameLocation

They affects both GetFrameCount() and GetFrameLocation() JVMTI 
functions.


This change has passed all tests on submit repo 
(mach5-one-ysuenaga-JDK-8248362-20200722-1249-12859056), and 
vmTestbase/nsk/{jdi,jdw,jvmti}, serviceability/{jdwp,jvmti} .



Thanks,

Yasumasa

Re: RFR (S) 8249822: SymbolPropertyTable creates an extra OopHandle per entry

[Erik Österlund]

Hi Coleen,

Looks good. One thing though: in the replace() function, assert that the 
obj_ptr != NULL instead of silently doing nothing.


Don't need another webrev.

Thanks,
/Erik

On 2020-07-22 22:55, coleen.phillim...@oracle.com wrote:
Summary: Add an assert to OopHandle assigment operator to catch 
leaking OopHandles, and fix code accordingly.


There are some jvmtiRedefineClasses.cpp changes here - basically, I 
moved the RedefineVerifyMark and comments into 
jvmtiRedefineClasses.cpp because I needed a Handle.


Ran tier1-6 tests and tier1 on all Oracle platforms.

open webrev at http://cr.openjdk.java.net/~coleenp/2020/8249822.01/webrev
bug link https://bugs.openjdk.java.net/browse/JDK-8249822

Thanks,
Coleen

RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents

[Lindenmaier, Goetz]

Hi Richard, 

Thanks for your two further explanations in the other thread. 
That made the points clear to me.

> > I was not that happy with the names saying not_global_escape
> > and similar. I now agreed you have to use the terms of the escape
> > analysis (NoEscape ArgEscape= throughout the runtime code. I'm still not 
> > happy with
> > the 'not' in the term, I always try to expand the name to some
> > sentence with a negated verb, but it makes no sense.
> > For example, "has_not_global_escape_in_scope" expands to
> > "Hasn't a global escape in its scope." in my thinking, which makes
> > no sense. You probably mean
> > "Has not-global escape in its scope." or "Has {ArgEscape|NoEscape}
> > in its scope."
> 
> > C2 is using the word "non" in this context, e.g., here
> > alloc->is_non_escaping.
> 
> There is also ConnectionGraph::not_global_escape()
That talks about a single node that represents a single 
Object. An object has a single state wrt. ea.
You use the term for safepoint which tracks a set of objects.
Here, has_not_global_excape can mean
  1. None of the several objects does escape globaly.
  2. There is at least one object that escapes globaly.

> > non obviously negates the adjective 'global',
> > non-global or nonglobal even is a English term I find in the
> > net.
> > So what about "has_non_global_escape_in_scope?"
> 
> And what about has_ea_local_in_scope?
That's good. Please document somewhere that 
Ea_local == ArgEscape | NoEscape.
That's what it is, right?

> > Does jvmti specify that the same limits are used ...?
> > ok on your side.
> 
> I don't know and didn't find anything in a quick search.
Ok, not your business.

> 
> > jvmtiEnvBase.cpp  ok
> > jvmtiImpl.h|cpp  ok
> > jvmtiTagMap.cpp ok
> > whitebox.cpp ok
> 
> > deoptimization.cpp
> 
> > line 177: Please break line
> > line 246, 281: Please break line
> > 1578, 1583, 1589, 1632, 1649, 1651 Break line
> 
> > 1651: You use 'non'-terms, too: non-escaping :)
> 
> I know :) At least here it is wrong I'd say. "...has to be a not escaping 
> obj..."
> sounds better
> (hopefully not only to my german ears).
I thought the term non-escpaing makes it quite clear.
I just wanted to point out that using non above would
be similar to the wording here.

> > IterateHeapWithEscapeAnalysisEnabled.java
> 
> > line 415:
> > msg("wait until target thread has set testMethod_result");
> > while (testMethod_result == 0) {
> > Thread.sleep(50);
> > }
> > Might the test run into timeouts at this place?
> > The field is volatile, i.e. it will be reloaded
> > in each iteration. But will dontinline_testMethod
> > write it back to main memory in time?
> 
> You mean, the test could hang in that loop for a couple of minutes? I don't
> think so. There are cache coherence protocols in place which will invalidate
> stale data very timely.
Ok, anyways, it would only be a hanging test.
> 
> Ok. I've removed quite a lot of the occurrances.
> 
> > Also, I like full sentences in comments.
> > Especially for me as foreign speaker, this makes
> > things much more clear. I.e., I try to make it
> > a real sentence with articles, capitalized and a
> > dot at the end if there is a subject and a verb
> > in first place.
> > E.g., jvmtiEnvBase.cpp:1327
> 
> Are you referring to the following?
> (from
> http://cr.openjdk.java.net/~rrich/webrevs/2019/8227745/webrev.6/src/hots
> pot/share/prims/jvmtiEnvBase.cpp.frames.html)
> 
> 1326
> 1327   // If the frame is a compiled one, need to deoptimize it.
> 1328   if (vf->is_compiled_frame()) {
> 
> This line 1327 is preexisting.
Sorry, wrong line number again. 
I think I meant
1333 // eagerly reallocate scalar replaced objects.

But I must admit, the subject is missing. It's one of these 
imperative sentences where the subject is left out, which 
are used throughout documentation.
Bad example, but still a correct sentence, so qualifies 
for punctuation?

Best regards,
  Goetz.

Re: jmx-dev MBean attribute deprecation

[Daniel Fuchs]

Hi Milan,

JMX is being maintained by serviceability these days.
I am forwarding your question there.

This is an interesting observation. JMX makes it possible to
define and use annotations on methods/getters/setters, to add
key/value pairs to the Descriptor of the corresponding
MBeanFeatureInfo.

The mechanism should probably be extended so that the standard
@Deprecated annotation is also reflected in the Descriptor.

best regards,

-- daniel

On 23/07/2020 13:25, Milan Mimica wrote:

Hello list

With deprecation of OperatingSystem#SystemCpuLoad in favor of 
OperatingSystem#CpuLoad [1] an issue raised with generic tools that 
query all MBean attributes. Querying CPU usage is not side-effect-free: 
the immediate subsequent query for CPU load will often return a skewed 
value (on Linux at least, haven't checked others)[2]. This results in 
exported data where either new or deprecated MBean value is wrong and 
misleading.


There are many tools and libraries that generically export JMX metrics, 
so I think this should be addressed somehow. Maybe we could add a 
property in MBeanAttributeInfo which would mark the attribute as 
deprecated? These tools could then skip such metrics.



[1] 
https://docs.oracle.com/en/java/javase/14/docs/api/jdk.management/com/sun/management/OperatingSystemMXBean.html#getSystemCpuLoad()
[2] 
https://medium.com/@milan.mimica/the-java-cpu-usage-observer-effect-18808b18323f


--
Milan Mimica

Re: [15?] RFR (S): 8249192: MonitorInfo stores raw oops across safepoints

[David Holmes]

Hi Thomas,

Incrementals looks good to me.

Thanks,
David
-

On 23/07/2020 7:39 pm, Thomas Schatzl wrote:

Hi Dan and Serguei,

   thanks for your reviews.

On 22.07.20 19:04, Daniel D. Daugherty wrote:

jdk15:

http://cr.openjdk.java.net/~tschatzl/8249192/webrev.jdk15.2/ (full) 


src/hotspot/share/prims/jvmtiEnvBase.cpp
 old L1029:   ResourceMark rm;
 It's not clear (to me anyway) why this ResourceMark is removed.

 Update: I saw the discussion of "ResourceMark rm" in JDK15 
versus
 "ResourceMark rm(current_thread)" in JDK16, but that doesn't 
tell

 me why it was necessary to remove that ResourceMark.


The method that is guarded by this ResourceMark contains the necessary 
ResourceMark itself, so I removed it.




src/hotspot/share/prims/stackwalk.cpp
 L291:     ResourceMark rm;
 L292:     HandleMark hm;
 Since there's a TRAPS parameter, these should be 'rm(THREAD)' 
and

 'hm(THREAD)'.

src/hotspot/share/runtime/biasedLocking.cpp
 No comments.

src/hotspot/share/runtime/deoptimization.cpp
 No comments.

src/hotspot/share/runtime/vframe.cpp
 L461:   _lock  = lock;
 nit - extra space before '='.

src/hotspot/share/runtime/vframe.hpp
 L32: #include "runtime/handles.inline.hpp"
 nit - new include is out of order; should be after frame.hpp.

src/hotspot/share/runtime/vframeArray.cpp
 No comments.

src/hotspot/share/runtime/vframe_hp.cpp
 Skipped - no changes.

src/hotspot/share/services/threadService.cpp
 No comments.



All fixed, and incorporating Serguei's changes in the other email as well.

jdk16:
http://cr.openjdk.java.net/~tschatzl/8249192/webrev.3/ (full)
http://cr.openjdk.java.net/~tschatzl/8249192/webrev.2_to_3/ (diff)

jdk15:
http://cr.openjdk.java.net/~tschatzl/8249192/webrev.jdk15.3/ (full)
http://cr.openjdk.java.net/~tschatzl/8249192/webrev.jdk15.2_to_3/ (diff)

Note that the jdk15 change will only go into 15.0.2 as discussion with 
the release team showed that the change is too risky for earlier 
releases. See the relevant CR comment for details.


Thanks,
   Thomas

Re: RFR (S) 8249822: SymbolPropertyTable creates an extra OopHandle per entry

[coleen . phillimore]

On 7/22/20 8:53 PM, serguei.spit...@oracle.com wrote:

Hi Coleen,

The fix looks good to me.


Thank you for looking at this Serguei.



On 7/22/20 13:55, coleen.phillim...@oracle.com wrote:
Summary: Add an assert to OopHandle assigment operator to catch 
leaking OopHandles, and fix code accordingly.


There are some jvmtiRedefineClasses.cpp changes here - basically, I 
moved the RedefineVerifyMark and comments into 
jvmtiRedefineClasses.cpp because I needed a Handle.


I think, the jvmtiRedefineClasses.cpp is a better place for the 
RedefineVerifyMark.
But could you, please, explain a little bit more why this move was 
necessary?

Why Handle can not be used in the jvmtiThreadState.cpp?


I did have a patch where I moved the constructor and destructor in 
jvmtiThreadState.cpp but thought it made more sense just to move the 
whole class since nothing else would ever use it.   I couldn't put the 
code change in jvmtiThreadState.hpp in place because I'd need to include 
handles.inline.hpp there.  It seems like jvmtiRedefineClasses.cpp 
already included all the files it needed, so that's where I moved it.


Thanks,
Coleen




Thanks,
Serguei



Ran tier1-6 tests and tier1 on all Oracle platforms.

open webrev at 
http://cr.openjdk.java.net/~coleenp/2020/8249822.01/webrev

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

Thanks,
Coleen

Re: [15?] RFR (S): 8249192: MonitorInfo stores raw oops across safepoints

[Thomas Schatzl]

Hi Dan and Serguei,

  thanks for your reviews.

On 22.07.20 19:04, Daniel D. Daugherty wrote:

jdk15:

http://cr.openjdk.java.net/~tschatzl/8249192/webrev.jdk15.2/ (full) 


src/hotspot/share/prims/jvmtiEnvBase.cpp
     old L1029:   ResourceMark rm;
     It's not clear (to me anyway) why this ResourceMark is removed.

     Update: I saw the discussion of "ResourceMark rm" in JDK15 versus
     "ResourceMark rm(current_thread)" in JDK16, but that doesn't tell
     me why it was necessary to remove that ResourceMark.


The method that is guarded by this ResourceMark contains the necessary 
ResourceMark itself, so I removed it.




src/hotspot/share/prims/stackwalk.cpp
     L291:     ResourceMark rm;
     L292:     HandleMark hm;
     Since there's a TRAPS parameter, these should be 'rm(THREAD)' and
     'hm(THREAD)'.

src/hotspot/share/runtime/biasedLocking.cpp
     No comments.

src/hotspot/share/runtime/deoptimization.cpp
     No comments.

src/hotspot/share/runtime/vframe.cpp
     L461:   _lock  = lock;
     nit - extra space before '='.

src/hotspot/share/runtime/vframe.hpp
     L32: #include "runtime/handles.inline.hpp"
     nit - new include is out of order; should be after frame.hpp.

src/hotspot/share/runtime/vframeArray.cpp
     No comments.

src/hotspot/share/runtime/vframe_hp.cpp
     Skipped - no changes.

src/hotspot/share/services/threadService.cpp
     No comments.



All fixed, and incorporating Serguei's changes in the other email as well.

jdk16:
http://cr.openjdk.java.net/~tschatzl/8249192/webrev.3/ (full)
http://cr.openjdk.java.net/~tschatzl/8249192/webrev.2_to_3/ (diff)

jdk15:
http://cr.openjdk.java.net/~tschatzl/8249192/webrev.jdk15.3/ (full)
http://cr.openjdk.java.net/~tschatzl/8249192/webrev.jdk15.2_to_3/ (diff)

Note that the jdk15 change will only go into 15.0.2 as discussion with 
the release team showed that the change is too risky for earlier 
releases. See the relevant CR comment for details.


Thanks,
  Thomas