Re: RFR: 8255706: The JDWP debug agent unecessarily checks for JVMTI_ERROR_INTERRUPT after calling RawMonitorEnter
On Mon, 2 Nov 2020 22:11:48 GMT, Chris Plummer wrote: > Remove code that retries if RawMonitorEnter is interrupted since that can't > happen: > > https://docs.oracle.com/en/java/javase/14/docs/specs/jvmti.html#RawMonitorEnter Marked as reviewed by alanb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1022
Integrated: 8254723: add diagnostic command to write Linux perf map file
On Tue, 20 Oct 2020 09:27:45 GMT, Nick Gasson wrote: > When using the Linux "perf" tool to do system profiling, symbol names of > running Java methods cannot be decoded, resulting in unhelpful output > such as: > > 10.52% [JIT] tid 236748 [.] 0x7f6fdb75d223 > > Perf can read a simple text file format describing the mapping between > address ranges and symbol names for a particular process [1]. > > It's possible to generate this already for Java processes using a JVMTI > plugin such as perf-map-agent [2]. However this requires compiling > third-party code and then loading the agent into your Java process. It > would be more convenient if Hotspot could write this file directly using > a diagnostic command. The information required is almost identical to > that of the existing Compiler.codelist command. > > This patch adds a Compiler.perfmap diagnostic command on Linux only. To > use, first run "jcmd Compiler.perfmap" and then "perf top" or > "perf record" and the report should show decoded Java symbol names for > that process. > > As this just writes a snapshot of the code cache when the command is > run, it will become stale if methods are compiled later or unloaded. > However this shouldn't be a big problem in practice if the map file is > generated after the application has warmed up. > > [1] > https://github.com/torvalds/linux/blob/master/tools/perf/Documentation/jit-interface.txt > [2] https://github.com/jvm-profiling-tools/perf-map-agent This pull request has now been integrated. Changeset: 50357d13 Author:Nick Gasson URL: https://git.openjdk.java.net/jdk/commit/50357d13 Stats: 171 lines in 8 files changed: 169 ins; 1 del; 1 mod 8254723: add diagnostic command to write Linux perf map file Reviewed-by: ysuenaga, sspitsyn - PR: https://git.openjdk.java.net/jdk/pull/760
RFR: 8255706: The JDWP debug agent unecessarily checks for JVMTI_ERROR_INTERRUPT after calling RawMonitorEnter
Remove code that retries if RawMonitorEnter is interrupted since that can't happen: https://docs.oracle.com/en/java/javase/14/docs/specs/jvmti.html#RawMonitorEnter - Commit messages: - RawMonitorEnter will never return JVMTI_ERROR_INTERRUPT, so don't check for it. Changes: https://git.openjdk.java.net/jdk/pull/1022/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=1022&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8255706 Stats: 10 lines in 1 file changed: 0 ins; 7 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/1022.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1022/head:pull/1022 PR: https://git.openjdk.java.net/jdk/pull/1022
Re: RFR: 8255452: Doing GC during JVMTI MethodExit event posting breaks return oop [v2]
On Mon, 2 Nov 2020 17:52:58 GMT, Erik Österlund wrote: >> src/hotspot/share/prims/jvmtiExport.cpp line 1570: >> >>> 1568: // return a flag when a method terminates by throwing an exception >>> 1569: // i.e. if an exception is thrown and it's not caught by the >>> current method >>> 1570: bool exception_exit = state->is_exception_detected() && >>> !state->is_exception_caught(); >> >> So this only applies to the case where the post_method_exit comes from >> remove_activation? Good to have it only on this path in this case. > > I'm not sure. There might be other cases, when remove_activation is called by > the exception code. That's why I didn't want to change it to just true in > this path. The post_method_exit can come from Zero interpreter: src/hotspot/share/interpreter/zero/bytecodeInterpreter.cpp: CALL_VM_NOCHECK(InterpreterRuntime::post_method_exit(THREAD)); - PR: https://git.openjdk.java.net/jdk/pull/930
Re: RFR: 8255452: Doing GC during JVMTI MethodExit event posting breaks return oop [v2]
On Mon, 2 Nov 2020 16:20:09 GMT, Coleen Phillimore wrote:
>> Erik Österlund has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Coleen CR1: Refactoring
>
> This looks better. Just to have the JRT_BLOCK be unconditional is an
> improvement.
Erik,
Thank you for the update! It looks more elegant.
One concern is that after the move of this fragment to the
post_method_exit_inner:
1614 if (state == NULL || !state->is_interp_only_mode()) {
1615 // for any thread that actually wants method exit, interp_only_mode is
set
1616 return;
1617 }
there is no guarantee that the current frame is interpreted below:
1580 if (!exception_exit) {
1581 oop oop_result;
1582 BasicType type = current_frame.interpreter_frame_result(&oop_result,
&value);
. . .
1597 if (result.not_null() && !mh->is_native()) {
1598 // We have to restore the oop on the stack for interpreter frames
1599 *(oop*)current_frame.interpreter_frame_tos_address() = result();
1600 }
Probably, extra checks for current_frame.is_interpreted_frame() in these
fragments will be sufficient.
-
PR: https://git.openjdk.java.net/jdk/pull/930
Integrated: 8255695: Some JVMTI calls in the jdwp debug agent are using FUNC_PTR instead of JVMTI_FUNC_PTR
On Fri, 30 Oct 2020 21:11:09 GMT, Chris Plummer wrote: > Use JVMTI_FUNC_PTR instead of FUNC_PTR for most JVMTI calls. See CR for > details. This pull request has now been integrated. Changeset: c7747416 Author:Chris Plummer URL: https://git.openjdk.java.net/jdk/commit/c7747416 Stats: 14 lines in 1 file changed: 0 ins; 0 del; 14 mod 8255695: Some JVMTI calls in the jdwp debug agent are using FUNC_PTR instead of JVMTI_FUNC_PTR Reviewed-by: sspitsyn, amenkov - PR: https://git.openjdk.java.net/jdk/pull/971
Integrated: 8255696: JDWP debug agent's canSuspendResumeThreadLists() should be removed
On Fri, 30 Oct 2020 20:42:24 GMT, Chris Plummer wrote: > This RFR removes the debug agent's canSuspendResumeThreadLists() function and > code that depends on it. Please see the CR for a description of why this is > being done. This pull request has now been integrated. Changeset: ceba2f85 Author:Chris Plummer URL: https://git.openjdk.java.net/jdk/commit/ceba2f85 Stats: 33 lines in 3 files changed: 0 ins; 29 del; 4 mod 8255696: JDWP debug agent's canSuspendResumeThreadLists() should be removed Reviewed-by: amenkov, sspitsyn - PR: https://git.openjdk.java.net/jdk/pull/970
Integrated: 8255694: memory leak in JDWP debug agent after calling JVMTI GetAllThreads
On Fri, 30 Oct 2020 20:30:49 GMT, Chris Plummer wrote: > The debug agent needs to deallocate memory that is allocated by calls to > GetAllThreads. There were two places were this was not being done, resulting > in a memory leak. This pull request has now been integrated. Changeset: a250716a Author:Chris Plummer URL: https://git.openjdk.java.net/jdk/commit/a250716a Stats: 4 lines in 1 file changed: 2 ins; 0 del; 2 mod 8255694: memory leak in JDWP debug agent after calling JVMTI GetAllThreads Reviewed-by: amenkov, sspitsyn - PR: https://git.openjdk.java.net/jdk/pull/968
Re: RFR: 8255695: Some JVMTI calls in the jdwp debug agent are using FUNC_PTR instead of JVMTI_FUNC_PTR
On Fri, 30 Oct 2020 21:11:09 GMT, Chris Plummer wrote: > Use JVMTI_FUNC_PTR instead of FUNC_PTR for most JVMTI calls. See CR for > details. Marked as reviewed by amenkov (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/971
Re: RFR: 8255694: memory leak in JDWP debug agent after calling JVMTI GetAllThreads [v2]
> The debug agent needs to deallocate memory that is allocated by calls to > GetAllThreads. There were two places were this was not being done, resulting > in a memory leak. Chris Plummer has updated the pull request incrementally with one additional commit since the last revision: Remove unecessary semicolon - Changes: - all: https://git.openjdk.java.net/jdk/pull/968/files - new: https://git.openjdk.java.net/jdk/pull/968/files/06063b7d..4e4dcae6 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=968&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=968&range=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/968.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/968/head:pull/968 PR: https://git.openjdk.java.net/jdk/pull/968
Re: RFR: 8255695: Some JVMTI calls in the jdwp debug agent are using FUNC_PTR instead of JVMTI_FUNC_PTR
On Sat, 31 Oct 2020 08:12:50 GMT, Serguei Spitsyn wrote: >> Use JVMTI_FUNC_PTR instead of FUNC_PTR for most JVMTI calls. See CR for >> details. > > Looks good. > Thanks, > Serguei Ping! I could use one more review. The changes are pretty trivial. Thanks! - PR: https://git.openjdk.java.net/jdk/pull/971
Re: RFR: 8255452: Doing GC during JVMTI MethodExit event posting breaks return oop [v2]
On Mon, 2 Nov 2020 16:19:59 GMT, Coleen Phillimore wrote: >> Erik Österlund has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Coleen CR1: Refactoring > > src/hotspot/share/prims/jvmtiExport.cpp line 1570: > >> 1568: // return a flag when a method terminates by throwing an exception >> 1569: // i.e. if an exception is thrown and it's not caught by the current >> method >> 1570: bool exception_exit = state->is_exception_detected() && >> !state->is_exception_caught(); > > So this only applies to the case where the post_method_exit comes from > remove_activation? Good to have it only on this path in this case. I'm not sure. There might be other cases, when remove_activation is called by the exception code. That's why I didn't want to change it to just true in this path. - PR: https://git.openjdk.java.net/jdk/pull/930
Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v2]
On Mon, 2 Nov 2020 16:52:02 GMT, Coleen Phillimore wrote: >> I thought that we didn't load the archived heap from CDS, if JVMTI heap >> walker capabilities are in place, as we didn't want this kind of >> interactions. But maybe I'm missing something, since you said having this if >> statement here made a difference. > > Now I remember. I added an assert in JvmtiTagMapTable::find() for oop != > NULL which didn't exist in the current hashmap code. The current hashmap > code just didn't find a null oop. I tracked it down to the fact that we're > finding dormant objects whose class hasn't been loaded yet. So I think we do load the archived heap from CDS. The heap walker capabilities can be added dynamically. - PR: https://git.openjdk.java.net/jdk/pull/967
Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v2]
On Mon, 2 Nov 2020 15:45:18 GMT, Erik Österlund wrote: >> Because it crashed with my changes and didn't without. I cannot recollect >> why. > > I thought that we didn't load the archived heap from CDS, if JVMTI heap > walker capabilities are in place, as we didn't want this kind of > interactions. But maybe I'm missing something, since you said having this if > statement here made a difference. Now I remember. I added an assert in JvmtiTagMapTable::find() for oop != NULL which didn't exist in the current hashmap code. The current hashmap code just didn't find a null oop. I tracked it down to the fact that we're finding dormant objects whose class hasn't been loaded yet. - PR: https://git.openjdk.java.net/jdk/pull/967
Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v2]
On Mon, 2 Nov 2020 15:18:43 GMT, Erik Österlund wrote:
>> Coleen Phillimore has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Code review comments from StefanK.
>
> test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/capability/CM02/cm02t001/cm02t001.cpp
> line 656:
>
>> 654: result = NSK_FALSE;
>> 655:
>> 656: printf("Object free events %d\n", ObjectFreeEventsCount);
>
> Is this old debug info you forgot to remove? Other code seems to use
> NSK_DISPLAY macros instead.
yes, removed it.
> src/hotspot/share/prims/jvmtiTagMap.cpp line 345:
>
>> 343:
>> 344: // Check if we have to process for concurrent GCs.
>> 345: check_hashmap(false);
>
> Maybe add a comment stating the parameter name, as was done in other
> callsites for check_hashmap.
Ok, will I run afoul of the ZGC people putting the parameter name after the
parameter and the rest of the code, it is before?
> src/hotspot/share/prims/jvmtiTagMap.cpp line 3009:
>
>> 3007: // Lock each hashmap from concurrent posting and cleaning
>> 3008: MutexLocker ml(tag_map->lock(),
>> Mutex::_no_safepoint_check_flag);
>> 3009: tag_map->hashmap()->unlink_and_post(tag_map->env());
>
> This could call unlink_and_post_locked instead of manually locking.
Ok, 2 requests. I can call that then and move the logging.
-
PR: https://git.openjdk.java.net/jdk/pull/967
Re: RFR: 8255452: Doing GC during JVMTI MethodExit event posting breaks return oop [v2]
On Mon, 2 Nov 2020 11:14:10 GMT, Erik Österlund wrote: >> The imasm::remove_activation() call does not deal with safepoints very well. >> However, when the MethodExit JVMTI event is being called, we call into the >> runtime in the middle of remove_activation(). If the value being returned is >> an object type, then the top-of-stack contains the oop. However, the GC does >> not traverse said oop in any oop map, because it is simply not expected that >> we safepoint in the middle of remove_activation(). >> >> The JvmtiExport::post_method_exit() function we end up calling, reads the >> top-of-stack oop, and puts it in a handle. Then it calls JVMTI callbacks, >> that eventually call Java and a bunch of stuff that safepoints. So after the >> JVMTI callback, we can expect the top-of-stack oop to be broken. >> Unfortunately, when we continue, we therefore end up returning a broken oop. >> >> Notably, the fact that InterpreterRuntime::post_method_exit is a JRT_ENTRY, >> is wrong, as we can safepoint on the way back to Java, which will break the >> return oop in a similar way. So this patch makes it a JRT_BLOCK_ENTRY, >> moving the transition to VM and back, into a block of code that is protected >> against GC. Before the JRT_BLOCK is called, we stash away the return oop, >> and after the JRT_BLOCK_END, we restore the top-of-stack oop. In the path >> when InterpreterRuntime::post_method_exit is called when throwing an >> exception, we don't have the same problem of retaining an oop result, and >> hence the JRT_BLOCK/JRT_BLOCK_END section is not performed in this case; the >> logic is the same as before for this path. >> >> This is a JVMTI bug that has probably been around for a long time. It >> crashes with all GCs, but was discovered recently after concurrent stack >> processing, as StefanK has been running better GC stressing code in JVMTI, >> and the bug reproduced more easily with concurrent stack processing, as the >> timings were a bit different. The following reproducer failed pretty much >> 100% of the time: >> while true; do make test JTREG="RETAIN=all" >> TEST=test/hotspot/jtreg/vmTestbase/nsk/jdi/MethodExitEvent/returnValue/returnValue003/returnValue003.java >> TEST_OPTS_JAVA_OPTIONS="-XX:+UseZGC -Xmx2g -XX:ZCollectionInterval=0.0001 >> -XX:ZFragmentationLimit=0.01 -XX:+VerifyOops -XX:+ZVerifyViews -Xint" ; done >> >> With my fix I can run this repeatedly without any more failures. I have also >> sanity checked the patch by running tier 1-5, so that it does not introduces >> any new issues on its own. I have also used Stefan's nice external GC >> stressing with jcmd technique that was used to trigger crashes with other >> GCs, to make sure said crashes no longer reproduce either. > > Erik Österlund has updated the pull request incrementally with one additional > commit since the last revision: > > Coleen CR1: Refactoring This looks better. Just to have the JRT_BLOCK be unconditional is an improvement. src/hotspot/share/prims/jvmtiExport.cpp line 1570: > 1568: // return a flag when a method terminates by throwing an exception > 1569: // i.e. if an exception is thrown and it's not caught by the current > method > 1570: bool exception_exit = state->is_exception_detected() && > !state->is_exception_caught(); So this only applies to the case where the post_method_exit comes from remove_activation? Good to have it only on this path in this case. - Marked as reviewed by coleenp (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/930
Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v2]
On Mon, 2 Nov 2020 12:50:23 GMT, Coleen Phillimore wrote: >> src/hotspot/share/prims/jvmtiTagMap.cpp line 954: >> >>> 952: o->klass()->external_name()); >>> 953: return; >>> 954: } >> >> Why is this done as a part of this RFE? Is this a bug fix that should be >> done as a separate patch? > > Because it crashed with my changes and didn't without. I cannot recollect > why. I thought that we didn't load the archived heap from CDS, if JVMTI heap walker capabilities are in place, as we didn't want this kind of interactions. But maybe I'm missing something, since you said having this if statement here made a difference. - PR: https://git.openjdk.java.net/jdk/pull/967
Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v2]
On Mon, 2 Nov 2020 15:58:15 GMT, Coleen Phillimore wrote:
>> This change turns the HashTable that JVMTI uses for object tagging into a
>> regular Hotspot hashtable - the one in hashtable.hpp with resizing and
>> rehashing. Instead of pointing directly to oops so that GC has to walk the
>> table to follow oops and then to rehash the table, this table points to
>> WeakHandle. GC walks the backing OopStorages concurrently.
>>
>> The hash function for the table is a hash of the lower 32 bits of the
>> address. A flag is set during GC (gc_notification if in a safepoint, and
>> through a call to JvmtiTagMap::needs_processing()) so that the table is
>> rehashed at the next use.
>>
>> The gc_notification mechanism of weak oop processing is used to notify Jvmti
>> to post ObjectFree events. In concurrent GCs there can be a window of time
>> between weak oop marking where the oop is unmarked, so dead (the phantom
>> load in peek returns NULL) but the gc_notification hasn't been done yet. In
>> this window, a heap walk or GetObjectsWithTags call would not find an object
>> before the ObjectFree event is posted. This is dealt with in two ways:
>>
>> 1. In the Heap walk, there's an unconditional table walk to post events if
>> events are needed to post.
>> 2. For GetObjectWithTags, if a dead oop is found in the table and posting is
>> required, we use the VM thread to post the event.
>>
>> Event posting cannot be done in a JavaThread because the posting needs to be
>> done while holding the table lock, so that the JvmtiEnv state doesn't change
>> before posting is done. ObjectFree callbacks are limited in what they can
>> do as per the JVMTI Specification. The allowed callbacks to the VM already
>> have code to allow NonJava threads.
>>
>> To avoid rehashing, I also tried to use object->identity_hash() but this
>> breaks because entries can be added to the table during heapwalk, where the
>> objects use marking. The starting markWord is saved and restored. Adding a
>> hashcode during this operation makes restoring the former markWord (locked,
>> inflated, etc) too complicated. Plus we don't want all these objects to
>> have hashcodes because locking operations after tagging would have to always
>> use inflated locks.
>>
>> Much of this change is to remove serial weak oop processing for the
>> weakProcessor, ZGC and Shenandoah. The GCs have been stress tested with
>> jvmti code.
>>
>> It has also been tested with tier1-6.
>>
>> Thank you to Stefan, Erik and Kim for their help with this change.
>
> Coleen Phillimore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Code review comments from StefanK.
Looks great in general. Great work Coleen, and thanks again for fixing this. I
like all the red lines in the GC code. I added a few nits/questions.
test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/capability/CM02/cm02t001/cm02t001.cpp
line 656:
> 654: result = NSK_FALSE;
> 655:
> 656: printf("Object free events %d\n", ObjectFreeEventsCount);
Is this old debug info you forgot to remove? Other code seems to use
NSK_DISPLAY macros instead.
src/hotspot/share/prims/jvmtiTagMap.cpp line 345:
> 343:
> 344: // Check if we have to process for concurrent GCs.
> 345: check_hashmap(false);
Maybe add a comment stating the parameter name, as was done in other callsites
for check_hashmap.
src/hotspot/share/prims/jvmtiTagMap.cpp line 3009:
> 3007: // Lock each hashmap from concurrent posting and cleaning
> 3008: MutexLocker ml(tag_map->lock(),
> Mutex::_no_safepoint_check_flag);
> 3009: tag_map->hashmap()->unlink_and_post(tag_map->env());
This could call unlink_and_post_locked instead of manually locking.
-
Changes requested by eosterlund (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/967
Integrated: 8255616: Disable AOT and Graal in Oracle OpenJDK
On Fri, 30 Oct 2020 17:40:51 GMT, Vladimir Kozlov wrote: > We shipped Ahead-of-Time compilation (the jaotc tool) in JDK 9, as an > experimental feature. We shipped Graal as an experimental JIT compiler in JDK > 10. We haven't seen much use of these features, and the effort required to > support and enhance them is significant. We therefore intend to disable these > features in Oracle builds as of JDK 16. > > We'll leave the sources for these features in the repository, in case any one > else is interested in building them. But we will not update or test them. > > We'll continue to build and ship JVMCI as an experimental feature in Oracle > builds. > > Tested changes in all tiers. > > I verified that with these changes I still able to build Graal in open repo > and run graalunit testing: > > `open$ bash test/hotspot/jtreg/compiler/graalunit/downloadLibs.sh > /mydir/graalunit_lib/` > `open$ bash configure --with-debug-level=fastdebug > --with-graalunit-lib=/mydir/graalunit_lib/ --with-jtreg=/mydir/jtreg` > `open$ make jdk-image` > `open$ make test-image` > `open$ make run-test TEST=compiler/graalunit/HotspotTest.java` This pull request has now been integrated. Changeset: 2f7d34f2 Author:Vladimir Kozlov URL: https://git.openjdk.java.net/jdk/commit/2f7d34f2 Stats: 36 lines in 4 files changed: 21 ins; 11 del; 4 mod 8255616: Disable AOT and Graal in Oracle OpenJDK Reviewed-by: iignatyev, vlivanov, iveresov, ihse - PR: https://git.openjdk.java.net/jdk/pull/960
Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v2]
On Mon, 2 Nov 2020 15:58:15 GMT, Coleen Phillimore wrote: >> This change turns the HashTable that JVMTI uses for object tagging into a >> regular Hotspot hashtable - the one in hashtable.hpp with resizing and >> rehashing. Instead of pointing directly to oops so that GC has to walk the >> table to follow oops and then to rehash the table, this table points to >> WeakHandle. GC walks the backing OopStorages concurrently. >> >> The hash function for the table is a hash of the lower 32 bits of the >> address. A flag is set during GC (gc_notification if in a safepoint, and >> through a call to JvmtiTagMap::needs_processing()) so that the table is >> rehashed at the next use. >> >> The gc_notification mechanism of weak oop processing is used to notify Jvmti >> to post ObjectFree events. In concurrent GCs there can be a window of time >> between weak oop marking where the oop is unmarked, so dead (the phantom >> load in peek returns NULL) but the gc_notification hasn't been done yet. In >> this window, a heap walk or GetObjectsWithTags call would not find an object >> before the ObjectFree event is posted. This is dealt with in two ways: >> >> 1. In the Heap walk, there's an unconditional table walk to post events if >> events are needed to post. >> 2. For GetObjectWithTags, if a dead oop is found in the table and posting is >> required, we use the VM thread to post the event. >> >> Event posting cannot be done in a JavaThread because the posting needs to be >> done while holding the table lock, so that the JvmtiEnv state doesn't change >> before posting is done. ObjectFree callbacks are limited in what they can >> do as per the JVMTI Specification. The allowed callbacks to the VM already >> have code to allow NonJava threads. >> >> To avoid rehashing, I also tried to use object->identity_hash() but this >> breaks because entries can be added to the table during heapwalk, where the >> objects use marking. The starting markWord is saved and restored. Adding a >> hashcode during this operation makes restoring the former markWord (locked, >> inflated, etc) too complicated. Plus we don't want all these objects to >> have hashcodes because locking operations after tagging would have to always >> use inflated locks. >> >> Much of this change is to remove serial weak oop processing for the >> weakProcessor, ZGC and Shenandoah. The GCs have been stress tested with >> jvmti code. >> >> It has also been tested with tier1-6. >> >> Thank you to Stefan, Erik and Kim for their help with this change. > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > Code review comments from StefanK. Shenandoah part looks good. - Marked as reviewed by zgu (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/967
Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v2]
> This change turns the HashTable that JVMTI uses for object tagging into a > regular Hotspot hashtable - the one in hashtable.hpp with resizing and > rehashing. Instead of pointing directly to oops so that GC has to walk the > table to follow oops and then to rehash the table, this table points to > WeakHandle. GC walks the backing OopStorages concurrently. > > The hash function for the table is a hash of the lower 32 bits of the > address. A flag is set during GC (gc_notification if in a safepoint, and > through a call to JvmtiTagMap::needs_processing()) so that the table is > rehashed at the next use. > > The gc_notification mechanism of weak oop processing is used to notify Jvmti > to post ObjectFree events. In concurrent GCs there can be a window of time > between weak oop marking where the oop is unmarked, so dead (the phantom load > in peek returns NULL) but the gc_notification hasn't been done yet. In this > window, a heap walk or GetObjectsWithTags call would not find an object > before the ObjectFree event is posted. This is dealt with in two ways: > > 1. In the Heap walk, there's an unconditional table walk to post events if > events are needed to post. > 2. For GetObjectWithTags, if a dead oop is found in the table and posting is > required, we use the VM thread to post the event. > > Event posting cannot be done in a JavaThread because the posting needs to be > done while holding the table lock, so that the JvmtiEnv state doesn't change > before posting is done. ObjectFree callbacks are limited in what they can do > as per the JVMTI Specification. The allowed callbacks to the VM already have > code to allow NonJava threads. > > To avoid rehashing, I also tried to use object->identity_hash() but this > breaks because entries can be added to the table during heapwalk, where the > objects use marking. The starting markWord is saved and restored. Adding a > hashcode during this operation makes restoring the former markWord (locked, > inflated, etc) too complicated. Plus we don't want all these objects to have > hashcodes because locking operations after tagging would have to always use > inflated locks. > > Much of this change is to remove serial weak oop processing for the > weakProcessor, ZGC and Shenandoah. The GCs have been stress tested with > jvmti code. > > It has also been tested with tier1-6. > > Thank you to Stefan, Erik and Kim for their help with this change. Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision: Code review comments from StefanK. - Changes: - all: https://git.openjdk.java.net/jdk/pull/967/files - new: https://git.openjdk.java.net/jdk/pull/967/files/534326da..cb4c83e0 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=967&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=967&range=00-01 Stats: 75 lines in 9 files changed: 12 ins; 48 del; 15 mod Patch: https://git.openjdk.java.net/jdk/pull/967.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/967/head:pull/967 PR: https://git.openjdk.java.net/jdk/pull/967
Re: RFR: 8212879: Make JVMTI TagMap table not hash on oop address
On Fri, 30 Oct 2020 20:46:31 GMT, Erik Joelsson wrote: >> This change turns the HashTable that JVMTI uses for object tagging into a >> regular Hotspot hashtable - the one in hashtable.hpp with resizing and >> rehashing. Instead of pointing directly to oops so that GC has to walk the >> table to follow oops and then to rehash the table, this table points to >> WeakHandle. GC walks the backing OopStorages concurrently. >> >> The hash function for the table is a hash of the lower 32 bits of the >> address. A flag is set during GC (gc_notification if in a safepoint, and >> through a call to JvmtiTagMap::needs_processing()) so that the table is >> rehashed at the next use. >> >> The gc_notification mechanism of weak oop processing is used to notify Jvmti >> to post ObjectFree events. In concurrent GCs there can be a window of time >> between weak oop marking where the oop is unmarked, so dead (the phantom >> load in peek returns NULL) but the gc_notification hasn't been done yet. In >> this window, a heap walk or GetObjectsWithTags call would not find an object >> before the ObjectFree event is posted. This is dealt with in two ways: >> >> 1. In the Heap walk, there's an unconditional table walk to post events if >> events are needed to post. >> 2. For GetObjectWithTags, if a dead oop is found in the table and posting is >> required, we use the VM thread to post the event. >> >> Event posting cannot be done in a JavaThread because the posting needs to be >> done while holding the table lock, so that the JvmtiEnv state doesn't change >> before posting is done. ObjectFree callbacks are limited in what they can >> do as per the JVMTI Specification. The allowed callbacks to the VM already >> have code to allow NonJava threads. >> >> To avoid rehashing, I also tried to use object->identity_hash() but this >> breaks because entries can be added to the table during heapwalk, where the >> objects use marking. The starting markWord is saved and restored. Adding a >> hashcode during this operation makes restoring the former markWord (locked, >> inflated, etc) too complicated. Plus we don't want all these objects to >> have hashcodes because locking operations after tagging would have to always >> use inflated locks. >> >> Much of this change is to remove serial weak oop processing for the >> weakProcessor, ZGC and Shenandoah. The GCs have been stress tested with >> jvmti code. >> >> It has also been tested with tier1-6. >> >> Thank you to Stefan, Erik and Kim for their help with this change. > > Build changes look ok. There should be a thank you emoji that doesn't send email except maybe to the person reviewing the code. Thank you @erikj79 and @magicus for reviewing the build changes. There should also be a 'fixed' emoji. - PR: https://git.openjdk.java.net/jdk/pull/967
Re: RFR: 8212879: Make JVMTI TagMap table not hash on oop address
On Fri, 30 Oct 2020 20:23:04 GMT, Coleen Phillimore wrote: > This change turns the HashTable that JVMTI uses for object tagging into a > regular Hotspot hashtable - the one in hashtable.hpp with resizing and > rehashing. Instead of pointing directly to oops so that GC has to walk the > table to follow oops and then to rehash the table, this table points to > WeakHandle. GC walks the backing OopStorages concurrently. > > The hash function for the table is a hash of the lower 32 bits of the > address. A flag is set during GC (gc_notification if in a safepoint, and > through a call to JvmtiTagMap::needs_processing()) so that the table is > rehashed at the next use. > > The gc_notification mechanism of weak oop processing is used to notify Jvmti > to post ObjectFree events. In concurrent GCs there can be a window of time > between weak oop marking where the oop is unmarked, so dead (the phantom load > in peek returns NULL) but the gc_notification hasn't been done yet. In this > window, a heap walk or GetObjectsWithTags call would not find an object > before the ObjectFree event is posted. This is dealt with in two ways: > > 1. In the Heap walk, there's an unconditional table walk to post events if > events are needed to post. > 2. For GetObjectWithTags, if a dead oop is found in the table and posting is > required, we use the VM thread to post the event. > > Event posting cannot be done in a JavaThread because the posting needs to be > done while holding the table lock, so that the JvmtiEnv state doesn't change > before posting is done. ObjectFree callbacks are limited in what they can do > as per the JVMTI Specification. The allowed callbacks to the VM already have > code to allow NonJava threads. > > To avoid rehashing, I also tried to use object->identity_hash() but this > breaks because entries can be added to the table during heapwalk, where the > objects use marking. The starting markWord is saved and restored. Adding a > hashcode during this operation makes restoring the former markWord (locked, > inflated, etc) too complicated. Plus we don't want all these objects to have > hashcodes because locking operations after tagging would have to always use > inflated locks. > > Much of this change is to remove serial weak oop processing for the > weakProcessor, ZGC and Shenandoah. The GCs have been stress tested with > jvmti code. > > It has also been tested with tier1-6. > > Thank you to Stefan, Erik and Kim for their help with this change. I think I addressed your comments, retesting now. Thank you! - PR: https://git.openjdk.java.net/jdk/pull/967
Re: RFR: 8212879: Make JVMTI TagMap table not hash on oop address
On Mon, 2 Nov 2020 08:08:53 GMT, Stefan Karlsson wrote:
>> This change turns the HashTable that JVMTI uses for object tagging into a
>> regular Hotspot hashtable - the one in hashtable.hpp with resizing and
>> rehashing. Instead of pointing directly to oops so that GC has to walk the
>> table to follow oops and then to rehash the table, this table points to
>> WeakHandle. GC walks the backing OopStorages concurrently.
>>
>> The hash function for the table is a hash of the lower 32 bits of the
>> address. A flag is set during GC (gc_notification if in a safepoint, and
>> through a call to JvmtiTagMap::needs_processing()) so that the table is
>> rehashed at the next use.
>>
>> The gc_notification mechanism of weak oop processing is used to notify Jvmti
>> to post ObjectFree events. In concurrent GCs there can be a window of time
>> between weak oop marking where the oop is unmarked, so dead (the phantom
>> load in peek returns NULL) but the gc_notification hasn't been done yet. In
>> this window, a heap walk or GetObjectsWithTags call would not find an object
>> before the ObjectFree event is posted. This is dealt with in two ways:
>>
>> 1. In the Heap walk, there's an unconditional table walk to post events if
>> events are needed to post.
>> 2. For GetObjectWithTags, if a dead oop is found in the table and posting is
>> required, we use the VM thread to post the event.
>>
>> Event posting cannot be done in a JavaThread because the posting needs to be
>> done while holding the table lock, so that the JvmtiEnv state doesn't change
>> before posting is done. ObjectFree callbacks are limited in what they can
>> do as per the JVMTI Specification. The allowed callbacks to the VM already
>> have code to allow NonJava threads.
>>
>> To avoid rehashing, I also tried to use object->identity_hash() but this
>> breaks because entries can be added to the table during heapwalk, where the
>> objects use marking. The starting markWord is saved and restored. Adding a
>> hashcode during this operation makes restoring the former markWord (locked,
>> inflated, etc) too complicated. Plus we don't want all these objects to
>> have hashcodes because locking operations after tagging would have to always
>> use inflated locks.
>>
>> Much of this change is to remove serial weak oop processing for the
>> weakProcessor, ZGC and Shenandoah. The GCs have been stress tested with
>> jvmti code.
>>
>> It has also been tested with tier1-6.
>>
>> Thank you to Stefan, Erik and Kim for their help with this change.
>
> src/hotspot/share/gc/shared/oopStorageSet.hpp line 41:
>
>> 39: // Must be updated when new OopStorages are introduced
>> 40: static const uint strong_count = 4 JVMTI_ONLY(+ 1);
>> 41: static const uint weak_count = 5 JVMTI_ONLY(+1) JFR_ONLY(+ 1);
>
> All other uses `+ 1` instead of `+1`.
Fixed, although I think the space looks strange there but I'll go along.
> src/hotspot/share/gc/shared/weakProcessorPhaseTimes.hpp line 49:
>
>> 47: double _phase_times_sec[1];
>> 48: size_t _phase_dead_items[1];
>> 49: size_t _phase_total_items[1];
>
> This should be removed and the associated reset_items
Removed.
> src/hotspot/share/gc/z/zOopClosures.hpp line 64:
>
>> 62: };
>> 63:
>> 64: class ZPhantomKeepAliveOopClosure : public ZRootsIteratorClosure {
>
> Seems like you flipped the location of these two. Maybe revert?
Reverted. There was a rebasing conflict here so this was unintentional.
> src/hotspot/share/prims/jvmtiExport.hpp line 405:
>
>> 403:
>> 404: // Delete me and all my callers!
>> 405: static void weak_oops_do(BoolObjectClosure* b, OopClosure* f) {}
>
> Maybe delete?
Yes, meant to do that.
> src/hotspot/share/prims/jvmtiTagMap.cpp line 131:
>
>> 129: // Operating on the hashmap must always be locked, since concurrent
>> GC threads may
>> 130: // notify while running through a safepoint.
>> 131: assert(is_locked(), "checking");
>
> Maybe move this to the top of the function to make it very clear.
ok.
> src/hotspot/share/prims/jvmtiTagMap.cpp line 133:
>
>> 131: assert(is_locked(), "checking");
>> 132: if (post_events && env()->is_enabled(JVMTI_EVENT_OBJECT_FREE)) {
>> 133: log_info(jvmti, table)("TagMap table needs posting before heap
>> walk");
>
> Not sure about the "before heap walk" since this is also done from
> GetObjectsWithTags, which does *not* do a heap walk but still requires
> posting.
I don't call check_hashmap for GetObjectsWithTags.
> src/hotspot/share/prims/jvmtiTagMap.cpp line 140:
>
>> 138: hashmap()->rehash();
>> 139: _needs_rehashing = false;
>> 140: }
>
> It's not clear to me that it's correct to rehash *after* posting. I think it
> is, because unlink_and_post will use load barriers to fixup old pointers.
I think it's better that the rehashing doesn't encounter null entries and the
WeakHandle.peek() operation is used for both so I hope it would get the same
answer. If not, which seem
Re: RFR: 8212879: Make JVMTI TagMap table not hash on oop address
On Mon, 2 Nov 2020 08:34:17 GMT, Stefan Karlsson wrote: >> src/hotspot/share/prims/jvmtiTagMap.cpp line 126: >> >>> 124: // concurrent GCs. So fix it here once we have a lock or are >>> 125: // at a safepoint. >>> 126: // SetTag and GetTag should not post events! >> >> I think it would be good to explain why. Otherwise, this just leaves the >> readers wondering why this is the case. > > Maybe even move this comment to the set_tag/get_tag code. I was trying to explain why there's a boolean there but I can put this comment at both get_tag and set_tag. // Check if we have to processing for concurrent GCs. // GetTag should not post events because the JavaThread has to // transition to native for the callback and this cannot stop for // safepoints with the hashmap lock held. check_hashmap(false); - PR: https://git.openjdk.java.net/jdk/pull/967
Re: RFR: 8255616: Disable AOT and Graal in Oracle OpenJDK
On Fri, 30 Oct 2020 17:40:51 GMT, Vladimir Kozlov wrote: > We shipped Ahead-of-Time compilation (the jaotc tool) in JDK 9, as an > experimental feature. We shipped Graal as an experimental JIT compiler in JDK > 10. We haven't seen much use of these features, and the effort required to > support and enhance them is significant. We therefore intend to disable these > features in Oracle builds as of JDK 16. > > We'll leave the sources for these features in the repository, in case any one > else is interested in building them. But we will not update or test them. > > We'll continue to build and ship JVMCI as an experimental feature in Oracle > builds. > > Tested changes in all tiers. > > I verified that with these changes I still able to build Graal in open repo > and run graalunit testing: > > `open$ bash test/hotspot/jtreg/compiler/graalunit/downloadLibs.sh > /mydir/graalunit_lib/` > `open$ bash configure --with-debug-level=fastdebug > --with-graalunit-lib=/mydir/graalunit_lib/ --with-jtreg=/mydir/jtreg` > `open$ make jdk-image` > `open$ make test-image` > `open$ make run-test TEST=compiler/graalunit/HotspotTest.java` Build changes look good. - Marked as reviewed by ihse (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/960
Re: RFR: 8212879: Make JVMTI TagMap table not hash on oop address
On Fri, 30 Oct 2020 20:23:04 GMT, Coleen Phillimore wrote: > This change turns the HashTable that JVMTI uses for object tagging into a > regular Hotspot hashtable - the one in hashtable.hpp with resizing and > rehashing. Instead of pointing directly to oops so that GC has to walk the > table to follow oops and then to rehash the table, this table points to > WeakHandle. GC walks the backing OopStorages concurrently. > > The hash function for the table is a hash of the lower 32 bits of the > address. A flag is set during GC (gc_notification if in a safepoint, and > through a call to JvmtiTagMap::needs_processing()) so that the table is > rehashed at the next use. > > The gc_notification mechanism of weak oop processing is used to notify Jvmti > to post ObjectFree events. In concurrent GCs there can be a window of time > between weak oop marking where the oop is unmarked, so dead (the phantom load > in peek returns NULL) but the gc_notification hasn't been done yet. In this > window, a heap walk or GetObjectsWithTags call would not find an object > before the ObjectFree event is posted. This is dealt with in two ways: > > 1. In the Heap walk, there's an unconditional table walk to post events if > events are needed to post. > 2. For GetObjectWithTags, if a dead oop is found in the table and posting is > required, we use the VM thread to post the event. > > Event posting cannot be done in a JavaThread because the posting needs to be > done while holding the table lock, so that the JvmtiEnv state doesn't change > before posting is done. ObjectFree callbacks are limited in what they can do > as per the JVMTI Specification. The allowed callbacks to the VM already have > code to allow NonJava threads. > > To avoid rehashing, I also tried to use object->identity_hash() but this > breaks because entries can be added to the table during heapwalk, where the > objects use marking. The starting markWord is saved and restored. Adding a > hashcode during this operation makes restoring the former markWord (locked, > inflated, etc) too complicated. Plus we don't want all these objects to have > hashcodes because locking operations after tagging would have to always use > inflated locks. > > Much of this change is to remove serial weak oop processing for the > weakProcessor, ZGC and Shenandoah. The GCs have been stress tested with > jvmti code. > > It has also been tested with tier1-6. > > Thank you to Stefan, Erik and Kim for their help with this change. Build changes look good. Not reviewed hotspot changes. - Marked as reviewed by ihse (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/967
Re: RFR: 8212879: Make JVMTI TagMap table not hash on oop address
On Mon, 2 Nov 2020 08:25:28 GMT, Stefan Karlsson wrote: >> This change turns the HashTable that JVMTI uses for object tagging into a >> regular Hotspot hashtable - the one in hashtable.hpp with resizing and >> rehashing. Instead of pointing directly to oops so that GC has to walk the >> table to follow oops and then to rehash the table, this table points to >> WeakHandle. GC walks the backing OopStorages concurrently. >> >> The hash function for the table is a hash of the lower 32 bits of the >> address. A flag is set during GC (gc_notification if in a safepoint, and >> through a call to JvmtiTagMap::needs_processing()) so that the table is >> rehashed at the next use. >> >> The gc_notification mechanism of weak oop processing is used to notify Jvmti >> to post ObjectFree events. In concurrent GCs there can be a window of time >> between weak oop marking where the oop is unmarked, so dead (the phantom >> load in peek returns NULL) but the gc_notification hasn't been done yet. In >> this window, a heap walk or GetObjectsWithTags call would not find an object >> before the ObjectFree event is posted. This is dealt with in two ways: >> >> 1. In the Heap walk, there's an unconditional table walk to post events if >> events are needed to post. >> 2. For GetObjectWithTags, if a dead oop is found in the table and posting is >> required, we use the VM thread to post the event. >> >> Event posting cannot be done in a JavaThread because the posting needs to be >> done while holding the table lock, so that the JvmtiEnv state doesn't change >> before posting is done. ObjectFree callbacks are limited in what they can >> do as per the JVMTI Specification. The allowed callbacks to the VM already >> have code to allow NonJava threads. >> >> To avoid rehashing, I also tried to use object->identity_hash() but this >> breaks because entries can be added to the table during heapwalk, where the >> objects use marking. The starting markWord is saved and restored. Adding a >> hashcode during this operation makes restoring the former markWord (locked, >> inflated, etc) too complicated. Plus we don't want all these objects to >> have hashcodes because locking operations after tagging would have to always >> use inflated locks. >> >> Much of this change is to remove serial weak oop processing for the >> weakProcessor, ZGC and Shenandoah. The GCs have been stress tested with >> jvmti code. >> >> It has also been tested with tier1-6. >> >> Thank you to Stefan, Erik and Kim for their help with this change. > > src/hotspot/share/prims/jvmtiTagMap.cpp line 126: > >> 124: // concurrent GCs. So fix it here once we have a lock or are >> 125: // at a safepoint. >> 126: // SetTag and GetTag should not post events! > > I think it would be good to explain why. Otherwise, this just leaves the > readers wondering why this is the case. Maybe even move this comment to the set_tag/get_tag code. - PR: https://git.openjdk.java.net/jdk/pull/967
Re: RFR: 8212879: Make JVMTI TagMap table not hash on oop address
On Fri, 30 Oct 2020 20:23:04 GMT, Coleen Phillimore wrote:
> This change turns the HashTable that JVMTI uses for object tagging into a
> regular Hotspot hashtable - the one in hashtable.hpp with resizing and
> rehashing. Instead of pointing directly to oops so that GC has to walk the
> table to follow oops and then to rehash the table, this table points to
> WeakHandle. GC walks the backing OopStorages concurrently.
>
> The hash function for the table is a hash of the lower 32 bits of the
> address. A flag is set during GC (gc_notification if in a safepoint, and
> through a call to JvmtiTagMap::needs_processing()) so that the table is
> rehashed at the next use.
>
> The gc_notification mechanism of weak oop processing is used to notify Jvmti
> to post ObjectFree events. In concurrent GCs there can be a window of time
> between weak oop marking where the oop is unmarked, so dead (the phantom load
> in peek returns NULL) but the gc_notification hasn't been done yet. In this
> window, a heap walk or GetObjectsWithTags call would not find an object
> before the ObjectFree event is posted. This is dealt with in two ways:
>
> 1. In the Heap walk, there's an unconditional table walk to post events if
> events are needed to post.
> 2. For GetObjectWithTags, if a dead oop is found in the table and posting is
> required, we use the VM thread to post the event.
>
> Event posting cannot be done in a JavaThread because the posting needs to be
> done while holding the table lock, so that the JvmtiEnv state doesn't change
> before posting is done. ObjectFree callbacks are limited in what they can do
> as per the JVMTI Specification. The allowed callbacks to the VM already have
> code to allow NonJava threads.
>
> To avoid rehashing, I also tried to use object->identity_hash() but this
> breaks because entries can be added to the table during heapwalk, where the
> objects use marking. The starting markWord is saved and restored. Adding a
> hashcode during this operation makes restoring the former markWord (locked,
> inflated, etc) too complicated. Plus we don't want all these objects to have
> hashcodes because locking operations after tagging would have to always use
> inflated locks.
>
> Much of this change is to remove serial weak oop processing for the
> weakProcessor, ZGC and Shenandoah. The GCs have been stress tested with
> jvmti code.
>
> It has also been tested with tier1-6.
>
> Thank you to Stefan, Erik and Kim for their help with this change.
Commented on nits, and reviewed GC code and tag map code. Didn't look closely
on the hashmap changes.
src/hotspot/share/gc/shared/oopStorageSet.hpp line 41:
> 39: // Must be updated when new OopStorages are introduced
> 40: static const uint strong_count = 4 JVMTI_ONLY(+ 1);
> 41: static const uint weak_count = 5 JVMTI_ONLY(+1) JFR_ONLY(+ 1);
All other uses `+ 1` instead of `+1`.
src/hotspot/share/gc/shared/weakProcessorPhaseTimes.hpp line 49:
> 47: double _phase_times_sec[1];
> 48: size_t _phase_dead_items[1];
> 49: size_t _phase_total_items[1];
This should be removed and the associated reset_items
src/hotspot/share/gc/z/zOopClosures.hpp line 64:
> 62: };
> 63:
> 64: class ZPhantomKeepAliveOopClosure : public ZRootsIteratorClosure {
Seems like you flipped the location of these two. Maybe revert?
src/hotspot/share/prims/jvmtiExport.hpp line 405:
> 403:
> 404: // Delete me and all my callers!
> 405: static void weak_oops_do(BoolObjectClosure* b, OopClosure* f) {}
Maybe delete?
src/hotspot/share/prims/jvmtiTagMap.cpp line 126:
> 124: // concurrent GCs. So fix it here once we have a lock or are
> 125: // at a safepoint.
> 126: // SetTag and GetTag should not post events!
I think it would be good to explain why. Otherwise, this just leaves the
readers wondering why this is the case.
src/hotspot/share/prims/jvmtiTagMap.cpp line 131:
> 129: // Operating on the hashmap must always be locked, since concurrent GC
> threads may
> 130: // notify while running through a safepoint.
> 131: assert(is_locked(), "checking");
Maybe move this to the top of the function to make it very clear.
src/hotspot/share/prims/jvmtiTagMap.cpp line 133:
> 131: assert(is_locked(), "checking");
> 132: if (post_events && env()->is_enabled(JVMTI_EVENT_OBJECT_FREE)) {
> 133: log_info(jvmti, table)("TagMap table needs posting before heap
> walk");
Not sure about the "before heap walk" since this is also done from
GetObjectsWithTags, which does *not* do a heap walk but still requires posting.
src/hotspot/share/prims/jvmtiTagMap.cpp line 140:
> 138: hashmap()->rehash();
> 139: _needs_rehashing = false;
> 140: }
It's not clear to me that it's correct to rehash *after* posting. I think it
is, because unlink_and_post will use load barriers to fixup old pointers.
src/hotspot/share/prims/jvmtiTagMap.cpp line 146:
> 144: // The ZDriver may be walking the hashmaps concurrently so all these
Re: RFR: 8255452: Doing GC during JVMTI MethodExit event posting breaks return oop [v2]
On Sat, 31 Oct 2020 09:54:09 GMT, Serguei Spitsyn wrote: > Hi Erik, > > Nice discovery! Indeed, this is a long standing issue. It looks good in > general. > I agree with Coleen, it would be nice if there is an elegant way to move the > oop_result saving/restoring code to InterpreterRuntime::post_method_exit. > Otherwise, I'm okay with what you have now. > It is also nice discovery of the issue with clearing the expression stack. I > think, it was my mistake in the initial implementation of the > ForceEarlyReturn when I followed the PopFrame implementation pattern. It is > good to separate it from the current fix. > > Thanks, > Serguei Thanks for reviewing this Serguei. And thanks for confirming our suspicions regarding clearing of the expression stack. I wasn't sure if anyone would be around that knew how it ended up there! I made the refactoring that you and Coleen wanted, I think. Hope you like it! - PR: https://git.openjdk.java.net/jdk/pull/930
Re: RFR: 8255452: Doing GC during JVMTI MethodExit event posting breaks return oop [v2]
On Fri, 30 Oct 2020 14:09:46 GMT, Erik Österlund wrote: >> Oh that's actually horrible. I wonder if it's possible to hoist saving the >> result oop into the InterpreterRuntime entry. And pass the Handle into >> JvmtiExport::post_method_exit(). > > I tried that first, and ended up with a bunch of non-trivial code duplication > instead, as reading the oop is done in both paths but for different reasons. > One to preserve/restore it (interpreter remove_activation entry), but also > inside of JvmtiExport::post_method_exit() so that it can be passed into the > MethodExit. I will give it another shot and see if it is possible to refactor > it in a better way. I uploaded a CR that does pretty much what you suggested, ish. Hope you like it! - PR: https://git.openjdk.java.net/jdk/pull/930
Re: RFR: 8255452: Doing GC during JVMTI MethodExit event posting breaks return oop [v2]
On Sat, 31 Oct 2020 09:54:09 GMT, Serguei Spitsyn wrote: >> Erik Österlund has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Coleen CR1: Refactoring > > Hi Erik, > > Nice discovery! Indeed, this is a long standing issue. It looks good in > general. > I agree with Coleen, it would be nice if there is an elegant way to move the > oop_result saving/restoring code to InterpreterRuntime::post_method_exit. > Otherwise, I'm okay with what you have now. > It is also nice discovery of the issue with clearing the expression stack. I > think, it was my mistake in the initial implementation of the > ForceEarlyReturn when I followed the PopFrame implementation pattern. It is > good to separate it from the current fix. > > Thanks, > Serguei I uploaded a new commit to perform some refactoring as requested by Coleen and Serguei. I made the oop save/restore + JRT_BLOCK logic belong only to the path taken from InterpreterRuntime::post_method_exit. An inner posting method is called both from that path and from JvmtiExport::notice_unwind_due_to_exception. I think the result is an improvement in terms of how clear it is. I didn't want to move logic all the way back to InterpreterRuntime::post_method_exit though, as I don't think it looks pretty to have large chunks of JVMTI implementation details in the interpreterRuntime.cpp file. So I did basically what you suggested, with the slight difference of moving all the JVMTI implementation into the JVMTI file instead, which is just called from InterpreterRuntime::post_method_exit. Hope you are okay with this! - PR: https://git.openjdk.java.net/jdk/pull/930
Re: RFR: 8255452: Doing GC during JVMTI MethodExit event posting breaks return oop [v2]
> The imasm::remove_activation() call does not deal with safepoints very well. > However, when the MethodExit JVMTI event is being called, we call into the > runtime in the middle of remove_activation(). If the value being returned is > an object type, then the top-of-stack contains the oop. However, the GC does > not traverse said oop in any oop map, because it is simply not expected that > we safepoint in the middle of remove_activation(). > > The JvmtiExport::post_method_exit() function we end up calling, reads the > top-of-stack oop, and puts it in a handle. Then it calls JVMTI callbacks, > that eventually call Java and a bunch of stuff that safepoints. So after the > JVMTI callback, we can expect the top-of-stack oop to be broken. > Unfortunately, when we continue, we therefore end up returning a broken oop. > > Notably, the fact that InterpreterRuntime::post_method_exit is a JRT_ENTRY, > is wrong, as we can safepoint on the way back to Java, which will break the > return oop in a similar way. So this patch makes it a JRT_BLOCK_ENTRY, moving > the transition to VM and back, into a block of code that is protected against > GC. Before the JRT_BLOCK is called, we stash away the return oop, and after > the JRT_BLOCK_END, we restore the top-of-stack oop. In the path when > InterpreterRuntime::post_method_exit is called when throwing an exception, we > don't have the same problem of retaining an oop result, and hence the > JRT_BLOCK/JRT_BLOCK_END section is not performed in this case; the logic is > the same as before for this path. > > This is a JVMTI bug that has probably been around for a long time. It crashes > with all GCs, but was discovered recently after concurrent stack processing, > as StefanK has been running better GC stressing code in JVMTI, and the bug > reproduced more easily with concurrent stack processing, as the timings were > a bit different. The following reproducer failed pretty much 100% of the time: > while true; do make test JTREG="RETAIN=all" > TEST=test/hotspot/jtreg/vmTestbase/nsk/jdi/MethodExitEvent/returnValue/returnValue003/returnValue003.java > TEST_OPTS_JAVA_OPTIONS="-XX:+UseZGC -Xmx2g -XX:ZCollectionInterval=0.0001 > -XX:ZFragmentationLimit=0.01 -XX:+VerifyOops -XX:+ZVerifyViews -Xint" ; done > > With my fix I can run this repeatedly without any more failures. I have also > sanity checked the patch by running tier 1-5, so that it does not introduces > any new issues on its own. I have also used Stefan's nice external GC > stressing with jcmd technique that was used to trigger crashes with other > GCs, to make sure said crashes no longer reproduce either. Erik Österlund has updated the pull request incrementally with one additional commit since the last revision: Coleen CR1: Refactoring - Changes: - all: https://git.openjdk.java.net/jdk/pull/930/files - new: https://git.openjdk.java.net/jdk/pull/930/files/d7500082..ae6355fd Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=930&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=930&range=00-01 Stats: 41 lines in 2 files changed: 13 ins; 19 del; 9 mod Patch: https://git.openjdk.java.net/jdk/pull/930.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/930/head:pull/930 PR: https://git.openjdk.java.net/jdk/pull/930
Re: RFR: 8254723: add diagnostic command to write Linux perf map file [v5]
On Tue, 27 Oct 2020 04:21:33 GMT, Nick Gasson wrote: >> When using the Linux "perf" tool to do system profiling, symbol names of >> running Java methods cannot be decoded, resulting in unhelpful output >> such as: >> >> 10.52% [JIT] tid 236748 [.] 0x7f6fdb75d223 >> >> Perf can read a simple text file format describing the mapping between >> address ranges and symbol names for a particular process [1]. >> >> It's possible to generate this already for Java processes using a JVMTI >> plugin such as perf-map-agent [2]. However this requires compiling >> third-party code and then loading the agent into your Java process. It >> would be more convenient if Hotspot could write this file directly using >> a diagnostic command. The information required is almost identical to >> that of the existing Compiler.codelist command. >> >> This patch adds a Compiler.perfmap diagnostic command on Linux only. To >> use, first run "jcmd Compiler.perfmap" and then "perf top" or >> "perf record" and the report should show decoded Java symbol names for >> that process. >> >> As this just writes a snapshot of the code cache when the command is >> run, it will become stale if methods are compiled later or unloaded. >> However this shouldn't be a big problem in practice if the map file is >> generated after the application has warmed up. >> >> [1] >> https://github.com/torvalds/linux/blob/master/tools/perf/Documentation/jit-interface.txt >> [2] https://github.com/jvm-profiling-tools/perf-map-agent > > Nick Gasson has updated the pull request incrementally with one additional > commit since the last revision: > > Make DumpPerfMapAtExit a diagnostic option Ok, I reviewed this change. I guess you should close [JDK-8254723](https://bugs.openjdk.java.net/browse/JDK-8254723). - Marked as reviewed by ysuenaga (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/760
