Re: RFR(M): 8247515: OSX pc_to_symbol() lookup does not work with core files
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 to locate them. This was done in ps_core.c, and there are "JDK-8249779" comment references in the code where I did this. The end result of this is that PMap for core files won't show system libraries, and PStack for core files won't show symbols for addresses in system libraries. Note that currently support for PMap and PStack is disabled for OSX, but I will shortly send out a review to enable them for OSX core files as part of the fix for [2] JDK-8248882. [1] https://bugs.openjdk.java.net/browse/JDK-8249779 [2] https://bugs.openjdk.java.net/browse/JDK-8248882 thanks, Chris On 7/14/20 5:46 PM, serguei.spit...@oracle.com wrote: Okay, I'll wait for new webrev
Re: RFR: 8248362: JVMTI frame operations should use Thread-Local Handshake
On 23/07/2020 1:13 pm, Yasumasa Suenaga wrote:
Hi David,
Thanks for your comment!
On 2020/07/23 11:01, David Holmes wrote:
Hi Yasumasa,
On 23/07/2020 12: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/
Migrate JVMTI frame operations to Thread-Local Handshakes from VM
Operations.
- VM_GetFrameCount
- VM_GetFrameLocation
They affects both GetFrameCount() and GetFrameLocation() JVMTI
functions.
Your changes all seem fine.
But I'm a bit confused about the existing code. In
JvmtiEnv::GetFrameLocation we have:
if (java_thread == JavaThread::current()) {
err = get_frame_location(java_thread, depth, method_ptr,
location_ptr);
but then in get_frame_location we have:
assert((SafepointSynchronize::is_at_safepoint() ||
java_thread->is_thread_fully_suspended(false, &debug_bits)),
"at safepoint or target thread is suspended");
and that assert must surely fire if called by the current thread for
itself! ???
is_thread_fully_suspended() returns true when it calles from current
thread, so this assert would not fire.
I had not realized it was defined that way. Seems like a bit of a kludge
to avoid making the call conditional on a check of the current thread. :(
Cheers,
David
Yasumasa
Thanks,
David
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(L): 8215624: add parallel heap inspection support for jmap histo(G1)(Internet mail)
Hi Paul,
Thanks for your help, that all looks good to me.
Just 2 minor changes:
• delete the final return in ParHeapInspectTask::work, you mentioned it
but seems not include in the webrev :-)
• delete a unnecessary blank line in heapInspect.cpp at merge_entry()
#
--- old/src/hotspot/share/memory/heapInspection.cpp 2020-07-23
11:23:29.281666456 +0800
+++ new/src/hotspot/share/memory/heapInspection.cpp 2020-07-23
11:23:29.017666447 +0800
@@ -251,7 +251,6 @@
_size_of_instances_in_words += cie->words();
return true;
}
-
return false;
}
@@ -568,7 +567,6 @@
Atomic::add(&_missed_count, missed_count);
} else {
Atomic::store(&_success, false);
- return;
}
}
#
Here is the webrev
http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_08/
BRs,
Lin
-
From: "Hohensee, Paul"
Date: Thursday, July 23, 2020 at 6:48 AM
To: "linzang(臧琳)" , Stefan Karlsson
, "serguei.spit...@oracle.com"
, David Holmes ,
serviceability-dev ,
"hotspot-gc-...@openjdk.java.net"
Subject: RE: RFR(L): 8215624: add parallel heap inspection support for jmap
histo(G1)(Internet mail)
Just small things.
heapInspection.cpp:
In ParHeapInspectTask::work, remove the final return statement and fix the
following ‘}’ indent. I.e., replace
+ Atomic::store(&_success, false);
+ return;
+ }
with
+ Atomic::store(&_success, false);
+ }
In HeapInspection::heap_inspection, missed_count should be a uint to match
other missed_count declarations, and should be initialized to the result of
populate_table() rather than separately to 0.
attachListener.cpp:
In heap_inspection, initial_processor_count returns an int, so cast its result
to a uint.
Similarly, parse_uintx returns a uintx, so cast its result (num) to uint when
assigning to parallel_thread_num.
BasicJMapTest.java:
I took the liberty of refactoring testHisto*/histoToFile/testDump*/dump to
remove redundant interposition methods and make histoToFile and dump look as
similar as possible.
Webrev with the above changes in
http://cr.openjdk.java.net/~phh/8214535/webrev.01/
Thanks,
Paul
On 7/15/20, 2:13 AM, "linzang(臧琳)" wrote:
Upload a new webrev at
http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_07/
It fix a potential issue that unexpected number of threads maybe
calculated for "parallel" option of jmap -histo in container.
As shown at
http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_07-delta/src/hotspot/share/services/attachListener.cpp.udiff.html
### attachListener.cpp
@@ -252,11 +252,11 @@
static jint heap_inspection(AttachOperation* op, outputStream* out) {
bool live_objects_only = true; // default is true to retain the
behavior before this change is made
outputStream* os = out; // if path not specified or path is NULL, use
out
fileStream* fs = NULL;
const char* arg0 = op->arg(0);
- uint parallel_thread_num = MAX(1, os::processor_count() * 3 / 8); //
default is less than half of processors.
+ uint parallel_thread_num = MAX(1, os::initial_active_processor_count() *
3 / 8); // default is less than half of processors.
if (arg0 != NULL && (strlen(arg0) > 0)) {
if (strcmp(arg0, "-all") != 0 && strcmp(arg0, "-live") != 0) {
out->print_cr("Invalid argument to inspectheap operation: %s", arg0);
return JNI_ERR;
}
###
Thanks.
BRs,
Lin
On 2020/7/9, 3:22 PM, "linzang(臧琳)" wrote:
Hi Paul,
Thanks for reviewing!
>>
>> I'd move all the argument parsing code to JMap.java and just
pass the results to Hotspot. Both histo() in JMap.java and code in
attachListener.* parse the command line arguments, though the code in histo()
doesn't parse the argument to "parallel". I'd upgrade the code in histo() to do
a complete parse and pass the option values to executeCommandForPid as before:
there would just be more of them now. That would allow you to eliminate all the
parsing code in attachListener.cpp as well as the change to arguments.hpp.
>>
The reason I made the change in Jmap.java that compose all
arguments as 1 string , instead of passing 3 argments, is to avoid the
compatibility issue, as we discussed in
http://mail.openjdk.java.net/pipermail/serviceability-dev/2019-February/thread.html#27240.
The root cause of the compatibility issue is because max argument count in
HotspotVirtualMachineImpl.java and attachlistener.cpp need to be enlarged
(changes like http://hg.openjdk.java.net/jdk/jdk/rev/e7cf035682e3#l2.1) when
jmap has more than 3 arguments. But if user
Re: RFR: 8248362: JVMTI frame operations should use Thread-Local Handshake
Hi Dan,
Thanks for your comment!
I uploaded 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
On 2020/07/23 10:04, 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.
Fixed.
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...
I want to fix it in another RFE as you said if it is needed.
If we don't hear the reason from Serguei, I want to keep this change.
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.
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.
I think it is new normal as David said in the reply, and also other closures
don't say about it.
So I did not change about it in new webrev.
src/hotspot/share/prims/jvmtiEnvBase.hpp
L580: // HandshakeClosure to frame location.
typo - s/to frame/to get frame/
Fixed.
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.
Thanks, but I uploaded new webrev just in case :)
Yasumasa
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: 8248362: JVMTI frame operations should use Thread-Local Handshake
Hi David,
Thanks for your comment!
On 2020/07/23 11:01, David Holmes wrote:
Hi Yasumasa,
On 23/07/2020 12: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/
Migrate JVMTI frame operations to Thread-Local Handshakes from VM Operations.
- VM_GetFrameCount
- VM_GetFrameLocation
They affects both GetFrameCount() and GetFrameLocation() JVMTI functions.
Your changes all seem fine.
But I'm a bit confused about the existing code. In JvmtiEnv::GetFrameLocation
we have:
if (java_thread == JavaThread::current()) {
err = get_frame_location(java_thread, depth, method_ptr, location_ptr);
but then in get_frame_location we have:
assert((SafepointSynchronize::is_at_safepoint() ||
java_thread->is_thread_fully_suspended(false, &debug_bits)),
"at safepoint or target thread is suspended");
and that assert must surely fire if called by the current thread for itself! ???
is_thread_fully_suspended() returns true when it calles from current thread, so
this assert would not fire.
Yasumasa
Thanks,
David
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: 8248362: JVMTI frame operations should use Thread-Local Handshake
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: 8248362: JVMTI frame operations should use Thread-Local Handshake
Hi Yasumasa,
On 23/07/2020 12: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/
Migrate JVMTI frame operations to Thread-Local Handshakes from VM
Operations.
- VM_GetFrameCount
- VM_GetFrameLocation
They affects both GetFrameCount() and GetFrameLocation() JVMTI functions.
Your changes all seem fine.
But I'm a bit confused about the existing code. In
JvmtiEnv::GetFrameLocation we have:
if (java_thread == JavaThread::current()) {
err = get_frame_location(java_thread, depth, method_ptr,
location_ptr);
but then in get_frame_location we have:
assert((SafepointSynchronize::is_at_safepoint() ||
java_thread->is_thread_fully_suspended(false, &debug_bits)),
"at safepoint or target thread is suspended");
and that assert must surely fire if called by the current thread for
itself! ???
Thanks,
David
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
Fwd: Re: jdk/submit maintenance, July 23rd - July 27th
Forwarded Message Subject: Re: jdk/submit maintenance, July 23rd - July 27th Date: Wed, 22 Jul 2020 21:39:40 -0400 From: Stanislav Smirnov To: jdk-dev Hello, Due to circumstances beyond our control the planned maintenance is called off. This means, that jdk/submit will continue to work. Best regards, Stanislav Smirnov On Jul 22, 2020, at 3:26 PM, Stanislav Smirnov wrote: Hello, A planned maintenance will happen this week, July 23rd - 27th. jdk/submit builds will not be available starting tomorrow (Thursday 23rd) at 10am PT / 17:00 GMT. The system should be back online by 10am PT / 17:00 GMT, July 27th. Best regards, Stanislav Smirnov
Re: RFR: 8248362: JVMTI frame operations should use Thread-Local Handshake
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.
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: 8248362: JVMTI frame operations should use Thread-Local Handshake
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.
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
Hi Coleen, The fix looks good to me. 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? 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: 8248362: JVMTI frame operations should use Thread-Local Handshake
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: [15?] RFR (S): 8249192: MonitorInfo stores raw oops across safepoints
Hi Thomas, Thank you for the update, reply and 15 backport clarification. The update looks good and the testing looks sufficient to me. One minor suggestion: http://cr.openjdk.java.net/~tschatzl/8249192/webrev.2/src/hotspot/share/runtime/biasedLocking.cpp.frames.html 909 ResourceMark rm; 910 HandleMark hm; 911 Thread* cur = Thread::current(); This can be refactored a little bit: Thread* cur = Thread::current(); ResourceMark rm(cur); HandleMark hm(cur); No need in new webrev if you decide to use the above. Thanks, Serguei On 7/22/20 01:14, Thomas Schatzl wrote Hi Serguei, thanks for your review. On 22.07.20 04:13, serguei.spit...@oracle.com wrote: Hi Thomas, The fix looks okay to me. The 15 fix is identical to 16. no :) It is very subtle. As mentioned, in jvmtiEnvBase.cpp in the original code, in jdk15 we had: line 1029: ResourceMark rm; in jdk16: line 1008: ResourceMark rm(current_thread); Both lines were ultimately removed with this recent change, but still cause merge errors if you port the patch from one version to the other. Sorry if that has been confusing. This file finally was not changed: src/hotspot/share/runtime/vframe_hp.cpp . This is an artifact of webrev in conjunction with mq because the original change had some modifications which were retracted in the -1 webrev there. There will not be any change in any push for that file. Several files need a copyright comment update. Provided new webrevs with only comment updates below. Did not do new testing just for these comment updates though. What tests do you run? We need at least tier3 to make sure there are no regressions in the JVMTI and JDI tests. The jdk15 .1 patch has been tested with 1.2k of that LocalsAndOperands test with the options that originally reproduced it in 1/100 cases. hs-tier1-5, no failures at all. jdk16 has had testing with the .0 patch doing 1.8k of LocalsAndOperands.java, tier1-5, and tier8 with JDK-8249676 reinstated that earlier caused lots of issues there (and none without). Since there has been no substantial change in how the patch works, only some refactoring, so I think these are still valid. See the internal comments in the CR for links. New webrevs: jdk15: http://cr.openjdk.java.net/~tschatzl/8249192/webrev.jdk15.2/ (full) http://cr.openjdk.java.net/~tschatzl/8249192/webrev.jdk15.1_to_2/ (diff) jdk16: http://cr.openjdk.java.net/~tschatzl/8249192/webrev.2/ (full) http://cr.openjdk.java.net/~tschatzl/8249192/webrev.1_to_2/ (diff) Thanks, Thomas
Fwd: jdk/submit maintenance, July 23rd - July 27th
Just in case people don't see the jdk-dev email. David - Forwarded Message Subject: jdk/submit maintenance, July 23rd - July 27th Date: Wed, 22 Jul 2020 15:26:30 -0400 From: Stanislav Smirnov To: jdk-dev Hello, A planned maintenance will happen this week, July 23rd - 27th. jdk/submit builds will not be available starting tomorrow (Thursday 23rd) at 10am PT / 17:00 GMT. The system should be back online by 10am PT / 17:00 GMT, July 27th. Best regards, Stanislav Smirnov
RFR (S) 8249822: SymbolPropertyTable creates an extra OopHandle per entry
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
Hi Goetz, > Thanks for the quick reply. Yes, this time it didn't take that long... [... snip ...] > > > > > I understand you annotate at safepoints where the escape analysis > > > > > finds out that an object is "better" than global escape. > > > > > This are the cases where the analysis identifies optimization > > > > > opportunities. These annotations are then used to deoptimize > > > > > frames and the objects referenced by them. > > > > > Doesn't this overestimate the optimized > > > > > objects? E.g., eliminate_alloc_node has many cases where it bails > > > > > out. > > > > > > > > Yes, the implementation is conservative, but it is comparatively simple > > and > > > > the additional debug > > > > info is just 2 flags per safepoint. > > > Thanks. It also helped that you explained to me offline that > > > there are more optimizations than only lock elimination and scalar > > > replacement done based on the ea information. > > > The ea refines the IR graph with allows follow up optimizations > > > which can not easily be tracked back to the escaping objects or > > > the call sites where they do not escape. > > > Thus, if there are non-global escaping objects, you have to > > > deoptimize the frame. > > > Did I repeat that correctly? > > > > Mostly, but there are also cases where deoptimization is required if and > > only > > if ea-local objects > > are passed as arguments. This is the case when values are not read directly > > from a frame, but from a callee frame. > Hmm, don't get this completely, but ok. Let C be a callee frame of B which is a callee of A. If you use JVMTI to read an object reference from a local variable of C then the implementation of JDK-8227745 deoptimizes A if it passes any ea-local as argument, because the reference could be ea-local in A and there might be optimizations that are invalid after the escape state change. > > > > Accesses to instance > > > > members or array elements can be optimized as well. > > > You mean the compiler can/will ignore volatile or memory ordering > > > requirements for non-escaping objects? Sounds reasonable to do. > > > > Yes, for instance. Also without volatile modifiers it will eliminate > > accesses. > > Here is an example: > > Method A has a NoEscape allocation O that is not scalar replaced. A calls > > Method B, which is not > > inlined. When you use your debugger to break in B, then modify a field of O, > > then this modification > > would have no effect without deoptimization, because the jit assumes that B > > cannot modify O without > > a reference to it. > Yes, A can keep O in a register, while the JVMTI thread would write to > the location in the stack where the local is held (if it was written back). Not quite. It is the value of the field of O that is in a register not the reference to O itself. The agent changes the field's value in the /java heap/ (remember: O is _not_ scalar replaced), but the fields value is not reloaded after return from B. > > > > > Syncronization: looks good. I think others had a look at this before. > > > > > > > > > > EscapeBarrier::deoptimize_objects_internal() > > > > > The method name is misleading, it is not used by > > > > > deoptimize_objects(). > > > > > Also, method with the same name is in Deopitmization. > > > > > Proposal: deoptimize_objects_thread() ? > > > > > > > > Sorry, but I don't see, why it would be misleading. > > > > What would be the meaning of 'deoptimize_objects_thread'? I don't > > > > understand that name. > > > 1. I have no idea why it's called "_internal". Because it is private? > > >By the name, I would expect that EscapeBarrier::deoptimize_objects() > > >calls it for some internal tasks. But it does not. > > > > Well, I'd say it is pretty internal, what's happening in that method. So > > IMHO > > the suffix _internal > > is a match. > > > > > 2. My proposal: deoptimize_objects_all_threads() iterates all threads > > > and calls deoptimize_objects(_one)_thread(thread) for each of these. > > > That's how I would have named it. > > > But no bike shedding, if you don't see what I mean it's not obvious. > > Ok. We could have a quick call, too, if you like. > Ok, I think I have understood the remaining points. I'm fine with this > so far. Thanks again and best regards, Richard. -Original Message- From: Lindenmaier, Goetz Sent: Mittwoch, 22. Juli 2020 18:22 To: Reingruber, Richard ; serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net Subject: RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents Hi Richard, Thanks for the quick reply. > > > With DeoptimizeObjectsALot enabled internal threads are started that > > > deoptimize frames and > > > objects. The number of threads started are given with > > > DeoptimizeObjectsALotThreadCountAll and > > > DeoptimizeObjectsALotThreadCountSingle. The former targets all > existing > > > t
RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents
Hi Goetz,
> > I'll answer to the obvious things in this mail now.
> > I'll go through the code thoroughly again and write
> > a review of my findings thereafter.
> As promised a detailed walk-throug, but without any major findings:
> c1_IR.hpp: ok
> ci_Env.h|cpp: ok
> compiledMethod.cpp, nmethod.cpp: ok
> debugInfoRec.h|cpp: ok
> scopeDesc.h|cpp ok
> compileBroker.h|cpp:
> Maybe a bit of documentation how and why you start
> the threads? I had expected there are two test
> scenarios run after each other, but now I understand 'Single'
> and 'All' run simultaneously. Well, this really is a stress test!
> Also good the two variants of depotimization are
> stressed against each other.
> Besides that really nice it's all in one place.
Done.
> rootResolver.cpp: ok
> jvmciCodeInstaller.cpp: ok
> c2compiler.cpp: The essence of this change! Just one line :)
> Great!
:)
> callnode.hpp ok
> escape.h|cpp ok
> macro.cpp
> 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()
> 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?
> matcher.cpp ok
> output.cpp:1071
> Please break the long line.
Done.
> jvmtiCodeBlobEvents.cpp ok
> jvmtiEnv.cpp
> MaxJavaStackTraceDepth is only documented to affect
> the exceptions stack trace depth, not to limit jvmti
> operations. Therefore I wondered why it is used here.
> Non of your business, but the flag should
> document this in globals.hpp, too.
> 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.
> 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).
> 2805, 2929, 2946ff, break lines
> deoptimization.hpp
> 158, 174, 176 ... I would break lines too, but here you are in
> good company :)
Done.
> globals.hpp ok
> mutexLocker.h|cpp ok
> objectMonitor.cpp ok
> thread.cpp
> 2631 typo: sapfepont --> safepoint
Done.
> thread.hpp ok
> thread.inline.hpp ok
> vframe.cpp ok
> vframe_hp.cpp 458ff break lines
> vframe_hp.hpp ok
> macros.hpp ok
> TEST.ROOT ok
> WhiteBox.java ok
> 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.
> libIterateHeapWithEscapeAnalysisEnabled.c ok
> EATests.java
> This is a very elaborate test.
> I found a row of test cases illustrating issues
> we talked about before. Really helpful!
> 1311: TypeO materialize -> materialized
Found and fix typo at line 1369.
(Probably the cursor was on 1311 and your eyes on 1369 ;))
> 1640: setting local variable i triggers always deoptimization
> --> setting local variable i always triggers deoptimization
Fixed.
> 2176: dontinline_calee --> dontinline_callee
> 2510: poping --> popping ... but I'm not sure here.
Done.
> https://www.urbandictionary.com/define.php?term=poping
> poping
> Drinking large amounts of Dextromethorphan Hydrobromide (DXM)based cough
> syrup, and then embarking on an adventure while wandering around
> neighborhoods or parks all night. This is usually done while listening to
> Punk rock music from a portable jambox.
> ;)
> Don’t do it! 😊
OMG! How come you know?! ;)
> EATestsJVMTI.java
> I think you can just copy this test description into the other
> test. You can have two @test comments, they will be treated
> as separate tests. The @requires will be evaluated accordingly.
> For an example see
> test/hotspot/jtreg/runtime/exceptionMs
Re: [15?] RFR (S): 8249192: MonitorInfo stores raw oops across safepoints
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. 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. jdk16: http://cr.openjdk.java.net/~tschatzl/8249192/webrev.2/ (full) Same comments as for the JDK15 version. I also compared the two patches using jfilemerge. Most of what I have above are nits so I'm good with both of these patches. I also looked for lifetime issues with the new ResourceMarks and HandleMarks, but I don't see any issues. Dan On 7/22/20 4:14 AM, Thomas Schatzl wrote: Hi Serguei, thanks for your review. On 22.07.20 04:13, serguei.spit...@oracle.com wrote: Hi Thomas, The fix looks okay to me. The 15 fix is identical to 16. no :) It is very subtle. As mentioned, in jvmtiEnvBase.cpp in the original code, in jdk15 we had: line 1029: ResourceMark rm; in jdk16: line 1008: ResourceMark rm(current_thread); Both lines were ultimately removed with this recent change, but still cause merge errors if you port the patch from one version to the other. Sorry if that has been confusing. This file finally was not changed: src/hotspot/share/runtime/vframe_hp.cpp . This is an artifact of webrev in conjunction with mq because the original change had some modifications which were retracted in the -1 webrev there. There will not be any change in any push for that file. Several files need a copyright comment update. Provided new webrevs with only comment updates below. Did not do new testing just for these comment updates though. What tests do you run? We need at least tier3 to make sure there are no regressions in the JVMTI and JDI tests. The jdk15 .1 patch has been tested with 1.2k of that LocalsAndOperands test with the options that originally reproduced it in 1/100 cases. hs-tier1-5, no failures at all. jdk16 has had testing with the .0 patch doing 1.8k of LocalsAndOperands.java, tier1-5, and tier8 with JDK-8249676 reinstated that earlier caused lots of issues there (and none without). Since there has been no substantial change in how the patch works, only some refactoring, so I think these are still valid. See the internal comments in the CR for links. New webrevs: jdk15: http://cr.openjdk.java.net/~tschatzl/8249192/webrev.jdk15.2/ (full) http://cr.openjdk.java.net/~tschatzl/8249192/webrev.jdk15.1_to_2/ (diff) jdk16: http://cr.openjdk.java.net/~tschatzl/8249192/webrev.2/ (full) http://cr.openjdk.java.net/~tschatzl/8249192/webrev.1_to_2/ (diff) Thanks, Thomas
Re: RFR: 8216324: GetClassMethods is confused by the presence of default methods in super interfaces
Thank you, Serguei and Alex, for reviewing this change. I will apply suggested
corrections before pushing the fix.
Best regards,
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: "serguei.spit...@oracle.com"
Date: Monday, July 20, 2020 at 6:48 PM
To: Alex Menkov , Daniil Titov
, serviceability-dev
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/GetClassMethods/InterfaceDefMethods.java.html
I like the test simplicity.
Default methods are always in interfaces.
I'd suggest to rename the test to something like: DefaultMethods.java or
OverpassMethods.java.
http://cr.openjdk.java.net/~dtitov/8216324/webrev.01/test/hotspot/jtreg/serviceability/jvmti/GetClassMethods/libInterfaceDefMethods.cpp.html
44 if (options
RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents
Hi Richard, Thanks for the quick reply. > > > With DeoptimizeObjectsALot enabled internal threads are started that > > > deoptimize frames and > > > objects. The number of threads started are given with > > > DeoptimizeObjectsALotThreadCountAll and > > > DeoptimizeObjectsALotThreadCountSingle. The former targets all > existing > > > threads whereas the > > > latter operates on a single thread selected round robin. > > > > > > I removed the mode where deoptimizations were performed at every nth > > > exit from the runtime. I never used it. > > > Do I get it right? You have a n:1 and a n:all test scenario. > > n:1: n threads deoptimize 1 Jana threadwhere n => DOALThreadCountSingle > > n:m: n threads deoptimize all Java threads where n = DOALThreadCountAll? > > Not quite. > > -XX:+DeoptimizeObjectsALot // required > -XX:DeoptimizeObjectsALotThreadCountAll=m > -XX:DeoptimizeObjectsALotThreadCountSingle=n > > Will start m+n threads. Each operating on all existing JavaThreads using > EscapeBarriers. The > difference between the 2 thread types is that one distinct EscapeBarrier > targets either just a > single thread or all exisitng threads at onece. If just one single thread is > targeted per > EscapeBarrier, then it is not always the same thread, but threads are selected > round robin. So there > will be n threads selecting independently single threads round robin per > EscapeBarrier and m threads > that target all threads in every EscapeBarrier. Ok, yes, that is how I understood it. > > > * EscapeBarrier::sync_and_suspend_one(): use a direct handshake and > > > execute it always independently > > > of is_thread_fully_suspended(). > > Is this also a performance optimization? > > Maybe a minor one. OK > > > * JavaThread::wait_for_object_deoptimization(): > > > - Bugfix: the last check of is_obj_deopt_suspend() must be /after/ the > > > safepoint check! This > > > caused issues with not walkable stacks with DeoptimizeObjectsALot. > > OK. As I understand, there was one safepoint check in the old version, > > now there is one in each iteration. I assume this is intended, right? > > Yes it is. The important thing here is (A) a safepoint check is needed /after/ > leaving a safe state > (_thread_in_native, _thread_blocked). (B) Shared variables that are modified > at safepoints or with handshakes need to be reread /after/ the safepoint > check. > > BTW: I only noticed now that since JDK-8240918 JavaThreads themselves > must disarm their polling > page. Originally (before handshakes) this was done by the VM thread. With > handshakes it was done by > the thread executing the handshake op. This was changed for > OrderAccess::cross_modify_fence() where > the poll is left armed if the thread is in native and sice JDK-8240918 it is > always left armed. So > when a thread leaves a safe state (native, blocked) and there was a > handshake/vm op, it will always > call SafepointMechanism::block_if_requested_slow(), even if the > handshake/vm operation have been > processed already and everybody else is happyly executing bytecodes :) Ok. > Still (A) and (B) hold. > > > - Added limited spinning inspired by HandshakeSpinYield to fix > > > regression in > > > microbenchmark [1] > > Ok. Nice improvement, nice catch! > > Yes. It certainly took some time to find out. > > > > > > > I refer to some more changes answering your questions and comments > inline > > > below. > > > > > > Thanks, > > > Richard. > > > > > > [1] Microbenchmark: > > > > http://cr.openjdk.java.net/~rrich/webrevs/2019/8227745/webrev.6.microbe > nchmark/ > > > > > > > > > I understand you annotate at safepoints where the escape analysis > > > > finds out that an object is "better" than global escape. > > > > This are the cases where the analysis identifies optimization > > > > opportunities. These annotations are then used to deoptimize > > > > frames and the objects referenced by them. > > > > Doesn't this overestimate the optimized > > > > objects? E.g., eliminate_alloc_node has many cases where it bails > > > > out. > > > > > > Yes, the implementation is conservative, but it is comparatively simple > and > > > the additional debug > > > info is just 2 flags per safepoint. > > Thanks. It also helped that you explained to me offline that > > there are more optimizations than only lock elimination and scalar > > replacement done based on the ea information. > > The ea refines the IR graph with allows follow up optimizations > > which can not easily be tracked back to the escaping objects or > > the call sites where they do not escape. > > Thus, if there are non-global escaping objects, you have to > > deoptimize the frame. > > Did I repeat that correctly? > > Mostly, but there are also cases where deoptimization is required if and only > if ea-local objects > are passed as arguments. This is the case when values are not read directly > from a frame, but from a callee frame. Hmm, don't get this completely, but ok.
Re: 8248362: JVMTI frame operations should use Thread-Local Handshake
Thanks David! Your suggestion seems to work fine on submit repo. I will send review request. Yasumasa On 2020/07/22 21:29, David Holmes wrote: On 22/07/2020 6:19 pm, Yasumasa Suenaga wrote: On 2020/07/22 16:57, David Holmes wrote: Hi Yasumasa, On 22/07/2020 5:39 pm, Yasumasa Suenaga wrote: Hi all, I'm working for fixing JDK-8248362, but I saw some errors on submit repo. Someone can share the details of mach5-one-ysuenaga-JDK-8248362-20200722-0550-12850261 ? I wonder why build task of linux-x64 was failed because I can do it on my Fedora 32 box. [2020-07-22T06:21:49,141Z] ./open/src/hotspot/share/prims/jvmtiThreadState.cpp:222:45: error: no member named 'active_handshaker' in 'JavaThread' [2020-07-22T06:21:49,142Z] current_thread == get_thread()->active_handshaker(), [2020-07-22T06:21:49,142Z] ^ Thanks David! This statement is in guarantee(), so it seems to be failed to build for production VM. guarantee() call has been introduced in JDK-6471769, originally it was assert() call. Can we replace guarantee() to assert() at this point? or are there methods to detect the call is happened in direct handshake without active_handshaker()? I would replace with assert. There's no non-debug query for the handshaker. David - Thanks, Yasumasa David - Thanks, Yasumasa
RFR: 8248362: JVMTI frame operations should use Thread-Local Handshake
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: [15?] RFR (S): 8249192: MonitorInfo stores raw oops across safepoints
On 7/22/20 8:11 AM, coleen.phillim...@oracle.com wrote: On 7/22/20 4:21 AM, Thomas Schatzl wrote: Hi, On 22.07.20 02:42, David Holmes wrote: Hi Thomas, I've looked at the incremental update and I am happy with that. In the response to Serguei's review there were some comment updates and new webrevs. I also, prompted by you mentioning it, took a deeper look at the biased-locking code to ensure it also keeps the MonitorInfo's thread-confined, and to see whether the handshake versions could themselves be susceptible to interference from safepoints (which they can't as far as I can determine). And that all seems fine. Thanks for looking at this again in more detail. I couldn't spot problems either, but this is not code I am proficient with. As per offline discussions I know that there has been an alternate proposal for a completely localized fix in the stackwalker code that simply retrieves the list of monitors, uses the length to create the array, then re-retrieves the list of monitors to populate the array (the length of which can't change as we are dealing with the current thread). My only concern with that approach is the performance impact if we have deep stacks with lots of monitors. There is a microbenchmark for StackWalker in the repo: open/test/micro/org/openjdk/bench/java/lang/StackWalkBench.java but it doesn't test anything to do with monitor usage. Ultimately I am good with either change, as long as it's being fixed. I initially thought about this solution too, but had the same concerns. Stacks can be really deep particularly with some frameworks (maybe not fully materialized) but idk the actual impact of doing the walk twice. Potentially you could have different fixes for different versions. Yes. These patches look good to me for JDK 15 and 16. We'll open an RFE to consider the alternate patch after more performance testing for future versions, but this fixes the problem and will not be difficult to backport to JDK 11. Coleen, So it looks like you, David and Serguei are comfortable with the larger patch for both JDK15 and JDK16. That's good news! It's also good news that you think this not be difficult to backport to JDK11. For a post RDP2 push to JDK15, the bug's "Fix in Release" value will have to be changed to 15 and a request for approval made. I've attached Mark R's email about the process. Follow the links and submit the approval requests. As for the alternate fix, I only came up with it as a lower risk alternative that could be pushed this late in the JDK15 cycle and would be easy to push to JDK11u-oracle. Right now it appears that we won't need it, but just in case we get push back on JDK15 approval, I'll finish the due diligence testing. Thomas, thanks for tackling this issue and for your patience in the review process. Also, thanks for the GC debugging patch! Dan Thank you! Coleen Thanks, Thomas --- Begin Message --- Per the JDK 15 schedule [1], we are now in Rampdown Phase Two. The overall feature set is frozen. No further JEPs will be targeted to this release. Per the JDK Release Process [2] we now turn our focus to P1 and P2 bugs, which can be fixed with approval [3]. Late enhancements are still possible, with approval [4], but the bar is now extraordinarily high. If you’re responsible for any of the bugs on the RDP 2 candidate-bug list [5] then please see JEP 3 for guidance on how to handle them. - Mark [1] https://openjdk.java.net/projects/jdk/15/#Schedule [2] https://openjdk.java.net/jeps/3 [3] https://openjdk.java.net/jeps/3#Fix-Request-Process [4] https://openjdk.java.net/jeps/3#Late-Enhancement-Request-Process [5] https://j.mp/jdk-rdp-2 --- End Message ---
Re: RFR (M) 8249768: Move static oops and NullPointerException oops from Universe into OopStorage
On 7/22/20 8:44 AM, Erik Österlund wrote: Hi Coleen, This looks good to me. It's still a bit shady to me that assignment of OopHandles overwrites the oop*, potentially causing memory leaks (the previous oop* is not released). Therefore, it is implied that setters are only used once, to store a new handle over an uninitialized handle. But we can fix the safety of this in another patch. Thank you, Erik, and thank you for suggesting an assert in the assignment operator. These assignments do not hit that assert but there were others that did, so I have a separate patch that I'm working on. See https://bugs.openjdk.java.net/browse/JDK-8249822 Thank you for reviewing this. Coleen Thanks, /Erik On 2020-07-20 22:53, coleen.phillim...@oracle.com wrote: Summary: Move static oops into OopStorage and make NPE oops an objArrayOop. I've broken up moving oops in Universe to OopStorage into several parts. This change moves the global static oops. These OopHandles are not released. This has been tested with tier1-3 and on also remote-build -b linux-arm32,linux-ppc64le-debug,linux-s390x-debug,linux-x64-zero. open webrev at http://cr.openjdk.java.net/~coleenp/2020/8249768.01/webrev bug link https://bugs.openjdk.java.net/browse/JDK-8249768 Thanks, Coleen
Re: RFR (M) 8249768: Move static oops and NullPointerException oops from Universe into OopStorage
On 7/22/20 8:42 AM, David Holmes wrote:
On 22/07/2020 9:50 pm, coleen.phillim...@oracle.com wrote:
On 7/22/20 2:32 AM, David Holmes wrote:
Hi Coleen,
On 22/07/2020 12:19 am, coleen.phillim...@oracle.com wrote:
On 7/20/20 10:47 PM, David Holmes wrote:
Hi Coleen,
cc'ing serviceability due to SA changes.
On 21/07/2020 6:53 am, coleen.phillim...@oracle.com wrote:
Summary: Move static oops into OopStorage and make NPE oops an
objArrayOop.
I've broken up moving oops in Universe to OopStorage into several
parts. This change moves the global static oops. These
OopHandles are not released.
Overall looks good. But two things ...
1. naming
! // preallocated error objects (no backtrace)
! static OopHandle _out_of_memory_error;
// array of preallocated error objects with backtrace
! static OopHandle _preallocated_out_of_memory_error_array;
Both of these are pre-allocated arrays of OopHandles, differing
only in whether the underlying OOME has a backtrace or not. I find
the newly introduced _out_of_memory_error unclear in that regard.
At a minimum could _out_of_memory_error become
_out_of_memory_errors ? But ideally can we name these two arrays
in a more distinguishable way?
I put this code in functions next to each other because it was
confusing. The _out_of_memory_error array is really preallocated
throwables, that are used to fill in the message for
preallocated_out_of_memory_errors if there are enough of the latter
left.
I could rename _out_of_memory_error =>
_out_of_memory_error_throwables ?
That doesn't really help. As I said both of these variables are
arrays of pre-allocated OOME instances (which are throwables) - the
only difference is one set is single-use (as it can have its
backtrace set) while the other is reusable. The existing variable
_preallocated_out_of_memory_error_array
tells you clearly it is an array of preallocated OOME instances (but
doesn't saying anything about the backtrace or being single-use).
The problem is that that is exactly what _out_of_memory_error is as
well, but the name _out_of_memory_error doesn't convey that it is an
array, nor that anything is pre-allocated (and also nothing about
backtraces or re-usability). So given we now have two arrays of
extremely similar things, it would be clearer to give these clearer
names. If we want to keep the existing
_preallocated_out_of_memory_error_array
name, then the new array should indicate how it differs e.g.
_reusable_preallocated_out_of_memory_error_array
What do you think?
This confuses me more than the code does. Which array is this? This
is a terrible name for whichever one it is (I guess the original
_out_of_memory_error). I don't think it's interesting to have the
name _array in it, but being plural does tell you what it is, like
_out_of_memory_errors.
Yes at least being plural is essential to realize it is actually an
array.
The implementation is a bit weird and some long name isn't going to
help anyone. The abstraction that this is _out_of_memory_errors is
all anyone outside this implementation needs to know.
My point, which is obviously not getting across, is that you now have
two arrays of these out-of-memory-errors that are almost identical,
except one is used for one purpose and one used for another, but the
variable names don't give you any clue about this.
I actually understand this, but the suggested names don't help. You
really need to look at the code and the comments in universe.hpp to see
the distinction. I don't think we can provide more illumination with
long names. Since I moved the functions next to each other, it makes
more sense when one reads it.
But lets' just add the 's' and move on. :)
Thanks, I added the 's' and fixed the formatting. Thank you for
reviewing this.
Coleen
Cheers,
David
-
I also spotted a minor nit:
187 oop Universe::system_thread_group() { return
_system_thread_group.resolve(); }
188 void Universe::set_system_thread_group(oop group) {
_system_thread_group = OopHandle(vm_global(), group); }
Extra spaces after oop on L187.
Ok I'll fix the spacing.
Thanks,
Coleen
Thanks,
David
-
2. SA
You've simply deleted all the SA/vmstructs code that referenced
the oops that are no longer present. Does the SA not care about
these things and need access to them? (I don't know hence cc to
serviceability folk.)
Yes, the SA code was never used, so I deleted it. SA might need in
oop inspection to add walking Universe::vm_global() OopStorage area
to find where oops come from if there's an error but there doesn't
seem to be any other reason for SA to use these oops.
Thanks,
Coleen
Thanks,
David
-
This has been tested with tier1-3 and on also remote-build -b
linux-arm32,linux-ppc64le-debug,linux-s390x-debug,linux-x64-zero.
open webrev at
http://cr.openjdk.java.net/~coleenp/2020/8249768.01/webrev
bug link https://bugs.openjdk.java.net/browse/JDK-
Re: RFR (M) 8249768: Move static oops and NullPointerException oops from Universe into OopStorage
On 22/07/2020 9:50 pm, coleen.phillim...@oracle.com wrote:
On 7/22/20 2:32 AM, David Holmes wrote:
Hi Coleen,
On 22/07/2020 12:19 am, coleen.phillim...@oracle.com wrote:
On 7/20/20 10:47 PM, David Holmes wrote:
Hi Coleen,
cc'ing serviceability due to SA changes.
On 21/07/2020 6:53 am, coleen.phillim...@oracle.com wrote:
Summary: Move static oops into OopStorage and make NPE oops an
objArrayOop.
I've broken up moving oops in Universe to OopStorage into several
parts. This change moves the global static oops. These OopHandles
are not released.
Overall looks good. But two things ...
1. naming
! // preallocated error objects (no backtrace)
! static OopHandle _out_of_memory_error;
// array of preallocated error objects with backtrace
! static OopHandle _preallocated_out_of_memory_error_array;
Both of these are pre-allocated arrays of OopHandles, differing only
in whether the underlying OOME has a backtrace or not. I find the
newly introduced _out_of_memory_error unclear in that regard. At a
minimum could _out_of_memory_error become _out_of_memory_errors ?
But ideally can we name these two arrays in a more distinguishable way?
I put this code in functions next to each other because it was
confusing. The _out_of_memory_error array is really preallocated
throwables, that are used to fill in the message for
preallocated_out_of_memory_errors if there are enough of the latter
left.
I could rename _out_of_memory_error =>
_out_of_memory_error_throwables ?
That doesn't really help. As I said both of these variables are arrays
of pre-allocated OOME instances (which are throwables) - the only
difference is one set is single-use (as it can have its backtrace set)
while the other is reusable. The existing variable
_preallocated_out_of_memory_error_array
tells you clearly it is an array of preallocated OOME instances (but
doesn't saying anything about the backtrace or being single-use). The
problem is that that is exactly what _out_of_memory_error is as well,
but the name _out_of_memory_error doesn't convey that it is an array,
nor that anything is pre-allocated (and also nothing about backtraces
or re-usability). So given we now have two arrays of extremely similar
things, it would be clearer to give these clearer names. If we want to
keep the existing
_preallocated_out_of_memory_error_array
name, then the new array should indicate how it differs e.g.
_reusable_preallocated_out_of_memory_error_array
What do you think?
This confuses me more than the code does. Which array is this? This is
a terrible name for whichever one it is (I guess the original
_out_of_memory_error). I don't think it's interesting to have the name
_array in it, but being plural does tell you what it is, like
_out_of_memory_errors.
Yes at least being plural is essential to realize it is actually an array.
The implementation is a bit weird and some long
name isn't going to help anyone. The abstraction that this is
_out_of_memory_errors is all anyone outside this implementation needs to
know.
My point, which is obviously not getting across, is that you now have
two arrays of these out-of-memory-errors that are almost identical,
except one is used for one purpose and one used for another, but the
variable names don't give you any clue about this.
But lets' just add the 's' and move on. :)
Cheers,
David
-
I also spotted a minor nit:
187 oop Universe::system_thread_group() { return
_system_thread_group.resolve(); }
188 void Universe::set_system_thread_group(oop group) {
_system_thread_group = OopHandle(vm_global(), group); }
Extra spaces after oop on L187.
Ok I'll fix the spacing.
Thanks,
Coleen
Thanks,
David
-
2. SA
You've simply deleted all the SA/vmstructs code that referenced the
oops that are no longer present. Does the SA not care about these
things and need access to them? (I don't know hence cc to
serviceability folk.)
Yes, the SA code was never used, so I deleted it. SA might need in
oop inspection to add walking Universe::vm_global() OopStorage area
to find where oops come from if there's an error but there doesn't
seem to be any other reason for SA to use these oops.
Thanks,
Coleen
Thanks,
David
-
This has been tested with tier1-3 and on also remote-build -b
linux-arm32,linux-ppc64le-debug,linux-s390x-debug,linux-x64-zero.
open webrev at
http://cr.openjdk.java.net/~coleenp/2020/8249768.01/webrev
bug link https://bugs.openjdk.java.net/browse/JDK-8249768
Thanks,
Coleen
Re: 8248362: JVMTI frame operations should use Thread-Local Handshake
On 22/07/2020 6:19 pm, Yasumasa Suenaga wrote: On 2020/07/22 16:57, David Holmes wrote: Hi Yasumasa, On 22/07/2020 5:39 pm, Yasumasa Suenaga wrote: Hi all, I'm working for fixing JDK-8248362, but I saw some errors on submit repo. Someone can share the details of mach5-one-ysuenaga-JDK-8248362-20200722-0550-12850261 ? I wonder why build task of linux-x64 was failed because I can do it on my Fedora 32 box. [2020-07-22T06:21:49,141Z] ./open/src/hotspot/share/prims/jvmtiThreadState.cpp:222:45: error: no member named 'active_handshaker' in 'JavaThread' [2020-07-22T06:21:49,142Z] current_thread == get_thread()->active_handshaker(), [2020-07-22T06:21:49,142Z] ^ Thanks David! This statement is in guarantee(), so it seems to be failed to build for production VM. guarantee() call has been introduced in JDK-6471769, originally it was assert() call. Can we replace guarantee() to assert() at this point? or are there methods to detect the call is happened in direct handshake without active_handshaker()? I would replace with assert. There's no non-debug query for the handshaker. David - Thanks, Yasumasa David - Thanks, Yasumasa
Re: RFR (M) 8249650: Optimize JNIHandle::make_local thread variable usage
Ok, looks good to me.
Colen
On 7/21/20 10:46 PM, David Holmes wrote:
Hi Coleen,
On 22/07/2020 4:01 am, coleen.phillim...@oracle.com wrote:
This looks like a nice cleanup.
Thanks for looking at this.
http://cr.openjdk.java.net/~dholmes/8249650/webrev.v2/src/hotspot/share/runtime/jniHandles.cpp.udiff.html
I'm wondering why you took out the NULL return for make_local()
without a thread argument? Here you may call Thread::current()
unnecessarily.
jobject JNIHandles::make_local(oop obj) {
- if (obj == NULL) {
- return NULL; // ignore null handles
- } else {
- Thread* thread = Thread::current();
- assert(oopDesc::is_oop(obj), "not an oop");
- assert(!current_thread_in_native(), "must not be in native");
- return thread->active_handles()->allocate_handle(obj);
- }
+ return make_local(Thread::current(), obj);
}
I was simply using a standard call forwarding pattern to avoid code
duplication. I suspect passing NULL is very rare so the unnecessary
Thread::current() call is not an issue. Otherwise, if not NULL, the
NULL check would happen twice (unless I keep the duplicated
implementations).
Beyond the scope of this fix, but it'd be cool to not have a version
that doesn't take thread, since there may be many more callers that
already have Thread::current().
Indeed! And in fact I had missed a number of these in jvm.cpp and
jni.cpp so I have fixed those. I've filed a RFE for other cases:
https://bugs.openjdk.java.net/browse/JDK-8249837
Updated webrev:
http://cr.openjdk.java.net/~dholmes/8249650/webrev.v3/
If this passes tier 1-3 re-testing then I plan to push.
Thanks,
David
-
Coleen
On 7/20/20 1:53 AM, David Holmes wrote:
Hi Kim,
Thanks for looking at this.
Updated webrev at:
http://cr.openjdk.java.net/~dholmes/8249650/webrev.v2/
On 20/07/2020 3:22 pm, Kim Barrett wrote:
On Jul 20, 2020, at 12:16 AM, David Holmes
wrote:
Subject line got truncated by accident ...
On 20/07/2020 11:06 am, David Holmes wrote:
Bug: https://bugs.openjdk.java.net/browse/JDK-8249650
webrev: http://cr.openjdk.java.net/~dholmes/8249650/webrev/
This is a simple cleanup that touches files across a number of VM
areas - hence the cross-post.
Whilst working on a different JNI fix I noticed that in most
cases in jni.cpp we were using the following form of make_local:
JNIHandles::make_local(env, obj);
and what that form does is first extract the thread from the JNIEnv:
JavaThread* thread = JavaThread::thread_from_jni_environment(env);
return thread->active_handles()->allocate_handle(obj);
but there is also another, faster, variant for when you already
have the "thread":
jobject JNIHandles::make_local(Thread* thread, oop obj) {
return thread->active_handles()->allocate_handle(obj);
}
When you look at the JNI_ENTRY wrapper (and related JVM_ENTRY,
WB_ENTRY, UNSAFE_ENTRY etc) it has already extracted the thread
from the JNIEnv:
JavaThread*
thread=JavaThread::thread_from_jni_environment(env);
and further defined:
Thread* THREAD = thread;
so we always already have direct access to the "thread" available
(or indirect via TRAPS), and in fact we can end up removing the
make_local(JNIEnv* env, oop obj) variant altogether.
Along the way I spotted some related issues with unnecessary use
of Thread::current() when it is already available from TRAPS, and
some other cases where we extracted the JNIEnv from a thread only
to later extract the thread from the JNIEnv.
Testing: tiers 1 - 3
Thanks,
David
-
--
src/hotspot/share/classfile/javaClasses.cpp
439 JNIEnv *env = thread->jni_environment();
Since env is no longer used on the next line, move this down to where
it is used, at line 444.
Fixed.
--
src/hotspot/share/classfile/verifier.cpp
299 JNIEnv *env = thread->jni_environment();
env now seems to only be used at line 320. Move this closer.
Fixed.
--
src/hotspot/share/prims/jni.cpp
743 result = JNIHandles::make_local(THREAD, result_handle());
jni_PopLocalFrame is now using a mix of "thread" and "THREAD", where
previously it just used "thread". Maybe this change shouldn't be made?
Or can the other uses be changed to THREAD for consistency?
"thread" and "THREAD" are interchangeable for anything expecting a
"Thread*" (and somewhat surprisingly a number of API's that only
work for JavaThreads actually take a Thread*. :( ). I had choice
between trying to be file-wide consistent with the make_local calls,
versus local-code consistent, and used THREAD as it is available in
both JNI_ENTRY and via TRAPS. But I can certainly make a local
change to "thread" for local consistency.
--
src/hotspot/share/prims/jvm.cpp
The calls to JvmtiExport::pos
Re: [15?] RFR (S): 8249192: MonitorInfo stores raw oops across safepoints
On 7/22/20 4:21 AM, Thomas Schatzl wrote: Hi, On 22.07.20 02:42, David Holmes wrote: Hi Thomas, I've looked at the incremental update and I am happy with that. In the response to Serguei's review there were some comment updates and new webrevs. I also, prompted by you mentioning it, took a deeper look at the biased-locking code to ensure it also keeps the MonitorInfo's thread-confined, and to see whether the handshake versions could themselves be susceptible to interference from safepoints (which they can't as far as I can determine). And that all seems fine. Thanks for looking at this again in more detail. I couldn't spot problems either, but this is not code I am proficient with. As per offline discussions I know that there has been an alternate proposal for a completely localized fix in the stackwalker code that simply retrieves the list of monitors, uses the length to create the array, then re-retrieves the list of monitors to populate the array (the length of which can't change as we are dealing with the current thread). My only concern with that approach is the performance impact if we have deep stacks with lots of monitors. There is a microbenchmark for StackWalker in the repo: open/test/micro/org/openjdk/bench/java/lang/StackWalkBench.java but it doesn't test anything to do with monitor usage. Ultimately I am good with either change, as long as it's being fixed. I initially thought about this solution too, but had the same concerns. Stacks can be really deep particularly with some frameworks (maybe not fully materialized) but idk the actual impact of doing the walk twice. Potentially you could have different fixes for different versions. Yes. These patches look good to me for JDK 15 and 16. We'll open an RFE to consider the alternate patch after more performance testing for future versions, but this fixes the problem and will not be difficult to backport to JDK 11. Thank you! Coleen Thanks, Thomas
Re: RFR (M) 8249768: Move static oops and NullPointerException oops from Universe into OopStorage
On 7/22/20 2:32 AM, David Holmes wrote:
Hi Coleen,
On 22/07/2020 12:19 am, coleen.phillim...@oracle.com wrote:
On 7/20/20 10:47 PM, David Holmes wrote:
Hi Coleen,
cc'ing serviceability due to SA changes.
On 21/07/2020 6:53 am, coleen.phillim...@oracle.com wrote:
Summary: Move static oops into OopStorage and make NPE oops an
objArrayOop.
I've broken up moving oops in Universe to OopStorage into several
parts. This change moves the global static oops. These OopHandles
are not released.
Overall looks good. But two things ...
1. naming
! // preallocated error objects (no backtrace)
! static OopHandle _out_of_memory_error;
// array of preallocated error objects with backtrace
! static OopHandle _preallocated_out_of_memory_error_array;
Both of these are pre-allocated arrays of OopHandles, differing only
in whether the underlying OOME has a backtrace or not. I find the
newly introduced _out_of_memory_error unclear in that regard. At a
minimum could _out_of_memory_error become _out_of_memory_errors ?
But ideally can we name these two arrays in a more distinguishable way?
I put this code in functions next to each other because it was
confusing. The _out_of_memory_error array is really preallocated
throwables, that are used to fill in the message for
preallocated_out_of_memory_errors if there are enough of the latter
left.
I could rename _out_of_memory_error =>
_out_of_memory_error_throwables ?
That doesn't really help. As I said both of these variables are arrays
of pre-allocated OOME instances (which are throwables) - the only
difference is one set is single-use (as it can have its backtrace set)
while the other is reusable. The existing variable
_preallocated_out_of_memory_error_array
tells you clearly it is an array of preallocated OOME instances (but
doesn't saying anything about the backtrace or being single-use). The
problem is that that is exactly what _out_of_memory_error is as well,
but the name _out_of_memory_error doesn't convey that it is an array,
nor that anything is pre-allocated (and also nothing about backtraces
or re-usability). So given we now have two arrays of extremely similar
things, it would be clearer to give these clearer names. If we want to
keep the existing
_preallocated_out_of_memory_error_array
name, then the new array should indicate how it differs e.g.
_reusable_preallocated_out_of_memory_error_array
What do you think?
This confuses me more than the code does. Which array is this? This is
a terrible name for whichever one it is (I guess the original
_out_of_memory_error). I don't think it's interesting to have the name
_array in it, but being plural does tell you what it is, like
_out_of_memory_errors. The implementation is a bit weird and some long
name isn't going to help anyone. The abstraction that this is
_out_of_memory_errors is all anyone outside this implementation needs to
know.
I also spotted a minor nit:
187 oop Universe::system_thread_group() { return
_system_thread_group.resolve(); }
188 void Universe::set_system_thread_group(oop group) {
_system_thread_group = OopHandle(vm_global(), group); }
Extra spaces after oop on L187.
Ok I'll fix the spacing.
Thanks,
Coleen
Thanks,
David
-
2. SA
You've simply deleted all the SA/vmstructs code that referenced the
oops that are no longer present. Does the SA not care about these
things and need access to them? (I don't know hence cc to
serviceability folk.)
Yes, the SA code was never used, so I deleted it. SA might need in
oop inspection to add walking Universe::vm_global() OopStorage area
to find where oops come from if there's an error but there doesn't
seem to be any other reason for SA to use these oops.
Thanks,
Coleen
Thanks,
David
-
This has been tested with tier1-3 and on also remote-build -b
linux-arm32,linux-ppc64le-debug,linux-s390x-debug,linux-x64-zero.
open webrev at
http://cr.openjdk.java.net/~coleenp/2020/8249768.01/webrev
bug link https://bugs.openjdk.java.net/browse/JDK-8249768
Thanks,
Coleen
Re: RFR(M): 8247515: OSX pc_to_symbol() lookup does not work with core files
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 to locate
them. This was done in ps_core.c, and there are "JDK-8249779" comment
references in the code where I did this. The end result of this is
that PMap for core files won't show system libraries, and PStack for
core files won't show symbols for addresses in system libraries. Note
that currently support for PMap and PStack is disabled for OSX, but I
will shortly send out a review to enable them for OSX core files as
part of the fix for [2] JDK-8248882.
[1] https://bugs.openjdk.java.net/browse/JDK-8249779
[2] https://bugs.openjdk.java.net/browse/JDK-8248882
thanks,
Chris
On 7/14/20 5:46 PM, serguei.spit...@oracle.com wrote:
Okay, I'll wait for new webrev version to review.
Thanks,
Serguei
On 7/14/20 17:40, Chris Plummer wrote:
Hi Serguie,
Thanks for reviewing. This webrev is in limbo right now as I
discovered some issues that Kevin and I have been discussing off
line. One is that the code assumes the caller has first checked to
make sure that the symbol is in a library, where-as the actual
callers assume NULL will be returned if the symbol is not in a
library. The end result is that we end up returning a symbol, even
for address in the code cache or interpreter. So in stack traces
these frame show up as the last symbol in the last library, plus
some very large offset. I have a fix for that were now I track the
size of each library. But there is another issue with the code that
tries to discover all libraries in the core file. It misses a very
large number of system libraries. We understand why, but are not
sure of the solution. I might just change to code to only worry
about user libraries (JDK libs and other JNI libs).
Some comments below also.
On 7/14/20 4:37 PM, serguei.spit...@oracle.com wrote:
Hi Chris,
I like the suggestion from Kevin below.
I'm not sure which suggestion since I updated the webrev based on
his initial suggestion.
I have a couple of minor comments so far.
http://cr.openjdk.java.net/~cjplummer/8247515/webrev.01/src/jdk.hotspot.agent/macosx/native/libsaproc/libproc_impl.c.frames.html
313 if (!lib->next || lib->next->base >= addr) {
I wonder if the check above has to be:
313 if (!lib->next || lib->next->base > addr) {
Yes, > would be better, although this goes away with my fix that
tracks the size of each library.
http://cr.openjdk.java.net/~cjplummer/8247515/webrev.01/src/jdk.hotspot.agent/macosx/native/libsaproc/symtab.c.frames.html
417 if (offset_from_sym >= 0) { // ignore symbols that comes after
"offset"
Replace: comes => come
Ok.
thanks,
Chris
Thanks,
Serguei
On 7/8/20 03:23, Kevin Walls wrote:
Sure -- I was thinking lowest_offset_from_sym initialising
starting at a high positive integer (that would now be PTRDIFF_MAX
I think) to save a comparison with e.g. -1, you can just check if
the new offset is less than lowest_offset_from_sym
With the ptrdiff_t change you made, this all looks good to me
however you decide. 8-)
On 07/07/2020 21:17, Chris Plummer wrote:
Hi Kevin,
Thanks for the review. Yes, that lack of initialization of
lowest_offset_from_sym is a bug. I'm real surprised the compiler
didn't catch it as it will be uninitialized garbage the first
time it is referenced. Fortunately usually the eventual offset is
very small if not 0, so probably this never prevented a proper
match. I think there's also another bug:
415 uintptr_t offset_from_sym = offset - sym->offset;
"offset" is the passed in offset, essentially the address of the
symbol we are interested in, but given as an offset from the
start of the DSO. "sym->offset" is also an offset from the start
of the DSO. It could be located before or after "offset". This
means the math could result in a negative number, whic
RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents
Hi Goetz, > I'll answer to the obvious things in this mail now. > I'll go through the code thoroughly again and write > a review of my findings thereafter. Sure. If trimmed my citations to relevant parts. > > The delta includes many changes in comments, renaming of names, etc. So > > I'd like to summarize > > functional changes: > > > > * Collected all the code for the testing feature DeoptimizeObjectsALot in > > compileBroker.cpp and reworked it. > Thanks, this makes it much more compact. > > With DeoptimizeObjectsALot enabled internal threads are started that > > deoptimize frames and > > objects. The number of threads started are given with > > DeoptimizeObjectsALotThreadCountAll and > > DeoptimizeObjectsALotThreadCountSingle. The former targets all existing > > threads whereas the > > latter operates on a single thread selected round robin. > > > > I removed the mode where deoptimizations were performed at every nth > > exit from the runtime. I never used it. > Do I get it right? You have a n:1 and a n:all test scenario. > n:1: n threads deoptimize 1 Jana threadwhere n = DOALThreadCountSingle > n:m: n threads deoptimize all Java threads where n = DOALThreadCountAll? Not quite. -XX:+DeoptimizeObjectsALot // required -XX:DeoptimizeObjectsALotThreadCountAll=m -XX:DeoptimizeObjectsALotThreadCountSingle=n Will start m+n threads. Each operating on all existing JavaThreads using EscapeBarriers. The difference between the 2 thread types is that one distinct EscapeBarrier targets either just a single thread or all exisitng threads at onece. If just one single thread is targeted per EscapeBarrier, then it is not always the same thread, but threads are selected round robin. So there will be n threads selecting independently single threads round robin per EscapeBarrier and m threads that target all threads in every EscapeBarrier. > > * EscapeBarrier::sync_and_suspend_one(): use a direct handshake and > > execute it always independently > > of is_thread_fully_suspended(). > Is this also a performance optimization? Maybe a minor one. > > * JavaThread::wait_for_object_deoptimization(): > > - Bugfix: the last check of is_obj_deopt_suspend() must be /after/ the > > safepoint check! This > > caused issues with not walkable stacks with DeoptimizeObjectsALot. > OK. As I understand, there was one safepoint check in the old version, > now there is one in each iteration. I assume this is intended, right? Yes it is. The important thing here is (A) a safepoint check is needed /after/ leaving a safe state (_thread_in_native, _thread_blocked). (B) Shared variables that are modified at safepoints or with handshakes need to be reread /after/ the safepoint check. BTW: I only noticed now that since JDK-8240918 JavaThreads themselves must disarm their polling page. Originally (before handshakes) this was done by the VM thread. With handshakes it was done by the thread executing the handshake op. This was change for OrderAccess::cross_modify_fence() where the poll is left armed if the thread is in native and sice JDK-8240918 it is always left armed. So when a thread leaves a safe state (native, blocked) and there was a handshake/vm op, it will always call SafepointMechanism::block_if_requested_slow(), even if the handshake/vm operation have been processed already and everybody else is happyly executing bytecodes :) Still (A) and (B) hold. > > - Added limited spinning inspired by HandshakeSpinYield to fix regression > > in > > microbenchmark [1] > Ok. Nice improvement, nice catch! Yes. It certainly took some time to find out. > > > > I refer to some more changes answering your questions and comments inline > > below. > > > > Thanks, > > Richard. > > > > [1] Microbenchmark: > > http://cr.openjdk.java.net/~rrich/webrevs/2019/8227745/webrev.6.microbenchmark/ > > > > > I understand you annotate at safepoints where the escape analysis > > > finds out that an object is "better" than global escape. > > > This are the cases where the analysis identifies optimization > > > opportunities. These annotations are then used to deoptimize > > > frames and the objects referenced by them. > > > Doesn't this overestimate the optimized > > > objects? E.g., eliminate_alloc_node has many cases where it bails > > > out. > > > > Yes, the implementation is conservative, but it is comparatively simple and > > the additional debug > > info is just 2 flags per safepoint. > Thanks. It also helped that you explained to me offline that > there are more optimizations than only lock elimination and scalar > replacement done based on the ea information. > The ea refines the IR graph with allows follow up optimizations > which can not easily be tracked back to the escaping objects or > the call sites where they do not escape. > Thus, if there are non-global escaping objects, you have to > deoptimize the frame. > Did I repeat that correctly? Mostly, but there are also cases, where deoptimizat
Re: [15?] RFR (S): 8249192: MonitorInfo stores raw oops across safepoints
Hi, On 22.07.20 02:42, David Holmes wrote: Hi Thomas, I've looked at the incremental update and I am happy with that. In the response to Serguei's review there were some comment updates and new webrevs. I also, prompted by you mentioning it, took a deeper look at the biased-locking code to ensure it also keeps the MonitorInfo's thread-confined, and to see whether the handshake versions could themselves be susceptible to interference from safepoints (which they can't as far as I can determine). And that all seems fine. Thanks for looking at this again in more detail. I couldn't spot problems either, but this is not code I am proficient with. As per offline discussions I know that there has been an alternate proposal for a completely localized fix in the stackwalker code that simply retrieves the list of monitors, uses the length to create the array, then re-retrieves the list of monitors to populate the array (the length of which can't change as we are dealing with the current thread). My only concern with that approach is the performance impact if we have deep stacks with lots of monitors. There is a microbenchmark for StackWalker in the repo: open/test/micro/org/openjdk/bench/java/lang/StackWalkBench.java but it doesn't test anything to do with monitor usage. Ultimately I am good with either change, as long as it's being fixed. I initially thought about this solution too, but had the same concerns. Stacks can be really deep particularly with some frameworks (maybe not fully materialized) but idk the actual impact of doing the walk twice. Potentially you could have different fixes for different versions. Thanks, Thomas
Re: [15?] RFR (S): 8249192: MonitorInfo stores raw oops across safepoints
Hi Serguei, thanks for your review. On 22.07.20 04:13, serguei.spit...@oracle.com wrote: Hi Thomas, The fix looks okay to me. The 15 fix is identical to 16. no :) It is very subtle. As mentioned, in jvmtiEnvBase.cpp in the original code, in jdk15 we had: line 1029: ResourceMark rm; in jdk16: line 1008: ResourceMark rm(current_thread); Both lines were ultimately removed with this recent change, but still cause merge errors if you port the patch from one version to the other. Sorry if that has been confusing. This file finally was not changed: src/hotspot/share/runtime/vframe_hp.cpp . This is an artifact of webrev in conjunction with mq because the original change had some modifications which were retracted in the -1 webrev there. There will not be any change in any push for that file. Several files need a copyright comment update. Provided new webrevs with only comment updates below. Did not do new testing just for these comment updates though. What tests do you run? We need at least tier3 to make sure there are no regressions in the JVMTI and JDI tests. The jdk15 .1 patch has been tested with 1.2k of that LocalsAndOperands test with the options that originally reproduced it in 1/100 cases. hs-tier1-5, no failures at all. jdk16 has had testing with the .0 patch doing 1.8k of LocalsAndOperands.java, tier1-5, and tier8 with JDK-8249676 reinstated that earlier caused lots of issues there (and none without). Since there has been no substantial change in how the patch works, only some refactoring, so I think these are still valid. See the internal comments in the CR for links. New webrevs: jdk15: http://cr.openjdk.java.net/~tschatzl/8249192/webrev.jdk15.2/ (full) http://cr.openjdk.java.net/~tschatzl/8249192/webrev.jdk15.1_to_2/ (diff) jdk16: http://cr.openjdk.java.net/~tschatzl/8249192/webrev.2/ (full) http://cr.openjdk.java.net/~tschatzl/8249192/webrev.1_to_2/ (diff) Thanks, Thomas
Re: 8248362: JVMTI frame operations should use Thread-Local Handshake
On 2020/07/22 16:57, David Holmes wrote: Hi Yasumasa, On 22/07/2020 5:39 pm, Yasumasa Suenaga wrote: Hi all, I'm working for fixing JDK-8248362, but I saw some errors on submit repo. Someone can share the details of mach5-one-ysuenaga-JDK-8248362-20200722-0550-12850261 ? I wonder why build task of linux-x64 was failed because I can do it on my Fedora 32 box. [2020-07-22T06:21:49,141Z] ./open/src/hotspot/share/prims/jvmtiThreadState.cpp:222:45: error: no member named 'active_handshaker' in 'JavaThread' [2020-07-22T06:21:49,142Z] current_thread == get_thread()->active_handshaker(), [2020-07-22T06:21:49,142Z] ^ Thanks David! This statement is in guarantee(), so it seems to be failed to build for production VM. guarantee() call has been introduced in JDK-6471769, originally it was assert() call. Can we replace guarantee() to assert() at this point? or are there methods to detect the call is happened in direct handshake without active_handshaker()? Thanks, Yasumasa David - Thanks, Yasumasa
Re: 8248362: JVMTI frame operations should use Thread-Local Handshake
Hi Yasumasa, On 22/07/2020 5:39 pm, Yasumasa Suenaga wrote: Hi all, I'm working for fixing JDK-8248362, but I saw some errors on submit repo. Someone can share the details of mach5-one-ysuenaga-JDK-8248362-20200722-0550-12850261 ? I wonder why build task of linux-x64 was failed because I can do it on my Fedora 32 box. [2020-07-22T06:21:49,141Z] ./open/src/hotspot/share/prims/jvmtiThreadState.cpp:222:45: error: no member named 'active_handshaker' in 'JavaThread' [2020-07-22T06:21:49,142Z] current_thread == get_thread()->active_handshaker(), [2020-07-22T06:21:49,142Z] ^ David - Thanks, Yasumasa
8248362: JVMTI frame operations should use Thread-Local Handshake
Hi all, I'm working for fixing JDK-8248362, but I saw some errors on submit repo. Someone can share the details of mach5-one-ysuenaga-JDK-8248362-20200722-0550-12850261 ? I wonder why build task of linux-x64 was failed because I can do it on my Fedora 32 box. Thanks, Yasumasa
