Re: RFR: 8285739: disable EA when both JVMTI and Preview are enabled [v2]
On Fri, 20 May 2022 20:09:59 GMT, Leonid Mesnik wrote: >> The fix disables EscapeBarrier and EscapeAnalysis when certain JVMTI >> capabilities are enabled and --enable-preview. >> >> It restores the same behavior as it was before >> https://bugs.openjdk.java.net/browse/JDK-8227745 "Enable Escape Analysis for >> Better Performance in the Presence of JVMTI Agents" is implemented when >> Continuations are enabled. > > Leonid Mesnik has updated the pull request incrementally with one additional > commit since the last revision: > > 2nd v Update is good. - Marked as reviewed by kvn (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8589
Re: RFR: 8285739: disable EA when both JVMTI and Preview are enabled
On Mon, 9 May 2022 01:36:39 GMT, Leonid Mesnik wrote: > The fix disables EscapeBarrier and EscapeAnalysis when certain JVMTI > capabilities are enabled and --enable-preview. > > It restores the same behavior as it was before > https://bugs.openjdk.java.net/browse/JDK-8227745 "Enable Escape Analysis for > Better Performance in the Presence of JVMTI Agents" is implemented when > Continuations are enabled. Good. - Marked as reviewed by kvn (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8589
Re: RFR: 8284903: Fix typos in hotspot
On Fri, 15 Apr 2022 07:40:04 GMT, Magnus Ihse Bursie wrote: > I ran `codespell` on hotspot, and accepted those changes where it indeed > discovered real typos. > > You'd be surprised over the many implementions of instrinsics and other > intructions accross all archtectures I've encounted, so for the preceding > reason it's neccesery to sucessfully seach for exisiting typos... Nice work! I reviewed compiler related code: `cpu`, `os`, `os_cpu`, `share/adlc`, `share/c1`, `share/ci`, `share/code`, `share/compiler`, `share/jvmci`, `share/opto`. - Marked as reviewed by kvn (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8260
Re: RFR: 8276799: Implementation of JEP 422: Linux/RISC-V Port [v4]
On Wed, 23 Mar 2022 02:03:26 GMT, Fei Yang wrote:
>> This PR implements JEP 422: Linux/RISC-V Port [1].
>> The PR starts as a squashed merge of the
>> https://openjdk.java.net/projects/riscv-port branch.
>>
>> This has been tested with jtreg tier{1,2,3,4} and jcstress on HiFive
>> Unmatched board. Dacapo, SPECjbb2015 and SPECjvm2008 benchmark tests are
>> also carried out regularly. So it should be good enough to run most Java
>> programs.
>>
>> [1] https://openjdk.java.net/jeps/422
>
> Fei Yang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fix copyright header
Update looks good.
Testing results are also good.
-
Marked as reviewed by kvn (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/6294
Re: RFR: 8276799: Implementation of JEP 422: Linux/RISC-V Port [v3]
On Tue, 22 Mar 2022 11:50:13 GMT, Fei Yang wrote:
>> This PR implements JEP 422: Linux/RISC-V Port [1].
>> The PR starts as a squashed merge of the
>> https://openjdk.java.net/projects/riscv-port branch.
>>
>> This has been tested with jtreg tier{1,2,3,4} and jcstress on HiFive
>> Unmatched board. Dacapo, SPECjbb2015 and SPECjvm2008 benchmark tests are
>> also carried out regularly. So it should be good enough to run most Java
>> programs.
>>
>> [1] https://openjdk.java.net/jeps/422
>
> Fei Yang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Address review comments
src/hotspot/cpu/riscv/disassembler_riscv.hpp line 18:
> 16: *
> 17: * You should have received a copy of the GNU General Public License
> version
> 18: * 2 along with this work; if not, write to the Free Software Foundation,
> * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
These 2 lines merged into 1 accidentally causing failure in copyright headers
verification.
-
PR: https://git.openjdk.java.net/jdk/pull/6294
Re: RFR: 8276799: Implementation of JEP 422: Linux/RISC-V Port [v3]
On Tue, 22 Mar 2022 11:50:13 GMT, Fei Yang wrote:
>> This PR implements JEP 422: Linux/RISC-V Port [1].
>> The PR starts as a squashed merge of the
>> https://openjdk.java.net/projects/riscv-port branch.
>>
>> This has been tested with jtreg tier{1,2,3,4} and jcstress on HiFive
>> Unmatched board. Dacapo, SPECjbb2015 and SPECjvm2008 benchmark tests are
>> also carried out regularly. So it should be good enough to run most Java
>> programs.
>>
>> [1] https://openjdk.java.net/jeps/422
>
> Fei Yang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Address review comments
I looked on C1/C2 changes and compiler tests. Seems reasonable.
But before approval I would need to run changes through our testing.
-
PR: https://git.openjdk.java.net/jdk/pull/6294
Re: RFR: 8278423: ExtendedDTraceProbes should be deprecated [v8]
On Thu, 10 Feb 2022 08:46:50 GMT, Emanuel Peter wrote: >> Deprecated ExtendedDTraceProbes. >> Edited help messages and man pages accordingly, added the 3 flags to man >> pages. >> Added flag to VMDeprecatedOptions test. >> Replaced the flag with 3 flags in SDTProbesGNULinuxTest.java. >> >> Checked that tests are not affected. > > Emanuel Peter has updated the pull request incrementally with one additional > commit since the last revision: > > fixes to documentation requested by reviewers Good. - Marked as reviewed by kvn (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7110
Deprecate DTrace support
Hi The question about dropping DTrace support in HotSpot VM rose during discussion 8278423: "ExtendedDTraceProbes should be deprecated" [1]. It is not clear for me about Oracle support for it after we dropped SPARC. Do we still need to support it or we can drop it (remove code from HotSpot)? Thanks, Vladimir K [1] https://bugs.openjdk.java.net/browse/JDK-8278423
Re: [jdk18] RFR: 8278239: vmTestbase/nsk/jvmti/RedefineClasses/StressRedefine failed with EXCEPTION_ACCESS_VIOLATION at 0x000000000000000d
On Tue, 21 Dec 2021 20:51:04 GMT, Coleen Phillimore wrote: > This is the fix for https://github.com/openjdk/jdk/pull/6900 retargeted to > JDK 18. > > Thanks to @stefank and @fisk for the diagnosis. ZGC is looking at metadata in > unloaded nmethods, but redefinition doesn't keep this metadata from being > deallocated. Change the iterators that mark metadata in use to walk unloaded > nmethods as well as other alive nmethods. > > The test case reproduces the crash on windows if run in an 100 iteration > loop. This fix does not crash in the same test. Also ran tier1-6. > > Above testing in progress. okay - Marked as reviewed by kvn (Reviewer). PR: https://git.openjdk.java.net/jdk18/pull/63
Re: RFR: 8274982: Add a test for 8269574.
On Mon, 11 Oct 2021 09:55:28 GMT, Evgeny Nikitin wrote: > This PR contains a relatively simple test which verifies that JVMTI-agents > are correctly informed about exceptions caught in C2-compiled code. The > 8269574 introduces pre-allocated exceptions in some paths, so the test tries > to produce a number of various exceptions and check that provided small JVMTI > agent got notified about all of them. I will leave approval to @sspitsyn and @dholmes-ora - PR: https://git.openjdk.java.net/jdk/pull/5889
Re: RFR: 8274982: Add a test for 8269574.
On Mon, 11 Oct 2021 09:55:28 GMT, Evgeny Nikitin wrote: > This PR contains a relatively simple test which verifies that JVMTI-agents > are correctly informed about exceptions caught in C2-compiled code. The > 8269574 introduces pre-allocated exceptions in some paths, so the test tries > to produce a number of various exceptions and check that provided small JVMTI > agent got notified about all of them. I am fine with changes. What testing was done? What testing tiers were run? - PR: https://git.openjdk.java.net/jdk/pull/5889
Re: RFR: 8274982: Add a test for 8269574.
On Mon, 11 Oct 2021 09:55:28 GMT, Evgeny Nikitin wrote: > This PR contains a relatively simple test which verifies that JVMTI-agents > are correctly informed about exceptions caught in C2-compiled code. The > 8269574 introduces pre-allocated exceptions in some paths, so the test tries > to produce a number of various exceptions and check that provided small JVMTI > agent got notified about all of them. Someone from serviceability group have to look on this test too. - PR: https://git.openjdk.java.net/jdk/pull/5889
Re: RFR: 8275729: Qualified method names in CodeHeap Analytics
On Mon, 1 Nov 2021 20:51:39 GMT, Evgeny Astigeevich wrote: > This PR changes nmethods names in `METHOD NAMES for CodeHeap` section to be > qualified. > Testing: > - `make test TEST="gtest"`: Passed > - `make run-test TEST="tier1"`: Passed > - `make run-test TEST="tier2"`: Passed > - `make run-test > TEST=serviceability/dcmd/compiler/CodeHeapAnalyticsMethodNames.java`: Passed BTW, you need to update Copyright year in file. - PR: https://git.openjdk.java.net/jdk/pull/6200
Re: RFR: 8275729: Qualified method names in CodeHeap Analytics
On Tue, 2 Nov 2021 23:03:22 GMT, Evgeny Astigeevich wrote: > Is NULL method holder an acceptable situation? Could it be a sign of a bug? You are right, all methods should have class holders. I just followed code pattern. > BTW, `Klass::external_name()` returns `` if `Klass::name()` is > `NULL`. I see, you want to have the same string instead of ``. Reasonable. I will test your changes too. File PR and I will review and post testing results. - PR: https://git.openjdk.java.net/jdk/pull/6200
Re: RFR: 8275729: Qualified method names in CodeHeap Analytics
On Mon, 1 Nov 2021 20:51:39 GMT, Evgeny Astigeevich
wrote:
> This PR changes nmethods names in `METHOD NAMES for CodeHeap` section to be
> qualified.
> Testing:
> - `make test TEST="gtest"`: Passed
> - `make run-test TEST="tier1"`: Passed
> - `make run-test TEST="tier2"`: Passed
> - `make run-test
> TEST=serviceability/dcmd/compiler/CodeHeapAnalyticsMethodNames.java`: Passed
Yes, I am currently testing similar fix:
-Klass* klass = method->method_holder();
-assert(klass->is_loader_alive(), "must be alive");
+Klass* methHolder = method->method_holder();
+const char*methHolderS = (methHolder == NULL) ? NULL :
methHolder->external_name();
+methHolderS = (methHolderS == NULL) ? "" : methHolderS;
-ast->print("%s.", klass->external_name());
+ast->print("%s.", methHolderS);
Note, failed test is `closed` so I have to run testing.
-
PR: https://git.openjdk.java.net/jdk/pull/6200
Re: RFR: 8275729: Qualified method names in CodeHeap Analytics
On Mon, 1 Nov 2021 20:51:39 GMT, Evgeny Astigeevich wrote: > This PR changes nmethods names in `METHOD NAMES for CodeHeap` section to be > qualified. > Testing: > - `make test TEST="gtest"`: Passed > - `make run-test TEST="tier1"`: Passed > - `make run-test TEST="tier2"`: Passed > - `make run-test > TEST=serviceability/dcmd/compiler/CodeHeapAnalyticsMethodNames.java`: Passed I don't think we need this assert just to print klass's name - PR: https://git.openjdk.java.net/jdk/pull/6200
Re: RFR: 8275729: Qualified method names in CodeHeap Analytics
On Mon, 1 Nov 2021 20:51:39 GMT, Evgeny Astigeevich wrote: > This PR changes nmethods names in `METHOD NAMES for CodeHeap` section to be > qualified. > Testing: > - `make test TEST="gtest"`: Passed > - `make run-test TEST="tier1"`: Passed > - `make run-test TEST="tier2"`: Passed > - `make run-test > TEST=serviceability/dcmd/compiler/CodeHeapAnalyticsMethodNames.java`: Passed The change caused failure in our testing: https://bugs.openjdk.java.net/browse/JDK-8276429 @eastig I will assign it to you - PR: https://git.openjdk.java.net/jdk/pull/6200
Re: RFR: 8218885: Restore pop_frame and force_early_return functionality for Graal
On Wed, 22 Sep 2021 05:40:40 GMT, Tom Rodriguez wrote: > This logic no longer seems to be necessary since the adjustCompilationLevel > callback has been removed. Yes, running locally with GraalVM is fine. - Marked as reviewed by kvn (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5625
Re: RFR: 8218885: Restore pop_frame and force_early_return functionality for Graal
On Wed, 22 Sep 2021 05:40:40 GMT, Tom Rodriguez wrote: > This logic no longer seems to be necessary since the adjustCompilationLevel > callback has been removed. @tkrodriguez Did you test this changes with GraalVM? Would be nice to run it with command line which Serguei pointed. We will be fine if it passed with changes. - PR: https://git.openjdk.java.net/jdk/pull/5625
Re: [jdk17] RFR: 8067223: [TESTBUG] Rename Whitebox API package [v2]
On Sat, 31 Jul 2021 20:42:10 GMT, Igor Ignatyev wrote: >> Hi all, >> >> could you please review this big tedious and trivial(-ish) patch which moves >> `sun.hotspot.WhiteBox` and related classes to `jdk.test.whitebox` package? >> >> the majority of the patch is the following substitutions: >> - `s~sun/hotspot/WhiteBox~jdk/test/whitebox/WhiteBox~g` >> - `s/sun.hotspot.parser/jdk.test.whitebox.parser/g` >> - `s/sun.hotspot.cpuinfo/jdk.test.whitebox.cpuinfo/g` >> - `s/sun.hotspot.code/jdk.test.whitebox.code/g` >> - `s/sun.hotspot.gc/jdk.test.whitebox.gc/g` >> - `s/sun.hotspot.WhiteBox/jdk.test.whitebox.WhiteBox/g` >> >> testing: tier1-4 >> >> Thanks, >> -- Igor > > Igor Ignatyev has refreshed the contents of this pull request, and previous > commits have been removed. The incremental views will show differences > compared to the previous content of the PR. I agree with these revised changes for JDK 17. - Marked as reviewed by kvn (Reviewer). PR: https://git.openjdk.java.net/jdk17/pull/290
Re: [jdk17] RFR: 8067223: [TESTBUG] Rename Whitebox API package
On Wed, 28 Jul 2021 17:13:49 GMT, Igor Ignatyev wrote: > Hi all, > > could you please review this big tedious and trivial(-ish) patch which moves > `sun.hotspot.WhiteBox` and related classes to `jdk.test.whitebox` package? > > the majority of the patch is the following substitutions: > - `s~sun/hotspot/WhiteBox~jdk/test/whitebox/WhiteBox~g` > - `s/sun.hotspot.parser/jdk.test.whitebox.parser/g` > - `s/sun.hotspot.cpuinfo/jdk.test.whitebox.cpuinfo/g` > - `s/sun.hotspot.code/jdk.test.whitebox.code/g` > - `s/sun.hotspot.gc/jdk.test.whitebox.gc/g` > - `s/sun.hotspot.WhiteBox/jdk.test.whitebox.WhiteBox/g` > > testing: tier1-4 > > Thanks, > -- Igor I know that tests fixes could be pushed during RDP2 without approval. But these one is very big and it is not a fix - it is enhancement. What is the reason you want to push it into JDK 17 just few days before first Release Candidate? Instead of pushing it into JDK 18. And I can't even review it because GutHub UI hangs on these big changes. - PR: https://git.openjdk.java.net/jdk17/pull/290
Re: RFR: 8271323: [TESTBUG] serviceability/sa/ClhsdbCDSCore.java fails with -XX:TieredStopAtLevel=1
On Tue, 27 Jul 2021 09:24:38 GMT, Nick Gasson wrote: > This test fails reliably with `-XX:TieredStopAtLevel=1` since JDK-8251462. > MDOs are no longer allocated in this mode so the clhsdb `printmdo -a` command > prints nothing. The failure is basically the same as JDK-8236042 which > happened with `-Xcomp -XX:TieredStopAtLevel=1` so the workaround for that > just needs to be adjusted slightly. Good and trivial. - Marked as reviewed by kvn (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4910
Re: RFR: 8261236: C2: ClhsdbJstackXcompStress test fails when StressGCM is enabled
On Fri, 16 Jul 2021 10:24:56 GMT, Nick Gasson wrote:
> The clhsdb jstack command crashes when the debugged program is run with
> `-Xcomp -XX:+StressGCM -XX:StressSeed=2` on AArch64:
>
>
> sun.jvm.hotspot.utilities.AssertionFailure: sanity check
> at jdk.hotspot.agent/sun.jvm.hotspot.utilities.Assert.that(Assert.java:32)
> at
> jdk.hotspot.agent/sun.jvm.hotspot.runtime.RegisterMap.setLocation(RegisterMap.java:160)
> at
> jdk.hotspot.agent/sun.jvm.hotspot.compiler.ImmutableOopMapSet.updateRegisterMap(ImmutableOopMapSet.java:303)
> at
> jdk.hotspot.agent/sun.jvm.hotspot.runtime.aarch64.AARCH64Frame.senderForCompiledFrame(AARCH64Frame.java:407)
>
>
> The assertion failure here is:
>
>
> Assert.that(0 <= i && i < regCount, "sanity check");
>
>
> I.e. there's an invalid register in the oop map.
>
> The problem seems to be caused by the changes in JDK-8231586 which changed
> `OopMapValue::oop_types` from a bit mask to normal integer enum. However the
> changes in the C++ code weren't mirrored in SA's OopMapStream which still
> treats OopTypes as a bit mask.
>
> The particular oop map this is crashing on looks like this:
>
>
> ImmutableOopMap {[24]=Oop [32]=Oop [40]=Derived_oop_[24] } pc offsets: 324
>
>
> The code is looking for callee saved values (type=2) by AND-ing with each oop
> value type in the set, so it wrongly interprets the derived oop [40] (type=3)
> as a callee saved register.
>
> This patch just mirrors the changes to the C++ code into the corresponding SA
> classes. The C++ OopMapStream constructor no longer takes a type filter
> argument and callers are expected filter themselves, so I've made the same
> change to the Java code.
>
> This bug can also be seen using the clhsdb "disassemble" command. For
> example the above oop map is currently printed incorrectly as:
>
>
> OopMap:
> NarrowOops:[40]
> Callee saved:[40] = [24]
> Derived oops:[40] = [24]
>
>
> With this patch it becomes:
>
>
> OopMap:
> Oops:[24] [32]
> Derived oops:[40] = [24]
>
>
> This bug was reported on AArch64 but it seems to be just luck that we don't
> see it on other platforms.
>
> Tested tier1 and hotspot_serviceability on AArch64 and x86.
Good.
-
Marked as reviewed by kvn (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/4807
Re: RFR: 8269652: Factor out the common code for creating system j.l.Thread objects
On Fri, 2 Jul 2021 07:03:50 GMT, David Holmes wrote: > Please review this simple refactoring to share the common code used to create > j.l.Thread instances for the internal VM JavaThreads. (Also fix a missing > space from my previous change in thread.cpp.) > > It is all very straight-forward. > > Compiler folk I can go one step further and remove > CompileBroker::create_thread_oop if desired. > > Testing: tiers 1-3, GHA > > Thanks, > David Good. - Marked as reviewed by kvn (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4663
Re: RFR: 8269466: Factor out the common code for initializing and starting internal VM JavaThreads [v4]
On Thu, 1 Jul 2021 22:17:27 GMT, David Holmes wrote:
>> Please see the JBS issue for more details, but basically we have 8 different
>> kinds of internal VM JavaThreads (grouping the three types of CompilerThread
>> together) that all basically duplicated the logic for initializing
>> (preparing is the term we use for user-defined JavaThreads) and starting the
>> new thread. This common code can be factored out into static helpers in
>> JavaThread.
>>
>> This change does not look at the way the java.lang.Thread instance is
>> created - that will be handled by a separate RFE.
>>
>> The semantics of the changes are not identical, but I don't believe there is
>> any observable change in behaviour. The scope of holding the Threads_lock
>> has been reduced, and we now create the JavaThread instances ("new
>> XXXThread(...)") outside of the lock. As far as I can see nothing in the
>> construction process needs to happen under the Threads_lock.
>>
>> A few of the threads use a static `_instance` field to hold a reference to
>> the create JavaThread. This proved very difficult to handle, as logically
>> the field would need to be updated in the middle of the new factored-out
>> method: after setting all the fields but before releasing the newly started
>> thread. I eventually realized that in all but one case those `_instance`
>> fields are never used and so could be deleted. The one case remaining does
>> not need to be set as I just described, but can be set after the thread has
>> started, as the new thread does not examine it (arguably its existence is
>> unnecessary).
>>
>> The trickiest changes related to the CompilerThreads, so they need
>> particular scrutiny.
>>
>> Testing: tiers 1-3
>>
>> Thanks,
>> David
>
> David Holmes has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Restore null check to appease static analysis.
> Add assertion on osthread()
Marked as reviewed by kvn (Reviewer).
-
PR: https://git.openjdk.java.net/jdk/pull/4629
Re: RFR: 8269466: Factor out the common code for initializing and starting internal VM JavaThreads [v3]
On Thu, 1 Jul 2021 22:02:40 GMT, David Holmes wrote:
>> src/hotspot/share/compiler/compileBroker.cpp line 939:
>>
>>> 937: && comp->num_compiler_threads() > 0) {
>>> 938: // The new thread is not known to Thread-SMR yet so we can just
>>> delete.
>>> 939: delete new_thread;
>>
>> Need `new_thread != NULL` check if you do as I suggested in previous comment.
>
> I don't think so, you can apply `delete` to a NULL pointer (whereas
> previously we could not call `smr_delete` on a NULL pointer.
okay then.
-
PR: https://git.openjdk.java.net/jdk/pull/4629
Re: RFR: 8269466: Factor out the common code for initializing and starting internal VM JavaThreads [v3]
On Thu, 1 Jul 2021 04:18:28 GMT, David Holmes wrote:
>> Please see the JBS issue for more details, but basically we have 8 different
>> kinds of internal VM JavaThreads (grouping the three types of CompilerThread
>> together) that all basically duplicated the logic for initializing
>> (preparing is the term we use for user-defined JavaThreads) and starting the
>> new thread. This common code can be factored out into static helpers in
>> JavaThread.
>>
>> This change does not look at the way the java.lang.Thread instance is
>> created - that will be handled by a separate RFE.
>>
>> The semantics of the changes are not identical, but I don't believe there is
>> any observable change in behaviour. The scope of holding the Threads_lock
>> has been reduced, and we now create the JavaThread instances ("new
>> XXXThread(...)") outside of the lock. As far as I can see nothing in the
>> construction process needs to happen under the Threads_lock.
>>
>> A few of the threads use a static `_instance` field to hold a reference to
>> the create JavaThread. This proved very difficult to handle, as logically
>> the field would need to be updated in the middle of the new factored-out
>> method: after setting all the fields but before releasing the newly started
>> thread. I eventually realized that in all but one case those `_instance`
>> fields are never used and so could be deleted. The one case remaining does
>> not need to be set as I just described, but can be set after the thread has
>> started, as the new thread does not examine it (arguably its existence is
>> unnecessary).
>>
>> The trickiest changes related to the CompilerThreads, so they need
>> particular scrutiny.
>>
>> Testing: tiers 1-3
>>
>> Thanks,
>> David
>
> David Holmes has updated the pull request incrementally with three additional
> commits since the last revision:
>
> - Rename vm_exit_on_thread_allocation_failure to vm_exit_on_osthread_failure
> - Adjust comment
> - Comments from PR review:
>- remove unnecessary new_thread NULL checks
>- adjust some comments
>- fix whitespace
Few comments for compileBroker.cpp changes.
src/hotspot/share/compiler/compileBroker.cpp line 909:
> 907: // JavaThread due to lack of resources. We will handle that failure
> below.
> 908:
> 909: if (new_thread->osthread() != NULL) {
I think you do need to check `new_thread != NULL` here to please Parfait for
`default:` case.
src/hotspot/share/compiler/compileBroker.cpp line 939:
> 937: && comp->num_compiler_threads() > 0) {
> 938: // The new thread is not known to Thread-SMR yet so we can just
> delete.
> 939: delete new_thread;
Need `new_thread != NULL` check if you do as I suggested in previous comment.
-
PR: https://git.openjdk.java.net/jdk/pull/4629
Re: [jdk17] RFR: 8269691: ProblemList sun/management/jdp/JdpDefaultsTest.java on Linux-aarch64
On Wed, 30 Jun 2021 17:27:00 GMT, Daniel D. Daugherty wrote: > A trivial fix to ProblemList sun/management/jdp/JdpDefaultsTest.java on > Linux-aarch64. trivial - Marked as reviewed by kvn (Reviewer). PR: https://git.openjdk.java.net/jdk17/pull/182
Re: RFR: 8264941: Remove CodeCache::mark_for_evol_deoptimization() method
On Wed, 16 Jun 2021 12:52:46 GMT, Coleen Phillimore wrote: > This change removes the mark_for_evol_deoptimization method and removes the > flag that all dependencies are recorded. Before the change to walk the > entire nmethod looking for "old" (redefined) methods with metadata_do(), we > used to find methods in the code cache to deoptimize based on evol_method > dependencies. If the dependencies weren't yet recorded, we had to deoptimize > all of the methods. A long time ago, we had a customer who was unhappy with > the pause for this when they had late attach. Now we don't have this problem. > The evol_method dependencies are still used by the compiler to check for old > methods during compilation. I didn't change this but it might be something > someone who knows the compiler better can do differently and remove these > dependencies too. > Tested with tier1-6. Good. - Marked as reviewed by kvn (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4509
Re: RFR: 8267464: Circular-dependency resilient inline headers [v7]
On Fri, 28 May 2021 07:24:37 GMT, Stefan Karlsson wrote:
>> I'd like to propose a small adjustment to how we write .inline.hpp files, in
>> the hope to reduce include problems because of circular dependencies between
>> inline headers.
>>
>> This is a small, contrived example to show the problem:
>>
>> // a.hpp
>> #pragma once
>>
>> void a1();
>> void a2();
>>
>>
>> // a.inline.hpp
>> #pragma once
>>
>> #include "a.hpp"
>> #include "b.inline.hpp"
>>
>> inline void a1() {
>> b1();
>> }
>>
>> inline void a2() {
>> }
>>
>>
>> // b.hpp
>> #pragma once
>>
>> void b1();
>> void b2();
>>
>>
>> // b.inline.hpp
>> #pragma once
>>
>> #include "a.inline.hpp"
>> #include "b.hpp"
>>
>> inline void b1() {
>> }
>>
>> inline void b2() {
>> a1();
>> }
>>
>> The following code compiles fine:
>>
>> // a.cpp
>> #include "a.inline.hpp"
>>
>> int main() {
>> a1();
>>
>> return 0;
>> }
>>
>> But the following:
>>
>> // b.cpp
>> #include "b.inline.hpp"
>>
>> int main() {
>> b1();
>>
>> return 0;
>> }
>>
>>
>> fails with the this error message:
>>
>> In file included from b.inline.hpp:3,
>> from b.cpp:1:
>> a.inline.hpp: In function ‘void a1()’:
>> a.inline.hpp:8:3: error: ‘b1’ was not declared in this scope; did you mean
>> ‘a1’?
>>
>> We can see the problem with g++ -E:
>>
>> # 1 "a.inline.hpp" 1
>> # 1 "a.hpp" 1
>>
>> void a1();
>> void a2();
>>
>> # 4 "a.inline.hpp" 2
>>
>> inline void a1() {
>> b1();
>> }
>>
>> inline void a2() {
>> }
>>
>> # 4 "b.inline.hpp" 2
>> # 1 "b.hpp" 1
>>
>> void b1();
>> void b2();
>>
>> # 5 "b.inline.hpp" 2
>>
>> inline void b1() {
>> }
>>
>> inline void b2() {
>> a1();
>> }
>>
>> # 2 "b.cpp" 2
>>
>> int main() {
>> b1();
>>
>> return 0;
>> }
>>
>>
>> b1() is called before it has been declared. b.inline.hpp inlined
>> a.inline.hpp, which uses functions declared in b.hpp. However, at that point
>> we've not included enough of b.inline.hpp to have declared b1().
>>
>> This is a small and easy example. In the JVM the circular dependencies
>> usually involves more than two files, and often from different sub-systems
>> of the JVM. We have so-far solved this by restructuring the code, making
>> functions out-of-line, creating proxy objects, etc. However, the more we use
>> templates, the more this is going to become a reoccurring nuisance. And in
>> experiments with lambdas, which requires templates, this very quickly showed
>> up as a problem.
>>
>> I propose that we solve most (all?) of these issues by adding a rule that
>> states that .inline.hpp files should start by including the corresponding
>> .hpp, and that the .hpp should contain the declarations of all externally
>> used parts of the .inline.hpp file. This should guarantee that the
>> declarations are always present before any subsequent include can create a
>> circular include dependency back to the original file.
>>
>> In the example above, b.inline.hpp would become:
>>
>> // b.inline.hpp
>> #pragma once
>>
>> #include "b.hpp"
>> #include "a.inline.hpp"
>>
>> inline void b1() {
>> }
>>
>> inline void b2() {
>> a1();
>> }
>>
>> and now both a.cpp and b.cpp compiles. The generated output now looks like
>> this:
>>
>> # 1 "b.inline.hpp" 1
>> # 1 "b.hpp" 1
>>
>> void b1();
>> void b2();
>>
>> # 4 "b.inline.hpp" 2
>> # 1 "a.inline.hpp" 1
>>
>> # 1 "a.hpp" 1
>>
>> void a1();
>> void a2();
>>
>> # 4 "a.inline.hpp" 2
>>
>> inline void a1() {
>> b1();
>> }
>>
>> inline void a2() {
>> }
>>
>> # 5 "b.inline.hpp" 2
>>
>> inline void b1() {
>> }
>>
>> inline void b2() {
>> a1();
>> }
>>
>> # 2 "b.cpp" 2
>>
>> int main() {
>> b1();
>>
>> return 0;
>> }
>>
>> The declarations come first, and the compiler is happy.
>>
>> An alternative to this, that has been proposed by other HotSpot GC devs have
>> been to always include all .hpp files first, and then have a section of
>> .inline.hpp includes. I've experimented with that as well, but I think it
>> requires more maintenance and vigilance of *users* .inline.hpp files (add
>> .hpp include to the first section, add .inline.hpp section to second). The
>> proposed structures only requires extra care from *writers* of .inline.hpp
>> files. All others can include .hpp and .inline.hpp as we've been doing for a
>> long time now.
>>
>> Some notes about this patch:
>> 1) Some externally-used declarations have been placed in .inline.hpp files,
>> and not in the .hpp. Those should be moved. I have not done that.
>> 2) Some .inline.hpp files don't have a corresponding .hpp file. I could
>> either try to fix that in this patch, or we could take care of that as
>> separate patches later.
>> 3) The style I chose was to add the .hpp include and a extra blank line,
>> separating it from the rest of the includes. I think I like that style,
>> because it makes it easy for those writing .inline.hpp to recognize this
>> pattern. And it removes the confusion why this include isn't so
Re: RFR: 8252685: APIs that require JavaThread should take JavaThread arguments [v2]
On Tue, 11 May 2021 01:56:21 GMT, David Holmes wrote: >> Our code is littered with API's that take, or manifest, a Thread* and then >> assert/guarantee that it must be a JavaThread, rather than >> taking/manifesting a JavaThread in the first place. The main reason for this >> is that the TRAPS macro, used in relation to exception generation and >> processing, is declared as "Thread* THREAD". In practice only JavaThreads >> that can execute Java code can generate arbitrary exceptions, because it >> requires executing Java code. In other places we can get away with other >> kinds of threads "throwing" exceptions because they are only pre-allocated >> instances that require no Java code execution (e.g. OOME), or when executed >> by a non-JavaThread the code actually by-passes the logic would throw an >> exception. Such code also eventually clears the exception and reports the >> OOM by some other means. We have been progressively untangling these code >> paths and modifying TRAPS/CHECK usage so that only JavaThreads will call >> TRAPS methods and throw exceptions. Having done t hat pre-work we are now ready to convert TRAPS to be "JavaThread* THREAD" and that is what this change does. The resulting changes are largely mechanical: >> >> - declarations of "Thread* THREAD" become "JavaThread* THREAD" (where THREAD >> is needed for exceptions, otherwise THREAD is renamed to current) >> - methods that took a Thread* parameter that was always THREAD, now take a >> JavaThread* param >> - Manifestations of Thread::current() become JavaThread::current() >> - THREAD->as_Java_thread() becomes just THREAD >> - THREAD->is_Java_thread() is reworked so that THREAD is "current" >> >> There are still places where a CompilerThread (which is a JavaThread but may >> not be able to execute Java code) calls a TRAPS function (primarily where >> OOME is possible). Fixing that would be a future RFE and may not be possible >> without reworking a lot of the allocation code paths. >> >> Testing: >> - tiers 1-8 on Linux-x64 >> - all builds in tiers 1-4 >> - tiers 1-3 on all platforms >> >> Thanks, >> David > > David Holmes has updated the pull request incrementally with one additional > commit since the last revision: > > Review feedback from Serguei Compiler related changes seems fine. @dougxc, note that jvmci is affected. It looks reasonable for me but you may need to check if it is correct from libgraal POV. - Marked as reviewed by kvn (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/3877
Integrated: 8264805: Remove the experimental Ahead-of-Time Compiler
On Thu, 8 Apr 2021 15:23:52 GMT, Vladimir Kozlov wrote: > As part of [JEP 410](http://openjdk.java.net/jeps/410) remove code related to > Ahead-of-Time Compiler from JDK: > > - `jdk.aot` module — the `jaotc` tool > - `src/hotspot/share/aot` — loads AoT compiled code into VM for execution > - related HotSpot code guarded by `#if INCLUDE_AOT` > > Additionally, remove tests as well as code in makefiles related to AoT > compilation. > > Tested hs-tier1-4 This pull request has now been integrated. Changeset: 694acedf Author:Vladimir Kozlov URL: https://git.openjdk.java.net/jdk/commit/694acedf Stats: 26972 lines in 378 files changed: 2 ins; 26772 del; 198 mod 8264805: Remove the experimental Ahead-of-Time Compiler Reviewed-by: coleenp, erikj, stefank, iignatyev, dholmes, aph, shade, iklam, mchung, iveresov - PR: https://git.openjdk.java.net/jdk/pull/3398
Re: RFR: 8264805: Remove the experimental Ahead-of-Time Compiler [v6]
> As part of [JEP 410](http://openjdk.java.net/jeps/410) remove code related to > Ahead-of-Time Compiler from JDK: > > - `jdk.aot` module — the `jaotc` tool > - `src/hotspot/share/aot` — loads AoT compiled code into VM for execution > - related HotSpot code guarded by `#if INCLUDE_AOT` > > Additionally, remove tests as well as code in makefiles related to AoT > compilation. > > Tested hs-tier1-4 Vladimir Kozlov has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains six commits: - Merge branch 'master' into JDK-8264805 - Address review comments - Remove exports from Graal module to jdk.aot - Merge branch 'master' into JDK-8264805 - Merge branch 'master' into JDK-8264805 - 8264805: Remove the experimental Ahead-of-Time Compiler - Changes: https://git.openjdk.java.net/jdk/pull/3398/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3398&range=05 Stats: 26972 lines in 378 files changed: 2 ins; 26772 del; 198 mod Patch: https://git.openjdk.java.net/jdk/pull/3398.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3398/head:pull/3398 PR: https://git.openjdk.java.net/jdk/pull/3398
Re: RFR: 8265180: JvmtiCompiledMethodLoadEvent should include the stub section of nmethods
On Wed, 14 Apr 2021 00:35:55 GMT, Tom Rodriguez wrote: > 8265180: JvmtiCompiledMethodLoadEvent should include the stub section of > nmethods Good. - Marked as reviewed by kvn (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/3481
Re: RFR: 8264805: Remove the experimental Ahead-of-Time Compiler [v5]
> As part of [JEP 410](http://openjdk.java.net/jeps/410) remove code related to > Ahead-of-Time Compiler from JDK: > > - `jdk.aot` module — the `jaotc` tool > - `src/hotspot/share/aot` — loads AoT compiled code into VM for execution > - related HotSpot code guarded by `#if INCLUDE_AOT` > > Additionally, remove tests as well as code in makefiles related to AoT > compilation. > > Tested hs-tier1-4 Vladimir Kozlov has updated the pull request incrementally with one additional commit since the last revision: Address review comments - Changes: - all: https://git.openjdk.java.net/jdk/pull/3398/files - new: https://git.openjdk.java.net/jdk/pull/3398/files/6cce0f6c..71a166c1 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3398&range=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3398&range=03-04 Stats: 36 lines in 9 files changed: 0 ins; 27 del; 9 mod Patch: https://git.openjdk.java.net/jdk/pull/3398.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3398/head:pull/3398 PR: https://git.openjdk.java.net/jdk/pull/3398
Re: RFR: 8264805: Remove the experimental Ahead-of-Time Compiler [v4]
On Fri, 9 Apr 2021 16:54:35 GMT, Ioi Lam wrote:
>> Vladimir Kozlov has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Remove exports from Graal module to jdk.aot
>
> src/hotspot/share/oops/methodCounters.cpp line 77:
>
>> 75: }
>> 76:
>> 77: void MethodCounters::metaspace_pointers_do(MetaspaceClosure* it) {
>
> MethodCounters::metaspace_pointers_do can be removed altogether (also need to
> remove the declaration in methodCounter.hpp).
Done.
-
PR: https://git.openjdk.java.net/jdk/pull/3398
Re: RFR: 8264805: Remove the experimental Ahead-of-Time Compiler [v4]
On Fri, 9 Apr 2021 16:34:58 GMT, Igor Veresov wrote:
>> Vladimir Kozlov has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Remove exports from Graal module to jdk.aot
>
> src/hotspot/share/jvmci/jvmciCodeInstaller.cpp line 1184:
>
>> 1182: }
>> 1183: } else if
>> (jvmci_env()->isa_HotSpotMetaspaceConstantImpl(constant)) {
>> 1184: if (!_immutable_pic_compilation) {
>
> All _immutable_pic_compilation mentions can be removed as well. It is true
> only for AOT compiles produced by Graal. It's never going to be used without
> AOT.
Done. I removed _immutable_pic_compilation in JVMCI in Hotspot.
-
PR: https://git.openjdk.java.net/jdk/pull/3398
Re: RFR: 8264805: Remove the experimental Ahead-of-Time Compiler [v4]
On Fri, 9 Apr 2021 16:30:41 GMT, Igor Veresov wrote: >> Vladimir Kozlov has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Remove exports from Graal module to jdk.aot > > src/hotspot/share/oops/instanceKlass.hpp line 257: > >> 255: _misc_declares_nonstatic_concrete_methods = 1 << 6, // directly >> declares non-static, concrete methods >> 256: _misc_has_been_redefined = 1 << 7, // class has >> been redefined >> 257: _unused = 1 << 8, // > > Any particular reason we don't want to remove this gap? Less changes. We don't get any benefits from removing it. It is opposite - if we need a new value we will use it without changing following values again. - PR: https://git.openjdk.java.net/jdk/pull/3398
Re: RFR: 8264805: Remove the experimental Ahead-of-Time Compiler [v4]
On Fri, 9 Apr 2021 08:32:59 GMT, Aleksey Shipilev wrote:
>> Vladimir Kozlov has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Remove exports from Graal module to jdk.aot
>
> src/hotspot/share/code/compiledIC.cpp line 715:
>
>> 713: tty->print("interpreted");
>> 714: } else {
>> 715: tty->print("unknown");
>
> We can probably split this cleanup into the minor issue, your call. The
> benefit of separate issue: backportability.
Reverted and filed https://bugs.openjdk.java.net/browse/JDK-8265013
-
PR: https://git.openjdk.java.net/jdk/pull/3398
Re: RFR: 8264805: Remove the experimental Ahead-of-Time Compiler [v4]
On Fri, 9 Apr 2021 03:03:33 GMT, David Holmes wrote:
>> Vladimir Kozlov has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Remove exports from Graal module to jdk.aot
>
> src/hotspot/share/memory/heap.hpp line 174:
>
>> 172: bool contains(const void* p) const { return low() <= p &&
>> p < high(); }
>> 173: bool contains_blob(const CodeBlob* blob) const {
>> 174: const void* start = (void*)blob;
>
> start seems redundant now
Done:
bool contains_blob(const CodeBlob* blob) const {
-const void* start = (void*)blob;
-return contains(start);
+return contains((void*)blob);
}
-
PR: https://git.openjdk.java.net/jdk/pull/3398
Re: RFR: 8264805: Remove the experimental Ahead-of-Time Compiler [v4]
On Fri, 9 Apr 2021 08:29:21 GMT, Aleksey Shipilev wrote: >> Vladimir Kozlov has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Remove exports from Graal module to jdk.aot > > src/hotspot/cpu/x86/globalDefinitions_x86.hpp line 73: > >> 71: >> 72: #if INCLUDE_JVMCI >> 73: #define COMPRESSED_CLASS_POINTERS_DEPENDS_ON_COMPRESSED_OOPS >> (EnableJVMCI) > > Minor nit: can probably drop parentheses here. done - PR: https://git.openjdk.java.net/jdk/pull/3398
Re: RFR: 8264805: Remove the experimental Ahead-of-Time Compiler [v4]
On Fri, 9 Apr 2021 02:44:23 GMT, David Holmes wrote: >> Vladimir Kozlov has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Remove exports from Graal module to jdk.aot > > src/hotspot/cpu/x86/compiledIC_x86.cpp line 134: > >> 132: #ifdef ASSERT >> 133: CodeBlob *cb = CodeCache::find_blob_unsafe((address) _call); >> 134: assert(cb, "sanity"); > > Nit: implied boolean - use "cb != NULL" done - PR: https://git.openjdk.java.net/jdk/pull/3398
Re: RFR: 8264805: Remove the experimental Ahead-of-Time Compiler [v4]
On Fri, 9 Apr 2021 17:09:58 GMT, Vladimir Kozlov wrote: >> Hi Vladimir, >> >> This looks good to me - I went through all the files. >> >> It was nice to see how nicely compartmentalized the AOT feature was - that >> made checking the changes quite simple. The one exception was the >> fingerprinting code, which for some reason was AOT-only but not marked that >> way, so I'm not sure what the story is there. ?? >> >> I was also surprised to see some of the flags were not marked experimental, >> so there will need to a CSR request to remove those without going through >> the normal deprecation process. >> >> Thanks, >> David > >> Hi Vladimir, >> >> This looks good to me - I went through all the files. >> >> It was nice to see how nicely compartmentalized the AOT feature was - that >> made checking the changes quite simple. The one exception was the >> fingerprinting code, which for some reason was AOT-only but not marked that >> way, so I'm not sure what the story is there. ?? >> >> I was also surprised to see some of the flags were not marked experimental, >> so there will need to a CSR request to remove those without going through >> the normal deprecation process. >> >> Thanks, >> David > > Thank you, David. > We thought that we could use fingerprint code for other cases that is why we > did not put it under `#if INCLUDE_AOT`. > I filed CSR for AOT product flags removal: > https://bugs.openjdk.java.net/browse/JDK-8265000 Thank you, Igor Ignatyev, Igor Veresov, Ioi, Aleksey and Andrew for reviews. I will update changes based on your comments. - PR: https://git.openjdk.java.net/jdk/pull/3398
Re: RFR: 8264805: Remove the experimental Ahead-of-Time Compiler [v4]
On Fri, 9 Apr 2021 04:32:14 GMT, David Holmes wrote: > Hi Vladimir, > > This looks good to me - I went through all the files. > > It was nice to see how nicely compartmentalized the AOT feature was - that > made checking the changes quite simple. The one exception was the > fingerprinting code, which for some reason was AOT-only but not marked that > way, so I'm not sure what the story is there. ?? > > I was also surprised to see some of the flags were not marked experimental, > so there will need to a CSR request to remove those without going through the > normal deprecation process. > > Thanks, > David Thank you, David. We thought that we could use fingerprint code for other cases that is why we did not put it under `#if INCLUDE_AOT`. I filed CSR for AOT product flags removal: https://bugs.openjdk.java.net/browse/JDK-8265000 - PR: https://git.openjdk.java.net/jdk/pull/3398
Re: RFR: 8264805: Remove the experimental Ahead-of-Time Compiler [v4]
On Thu, 8 Apr 2021 20:59:59 GMT, Coleen Phillimore wrote:
> There's a comment above
> void VM_RedefineClasses::mark_dependent_code(InstanceKlass* ik) {
> that should be rewritten, so I think we should just file an RFE to remove it
> afterwards.
https://bugs.openjdk.java.net/browse/JDK-8264941
-
PR: https://git.openjdk.java.net/jdk/pull/3398
Re: RFR: 8264805: Remove the experimental Ahead-of-Time Compiler [v4]
On Thu, 8 Apr 2021 20:00:30 GMT, Vladimir Kozlov wrote:
>> GC changes look good.
>
>> void CodeCache::mark_for_evol_deoptimization(InstanceKlass* dependee) {
>> This function, its caller and the function in jvmtiRedefineClasses.cpp that
>> calls it can be deleted also, but you can file a separate RFE for that if
>> you want.
>
> Thank you, Coleen, for review. I will wait other comments and will remove
> this code.
Thank you, Erik and Stefan.
-
PR: https://git.openjdk.java.net/jdk/pull/3398
Re: RFR: 8264805: Remove the experimental Ahead-of-Time Compiler [v4]
On Thu, 8 Apr 2021 19:58:11 GMT, Stefan Karlsson wrote:
>> Vladimir Kozlov has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Remove exports from Graal module to jdk.aot
>
> GC changes look good.
> void CodeCache::mark_for_evol_deoptimization(InstanceKlass* dependee) {
> This function, its caller and the function in jvmtiRedefineClasses.cpp that
> calls it can be deleted also, but you can file a separate RFE for that if you
> want.
Thank you, Coleen, for review. I will wait other comments and will remove this
code.
-
PR: https://git.openjdk.java.net/jdk/pull/3398
Re: RFR: 8264805: Remove the experimental Ahead-of-Time Compiler [v4]
> As part of [JEP 410](http://openjdk.java.net/jeps/410) remove code related to > Ahead-of-Time Compiler from JDK: > > - `jdk.aot` module — the `jaotc` tool > - `src/hotspot/share/aot` — loads AoT compiled code into VM for execution > - related HotSpot code guarded by `#if INCLUDE_AOT` > > Additionally, remove tests as well as code in makefiles related to AoT > compilation. > > Tested hs-tier1-4 Vladimir Kozlov has updated the pull request incrementally with one additional commit since the last revision: Remove exports from Graal module to jdk.aot - Changes: - all: https://git.openjdk.java.net/jdk/pull/3398/files - new: https://git.openjdk.java.net/jdk/pull/3398/files/3dbc6210..6cce0f6c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3398&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3398&range=02-03 Stats: 39 lines in 3 files changed: 0 ins; 36 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/3398.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3398/head:pull/3398 PR: https://git.openjdk.java.net/jdk/pull/3398
Re: RFR: 8264805: Remove the experimental Ahead-of-Time Compiler [v3]
> As part of [JEP 410](http://openjdk.java.net/jeps/410) remove code related to > Ahead-of-Time Compiler from JDK: > > - `jdk.aot` module — the `jaotc` tool > - `src/hotspot/share/aot` — loads AoT compiled code into VM for execution > - related HotSpot code guarded by `#if INCLUDE_AOT` > > Additionally, remove tests as well as code in makefiles related to AoT > compilation. > > Tested hs-tier1-4 Vladimir Kozlov has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains three commits: - Merge branch 'master' into JDK-8264805 - Merge branch 'master' into JDK-8264805 - 8264805: Remove the experimental Ahead-of-Time Compiler - Changes: https://git.openjdk.java.net/jdk/pull/3398/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3398&range=02 Stats: 26906 lines in 375 files changed: 4 ins; 26709 del; 193 mod Patch: https://git.openjdk.java.net/jdk/pull/3398.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3398/head:pull/3398 PR: https://git.openjdk.java.net/jdk/pull/3398
Re: RFR: 8264805: Remove the experimental Ahead-of-Time Compiler [v2]
> As part of [JEP 410](http://openjdk.java.net/jeps/410) remove code related to > Ahead-of-Time Compiler from JDK: > > - `jdk.aot` module — the `jaotc` tool > - `src/hotspot/share/aot` — loads AoT compiled code into VM for execution > - related HotSpot code guarded by `#if INCLUDE_AOT` > > Additionally, remove tests as well as code in makefiles related to AoT > compilation. > > Tested hs-tier1-4 Vladimir Kozlov has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains two commits: - Merge branch 'master' into JDK-8264805 - 8264805: Remove the experimental Ahead-of-Time Compiler - Changes: https://git.openjdk.java.net/jdk/pull/3398/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3398&range=01 Stats: 26906 lines in 375 files changed: 4 ins; 26709 del; 193 mod Patch: https://git.openjdk.java.net/jdk/pull/3398.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3398/head:pull/3398 PR: https://git.openjdk.java.net/jdk/pull/3398
RFR: 8264805: Remove the experimental Ahead-of-Time Compiler
As part of [JEP 410](http://openjdk.java.net/jeps/410) remove code related to Ahead-of-Time Compiler from JDK: - `jdk.aot` module — the `jaotc` tool - `src/hotspot/share/aot` — loads AoT compiled code into VM for execution - related HotSpot code guarded by `#if INCLUDE_AOT` Additionally, remove tests as well as code in makefiles related to AoT compilation. Tested hs-tier1-4 - Commit messages: - 8264805: Remove the experimental Ahead-of-Time Compiler Changes: https://git.openjdk.java.net/jdk/pull/3398/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3398&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8264805 Stats: 26903 lines in 375 files changed: 4 ins; 26703 del; 196 mod Patch: https://git.openjdk.java.net/jdk/pull/3398.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3398/head:pull/3398 PR: https://git.openjdk.java.net/jdk/pull/3398
Re: RFR: 8261447: MethodInvocationCounters frequently run into overflow [v10]
On Wed, 3 Mar 2021 13:28:21 GMT, Lutz Schmidt wrote: >> Dear community, >> may I please request reviews for this fix, improving the usefulness of >> method invocation counters. >> - aggregation counters are retyped as uint64_t, shifting the overflow >> probability way out (> 500 years in case of a 1 GHz counter update >> frequency). >> - counters for individual methods are interpreted as (unsigned int), in >> contrast to their declaration as int. This gives us a factor of two before >> the counters overflow. >> - as a special case, "compiled_invocation_counter" is retyped as long, >> because it has a higher update frequency than other counters. >> - before/after sample output is attached to the bug description. >> >> Thank you! >> Lutz > > Lutz Schmidt has updated the pull request incrementally with one additional > commit since the last revision: > > revert copyright change to get rid of unchanged file Good. - Marked as reviewed by kvn (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2511
Re: RFR: 8261447: MethodInvocationCounters frequently run into overflow [v8]
On Tue, 2 Mar 2021 17:23:23 GMT, Lutz Schmidt wrote: >> src/hotspot/cpu/x86/vtableStubs_x86_32.cpp line 159: >> >>> 157: return NULL; >>> 158: } >>> 159: >> >> Why you did not update asm instruction to update `nof_megamorphic_calls` in >> this file? > > The reason is plain simple: there is no incrementq() for x86_32. I could > emulate that with a few lines like > address ctrAddr = (address)SharedRuntime::nof_megamorphic_calls_addr(); > __ lea(rscratch1, ExternalAddress(ctrAddr)); > __ addl(Address(rscratch1, 0), 1); > __ adcl(Address(rscratch1, 4), 0); > Not sure if that would be desirable here. Just let me know. As is, the code > just updates the less significant half of the 8-byte counter. Okay, let keep as it is. Then revert this file back - the only change is new empty line. - PR: https://git.openjdk.java.net/jdk/pull/2511
Re: RFR: 8261447: MethodInvocationCounters frequently run into overflow [v8]
On Thu, 25 Feb 2021 09:01:10 GMT, Lutz Schmidt wrote:
>> Dear community,
>> may I please request reviews for this fix, improving the usefulness of
>> method invocation counters.
>> - aggregation counters are retyped as uint64_t, shifting the overflow
>> probability way out (> 500 years in case of a 1 GHz counter update
>> frequency).
>> - counters for individual methods are interpreted as (unsigned int), in
>> contrast to their declaration as int. This gives us a factor of two before
>> the counters overflow.
>> - as a special case, "compiled_invocation_counter" is retyped as long,
>> because it has a higher update frequency than other counters.
>> - before/after sample output is attached to the bug description.
>>
>> Thank you!
>> Lutz
>
> Lutz Schmidt has updated the pull request incrementally with one additional
> commit since the last revision:
>
> comment changes requested by TheRealMDoerr
I have few comments.
src/hotspot/cpu/x86/vtableStubs_x86_32.cpp line 159:
> 157: return NULL;
> 158: }
> 159:
Why you did not update asm instruction to update `nof_megamorphic_calls` in
this file?
src/hotspot/share/oops/method.cpp line 530:
> 528:
> 529: if (method_data() != NULL) {
> 530: unsigned int dcc= (unsigned int)method_data()->decompile_count();
decompile_count() returns `uint` why do cast and why you check decompile_count
for overflow? It is very rare updated and limited by
`PerMethodRecompilationCutoff` flag (400 by default):
https://github.com/openjdk/jdk/blob/master/src/hotspot/share/oops/methodData.hpp#L2391
src/hotspot/share/oops/method.cpp line 518:
> 516: // Print a "overflow" notification to create awareness.
> 517: const char* addMsg;
> 518: unsigned int maxInt = (1U<<31) - 1;
Why not use INT_MAX?
-
PR: https://git.openjdk.java.net/jdk/pull/2511
Re: RFR: 8251462: Remove legacy compilation policy [v5]
On Sun, 24 Jan 2021 03:53:00 GMT, Igor Veresov wrote:
>> This change removes the legacy compilation policy and an emulation mode to
>> the tiered policy to simulate the old behavior with
>> ```-XX:-TieredCompilation```. The change removed a bunch of interpreter
>> code, devirtualizes the compilation policy API, adds a consistent way to
>> query compiler configuration with the new ```CompilerConfig``` API.
>>
>> I've tested this with hs-tier{1,2,3,4,5}. And also made sure it builds and
>> works with C1/C2-Graal/AOT being enabled/disabled.
>>
>> Since there are platform-specific changes I would greatly appreciate some
>> help from the maintainers of the specific ports to verify the build and run
>> basic smoke tests. I've already tested x64 and aarch64. Thanks!
>
> Igor Veresov has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Address Vladimir's review comments
I approve it based on performance results.
-
Marked as reviewed by kvn (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/1985
Re: RFR: 8251462: Remove legacy compilation policy [v5]
On Sun, 24 Jan 2021 03:53:00 GMT, Igor Veresov wrote:
>> This change removes the legacy compilation policy and an emulation mode to
>> the tiered policy to simulate the old behavior with
>> ```-XX:-TieredCompilation```. The change removed a bunch of interpreter
>> code, devirtualizes the compilation policy API, adds a consistent way to
>> query compiler configuration with the new ```CompilerConfig``` API.
>>
>> I've tested this with hs-tier{1,2,3,4,5}. And also made sure it builds and
>> works with C1/C2-Graal/AOT being enabled/disabled.
>>
>> Since there are platform-specific changes I would greatly appreciate some
>> help from the maintainers of the specific ports to verify the build and run
>> basic smoke tests. I've already tested x64 and aarch64. Thanks!
>
> Igor Veresov has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Address Vladimir's review comments
Looks better now. I would like to see result of performance testing before
approving.
-
PR: https://git.openjdk.java.net/jdk/pull/1985
Re: RFR: 8251462: Remove legacy compilation policy [v4]
On Fri, 22 Jan 2021 20:17:24 GMT, Igor Veresov wrote: >> I don't think so: >>> java -Xint -XX:+UnlockExperimentalVMOptions -XX:+EnableJVMCI -version >> Java HotSpot(TM) 64-Bit Server VM warning: JVMCI Compiler disabled due to >> -Xint. >> >>> java -Xint -XX:+UnlockExperimentalVMOptions -XX:+UseAOT -XX:+PrintAOT >>> -version >> Java HotSpot(TM) 64-Bit Server VM warning: -Xint is not compatible with AOT >> (switching AOT off) > > You're right. Perhaps I was overly defensive. It doesn't hurt though, less > potential surprises in the future (as you correctly noticed those are used to > make sure there are no nmethods produced by any other compiler but C1). > Also you still can have a C1 and AOT combination. Yes. You can have C1 in combination with AOT and JVMCI (hosted). I looked on use cases for these 4 methods and it seems it will be hard to only use one. Okay. Leave them as it is. May be add comments to explain why you needs these variants. - PR: https://git.openjdk.java.net/jdk/pull/1985
Re: RFR: 8251462: Remove legacy compilation policy [v4]
On Fri, 22 Jan 2021 19:50:48 GMT, Igor Veresov wrote:
>> src/hotspot/share/compiler/compilerDefinitions.cpp line 84:
>>
>>> 82: } else if (CompilerConfig::is_c2_or_jvmci_compiler_only()) {
>>> 83: _mode = Mode::HIGH_ONLY;
>>> 84: } else if (CompilerConfig::is_jvmci_compiler() &&
>>> !TieredCompilation) {
>>
>> Should you check `CompilerConfig::is_tiered()` instead of
>> `TieredCompilation` flag?
>
> I wanted to be explicit about what's happening here. I'd like it to be
> obvious that the we're switching to ```HIGH_ONLY_QUICK_INTERNAL``` mode as a
> result of the user specifying -XX:-TieredCompilation on the command line.
>
> There is a bug on this line however. I should be checking if c1 is present
> and enabled.
okay
-
PR: https://git.openjdk.java.net/jdk/pull/1985
Re: RFR: 8251462: Remove legacy compilation policy [v4]
On Fri, 22 Jan 2021 18:26:43 GMT, Igor Veresov wrote: >> Hmm, may be we don't need these funxctions. >> Seems is_c1_only() will be true only when JVMCI is off. Right? We can also >> reinforce it to exclude AOT too. >> And AOT and JVMCI are disabled with -Xint. > > It's not so easy. You can have AOT or JVMCI enabled with -Xint. JVMCI code > installs can be initiated not only as a result of method going through the > compiler broker. Remember that you can run Graal in "host" mode, where it is > a normal C1/C2 system (or may be just an interpreter); Truffle can work in a > mode like that. And you can totally run with AOT and interpreter (without any > JIT). I don't think so: > java -Xint -XX:+UnlockExperimentalVMOptions -XX:+EnableJVMCI -version Java HotSpot(TM) 64-Bit Server VM warning: JVMCI Compiler disabled due to -Xint. > java -Xint -XX:+UnlockExperimentalVMOptions -XX:+UseAOT -XX:+PrintAOT -version Java HotSpot(TM) 64-Bit Server VM warning: -Xint is not compatible with AOT (switching AOT off) - PR: https://git.openjdk.java.net/jdk/pull/1985
Re: RFR: 8251462: Remove legacy compilation policy [v4]
On Fri, 22 Jan 2021 18:15:40 GMT, Igor Veresov wrote:
>> src/hotspot/share/compiler/compilerDefinitions.cpp line 62:
>>
>>> 60: }
>>> 61: } else if (strcmp(CompilationMode, "high-only") == 0) {
>>> 62: if (!CompilerConfig::has_c2() &&
>>> !CompilerConfig::is_jvmci_compiler()) {
>>
>> Is using `!is_c2_or_jvmci_compiler_available()` better here? All flags
>> should be processed at this point. And we could have some
>> `TieredStopAtLevel` flags.
>> It looks like you have cross dependency between CompilerConfig and
>> CompilationModeFlag which make using `is_c2_or_jvmci_compiler_available()`
>> impossible here.
>
> Yes, there is an unfortunate cross dependency. It would probably work fine
> since ```_mode``` is initialized to ```NORMAL``` before parsing the flag. But
> I wanted to not use any function that would depend on the value of the
> ```_mode``` during parsing. It could create trouble in the future. I'll put
> more comments about that.
okay.
-
PR: https://git.openjdk.java.net/jdk/pull/1985
Re: RFR: 8251462: Remove legacy compilation policy [v4]
On Fri, 22 Jan 2021 15:19:39 GMT, Igor Veresov wrote:
>> src/hotspot/share/compiler/compilerDefinitions.hpp line 234:
>>
>>> 232: static bool is_interpreter_only() {
>>> 233: return Arguments::is_interpreter_only() || TieredStopAtLevel ==
>>> CompLevel_none;
>>> 234: }
>>
>> Can you move these functions after has_*() functions? They are used before.
>
> Right now has_() functions are defined before uses. Do you want them to be
> after the uses? Please confirm.
Don't touch has_() - they are in correct place. I am asking only to move these
4 is_() functions between has_() and is_c1_only() function.
>> src/hotspot/share/compiler/compilerDefinitions.hpp line 243:
>>
>>> 241: static bool is_c1_only_no_aot_or_jvmci() {
>>> 242: return is_c1_only() && !is_aot() && !is_jvmci();
>>> 243: }
>>
>> These names are a little confusing: what about C2, why only no AOT and
>> JVMCI. I understand that you want to check if JVMCI or AOT can install their
>> compiled code.
>> May be `is_c1_nmethods_only`, `is_c1_nmethods_or_interpreter_only` ?
>
> I guess it's a matter of naming convention. What I tried to make the
> CompilerConfig API about is to check if compilers are present/enabled and in
> which combination. Of course presence of a compiler implies that we're going
> to see nmethod produced by it. I'd like to keep the current naming if it's
> not a huge eyesore for you.
Hmm, may be we don't need these funxctions.
Seems is_c1_only() will be true only when JVMCI is off. Right? We can also
reinforce it to exclude AOT too.
And AOT and JVMCI are disabled with -Xint.
-
PR: https://git.openjdk.java.net/jdk/pull/1985
Re: RFR: 8251462: Remove legacy compilation policy [v3]
On Fri, 22 Jan 2021 17:14:15 GMT, Igor Veresov wrote: >> src/hotspot/share/aot/aotCodeHeap.cpp line 194: >> >>> 192: // AOT libs are loaded before heap initialized so shift values are >>> not set. >>> 193: // It is okay since ObjectAlignmentInBytes flag which defines shifts >>> value is set before AOT libs are loaded. >>> 194: // Set shifts value based on first AOT library config. >> >> Why this code is removed? > > It's always running with the tiered policy now. Prior to this change if you > attempted to run an AOT library with tiered-style profiling compiled in with > the non-tiered policy it wouldn't work correctly. That's why we had this > check. Now tiered is the only policy, so it will work correctly whether you > have the AOT code with profiling or not. Okay. Got it - PR: https://git.openjdk.java.net/jdk/pull/1985
Re: RFR: 8260296: SA's dumpreplaydata fails
On Fri, 22 Jan 2021 14:21:52 GMT, Roland Westrelin wrote: > I noticed that the SA's dumpreplaydata command fails with: > > java.lang.AssertionError: CLHSDB wasn't run successfully: Opening core file, > please wait... > hsdb> Exception in thread "main" java.lang.InternalError: ciMetadata does not > appear to be polymorphic > > with a simple test program. This happens because the SA can't find the > vtable symbol for ciMetadata (build produced by gcc 9.2.1). AFAIU, > there's nothing in our build system that hides that symbol. I had to > move one method's definition from the header file to the cpp file for > the symbol to be visible again. > > We have a test that checks dumpreplaydata but it doesn't catch that > problem. The test produces a replay file from a core file with the SA > by running a simple test with -Xcomp and CICrash=1. So the replay data > has very little or no profile data (which is what causes the problem > above). I propose running a slightly more complicated test method and > crashing after the method has had time to run for long enough to > collect profile data. > > The other shortcoming of the test is that it doesn't look at the > content of the replay file. It only warns if they differ. The replay > file produced by the VM and the one produced by the SA should be > identical (except for comment lines). So I propose we check that. > > Finally, I can't run that test on my system because core files are > handled by systemd (I'm running some recent version of fedora). I > suppose, the system can be configured differently but having the test > work out the box is nice. I extended the test case to handle that. > > With the improved test, there are a few differences between the VM and > SA replay files caused by VM changes that were not mirrored in the > SA. I fixed those. Looks good to me. - Marked as reviewed by kvn (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2195
Re: RFR: 8251462: Remove legacy compilation policy [v4]
On Tue, 12 Jan 2021 05:04:11 GMT, Igor Veresov wrote:
>> This change removes the legacy compilation policy and an emulation mode to
>> the tiered policy to simulate the old behavior with
>> ```-XX:-TieredCompilation```. The change removed a bunch of interpreter
>> code, devirtualizes the compilation policy API, adds a consistent way to
>> query compiler configuration with the new ```CompilerConfig``` API.
>>
>> I've tested this with hs-tier{1,2,3,4,5}. And also made sure it builds and
>> works with C1/C2-Graal/AOT being enabled/disabled.
>>
>> Since there are platform-specific changes I would greatly appreciate some
>> help from the maintainers of the specific ports to verify the build and run
>> basic smoke tests. I've already tested x64 and aarch64. Thanks!
>
> Igor Veresov has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Check legacy flags validity before deriving flag values for emulation mode.
I would also suggest to do performance runs. Ask Eric for help. Changes are
significant and may affect performance due to some typo.
src/hotspot/share/compiler/compilerDefinitions.hpp line 234:
> 232: static bool is_interpreter_only() {
> 233: return Arguments::is_interpreter_only() || TieredStopAtLevel ==
> CompLevel_none;
> 234: }
Can you move these functions after has_*() functions? They are used before.
src/hotspot/share/compiler/compilerDefinitions.hpp line 179:
> 177: }
> 178: // Is the JVM in a configuration that permits only c2-compiled methods?
> 179: // JVMCI compiler replaces C2.
The comment `JVMCI compiler replaces C2.` should be removed or moved to
`is_jvmci_compiler_only()` where it is make more sense.
src/hotspot/share/compiler/compilerDefinitions.hpp line 171:
> 169: }
> 170:
> 171: static bool is_c2_available() {
For me `_available` suffix sounds similar to `has_`. May be `_enabled` is
better. At least it is less confusing where it is called.
src/hotspot/cpu/aarch64/c1_LIRGenerator_aarch64.cpp line 1414:
> 1412: // use LD;DMB but stores use STLR. This can happen if C2 compiles
> 1413: // the stores in one method and C1 compiles the loads in another.
> 1414: if (!CompilerConfig::is_c1_or_interpreter_only_no_aot_or_jvmci()) {
It is C1 code which should not be executed in -Xint. Why check
`interpreter_only`?
src/hotspot/cpu/aarch64/gc/shenandoah/c1/shenandoahBarrierSetC1_aarch64.cpp
line 54:
> 52: ShenandoahBarrierSet::assembler()->cmpxchg_oop(masm->masm(), addr,
> cmpval, newval, /*acquire*/ true, /*release*/ true, /*is_cae*/ false, result);
> 53:
> 54: if (CompilerConfig::is_c1_or_interpreter_only_no_aot_or_jvmci()) {
Again about `interpreter_only` check.
src/hotspot/share/compiler/compilerDefinitions.hpp line 243:
> 241: static bool is_c1_only_no_aot_or_jvmci() {
> 242: return is_c1_only() && !is_aot() && !is_jvmci();
> 243: }
These names are a little confusing: what about C2, why only no AOT and JVMCI. I
understand that you want to check if JVMCI or AOT can install their compiled
code.
May be `is_c1_nmethods_only`, `is_c1_nmethods_or_interpreter_only` ?
src/hotspot/share/oops/method.cpp line 1013:
> 1011: return false;
> 1012: if (comp_level == CompLevel_any)
> 1013: return is_not_c1_compilable() && is_not_c2_compilable();
Was it bug before?
src/hotspot/share/compiler/compilerDefinitions.cpp line 62:
> 60: }
> 61: } else if (strcmp(CompilationMode, "high-only") == 0) {
> 62: if (!CompilerConfig::has_c2() &&
> !CompilerConfig::is_jvmci_compiler()) {
Is using `!is_c2_or_jvmci_compiler_available()` better here? All flags should
be processed at this point. And we could have some `TieredStopAtLevel` flags.
It looks like you have cross dependency between CompilerConfig and
CompilationModeFlag which make using `is_c2_or_jvmci_compiler_available()`
impossible here.
src/hotspot/share/compiler/compilerDefinitions.cpp line 84:
> 82: } else if (CompilerConfig::is_c2_or_jvmci_compiler_only()) {
> 83: _mode = Mode::HIGH_ONLY;
> 84: } else if (CompilerConfig::is_jvmci_compiler() && !TieredCompilation)
> {
Should you check `CompilerConfig::is_tiered()` instead of `TieredCompilation`
flag?
-
PR: https://git.openjdk.java.net/jdk/pull/1985
Re: RFR: 8251462: Remove legacy compilation policy [v3]
On Thu, 7 Jan 2021 23:06:19 GMT, Igor Veresov wrote:
>> This change removes the legacy compilation policy and an emulation mode to
>> the tiered policy to simulate the old behavior with
>> ```-XX:-TieredCompilation```. The change removed a bunch of interpreter
>> code, devirtualizes the compilation policy API, adds a consistent way to
>> query compiler configuration with the new ```CompilerConfig``` API.
>>
>> I've tested this with hs-tier{1,2,3,4,5}. And also made sure it builds and
>> works with C1/C2-Graal/AOT being enabled/disabled.
>>
>> Since there are platform-specific changes I would greatly appreciate some
>> help from the maintainers of the specific ports to verify the build and run
>> basic smoke tests. I've already tested x64 and aarch64. Thanks!
>
> Igor Veresov has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fix another s390 compilation failure
src/hotspot/share/aot/aotCodeHeap.cpp line 194:
> 192: // AOT libs are loaded before heap initialized so shift values are not
> set.
> 193: // It is okay since ObjectAlignmentInBytes flag which defines shifts
> value is set before AOT libs are loaded.
> 194: // Set shifts value based on first AOT library config.
Why this code is removed?
src/hotspot/share/compiler/compileBroker.cpp line 972:
> 970:
> 971: // Initialize the compilation queue
> 972: if (_c2_count > 0) {
Is ZERO treated as `is_interpreter_only()` ? How this change works with ZERO?
src/hotspot/share/runtime/vmStructs.cpp line 296:
> 294: JVMTI_ONLY(nonstatic_field(MethodCounters, _number_of_breakpoints,
> u2)) \
> 295: nonstatic_field(MethodCounters, _invocation_counter,
> InvocationCounter) \
> 296: nonstatic_field(MethodCounters, _backedge_counter,
> InvocationCounter) \
You need to fix SA agent:
https://github.com/openjdk/jdk/blob/master/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/MethodCounters.java#L52
src/hotspot/share/jvmci/vmStructs_jvmci.cpp line 229:
> 227: JVMTI_ONLY(nonstatic_field(MethodCounters, _number_of_breakpoints,
> u2)) \
> 228: nonstatic_field(MethodCounters, _invocation_counter,
> InvocationCounter) \
> 229: nonstatic_field(MethodCounters, _backedge_counter,
> InvocationCounter) \
I don't see Java JVMCI changes. Do you need them?
test/hotspot/jtreg/TEST.quick-groups line 1787:
> 1785: vmTestbase/jit/t/t112/t112.java \
> 1786: vmTestbase/jit/t/t113/t113.java \
> 1787: vmTestbase/jit/verifier/VerifyInitLocal/VerifyInitLocal.java \
Why this test removed?
-
PR: https://git.openjdk.java.net/jdk/pull/1985
Re: RFR: 8256424: Move ciSymbol::symbol_name() to ciSymbols::symbol_name()
On Tue, 17 Nov 2020 12:53:48 GMT, Claes Redestad wrote: > This moves the mirroring of vmSymbols in ciSymbols to a separate file, > ciSymbols.hpp, to reduce includes throughout hotspot (and clean up the > ciSymbol namespace). In a few places code is moved from .hpp to .cpp to > facilitate this. > > With PCH disabled, this reduces total includes from 276949 to 276332 Marked as reviewed by kvn (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1256
Re: RFR: 8256741: Reduce footprint of compiler interface data structures
On Fri, 20 Nov 2020 12:19:48 GMT, Claes Redestad wrote: > A few data structure in the ci allocate unconditionally created > GrowableArrays out-of-line, have fields that are newer updated/read, or are > unnecessarily cached. By cleaning this up we can slightly reduce memory used > for JIT compilations while slightly speeding them up. Good. - Marked as reviewed by kvn (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1346
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: 8255616: Disable AOT and Graal in Oracle OpenJDK
On Sun, 1 Nov 2020 20:15:01 GMT, Igor Veresov 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` > > Marked as reviewed by iveresov (Reviewer). Thank you for reviews. - PR: https://git.openjdk.java.net/jdk/pull/960
RFR: 8255616: Disable AOT and Graal in Oracle OpenJDK
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` - Commit messages: - 8255616: Disable AOT and Graal in Oracle OpenJDK Changes: https://git.openjdk.java.net/jdk/pull/960/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=960&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8255616 Stats: 36 lines in 4 files changed: 21 ins; 11 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/960.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/960/head:pull/960 PR: https://git.openjdk.java.net/jdk/pull/960
Re: RFR: 8255072: [TESTBUG] com/sun/jdi/EATests.java should not fail if expected VMOutOfMemoryException is not thrown [v5]
On Thu, 29 Oct 2020 14:53:54 GMT, Richard Reingruber wrote: >> The following test cases try to provoke VMOutOfMemoryException during object >> reallocation because of JVMTI PopFrame / ForceEarlyReturn: >> >> EAPopFrameNotInlinedReallocFailure >> EAPopInlinedMethodWithScalarReplacedObjectsReallocFailure >> EAForceEarlyReturnOfInlinedMethodWithScalarReplacedObjectsReallocFailure >> >> For ZGC (so far) this is not 100% reliable. >> >> Just ignoring the runs where the expected OOME was not raised was not >> accepted. >> >> Summary of the now accepted solution: >> >> - The 3 problematic test cases are skipped if ZGC is selected. >> >> - They are also skipped if no OOME during object reallocation can be >> expected because allocations are not eliminated. >> >> - In consumeAllMemory, as a last step, empty LinkedList nodes are created >> without long array to fill up small blocks of free memory. >> >> - EATests.java is removed from the problem list for ZGC. > > Richard Reingruber has updated the pull request incrementally with one > additional commit since the last revision: > > Replaced vm.jvmci with vm.graal.enabled in @requires clause. Good - Marked as reviewed by kvn (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/775
Re: RFR: 8255072: [TESTBUG] com/sun/jdi/EATests.java should not fail if expected VMOutOfMemoryException is not thrown [v4]
On Tue, 27 Oct 2020 10:16:29 GMT, Richard Reingruber wrote: >> The following test cases try to provoke VMOutOfMemoryException during object >> reallocation because of JVMTI PopFrame / ForceEarlyReturn: >> >> EAPopFrameNotInlinedReallocFailure >> EAPopInlinedMethodWithScalarReplacedObjectsReallocFailure >> EAForceEarlyReturnOfInlinedMethodWithScalarReplacedObjectsReallocFailure >> >> For ZGC (so far) this is not 100% reliable. >> >> Just ignoring the runs where the expected OOME was not raised was not >> accepted. >> >> Summary of the now accepted solution: >> >> - The 3 problematic test cases are skipped if ZGC is selected. >> >> - They are also skipped if no OOME during object reallocation can be >> expected because allocations are not eliminated. >> >> - In consumeAllMemory, as a last step, empty LinkedList nodes are created >> without long array to fill up small blocks of free memory. >> >> - EATests.java is removed from the problem list for ZGC. > > Richard Reingruber has updated the pull request incrementally with one > additional commit since the last revision: > > Whitespace/indentation clean-up. Please change @requires for testing with Graal to `vm.graal.enabled` instead of `vm.jvmci` - Changes requested by kvn (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/775
Re: RFR: 8255200: ProblemList com/sun/jdi/EATests.java for ZGC
On Wed, 21 Oct 2020 21:04:55 GMT, Daniel D. Daugherty wrote: > Reduce the noise in the JDK16 CI by ProblemListing com/sun/jdi/EATests.java > for ZGC. Good. - Marked as reviewed by kvn (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/787
Re: RFR: 8253525: Implement getInstanceSize/sizeOf intrinsics [v5]
On Wed, 21 Oct 2020 17:31:37 GMT, Aleksey Shipilev wrote: >> This is fork off the SizeOf JEP, JDK-8249196. There is already the entry >> point in JDK that can use the intrinsic like this: >> `Instrumentation.getInstanceSize`. Therefore, we can implement the C1/C2 >> intrinsic now, hook it up to `Instrumentation`, and let the tools use that >> fast path today. >> >> With this patch, JOL is able to be close to `deepSizeOf` implementation from >> SizeOf JEP. >> >> Example performance improvements for sizing up a custom linked list: >> >> Benchmark (size) Mode Cnt Score Error Units >> >> # Default >> LinkedChainBench.linkedChain 1 avgt5 705.835 ±8.051 ns/op >> LinkedChainBench.linkedChain 10 avgt53148.874 ± 37.856 ns/op >> LinkedChainBench.linkedChain 100 avgt5 28693.256 ± 142.254 ns/op >> LinkedChainBench.linkedChain1000 avgt5 290161.590 ± 4594.631 ns/op >> >> # Instrumentation attached, no intrinsics >> LinkedChainBench.linkedChain 1 avgt5159.659 ± 19.238 ns/op >> LinkedChainBench.linkedChain 10 avgt5717.659 ± 22.540 ns/op >> LinkedChainBench.linkedChain 100 avgt5 7739.394 ± 111.683 ns/op >> LinkedChainBench.linkedChain1000 avgt5 80724.238 ± 2887.794 ns/op >> >> # Instrumentation attached, new intrinsics >> LinkedChainBench.linkedChain 1 avgt5 95.254 ± 0.808 ns/op >> LinkedChainBench.linkedChain 10 avgt5261.564 ± 8.524 ns/op >> LinkedChainBench.linkedChain 100 avgt5 3367.192 ± 21.128 ns/op >> LinkedChainBench.linkedChain1000 avgt5 34148.851 ± 373.080 ns/op > > Aleksey Shipilev 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 five additional > commits since the last revision: > > - The new intrinsic-related test > - Revert the change to test > - Merge branch 'master' into JDK-8253525-sizeof-intrinsics > - Add new intrinsics to toBeInvestigated list in CheckGraalIntrinsics.java > - 8253525: Implement getInstanceSize/sizeOf intrinsics Good. - Marked as reviewed by kvn (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/650
Re: RFR: 8253525: Implement getInstanceSize/sizeOf intrinsics [v3]
On Tue, 20 Oct 2020 17:54:21 GMT, Vladimir Kozlov wrote: >> ...or I can put new test into a separate file. But the existing test is >> quite inferior compared to the new one, so it >> does not seem to make a lot of sense to keep it. > > It was mistake in 8253191 (I file bug). If you modify existing file (even if > you keep only test name the same) you have > to preserve original Copyright and add new Copyright line. You don't need > create new file. We have a lot of cases with > 2 or more Copyright lines - it is normal: > https://github.com/openjdk/jdk/blob/master/test/hotspot/jtreg/compiler/vectorization/TestVectorsNotSavedAtSafepoint.java I file 8255067 to restore Copyright line in TestUnsignedByteCompare.java test file. - PR: https://git.openjdk.java.net/jdk/pull/650
Re: RFR: 8253525: Implement getInstanceSize/sizeOf intrinsics [v3]
On Tue, 20 Oct 2020 17:43:07 GMT, Aleksey Shipilev wrote: >> Well, I am replacing the entire file. There is a recent precedent of the >> similar change >> [here](https://github.com/openjdk/jdk/commit/6d13c766#diff-0daf75421f8fdb55a5640742ef6f12730fe1b370cc864311c188ad6b51fe). >> Either that should be reversed, or this one accepted. > > ...or I can put new test into a separate file. But the existing test is quite > inferior compared to the new one, so it > does not seem to make a lot of sense to keep it. It was mistake in 8253191 (I file bug). If you modify existing file (even if you keep only test name the same) you have to preserve original Copyright and add new Copyright line. You don't need create new file. We have a lot of cases with 2 or more Copyright lines - it is normal: https://github.com/openjdk/jdk/blob/master/test/hotspot/jtreg/compiler/vectorization/TestVectorsNotSavedAtSafepoint.java - PR: https://git.openjdk.java.net/jdk/pull/650
Re: RFR: 8253525: Implement getInstanceSize/sizeOf intrinsics [v3]
On Mon, 19 Oct 2020 06:57:24 GMT, Aleksey Shipilev wrote: >> This is fork off the SizeOf JEP, JDK-8249196. There is already the entry >> point in JDK that can use the intrinsic like >> this: `Instrumentation.getInstanceSize`. Therefore, we can implement the >> C1/C2 intrinsic now, hook it up to >> `Instrumentation`, and let the tools use that fast path today. With this >> patch, JOL is able to be close to >> `deepSizeOf` implementation from SizeOf JEP. >> Example performance improvements for sizing up a custom linked list: >> >> Benchmark (size) Mode Cnt Score Error Units >> >> # Default >> LinkedChainBench.linkedChain 1 avgt5 705.835 ±8.051 ns/op >> LinkedChainBench.linkedChain 10 avgt53148.874 ± 37.856 ns/op >> LinkedChainBench.linkedChain 100 avgt5 28693.256 ± 142.254 ns/op >> LinkedChainBench.linkedChain1000 avgt5 290161.590 ± 4594.631 ns/op >> >> # Instrumentation attached, no intrinsics >> LinkedChainBench.linkedChain 1 avgt5159.659 ± 19.238 ns/op >> LinkedChainBench.linkedChain 10 avgt5717.659 ± 22.540 ns/op >> LinkedChainBench.linkedChain 100 avgt5 7739.394 ± 111.683 ns/op >> LinkedChainBench.linkedChain1000 avgt5 80724.238 ± 2887.794 ns/op >> >> # Instrumentation attached, new intrinsics >> LinkedChainBench.linkedChain 1 avgt5 95.254 ± 0.808 ns/op >> LinkedChainBench.linkedChain 10 avgt5261.564 ± 8.524 ns/op >> LinkedChainBench.linkedChain 100 avgt5 3367.192 ± 21.128 ns/op >> LinkedChainBench.linkedChain1000 avgt5 34148.851 ± 373.080 ns/op > > Aleksey Shipilev has refreshed the contents of this pull request, and > previous commits have been removed. The > incremental views will show differences compared to the previous content of > the PR. test/jdk/java/lang/instrument/GetObjectSizeTest.java line 2: > 1: /* > 2: * Copyright (c) 2020, Red Hat, Inc. All rights reserved. You can't replace Copyright lines of one company with other. - PR: https://git.openjdk.java.net/jdk/pull/650
Re: RFR: 8253525: Implement getInstanceSize/sizeOf intrinsics [v4]
On Tue, 20 Oct 2020 10:24:35 GMT, Aleksey Shipilev wrote: >> This is fork off the SizeOf JEP, JDK-8249196. There is already the entry >> point in JDK that can use the intrinsic like >> this: `Instrumentation.getInstanceSize`. Therefore, we can implement the >> C1/C2 intrinsic now, hook it up to >> `Instrumentation`, and let the tools use that fast path today. With this >> patch, JOL is able to be close to >> `deepSizeOf` implementation from SizeOf JEP. >> Example performance improvements for sizing up a custom linked list: >> >> Benchmark (size) Mode Cnt Score Error Units >> >> # Default >> LinkedChainBench.linkedChain 1 avgt5 705.835 ±8.051 ns/op >> LinkedChainBench.linkedChain 10 avgt53148.874 ± 37.856 ns/op >> LinkedChainBench.linkedChain 100 avgt5 28693.256 ± 142.254 ns/op >> LinkedChainBench.linkedChain1000 avgt5 290161.590 ± 4594.631 ns/op >> >> # Instrumentation attached, no intrinsics >> LinkedChainBench.linkedChain 1 avgt5159.659 ± 19.238 ns/op >> LinkedChainBench.linkedChain 10 avgt5717.659 ± 22.540 ns/op >> LinkedChainBench.linkedChain 100 avgt5 7739.394 ± 111.683 ns/op >> LinkedChainBench.linkedChain1000 avgt5 80724.238 ± 2887.794 ns/op >> >> # Instrumentation attached, new intrinsics >> LinkedChainBench.linkedChain 1 avgt5 95.254 ± 0.808 ns/op >> LinkedChainBench.linkedChain 10 avgt5261.564 ± 8.524 ns/op >> LinkedChainBench.linkedChain 100 avgt5 3367.192 ± 21.128 ns/op >> LinkedChainBench.linkedChain1000 avgt5 34148.851 ± 373.080 ns/op > > Aleksey Shipilev has updated the pull request incrementally with one > additional commit since the last revision: > > Add new intrinsics to toBeInvestigated list in CheckGraalIntrinsics.java Changes requested by kvn (Reviewer). src/jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.hotspot.test/src/org/graalvm/compiler/hotspot/test/CheckGraalIntrinsics.java line 531: > 529: > "sun/instrument/InstrumentationImpl.getObjectSize0(JLjava/lang/Object;)J"); > 530: } > 531: Agree with this. - PR: https://git.openjdk.java.net/jdk/pull/650
Re: RFR: 8253525: Implement getInstanceSize/sizeOf intrinsics [v3]
On Mon, 19 Oct 2020 06:57:24 GMT, Aleksey Shipilev wrote: >> This is fork off the SizeOf JEP, JDK-8249196. There is already the entry >> point in JDK that can use the intrinsic like >> this: `Instrumentation.getInstanceSize`. Therefore, we can implement the >> C1/C2 intrinsic now, hook it up to >> `Instrumentation`, and let the tools use that fast path today. With this >> patch, JOL is able to be close to >> `deepSizeOf` implementation from SizeOf JEP. >> Example performance improvements for sizing up a custom linked list: >> >> Benchmark (size) Mode Cnt Score Error Units >> >> # Default >> LinkedChainBench.linkedChain 1 avgt5 705.835 ±8.051 ns/op >> LinkedChainBench.linkedChain 10 avgt53148.874 ± 37.856 ns/op >> LinkedChainBench.linkedChain 100 avgt5 28693.256 ± 142.254 ns/op >> LinkedChainBench.linkedChain1000 avgt5 290161.590 ± 4594.631 ns/op >> >> # Instrumentation attached, no intrinsics >> LinkedChainBench.linkedChain 1 avgt5159.659 ± 19.238 ns/op >> LinkedChainBench.linkedChain 10 avgt5717.659 ± 22.540 ns/op >> LinkedChainBench.linkedChain 100 avgt5 7739.394 ± 111.683 ns/op >> LinkedChainBench.linkedChain1000 avgt5 80724.238 ± 2887.794 ns/op >> >> # Instrumentation attached, new intrinsics >> LinkedChainBench.linkedChain 1 avgt5 95.254 ± 0.808 ns/op >> LinkedChainBench.linkedChain 10 avgt5261.564 ± 8.524 ns/op >> LinkedChainBench.linkedChain 100 avgt5 3367.192 ± 21.128 ns/op >> LinkedChainBench.linkedChain1000 avgt5 34148.851 ± 373.080 ns/op > > Aleksey Shipilev has refreshed the contents of this pull request, and > previous commits have been removed. The > incremental views will show differences compared to the previous content of > the PR. Always run graalunit testing with new intrinsics. You need to adjust Graal test: src/jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.hotspot.test/src/org/graalvm/compiler/hotspot/test/CheckGraalIntrinsics.java - Changes requested by kvn (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/650
Re: RFR: 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents [v10]
On Sat, 10 Oct 2020 08:34:23 GMT, Richard Reingruber 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. > > Richard Reingruber has updated the pull request with a new target base due to > a merge or a rebase. The pull request now > contains 21 commits: > - 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. > - More smaller changes proposed by Serguei. > - jvmtiDeferredUpdates.hpp: remove forward declarations. > - jvmtiDeferredLocalVariable: move member variables to the beginning of the > class definition. > - jvmtiDeferredUpdates.hpp: add/remove empty lines and improve indentation. > - Merge branch 'master' into JDK-8227745 > - ... and 11 more: > https://git.openjdk.java.net/jdk/compare/aaa0a2a0...06b139a9 Good. - Marked as reviewed by kvn (Reviewer). 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 Thu, 8 Oct 2020 16:55:31 GMT, Richard Reingruber 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. > > Richard Reingruber has updated the pull request with a new target base due to > a merge or a rebase. The pull request now > contains 19 commits: > - 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. > - More smaller changes proposed by Serguei. > - jvmtiDeferredUpdates.hpp: remove forward declarations. > - jvmtiDeferredLocalVariable: move member variables to the beginning of the > class definition. > - jvmtiDeferredUpdates.hpp: add/remove empty lines and improve indentation. > - Merge branch 'master' into JDK-8227745 > - Merge branch 'master' into JDK-8227745 > - Make parameter current_thread of JvmtiEnvBase::check_top_frame() a > JavaThread* again. > >With Asynchronous handshakes the type was changed from JavaThread* to > Thread* >but this is not necessary as check_top_frame() is not executed during a > handshake >/ safepoint (robehn confirmed). > - ... and 9 more: > https://git.openjdk.java.net/jdk/compare/d036dca0...d463b4f3 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); - Changes requested by kvn (Reviewer). 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 Thu, 8 Oct 2020 16:55:31 GMT, Richard Reingruber 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. > > Richard Reingruber has updated the pull request with a new target base due to > a merge or a rebase. The pull request now > contains 19 commits: > - 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. > - More smaller changes proposed by Serguei. > - jvmtiDeferredUpdates.hpp: remove forward declarations. > - jvmtiDeferredLocalVariable: move member variables to the beginning of the > class definition. > - jvmtiDeferredUpdates.hpp: add/remove empty lines and improve indentation. > - Merge branch 'master' into JDK-8227745 > - Merge branch 'master' into JDK-8227745 > - Make parameter current_thread of JvmtiEnvBase::check_top_frame() a > JavaThread* again. > >With Asynchronous handshakes the type was changed from JavaThread* to > Thread* >but this is not necessary as check_top_frame() is not executed during a > handshake >/ safepoint (robehn confirmed). > - ... and 9 more: > https://git.openjdk.java.net/jdk/compare/d036dca0...d463b4f3 Compiler changes seems fine. - Marked as reviewed by kvn (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/119
Re: RFR(s): 8248295: serviceability/jvmti/CompiledMethodLoad/Zombie.java failure with Graal
Looks good. Thanks, Vladimir K On 8/19/20 5:30 AM, Fairoz Matte wrote: Hi Vladimir, Thanks for the review. I would suggest to run test with -XX:+PrintCodeCache flag which prints CodeCache usage on exit. Also add '-ea -esa' flags - some runs failed with them because they increase Graal's methods size. Running test with immediately caused OOM error on my local linux machine: '-server -ea -esa -XX:+TieredCompilation -XX:+PrintCodeCache - XX:+UnlockExperimentalVMOptions -XX:+EnableJVMCI - XX:+UseJVMCICompiler -Djvmci.Compiler=graal' With -XX:ReservedCodeCacheSize=30m I got: [11.217s][warning][codecache] CodeCache is full. Compiler has been disabled. [11.217s][warning][codecache] Try increasing the code cache size using - XX:ReservedCodeCacheSize= With -XX:ReservedCodeCacheSize=50m I got this output: Further testing with PrintCodeCache, ReservedCodeCacheSize = 50MB is the safe one to use. CodeCache: size=51200Kb used=34401Kb max_used=34401Kb free=16798Kb May be you need to set it to 35m or better to 50m to be safe. Note, without Graal test uses only 5.5m: CodeCache: size=20480Kb used=5677Kb max_used=5688Kb free=14803Kb - I also forgot to ask you to update test's Copyright year. I have updated the copyright year. Updated webrev for the reference - http://cr.openjdk.java.net/~fmatte/8248295/webrev.01/ Thanks, Fairoz Regards, Vladimir K On 8/18/20 1:10 AM, Fairoz Matte wrote: Hi Vladimir, Thanks for looking into. This is intermittent crash, and is reproducible in windows debug build environment. Below is the testing performed. 1. Issues observed 7/100 runs, ReservedCodeCacheSize=20m with "- XX:+UnlockExperimentalVMOptions -XX:+EnableJVMCI - XX:+UseJVMCICompiler" 2. Issues observed 0/300 runs, ReservedCodeCacheSize=30m with "- XX:+UnlockExperimentalVMOptions -XX:+EnableJVMCI - XX:+UseJVMCICompiler" Thanks, Fairoz -Original Message- From: Vladimir Kozlov Sent: Monday, August 17, 2020 11:22 PM To: Fairoz Matte ; hotspot-compiler- d...@openjdk.java.net; serviceability-dev@openjdk.java.net Cc: Coleen Phillimore ; Dean Long Subject: Re: RFR(s): 8248295: serviceability/jvmti/CompiledMethodLoad/Zombie.java failure with Graal Hi Fairoz, How you determine that +10Mb is enough with Graal? Thanks, Vladimir On 8/17/20 5:46 AM, Fairoz Matte wrote: Hi, Please review this small test change to work with Graal. Background: Graal require more code cache compared to c1/c2. but the test case always set it to 20MB. This may not be sufficient when running graal. Default configuration for ReservedCodeCacheSize = 250MB With graal enabled, ReservedCodeCacheSize = 350MB Either we can modify the framework to honor ReservedCodeCacheSize for graal or just update the testcase. There are not many test cases they rely on ReservedCodeCacheSize or InitialCodeCacheSize. So the fix prefer the later one. JBS - https://bugs.openjdk.java.net/browse/JDK-8248295 Webrev - http://cr.openjdk.java.net/~fmatte/8248295/webrev.00/ Thanks, Fairoz
Re: RFR(s): 8248295: serviceability/jvmti/CompiledMethodLoad/Zombie.java failure with Graal
I would suggest to run test with -XX:+PrintCodeCache flag which prints CodeCache usage on exit. Also add '-ea -esa' flags - some runs failed with them because they increase Graal's methods size. Running test with immediately caused OOM error on my local linux machine: '-server -ea -esa -XX:+TieredCompilation -XX:+PrintCodeCache -XX:+UnlockExperimentalVMOptions -XX:+EnableJVMCI -XX:+UseJVMCICompiler -Djvmci.Compiler=graal' With -XX:ReservedCodeCacheSize=30m I got: [11.217s][warning][codecache] CodeCache is full. Compiler has been disabled. [11.217s][warning][codecache] Try increasing the code cache size using -XX:ReservedCodeCacheSize= With -XX:ReservedCodeCacheSize=50m I got this output: CodeCache: size=51200Kb used=34401Kb max_used=34401Kb free=16798Kb May be you need to set it to 35m or better to 50m to be safe. Note, without Graal test uses only 5.5m: CodeCache: size=20480Kb used=5677Kb max_used=5688Kb free=14803Kb - I also forgot to ask you to update test's Copyright year. Regards, Vladimir K On 8/18/20 1:10 AM, Fairoz Matte wrote: Hi Vladimir, Thanks for looking into. This is intermittent crash, and is reproducible in windows debug build environment. Below is the testing performed. 1. Issues observed 7/100 runs, ReservedCodeCacheSize=20m with "-XX:+UnlockExperimentalVMOptions -XX:+EnableJVMCI -XX:+UseJVMCICompiler" 2. Issues observed 0/300 runs, ReservedCodeCacheSize=30m with "-XX:+UnlockExperimentalVMOptions -XX:+EnableJVMCI -XX:+UseJVMCICompiler" Thanks, Fairoz -Original Message- From: Vladimir Kozlov Sent: Monday, August 17, 2020 11:22 PM To: Fairoz Matte ; hotspot-compiler- d...@openjdk.java.net; serviceability-dev@openjdk.java.net Cc: Coleen Phillimore ; Dean Long Subject: Re: RFR(s): 8248295: serviceability/jvmti/CompiledMethodLoad/Zombie.java failure with Graal Hi Fairoz, How you determine that +10Mb is enough with Graal? Thanks, Vladimir On 8/17/20 5:46 AM, Fairoz Matte wrote: Hi, Please review this small test change to work with Graal. Background: Graal require more code cache compared to c1/c2. but the test case always set it to 20MB. This may not be sufficient when running graal. Default configuration for ReservedCodeCacheSize = 250MB With graal enabled, ReservedCodeCacheSize = 350MB Either we can modify the framework to honor ReservedCodeCacheSize for graal or just update the testcase. There are not many test cases they rely on ReservedCodeCacheSize or InitialCodeCacheSize. So the fix prefer the later one. JBS - https://bugs.openjdk.java.net/browse/JDK-8248295 Webrev - http://cr.openjdk.java.net/~fmatte/8248295/webrev.00/ Thanks, Fairoz
Re: RFR(s): 8248295: serviceability/jvmti/CompiledMethodLoad/Zombie.java failure with Graal
Hi Fairoz, How you determine that +10Mb is enough with Graal? Thanks, Vladimir On 8/17/20 5:46 AM, Fairoz Matte wrote: Hi, Please review this small test change to work with Graal. Background: Graal require more code cache compared to c1/c2. but the test case always set it to 20MB. This may not be sufficient when running graal. Default configuration for ReservedCodeCacheSize = 250MB With graal enabled, ReservedCodeCacheSize = 350MB Either we can modify the framework to honor ReservedCodeCacheSize for graal or just update the testcase. There are not many test cases they rely on ReservedCodeCacheSize or InitialCodeCacheSize. So the fix prefer the later one. JBS - https://bugs.openjdk.java.net/browse/JDK-8248295 Webrev - http://cr.openjdk.java.net/~fmatte/8248295/webrev.00/ Thanks, Fairoz
Re: RFR(s): 8236042: [TESTBUG] serviceability/sa/ClhsdbCDSCore.java fails with -Xcomp -XX:TieredStopAtLevel=1
Fix is good. I think next are reasons you don't get MDO in this scenario. Tier1 (C1 compilation) does not generate profiling code and does not created MDO. C1 request MDO only with tiers 2 and 3 [1][2]. With -Xcomp flag a Java method is not executed in Interpreter but requests its compilation and waits when it is finished. As result MDO is not created in Interpreter too. May be late if a method is deoptimized it will be executed in Interpreter and MDO will be created. Thanks, Vladimir [1] http://hg.openjdk.java.net/jdk/jdk/file/796c9fa50850/src/hotspot/share/c1/c1_Compilation.hpp#l226 [2] http://hg.openjdk.java.net/jdk/jdk/file/796c9fa50850/src/hotspot/share/c1/c1_Compilation.cpp#l381 On 7/7/20 8:47 PM, Fairoz Matte wrote: Thanks Chris, for the review comments. I have updated the suggested change. Thanks, Fairoz -Original Message- From: Chris Plummer Sent: Wednesday, July 8, 2020 3:38 AM To: Fairoz Matte ; hotspot-compiler- d...@openjdk.java.net; serviceability-dev@openjdk.java.net Subject: Re: RFR(s): 8236042: [TESTBUG] serviceability/sa/ClhsdbCDSCore.java fails with -Xcomp -XX:TieredStopAtLevel=1 Hi Fairoz, Looks good, except for the missing space in "if(testJavaOpts...". thanks, Chris On 7/7/20 7:49 AM, Fairoz Matte wrote: Hi, Please review this small test change to consider the scenario when there is no "printmdo" output JBS - https://bugs.openjdk.java.net/browse/JDK-8236042 Webrev - http://cr.openjdk.java.net/~fmatte/8236042/webrev.00/ Thanks, Fairoz
Re: [15] RFR(T) 8247527: serviceability/dcmd/gc/HeapDumpCompressedTest.java fails with Graal + ZGC
Thank you, David Vladimir K On 7/3/20 3:18 PM, David Holmes wrote: On 4/07/2020 4:30 am, Vladimir Kozlov wrote: Thank you, David, for looking on changes. I will remember to update tests. I filed RFE 8248815 [1] for tracking. Can you approve this fix now? Yes - thanks. David Thanks, Vladimir K [1] https://bugs.openjdk.java.net/browse/JDK-8248815 On 7/2/20 10:09 PM, David Holmes wrote: Hi Igor, On 3/07/2020 12:59 pm, Igor Ignatyev wrote: Hi David, it's in my todo list to improve this situation and have vm.gc.X to take selected JIT into account; and update existing (>200) occurrences of 'vm.gc.X & !vm.graal.enabled' 200+ ouch! :( I guess this fix doesn't make the situation any worse in a practical sense. Thanks, David - -- Igor On Jul 2, 2020, at 7:25 PM, David Holmes wrote: Hi Vladimir, On 3/07/2020 12:02 pm, Vladimir Kozlov wrote: https://cr.openjdk.java.net/~kvn/8247527/webrev.00/ https://bugs.openjdk.java.net/browse/JDK-8247527 Test should have @requires which excludes running Graal with GC which it does not support. I find it somewhat disturbing that a generic test has to know about the limitations between GCs and Graal! I would have been more inclined to just exclude this test when running with Graal, even if that theoretically reduced the test coverage in a ting way. If/When Graal supports these other GCs who will remember to re-enable these test cases? Thanks, David Testing: hs-tier1,hs-tier4-graal Thanks, Vladimir
Re: [15] RFR(T) 8247527: serviceability/dcmd/gc/HeapDumpCompressedTest.java fails with Graal + ZGC
Thank you, David, for looking on changes. I will remember to update tests. I filed RFE 8248815 [1] for tracking. Can you approve this fix now? Thanks, Vladimir K [1] https://bugs.openjdk.java.net/browse/JDK-8248815 On 7/2/20 10:09 PM, David Holmes wrote: Hi Igor, On 3/07/2020 12:59 pm, Igor Ignatyev wrote: Hi David, it's in my todo list to improve this situation and have vm.gc.X to take selected JIT into account; and update existing (>200) occurrences of 'vm.gc.X & !vm.graal.enabled' 200+ ouch! :( I guess this fix doesn't make the situation any worse in a practical sense. Thanks, David - -- Igor On Jul 2, 2020, at 7:25 PM, David Holmes wrote: Hi Vladimir, On 3/07/2020 12:02 pm, Vladimir Kozlov wrote: https://cr.openjdk.java.net/~kvn/8247527/webrev.00/ https://bugs.openjdk.java.net/browse/JDK-8247527 Test should have @requires which excludes running Graal with GC which it does not support. I find it somewhat disturbing that a generic test has to know about the limitations between GCs and Graal! I would have been more inclined to just exclude this test when running with Graal, even if that theoretically reduced the test coverage in a ting way. If/When Graal supports these other GCs who will remember to re-enable these test cases? Thanks, David Testing: hs-tier1,hs-tier4-graal Thanks, Vladimir
Re: [15] RFR(T) 8247527: serviceability/dcmd/gc/HeapDumpCompressedTest.java fails with Graal + ZGC
Thank you, Igor Vladimir K On 7/2/20 7:24 PM, igor.ignat...@oracle.com wrote: LGTM — Igor On Jul 2, 2020, at 7:03 PM, Vladimir Kozlov wrote: https://cr.openjdk.java.net/~kvn/8247527/webrev.00/ https://bugs.openjdk.java.net/browse/JDK-8247527 Test should have @requires which excludes running Graal with GC which it does not support. Testing: hs-tier1,hs-tier4-graal Thanks, Vladimir
[15] RFR(T) 8247527: serviceability/dcmd/gc/HeapDumpCompressedTest.java fails with Graal + ZGC
https://cr.openjdk.java.net/~kvn/8247527/webrev.00/ https://bugs.openjdk.java.net/browse/JDK-8247527 Test should have @requires which excludes running Graal with GC which it does not support. Testing: hs-tier1,hs-tier4-graal Thanks, Vladimir
Re: RFR(S) 8238585: Use handshake for JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make compiled methods on stack not_entrant
Vladimir Ivanov is on break currently. It looks good to me. Thanks, Vladimir K On 5/26/20 7:31 AM, Reingruber, Richard wrote: Hi Vladimir, Webrev: http://cr.openjdk.java.net/~rrich/webrevs/8238585/webrev.0/ Not an expert in JVMTI code base, so can't comment on the actual changes. From JIT-compilers perspective it looks good. I put out webrev.1 a while ago [1]: Webrev:http://cr.openjdk.java.net/~rrich/webrevs/8238585/webrev.1/ Webrev(delta): http://cr.openjdk.java.net/~rrich/webrevs/8238585/webrev.1.inc/ You originally suggested to use a handshake to switch a thread into interpreter mode [2]. I'm using a direct handshake now, because I think it is the best fit. May I ask if webrev.1 still looks good to you from JIT-compilers perspective? Can I list you as (partial) Reviewer? Thanks, Richard. [1] http://mail.openjdk.java.net/pipermail/serviceability-dev/2020-April/031245.html [2] http://mail.openjdk.java.net/pipermail/serviceability-dev/2020-January/030340.html -Original Message- From: Vladimir Ivanov Sent: Freitag, 7. Februar 2020 09:19 To: Reingruber, Richard ; serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net Subject: Re: RFR(S) 8238585: Use handshake for JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make compiled methods on stack not_entrant Webrev: http://cr.openjdk.java.net/~rrich/webrevs/8238585/webrev.0/ Not an expert in JVMTI code base, so can't comment on the actual changes. From JIT-compilers perspective it looks good. Best regards, Vladimir Ivanov Bug:https://bugs.openjdk.java.net/browse/JDK-8238585 The change avoids making all compiled methods on stack not_entrant when switching a java thread to interpreter only execution for jvmti purposes. It is sufficient to deoptimize the compiled frames on stack. Additionally a handshake is used instead of a vm operation to walk the stack and do the deoptimizations. Testing: JCK and JTREG tests, also in Xcomp mode with fastdebug and release builds on all platforms. Thanks, Richard. See also my question if anyone knows a reason for making the compiled methods not_entrant: http://mail.openjdk.java.net/pipermail/serviceability-dev/2020-January/030339.html
Re: RFR(XS): 8245521: Remove STACK_BIAS
On 5/21/20 9:11 PM, David Holmes wrote: Hi Mikael, Looks good. +1 I assume the change to GraalHotSpotVMConfig.java is to allow it to work with older VMs? Yes. stackBias will be set to 0 if STACK_BIAS is not present. Otherwise it will be set to STACK_BIAS value. Thanks, Vladimir Thanks, David On 22/05/2020 1:36 pm, Mikael Vidstedt wrote: Please review this small change which removes the STACK_BIAS constant and its uses: JBS: https://bugs.openjdk.java.net/browse/JDK-8245521 webrev: http://cr.openjdk.java.net/~mikael/webrevs/8245521/webrev.00/open/webrev/ Background (from JBS): With Solaris/SPARC removed the STACK_BIAS definition in src/hotspot/share/utilities/globalDefinitions.hpp is now always 0 and can be removed. Testing: Tier1 Cheers, Mikael
Re: RFR: 8244224: Implementation of JEP 381: Remove the Solaris and SPARC Ports (hotspot)
I filed Graal issue to change mx script to filter out SPARC code when we do sync Graal changes into JDK. For Graal shared code we may need to have versioning for latest JDK as we do in other cases. Regards, Vladimir On 5/4/20 2:29 PM, Igor Ignatyev wrote: Hi Mikael, the changes in /test/ look good to me. I have a question regarding src/jdk.internal.vm.compiler/*, aren't these files part of graal-compiler and hence will be brought back by the next graal update? Thanks, -- Igor On May 3, 2020, at 10:12 PM, Mikael Vidstedt wrote: Please review this change which implements part of JEP 381: JBS: https://bugs.openjdk.java.net/browse/JDK-8244224 webrev: http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.00/hotspot/open/webrev/ JEP: https://bugs.openjdk.java.net/browse/JDK-8241787 Note: When reviewing this, please be aware that this exercise was *extremely* mind-numbing, so I appreciate your help reviewing all the individual changes carefully. You may want to get that coffee cup filled up (or whatever keeps you awake)! Background: Because of the size of the total patch and wide range of areas touched, this patch is one out of in total six partial patches which together make up the necessary changes to remove the Solaris and SPARC ports. The other patches are being sent out for review to mailing lists appropriate for the respective areas the touch. An email will be sent to jdk-dev summarizing all the patches/reviews. To be clear: this patch is *not* in itself complete and stand-alone - all of the (six) patches are needed to form a complete patch. Some changes in this patch may look wrong or incomplete unless also looking at the corresponding changes in other areas. For convenience, I’m including a link below[1] to the full webrev, but in case you have comments on changes in other areas, outside of the files included in this thread, please provide those comments directly in the thread on the appropriate mailing list for that area if possible. In case it helps, the changes were effectively produced by searching for and updating any code mentioning “solaris", “sparc”, “solstudio”, “sunos”, etc. More information about the areas impacted can be found in the JEP itself. A big thank you to Igor Ignatyev for helping make the changes to the hotspot tests! Also, I have a short list of follow-ups which I’m going to look at separately from this JEP/patch, mainly related to command line options/flags which are no longer relevant and should be deprecated/obsoleted/removed. Testing: A slightly earlier version of this change successfully passed tier1-8, as well as client tier1-2. Additional testing will be done after the first round of reviews has been completed. Cheers, Mikael [1] http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.00/all/open/webrev/
Re: RFR: 8244224: Implementation of JEP 381: Remove the Solaris and SPARC Ports (hotspot)
JIT, AOT, JVMCI and Graal changes seem fine to me. It would be interesting to see shared code execution coverage change. There are places where we use flags and setting instead of #ifdef SPARC which may not be executed now or executed partially. We may simplify such code too. Thanks, Vladimir On 5/3/20 10:12 PM, Mikael Vidstedt wrote: Please review this change which implements part of JEP 381: JBS: https://bugs.openjdk.java.net/browse/JDK-8244224 webrev: http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.00/hotspot/open/webrev/ JEP: https://bugs.openjdk.java.net/browse/JDK-8241787 Note: When reviewing this, please be aware that this exercise was *extremely* mind-numbing, so I appreciate your help reviewing all the individual changes carefully. You may want to get that coffee cup filled up (or whatever keeps you awake)! Background: Because of the size of the total patch and wide range of areas touched, this patch is one out of in total six partial patches which together make up the necessary changes to remove the Solaris and SPARC ports. The other patches are being sent out for review to mailing lists appropriate for the respective areas the touch. An email will be sent to jdk-dev summarizing all the patches/reviews. To be clear: this patch is *not* in itself complete and stand-alone - all of the (six) patches are needed to form a complete patch. Some changes in this patch may look wrong or incomplete unless also looking at the corresponding changes in other areas. For convenience, I’m including a link below[1] to the full webrev, but in case you have comments on changes in other areas, outside of the files included in this thread, please provide those comments directly in the thread on the appropriate mailing list for that area if possible. In case it helps, the changes were effectively produced by searching for and updating any code mentioning “solaris", “sparc”, “solstudio”, “sunos”, etc. More information about the areas impacted can be found in the JEP itself. A big thank you to Igor Ignatyev for helping make the changes to the hotspot tests! Also, I have a short list of follow-ups which I’m going to look at separately from this JEP/patch, mainly related to command line options/flags which are no longer relevant and should be deprecated/obsoleted/removed. Testing: A slightly earlier version of this change successfully passed tier1-8, as well as client tier1-2. Additional testing will be done after the first round of reviews has been completed. Cheers, Mikael [1] http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.00/all/open/webrev/
Re: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents
Yes, David
You are correct these changes touch all part of VM and may affect Graal (which
also has EA) too.
Changes should be tested in all our modes: tiered, C1 only, Graal, Interpreter. And I realized that
I only ran tier3-graal testing so I submitted the rest of Graal's tiers now.
I had assumed that our current testing (I ran all from tier1 to tier8) should exercise all paths in
VM these changes touch. But I may be wrong and it is correct to ask author to add testing in all VM
modes to make sure new code in VM's runtime and JVMTI is tested.
I do like to keep what current test is doing with C2. May be add an other test for other modes or
modify current one to enable to run it in other modes.
Thanks,
Vladimir
On 12/12/19 3:32 PM, David Holmes wrote:
On 13/12/2019 9:02 am, Reingruber, Richard wrote:
Hello Vladimir,
thanks for having a look.
> Use vm.compMode == "Xmixed" instead of vm.compMode != "Xcomp" to skip
> test from running in Interpreter mode too.
Done.
> You don't need vm.opt.TieredCompilation != true in @requires because you
> specified -XX:-TieredCompilation in @run command.
Ok.
> The test is specifically written for C2 only (not for C1 or Graal) to
> verify its Escape Analysis optimization.
> I did not look in great details into test's code but its analysis may be
> affected if C1 compiler is also used.
>
> Richard may clarify this.
The test cases aim to get their testmethod 'dontinline_testMethod' compiled by
C2. If they get C1
compiled before doesn't matter all that much. I've got a slight preference to
disabled tiered
compilation for simplicity.
My concern - perhaps unfounded - is that this seems to be being tested only in a pure C2 environment
when the actual changes will have to operate correctly in a tiered environment (and JVMCI).
Thanks,
David
Thanks, Richard.
-Original Message-
From: Vladimir Kozlov
Sent: Donnerstag, 12. Dezember 2019 19:20
To: David Holmes ; hotspot-runtime-...@openjdk.java.net;
hotspot-compiler-...@openjdk.java.net; serviceability-dev@openjdk.java.net; Reingruber, Richard
Subject: Re: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of
JVMTI Agents
Hi David,
Tiered is disabled because we don't want to see compilations and outputs
from C1 compiler which does not have EA.
The test is specifically written for C2 only (not for C1 or Graal) to
verify its Escape Analysis optimization.
I did not look in great details into test's code but its analysis may be
affected if C1 compiler is also used.
Richard may clarify this.
thanks,
Vladimir
On 12/11/19 1:04 PM, David Holmes wrote:
On 12/12/2019 5:21 am, Vladimir Kozlov wrote:
I will do full review later. I want to comment about test command line.
You don't need vm.opt.TieredCompilation != true in @requires because
you specified -XX:-TieredCompilation in @run command.
And per my comment this should be being tested with tiered as well.
David
Use vm.compMode == "Xmixed" instead of vm.compMode != "Xcomp" to skip
test from running in Interpreter mode too.
Thanks,
Vladimir
On 12/11/19 7:07 AM, Reingruber, Richard wrote:
Hi David,
> Most of the details here are in areas I can comment on in
detail, but I
> did take an initial general look at things.
Thanks for taking the time!
> The only thing that jumped out at me is that I think the
> DeoptimizeObjectsALotThread should be a hidden thread.
>
> + bool is_hidden_from_external_view() const { return true; }
Yes, it should. Will add the method like above.
> Also I don't see any testing of the DeoptimizeObjectsALotThread.
Without
> active testing this will just bit-rot.
DeoptimizeObjectsALot is meant for stress testing with a larger
workload. I will add a minimal test
to keep it fresh.
> Also on the tests I don't understand your @requires clause:
>
> @requires ((vm.compMode != "Xcomp") & vm.compiler2.enabled &
> (vm.opt.TieredCompilation != true))
>
> This seems to require that TieredCompilation is disabled, but
tiered is
> our normal mode of operation. ??
>
I removed the clause. I guess I wanted to target the tests towards
the code they are supposed to
test, and it's easier to analyze failures w/o tiered compilation and
with just one compiler thread.
Additionally I will make use of
compiler.whitebox.CompilerWhiteBoxTest.THRESHOLD in the tests.
Thanks,
Richard.
-Original Message-
From: David Holmes
Sent: Mittwoch, 11. Dezember 2019 08:03
To: Reingruber, Richard ;
serviceability-dev@openjdk.java.net;
hotspot-compiler-...@openjdk.java.net;
hotspot-runtime-...@openjdk.java.net
Subject: Re: RFR(L) 8227745: Enable Escape Analysis for Better
Performance in the Presence of JVMTI A
Re: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents
Hi David,
Tiered is disabled because we don't want to see compilations and outputs
from C1 compiler which does not have EA.
The test is specifically written for C2 only (not for C1 or Graal) to
verify its Escape Analysis optimization.
I did not look in great details into test's code but its analysis may be
affected if C1 compiler is also used.
Richard may clarify this.
thanks,
Vladimir
On 12/11/19 1:04 PM, David Holmes wrote:
On 12/12/2019 5:21 am, Vladimir Kozlov wrote:
I will do full review later. I want to comment about test command line.
You don't need vm.opt.TieredCompilation != true in @requires because
you specified -XX:-TieredCompilation in @run command.
And per my comment this should be being tested with tiered as well.
David
Use vm.compMode == "Xmixed" instead of vm.compMode != "Xcomp" to skip
test from running in Interpreter mode too.
Thanks,
Vladimir
On 12/11/19 7:07 AM, Reingruber, Richard wrote:
Hi David,
> Most of the details here are in areas I can comment on in
detail, but I
> did take an initial general look at things.
Thanks for taking the time!
> The only thing that jumped out at me is that I think the
> DeoptimizeObjectsALotThread should be a hidden thread.
>
> + bool is_hidden_from_external_view() const { return true; }
Yes, it should. Will add the method like above.
> Also I don't see any testing of the DeoptimizeObjectsALotThread.
Without
> active testing this will just bit-rot.
DeoptimizeObjectsALot is meant for stress testing with a larger
workload. I will add a minimal test
to keep it fresh.
> Also on the tests I don't understand your @requires clause:
>
> @requires ((vm.compMode != "Xcomp") & vm.compiler2.enabled &
> (vm.opt.TieredCompilation != true))
>
> This seems to require that TieredCompilation is disabled, but
tiered is
> our normal mode of operation. ??
>
I removed the clause. I guess I wanted to target the tests towards
the code they are supposed to
test, and it's easier to analyze failures w/o tiered compilation and
with just one compiler thread.
Additionally I will make use of
compiler.whitebox.CompilerWhiteBoxTest.THRESHOLD in the tests.
Thanks,
Richard.
-Original Message-
From: David Holmes
Sent: Mittwoch, 11. Dezember 2019 08:03
To: Reingruber, Richard ;
serviceability-dev@openjdk.java.net;
hotspot-compiler-...@openjdk.java.net;
hotspot-runtime-...@openjdk.java.net
Subject: Re: RFR(L) 8227745: Enable Escape Analysis for Better
Performance in the Presence of JVMTI Agents
Hi Richard,
On 11/12/2019 7:45 am, Reingruber, Richard wrote:
Hi,
I would like to get reviews please for
http://cr.openjdk.java.net/~rrich/webrevs/2019/8227745/webrev.3/
Corresponding RFE:
https://bugs.openjdk.java.net/browse/JDK-8227745
Fixes also https://bugs.openjdk.java.net/browse/JDK-8233915
And potentially https://bugs.openjdk.java.net/browse/JDK-8214584 [1]
Vladimir Kozlov kindly put webrev.3 through tier1-8 testing without
issues (thanks!). In addition the
change is being tested at SAP since I posted the first RFR some
months ago.
The intention of this enhancement is to benefit performance wise
from escape analysis even if JVMTI
agents request capabilities that allow them to access local variable
values. E.g. if you start-up
with -agentlib:jdwp=transport=dt_socket,server=y,suspend=n, then
escape analysis is disabled right
from the beginning, well before a debugger attaches -- if ever one
should do so. With the
enhancement, escape analysis will remain enabled until and after a
debugger attaches. EA based
optimizations are reverted just before an agent acquires the
reference to an object. In the JBS item
you'll find more details.
Most of the details here are in areas I can comment on in detail, but I
did take an initial general look at things.
The only thing that jumped out at me is that I think the
DeoptimizeObjectsALotThread should be a hidden thread.
+ bool is_hidden_from_external_view() const { return true; }
Also I don't see any testing of the DeoptimizeObjectsALotThread. Without
active testing this will just bit-rot.
Also on the tests I don't understand your @requires clause:
@requires ((vm.compMode != "Xcomp") & vm.compiler2.enabled &
(vm.opt.TieredCompilation != true))
This seems to require that TieredCompilation is disabled, but tiered is
our normal mode of operation. ??
Thanks,
David
Thanks,
Richard.
[1] Experimental fix for JDK-8214584 based on JDK-8227745
http://cr.openjdk.java.net/~rrich/webrevs/2019/8214584/experiment_v1.patch
Re: RFR(T) : 8235353 : clean up hotspot problem lists
I am fine with changes but we need to ask PPC64 supporter to verify that tests passed now. Thanks, Vladimir K On 12/4/19 11:52 AM, Igor Ignatyev wrote: http://cr.openjdk.java.net/~iignatyev//8235353/webrev.00 9 lines changed: 0 ins; 0 del; 9 mod; Hi all, could you please review this small and trivial cleanup which returns serviceablility/sa tests back to execution on linux-ppc64. the tests were problem listed due to 8211767[1], which is closed as a dup of resolved 8228649[2]. [1] https://bugs.openjdk.java.net/browse/JDK-8211767 [2] https://bugs.openjdk.java.net/browse/JDK-8228649 Thanks, -- Igor
Re: Should optimizations be observable for JVMTI agents?
I had very good discussion yesterday with Serguei Spitsyn from serviceability. He said that we may not need one solution. Both cases, monitoring and debugging, are important and we can use different solutions for them. As Andrew said, for monitoring you want to observe the same behavior as in production. I think we should not restrict JIT and others optimizations in this case. I also agree with Richard that JVMTI may report scalar replaced objects and elided monitors in this mode if requested - it is also interesting information. For debugging we may want to see locals and monitors present in compiled code and it is reasonable to avoid optimizations which remove them. But even for debugging we may want to use optimized code as we do when debugging optimized C code (we don't expect to see all locals in such case). May be we should check not which JVMTI capabilities are switched on but for what JVMTI is used for. Or add API for user to ask what he wants. As Richard mentioned in some email compiled frame can't be deoptimized (to get objects and monitors back) if it is not top frame on stack. So we can't rely on deoptimization here. JIT and Runtime need to know what to do with optimizations when agent is attached to JVM. I don't like suggestion for 8230956 to switch off EA when can_tag_objects is on. Based on declaration, can_tag_objects is always on. Which means we will have EA off in all cases including monitoring tools (JFR, managment, profiling) which is not acceptable for me. I share David's concern about missing cases for some JVMTI capabilities which are not checked when JIT decides to use optimizations. And I think it will be troublesome to go through all capabilities old and new to see if we need to add checks. For me as JIT compiler engineer it would be much simpler to ask JVMTI should optimizations be switched off or not and let JVMTI find in which case it is used and make decision instead of current case when JIT checks some JVMTI capabilities. I agree to provide information in compiled code for JVMTI - we do provide information about scalar replaced object already. And I think it would be much easier to document such relation between JIT and JVMTI. Thanks, Vladimir On 9/25/19 7:32 AM, Andrew Dinn wrote: On 25/09/2019 13:31, Reingruber, Richard wrote: > The terminology clarification is simply that - a clarification so that > when the spec says "heap" it means "Java heap", when it says "Thread" it > means "Java thread" etc without having to spell it out each time. I do > not read this as meaning anything about an "abstract virtual machine" > state, it is about the Java Virtual Machine state - which to mean can be > the concrete state as implemented by Hotspot, not some abstract > conceptual model state. It is more than that. It is the glue to the JVM and Java Language specifications. This is important to accept. Otherwise I would suggest to do your java debugging with gdb and Intel (or AMD??) x86 manuals at your hand. > We'll just have to agree to disagree here. Agreed. All I do is offer my points to our audience hoping for a few '+1' ;) Sorry, but I'm going to throw in a -1 here, not on any grounds of textual exegesis of the JVM spec rather those of simple pragmatism -- grounds that David already alluded to in his mention of finalization. The path you want to go down is to force disabling of optimizations as needed in order to ensure that JVMTI agents see a 'pure' version of the /abstract/ machine state as defined in the Java Virtual Machine spec. This is in many ways laudable: 1) such a 'pure' view is one that can be mapped fairly directly back to the original Java application code state (assuming you factor in enough knowledge of how javac translates code to bytecode and relying on the fact that javac does not do anything very tricksy by way of optimization during that translation) i.e. it is clear. 2) this pure view is going to be unchanged by whatever tricks the JIT and/or JVM might have up their sleeves i.e. it is consistent/repeatable. That latter point of consistency is, perhaps, more significant than the former one of clarity since for most programmers bytecode is only ever observed 'through a glass darkly'. The downside comes when you consider why you might want to use an agent. Primarily, JVMTI was provided for monitoring actual program execution. Agents can also be used to modify program execution, making program transformations that result in locally and/or globally variant behaviour. Well, actually, I guess some monitoring operations falls into that latter camp. In order to pursue your proposed path it is going to be necessary to disable, either piecemeal or wholesale, a variety of JIT optimizations. This would mean that use of an agent might involve a cost that is prohibitive. Primarily, that's a cost in performance (whether measured in space or time). When it comes to monitoring it is also the extra cost in
Re: RFR(S) 8230956: Should disable Escape Analysis when JVMTI capability can_tag_objects is taken
It is also not clear to me that it is bug. Based on all description this functionality is used to catch leaks in Java heap. But scalar replaced objects do not exists. JVMTI should not even see them. Thanks, Vladimir On 9/24/19 3:37 PM, Vladimir Kozlov wrote: can_tag_objects is "always" capability. If it is true then EA will be disabled in all cases when JVMTI agent is used. It is too broad. Am I missing something? Thanks, Vladimir On 9/13/19 7:12 AM, Reingruber, Richard wrote: Hi, could I please get reviews for Webrev: http://cr.openjdk.java.net/~rrich/webrevs/2019/8230956/webrev.0/ Bug: https://bugs.openjdk.java.net/browse/JDK-8230956 JVMTI provides functions to follow references beginning at the roots of the object graph and it provides functions to iterate all objects on the heap[1][2]. These functions are means to access objects which are otherwise local to a Java thread. In terms of escape analysis these local objects escape through these JVMTI functions invalidating optimizations based on escape analysis. Example: - Let J be a JavaThread that calls a compiled method M with a NoEscape instance I of class C that is scalar replaced. - JVMTI agent A uses JVMTI FollowReferences() to iterate the objects in the object graph tagging all instances of C. - A uses GetObjectsWithTags() to retrieve the tagged instances of C. - Error: I is missing because its allocation was eliminated / scalar replaced. Agents are required to possess the capability can_tag_objects in order to call the JVMTI heap functions that let objects escape. Currently it is not possible to revert EA based optimizations just before objects escape through JVMTI therefore escape analysis should be disabled as soon as the JVMTI capability can_tag_objects is taken. But this is not sufficient, because there may be compiled frames on stack with EA based optimizations when a JVMTI agent takes can_tag_objects (see included exclusive test cases), and then it does not help to disable escape analysis or invalidate compiled methods with ea based optimizations. In general it is still an improvement to do so. JDK-8227745 would be a complete solution to the issue. An further improvement could be to invalidate methods compiled by c2 when can_tag_objects gets added, but I'd rather suggest to integrated the implementation for JDK-8227745. Note also that after calling JVMTI AddCapabilities(), even with an empty set of capabilities, JvmtiExport::can_walk_any_space() will return true. I've run tier1 tests. Thanks, Richard. [1] https://docs.oracle.com/en/java/javase/11/docs/specs/jvmti.html#Heap [2] https://docs.oracle.com/en/java/javase/11/docs/specs/jvmti.html#Heap_1_0
Re: RFR(S) 8230956: Should disable Escape Analysis when JVMTI capability can_tag_objects is taken
can_tag_objects is "always" capability. If it is true then EA will be disabled in all cases when JVMTI agent is used. It is too broad. Am I missing something? Thanks, Vladimir On 9/13/19 7:12 AM, Reingruber, Richard wrote: Hi, could I please get reviews for Webrev: http://cr.openjdk.java.net/~rrich/webrevs/2019/8230956/webrev.0/ Bug:https://bugs.openjdk.java.net/browse/JDK-8230956 JVMTI provides functions to follow references beginning at the roots of the object graph and it provides functions to iterate all objects on the heap[1][2]. These functions are means to access objects which are otherwise local to a Java thread. In terms of escape analysis these local objects escape through these JVMTI functions invalidating optimizations based on escape analysis. Example: - Let J be a JavaThread that calls a compiled method M with a NoEscape instance I of class C that is scalar replaced. - JVMTI agent A uses JVMTI FollowReferences() to iterate the objects in the object graph tagging all instances of C. - A uses GetObjectsWithTags() to retrieve the tagged instances of C. - Error: I is missing because its allocation was eliminated / scalar replaced. Agents are required to possess the capability can_tag_objects in order to call the JVMTI heap functions that let objects escape. Currently it is not possible to revert EA based optimizations just before objects escape through JVMTI therefore escape analysis should be disabled as soon as the JVMTI capability can_tag_objects is taken. But this is not sufficient, because there may be compiled frames on stack with EA based optimizations when a JVMTI agent takes can_tag_objects (see included exclusive test cases), and then it does not help to disable escape analysis or invalidate compiled methods with ea based optimizations. In general it is still an improvement to do so. JDK-8227745 would be a complete solution to the issue. An further improvement could be to invalidate methods compiled by c2 when can_tag_objects gets added, but I'd rather suggest to integrated the implementation for JDK-8227745. Note also that after calling JVMTI AddCapabilities(), even with an empty set of capabilities, JvmtiExport::can_walk_any_space() will return true. I've run tier1 tests. Thanks, Richard. [1] https://docs.oracle.com/en/java/javase/11/docs/specs/jvmti.html#Heap [2] https://docs.oracle.com/en/java/javase/11/docs/specs/jvmti.html#Heap_1_0
