RFR: 8274716: JDWP Spec: the description for the Dispose command confuses suspend with resume.
The following sentence in the JDWP Specification describing the Dispose command confuses resume with suspend [1]: All threads suspended by the thread-level **resume** command or the VM-level **resume** command are resumed as many times as necessary for them to run. It should be changed to All threads suspended by the thread-level **suspend** command or the VM-level **suspend** command are resumed as many times as necessary for them to run. [1] [JDWP Spec, Dispose Command (JDK17).](https://docs.oracle.com/en/java/javase/17/docs/specs/jdwp/jdwp-protocol.html#JDWP_VirtualMachine_Dispose) - Commit messages: - Correct JDWP spec, Dispose command. Changes: https://git.openjdk.java.net/jdk/pull/5804/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=5804&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8274716 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/5804.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5804/head:pull/5804 PR: https://git.openjdk.java.net/jdk/pull/5804
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v9]
On Fri, 5 Mar 2021 11:11:44 GMT, Alan Hayward wrote: >>> I was building this PR on a new machine, and I now get the following error: >>> >>> > /Users/alahay01/java/gerrit_jdk/src/java.desktop/macosx/native/libjsound/PLATFORM_API_MacOSX_MidiUtils.c:258:31: >>> > error: cast to smaller integer type 'MIDIClientRef' (aka 'unsigned int') >>> > from 'void *' [-Werror,-Wvoid-pointer-to-int-cast] >>> > static MIDIClientRef client = (MIDIClientRef) NULL; >>> > ^~~~ >>> > /Users/alahay01/java/gerrit_jdk/src/java.desktop/macosx/native/libjsound/PLATFORM_API_MacOSX_MidiUtils.c:259:29: >>> > error: cast to smaller integer type 'MIDIPortRef' (aka 'unsigned int') >>> > from 'void *' [-Werror,-Wvoid-pointer-to-int-cast] >>> > static MIDIPortRef inPort = (MIDIPortRef) NULL; >>> > ^~ >>> > /Users/alahay01/java/gerrit_jdk/src/java.desktop/macosx/native/libjsound/PLATFORM_API_MacOSX_MidiUtils.c:260:30: >>> > error: cast to smaller integer type 'MIDIPortRef' (aka 'unsigned int') >>> > from 'void *' [-Werror,-Wvoid-pointer-to-int-cast] >>> > static MIDIPortRef outPort = (MIDIPortRef) NULL; >>> > ^~ >>> > /Users/alahay01/java/gerrit_jdk/src/java.desktop/macosx/native/libjsound/PLATFORM_API_MacOSX_MidiUtils.c:466:32: >>> > error: cast to smaller integer type 'MIDIEndpointRef' (aka 'unsigned >>> > int') from 'void *' [-Werror,-Wvoid-pointer-to-int-cast] >>> > MIDIEndpointRef endpoint = (MIDIEndpointRef) NULL; >>> > ^~ >>> > 4 errors generated. >>> >>> As far as I can tell the only difference between the two systems is the >>> xcode version: >>> >>> New system (failing) >>> % xcodebuild -version >>> Xcode 12.5 >>> Build version 12E5244e >>> >>> Old system (working) >>> % xcodebuild -version >>> Xcode 12.4 >>> Build version 12D4e >>> >>> Looks like the newer version of Xcode is being a little stricter with >>> casting? >>> Replacing the NULL with 0 seems to fix the issue. >> >> Hello >> there is one issue with the info you provided, it's from Xcode12.5 beta. >> And beta license agreement forbids sharing output of beta version of >> compiler&co >> So we can't say we have issue with newer xcode beta until that beta went >> public & released. >> Fixing issues you found now will mean someone have violated xcode beta >> license agreement and made these issues public. > >> Hello >> there is one issue with the info you provided, it's from Xcode12.5 beta. >> And beta license agreement forbids sharing output of beta version of >> compiler&co >> So we can't say we have issue with newer xcode beta until that beta went >> public & released. >> Fixing issues you found now will mean someone have violated xcode beta >> license agreement and made these issues public. > > Ok, I wasn't aware of that. I'll downgrade back to the non-beta version. Hi, @VladimirKempik reported failure of CompressedClassPointers.largeHeapAbove32GTest() with [JDK-8262895](https://bugs.openjdk.java.net/browse/JDK-8262895) on macos_aarch64. He also helped analyzing the issue (thanks!). Apparently the vm has difficulties mapping the heap of 31GB at one of the preferred addresses given in [`get_attach_addresses_for_disjoint_mode()`](https://github.com/openjdk/jdk/blob/8d3de4b1bdb5dc13bb7724596dc2123ba05bbb81/src/hotspot/share/memory/virtualspace.cpp#L477). This causes the test failure indirectly. IMO it could be worth the effort to find out why the heap cannot be mapped at the preferred addresses. Reviewers should decide on it. I wouldn't be able to do it myself though as I don't have access to a macos_aarch64 system. Alternatively I'd suggest to exlude macos_aarch64 from the subtest largeHeapAbove32GTest. Best regards, Richard. -- Details of the analysis In the following trace we see the vm trying to allocate the heap at addresses given in [`get_attach_addresses_for_disjoint_mode()`](https://github.com/openjdk/jdk/blob/8d3de4b1bdb5dc13bb7724596dc2123ba05bbb81/src/hotspot/share/memory/virtualspace.cpp#L477) without success: images/jdk/bin/java -XX:+UnlockDiagnosticVMOptions -XX:+UnlockExperimentalVMOptions -Xmx31g -XX:-UseAOT -Xlog:gc+metaspace=trace,cds=trace,heap+gc+exit=info,gc+heap+coops=trace -Xshare:off -XX:+VerifyBeforeGC -XX:HeapSearchSteps=40 -version OpenJDK 64-Bit Server VM warning: Shared spaces are not supported in this VM [0.005s][trace][gc,heap,coops] Trying to allocate at address 0x0010 heap of size 0x7c100 [0.005s][trace][gc,heap,coops] Trying to allocate at address 0x0018 heap of size 0x7c100 [0.005s][trace][gc,heap,coops] Trying to allocate at address 0x0020 heap of size 0x7c100 [0.005s][trace][gc,heap,coops] Trying to allocate at address 0x0040 heap of size 0x7c100 [0.005s][trace][gc,heap,coops] Trying to allocate at address 0x0050 heap of size 0x7c100 [0.005s][trace][gc,heap,coops] Trying to allocate at address 0x0008 heap of size 0x7c10
Re: RFR: 8259021: SharedSecrets should avoid double racy reads from non-volatile fields [v2]
On Mon, 4 Jan 2021 14:27:09 GMT, Peter Levart wrote: >> See: https://bugs.openjdk.java.net/browse/JDK-8259021 >> See also discussion in thread: >> https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-December/072798.html > > Peter Levart has updated the pull request incrementally with one additional > commit since the last revision: > > revert the unrelated change Marked as reviewed by rrich (Committer). - PR: https://git.openjdk.java.net/jdk/pull/1914
Re: RFR: 8259021: SharedSecrets should avoid double racy reads from non-volatile fields [v2]
On Mon, 4 Jan 2021 17:45:25 GMT, Mandy Chung wrote:
>> Peter Levart has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> revert the unrelated change
>
> Marked as reviewed by mchung (Reviewer).
>
>
> _Mailing list message from [Hans Boehm](mailto:hbo...@google.com) on
> [core-libs-dev](mailto:core-libs-dev@openjdk.java.net):_
>
> On Mon, Jan 4, 2021 at 8:34 AM Peter Levart
> wrote:
>
> > On Mon, 4 Jan 2021 15:57:33 GMT, Richard Reingruber
>
> wrote:
>
> > > > The bug title and the PR title need to be the same.
> > > > Editing either one is fine.
> > >
> > >
> > > But wouldn't it be legal for a compiler (java to bytecode or bytecode to
> > > machinecode) to replace references of my_local_copy with references to
> > > static_field?
> > > Foo my_local_copy = static_field;
> > > if (my_copy == null) {
> > > initialize();
> > > my_local_copy = static_field;
> > > }
> > > return my_local_copy;
> > > Only if static_field was volatile this would be illegal, wouldn't it?
> >
> >
> > @reinrich I don't think Java compilers may do that. If this was allowed,
>
> such variables would not be called "local".
>
> >
>
> Indeed. Such transformations are allowed in C and C++ (since data races
> result in undefined behavior, and thus the
> compiler is allowed to assume there are no concurrent writes), but not in
> Java.
Thanks for the explanation.
-
PR: https://git.openjdk.java.net/jdk/pull/1914
Re: RFR: 8259021 avoid double racy reads from non-volatile fields of SharedSecrets [v2]
On Mon, 4 Jan 2021 15:11:27 GMT, Roger Riggs wrote:
>> Peter Levart has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> revert the unrelated change
>
> The bug title and the PR title need to be the same.
> Editing either one is fine.
But wouldn't it be legal for a compiler (java to bytecode or bytecode to
machinecode) to replace references of my_local_copy with references to
static_field?
Foo my_local_copy = static_field;
if (my_copy == null) {
initialize();
my_local_copy = static_field;
}
return my_local_copy;
Only if static_field was volatile this would be illegal, wouldn't it?
-
PR: https://git.openjdk.java.net/jdk/pull/1914
Re: RFR: 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents [v10]
On Wed, 14 Oct 2020 20:50:45 GMT, Richard Reingruber wrote: >> Good. > >> >> >> Good. > > Thanks for the review, Vladimir (@vnkozlov)! > I'm still (stress) testing adaptations to lazy/concurrent thread stack > processing for ZGC. > --Richard. Thanks once more @TheRealMDoerr, @GoeLin, @dholmes-ora, @sspitsyn, @vnkozlov, @robehn, @pchilano for feedback and reviews. - PR: https://git.openjdk.java.net/jdk/pull/119
Re: RFR: 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents [v14]
> Hi, > > this is the continuation of the review of the implementation for: > > https://bugs.openjdk.java.net/browse/JDK-8227745 > https://bugs.openjdk.java.net/browse/JDK-8233915 > > It allows for JIT optimizations based on escape analysis even if JVMTI agents > acquire capabilities to access references > to objects that are subject to such optimizations, e.g. scalar replacement. > The implementation reverts such > optimizations just before access very much as when switching from JIT > compiled execution to the interpreter, aka > "deoptimization". Webrev.8 was the last one before before the transition to > Git/Github: > > http://cr.openjdk.java.net/~rrich/webrevs/8227745/webrev.8/ > > Thanks, Richard. Richard Reingruber has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 29 commits: - Merge branch 'master' into JDK-8227745 - Removed cross_modify_fence from JT::wait_for_object_deoptimization(). See JDK-8254264. - handle_special_runtime_exit_condition(): wait (blocked) for obj. deoptimization _before_ async ex. check. - Removed unused parameter from EscapeBarrierSuspendHandshake. - Adaptations to JDK-8254263: Remove special_runtime_exit_condition() check from ~ThreadInVMForHandshake() With JDK-8254263 the special_runtime_exit_condition() check was removed from ~ThreadInVMForHandshake() because now a thread never becomes unsafe when processing its own handshakes. EscapeBarrier uses handshakes to sync with the target thread for object deoptimization so we add a check for object deoptimization to ThreadSafepointState::handle_polling_page_exception(). In JavaThread::wait_for_object_deoptimization() we must check is_obj_deopt_suspend() again after handshake/safepoint processing because a handshake for obj. deopt suspend could have been processed. - Adaptions to lazy/concurrent thread stack processing for ZGC (JEP 376) - EATests.java improvements - Merge branch 'master' into JDK-8227745 - The constructor of StackFrameStream takes more parameters after JDK-8253180 - Merge branch 'master' into JDK-8227745 - ... and 19 more: https://git.openjdk.java.net/jdk/compare/1da28de8...8d09747b - Changes: https://git.openjdk.java.net/jdk/pull/119/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=119&range=13 Stats: 5860 lines in 53 files changed: 5642 ins; 116 del; 102 mod Patch: https://git.openjdk.java.net/jdk/pull/119.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/119/head:pull/119 PR: https://git.openjdk.java.net/jdk/pull/119
Re: RFR: 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents [v13]
> Hi, > > this is the continuation of the review of the implementation for: > > https://bugs.openjdk.java.net/browse/JDK-8227745 > https://bugs.openjdk.java.net/browse/JDK-8233915 > > It allows for JIT optimizations based on escape analysis even if JVMTI agents > acquire capabilities to access references > to objects that are subject to such optimizations, e.g. scalar replacement. > The implementation reverts such > optimizations just before access very much as when switching from JIT > compiled execution to the interpreter, aka > "deoptimization". Webrev.8 was the last one before before the transition to > Git/Github: > > http://cr.openjdk.java.net/~rrich/webrevs/8227745/webrev.8/ > > Thanks, Richard. Richard Reingruber has updated the pull request incrementally with one additional commit since the last revision: Removed cross_modify_fence from JT::wait_for_object_deoptimization(). See JDK-8254264. - Changes: - all: https://git.openjdk.java.net/jdk/pull/119/files - new: https://git.openjdk.java.net/jdk/pull/119/files/272fb025..2ca09188 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=119&range=12 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=119&range=11-12 Stats: 3 lines in 1 file changed: 0 ins; 3 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/119.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/119/head:pull/119 PR: https://git.openjdk.java.net/jdk/pull/119
Re: RFR: 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents [v12]
> Hi, > > this is the continuation of the review of the implementation for: > > https://bugs.openjdk.java.net/browse/JDK-8227745 > https://bugs.openjdk.java.net/browse/JDK-8233915 > > It allows for JIT optimizations based on escape analysis even if JVMTI agents > acquire capabilities to access references > to objects that are subject to such optimizations, e.g. scalar replacement. > The implementation reverts such > optimizations just before access very much as when switching from JIT > compiled execution to the interpreter, aka > "deoptimization". Webrev.8 was the last one before before the transition to > Git/Github: > > http://cr.openjdk.java.net/~rrich/webrevs/8227745/webrev.8/ > > Thanks, Richard. Richard Reingruber has updated the pull request incrementally with one additional commit since the last revision: handle_special_runtime_exit_condition(): wait (blocked) for obj. deoptimization _before_ async ex. check. - Changes: - all: https://git.openjdk.java.net/jdk/pull/119/files - new: https://git.openjdk.java.net/jdk/pull/119/files/f02f07b6..272fb025 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=119&range=11 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=119&range=10-11 Stats: 10 lines in 1 file changed: 5 ins; 5 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/119.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/119/head:pull/119 PR: https://git.openjdk.java.net/jdk/pull/119
Re: RFR: 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents [v11]
> Hi, > > this is the continuation of the review of the implementation for: > > https://bugs.openjdk.java.net/browse/JDK-8227745 > https://bugs.openjdk.java.net/browse/JDK-8233915 > > It allows for JIT optimizations based on escape analysis even if JVMTI agents > acquire capabilities to access references > to objects that are subject to such optimizations, e.g. scalar replacement. > The implementation reverts such > optimizations just before access very much as when switching from JIT > compiled execution to the interpreter, aka > "deoptimization". Webrev.8 was the last one before before the transition to > Git/Github: > > http://cr.openjdk.java.net/~rrich/webrevs/8227745/webrev.8/ > > Thanks, Richard. Richard Reingruber has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 26 commits: - Removed unused parameter from EscapeBarrierSuspendHandshake. - Adaptations to JDK-8254263: Remove special_runtime_exit_condition() check from ~ThreadInVMForHandshake() With JDK-8254263 the special_runtime_exit_condition() check was removed from ~ThreadInVMForHandshake() because now a thread never becomes unsafe when processing its own handshakes. EscapeBarrier uses handshakes to sync with the target thread for object deoptimization so we add a check for object deoptimization to ThreadSafepointState::handle_polling_page_exception(). In JavaThread::wait_for_object_deoptimization() we must check is_obj_deopt_suspend() again after handshake/safepoint processing because a handshake for obj. deopt suspend could have been processed. - Adaptions to lazy/concurrent thread stack processing for ZGC (JEP 376) - EATests.java improvements - Merge branch 'master' into JDK-8227745 - The constructor of StackFrameStream takes more parameters after JDK-8253180 - Merge branch 'master' into JDK-8227745 - Merge branch 'master' into JDK-8227745 - Merge branch 'master' into JDK-8227745 - Factorized fragment out of EscapeBarrier::deoptimize_objects_internal into new method in compiledVFrame. - ... and 16 more: https://git.openjdk.java.net/jdk/compare/9359ff03...f02f07b6 - Changes: https://git.openjdk.java.net/jdk/pull/119/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=119&range=10 Stats: 5863 lines in 53 files changed: 5645 ins; 116 del; 102 mod Patch: https://git.openjdk.java.net/jdk/pull/119.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/119/head:pull/119 PR: https://git.openjdk.java.net/jdk/pull/119
Re: RFR: 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents [v10]
On Wed, 14 Oct 2020 00:25:14 GMT, Vladimir Kozlov wrote: > > > Good. Thanks for the review, Vladimir (@vnkozlov)! I'm still (stress) testing adaptations to lazy/concurrent thread stack processing for ZGC. --Richard. - PR: https://git.openjdk.java.net/jdk/pull/119
Re: RFR: 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents [v9]
On Sun, 11 Oct 2020 07:20:07 GMT, Richard Reingruber wrote: >>> >>> >>> I tried to run testing with latest changes and latest JDK and build failed: >>> src/hotspot/share/runtime/escapeBarrier.cpp:310:35: error: no matching >>> function for call to >>> 'StackFrameStream::StackFrameStream(JavaThread*&)' 310 | StackFrameStream >>> fst(deoptee); >> >> I noticed this too. I wanted to test with ZGC before pushing the small >> fix. Unfortunately I get >> >> # Internal Error >> (/priv/d038402/git/reinrich/jdk_ea_new/src/hotspot/share/runtime/stackWatermark.inline.hpp:67), >> pid=90890, tid=90912 # assert(processing_started()) failed: Processing >> should already have started >> >> [...] >> >> Current thread (0x7f749c25b1c0): JavaThread "JDWP Transport Listener: >> dt_socket" daemon [_thread_in_vm, id=90912, >> stack(0x7f7474c9f000,0x7f7474da)] >> _threads_hazard_ptr=0x7f749c2b00c0, _nested_threads_hazard_ptr_cnt=0 >> Stack: [0x7f7474c9f000,0x7f7474da], sp=0x7f7474d9c240, >> free space=1012k >> Native frames: (J=compiled Java code, A=aot compiled Java code, >> j=interpreted, Vv=VM code, C=native code) >> V [libjvm.so+0x15b3255] StackWatermarkSet::on_iteration(JavaThread*, frame >> const&)+0xa5 >> V [libjvm.so+0xa1024f] frame::sender(RegisterMap*) const+0x13f >> V [libjvm.so+0xa048f8] frame::real_sender(RegisterMap*) const+0x18 >> V [libjvm.so+0x176261b] vframe::sender() const+0xeb >> V [libjvm.so+0x16cd56b] JavaThread::last_java_vframe(RegisterMap*)+0x5b >> V [libjvm.so+0xfa7a56] JvmtiEnvBase::vframeFor(JavaThread*, int)+0x46 >> V [libjvm.so+0xfab8e5] JvmtiEnvBase::check_top_frame(JavaThread*, >> JavaThread*, jvalue, TosState, Handle*)+0x1f5 >> V [libjvm.so+0xfac13e] JvmtiEnvBase::force_early_return(JavaThread*, >> jvalue, TosState)+0x15e >> V [libjvm.so+0xf36fa8] jvmti_ForceEarlyReturnLong+0x258 >> C [libjdwp.so+0xa8b3] forceEarlyReturn+0x293 >> C [libjdwp.so+0x12945] debugLoop_run+0x1f5 >> C [libjdwp.so+0x25bb3] attachThread+0x33 >> V [libjvm.so+0xfcf524] JvmtiAgentThread::call_start_function()+0x1d4 >> V [libjvm.so+0x16cc8f7] JavaThread::thread_main_inner()+0x247 >> V [libjvm.so+0x16d1ce8] Thread::call_run()+0xf8 >> V [libjvm.so+0x12dd75e] thread_native_entry(Thread*)+0x10e >> >> In the test case >> `EAForceEarlyReturnOfInlinedMethodWithScalarReplacedObjectsReallocFailure` >> of the >> new test `jdk/com/sun/jdi/EATests.java` >> >> So far I do not have an indication that the failure is caused by this change >> but >> when I run the test with -XX:-DoEscapeAnalysis then the test succeeds. >> >> I need to look more into it. Wish I was a ZGC expert :) >> >> Anyway I pushed the build fix. Tests succeed with default GC. > > The crash described above happens after JDK-8253180 > (https://github.com/openjdk/jdk/commit/b9873e18330b7e43ca47bc1c0655e7ab20828f7a) > when executing `EATests.java` with > ZGC: make run-test TEST=test/jdk/com/sun/jdi/EATests.java > JTREG=VM_OPTIONS=-XX:+UseZGC > > My understanding of JDK-8253180 (and ZGC) is rather vague. To me it looks as > if stackwalks outside of a > safepoint/handshake on suspended threads are currently not supported. It > would be my understanding that > `StackWatermarkSet::start_processing()` needs to be called before walking the > stack of a thread. Currently this is only > done in preparation of a safepoint or handshake. > `JvmtiEnvBase::check_top_frame()` walks the stack of a suspended > thread without safepoint/handshake. This triggers the crash in my opinion. > When `StackWatermarkSet::start_processing()` > is called before the test succeeds. I will ask Erik Österlund about this. The issues with ZGC concurrent thread stack processing will be resolved with #627 - PR: https://git.openjdk.java.net/jdk/pull/119
Re: RFR: 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents [v3]
On Fri, 25 Sep 2020 10:44:53 GMT, David Holmes wrote: > > > The minor updates in response to my comments are fine. > > The more major updates ... I can't really comment on. Thanks for looking at the changes and for giving feedback. - PR: https://git.openjdk.java.net/jdk/pull/119
Re: RFR: 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents [v3]
> Hi, > > this is the continuation of the review of the implementation for: > > https://bugs.openjdk.java.net/browse/JDK-8227745 > https://bugs.openjdk.java.net/browse/JDK-8233915 > > It allows for JIT optimizations based on escape analysis even if JVMTI agents > acquire capabilities to access references > to objects that are subject to such optimizations, e.g. scalar replacement. > The implementation reverts such > optimizations just before access very much as when switching from JIT > compiled execution to the interpreter, aka > "deoptimization". Webrev.8 was the last one before before the transition to > Git/Github: > > http://cr.openjdk.java.net/~rrich/webrevs/8227745/webrev.8/ > > Thanks, Richard. Richard Reingruber has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision: - Merge branch 'master' into JDK-8227745 - Changes based on dholmes' feedback. EscapeBarrier::sync_and_suspend_all(): - Set suspend flags before handshake because then the setting will be visible after leaving the _thread_blocked state in JavaThread::wait_for_object_deoptimization() JavaThread::wait_for_object_deoptimization() - Reuse SpinYield instead of new custom spinning code. - Do safepoint check after the loop. This is possible because the set_obj_deopt_flag is done before the handshake. - Don't set_suspend_equivalent() anymore for more simplicity. It's just an optimization (that won't pay off here). Added/updated source code comments. Additional smaller enhancements. - 8227745, 8233915: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents - Changes: - all: https://git.openjdk.java.net/jdk/pull/119/files - new: https://git.openjdk.java.net/jdk/pull/119/files/18dd54b4..5bf631ba Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=119&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=119&range=01-02 Stats: 26592 lines in 830 files changed: 13744 ins; 9718 del; 3130 mod Patch: https://git.openjdk.java.net/jdk/pull/119.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/119/head:pull/119 PR: https://git.openjdk.java.net/jdk/pull/119
Re: RFR: 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents [v2]
On Mon, 14 Sep 2020 05:08:39 GMT, David Holmes wrote:
>> Richard Reingruber has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Changes based on dholmes' feedback.
>>
>> EscapeBarrier::sync_and_suspend_all():
>>
>> - Set suspend flags before handshake because then the setting will be
>> visible
>> after leaving the _thread_blocked state in
>> JavaThread::wait_for_object_deoptimization()
>>
>> JavaThread::wait_for_object_deoptimization()
>>
>> - Reuse SpinYield instead of new custom spinning code.
>>
>> - Do safepoint check after the loop. This is possible because the
>> set_obj_deopt_flag is done before the handshake.
>>
>> - Don't set_suspend_equivalent() anymore for more simplicity. It's just
>> an
>> optimization (that won't pay off here).
>>
>> Added/updated source code comments.
>>
>> Additional smaller enhancements.
>
> src/hotspot/share/prims/whitebox.cpp line 884:
>
>> 882:
>> 883: WB_ENTRY(jboolean, WB_IsFrameDeoptimized(JNIEnv* env, jobject o, jint
>> depth))
>> 884: JavaThread* t = JavaThread::current();
>
> A WB_ENTRY is a JNI_ENTRY which means the current JavaThread is already
> available via the `thread` variable.
Done.
> src/hotspot/share/runtime/thread.cpp line 2660:
>
>> 2658: // showed 5% better performance when spinning.
>> 2659: if (should_spin_wait) {
>> 2660: // Inspired by HandshakeSpinYield
>
> Can we not reuse any existing spinning code in the VM?
Good point.
I wasn't aware of SpinYield. I use it now for the spinning.
I use 10 * SpinYield::default_spin_limit as spin limit based on
microbenchmarking:
http://cr.openjdk.java.net/~rrich/webrevs/8227745/pr116v2.microbenchmark/
-
PR: https://git.openjdk.java.net/jdk/pull/119
Re: RFR: 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents [v2]
> Hi, > > this is the continuation of the review of the implementation for: > > https://bugs.openjdk.java.net/browse/JDK-8227745 > https://bugs.openjdk.java.net/browse/JDK-8233915 > > It allows for JIT optimizations based on escape analysis even if JVMTI agents > acquire capabilities to access references > to objects that are subject to such optimizations, e.g. scalar replacement. > The implementation reverts such > optimizations just before access very much as when switching from JIT > compiled execution to the interpreter, aka > "deoptimization". Webrev.8 was the last one before before the transition to > Git/Github: > > http://cr.openjdk.java.net/~rrich/webrevs/8227745/webrev.8/ > > Thanks, Richard. Richard Reingruber has updated the pull request incrementally with one additional commit since the last revision: Changes based on dholmes' feedback. EscapeBarrier::sync_and_suspend_all(): - Set suspend flags before handshake because then the setting will be visible after leaving the _thread_blocked state in JavaThread::wait_for_object_deoptimization() JavaThread::wait_for_object_deoptimization() - Reuse SpinYield instead of new custom spinning code. - Do safepoint check after the loop. This is possible because the set_obj_deopt_flag is done before the handshake. - Don't set_suspend_equivalent() anymore for more simplicity. It's just an optimization (that won't pay off here). Added/updated source code comments. Additional smaller enhancements. - Changes: - all: https://git.openjdk.java.net/jdk/pull/119/files - new: https://git.openjdk.java.net/jdk/pull/119/files/79f5b823..18dd54b4 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=119&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=119&range=00-01 Stats: 109 lines in 6 files changed: 54 ins; 36 del; 19 mod Patch: https://git.openjdk.java.net/jdk/pull/119.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/119/head:pull/119 PR: https://git.openjdk.java.net/jdk/pull/119
Re: RFR: 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents [v2]
On Mon, 14 Sep 2020 05:02:42 GMT, David Holmes wrote:
>> Richard Reingruber has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Changes based on dholmes' feedback.
>>
>> EscapeBarrier::sync_and_suspend_all():
>>
>> - Set suspend flags before handshake because then the setting will be
>> visible
>> after leaving the _thread_blocked state in
>> JavaThread::wait_for_object_deoptimization()
>>
>> JavaThread::wait_for_object_deoptimization()
>>
>> - Reuse SpinYield instead of new custom spinning code.
>>
>> - Do safepoint check after the loop. This is possible because the
>> set_obj_deopt_flag is done before the handshake.
>>
>> - Don't set_suspend_equivalent() anymore for more simplicity. It's just
>> an
>> optimization (that won't pay off here).
>>
>> Added/updated source code comments.
>>
>> Additional smaller enhancements.
>
> src/hotspot/share/opto/macro.cpp line 1096:
>
>> 1094: // interpreter frames for this compiled frame and that won't play
>> 1095: // nice with JVMTI popframe.
>> 1096: if (!EliminateAllocations || !alloc->_is_non_escaping) {
>
> The comment needs to be updated to match the new code.
Done.
> src/hotspot/share/runtime/thread.cpp line 1926:
>
>> 1924: delete dlv;
>> 1925: } while (deferred->length() != 0);
>> 1926: delete deferred_updates();
>
> This looks suspect - we delete what is pointed to but we don't NULL the field.
Fixed.
> src/hotspot/share/runtime/thread.cpp line 2640:
>
>> 2638: }
>> 2639:
>> 2640: void JavaThread::wait_for_object_deoptimization() {
>
> I find this method very complex, and the fact it needs to recheck many of the
> conditions already checked in the calling
> code, suggests to me that the structure here is not quite right. Perhaps
> JavaThread::handle_special_runtime_exit_condition should be the place where
> we loop over each of the potential
> conditions, instead of calling per-condition code that then loops and
> potentially rechecks other conditions.
For now I reshaped and hopefully also simplified the method. It should resemble
its model java_suspend_self_with_safepoint_check() very much now. With this
similarity I'd see the added complexity to be small. In fact
wait_for_object_deoptimization() could be a drop-in replacement for
java_suspend_self_with_safepoint_check(). Well, the condition in the callers
would have to be adapted.
> src/hotspot/share/runtime/thread.cpp line 2647:
>
>> 2645: bool should_spin_wait = true;
>> 2646: do {
>> 2647: set_thread_state(_thread_blocked);
>
> It isn't obvious that this raw thread state change is safe.
I added a comment explaining it before the method definition.
-
PR: https://git.openjdk.java.net/jdk/pull/119
Re: RFR: 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents
On Mon, 14 Sep 2020 04:58:47 GMT, David Holmes wrote:
>> Hi,
>>
>> this is the continuation of the review of the implementation for:
>>
>> https://bugs.openjdk.java.net/browse/JDK-8227745
>> https://bugs.openjdk.java.net/browse/JDK-8233915
>>
>> It allows for JIT optimizations based on escape analysis even if JVMTI
>> agents acquire capabilities to access references
>> to objects that are subject to such optimizations, e.g. scalar replacement.
>> The implementation reverts such
>> optimizations just before access very much as when switching from JIT
>> compiled execution to the interpreter, aka
>> "deoptimization". Webrev.8 was the last one before before the transition to
>> Git/Github:
>>
>> http://cr.openjdk.java.net/~rrich/webrevs/8227745/webrev.8/
>>
>> Thanks, Richard.
>
> src/hotspot/share/compiler/compileBroker.cpp line 844:
>
>> 842: void DeoptimizeObjectsALotThread::deoptimize_objects_alot_loop_single()
>> {
>> 843: HandleMark hm(this);
>> 844: while (!this->is_terminated()) {
>
> The terminated state of a thread is used to communicate to other threads
> whether this thread has terminated. It isn't
> something that can be used to control the work-loop of the thread itself!
> This is an infinite loop as far as I can see.
You're right. Thanks. I'll make it an explicit infinite loop. This should be ok
because DeoptimizeObjectsALotThread's
as the other threads created in CompileBroker::make_thread() are daemon threads.
> src/hotspot/share/compiler/compileBroker.cpp line 862:
>
>> 860: void DeoptimizeObjectsALotThread::deoptimize_objects_alot_loop_all() {
>> 861: HandleMark hm(this);
>> 862: while (!is_terminated()) {
>
> Same comment as above.
Same reply :)
-
PR: https://git.openjdk.java.net/jdk/pull/119
Re: RFR: 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents
On Mon, 14 Sep 2020 04:57:21 GMT, David Holmes wrote: >> Hi, >> >> this is the continuation of the review of the implementation for: >> >> https://bugs.openjdk.java.net/browse/JDK-8227745 >> https://bugs.openjdk.java.net/browse/JDK-8233915 >> >> It allows for JIT optimizations based on escape analysis even if JVMTI >> agents acquire capabilities to access references >> to objects that are subject to such optimizations, e.g. scalar replacement. >> The implementation reverts such >> optimizations just before access very much as when switching from JIT >> compiled execution to the interpreter, aka >> "deoptimization". Webrev.8 was the last one before before the transition to >> Git/Github: >> >> http://cr.openjdk.java.net/~rrich/webrevs/8227745/webrev.8/ >> >> Thanks, Richard. > > src/hotspot/share/compiler/compileBroker.cpp line 831: > >> 829: MonitorLocker ml(dt, EscapeBarrier_lock, >> Mutex::_no_safepoint_check_flag); >> 830: static int single_thread_count = 0; >> 831: enter_single_loop = single_thread_count++ < >> DeoptimizeObjectsALotThreadCountSingle; > > The update to `single_thread_count` is not atomic. I think it is atomic because it is never accessed without holding EscapeBarrier_lock - PR: https://git.openjdk.java.net/jdk/pull/119
Re: RFR: 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents
On Fri, 11 Sep 2020 09:54:50 GMT, Erik Helin wrote: > > > Sorry, now I see. Yes, please remove `, 8233915` from the title! Thanks for helping. The commit message does look better now. - PR: https://git.openjdk.java.net/jdk/pull/119
