Integrated: 8287726: Fix JVMTI tests with "requires vm.continuations" after JDK-8287496
On Thu, 2 Jun 2022 09:54:31 GMT, Aleksey Shipilev wrote: > [JDK-8287496](https://bugs.openjdk.java.net/browse/JDK-8287496) brought the > alternative Loom implementation that can be used by ports as the fallback. > That fallback does not support JVMTI entirely, so lots of tests fail. Some > JVMTI is still supported, so cutting off at `@requires vm.jvmti` seems too > broad. They should be predicated with `@requires vm.continuations` to be > skipped when fallback is used. > > This also allows reverting relevant x86_32 problemlist exclusions. > > Additional testing: > - [x] Linux x86_32 fastdebug, `serviceability/jvmti` works, many tests > skipped > - [x] Linux x86_64 fastdebug, `serviceability/jvmti` still works, with all > current tests executing This pull request has now been integrated. Changeset: 3cfd38ca Author:Aleksey Shipilev URL: https://git.openjdk.java.net/jdk/commit/3cfd38caf10c18f71c0fc8c9a09c0d1179373ce7 Stats: 138 lines in 69 files changed: 69 ins; 69 del; 0 mod 8287726: Fix JVMTI tests with "requires vm.continuations" after JDK-8287496 Reviewed-by: alanb, rehn, lmesnik - PR: https://git.openjdk.java.net/jdk/pull/8990
Re: RFR: 8287726: Fix JVMTI tests with "requires vm.continuations" after JDK-8287496
On Thu, 2 Jun 2022 09:54:31 GMT, Aleksey Shipilev wrote: > [JDK-8287496](https://bugs.openjdk.java.net/browse/JDK-8287496) brought the > alternative Loom implementation that can be used by ports as the fallback. > That fallback does not support JVMTI entirely, so lots of tests fail. Some > JVMTI is still supported, so cutting off at `@requires vm.jvmti` seems too > broad. They should be predicated with `@requires vm.continuations` to be > skipped when fallback is used. > > This also allows reverting relevant x86_32 problemlist exclusions. > > Additional testing: > - [x] Linux x86_32 fastdebug, `serviceability/jvmti` works, many tests > skipped > - [x] Linux x86_64 fastdebug, `serviceability/jvmti` still works, with all > current tests executing Thank you! - PR: https://git.openjdk.java.net/jdk/pull/8990
Re: RFR: 8287726: Fix JVMTI tests with "requires vm.continuations" after JDK-8287496
On Thu, 2 Jun 2022 09:54:31 GMT, Aleksey Shipilev wrote: > [JDK-8287496](https://bugs.openjdk.java.net/browse/JDK-8287496) brought the > alternative Loom implementation that can be used by ports as the fallback. > That fallback does not support JVMTI entirely, so lots of tests fail. Some > JVMTI is still supported, so cutting off at `@requires vm.jvmti` seems too > broad. They should be predicated with `@requires vm.continuations` to be > skipped when fallback is used. > > This also allows reverting relevant x86_32 problemlist exclusions. > > Additional testing: > - [x] Linux x86_32 fastdebug, `serviceability/jvmti` works, many tests > skipped > - [x] Linux x86_64 fastdebug, `serviceability/jvmti` still works, with all > current tests executing Trivial, or? I would like to have this integrated sooner to clean up our testing. - PR: https://git.openjdk.java.net/jdk/pull/8990
Re: RFR: 8287726: Fix JVMTI tests with "requires vm.continuations" after JDK-8287496
On Thu, 2 Jun 2022 11:05:30 GMT, Alan Bateman wrote: > I expect you can unexclude the runtime/* tests from this section too. Also > the same section in test/jdk/ProblemList.txt that excludes the tests on > x86_32 can be cleaned up too, maybe a separate PR. Yes, in separate PR. In this PR, I want to deal with JVMTI tests specifically. We can take JVMTI tests off the problemlist exactly because of `@requires vm.continuations` added in this PR. - PR: https://git.openjdk.java.net/jdk/pull/8990
RFR: 8287726: Fix JVMTI tests with "requires vm.continuations" after JDK-8287496
[JDK-8287496](https://bugs.openjdk.java.net/browse/JDK-8287496) brought the alternative Loom implementation that can be used by ports as the fallback. That fallback does not support JVMTI entirely, so lots of tests fail. Some JVMTI is still supported, so cutting off at `@requires vm.jvmti` seems too broad. They should be predicated with `@requires vm.continuations` to be skipped when fallback is used. This also allows reverting x86_32 problemlist exclusions, which serves a proof of concept that [JDK-8287496](https://bugs.openjdk.java.net/browse/JDK-8287496) indeed works. Additional testing: - [x] Linux x86_32 fastdebug, `serviceability/jvmti` works, many tests skipped - [ ] Linux x86_64 fastdebug, `serviceability/jvmti` still works, with all current tests executing - Commit messages: - Only trim down JVMTI tests from x86_32 problemlists - Fix Changes: https://git.openjdk.java.net/jdk/pull/8990/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8990=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8287726 Stats: 138 lines in 69 files changed: 69 ins; 69 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/8990.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8990/head:pull/8990 PR: https://git.openjdk.java.net/jdk/pull/8990
Re: RFR: 8287496: Alternative virtual thread implementation that maps to OS thread [v3]
On Wed, 1 Jun 2022 06:04:14 GMT, Alan Bateman wrote: >> This patch adds an alternative virtual thread implementation where each >> virtual thread is backed by an OS thread. It doesn't scale but it can be >> used by ports that don't have continuations support in the VM. Aside from >> scalability, the lack of continuations support means: >> >> 1. JVM TI is not supported when running with --enable-preview (the JVM TI >> spec allows for this) >> 2. jshell --enable-preview can't be used (as jshell uses the debugger APIs >> and so needs JVM TI) >> >> The VM option "VMContinuations" is added as an experimental option so it can >> be used by tests. A number of tests are changed to re-run with >> -XX:-VMContinuations. A new jtreg property is added so that tests that need >> the underlying VM support to be present can use "@requires vm.continuations" >> in the test description. A follow-up change would be to add "@requires >> vm.continuations" to the ~70 serviceability/jvmti/vthread that run with >> preview features enabled. > > Alan Bateman has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 11 commits: > > - Fixed another typo in comment > - Merge > - Fix typos in comments > - Allowing linking without intrinsic stub, contributed-by: rehn > - Continuation clinit should fail if no continuations support > - Fix merge issue with test > - Change VMContinuations to be experimental option, ensure JNI GetEnv > returns EVERSION > - Update > - Expend test coverage > - Merge > - ... and 1 more: > https://git.openjdk.java.net/jdk/compare/3deb58a8...a5c06cb6 > A new jtreg property is added so that tests that need the underlying VM > support to be present can use "@requires vm.continuations" in the test > description. A follow-up change would be to add "@requires vm.continuations" > to the ~70 serviceability/jvmti/vthread that run with preview features > enabled. See #8990. - PR: https://git.openjdk.java.net/jdk/pull/8939
Re: RFR: 8287496: Alternative virtual thread implementation that maps to OS thread [v2]
On Tue, 31 May 2022 16:54:41 GMT, Boris Ulasevich wrote: > I expected this change to fix the broken ARM32 port, but it doesn't work. It would not fix ARM32, because the interpreter stubs need to be predicated on `Continuations::enabled()`. Also, as my ARM32 experiments show (https://github.com/openjdk/jdk/pull/8634/files#diff-027490ce3f4a92be9b489d9d2e54c7baaea87b7489399b198543c79f1ce1e2e3R4208-R4216) -- there is a breakage somewhere in C2 as well, which this patch would not seem to resolve as well. So, this is a stepping stone for *some* support, but it does not resolve porting situation fully. - PR: https://git.openjdk.java.net/jdk/pull/8939
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v12]
On Thu, 5 May 2022 18:54:49 GMT, Alan Bateman wrote: > We mailed porters-dev in Sep 2021 to give a heads up that this feature would > require porting work so it shouldn't be a surprise. We have been open to > including other ports with the initial integration but it was never a goal to > have all the ports done in advance of that. It is understandable to ship the preview feature not implemented on all platforms. It is even understandable to ship the final product feature that breaks some platforms in an clearly understandable way, prompting platform maintainers to implement the agreed-upon, final Java feature. What I am seeing, though, is that just running `java -version` (!) after integrating a *preview feature* is broken! > Most of the new code in the VM is only used when running with > --enable-preview. You'll see several places that test > Continuations::enabled(). It should be possible to get these port running > without -enable-preview without much effort. The feature has to be > implemented to pass all the tests of course. It is not as clear-cut, unfortunately. I see there are substantial changes in deopt machinery with post-call NOPs -- and there maybe more stuff lurking after that one is implemented -- so it is not as simple as changing `Unimplemented()` to guarding with `Continuations::enabled()`. So this looks to me that the core deopt machinery is affected, whether Loom is enabled or not. Is the impact of that deopt machinery change on the VM stability and VM ports discussed, understood, documented somewhere? I would have honestly expected those core changes to be heavily conditionalized with either `ifdef`-s, or runtime checks, or both, so that both unimplemented platforms had the old behavior *and* the implemented platforms had a fallback to old behavior if preview feature was broken. The current code is fine for experimental Loom repo, but I firmly believe mainline should have much stronger safety/reliability requirements. My fear is that an integration like this would wreck JDK 19 release. So my question stands: Are we breaking those platforms? Are we sure the unconditional VM changes are problem-free, implementable everywhere, etc? The answer might be "Yes, we are integrating, let the chips fall where they may", but I also believe that should be the call made by responsible platform/VM architects, who should explicitly weigh in and accept the risk of wide JDK 19 breakage. - PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v12]
On Thu, 5 May 2022 09:35:42 GMT, Alan Bateman wrote: >> This is the implementation of JEP 425: Virtual Threads (Preview). >> >> We will refresh this PR periodically to pick up changes and fixes from the >> loom repo. >> >> Most of the new mechanisms in the HotSpot VM are disabled by default and >> require running with `--enable-preview` to enable. >> >> The patch has support for x64 and aarch64 on the usual operating systems >> (Linux, macOS, and Windows). There are stubs (calling _Unimplemented_) for >> zero and some of the other ports. Additional ports can be contributed via >> PRs against the fibers branch in the loom repo. >> >> There are changes in many areas. To reduce notifications/mails, the labels >> have been trimmed down for now to hotspot, serviceability and core-libs. We >> can add additional labels, if required, as the PR progresses. >> >> The changes include a refresh of java.util.concurrent and ForkJoinPool from >> Doug Lea's CVS. These changes will probably be proposed and integrated in >> advance of this PR. >> >> The changes include some non-exposed and low-level infrastructure to support >> the (in draft) JEPs for Structured Concurrency and Extent Locals. This is to >> make life a bit easier and avoid having to separate VM changes and juggle >> branches at this time. > > Alan Bateman has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 19 commits: > > - Refresh 12aa09ce7efd48425cc080d0b8761aca0f3e215f > - Merge with 1bb4de2e2868a539846ec48dd43fd623c2ba69a5 > - Refresh d77f7a49af75bcc5b20686805ff82a93a20dde05 > - Merge with 4b2c82200fdc01de868cf414e40a4d891e753f89 > - Refresh 6091080db743ece5f1b2111fcc35a5f2179a403a > - Merge with cfcba1fccc8e3e6a68e1cb1826b70e076d5d83c4 > - Refresh ee9fa8ed05ec22de7a13383052d68aa8aa7832ec > - Merge with jdk-19+20 > - Refresh 7965cc6b168e567ac2596f2fbc3b00a7d99b7e1e > - Refresh 8d8f0a2fd646e57fe6b4e8ab669f836dc46dda69 > - ... and 9 more: > https://git.openjdk.java.net/jdk/compare/1bb4de2e...86bea8ec I am sorry to be a buzzkill here, but this integration would break lots of platforms even when Loom functionality is not enabled/used. For example, running `java -version` on RISC-V runs into many issues: `TemplateInterpreterGenerator::generate_Continuation_doYield_entry` runs into `Unimplemented()`, `UnsafeCopyMemory` asserts about UCM table size, `NativeDeoptInstruction::is_deopt` would run into `Unimplemented()` while being called from signal handler. This effectively blocks development on those platforms, and it seems to be too much breakage for a preview feature. Are we really breaking these platforms? Do we have plans in place with maintainers of those platforms to fix all this in JDK 19 timeframe? I'd suggest holding off the integration until such a plan and agreement is in place. - Changes requested by shade (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v11]
On Wed, 4 May 2022 12:12:48 GMT, Alan Bateman wrote: >> This is the implementation of JEP 425: Virtual Threads (Preview). >> >> We will refresh this PR periodically to pick up changes and fixes from the >> loom repo. >> >> Most of the new mechanisms in the HotSpot VM are disabled by default and >> require running with `--enable-preview` to enable. >> >> The patch has support for x64 and aarch64 on the usual operating systems >> (Linux, macOS, and Windows). There are stubs (calling _Unimplemented_) for >> zero and some of the other ports. Additional ports can be contributed via >> PRs against the fibers branch in the loom repo. >> >> There are changes in many areas. To reduce notifications/mails, the labels >> have been trimmed down for now to hotspot, serviceability and core-libs. We >> can add additional labels, if required, as the PR progresses. >> >> The changes include a refresh of java.util.concurrent and ForkJoinPool from >> Doug Lea's CVS. These changes will probably be proposed and integrated in >> advance of this PR. >> >> The changes include some non-exposed and low-level infrastructure to support >> the (in draft) JEPs for Structured Concurrency and Extent Locals. This is to >> make life a bit easier and avoid having to separate VM changes and juggle >> branches at this time. > > Alan Bateman has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 17 commits: > > - Refresh d77f7a49af75bcc5b20686805ff82a93a20dde05 > - Merge with 4b2c82200fdc01de868cf414e40a4d891e753f89 > - Refresh 6091080db743ece5f1b2111fcc35a5f2179a403a > - Merge with cfcba1fccc8e3e6a68e1cb1826b70e076d5d83c4 > - Refresh ee9fa8ed05ec22de7a13383052d68aa8aa7832ec > - Merge with jdk-19+20 > - Refresh 7965cc6b168e567ac2596f2fbc3b00a7d99b7e1e > - Refresh 8d8f0a2fd646e57fe6b4e8ab669f836dc46dda69 > - Refresh cf561073f48fad58e931a5285b92629aa83c9bd1 > - Merge with jdk-19+19 > - ... and 7 more: > https://git.openjdk.java.net/jdk/compare/4b2c8220...f06aff75 > The patch has support for x64 and aarch64 on the usual operating systems > (Linux, macOS, and Windows). There are stubs (calling _Unimplemented_) for > zero and some of the other ports. Additional ports can be contributed via PRs > against the fibers branch in the loom repo. So, does this PR pass current GHA checks? I see they are not enabled for this PR. It would be unfortunate for this large integration to break builds/tests for smaller PRs that would follow it. - PR: https://git.openjdk.java.net/jdk/pull/8166
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
Looks okay to me.
-
Marked as reviewed by shade (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/6294
Integrated: 8282170: JVMTI SetBreakpoint metaspace allocation test
On Mon, 21 Feb 2022 10:55:42 GMT, Aleksey Shipilev wrote: > There are few bugs in SetBreakpoint when it reaches for metaspace allocation, > notably [JDK-8214992](https://bugs.openjdk.java.net/browse/JDK-8214992) and > [JDK-8264149](https://bugs.openjdk.java.net/browse/JDK-8264149). This adds > the test that exercises the metaspace allocation paths. > > Requires [JDK-8282172](https://bugs.openjdk.java.net/browse/JDK-8282172) to > pass cleanly in fastdebug mode. > > Additional testing: > - [x] New test on Linux x86_64 fastdebug > - [x] New test on Linux x86_64 release > - [x] New test with > [JDK-8264149](https://bugs.openjdk.java.net/browse/JDK-8264149) reverted > fails on Linux x86_64 release This pull request has now been integrated. Changeset: 7b91bbba Author:Aleksey Shipilev URL: https://git.openjdk.java.net/jdk/commit/7b91bbba82e871edaf133343415e254972c6ddc7 Stats: 193 lines in 2 files changed: 193 ins; 0 del; 0 mod 8282170: JVMTI SetBreakpoint metaspace allocation test Reviewed-by: cjplummer, lmesnik - PR: https://git.openjdk.java.net/jdk/pull/7554
Re: RFR: 8282170: JVMTI SetBreakpoint metaspace allocation test [v5]
On Wed, 9 Mar 2022 10:11:41 GMT, Aleksey Shipilev wrote: >> There are few bugs in SetBreakpoint when it reaches for metaspace >> allocation, notably >> [JDK-8214992](https://bugs.openjdk.java.net/browse/JDK-8214992) and >> [JDK-8264149](https://bugs.openjdk.java.net/browse/JDK-8264149). This adds >> the test that exercises the metaspace allocation paths. >> >> Requires [JDK-8282172](https://bugs.openjdk.java.net/browse/JDK-8282172) to >> pass cleanly in fastdebug mode. >> >> Additional testing: >> - [x] New test on Linux x86_64 fastdebug >> - [x] New test on Linux x86_64 release >> - [x] New test with >> [JDK-8264149](https://bugs.openjdk.java.net/browse/JDK-8264149) reverted >> fails on Linux x86_64 release > > Aleksey Shipilev has updated the pull request incrementally with one > additional commit since the last revision: > > Convert test to C++ Thanks! - PR: https://git.openjdk.java.net/jdk/pull/7554
Re: RFR: 8282170: JVMTI SetBreakpoint metaspace allocation test [v4]
On Tue, 8 Mar 2022 19:16:25 GMT, Leonid Mesnik wrote: >> 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 seven additional >> commits since the last revision: >> >> - Rewrite test to use runtime class generation >> - Review comments >> - Merge branch 'master' into JDK-8282170-jvmti-setbreakpoint-test >> - Fix copyright >> - Merge branch 'master' into JDK-8282170-jvmti-setbreakpoint-test >> - Merge branch 'master' into JDK-8282170-jvmti-setbreakpoint-test >> - Fix > > thank you for fixing @lmesnik, @plummercj -- please re-ack the new version, thanks. - PR: https://git.openjdk.java.net/jdk/pull/7554
Re: RFR: 8282170: JVMTI SetBreakpoint metaspace allocation test [v5]
> There are few bugs in SetBreakpoint when it reaches for metaspace allocation, > notably [JDK-8214992](https://bugs.openjdk.java.net/browse/JDK-8214992) and > [JDK-8264149](https://bugs.openjdk.java.net/browse/JDK-8264149). This adds > the test that exercises the metaspace allocation paths. > > Requires [JDK-8282172](https://bugs.openjdk.java.net/browse/JDK-8282172) to > pass cleanly in fastdebug mode. > > Additional testing: > - [x] New test on Linux x86_64 fastdebug > - [x] New test on Linux x86_64 release > - [x] New test with > [JDK-8264149](https://bugs.openjdk.java.net/browse/JDK-8264149) reverted > fails on Linux x86_64 release Aleksey Shipilev has updated the pull request incrementally with one additional commit since the last revision: Convert test to C++ - Changes: - all: https://git.openjdk.java.net/jdk/pull/7554/files - new: https://git.openjdk.java.net/jdk/pull/7554/files/3a6f943e..5e10e99d Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7554=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7554=03-04 Stats: 221 lines in 2 files changed: 111 ins; 110 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/7554.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7554/head:pull/7554 PR: https://git.openjdk.java.net/jdk/pull/7554
Re: RFR: 8282170: JVMTI SetBreakpoint metaspace allocation test [v4]
On Wed, 9 Mar 2022 02:20:22 GMT, Serguei Spitsyn wrote: > Not a review yet but I'd suggest to convert .c to .cpp. In long term we would > like to get rid of the *.c files in the serviceability/jvmti test folder for > consistency. Good thinking. I converted the test to C++, and this also allowed to simplify the JNI/JVMTI calls a bit. - PR: https://git.openjdk.java.net/jdk/pull/7554
Re: RFR: 8282170: JVMTI SetBreakpoint metaspace allocation test [v3]
On Mon, 28 Feb 2022 18:49:05 GMT, Aleksey Shipilev wrote: >> There are few bugs in SetBreakpoint when it reaches for metaspace >> allocation, notably >> [JDK-8214992](https://bugs.openjdk.java.net/browse/JDK-8214992) and >> [JDK-8264149](https://bugs.openjdk.java.net/browse/JDK-8264149). This adds >> the test that exercises the metaspace allocation paths. >> >> Requires [JDK-8282172](https://bugs.openjdk.java.net/browse/JDK-8282172) to >> pass cleanly in fastdebug mode. >> >> Additional testing: >> - [x] New test on Linux x86_64 fastdebug >> - [x] New test on Linux x86_64 release >> - [x] New test with >> [JDK-8264149](https://bugs.openjdk.java.net/browse/JDK-8264149) reverted >> fails on Linux x86_64 release > > Aleksey Shipilev has updated the pull request incrementally with one > additional commit since the last revision: > > Fix copyright I also rewritten the test to use bundled ASM instead of long target class. This also allows bumping the number of methods in the target without adding a lot of new lines. - PR: https://git.openjdk.java.net/jdk/pull/7554
Re: RFR: 8282170: JVMTI SetBreakpoint metaspace allocation test [v3]
On Mon, 7 Mar 2022 16:58:38 GMT, Leonid Mesnik wrote:
>> Aleksey Shipilev has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Fix copyright
>
> test/hotspot/jtreg/serviceability/jvmti/SetBreakpoint/TestManyBreakpoints.java
> line 35:
>
>> 33: */
>> 34:
>> 35: package serviceability.jvmti.SetBreakpoint;
>
> The serviceability tests don't use packages. The default package is used.
> Please remove it to be consistent.
Done.
> test/hotspot/jtreg/serviceability/jvmti/SetBreakpoint/libTestManyBreakpoints.c
> line 2:
>
>> 1: /*
>> 2: * Copyright (c) 2013, 2022, Oracle and/or its affiliates. All rights
>> reserved.
>
> Shouldn't it be just 2022?
Done.
> test/hotspot/jtreg/serviceability/jvmti/SetBreakpoint/libTestManyBreakpoints.c
> line 35:
>
>> 33: void JNICALL classprepare(jvmtiEnv* jvmti_env, JNIEnv* jni_env, jthread
>> thread, jclass klass) {
>> 34: char* buf;
>> 35: (*jvmti)->GetClassSignature(jvmti, klass, , NULL);
>
> It is required to check jvmti error status for every function. So test fails
> early.
> There is no common library so far so you could just copy-paste
> 'check_jvmti_status' from other tests.
Done.
-
PR: https://git.openjdk.java.net/jdk/pull/7554
Re: RFR: 8282170: JVMTI SetBreakpoint metaspace allocation test [v4]
> There are few bugs in SetBreakpoint when it reaches for metaspace allocation, > notably [JDK-8214992](https://bugs.openjdk.java.net/browse/JDK-8214992) and > [JDK-8264149](https://bugs.openjdk.java.net/browse/JDK-8264149). This adds > the test that exercises the metaspace allocation paths. > > Requires [JDK-8282172](https://bugs.openjdk.java.net/browse/JDK-8282172) to > pass cleanly in fastdebug mode. > > Additional testing: > - [x] New test on Linux x86_64 fastdebug > - [x] New test on Linux x86_64 release > - [x] New test with > [JDK-8264149](https://bugs.openjdk.java.net/browse/JDK-8264149) reverted > fails on Linux x86_64 release 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 seven additional commits since the last revision: - Rewrite test to use runtime class generation - Review comments - Merge branch 'master' into JDK-8282170-jvmti-setbreakpoint-test - Fix copyright - Merge branch 'master' into JDK-8282170-jvmti-setbreakpoint-test - Merge branch 'master' into JDK-8282170-jvmti-setbreakpoint-test - Fix - Changes: - all: https://git.openjdk.java.net/jdk/pull/7554/files - new: https://git.openjdk.java.net/jdk/pull/7554/files/2df7af3b..3a6f943e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7554=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7554=02-03 Stats: 15566 lines in 474 files changed: 9879 ins; 3722 del; 1965 mod Patch: https://git.openjdk.java.net/jdk/pull/7554.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7554/head:pull/7554 PR: https://git.openjdk.java.net/jdk/pull/7554
Re: RFR: 8282170: JVMTI SetBreakpoint metaspace allocation test
On Tue, 22 Feb 2022 19:54:59 GMT, Chris Plummer wrote: >> There are few bugs in SetBreakpoint when it reaches for metaspace >> allocation, notably >> [JDK-8214992](https://bugs.openjdk.java.net/browse/JDK-8214992) and >> [JDK-8264149](https://bugs.openjdk.java.net/browse/JDK-8264149). This adds >> the test that exercises the metaspace allocation paths. >> >> Requires [JDK-8282172](https://bugs.openjdk.java.net/browse/JDK-8282172) to >> pass cleanly in fastdebug mode. >> >> Additional testing: >> - [x] New test on Linux x86_64 fastdebug >> - [x] New test on Linux x86_64 release >> - [x] New test with >> [JDK-8264149](https://bugs.openjdk.java.net/browse/JDK-8264149) reverted >> fails on Linux x86_64 release > > So you load a class 50 times in separate ClassLoaders, and each load triggers > the ClassPrepare event, and each time that happens you set a breakpoint on > each of the 1000 methods, meaning a total of 5000 breakpoints. Doing this > will eventually trigger calling > CollectorPolicy::satisfy_failed_metadata_allocation(). Previous to the > [8264149](https://bugs.openjdk.java.net/browse/JDK-8264149) fix, this used to > trigger an assert. Am I understanding this properly? > Thanks @plummercj! Anyone else to review? Friendly reminder. - PR: https://git.openjdk.java.net/jdk/pull/7554
Re: RFR: 8282170: JVMTI SetBreakpoint metaspace allocation test
On Tue, 22 Feb 2022 19:54:59 GMT, Chris Plummer wrote: >> There are few bugs in SetBreakpoint when it reaches for metaspace >> allocation, notably >> [JDK-8214992](https://bugs.openjdk.java.net/browse/JDK-8214992) and >> [JDK-8264149](https://bugs.openjdk.java.net/browse/JDK-8264149). This adds >> the test that exercises the metaspace allocation paths. >> >> Requires [JDK-8282172](https://bugs.openjdk.java.net/browse/JDK-8282172) to >> pass cleanly in fastdebug mode. >> >> Additional testing: >> - [x] New test on Linux x86_64 fastdebug >> - [x] New test on Linux x86_64 release >> - [x] New test with >> [JDK-8264149](https://bugs.openjdk.java.net/browse/JDK-8264149) reverted >> fails on Linux x86_64 release > > So you load a class 50 times in separate ClassLoaders, and each load triggers > the ClassPrepare event, and each time that happens you set a breakpoint on > each of the 1000 methods, meaning a total of 5000 breakpoints. Doing this > will eventually trigger calling > CollectorPolicy::satisfy_failed_metadata_allocation(). Previous to the > [8264149](https://bugs.openjdk.java.net/browse/JDK-8264149) fix, this used to > trigger an assert. Am I understanding this properly? Thanks @plummercj! Anyone else to review? - PR: https://git.openjdk.java.net/jdk/pull/7554
Re: RFR: 8282170: JVMTI SetBreakpoint metaspace allocation test [v3]
On Tue, 22 Feb 2022 19:43:46 GMT, Chris Plummer wrote: >> Aleksey Shipilev has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Fix copyright > > test/hotspot/jtreg/serviceability/jvmti/SetBreakpoint/TestManyBreakpoints.java > line 2: > >> 1: /* >> 2: * Copyright (c) 2013, 2022, Oracle and/or its affiliates. All rights >> reserved. > > Unless you copied a substantial part of this test, I don't think the 2013 > copyright is necessary. Fixed. - PR: https://git.openjdk.java.net/jdk/pull/7554
Re: RFR: 8282170: JVMTI SetBreakpoint metaspace allocation test [v3]
> There are few bugs in SetBreakpoint when it reaches for metaspace allocation, > notably [JDK-8214992](https://bugs.openjdk.java.net/browse/JDK-8214992) and > [JDK-8264149](https://bugs.openjdk.java.net/browse/JDK-8264149). This adds > the test that exercises the metaspace allocation paths. > > Requires [JDK-8282172](https://bugs.openjdk.java.net/browse/JDK-8282172) to > pass cleanly in fastdebug mode. > > Additional testing: > - [x] New test on Linux x86_64 fastdebug > - [x] New test on Linux x86_64 release > - [x] New test with > [JDK-8264149](https://bugs.openjdk.java.net/browse/JDK-8264149) reverted > fails on Linux x86_64 release Aleksey Shipilev has updated the pull request incrementally with one additional commit since the last revision: Fix copyright - Changes: - all: https://git.openjdk.java.net/jdk/pull/7554/files - new: https://git.openjdk.java.net/jdk/pull/7554/files/4edd20df..2df7af3b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7554=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7554=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/7554.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7554/head:pull/7554 PR: https://git.openjdk.java.net/jdk/pull/7554
Re: RFR: 8282170: JVMTI SetBreakpoint metaspace allocation test [v2]
> There are few bugs in SetBreakpoint when it reaches for metaspace allocation, > notably [JDK-8214992](https://bugs.openjdk.java.net/browse/JDK-8214992) and > [JDK-8264149](https://bugs.openjdk.java.net/browse/JDK-8264149). This adds > the test that exercises the metaspace allocation paths. > > Requires [JDK-8282172](https://bugs.openjdk.java.net/browse/JDK-8282172) to > pass cleanly in fastdebug mode. > > Additional testing: > - [x] New test on Linux x86_64 fastdebug > - [x] New test on Linux x86_64 release > - [x] New test with > [JDK-8264149](https://bugs.openjdk.java.net/browse/JDK-8264149) reverted > fails on Linux x86_64 release 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 three additional commits since the last revision: - Merge branch 'master' into JDK-8282170-jvmti-setbreakpoint-test - Merge branch 'master' into JDK-8282170-jvmti-setbreakpoint-test - Fix - Changes: - all: https://git.openjdk.java.net/jdk/pull/7554/files - new: https://git.openjdk.java.net/jdk/pull/7554/files/5ffee594..4edd20df Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7554=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7554=00-01 Stats: 11653 lines in 264 files changed: 8362 ins; 1419 del; 1872 mod Patch: https://git.openjdk.java.net/jdk/pull/7554.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7554/head:pull/7554 PR: https://git.openjdk.java.net/jdk/pull/7554
Re: RFR: 8282170: JVMTI SetBreakpoint metaspace allocation test
On Tue, 22 Feb 2022 19:54:59 GMT, Chris Plummer wrote: > So you load a class 50 times in separate ClassLoaders, and each load triggers > the ClassPrepare event, and each time that happens you set a breakpoint on > each of the 1000 methods, meaning a total of 5000 breakpoints. Doing this > will eventually trigger calling > CollectorPolicy::satisfy_failed_metadata_allocation(). Previous to the > [8264149](https://bugs.openjdk.java.net/browse/JDK-8264149) fix, this used to > trigger an assert. Am I understanding this properly? Yes. That assert is actually the part of the larger issue: metaspace is full, GC is requested, and VM would probably hang if we allow it to happen, since we cannot easily schedule Full GCs when already at safepoint. - PR: https://git.openjdk.java.net/jdk/pull/7554
RFR: 8282170: JVMTI SetBreakpoint metaspace allocation test
There are few bugs in SetBreakpoint when it reaches for metaspace allocation, notably [JDK-8214992](https://bugs.openjdk.java.net/browse/JDK-8214992) and [JDK-8264149](https://bugs.openjdk.java.net/browse/JDK-8264149). This adds the test that exercises the metaspace allocation paths. Additional testing: - [x] New test on Linux x86_64 fastdebug - [x] New test on Linux x86_64 release - [x] New test with [JDK-8264149](https://bugs.openjdk.java.net/browse/JDK-8264149) reverted fails on Linux x86_64 release - Commit messages: - Fix Changes: https://git.openjdk.java.net/jdk/pull/7554/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7554=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8282170 Stats: 1127 lines in 2 files changed: 1127 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/7554.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7554/head:pull/7554 PR: https://git.openjdk.java.net/jdk/pull/7554
Integrated: 8280889: java/lang/instrument/GetObjectSizeIntrinsicsTest.java fails with -XX:-UseCompressedOops
On Fri, 28 Jan 2022 15:37:59 GMT, Aleksey Shipilev wrote: > Recent test regression after adding new cases in the test. Without compressed > oops, ~1G elements `Object[]` array takes >8G of memory, which fails the > test. The fix cuts it down to 512M when reference size is 8 bytes. > Additionally, `ObjectAlignmentInBytes=32` is excessive for new test configs. > > Additional testing: > - [x] Linux x86_64 fastdebug, default, affected test still passes > - [x] Linux x86_32 fastdebug, default, affected test still passes > - [x] Linux x86_64 fastdebug, `-XX:-UseCompressedOops`, affected test now > passes This pull request has now been integrated. Changeset: 251351f4 Author:Aleksey Shipilev URL: https://git.openjdk.java.net/jdk/commit/251351f49498ea39150b38860b8f73232fbaf05d Stats: 10 lines in 1 file changed: 1 ins; 3 del; 6 mod 8280889: java/lang/instrument/GetObjectSizeIntrinsicsTest.java fails with -XX:-UseCompressedOops Reviewed-by: sspitsyn, dcubed - PR: https://git.openjdk.java.net/jdk/pull/7269
Re: RFR: 8280889: java/lang/instrument/GetObjectSizeIntrinsicsTest.java fails with -XX:-UseCompressedOops
On Fri, 28 Jan 2022 15:37:59 GMT, Aleksey Shipilev wrote: > Recent test regression after adding new cases in the test. Without compressed > oops, ~1G elements `Object[]` array takes >8G of memory, which fails the > test. The fix cuts it down to 512M when reference size is 8 bytes. > Additionally, `ObjectAlignmentInBytes=32` is excessive for new test configs. > > Additional testing: > - [x] Linux x86_64 fastdebug, default, affected test still passes > - [x] Linux x86_32 fastdebug, default, affected test still passes > - [x] Linux x86_64 fastdebug, `-XX:-UseCompressedOops`, affected test now > passes Thanks! - PR: https://git.openjdk.java.net/jdk/pull/7269
RFR: 8280889: java/lang/instrument/GetObjectSizeIntrinsicsTest.java fails with -XX:-UseCompressedOops
Recent test regression after adding new cases in the test. Without compressed oops, ~1G elements `Object[]` array takes >8G of memory, which fails the test. The fix cuts it down to 512M when reference size is 8 bytes. Additionally, `ObjectAlignmentInBytes=32` is excessive for new test configs. Additional testing: - [x] Linux x86_64 fastdebug, default, affected test still passes - [x] Linux x86_32 fastdebug, default, affected test still passes - [x] Linux x86_64 fastdebug, `-XX:-UseCompressedOops`, affected test now passes - Commit messages: - Fix Changes: https://git.openjdk.java.net/jdk/pull/7269/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7269=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8280889 Stats: 10 lines in 1 file changed: 1 ins; 3 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/7269.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7269/head:pull/7269 PR: https://git.openjdk.java.net/jdk/pull/7269
Integrated: 8280166: Extend java/lang/instrument/GetObjectSizeIntrinsicsTest.java test cases
On Tue, 18 Jan 2022 18:33:29 GMT, Aleksey Shipilev wrote: > While working on JDK-8280003, I noticed that > java/lang/instrument/GetObjectSizeIntrinsicsTest.java does not test arrays > with more than 1-byte size elements, and no large arrays (past 4G limit) are > tested either. It would be better to add those test cases. > > Additional testing: > - [x] Linux x86_64 fastdebug, affected test still passes > - [x] Linux x86_32 fastdebug, affected test still passes > - [x] Linux AArch64 fastdebug, affected test still passes > - [x] Linux PPC64 fastdebug, affected test still passes This pull request has now been integrated. Changeset: 76fe03fe Author:Aleksey Shipilev URL: https://git.openjdk.java.net/jdk/commit/76fe03fe01a7c824e2e9263de95b8bcbb4b9d752 Stats: 89 lines in 1 file changed: 62 ins; 0 del; 27 mod 8280166: Extend java/lang/instrument/GetObjectSizeIntrinsicsTest.java test cases Reviewed-by: sspitsyn, lmesnik - PR: https://git.openjdk.java.net/jdk/pull/7132
Re: RFR: 8280166: Extend java/lang/instrument/GetObjectSizeIntrinsicsTest.java test cases [v3]
On Tue, 18 Jan 2022 19:36:18 GMT, Chris Plummer wrote:
>> 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 four additional
>> commits since the last revision:
>>
>> - Update copyright dates
>> - Merge branch 'master' into JDK-8280166-getobjectsize
>> - Merge branch 'master' into JDK-8280166-getobjectsize
>> - Fix
>
> test/jdk/java/lang/instrument/GetObjectSizeIntrinsicsTest.java line 326:
>
>> 324:
>> 325: public static void main(String[] args)throws Throwable {
>> 326: new GetObjectSizeIntrinsicsTest(args[0], (args.length >= 2 ?
>> args[1] : "")).runTest();
>
> Shouldn't this be `args.length == 2`?
@plummercj This does not look like a review-blocking comment, so I am going to
proceed with integration, if there are no other comments.
-
PR: https://git.openjdk.java.net/jdk/pull/7132
Re: RFR: 8280166: Extend java/lang/instrument/GetObjectSizeIntrinsicsTest.java test cases [v2]
On Fri, 21 Jan 2022 16:19:07 GMT, Leonid Mesnik wrote: > Please update copyright years. Updated, thanks! - PR: https://git.openjdk.java.net/jdk/pull/7132
Re: RFR: 8280166: Extend java/lang/instrument/GetObjectSizeIntrinsicsTest.java test cases [v3]
> While working on JDK-8280003, I noticed that > java/lang/instrument/GetObjectSizeIntrinsicsTest.java does not test arrays > with more than 1-byte size elements, and no large arrays (past 4G limit) are > tested either. It would be better to add those test cases. > > Additional testing: > - [x] Linux x86_64 fastdebug, affected test still passes > - [x] Linux x86_32 fastdebug, affected test still passes > - [x] Linux AArch64 fastdebug, affected test still passes > - [x] Linux PPC64 fastdebug, affected test still passes 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 four additional commits since the last revision: - Update copyright dates - Merge branch 'master' into JDK-8280166-getobjectsize - Merge branch 'master' into JDK-8280166-getobjectsize - Fix - Changes: - all: https://git.openjdk.java.net/jdk/pull/7132/files - new: https://git.openjdk.java.net/jdk/pull/7132/files/dd9ce049..98653011 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7132=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7132=01-02 Stats: 327 lines in 51 files changed: 159 ins; 67 del; 101 mod Patch: https://git.openjdk.java.net/jdk/pull/7132.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7132/head:pull/7132 PR: https://git.openjdk.java.net/jdk/pull/7132
Re: RFR: 8280166: Extend java/lang/instrument/GetObjectSizeIntrinsicsTest.java test cases [v2]
> While working on JDK-8280003, I noticed that > java/lang/instrument/GetObjectSizeIntrinsicsTest.java does not test arrays > with more than 1-byte size elements, and no large arrays (past 4G limit) are > tested either. It would be better to add those test cases. > > Additional testing: > - [x] Linux x86_64 fastdebug, affected test still passes > - [x] Linux x86_32 fastdebug, affected test still passes > - [x] Linux AArch64 fastdebug, affected test still passes > - [x] Linux PPC64 fastdebug, affected test still passes 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 two additional commits since the last revision: - Merge branch 'master' into JDK-8280166-getobjectsize - Fix - Changes: - all: https://git.openjdk.java.net/jdk/pull/7132/files - new: https://git.openjdk.java.net/jdk/pull/7132/files/29e6fd24..dd9ce049 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7132=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7132=00-01 Stats: 7693 lines in 284 files changed: 5179 ins; 1672 del; 842 mod Patch: https://git.openjdk.java.net/jdk/pull/7132.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7132/head:pull/7132 PR: https://git.openjdk.java.net/jdk/pull/7132
Re: RFR: 8280166: Extend java/lang/instrument/GetObjectSizeIntrinsicsTest.java test cases
On Tue, 18 Jan 2022 19:36:18 GMT, Chris Plummer wrote:
>> While working on JDK-8280003, I noticed that
>> java/lang/instrument/GetObjectSizeIntrinsicsTest.java does not test arrays
>> with more than 1-byte size elements, and no large arrays (past 4G limit) are
>> tested either. It would be better to add those test cases.
>>
>> Additional testing:
>> - [x] Linux x86_64 fastdebug, affected test still passes
>> - [x] Linux x86_32 fastdebug, affected test still passes
>> - [x] Linux AArch64 fastdebug, affected test still passes
>> - [x] Linux PPC64 fastdebug, affected test still passes
>
> test/jdk/java/lang/instrument/GetObjectSizeIntrinsicsTest.java line 326:
>
>> 324:
>> 325: public static void main(String[] args)throws Throwable {
>> 326: new GetObjectSizeIntrinsicsTest(args[0], (args.length >= 2 ?
>> args[1] : "")).runTest();
>
> Shouldn't this be `args.length == 2`?
@plummercj, are you good with this explanation?
-
PR: https://git.openjdk.java.net/jdk/pull/7132
Re: RFR: 8280166: Extend java/lang/instrument/GetObjectSizeIntrinsicsTest.java test cases
On Tue, 18 Jan 2022 19:36:18 GMT, Chris Plummer wrote:
>> While working on JDK-8280003, I noticed that
>> java/lang/instrument/GetObjectSizeIntrinsicsTest.java does not test arrays
>> with more than 1-byte size elements, and no large arrays (past 4G limit) are
>> tested either. It would be better to add those test cases.
>>
>> Additional testing:
>> - [x] Linux x86_64 fastdebug, affected test still passes
>> - [x] Linux x86_32 fastdebug, affected test still passes
>> - [x] Linux AArch64 fastdebug, affected test still passes
>> - [x] Linux PPC64 fastdebug, affected test still passes
>
> test/jdk/java/lang/instrument/GetObjectSizeIntrinsicsTest.java line 326:
>
>> 324:
>> 325: public static void main(String[] args)throws Throwable {
>> 326: new GetObjectSizeIntrinsicsTest(args[0], (args.length >= 2 ?
>> args[1] : "")).runTest();
>
> Shouldn't this be `args.length == 2`?
`>= 2` is more future-proof, I think, as it checks that _at least_ two
arguments exists, which allows to read `args[1]`. Accidental addition of third
argument would not silently break the test.
-
PR: https://git.openjdk.java.net/jdk/pull/7132
RFR: 8280166: Extend java/lang/instrument/GetObjectSizeIntrinsicsTest.java test cases
While working on JDK-8280003, I noticed that java/lang/instrument/GetObjectSizeIntrinsicsTest.java does not test arrays with more than 1-byte size elements, and no large arrays (past 4G limit) are tested either. It would be better to add those test cases. Additional testing: - [x] Linux x86_64 fastdebug, affected test still passes - [x] Linux x86_32 fastdebug, affected test still passes - [ ] Linux AArch64 fastdebug, affected test still passes - [ ] Linux PPC64 fastdebug, affected test still passes - Commit messages: - Fix Changes: https://git.openjdk.java.net/jdk/pull/7132/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7132=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8280166 Stats: 88 lines in 1 file changed: 62 ins; 0 del; 26 mod Patch: https://git.openjdk.java.net/jdk/pull/7132.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7132/head:pull/7132 PR: https://git.openjdk.java.net/jdk/pull/7132
Re: RFR: JDK-8280002: jmap -histo may leak stream
On Fri, 14 Jan 2022 10:04:53 GMT, Thomas Stuefe wrote: > Very trivial fix to a handle/memory leak. > > JDK-8215624 added parallel heap iteration to both `jmap -histo` and `jcmd > GC.class_histogram`. When called with an explicit file and an invalid > argument for number of threads, it leaks the file (bit of memory and a > handle). > > Reproduce with: > > `jmap -histo:parallel=notanumber,file=xx.txt` > > Can only be reproduced with jmap. jcmd is safe, arguments are handled > correctly in shared code. Okay then! - Marked as reviewed by shade (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7078
Re: RFR: JDK-8280002: jmap -histo may leak stream
On Fri, 14 Jan 2022 10:04:53 GMT, Thomas Stuefe wrote: > Very trivial fix to a handle/memory leak. > > JDK-8215624 added parallel heap iteration to both `jmap -histo` and `jcmd > GC.class_histogram`. When called with an explicit file and an invalid > argument for number of threads, it leaks the file (bit of memory and a > handle). > > Reproduce with: > > `jmap -histo:parallel=notanumber,file=xx.txt` > > Can only be reproduced with jmap. jcmd is safe, arguments are handled > correctly in shared code. This is not as trivial, AFAICS. Note that the existing code `delete fs` after checking `if (os != NULL && os != out)`. Does it mean this patch can effectively `delete out`? - PR: https://git.openjdk.java.net/jdk/pull/7078
Re: RFR: 8274903: Zero: Support AsyncGetCallTrace [v5]
On Tue, 30 Nov 2021 11:26:04 GMT, Andrew Haley wrote:
>> 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 seven additional
>> commits since the last revision:
>>
>> - Merge branch 'master' into JDK-8274903-zero-asyncgetcalltrace
>> - Fix a comment
>> - Merge branch 'master' into JDK-8274903-zero-asyncgetcalltrace
>> - More reviews
>> - Review feedback
>> - Merge branch 'master' into JDK-8274903-zero-asyncgetcalltrace
>> - Initial work: runs async-profiler successfully
>
> src/hotspot/cpu/zero/frame_zero.cpp line 139:
>
>> 137: assert(is_interpreted_frame(), "Not an interpreted frame");
>> 138: // These are reasonable sanity checks
>> 139: if (fp() == 0 || (intptr_t(fp()) & (wordSize-1)) != 0) {
>
> Use `is_aligned()` here?
Paging @theRealAph ;)
-
PR: https://git.openjdk.java.net/jdk/pull/5848
Re: RFR: 8274903: Zero: Support AsyncGetCallTrace [v5]
On Tue, 30 Nov 2021 11:26:04 GMT, Andrew Haley wrote:
>> 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 seven additional
>> commits since the last revision:
>>
>> - Merge branch 'master' into JDK-8274903-zero-asyncgetcalltrace
>> - Fix a comment
>> - Merge branch 'master' into JDK-8274903-zero-asyncgetcalltrace
>> - More reviews
>> - Review feedback
>> - Merge branch 'master' into JDK-8274903-zero-asyncgetcalltrace
>> - Initial work: runs async-profiler successfully
>
> src/hotspot/cpu/zero/frame_zero.cpp line 139:
>
>> 137: assert(is_interpreted_frame(), "Not an interpreted frame");
>> 138: // These are reasonable sanity checks
>> 139: if (fp() == 0 || (intptr_t(fp()) & (wordSize-1)) != 0) {
>
> Use `is_aligned()` here?
@theRealAph Do you agree with above? Any more comments?
-
PR: https://git.openjdk.java.net/jdk/pull/5848
Re: RFR: 8274903: Zero: Support AsyncGetCallTrace [v5]
On Tue, 30 Nov 2021 11:26:04 GMT, Andrew Haley wrote:
>> 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 seven additional
>> commits since the last revision:
>>
>> - Merge branch 'master' into JDK-8274903-zero-asyncgetcalltrace
>> - Fix a comment
>> - Merge branch 'master' into JDK-8274903-zero-asyncgetcalltrace
>> - More reviews
>> - Review feedback
>> - Merge branch 'master' into JDK-8274903-zero-asyncgetcalltrace
>> - Initial work: runs async-profiler successfully
>
> src/hotspot/cpu/zero/frame_zero.cpp line 139:
>
>> 137: assert(is_interpreted_frame(), "Not an interpreted frame");
>> 138: // These are reasonable sanity checks
>> 139: if (fp() == 0 || (intptr_t(fp()) & (wordSize-1)) != 0) {
>
> Use `is_aligned()` here?
I could, but this matches what other platforms are doing in their
`frame::is_interpreted_frame_valid()`. If there are no other fixes needed, okay
if I keep this one in place? Otherwise, I would need to re-test the whole thing
for a minor touchup, which is tedious.
-
PR: https://git.openjdk.java.net/jdk/pull/5848
Re: RFR: 8274903: Zero: Support AsyncGetCallTrace [v5]
> This is a Zero infrastructure improvement that makes Zero VM work with > AsyncGetCallTrace, and by extension, async-profiler. > > Zero is quite odd in stack management. The "real" stack actually contains the > C++ Interpreter and the rest of VM code. The Java stack is reported through > the usual "frame" mechanism the rest of VM uses to get the mapping from > Template Interpreter, stub, and compiled code. So, to support Java-centric > AsyncGetCallTrace, we t "only" need Zero to report the proper Java frames > from its ZeroStack from the profiling/signal handlers. > > Additional testing: > - [x] Linux x86_64 Zero `serviceability/AsyncGetCallTrace` now pass > - [x] Linux x86_64 Zero works with `async-profiler` 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 seven additional commits since the last revision: - Merge branch 'master' into JDK-8274903-zero-asyncgetcalltrace - Fix a comment - Merge branch 'master' into JDK-8274903-zero-asyncgetcalltrace - More reviews - Review feedback - Merge branch 'master' into JDK-8274903-zero-asyncgetcalltrace - Initial work: runs async-profiler successfully - Changes: - all: https://git.openjdk.java.net/jdk/pull/5848/files - new: https://git.openjdk.java.net/jdk/pull/5848/files/bc4ba33b..373f15ae Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5848=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5848=03-04 Stats: 22783 lines in 424 files changed: 13220 ins; 6227 del; 3336 mod Patch: https://git.openjdk.java.net/jdk/pull/5848.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5848/head:pull/5848 PR: https://git.openjdk.java.net/jdk/pull/5848
Re: RFR: 8274903: Zero: Support AsyncGetCallTrace [v4]
> This is a Zero infrastructure improvement that makes Zero VM work with > AsyncGetCallTrace, and by extension, async-profiler. > > Zero is quite odd in stack management. The "real" stack actually contains the > C++ Interpreter and the rest of VM code. The Java stack is reported through > the usual "frame" mechanism the rest of VM uses to get the mapping from > Template Interpreter, stub, and compiled code. So, to support Java-centric > AsyncGetCallTrace, we t "only" need Zero to report the proper Java frames > from its ZeroStack from the profiling/signal handlers. > > Additional testing: > - [x] Linux x86_64 Zero `serviceability/AsyncGetCallTrace` now pass > - [x] Linux x86_64 Zero works with `async-profiler` 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 six additional commits since the last revision: - Fix a comment - Merge branch 'master' into JDK-8274903-zero-asyncgetcalltrace - More reviews - Review feedback - Merge branch 'master' into JDK-8274903-zero-asyncgetcalltrace - Initial work: runs async-profiler successfully - Changes: - all: https://git.openjdk.java.net/jdk/pull/5848/files - new: https://git.openjdk.java.net/jdk/pull/5848/files/68ef4b63..bc4ba33b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5848=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5848=02-03 Stats: 44745 lines in 800 files changed: 32663 ins; 5661 del; 6421 mod Patch: https://git.openjdk.java.net/jdk/pull/5848.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5848/head:pull/5848 PR: https://git.openjdk.java.net/jdk/pull/5848
Re: RFR: 8274903: Zero: Support AsyncGetCallTrace [v3]
On Wed, 10 Nov 2021 18:03:00 GMT, Serguei Spitsyn wrote: >> Aleksey Shipilev has updated the pull request incrementally with one >> additional commit since the last revision: >> >> More reviews > > Marked as reviewed by sspitsyn (Reviewer). > Thank you, @sspitsyn! Any more reviews, anyone? No other reviews? I'd like to integrate this soon. - PR: https://git.openjdk.java.net/jdk/pull/5848
Re: RFR: 8274903: Zero: Support AsyncGetCallTrace [v3]
On Wed, 10 Nov 2021 18:03:00 GMT, Serguei Spitsyn wrote: >> Aleksey Shipilev has updated the pull request incrementally with one >> additional commit since the last revision: >> >> More reviews > > Marked as reviewed by sspitsyn (Reviewer). Thank you, @sspitsyn! Any more reviews, anyone? - PR: https://git.openjdk.java.net/jdk/pull/5848
Re: RFR: 8274903: Zero: Support AsyncGetCallTrace [v3]
On Wed, 10 Nov 2021 12:44:16 GMT, Serguei Spitsyn wrote: >> Aleksey Shipilev has updated the pull request incrementally with one >> additional commit since the last revision: >> >> More reviews > > src/hotspot/share/prims/forte.cpp line 348: > >> 346: return false; >> 347: } >> 348: #endif > > Could you, please, add some simple comments explaining each case at lines: > 325, 329 and 336? See new commits! - PR: https://git.openjdk.java.net/jdk/pull/5848
Re: RFR: 8274903: Zero: Support AsyncGetCallTrace [v3]
> This is a Zero infrastructure improvement that makes Zero VM work with > AsyncGetCallTrace, and by extension, async-profiler. > > Zero is quite odd in stack management. The "real" stack actually contains the > C++ Interpreter and the rest of VM code. The Java stack is reported through > the usual "frame" mechanism the rest of VM uses to get the mapping from > Template Interpreter, stub, and compiled code. So, to support Java-centric > AsyncGetCallTrace, we t "only" need Zero to report the proper Java frames > from its ZeroStack from the profiling/signal handlers. > > Additional testing: > - [x] Linux x86_64 Zero `serviceability/AsyncGetCallTrace` now pass > - [x] Linux x86_64 Zero works with `async-profiler` Aleksey Shipilev has updated the pull request incrementally with one additional commit since the last revision: More reviews - Changes: - all: https://git.openjdk.java.net/jdk/pull/5848/files - new: https://git.openjdk.java.net/jdk/pull/5848/files/8e25258d..68ef4b63 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5848=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5848=01-02 Stats: 7 lines in 1 file changed: 7 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/5848.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5848/head:pull/5848 PR: https://git.openjdk.java.net/jdk/pull/5848
Re: RFR: 8274903: Zero: Support AsyncGetCallTrace [v2]
On Wed, 10 Nov 2021 12:41:44 GMT, Serguei Spitsyn wrote: >> 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 three additional >> commits since the last revision: >> >> - Review feedback >> - Merge branch 'master' into JDK-8274903-zero-asyncgetcalltrace >> - Initial work: runs async-profiler successfully > > src/hotspot/cpu/zero/frame_zero.cpp line 174: > >> 172: >> 173: // validate locals >> 174: address locals = (address) *interpreter_frame_locals_addr(); > > Unneeded spaces around '(address)'. Fixed. - PR: https://git.openjdk.java.net/jdk/pull/5848
Re: RFR: 8274903: Zero: Support AsyncGetCallTrace [v2]
> This is a Zero infrastructure improvement that makes Zero VM work with > AsyncGetCallTrace, and by extension, async-profiler. > > Zero is quite odd in stack management. The "real" stack actually contains the > C++ Interpreter and the rest of VM code. The Java stack is reported through > the usual "frame" mechanism the rest of VM uses to get the mapping from > Template Interpreter, stub, and compiled code. So, to support Java-centric > AsyncGetCallTrace, we t "only" need Zero to report the proper Java frames > from its ZeroStack from the profiling/signal handlers. > > Additional testing: > - [x] Linux x86_64 Zero `serviceability/AsyncGetCallTrace` now pass > - [x] Linux x86_64 Zero works with `async-profiler` 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 three additional commits since the last revision: - Review feedback - Merge branch 'master' into JDK-8274903-zero-asyncgetcalltrace - Initial work: runs async-profiler successfully - Changes: - all: https://git.openjdk.java.net/jdk/pull/5848/files - new: https://git.openjdk.java.net/jdk/pull/5848/files/5575516c..8e25258d Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5848=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5848=00-01 Stats: 888778 lines in 1818 files changed: 455790 ins; 426281 del; 6707 mod Patch: https://git.openjdk.java.net/jdk/pull/5848.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5848/head:pull/5848 PR: https://git.openjdk.java.net/jdk/pull/5848
Re: RFR: 8274903: Zero: Support AsyncGetCallTrace
On Thu, 7 Oct 2021 12:42:48 GMT, Aleksey Shipilev wrote: > This is a Zero infrastructure improvement that makes Zero VM work with > AsyncGetCallTrace, and by extension, async-profiler. > > Zero is quite odd in stack management. The "real" stack actually contains the > C++ Interpreter and the rest of VM code. The Java stack is reported through > the usual "frame" mechanism the rest of VM uses to get the mapping from > Template Interpreter, stub, and compiled code. So, to support Java-centric > AsyncGetCallTrace, we t "only" need Zero to report the proper Java frames > from its ZeroStack from the profiling/signal handlers. > > Additional testing: > - [x] Linux x86_64 Zero `serviceability/AsyncGetCallTrace` now pass > - [x] Linux x86_64 Zero works with `async-profiler` Anyone has opinions about this patch? :) - PR: https://git.openjdk.java.net/jdk/pull/5848
RFR: 8274903: Zero: Support AsyncGetCallTrace
This is a Zero infrastructure improvement that makes Zero VM work with AsyncGetCallTrace, and by extension, async-profiler. Zero is quite odd in stack management. The "real" stack actually contains the C++ Interpreter and the rest of VM code. The Java stack is reported through the usual "frame" mechanism the rest of VM uses to get the mapping from Template Interpreter, stub, and compiled code. So, to support Java-centric AsyncGetCallTrace, we t "only" need Zero to report the proper Java frames from its ZeroStack from the profiling/signal handlers. Additional testing: - [x] Linux x86_64 Zero `serviceability/AsyncGetCallTrace` now pass - [x] Linux x86_64 Zero works with `async-profiler` - Commit messages: - Initial work: runs async-profiler successfully Changes: https://git.openjdk.java.net/jdk/pull/5848/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5848=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8274903 Stats: 136 lines in 4 files changed: 124 ins; 6 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/5848.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5848/head:pull/5848 PR: https://git.openjdk.java.net/jdk/pull/5848
Integrated: 8274522: java/lang/management/ManagementFactory/MXBeanException.java test fails with Shenandoah
On Wed, 29 Sep 2021 17:46:46 GMT, Aleksey Shipilev wrote: > Fails like this: > > > $ CONF=linux-x86_64-server-fastdebug make run-test > TEST=java/lang/management/ManagementFactory/MXBeanException.java > TEST_VM_OPTS="-XX:+UseShenandoahGC" > > ... > > java.lang.NullPointerException: Cannot invoke > "java.lang.management.MemoryPoolMXBean.setUsageThreshold(long)" because > "MXBeanException.pool" is null > > > This test tries to find the pool that is `!p.isUsageThresholdSupported()`, > and for Shenandoah there is no such pool. Likewise with ZGC, which is already > filtered. > > Additional testing: > - [x] Affected test is now skipped for Shenandoah > - [x] Affected test is still skipped for Z > - [x] Affected test is still passing for G1 This pull request has now been integrated. Changeset: c0533ef2 Author:Aleksey Shipilev URL: https://git.openjdk.java.net/jdk/commit/c0533ef2d8e526aaec0eebe862f4bbefc159ea37 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8274522: java/lang/management/ManagementFactory/MXBeanException.java test fails with Shenandoah Reviewed-by: alanb, mchung - PR: https://git.openjdk.java.net/jdk/pull/5757
Re: RFR: 8274522: java/lang/management/ManagementFactory/MXBeanException.java test fails with Shenandoah
On Wed, 29 Sep 2021 17:46:46 GMT, Aleksey Shipilev wrote: > Fails like this: > > > $ CONF=linux-x86_64-server-fastdebug make run-test > TEST=java/lang/management/ManagementFactory/MXBeanException.java > TEST_VM_OPTS="-XX:+UseShenandoahGC" > > ... > > java.lang.NullPointerException: Cannot invoke > "java.lang.management.MemoryPoolMXBean.setUsageThreshold(long)" because > "MXBeanException.pool" is null > > > This test tries to find the pool that is `!p.isUsageThresholdSupported()`, > and for Shenandoah there is no such pool. Likewise with ZGC, which is already > filtered. > > Additional testing: > - [x] Affected test is now skipped for Shenandoah > - [x] Affected test is still skipped for Z > - [x] Affected test is still passing for G1 Thanks! - PR: https://git.openjdk.java.net/jdk/pull/5757
Integrated: 8274523: java/lang/management/MemoryMXBean/MemoryTest.java test should handle Shenandoah
On Wed, 29 Sep 2021 17:56:00 GMT, Aleksey Shipilev wrote: > Currently it fails with: > > > $ CONF=linux-x86_64-server-fastdebug make run-test > TEST=java/lang/management/MemoryMXBean/MemoryTest.java > > STDERR: > java.lang.RuntimeException: TEST FAILED: Number of heap pools = 1 but > expected <= 3 and >= 3 > > > Z already handles it with a special configuration, Shenandoah should do the > same. > > Additional testing: > - [x] Affected test now works for Shenandoah > - [x] Affected test still works for Z > - [x] Affected test still works for G1 This pull request has now been integrated. Changeset: f8415a9b Author:Aleksey Shipilev URL: https://git.openjdk.java.net/jdk/commit/f8415a9b2f610ed431e6948c8174f6d982e5b31f Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod 8274523: java/lang/management/MemoryMXBean/MemoryTest.java test should handle Shenandoah Reviewed-by: mchung, cjplummer - PR: https://git.openjdk.java.net/jdk/pull/5758
Re: RFR: 8274523: java/lang/management/MemoryMXBean/MemoryTest.java test should handle Shenandoah
On Wed, 29 Sep 2021 17:56:00 GMT, Aleksey Shipilev wrote: > Currently it fails with: > > > $ CONF=linux-x86_64-server-fastdebug make run-test > TEST=java/lang/management/MemoryMXBean/MemoryTest.java > > STDERR: > java.lang.RuntimeException: TEST FAILED: Number of heap pools = 1 but > expected <= 3 and >= 3 > > > Z already handles it with a special configuration, Shenandoah should do the > same. > > Additional testing: > - [x] Affected test now works for Shenandoah > - [x] Affected test still works for Z > - [x] Affected test still works for G1 Thanks! - PR: https://git.openjdk.java.net/jdk/pull/5758
RFR: 8274523: java/lang/management/MemoryMXBean/MemoryTest.java test should handle Shenandoah
Currently it fails with: $ CONF=linux-x86_64-server-fastdebug make run-test TEST=java/lang/management/MemoryMXBean/MemoryTest.java STDERR: java.lang.RuntimeException: TEST FAILED: Number of heap pools = 1 but expected <= 3 and >= 3 Z already handles it with a special configuration, Shenandoah should do the same. Additional testing: - [x] Affected test now works for Shenandoah - [x] Affected test still works for Z - [x] Affected test still works for G1 - Commit messages: - Fix Changes: https://git.openjdk.java.net/jdk/pull/5758/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5758=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8274523 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/5758.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5758/head:pull/5758 PR: https://git.openjdk.java.net/jdk/pull/5758
RFR: 8274522: java/lang/management/ManagementFactory/MXBeanException.java test fails with Shenandoah
Fails like this: $ CONF=linux-x86_64-server-fastdebug make run-test TEST=java/lang/management/ManagementFactory/MXBeanException.java TEST_VM_OPTS="-XX:+UseShenandoahGC" ... java.lang.NullPointerException: Cannot invoke "java.lang.management.MemoryPoolMXBean.setUsageThreshold(long)" because "MXBeanException.pool" is null This test tries to find the pool that is `!p.isUsageThresholdSupported()`, and for Shenandoah there is no such pool. Likewise with ZGC, which is already filtered. Additional testing: - [x] Affected test is now skipped for Shenandoah - [x] Affected test is still skipped for Z - [x] Affected test is still passing for G1 - Commit messages: - Fix Changes: https://git.openjdk.java.net/jdk/pull/5757/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5757=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8274522 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/5757.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5757/head:pull/5757 PR: https://git.openjdk.java.net/jdk/pull/5757
Re: RFR: 8264028: Typo in javax.management.relation.RelationService::purgeRelations
On Sun, 28 Feb 2021 10:48:55 GMT, 赵延 wrote: > 8264028: Typo in javax.management.relation.RelationService::purgeRelations Marked as reviewed by shade (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2765
Re: RFR: 8264805: Remove the experimental Ahead-of-Time Compiler [v4]
On Thu, 8 Apr 2021 17:24:38 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
>
> Vladimir Kozlov has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Remove exports from Graal module to jdk.aot
Shenandoah parts look good. I have a few minor comments after reading the rest.
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.
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.
-
Marked as reviewed by shade (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/3398
Re: RFR: 8264484: Replace uses of StringBuffer with StringBuilder in jdk.hotspot.agent [v3]
On Wed, 31 Mar 2021 23:47:41 GMT, Yasumasa Suenaga wrote: >> Andrey Turbanov has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 8264484: Replace uses of StringBuffer with StringBuilder in >> jdk.hotspot.agent >> >> fix indentation > > Looks good! > BTW have you found sponsor? I can sponsor you if you need. @YaSuenag, you can just sponsor without asking, I think. - PR: https://git.openjdk.java.net/jdk/pull/3275
Re: RFR: 8264484: Replace uses of StringBuffer with StringBuilder in jdk.hotspot.agent
On Tue, 30 Mar 2021 19:41:32 GMT, actuallyasmartname wrote: >> Found by IntelliJ IDEA inspection `Java | Java language level migration aids >> | Java 5 | 'StringBuffer' may be 'StringBuilder'` >> As suggested in >> https://github.com/openjdk/jdk/pull/1507#issuecomment-757369003 I've created >> separate PR for module `jdk.hotspot.agent` >> Similar cleanups in the past: >> https://bugs.openjdk.java.net/browse/JDK-8041679 >> https://bugs.openjdk.java.net/browse/JDK-8264029 > > Marked as reviewed by actuallyasmartn...@github.com (no known OpenJDK > username). Created https://bugs.openjdk.java.net/browse/JDK-8264484 for it. - PR: https://git.openjdk.java.net/jdk/pull/3275
Re: RFR: 8264396: Use the blessed modifier order in jdk.internal.jvmstat [v2]
On Tue, 30 Mar 2021 20:37:45 GMT, Alex Blewitt wrote: >> 8264396: Use the blessed modifier order in jdk.internal.jvmstat > > Alex Blewitt has updated the pull request incrementally with one additional > commit since the last revision: > > Updated copyright dates Looks fine to me as well. - Marked as reviewed by shade (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/3252
Integrated: 8264050: Remove unused field VM_HeapWalkOperation::_collecting_heap_roots
On Tue, 23 Mar 2021 15:06:02 GMT, Aleksey Shipilev wrote: > SonarCloud reports field `_collecting_heap_roots` is not initialized after > constructor ends. In fact, that field is not used anywhere. It was like that > since the initial load. We can trivially remove it. This pull request has now been integrated. Changeset: da512bf5 Author: Aleksey Shipilev URL: https://git.openjdk.java.net/jdk/commit/da512bf5 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod 8264050: Remove unused field VM_HeapWalkOperation::_collecting_heap_roots Reviewed-by: coleenp, tschatzl - PR: https://git.openjdk.java.net/jdk/pull/3153
RFR: 8264050: Remove unused field VM_HeapWalkOperation::_collecting_heap_roots
SonarCloud reports field `_collecting_heap_roots` is not initialized after constructor ends. In fact, that field is not used anywhere. It was like that since the initial load. We can trivially remove it. - Commit messages: - 8264050: Remove unused field VM_HeapWalkOperation::_collecting_heap_roots Changes: https://git.openjdk.java.net/jdk/pull/3153/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3153=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8264050 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/3153.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3153/head:pull/3153 PR: https://git.openjdk.java.net/jdk/pull/3153
Re: RFR: 8264028: Typo in javax.management.relation.RelationService::purgeRelations
On Tue, 9 Mar 2021 01:25:10 GMT, 赵延 wrote: >> Hi. Just a reminder for you that the copyright year should be updated to >> 2021. > >> Hi. Just a reminder for you that the copyright year should be updated to >> 2021. > > thanks Hi, I have submitted the bug for it: https://bugs.openjdk.java.net/browse/JDK-8264028 -- please change the PR synopsis to "8264028: Typo in javax.management.relation.RelationService::purgeRelations" to get it properly hooked to PR metadata. After that, bots should tell what is next. - PR: https://git.openjdk.java.net/jdk/pull/2765
Integrated: 8263434: Dangling references after MethodComparator::methods_EMCP
On Thu, 11 Mar 2021 11:02:43 GMT, Aleksey Shipilev wrote: > SonarCloud reports the following problem in MethodComparator::methods_EMCP: > "Address of stack memory associated with local variable 's_new' is still > referred to by the global variable '_s_new' upon returning to the caller. > This will be a dangling reference" > > Code inspection reveals the assignment to static variables is only needed to > pass them to helper methods. So, while this is not a detectable bug (yet), it > is still cleaner not to expose stack variables in globals. > > Additional testing: > - [x] Linux x86_64 fastdebug `tier1` > - [x] Linux x86_64 fastdebug, `vmTestbase_nsk_jvmti` This pull request has now been integrated. Changeset: f9f2eef9 Author:Aleksey Shipilev URL: https://git.openjdk.java.net/jdk/commit/f9f2eef9 Stats: 93 lines in 2 files changed: 4 ins; 7 del; 82 mod 8263434: Dangling references after MethodComparator::methods_EMCP Reviewed-by: coleenp, sspitsyn - PR: https://git.openjdk.java.net/jdk/pull/2937
Re: RFR: 8263434: Dangling references after MethodComparator::methods_EMCP [v3]
> SonarCloud reports the following problem in MethodComparator::methods_EMCP: > "Address of stack memory associated with local variable 's_new' is still > referred to by the global variable '_s_new' upon returning to the caller. > This will be a dangling reference" > > Code inspection reveals the assignment to static variables is only needed to > pass them to helper methods. So, while this is not a detectable bug (yet), it > is still cleaner not to expose stack variables in globals. > > Additional testing: > - [x] Linux x86_64 fastdebug `tier1` > - [x] Linux x86_64 fastdebug, `vmTestbase_nsk_jvmti` 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: - Update copyrights - Drop excess parentheses - Merge branch 'master' into JDK-8263434-dangling-methodcomparator-ecmp - Sprinkling consts - 8263434: Dangling references after MethodComparator::methods_EMCP - Changes: - all: https://git.openjdk.java.net/jdk/pull/2937/files - new: https://git.openjdk.java.net/jdk/pull/2937/files/5acc4807..82bf43e0 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2937=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2937=01-02 Stats: 33632 lines in 1544 files changed: 24507 ins; 4957 del; 4168 mod Patch: https://git.openjdk.java.net/jdk/pull/2937.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2937/head:pull/2937 PR: https://git.openjdk.java.net/jdk/pull/2937
Re: RFR: 8263434: Dangling references after MethodComparator::methods_EMCP [v2]
On Wed, 17 Mar 2021 01:40:56 GMT, Serguei Spitsyn wrote: > One nit: unneeded extra '()' what came from the original code: > ` if ((old_cp->klass_at_noresolve(cpi_old) != > new_cp->klass_at_noresolve(cpi_new)))` Sure, updated. Also updated copyrights. - PR: https://git.openjdk.java.net/jdk/pull/2937
Re: RFR: 8263434: Dangling references after MethodComparator::methods_EMCP [v2]
> SonarCloud reports the following problem in MethodComparator::methods_EMCP: > "Address of stack memory associated with local variable 's_new' is still > referred to by the global variable '_s_new' upon returning to the caller. > This will be a dangling reference" > > Code inspection reveals the assignment to static variables is only needed to > pass them to helper methods. So, while this is not a detectable bug (yet), it > is still cleaner not to expose stack variables in globals. > > Additional testing: > - [x] Linux x86_64 fastdebug `tier1` > - [x] Linux x86_64 fastdebug, `vmTestbase_nsk_jvmti` Aleksey Shipilev has updated the pull request incrementally with one additional commit since the last revision: Sprinkling consts - Changes: - all: https://git.openjdk.java.net/jdk/pull/2937/files - new: https://git.openjdk.java.net/jdk/pull/2937/files/9a43dca1..5acc4807 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2937=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2937=00-01 Stats: 13 lines in 2 files changed: 3 ins; 0 del; 10 mod Patch: https://git.openjdk.java.net/jdk/pull/2937.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2937/head:pull/2937 PR: https://git.openjdk.java.net/jdk/pull/2937
Re: RFR: 8263434: Dangling references after MethodComparator::methods_EMCP [v2]
On Thu, 11 Mar 2021 13:33:39 GMT, Coleen Phillimore wrote:
>> Aleksey Shipilev has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Sprinkling consts
>
> src/hotspot/share/prims/methodComparator.cpp line 67:
>
>> 65: bool MethodComparator::args_same(Bytecodes::Code c_old, Bytecodes::Code
>> c_new,
>> 66: BytecodeStream* s_old, BytecodeStream*
>> s_new,
>> 67: ConstantPool* old_cp, ConstantPool*
>> new_cp) {
>
> Can these be const pointers too?
Yes, they can. Sprinkled.
> src/hotspot/share/prims/methodComparator.cpp line 262:
>
>> 260: }
>> 261:
>> 262: bool MethodComparator::pool_constants_same(int cpi_old, int cpi_new,
>> ConstantPool* old_cp, ConstantPool* new_cp) {
>
> Can these be const?
Yes, they can. Sprinkled.
-
PR: https://git.openjdk.java.net/jdk/pull/2937
Integrated: 8261925: ProblemList com/sun/jdi/AfterThreadDeathTest.java on Linux
On Wed, 17 Feb 2021 18:46:51 GMT, Aleksey Shipilev wrote: > I am seeing exactly the same error in GitHub Actions on x86_32, as in > [JDK-8232839](https://bugs.openjdk.java.net/browse/JDK-8232839). > [JDK-8258832](JDK-8258832) put it on problemlist for linux-x64, let's elevate > that to linux-all. > > Additional testing: > - [x] Test on Linux x86_64 (still skipped) > - [x] Test on Linux x86_32 (now skipped) This pull request has now been integrated. Changeset: b695c7ee Author:Aleksey Shipilev URL: https://git.openjdk.java.net/jdk/commit/b695c7ee Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8261925: ProblemList com/sun/jdi/AfterThreadDeathTest.java on Linux Reviewed-by: dcubed - PR: https://git.openjdk.java.net/jdk/pull/2614
RFR: 8261925: ProblemList com/sun/jdi/AfterThreadDeathTest.java on Linux
I am seeing exactly the same error in GitHub Actions on x86_32, as in [JDK-8232839](https://bugs.openjdk.java.net/browse/JDK-8232839). [JDK-8258832](JDK-8258832) put it on problemlist for linux-x64, let's elevate that to linux-all. Additional testing: - [x] Test on Linux x86_64 (still skipped) - [x] Test on Linux x86_32 (now skipped) - Commit messages: - Problemlist Changes: https://git.openjdk.java.net/jdk/pull/2614/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=2614=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8261925 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/2614.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2614/head:pull/2614 PR: https://git.openjdk.java.net/jdk/pull/2614
Re: RFR: 8251462: Remove legacy compilation policy [v5]
On Wed, 27 Jan 2021 16:43:28 GMT, Igor Veresov wrote: >> I asked Eric to run the benchmarks. The results should be ready on Wednesday. > > The benchmarking is done. No regressions. There are mild improvements in > startup benchmarks on the order of 1-5% (I suspect because of the compilation > policy devirtualization). > > @dholmes-ora, do still plan to look at this? It's been a few weeks, I'd like > to push this. Please update from current master to get x86_32 built and tested in GH Actions. - PR: https://git.openjdk.java.net/jdk/pull/1985
Re: RFR: 8258836: JNI local refs exceed capacity getDiagnosticCommandInfo [v4]
On Wed, 20 Jan 2021 14:34:15 GMT, Severin Gehwolf wrote:
>> This patch adds some explicit capacity for local refs. New regression test
>> fails prior and passes after the patch.
>>
>> Thoughts?
>
> Severin Gehwolf 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 six additional
> commits since the last revision:
>
> - Merge branch 'master' into JDK-8258836-check-jni-mbeanserver
> - Actually assign the variable returned from PopLocalFrame
> - Merge test files into one
> - Adress review feedback from dholmes
> - Merge branch 'master' into JDK-8258836-check-jni-mbeanserver
> - 8258836: JNI local refs exceed capacity getDiagnosticCommandInfo
The test looks good. You might want to say `/reviewers 2` to keep bots from
assuming this approval is enough.
test/jdk/com/sun/management/DiagnosticCommandMBean/DcmdMBeanTestCheckJni.java
line 60:
> 58: class DcmdMBeanRunner {
> 59:
> 60: private static String HOTSPOT_DIAGNOSTIC_MXBEAN_NAME =
Excess newline and the filed could be `static final`. Actually, you might even
go and merge both classes, and then pass a fake java arg in `main` to
disambiguate the "driver" and "test" code, like:
public static void main(String[] args) {
if (args.length == 0) {
driver();
} else {
runner();
}
}
Your call.
-
Marked as reviewed by shade (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/2130
Re: RFR: 8258836: JNI local refs exceed capacity getDiagnosticCommandInfo
On Mon, 18 Jan 2021 14:10:56 GMT, Severin Gehwolf wrote:
> This patch adds some explicit capacity for local refs. New regression test
> fails prior and passes after the patch.
>
> Thoughts?
test/jdk/com/sun/management/DiagnosticCommandMBean/DcmdMBeanTestCheckJni.java
line 38:
> 36: public class DcmdMBeanTestCheckJni {
> 37:
> 38: public static void main(String[] args) throws Exception {
I thought that handling the whole test in one file is the good style. See for
example `
test/hotspot/jtreg/gc/stress/TestStressG1Uncommit.java`.
-
PR: https://git.openjdk.java.net/jdk/pull/2130
Re: RFR: 8259627: Potential memory leaks in JVMTI after JDK-8227745
On Wed, 13 Jan 2021 08:19:59 GMT, Aleksey Shipilev wrote: >> This change eliminates memory leaks in the JVMTI implementation reported by >> SonarCloud. >> >> The leaks occur when the Java heap is exhausted. > > This looks good to me, thanks! > @shipilev thanks for doing the analysis and reporting the issues. I found 2 > leaks. Do you see more that could be related to JDK-8227745 > ([40f847e](https://github.com/openjdk/jdk/commit/40f847e2))? > > I wanted to do a SonarCloud scan myself but was uncertain about the requested > permissions, me using the service for work, etc. Would it be possible to > publish the SC report? I think those are only two instances. I am only aware of [this one](https://sonarcloud.io/project/issues?fileUuids=AXaE0amt8L9hkQskFrwX=jdk=false=BUG). I meant to set up my own some time later. - PR: https://git.openjdk.java.net/jdk/pull/2055
Re: RFR: 8259627: Potential memory leaks in JVMTI after JDK-8227745
On Tue, 12 Jan 2021 19:09:43 GMT, Richard Reingruber wrote: > This change eliminates memory leaks in the JVMTI implementation reported by > SonarCloud. > > The leaks occur when the Java heap is exhausted. This looks good to me, thanks! - Marked as reviewed by shade (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2055
Integrated: 8259451: Zero: skip serviceability/sa tests, set vm.hasSA to false
On Fri, 8 Jan 2021 09:02:46 GMT, Aleksey Shipilev wrote: > Zero does not build SA (see make/autoconf/jdk-options.m4), yet the > serviceability/sa tests do not know about this, because vm.hasSA is still > "true" for Zero. This makes Serviceability test fail for no good reason. The > tests should be skipped for Zero. > > Additional testing: > - [x] Linux x86_64 Zero `serviceability/sa` tests (now skipped) > - [x] Linux x86_64 Server `serviceability/sa` tests (still run) This pull request has now been integrated. Changeset: 3974fd4f Author:Aleksey Shipilev URL: https://git.openjdk.java.net/jdk/commit/3974fd4f Stats: 3 lines in 1 file changed: 3 ins; 0 del; 0 mod 8259451: Zero: skip serviceability/sa tests, set vm.hasSA to false Reviewed-by: sgehwolf, cjplummer - PR: https://git.openjdk.java.net/jdk/pull/1999
RFR: 8259451: Zero: skip serviceability/sa tests, set vm.hasSA to false
Zero does not build SA (see make/autoconf/jdk-options.m4), yet the serviceability/sa tests do not know about this, because vm.hasSA is still "true" for Zero. This makes Serviceability test fail for no good reason. The tests should be skipped for Zero. Additional testing: - [x] Linux x86_64 Zero `serviceability/sa` tests (now skipped) - [x] Linux x86_64 Server `serviceability/sa` tests (still run) - Commit messages: - 8259451: Zero: skip serviceability/sa tests, set vm.hasSA to false Changes: https://git.openjdk.java.net/jdk/pull/1999/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1999=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8259451 Stats: 3 lines in 1 file changed: 3 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/1999.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1999/head:pull/1999 PR: https://git.openjdk.java.net/jdk/pull/1999
Re: RFR: 8256167: Convert JDK use of `Reference::get` to `Reference::refersTo`
On Thu, 3 Dec 2020 22:54:54 GMT, Mandy Chung wrote: > This patch replaces some uses of `Reference::get` to `Reference::refersTo` to > avoid keeping a referent strongly reachable that could cause unnecessary > delay in collecting such object. I only made change in some but not all > classes in core libraries when working with Kim on `Reference::refersTo`. > The remaining uses are left for the component owners to convert at > appropriate time. Replacements look fine to me. src/java.base/share/classes/java/util/WeakHashMap.java line 291: > 289: if (e.refersTo(key)) return true; > 290: > 291: // then checks for equality Obnoxiously minor nit: plurality is inconsistent. `check if the given entry...` above, and `then check[s] for equality`. Should be `...then check for equality`? - Marked as reviewed by shade (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1609
Re: RFR: 8257708: Remove redundant unmodifiableSet wrapper from already immutable set returned by Collections.singleton
On Sun, 29 Nov 2020 18:03:24 GMT, Andrey Turbanov wrote: > 8257708: Remove redundant unmodifiableSet wrapper from already immutable set > returned by Collections.singleton Looks good now! - Marked as reviewed by shade (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1505
Re: RFR: 8257708: Remove redundant unmodifiableSet wrapper from already immutable set returned by Collections.singleton
On Sun, 29 Nov 2020 18:03:24 GMT, Turbanov Andrey
wrote:
> 8257708: Remove redundant unmodifiableSet wrapper from already immutable set
> returned by Collections.singleton
Changes requested by shade (Reviewer).
Submitted -- https://bugs.openjdk.java.net/browse/JDK-8257708, rename this PR
to "8257708: Remove redundant unmodifiableSet wrapper from already immutable
set returned by Collections.singleton" to get it hooked up.
src/java.management/share/classes/java/lang/management/DefaultPlatformMBeanProvider.java
line 60:
> 58: private final Set classLoadingInterfaceNames =
> 59: Collections.singleton(
> 60: "java.lang.management.ClassLoadingMXBean");
Here and later, I think we can avoid awkward newlines:
`Collections.singleton("java.lang...")`
-
PR: https://git.openjdk.java.net/jdk/pull/1505
Re: RFR: 8253916: ResourceExhausted/resexhausted001 crashes on Linux-x64
On Tue, 1 Dec 2020 12:55:05 GMT, Coleen Phillimore wrote: > Give an error message rather than logging the error and then crashing later > because the JVM can't detect stack overflow. In a resource exhausted > situation, thread creation is also failing. This is the > vm_exit_out_of_memory message printed: > > `$ java -XX:+UseNewCode -version > [0.003s][warning][os,thread] Attempt to protect stack guard pages failed > (0x7f606b249000-0x7f606b24d000). > > There is insufficient memory for the Java Runtime Environment to continue. > Native memory allocation (mprotect) failed to protect 16384 bytes for memory > to guard stack pages > An error report file with more information is saved as: > /16mprotect/hs_err_pid30596.log` > ` You need to merge from master to get GH Actions fixes. They are currently failing with outdated dependencies in your branch, and master has a fix for that. - PR: https://git.openjdk.java.net/jdk/pull/1540
Re: RFR: 8256808: com/sun/jdi/CatchAllTest.java failed with "NullPointerException: Cannot invoke "lib.jdb.Jdb.log(String)" because "this.jdb" is null"
On Wed, 25 Nov 2020 21:36:12 GMT, Alex Menkov wrote:
> JdbTest can get exception before jdb field is initialized.
> As Jdb logging does not depend on the instance, made Jdb.log method static
test/jdk/com/sun/jdi/lib/jdb/JdbTest.java line 99:
> 97: Jdb.log("===");
> 98: Jdb.log("Exception thrown during test execution: " +
> e.getMessage());
> 99: Jdb.log("===");
Why not just print it to `System.out` then, eliminating the method altogether?
-
PR: https://git.openjdk.java.net/jdk/pull/1443
Re: RFR: 8250888: nsk/jvmti/scenarios/general_functions/GF08/gf08t001/TestDriver.java fails [v2]
On Fri, 20 Nov 2020 06:30:31 GMT, Guoxiong Li wrote: >> Hi Guoxiong, >> It looks good to me. >> Thanks, >> Serguei > > I run the test locally using the following command before I submit this patch. > make test > TEST="jtreg:hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/general_functions/GF08/gf08t001/TestDriver.java" > TEST_VM_OPTS=-XX:+UseShenandoahGC > This test passes which misleads me that my patch is right. > > In any case, I apologize for this mistake and will try my best to do more > testing before submitting patches in the future. > Thank you very much for taking the time to review. > > Best Regards. I tested this patch with Serial, Parallel, G1, Shenandoah, Z and it passes. - PR: https://git.openjdk.java.net/jdk/pull/1319
Re: RFR: 8250888: nsk/jvmti/scenarios/general_functions/GF08/gf08t001/TestDriver.java fails
On Thu, 19 Nov 2020 14:40:45 GMT, Guoxiong Li
wrote:
> Hi all,
>
> `TestDriver.java` used `sun.hotspot.WhiteBox.getBooleanVMFlag("Use*GC")`
> which could return null.
> This patch uses `sun.hotspot.gc.GC` instead of `sun.hotspot.WhiteBox` to
> avoid the NullPointerException.
> Thank you for taking the time to review.
>
> Best Regards.
I believe it should be `isSelected()` rather than `isSupported()`. Try to run
the test with different GCs like this to verify:
make run-test
TEST=nsk/jvmti/scenarios/general_functions/GF08/gf08t001/TestDriver.java
TEST_VM_OPTS=-XX:+UseShenandoahGC
-
Changes requested by shade (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/1319
Integrated: 8253525: Implement getInstanceSize/sizeOf intrinsics
On Wed, 14 Oct 2020 10:11:23 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 This pull request has now been integrated. Changeset: b4d01867 Author:Aleksey Shipilev URL: https://git.openjdk.java.net/jdk/commit/b4d01867 Stats: 655 lines in 12 files changed: 655 ins; 0 del; 0 mod 8253525: Implement getInstanceSize/sizeOf intrinsics Reviewed-by: kvn, sspitsyn - PR: https://git.openjdk.java.net/jdk/pull/650
Re: RFR: 8253525: Implement getInstanceSize/sizeOf intrinsics [v5]
On Wed, 21 Oct 2020 17:33:27 GMT, Vladimir Kozlov wrote: >> 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. Thanks @vnkozlov and @sspitsyn! - PR: https://git.openjdk.java.net/jdk/pull/650
Re: RFR: 8253525: Implement getInstanceSize/sizeOf intrinsics [v6]
On Tue, 10 Nov 2020 20:13:41 GMT, Serguei Spitsyn wrote: > One more nit, I forgot to list in my previous comment, is uneeded '()' around > comparisons: > `+ static final int REF_SIZE = ((compressedOops == null) || (compressedOops > == true)) ? 4 : 8;` Right. Fixed that. Thanks! - PR: https://git.openjdk.java.net/jdk/pull/650
Re: RFR: 8255982: Extend BasicJMapTest to test with different GC Heap [v3]
On Wed, 11 Nov 2020 10:48:12 GMT, Lin Zang wrote: >> The implementation of jmap tool depends on the implementation of object >> iteration by different GC heap. >> This patch extend the BasicJMapTest to cover differet GC Heap. > > Lin Zang has updated the pull request incrementally with one additional > commit since the last revision: > > Refine the test configuration. I think it is fine to omit Epsilon from this testing. There is another trick you can use: `@test id=serial` would name the tests appropriately, not just "id#": Running test 'jtreg:test/jdk/sun/tools/jmap/BasicJMapTest.java' Passed: sun/tools/jmap/BasicJMapTest.java#parallel Passed: sun/tools/jmap/BasicJMapTest.java#serial Passed: sun/tools/jmap/BasicJMapTest.java#z Passed: sun/tools/jmap/BasicJMapTest.java#g1 Passed: sun/tools/jmap/BasicJMapTest.java#shenandoah Test results: passed: 5 I also tested this patch with x86_32 that implicitly disables Z, and it passes. Also passes with TEST_VM_OPTS="-XX:+UseSerialGC", with only one config automatically selected. @iignatev might want to ack this, as our resident test overseer. - Marked as reviewed by shade (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1094
Re: RFR: 8253525: Implement getInstanceSize/sizeOf intrinsics [v7]
> 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: Drop parentheses around comparisons - Changes: - all: https://git.openjdk.java.net/jdk/pull/650/files - new: https://git.openjdk.java.net/jdk/pull/650/files/1b7290a3..89881e9f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=650=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk=650=05-06 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/650.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/650/head:pull/650 PR: https://git.openjdk.java.net/jdk/pull/650
Re: RFR: 8255982: Extend BasicJMapTest to test with different GC Heap
On Fri, 6 Nov 2020 12:54:28 GMT, Lin Zang wrote: > The implementation of jmap tool depends on the implementation of object > iteration by different GC heap. > This patch extend the BasicJMapTest to cover differet GC Heap. I believe this would fail when some GCs are not available. For example, in Minimal/Zero only Serial and Parallel are available. ZGC and Shenandoah are not available on all platforms. Plus, specifying another GC with `TEST_VM_OPTS` would probably fail with "multiple GCs selected". You need to split the tests like this, and protect each config with `@requires`: /* * @test * @summary Unit test for jmap utility * @key intermittent * @requires vm.gc.Parallel * @library /test/lib * @build jdk.test.lib.hprof.* * @build jdk.test.lib.hprof.model.* * @build jdk.test.lib.hprof.parser.* * @build jdk.test.lib.hprof.util.* * @run main/othervm/timeout=240 -XX:+UseParallelGC BasicJMapTest */ /* * @test * @summary Unit test for jmap utility * @key intermittent * @requires vm.gc.G1 * @library /test/lib * @build jdk.test.lib.hprof.* * @build jdk.test.lib.hprof.model.* * @build jdk.test.lib.hprof.parser.* * @build jdk.test.lib.hprof.util.* * @run main/othervm/timeout=240 -XX:+UseG1GC BasicJMapTest */ ... Maybe there is a way to clean up multiple `@build` tags to make the test config less verbose. - Changes requested by shade (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1094
Re: RFR: 8253525: Implement getInstanceSize/sizeOf intrinsics [v6]
> 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 eight additional commits since the last revision: - Trim down the iteration count, and use -Xbatch to wait for compilation - Use proper constant names in the test - Merge branch 'master' into JDK-8253525-sizeof-intrinsics - 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 - Changes: - all: https://git.openjdk.java.net/jdk/pull/650/files - new: https://git.openjdk.java.net/jdk/pull/650/files/482c2f24..1b7290a3 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=650=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk=650=04-05 Stats: 138167 lines in 1930 files changed: 82950 ins; 42900 del; 12317 mod Patch: https://git.openjdk.java.net/jdk/pull/650.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/650/head:pull/650 PR: https://git.openjdk.java.net/jdk/pull/650
Re: RFR: 8253525: Implement getInstanceSize/sizeOf intrinsics [v5]
On Tue, 10 Nov 2020 11:01:54 GMT, Serguei Spitsyn wrote: > I have a question and a couple of minor suggestions on new test. > Q: Why the value of ITERS is that big? What is the need to have this number > of iterations? The test verifies the answer does not change if we hit JIT compilers in that code. Since we are doing C1/C2 intrinsics, we need to execute the loops more than compilation-threshold times. Since the current threshold is about 100K, doing 1M iterations seems good: it is well past the compilation threshold times, and there is time to re-enter the newly compiled method. The test run time is still sane, 1 minute on my Linux x86_64 fastdebug. I can do 200K iterations and -Xbatch instead, though, see new change. This drops the test run time to 30 seconds. > Also, I do not like one-letter identifiers, especially if they are not local. > Could you, please, replace identifiers R and A with some short versions that > give a hint. > Something like REFSIZE and ALIGNMENT would be good enough. Renamed to `REF_SIZE` and `OBJ_ALIGN` instead. > Also, what tests did you run to make sure no regression is introduced? Old code calls into `oop::size()` to get the object size. That method decodes the object's layout helper. So when we replace it with intrinsic, we now have to test the different shapes of the layout helper and varying conditions for that decoding. So the new test tries to cover the comprehensive matrix: - the usual object shapes: objects, primitive arrays, object arrays; - different compressed oops modes that affect reference sizes; - different object alignment modes that affect object sizes; - different compilation modes: interpreter, C1, C2; - special paths like carrying special bits in layout helper for allocation slow-paths; I know that test is sensitive to compiler intrinsics bugs, as I used these tests to develop the intrinsics. If you inject simple off-by-one bugs into current C1/C2 intrinsics, new test catches that. The additional safety comes from the somewhat loose API requirement: it is specified to return the guess, and that guess might as well be wrong (not overly wrong though, for a quality implementation). - PR: https://git.openjdk.java.net/jdk/pull/650
Re: RFR: 8253525: Implement getInstanceSize/sizeOf intrinsics [v5]
On Wed, 21 Oct 2020 17:37:20 GMT, Aleksey Shipilev wrote: >> Good. > > Thanks for review, @kvn! I would also like a review from someone from > serviceability. Friendly reminder. - PR: https://git.openjdk.java.net/jdk/pull/650
Re: RFR: 8254270: linux 32 bit build doesn't compile libjdwp/log_messages.c
On Thu, 5 Nov 2020 02:11:23 GMT, Chris Plummer wrote: >> Apply patch suggested by @cl4es in the bug report. Passes >> linux-x86-open,linux-x64-open,linux-s390x-open,linux-arm32-debug,linux-ppc64le-debug >> builds with this patch, and tier1. >> >> thanks, >> Coleen > >> Where did the magic numbers in >> >> "%.19s.%.3d %.50s" >> >> come from? > > The first argument is a char[DT_SIZE], which is MAXLEN_DT+1, which is 20. The > +1 is probably for a null. The 3rd argument is char[TZ_SIZE], which is > TIMESTAMP_SIZE - MAXLEN_DT - MAXLEN_MS, which is 81 -19 - 5, which is 56. The > target buffer is char[MAXLEN_TIMESTAMP+1], which is 81 (and so is ltbuf). So > without any of Coleen's changes we are writing at most 19 + 3 + 1 + 56 + 1 > bytes, which is 80 bytes, into an 81 byte buffer. The compiler should not be > complaining here. > > And just to clarify, the compiler DOES see the size of the destination > buffer. It's looking at the source of the argument passed in. However, it > seems to have computed some lengths incorrectly and came to the conclusion > that the buffer might not be big enough to handle the known sizes of all the > snprintf arguments. Please make sure `linux-x86` jobs pass in GH actions. I think you can navigate to https://github.com/coleenp/jdk/runs/1355854648 -- and press "Re-run jobs" at top right. - PR: https://git.openjdk.java.net/jdk/pull/1067
Integrated: 8255810: Zero: build fails without JVMTI
On Tue, 3 Nov 2020 11:22:50 GMT, Aleksey Shipilev wrote: > Zero interpreter has two modes: with JVMTI built-in, and without. But we > cannot test it properly, because the build fails without JVMTI in shared > code. Mostly due to JDK-8147388. > > Additional testing: > - [x] Linux x86_64 Zero fastdebug builds `--with-jvm-features=-jvmti` This pull request has now been integrated. Changeset: ca216bae Author: Aleksey Shipilev URL: https://git.openjdk.java.net/jdk/commit/ca216bae Stats: 4 lines in 2 files changed: 2 ins; 0 del; 2 mod 8255810: Zero: build fails without JVMTI Reviewed-by: coleenp - PR: https://git.openjdk.java.net/jdk/pull/1036
Re: RFR: 8255810: Zero: build fails without JVMTI
On Tue, 3 Nov 2020 14:43:52 GMT, Coleen Phillimore wrote: >> Thanks @coleenp! Would you say it is trivial? > > Yes, trivial! Thanks! - PR: https://git.openjdk.java.net/jdk/pull/1036
Re: RFR: 8255810: Zero: build fails without JVMTI
On Tue, 3 Nov 2020 13:04:08 GMT, Coleen Phillimore wrote: >> Zero interpreter has two modes: with JVMTI built-in, and without. But we >> cannot test it properly, because the build fails without JVMTI in shared >> code. Mostly due to JDK-8147388. >> >> Additional testing: >> - [x] Linux x86_64 Zero fastdebug builds `--with-jvm-features=-jvmti` > > Looks good. Thanks @coleenp! Would you say it is trivial? - PR: https://git.openjdk.java.net/jdk/pull/1036
