Re: Integrated: 8284015: ProblemList containers/docker/TestJcmd.java on linux-x64
On Wed, 30 Mar 2022 15:09:08 GMT, Daniel D. Daugherty wrote: > A trivial fix to ProblemList containers/docker/TestJcmd.java on linux-x64. LGTM and is trivial. - Marked as reviewed by hseigel (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8042
Re: RFR: 8278423: ExtendedDTraceProbes should be deprecated [v7]
On Wed, 9 Feb 2022 16:20:49 GMT, Emanuel Peter wrote: >> Deprecated ExtendedDTraceProbes. >> Edited help messages and man pages accordingly, added the 3 flags to man >> pages. >> Added flag to VMDeprecatedOptions test. >> Replaced the flag with 3 flags in SDTProbesGNULinuxTest.java. >> >> Checked that tests are not affected. > > Emanuel Peter has updated the pull request incrementally with one additional > commit since the last revision: > > updated warning messages and added 3 flags to man-pages Other than the need to update the copyright dates to 2022, these changes look good. Thanks, Harold - Marked as reviewed by hseigel (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7110
Re: RFR: 8280543: Update the "java" and "jcmd" tool specification for CDS
On Fri, 28 Jan 2022 01:53:09 GMT, Ioi Lam wrote: > The discussion of CDS in the man pages need to be cleaned up and updated to > match the latest functionalities and intended usage. > > For java.md: > > - Reorganized the flow of the doc: Overview -> How to use -> How to create -> > Restrictions and notes. I think this will be easier to read. > - Added information about `jcmd VM.cds static_dump|dynamic_dump` > - Removed a few sections that are no longer relevant or are uncommon usage > (config files, sharing across two different apps) > - Removed discussion of uncommon error conditions (such as array classes) > - Removed detailed error messages. I think a simple note like "unsupported" > will be good enough for readers of the man page. > - Removed discussion of different version of JDK (these should have been in > release note) > > For jcmd.md: > > - Added some more details about default file name and output directory. > > For ease of reviewing, please see the pre-rendered HTML pages: > > http://cr.openjdk.java.net/~iklam/jdk19/8280543-update-java-and-jcmd-manpage-for-cds2/specs/man/ LGTM. The copyright in jcmd.1 needs updating. Thanks, Harold - Marked as reviewed by hseigel (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7255
Integrated: 8225093: Special property jdk.boot.class.path.append should not default to empty string
On Thu, 16 Dec 2021 17:33:29 GMT, Harold Seigel wrote: > Please review this fix for JDK-8225093 to set the default value of property > jdk.boot.class.path.append to NULL instead of "". This fix was tested by > running Mach5 tiers 1-2 on Linux, Mac OS, and Windows, and Mach5 tiers 3-5 on > Linux x64. The fix was also tested by running the agent.c example in > JDK-8224791. > > Thanks! Harold This pull request has now been integrated. Changeset: c08b2ac3 Author:Harold Seigel URL: https://git.openjdk.java.net/jdk/commit/c08b2ac34c436f07f7d43f25ce16c94a137597f5 Stats: 118 lines in 4 files changed: 116 ins; 0 del; 2 mod 8225093: Special property jdk.boot.class.path.append should not default to empty string Reviewed-by: dholmes, sspitsyn, alanb - PR: https://git.openjdk.java.net/jdk/pull/6868
Re: RFR: 8225093: Special property jdk.boot.class.path.append should not default to empty string [v3]
On Mon, 20 Dec 2021 18:36:45 GMT, Harold Seigel wrote: >> Please review this fix for JDK-8225093 to set the default value of property >> jdk.boot.class.path.append to NULL instead of "". This fix was tested by >> running Mach5 tiers 1-2 on Linux, Mac OS, and Windows, and Mach5 tiers 3-5 >> on Linux x64. The fix was also tested by running the agent.c example in >> JDK-8224791. >> >> Thanks! Harold > > Harold Seigel has updated the pull request incrementally with one additional > commit since the last revision: > > Add error code to test failure message Thanks Serguei, David, and Alan for the reviews! - PR: https://git.openjdk.java.net/jdk/pull/6868
Re: RFR: 8225093: Special property jdk.boot.class.path.append should not default to empty string [v3]
On Mon, 20 Dec 2021 18:36:45 GMT, Harold Seigel wrote: >> Please review this fix for JDK-8225093 to set the default value of property >> jdk.boot.class.path.append to NULL instead of "". This fix was tested by >> running Mach5 tiers 1-2 on Linux, Mac OS, and Windows, and Mach5 tiers 3-5 >> on Linux x64. The fix was also tested by running the agent.c example in >> JDK-8224791. >> >> Thanks! Harold > > Harold Seigel has updated the pull request incrementally with one additional > commit since the last revision: > > Add error code to test failure message CSR was approved. - PR: https://git.openjdk.java.net/jdk/pull/6868
Re: RFR: 8225093: Special property jdk.boot.class.path.append should not default to empty string [v2]
On Mon, 20 Dec 2021 17:33:58 GMT, Alan Bateman wrote:
>> Harold Seigel has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> fix test
>
> test/hotspot/jtreg/runtime/BootClassAppendProp/libGetBootClassPathAppendProp.c
> line 53:
>
>> 51: if (err != JVMTI_ERROR_NONE) {
>> 52: return (*env)->NewStringUTF(env, "wrong error code");
>> 53: }
>
> For diagnosability it may be helpful to have this native method throw an
> exception with the JVMTI error code or alternatively have the return String
> include the error code rather than a generic message "wrong error code".
Thanks Alan! I updated the test to include the JVM TI error code in the above
'wrong error code' message.
-
PR: https://git.openjdk.java.net/jdk/pull/6868
Re: RFR: 8225093: Special property jdk.boot.class.path.append should not default to empty string [v3]
> Please review this fix for JDK-8225093 to set the default value of property > jdk.boot.class.path.append to NULL instead of "". This fix was tested by > running Mach5 tiers 1-2 on Linux, Mac OS, and Windows, and Mach5 tiers 3-5 on > Linux x64. The fix was also tested by running the agent.c example in > JDK-8224791. > > Thanks! Harold Harold Seigel has updated the pull request incrementally with one additional commit since the last revision: Add error code to test failure message - Changes: - all: https://git.openjdk.java.net/jdk/pull/6868/files - new: https://git.openjdk.java.net/jdk/pull/6868/files/a1c9218b..97d60c7e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6868=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6868=01-02 Stats: 3 lines in 1 file changed: 2 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/6868.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6868/head:pull/6868 PR: https://git.openjdk.java.net/jdk/pull/6868
Re: RFR: 8225093: Special property jdk.boot.class.path.append should not default to empty string [v2]
On Fri, 17 Dec 2021 02:18:55 GMT, David Holmes wrote:
>> Harold Seigel has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> fix test
>
> test/hotspot/jtreg/runtime/BootClassAppendProp/GetBootClassPathAppendProp.java
> line 40:
>
>> 38:
>> 39: public static void main(String[] args) throws Exception {
>> 40: String vm_info_jvmti = getSystemProperty();
>
> Nit: strange name and doesn't follow Java style rules. Won't "path" suffice?
The strange name comes from the test that I copied to write this one. I'll
change it to 'path'.
-
PR: https://git.openjdk.java.net/jdk/pull/6868
Re: RFR: 8225093: Special property jdk.boot.class.path.append should not default to empty string [v2]
On Fri, 17 Dec 2021 03:17:23 GMT, Serguei Spitsyn wrote: >> Harold Seigel has updated the pull request incrementally with one additional >> commit since the last revision: >> >> fix test > > test/hotspot/jtreg/runtime/BootClassAppendProp/GetBootClassPathAppendProp.java > line 26: > >> 24: /** >> 25: * @test >> 26: * @bug 8224791 > > The 8224791 has been closed. Should it be replaced with 8225093? Yes! Thanks for pointing that out. - PR: https://git.openjdk.java.net/jdk/pull/6868
Re: RFR: 8225093: Special property jdk.boot.class.path.append should not default to empty string
On Thu, 16 Dec 2021 17:33:29 GMT, Harold Seigel wrote: > Please review this fix for JDK-8225093 to set the default value of property > jdk.boot.class.path.append to NULL instead of "". This fix was tested by > running Mach5 tiers 1-2 on Linux, Mac OS, and Windows, and Mach5 tiers 3-5 on > Linux x64. The fix was also tested by running the agent.c example in > JDK-8224791. > > Thanks! Harold Thanks David and Serguei for reviewing this, and thanks David for reviewing the CSR. Harold - PR: https://git.openjdk.java.net/jdk/pull/6868
Re: RFR: 8225093: Special property jdk.boot.class.path.append should not default to empty string [v2]
> Please review this fix for JDK-8225093 to set the default value of property > jdk.boot.class.path.append to NULL instead of "". This fix was tested by > running Mach5 tiers 1-2 on Linux, Mac OS, and Windows, and Mach5 tiers 3-5 on > Linux x64. The fix was also tested by running the agent.c example in > JDK-8224791. > > Thanks! Harold Harold Seigel has updated the pull request incrementally with one additional commit since the last revision: fix test - Changes: - all: https://git.openjdk.java.net/jdk/pull/6868/files - new: https://git.openjdk.java.net/jdk/pull/6868/files/bd80b91b..a1c9218b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6868=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6868=00-01 Stats: 6 lines in 1 file changed: 0 ins; 0 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/6868.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6868/head:pull/6868 PR: https://git.openjdk.java.net/jdk/pull/6868
RFR: 8225093: Special property jdk.boot.class.path.append should not default to empty string
Please review this fix for JDK-8225093 to set the default value of property jdk.boot.class.path.append to NULL instead of "". This fix was tested by running Mach5 tiers 1-2 on Linux, Mac OS, and Windows, and Mach5 tiers 3-5 on Linux x64. The fix was also tested by running the agent.c example in JDK-8224791. Thanks! Harold - Commit messages: - 8225093: Special property jdk.boot.class.path.append should not default to empty string Changes: https://git.openjdk.java.net/jdk/pull/6868/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=6868=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8225093 Stats: 116 lines in 4 files changed: 114 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/6868.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6868/head:pull/6868 PR: https://git.openjdk.java.net/jdk/pull/6868
Re: RFR: 8202579: Revisit VM_Version and VM_Version_ext for overlap and consolidation [v2]
On Tue, 14 Dec 2021 17:42:00 GMT, Coleen Phillimore wrote: >> This change makes VM_Version_Ext part of VM_Version (the platform dependent >> part) and moves some duplicated code. x86 had the most code in >> VM_Version_Ext, so the most code moved there. There might be some unneeded >> functions but I didn't want to remove them with this change. >> >> Tier1 (tier2-4 testing in progress) on linux and windows for x86, aarch64, >> Oracle platforms and tested builds on: >> linux-aarch64-debug,linux-x86-open,linux-s390x-open,linux-arm32-debug,linux-ppc64le-debug >> and >> linux-x64-zero,linux-x64-zero-debug,linux-x86-zero,linux-x86-zero-debug >> >> Ran JFR tests manually (it uses os_perf* CPUInformationInterface code). > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > Added an initialization assert. Thanks for doing this! Harold - Marked as reviewed by hseigel (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6820
Integrated: 8277481: Obsolete seldom used CDS flags
On Fri, 10 Dec 2021 15:01:29 GMT, Harold Seigel wrote: > Please review this change to obsolete deprecated CDS options UseSharedSpaces, > RequireSharedSpaces, DynamicDumpSharedSpaces, and DumpSharedSpaces. The > change was tested by running Mach5 tiers 1-2 on Linux, Mac OS, and Windows > and Mach5 tiers 3-5 on Linux x64 and Windows x64. > > The use of UseSharedSpaces in ps_core_common.c was tested on Mac OS x64 by > temporarily removing serviceability/sa/ClhsdbPmap.java#core from the problem > list. > > Thanks! Harold This pull request has now been integrated. Changeset: 14f7385a Author:Harold Seigel URL: https://git.openjdk.java.net/jdk/commit/14f7385a72972e1f15b3103cc75a60c5733f6d98 Stats: 152 lines in 13 files changed: 22 ins; 94 del; 36 mod 8277481: Obsolete seldom used CDS flags Reviewed-by: iklam, ccheung, dholmes - PR: https://git.openjdk.java.net/jdk/pull/6800
Re: RFR: 8277481: Obsolete seldom used CDS flags [v2]
On Fri, 10 Dec 2021 19:49:48 GMT, Harold Seigel wrote: >> Please review this change to obsolete deprecated CDS options >> UseSharedSpaces, RequireSharedSpaces, DynamicDumpSharedSpaces, and >> DumpSharedSpaces. The change was tested by running Mach5 tiers 1-2 on >> Linux, Mac OS, and Windows and Mach5 tiers 3-5 on Linux x64 and Windows x64. >> >> The use of UseSharedSpaces in ps_core_common.c was tested on Mac OS x64 by >> temporarily removing serviceability/sa/ClhsdbPmap.java#core from the problem >> list. >> >> Thanks! Harold > > Harold Seigel has updated the pull request incrementally with one additional > commit since the last revision: > > fix print_debug() message Thanks Ioi, Calvin, and David for the reviews! - PR: https://git.openjdk.java.net/jdk/pull/6800
Re: RFR: 8277481: Obsolete seldom used CDS flags [v2]
On Fri, 10 Dec 2021 19:37:31 GMT, Calvin Cheung wrote:
>> Harold Seigel has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> fix print_debug() message
>
> src/jdk.hotspot.agent/share/native/libsaproc/ps_core_common.c line 303:
>
>> 301: useSharedSpacesAddr = lookup_symbol(ph, jvm_name,
>> USE_SHARED_SPACES_SYM);
>> 302: if (useSharedSpacesAddr == 0) {
>> 303: print_debug("can't lookup 'UseSharedSpaces' symbol\n");
>
> Maybe the `print_debug` at line 311 should also be updated from "flag" to
> "symbol"?
fixed. Thanks for pointing it out.
-
PR: https://git.openjdk.java.net/jdk/pull/6800
Re: RFR: 8277481: Obsolete seldom used CDS flags [v2]
> Please review this change to obsolete deprecated CDS options UseSharedSpaces, > RequireSharedSpaces, DynamicDumpSharedSpaces, and DumpSharedSpaces. The > change was tested by running Mach5 tiers 1-2 on Linux, Mac OS, and Windows > and Mach5 tiers 3-5 on Linux x64 and Windows x64. > > The use of UseSharedSpaces in ps_core_common.c was tested on Mac OS x64 by > temporarily removing serviceability/sa/ClhsdbPmap.java#core from the problem > list. > > Thanks! Harold Harold Seigel has updated the pull request incrementally with one additional commit since the last revision: fix print_debug() message - Changes: - all: https://git.openjdk.java.net/jdk/pull/6800/files - new: https://git.openjdk.java.net/jdk/pull/6800/files/3f6c6dee..601a678f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6800=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6800=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/6800.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6800/head:pull/6800 PR: https://git.openjdk.java.net/jdk/pull/6800
RFR: 8277481: Obsolete seldom used CDS flags
Please review this change to obsolete deprecated CDS options UseSharedSpaces, RequireSharedSpaces, DynamicDumpSharedSpaces, and DumpSharedSpaces. The change was tested by running Mach5 tiers 1-2 on Linux, Mac OS, and Windows and Mach5 tiers 3-5 on Linux x64 and Windows x64. The use of UseSharedSpaces in ps_core_common.c was tested on Mac OS x64 by temporarily removing serviceability/sa/ClhsdbPmap.java#core from the problem list. Thanks! Harold - Commit messages: - fix typo - 8277481: Obsolete seldom used CDS flags Changes: https://git.openjdk.java.net/jdk/pull/6800/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=6800=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8277481 Stats: 151 lines in 13 files changed: 22 ins; 94 del; 35 mod Patch: https://git.openjdk.java.net/jdk/pull/6800.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6800/head:pull/6800 PR: https://git.openjdk.java.net/jdk/pull/6800
Re: RFR: 8276173: Clean up and remove unneeded casts in HeapDumper [v2]
On Tue, 2 Nov 2021 19:25:40 GMT, Leo Korinth wrote: >> HeapDumper does a lot of unneeded casts. Some arguments should be const. >> Headers are not correctly sorted. Comment about identifier size on Windows >> and Solaris is not true. >> >> First I cleaned up casting in the "union casting", but then I decided it was >> better to create a temporary bit_cast that we can use until we get the >> proper one in c++20. > > Leo Korinth has updated the pull request incrementally with one additional > commit since the last revision: > > restart failed github tests Looks good! Thanks, Harold - Marked as reviewed by hseigel (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6211
Re: RFR: 8275735: [linux] Remove deprecated Metrics api (kernel memory limit)
On Thu, 28 Oct 2021 13:03:56 GMT, Severin Gehwolf wrote: > Please review this change to remove some API which no longer works as > expected as recent OCI runtimes start to drop support for `--kernel-memory` > switch. See the bug for references. This part of the API is not present in > hotspot code. > > Testing: Container tests (cgroup v1) on Linux x86_64 (all pass) The changes look good. Thanks for doing this. Harold - Marked as reviewed by hseigel (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6156
Integrated: 8272124: Cgroup v1 initialization causes NullPointerException when cgroup path contains colon
On Mon, 16 Aug 2021 17:25:57 GMT, Harold Seigel wrote: > Please review this small fix for JDK-8272124. The fix puts a limit of 3 when > splitting self cgroup lines by ':' so that Cgroup paths won't get truncated > if they contain embedded ':'s. For example, an entry of > "11:memory:/user.sli:ce" in a /proc/self/cgroup file will now result in a > Cgroup path of "/user.sli:ce" instead of "/user.sli". > > The fix was tested with Mach5 tiers 1 and 2, and Mach5 tiers 3-5 on Linux x64 > and Linux aarch64. > > Thanks, Harold This pull request has now been integrated. Changeset: 4d6593ce Author:Harold Seigel URL: https://git.openjdk.java.net/jdk/commit/4d6593ce0243457e7431a5990957a8f880e0a3fb Stats: 57 lines in 2 files changed: 54 ins; 0 del; 3 mod 8272124: Cgroup v1 initialization causes NullPointerException when cgroup path contains colon Reviewed-by: mseledtsov, sgehwolf - PR: https://git.openjdk.java.net/jdk/pull/5127
Re: RFR: 8272124: Cgroup v1 initialization causes NullPointerException when cgroup path contains colon [v4]
On Wed, 18 Aug 2021 13:04:46 GMT, Harold Seigel wrote: >> Please review this small fix for JDK-8272124. The fix puts a limit of 3 >> when splitting self cgroup lines by ':' so that Cgroup paths won't get >> truncated if they contain embedded ':'s. For example, an entry of >> "11:memory:/user.sli:ce" in a /proc/self/cgroup file will now result in a >> Cgroup path of "/user.sli:ce" instead of "/user.sli". >> >> The fix was tested with Mach5 tiers 1 and 2, and Mach5 tiers 3-5 on Linux >> x64 and Linux aarch64. >> >> Thanks, Harold > > Harold Seigel has updated the pull request incrementally with one additional > commit since the last revision: > > Fix test typo and error Thanks Severin and Misha for your reviews and improvements for this fix! - PR: https://git.openjdk.java.net/jdk/pull/5127
Re: RFR: 8272124: Cgroup v1 initialization causes NullPointerException when cgroup path contains colon [v3]
On Wed, 18 Aug 2021 12:25:45 GMT, Harold Seigel wrote: >> Please review this small fix for JDK-8272124. The fix puts a limit of 3 >> when splitting self cgroup lines by ':' so that Cgroup paths won't get >> truncated if they contain embedded ':'s. For example, an entry of >> "11:memory:/user.sli:ce" in a /proc/self/cgroup file will now result in a >> Cgroup path of "/user.sli:ce" instead of "/user.sli". >> >> The fix was tested with Mach5 tiers 1 and 2, and Mach5 tiers 3-5 on Linux >> x64 and Linux aarch64. >> >> Thanks, Harold > > Harold Seigel has updated the pull request incrementally with one additional > commit since the last revision: > > add mountinfo containing colongs to test Thanks for finding those issues. Please review the latest iteration of this fix. Thanks, Harold - PR: https://git.openjdk.java.net/jdk/pull/5127
Re: RFR: 8272124: Cgroup v1 initialization causes NullPointerException when cgroup path contains colon [v4]
> Please review this small fix for JDK-8272124. The fix puts a limit of 3 when > splitting self cgroup lines by ':' so that Cgroup paths won't get truncated > if they contain embedded ':'s. For example, an entry of > "11:memory:/user.sli:ce" in a /proc/self/cgroup file will now result in a > Cgroup path of "/user.sli:ce" instead of "/user.sli". > > The fix was tested with Mach5 tiers 1 and 2, and Mach5 tiers 3-5 on Linux x64 > and Linux aarch64. > > Thanks, Harold Harold Seigel has updated the pull request incrementally with one additional commit since the last revision: Fix test typo and error - Changes: - all: https://git.openjdk.java.net/jdk/pull/5127/files - new: https://git.openjdk.java.net/jdk/pull/5127/files/fcc8c908..f339fe14 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5127=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5127=02-03 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/5127.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5127/head:pull/5127 PR: https://git.openjdk.java.net/jdk/pull/5127
Re: RFR: 8272124: Cgroup v1 initialization causes NullPointerException when path contains colon [v2]
On Tue, 17 Aug 2021 17:39:49 GMT, Harold Seigel wrote: >> Please review this small fix for JDK-8272124. The fix puts a limit of 3 >> when splitting self cgroup lines by ':' so that Cgroup paths won't get >> truncated if they contain embedded ':'s. For example, an entry of >> "11:memory:/user.sli:ce" in a /proc/self/cgroup file will now result in a >> Cgroup path of "/user.sli:ce" instead of "/user.sli". >> >> The fix was tested with Mach5 tiers 1 and 2, and Mach5 tiers 3-5 on Linux >> x64 and Linux aarch64. >> >> Thanks, Harold > > Harold Seigel has updated the pull request incrementally with one additional > commit since the last revision: > > add test case, comments, and other small changes Please review this update that modifies the new test case to have a mountinfo entry that contains multiple ":" characters. Thanks, Harold - PR: https://git.openjdk.java.net/jdk/pull/5127
Re: RFR: 8272124: Cgroup v1 initialization causes NullPointerException when path contains colon [v3]
> Please review this small fix for JDK-8272124. The fix puts a limit of 3 when > splitting self cgroup lines by ':' so that Cgroup paths won't get truncated > if they contain embedded ':'s. For example, an entry of > "11:memory:/user.sli:ce" in a /proc/self/cgroup file will now result in a > Cgroup path of "/user.sli:ce" instead of "/user.sli". > > The fix was tested with Mach5 tiers 1 and 2, and Mach5 tiers 3-5 on Linux x64 > and Linux aarch64. > > Thanks, Harold Harold Seigel has updated the pull request incrementally with one additional commit since the last revision: add mountinfo containing colongs to test - Changes: - all: https://git.openjdk.java.net/jdk/pull/5127/files - new: https://git.openjdk.java.net/jdk/pull/5127/files/5fa37e97..fcc8c908 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5127=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5127=01-02 Stats: 19 lines in 1 file changed: 18 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/5127.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5127/head:pull/5127 PR: https://git.openjdk.java.net/jdk/pull/5127
Re: RFR: 8272398: Update DockerTestUtils.buildJdkDockerImage() [v2]
On Tue, 17 Aug 2021 14:58:48 GMT, Mikhailo Seledtsov wrote: >> Please review this change that updates the buildJdkDockerImage() test >> library API. >> >> This work originated while working on "8195809: [TESTBUG] jps and jcmd -l >> support for containers is not tested". >> The initial intent was to extend the buildJdkDockerImage() API of >> DockerTestUtils to accept custom Dockerfile content. >> As I analyzed the usage of buildJdkDockerImage() I realized that: >> - 2nd argument "dockerfile" is always the same: "Dockerfile-BasicTest" >> its use has been obsolete for some time, in favor of Dockerfile >> generated by DockerTestUtils >> - 3rd argument "buildDirName" is also always the same: "jdk-docker" >> >> Hence I thought it would be a good idea to simplify this API and make it >> up-to-date. >> >> Also, since the method signature is being updated, I thought it would be a >> good idea to also change the name to use more generic container terminology: >> buildJdkDockerImage() --> buildJdkContainerImage() > > Mikhailo Seledtsov has updated the pull request incrementally with one > additional commit since the last revision: > > Addressing review feedback Other than that one obsolete comment the changes look good. Thanks, Harold test/lib/jdk/test/lib/containers/docker/DockerTestUtils.java line 175: > 173: /** > 174: * Build a container image based on given Dockerfile and image build > directory. > 175: * This comment looks wrong because there is no longer a given Dockerfile. - Marked as reviewed by hseigel (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5134
Re: RFR: 8272124: Cgroup v1 initialization causes NullPointerException when path contains colon [v2]
> Please review this small fix for JDK-8272124. The fix puts a limit of 3 when > splitting self cgroup lines by ':' so that Cgroup paths won't get truncated > if they contain embedded ':'s. For example, an entry of > "11:memory:/user.sli:ce" in a /proc/self/cgroup file will now result in a > Cgroup path of "/user.sli:ce" instead of "/user.sli". > > The fix was tested with Mach5 tiers 1 and 2, and Mach5 tiers 3-5 on Linux x64 > and Linux aarch64. > > Thanks, Harold Harold Seigel has updated the pull request incrementally with one additional commit since the last revision: add test case, comments, and other small changes - Changes: - all: https://git.openjdk.java.net/jdk/pull/5127/files - new: https://git.openjdk.java.net/jdk/pull/5127/files/918df2b4..5fa37e97 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5127=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5127=00-01 Stats: 41 lines in 2 files changed: 36 ins; 0 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/5127.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5127/head:pull/5127 PR: https://git.openjdk.java.net/jdk/pull/5127
Re: RFR: 8272124: Cgroup v1 initialization causes NullPointerException when path contains colon
On Mon, 16 Aug 2021 17:25:57 GMT, Harold Seigel wrote: > Please review this small fix for JDK-8272124. The fix puts a limit of 3 when > splitting self cgroup lines by ':' so that Cgroup paths won't get truncated > if they contain embedded ':'s. For example, an entry of > "11:memory:/user.sli:ce" in a /proc/self/cgroup file will now result in a > Cgroup path of "/user.sli:ce" instead of "/user.sli". > > The fix was tested with Mach5 tiers 1 and 2, and Mach5 tiers 3-5 on Linux x64 > and Linux aarch64. > > Thanks, Harold Thanks Misha and Severin for looking at this change! Please review this updated commit that tries to address Severin's comments. A new test case was added to TestCgroupSubsystemFactory.java for the multiple ':'s case and comments were added to CgroupSubsystemFactory.java. The ".filter(tokens -> (tokens.length >= 3))" code was removed but can be restored if need be. Thanks, Harold - PR: https://git.openjdk.java.net/jdk/pull/5127
RFR: 8272124: Cgroup v1 initialization causes NullPointerException when path contains colon
Please review this small fix for JDK-8272124. The fix puts a limit of 3 when splitting self cgroup lines by ':' so that Cgroup paths won't get truncated if they contain embedded ':'s. For example, an entry of "11:memory:/user.sli:ce" in a /proc/self/cgroup file will now result in a Cgroup path of "/user.sli:ce" instead of "/user.sli". The fix was tested with Mach5 tiers 1 and 2, and Mach5 tiers 3-5 on Linux x64 and Linux aarch64. Thanks, Harold - Commit messages: - 8272124: Cgroup v1 initialization causes NullPointerException when path contains colon Changes: https://git.openjdk.java.net/jdk/pull/5127/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5127=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8272124 Stats: 3 lines in 2 files changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/5127.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5127/head:pull/5127 PR: https://git.openjdk.java.net/jdk/pull/5127
Re: RFR: 8269186: [REDO] Remove CodeCache::mark_for_evol_deoptimization() method
On Wed, 23 Jun 2021 17:27:00 GMT, Coleen Phillimore wrote: > This is somewhat trivial change to remove > CodeCache::mark_for_evol_deoptimization() and its calling method, and nothing > else this time. > Ran vmTestbase/nsk/jvmti tests. LGTM Thanks, Harold - Marked as reviewed by hseigel (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4575
Integrated: 8243287: Removal of Unsafe::defineAnonymousClass
On Tue, 11 May 2021 12:50:31 GMT, Harold Seigel wrote: > Please review this large change to remove Unsafe::defineAnonymousClass(). > The change removes dAC relevant code and changes a lot of tests. Many of the > changed tests need renaming. I hope to do this in a follow up RFE. Some of > the tests were modified to use hidden classes, others were deleted because > either similar hidden classes tests already exist or they tested dAC specific > functionality, such as host classes. > > This change was tested with Mach5 tiers 1-2 on Linux, Mac OS, and Windows, > and Mach5 tiers 3-7 on Linux x64. > > Thanks, Harold This pull request has now been integrated. Changeset: e14b0268 Author:Harold Seigel URL: https://git.openjdk.java.net/jdk/commit/e14b0268411bba8eb01bf6c477cc8743a53ffd1c Stats: 3754 lines in 122 files changed: 75 ins; 3426 del; 253 mod 8243287: Removal of Unsafe::defineAnonymousClass Reviewed-by: iklam, mchung, alanb, dholmes - PR: https://git.openjdk.java.net/jdk/pull/3974
Re: RFR: 8243287: Removal of Unsafe::defineAnonymousClass [v5]
On Thu, 13 May 2021 07:19:03 GMT, David Holmes wrote: >> Harold Seigel has updated the pull request incrementally with one additional >> commit since the last revision: >> >> fix Weak hidden comment > > src/hotspot/share/oops/constantPool.hpp line 493: > >> 491: // object into a CONSTANT_String entry of an unsafe anonymous class. >> 492: // Methods internally created for method handles may also >> 493: // use pseudo-strings to link themselves to related metaobjects. > > Is this comment wrong? Are psuedo-strings not used by anything now? Thanks for looking at this. I discussed pseudo strings with Coleen and we didn't find any use of them besides unsafe.DefineAnonymousClass. - PR: https://git.openjdk.java.net/jdk/pull/3974
Re: RFR: 8243287: Removal of Unsafe::defineAnonymousClass [v4]
On Wed, 12 May 2021 22:30:30 GMT, Mandy Chung wrote:
>> Harold Seigel has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> test changes and small fixes
>
> src/hotspot/share/classfile/classLoaderData.cpp line 299:
>
>> 297: }
>> 298:
>> 299: // Weak hidden classes have their own ClassLoaderData that is marked to
>> keep alive
>
> `s/Weak/Non-strong/`
fixed. thanks for finding it.
> test/hotspot/jtreg/runtime/HiddenClasses/StressHiddenClasses.java line 39:
>
>> 37: import jdk.test.lib.compiler.InMemoryJavaCompiler;
>> 38:
>> 39: public class StressHiddenClasses {
>
> Since `StressClassLoadingTest` is revised to test hidden class, this test is
> a subset of it.
> I think this can be removed as JDK-8265694 suggests.
Thanks Mandy. I will remove the test as the fix for JDK-8265694 after this
change is pushed.
-
PR: https://git.openjdk.java.net/jdk/pull/3974
Re: RFR: 8243287: Removal of Unsafe::defineAnonymousClass [v5]
> Please review this large change to remove Unsafe::defineAnonymousClass(). > The change removes dAC relevant code and changes a lot of tests. Many of the > changed tests need renaming. I hope to do this in a follow up RFE. Some of > the tests were modified to use hidden classes, others were deleted because > either similar hidden classes tests already exist or they tested dAC specific > functionality, such as host classes. > > This change was tested with Mach5 tiers 1-2 on Linux, Mac OS, and Windows, > and Mach5 tiers 3-7 on Linux x64. > > Thanks, Harold Harold Seigel has updated the pull request incrementally with one additional commit since the last revision: fix Weak hidden comment - Changes: - all: https://git.openjdk.java.net/jdk/pull/3974/files - new: https://git.openjdk.java.net/jdk/pull/3974/files/5246dd76..38675761 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3974=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3974=03-04 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/3974.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3974/head:pull/3974 PR: https://git.openjdk.java.net/jdk/pull/3974
Re: RFR: 8243287: Removal of Unsafe::defineAnonymousClass [v3]
On Tue, 11 May 2021 17:07:35 GMT, Ioi Lam wrote: >> Harold Seigel has updated the pull request incrementally with one additional >> commit since the last revision: >> >> fix GetModuleTest.java > > src/hotspot/share/oops/instanceMirrorKlass.inline.hpp line 65: > >> 63: // so when handling the java mirror for the class we need to >> make sure its class >> 64: // loader data is claimed, this is done by calling do_cld >> explicitly. >> 65: // For non-string hidden classes the call to do_cld is made when >> the class > > Typo: non-strong fixed, thanks for finding this. - PR: https://git.openjdk.java.net/jdk/pull/3974
Re: RFR: 8243287: Removal of Unsafe::defineAnonymousClass [v3]
On Tue, 11 May 2021 20:49:46 GMT, Mandy Chung wrote: >> Harold Seigel has updated the pull request incrementally with one additional >> commit since the last revision: >> >> fix GetModuleTest.java > > src/jdk.internal.vm.ci/share/classes/jdk.vm.ci.meta/src/jdk/vm/ci/meta/MetaUtil.java > line 53: > >> 51: return simpleName; >> 52: } >> 53: // Must be a local class > > This file should not be changed. It refers to the Java language anonymous > class (not VM anonymous class). Changes have been restored. - PR: https://git.openjdk.java.net/jdk/pull/3974
Re: RFR: 8243287: Removal of Unsafe::defineAnonymousClass [v4]
> Please review this large change to remove Unsafe::defineAnonymousClass(). > The change removes dAC relevant code and changes a lot of tests. Many of the > changed tests need renaming. I hope to do this in a follow up RFE. Some of > the tests were modified to use hidden classes, others were deleted because > either similar hidden classes tests already exist or they tested dAC specific > functionality, such as host classes. > > This change was tested with Mach5 tiers 1-2 on Linux, Mac OS, and Windows, > and Mach5 tiers 3-7 on Linux x64. > > Thanks, Harold Harold Seigel has updated the pull request incrementally with one additional commit since the last revision: test changes and small fixes - Changes: - all: https://git.openjdk.java.net/jdk/pull/3974/files - new: https://git.openjdk.java.net/jdk/pull/3974/files/874a1603..5246dd76 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3974=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3974=02-03 Stats: 286 lines in 10 files changed: 22 ins; 256 del; 8 mod Patch: https://git.openjdk.java.net/jdk/pull/3974.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3974/head:pull/3974 PR: https://git.openjdk.java.net/jdk/pull/3974
Re: RFR: 8243287: Removal of Unsafe::defineAnonymousClass [v3]
On Tue, 11 May 2021 14:13:49 GMT, Harold Seigel wrote: >> Please review this large change to remove Unsafe::defineAnonymousClass(). >> The change removes dAC relevant code and changes a lot of tests. Many of >> the changed tests need renaming. I hope to do this in a follow up RFE. >> Some of the tests were modified to use hidden classes, others were deleted >> because either similar hidden classes tests already exist or they tested dAC >> specific functionality, such as host classes. >> >> This change was tested with Mach5 tiers 1-2 on Linux, Mac OS, and Windows, >> and Mach5 tiers 3-7 on Linux x64. >> >> Thanks, Harold > > Harold Seigel has updated the pull request incrementally with one additional > commit since the last revision: > > fix GetModuleTest.java Thanks Alan, Ioi, and Mandy for looking at this. The latest changes should address the issues that you found. Harold - PR: https://git.openjdk.java.net/jdk/pull/3974
Re: RFR: 8243287: Removal of Unsafe::defineAnonymousClass
Hi Brian, Thanks for looking at this. The JDK no longer creates unsafe anon classes. So, those tests could only find an unsafe anonymous class if they explicitly created one. In which case, the tests would need to call Unsafe.defineAnonymousClass(). And, hopefully, those tests have been handled in this webrev. If there are dead tests then they probably died when the JDK stopped creating unsafe anon classes. Note that none of them showed up as failures during regression testing. Harold On 5/11/2021 9:20 AM, Brian Goetz wrote: There may be some JDK code that checks for anon classes by comparing the name to see if it contains a slash, especially tests, but which don’t say “anonymous”. Did you do a search for these idioms too, which are now dead tests? Sent from my iPad On May 11, 2021, at 8:59 AM, Harold Seigel wrote: Please review this large change to remove Unsafe::defineAnonymousClass(). The change removes dAC relevant code and changes a lot of tests. Many of the changed tests need renaming. I hope to do this in a follow up RFE. Some of the tests were modified to use hidden classes, others were deleted because either similar hidden classes tests already exist or they tested dAC specific functionality, such as host classes. This change was tested with Mach5 tiers 1-2 on Linux, Mac OS, and Windows, and Mach5 tiers 3-7 on Linux x64. Thanks, Harold - Commit messages: - 8243287: Removal of Unsafe::defineAnonymousClass Changes: https://git.openjdk.java.net/jdk/pull/3974/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3974=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8243287 Stats: 3516 lines in 116 files changed: 69 ins; 3181 del; 266 mod Patch: https://git.openjdk.java.net/jdk/pull/3974.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3974/head:pull/3974 PR: https://git.openjdk.java.net/jdk/pull/3974
Re: RFR: 8243287: Removal of Unsafe::defineAnonymousClass [v3]
On Tue, 11 May 2021 13:41:53 GMT, Alan Bateman wrote:
>> test/jdk/java/lang/Class/GetModuleTest.java line 42:
>>
>>> 40: import static org.testng.Assert.*;
>>> 41:
>>> 42: public class GetModuleTest {
>>
>> testGetModuleOnVMAnonymousClass is the only test here that uses ASM so you
>> can remove the imports as part of the removal.
>
> Can you check test/jdkjava/lang/Class/attributes/ClassAttributesTest.java? It
> may minimally need a comment to be updated. That's the only additional test
> that I could find in test/jdk.
Hi Alan,
Thanks for find this. I removed the unneeded imports and @modules from
GetModulesTest.java and updated the comment in ClassAttributesTest.java.
Thanks, Harold
-
PR: https://git.openjdk.java.net/jdk/pull/3974
Re: RFR: 8243287: Removal of Unsafe::defineAnonymousClass [v3]
> Please review this large change to remove Unsafe::defineAnonymousClass(). > The change removes dAC relevant code and changes a lot of tests. Many of the > changed tests need renaming. I hope to do this in a follow up RFE. Some of > the tests were modified to use hidden classes, others were deleted because > either similar hidden classes tests already exist or they tested dAC specific > functionality, such as host classes. > > This change was tested with Mach5 tiers 1-2 on Linux, Mac OS, and Windows, > and Mach5 tiers 3-7 on Linux x64. > > Thanks, Harold Harold Seigel has updated the pull request incrementally with one additional commit since the last revision: fix GetModuleTest.java - Changes: - all: https://git.openjdk.java.net/jdk/pull/3974/files - new: https://git.openjdk.java.net/jdk/pull/3974/files/35c6634c..874a1603 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3974=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3974=01-02 Stats: 3 lines in 1 file changed: 0 ins; 2 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/3974.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3974/head:pull/3974 PR: https://git.openjdk.java.net/jdk/pull/3974
Re: RFR: 8243287: Removal of Unsafe::defineAnonymousClass [v2]
> Please review this large change to remove Unsafe::defineAnonymousClass(). > The change removes dAC relevant code and changes a lot of tests. Many of the > changed tests need renaming. I hope to do this in a follow up RFE. Some of > the tests were modified to use hidden classes, others were deleted because > either similar hidden classes tests already exist or they tested dAC specific > functionality, such as host classes. > > This change was tested with Mach5 tiers 1-2 on Linux, Mac OS, and Windows, > and Mach5 tiers 3-7 on Linux x64. > > Thanks, Harold Harold Seigel has updated the pull request incrementally with one additional commit since the last revision: test fixes - Changes: - all: https://git.openjdk.java.net/jdk/pull/3974/files - new: https://git.openjdk.java.net/jdk/pull/3974/files/233a4725..35c6634c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3974=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3974=00-01 Stats: 5 lines in 2 files changed: 0 ins; 3 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/3974.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3974/head:pull/3974 PR: https://git.openjdk.java.net/jdk/pull/3974
Integrated: 8264711: More runtime TRAPS cleanups
On Mon, 5 Apr 2021 17:57:13 GMT, Harold Seigel wrote: > Please review this additional cleanup of use of TRAPS in hotspot runtime > code. The changes were tested with Mach5 tiers 1-2 on Linux, Mac OS, and > Windows and Mach5 tiers 3-5 on Linux x64. > > Thanks, Harold This pull request has now been integrated. Changeset: af13c64f Author: Harold Seigel URL: https://git.openjdk.java.net/jdk/commit/af13c64f Stats: 97 lines in 26 files changed: 1 ins; 13 del; 83 mod 8264711: More runtime TRAPS cleanups Reviewed-by: lfoltan, pchilanomate, dholmes, dcubed - PR: https://git.openjdk.java.net/jdk/pull/3345
Re: RFR: 8264711: More runtime TRAPS cleanups [v4]
On Thu, 8 Apr 2021 10:10:00 GMT, David Holmes wrote: >> Harold Seigel has updated the pull request incrementally with one additional >> commit since the last revision: >> >> remove unneeded statement > > Hi Harold, > > Updates seem fine. > > Thanks, > David Thanks Lois, Patricio, David, and Dan for reviewing this! - PR: https://git.openjdk.java.net/jdk/pull/3345
Re: RFR: 8264711: More runtime TRAPS cleanups [v4]
On Mon, 5 Apr 2021 23:15:48 GMT, David Holmes wrote:
>> Harold Seigel has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> remove unneeded statement
>
> src/hotspot/share/classfile/klassFactory.cpp line 117:
>
>> 115:ClassLoaderData*
>> loader_data,
>> 116:Handle
>> protection_domain,
>> 117:
>> JvmtiCachedClassFileData** cached_class_file) {
>
> I don't think this removal of TRAPS is correct. The ClassFileLoadHook could
> result in an exception becoming pending.
Thanks! This change was reverted.
-
PR: https://git.openjdk.java.net/jdk/pull/3345
Re: RFR: 8264711: More runtime TRAPS cleanups [v2]
On Mon, 5 Apr 2021 23:28:38 GMT, David Holmes wrote:
>> Harold Seigel has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Undo change to ObjectSynchronizer::jni_exit()
>
> Hi Harold,
>
> Lots of good cleanup here but also a couple of things that I think are
> incorrect. Platform string creation can throw exceptions; and I also believe
> the ClassFileLoadHook can too.
>
> Thanks,
> David
Please review this latest version of this change containing reviewer suggested
changes.
Thanks, Harold
> src/hotspot/share/classfile/javaClasses.cpp line 396:
>
>> 394:
>> 395: // Converts a C string to a Java String based on current encoding
>> 396: Handle java_lang_String::create_from_platform_dependent_str(JavaThread*
>> current, const char* str) {
>
> This change is incorrect. JNU_NewStringPlatform can raise an exception so
> TRAPS here is correct.
Thanks for finding this. I reverted this change and the change to
as_platform_dependent_str() because it calls GetStringPlatformChars() which
calls JNU_GetStringPlatformChars() which can throw an exception.
> src/hotspot/share/prims/jvmtiEnv.cpp line 709:
>
>> 707:
>> 708: // need the path as java.lang.String
>> 709: Handle path =
>> java_lang_String::create_from_platform_dependent_str(THREAD, segment);
>
> This change will need reverting as an exception is possible as previously
> noted.
>
> But note that if there was no possibility of exception here then the
> following check of HAS_PENDING_EXCEPTION should also have been removed.
Reverted.
-
PR: https://git.openjdk.java.net/jdk/pull/3345
Re: RFR: 8264711: More runtime TRAPS cleanups [v4]
On Mon, 5 Apr 2021 19:08:38 GMT, Patricio Chilano Mateo
wrote:
>> Harold Seigel has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> remove unneeded statement
>
> src/hotspot/share/runtime/synchronizer.cpp line 609:
>
>> 607: // intentionally do not use CHECK on check_owner because we must exit
>> the
>> 608: // monitor even if an exception was already pending.
>> 609: if (monitor->check_owner(current)) {
>
> We can actually throw IMSE from check_owner() if this thread is not the real
> owner.
I reverted this change. Thanks.
-
PR: https://git.openjdk.java.net/jdk/pull/3345
Re: RFR: 8264711: More runtime TRAPS cleanups [v4]
> Please review this additional cleanup of use of TRAPS in hotspot runtime > code. The changes were tested with Mach5 tiers 1-2 on Linux, Mac OS, and > Windows and Mach5 tiers 3-5 on Linux x64. > > Thanks, Harold Harold Seigel has updated the pull request incrementally with one additional commit since the last revision: remove unneeded statement - Changes: - all: https://git.openjdk.java.net/jdk/pull/3345/files - new: https://git.openjdk.java.net/jdk/pull/3345/files/1ee327f0..0f1b4f6b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3345=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3345=02-03 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/3345.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3345/head:pull/3345 PR: https://git.openjdk.java.net/jdk/pull/3345
Re: RFR: 8264711: More runtime TRAPS cleanups [v3]
> Please review this additional cleanup of use of TRAPS in hotspot runtime > code. The changes were tested with Mach5 tiers 1-2 on Linux, Mac OS, and > Windows and Mach5 tiers 3-5 on Linux x64. > > Thanks, Harold Harold Seigel has updated the pull request incrementally with one additional commit since the last revision: restore improperly removed TRAPS - Changes: - all: https://git.openjdk.java.net/jdk/pull/3345/files - new: https://git.openjdk.java.net/jdk/pull/3345/files/e488fb8a..1ee327f0 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3345=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3345=01-02 Stats: 42 lines in 7 files changed: 5 ins; 3 del; 34 mod Patch: https://git.openjdk.java.net/jdk/pull/3345.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3345/head:pull/3345 PR: https://git.openjdk.java.net/jdk/pull/3345
Re: RFR: 8264711: More runtime TRAPS cleanups [v2]
On Mon, 5 Apr 2021 19:11:49 GMT, Patricio Chilano Mateo wrote: >> Harold Seigel has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Undo change to ObjectSynchronizer::jni_exit() > > Hi Harold, > > I looked at the changes to synchronization code and looks good except for one > issue below. > > Thanks, > Patricio Thanks Lois and Patricio for reviewing the change! I removed my bogus change to ObjectSynchronizer::jni_exit() which also made calls consistent in jfrJavaSupport.cpp. Harold - PR: https://git.openjdk.java.net/jdk/pull/3345
Re: RFR: 8264711: More runtime TRAPS cleanups [v2]
> Please review this additional cleanup of use of TRAPS in hotspot runtime > code. The changes were tested with Mach5 tiers 1-2 on Linux, Mac OS, and > Windows and Mach5 tiers 3-5 on Linux x64. > > Thanks, Harold Harold Seigel has updated the pull request incrementally with one additional commit since the last revision: Undo change to ObjectSynchronizer::jni_exit() - Changes: - all: https://git.openjdk.java.net/jdk/pull/3345/files - new: https://git.openjdk.java.net/jdk/pull/3345/files/b83f1404..e488fb8a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3345=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3345=00-01 Stats: 6 lines in 4 files changed: 1 ins; 0 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/3345.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3345/head:pull/3345 PR: https://git.openjdk.java.net/jdk/pull/3345
RFR: 8264711: More runtime TRAPS cleanups
Please review this additional cleanup of use of TRAPS in hotspot runtime code. The changes were tested with Mach5 tiers 1-2 on Linux, Mac OS, and Windows and Mach5 tiers 3-5 on Linux x64. Thanks, Harold - Commit messages: - 8264711: More runtime TRAPS cleanups Changes: https://git.openjdk.java.net/jdk/pull/3345/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3345=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8264711 Stats: 146 lines in 32 files changed: 5 ins; 19 del; 122 mod Patch: https://git.openjdk.java.net/jdk/pull/3345.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3345/head:pull/3345 PR: https://git.openjdk.java.net/jdk/pull/3345
Re: RFR: 8262280: Incorrect exception handling for VMThread in class redefinition
On Thu, 1 Apr 2021 16:58:00 GMT, Coleen Phillimore wrote: > This is a trivial change to remove the last TRAPS from redefine_single_class > which is called by the VM thread during a safepoint. > Tested with serviceability/jvmti/RedefineClasses, vmTestbase/nsk/jvmti,jdi > and jdk/java/lang/instrument tests. And tier1 sanity in progress. The changes look good and trivial. Thank! Harold - Marked as reviewed by hseigel (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/3310
Re: RFR: 8264538: Rename SystemDictionary::parse_stream [v3]
On Wed, 31 Mar 2021 21:41:39 GMT, Coleen Phillimore wrote: >> This function is used to call the classfile parser for hidden or anonymous >> classes, and for use with jvmti RedefineClasses. The latter only calls >> KlassFactory::create_from_stream and skips the rest of the code in >> SystemDictionary::parse_stream. >> >> Renamed SystemDictionary::parse_stream -> resolve_hidden_class_from_stream >> resolve_from_stream -> resolve_class_from_stream >> and have SystemDictionary::resolve_from_stream() call the right version >> depending on ClassLoadInfo flags. Callers of resolve_from_stream now pass >> protection domain via. ClassLoadInfo. >> >> So the external API is resolve_from_stream. >> >> Tested with tier1 on 4 Oracle supported platforms. > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > Add and remove comments. The changes look good! Thanks, Harold - Marked as reviewed by hseigel (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/3289
Re: RFR: 8264538: Rename SystemDictionary::parse_stream [v2]
On Wed, 31 Mar 2021 19:57:57 GMT, Coleen Phillimore wrote: >> This function is used to call the classfile parser for hidden or anonymous >> classes, and for use with jvmti RedefineClasses. The latter only calls >> KlassFactory::create_from_stream and skips the rest of the code in >> SystemDictionary::parse_stream. >> >> Renamed SystemDictionary::parse_stream -> resolve_hidden_class_from_stream >> resolve_from_stream -> resolve_class_from_stream >> and have SystemDictionary::resolve_from_stream() call the right version >> depending on ClassLoadInfo flags. Callers of resolve_from_stream now pass >> protection domain via. ClassLoadInfo. >> >> So the external API is resolve_from_stream. >> >> Tested with tier1 on 4 Oracle supported platforms. > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > fifix comment src/hotspot/share/prims/jvmtiRedefineClasses.hpp line 305: > 303: // - How do we serialize the RedefineClasses() API without deadlocking? > 304: // > 305: // - KlassFactory::create_from_stream() was called with a NULL protection Maybe delete the comment that goes from lines 305 - 309 ? - PR: https://git.openjdk.java.net/jdk/pull/3289
Re: RFR: 8264538: Rename SystemDictionary::parse_stream [v2]
On Wed, 31 Mar 2021 19:57:57 GMT, Coleen Phillimore wrote: >> This function is used to call the classfile parser for hidden or anonymous >> classes, and for use with jvmti RedefineClasses. The latter only calls >> KlassFactory::create_from_stream and skips the rest of the code in >> SystemDictionary::parse_stream. >> >> Renamed SystemDictionary::parse_stream -> resolve_hidden_class_from_stream >> resolve_from_stream -> resolve_class_from_stream >> and have SystemDictionary::resolve_from_stream() call the right version >> depending on ClassLoadInfo flags. Callers of resolve_from_stream now pass >> protection domain via. ClassLoadInfo. >> >> So the external API is resolve_from_stream. >> >> Tested with tier1 on 4 Oracle supported platforms. > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > fifix comment src/hotspot/share/prims/jvmtiRedefineClasses.cpp line 1395: > 1393: cl_info, > 1394: THREAD); > 1395: Could you add a comment above line 1390 saying you can't call resolve_class_from_stream() here because the resulting class should not go in the system dictionary? - PR: https://git.openjdk.java.net/jdk/pull/3289
Integrated: 8264193: Remove TRAPS parameters for modules and defaultmethods
On Mon, 29 Mar 2021 17:40:09 GMT, Harold Seigel wrote: > Please review this change for JDK-8264193 to remove unneeded TRAPS parameters > from modules and default methods files. Besides removing TRAPS, > Modules::get_named_module() was changed to return an oop instead of a > jobject, removing its need for a TRAPS parameter. > > This change was tested with Mach5 tiers 1 and 2 on Linux, Mac OS, and > Windows, and Mach5 tiers 3-5 on Linux x64. > > Thanks, Harold This pull request has now been integrated. Changeset: 6e74c3ab Author:Harold Seigel URL: https://git.openjdk.java.net/jdk/commit/6e74c3ab Stats: 61 lines in 8 files changed: 1 ins; 13 del; 47 mod 8264193: Remove TRAPS parameters for modules and defaultmethods Reviewed-by: lfoltan, ccheung, coleenp, dholmes - PR: https://git.openjdk.java.net/jdk/pull/3247
Re: RFR: 8264193: Remove TRAPS parameters for modules and defaultmethods
On Tue, 30 Mar 2021 01:40:02 GMT, David Holmes wrote: >> Please review this change for JDK-8264193 to remove unneeded TRAPS >> parameters from modules and default methods files. Besides removing TRAPS, >> Modules::get_named_module() was changed to return an oop instead of a >> jobject, removing its need for a TRAPS parameter. >> >> This change was tested with Mach5 tiers 1 and 2 on Linux, Mac OS, and >> Windows, and Mach5 tiers 3-5 on Linux x64. >> >> Thanks, Harold > > Great cleanup Harold! > > Thanks, > David Thanks Lois, Calvin, Coleen, and David for the reviews! - PR: https://git.openjdk.java.net/jdk/pull/3247
RFR: 8264193: Remove TRAPS parameters for modules and defaultmethods
Please review this change for JDK-8264193 to remove unneeded TRAPS parameters from modules and default methods files. Besides removing TRAPS, Modules::get_named_module() was changed to return an oop instead of a jobject, removing its need for a TRAPS parameter. This change was tested with Mach5 tiers 1 and 2 on Linux, Mac OS, and Windows, and Mach5 tiers 3-5 on Linux x64. Thanks, Harold - Commit messages: - 8264193: Remove TRAPS parameters for modules and defaultmethods Changes: https://git.openjdk.java.net/jdk/pull/3247/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3247=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8264193 Stats: 61 lines in 8 files changed: 1 ins; 13 del; 47 mod Patch: https://git.openjdk.java.net/jdk/pull/3247.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3247/head:pull/3247 PR: https://git.openjdk.java.net/jdk/pull/3247
Re: RFR: 8264004: Don't use TRAPS if no exceptions are thrown [v3]
On Tue, 23 Mar 2021 15:05:59 GMT, Coleen Phillimore wrote: >> Removed the TRAPS in function declarations in jvmtiRedefineClasses and in >> ConstantPool merging functions. >> Tested with vmTestbase/nsk/jvmti and tier1 (in progress). > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > Fix some obvious single use variables. Latest changes look good. Thanks, Harold - Marked as reviewed by hseigel (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/3141
Re: RFR: 8264004: Don't use TRAPS if no exceptions are thrown [v2]
On Tue, 23 Mar 2021 01:22:56 GMT, Coleen Phillimore wrote: >> Removed the TRAPS in function declarations in jvmtiRedefineClasses and in >> ConstantPool merging functions. >> Tested with vmTestbase/nsk/jvmti and tier1 (in progress). > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > missed THREAD that should be CHECK_false argument. The changes look good! Thanks, Harold - Marked as reviewed by hseigel (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/3141
Re: RFR: 8264004: Don't use TRAPS if no exceptions are thrown [v2]
On Tue, 23 Mar 2021 01:22:56 GMT, Coleen Phillimore wrote:
>> Removed the TRAPS in function declarations in jvmtiRedefineClasses and in
>> ConstantPool merging functions.
>> Tested with vmTestbase/nsk/jvmti and tier1 (in progress).
>
> Coleen Phillimore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> missed THREAD that should be CHECK_false argument.
src/hotspot/share/oops/constantPool.cpp line 1426:
> 1424: bool match_entry = compare_entry_to(k1, cp2, k2);
> 1425: bool match_operand = compare_operand_to(i1, cp2, i2);
> 1426: return (match_entry && match_operand);
Is it worth changing this to: If (compare_entry_to(...) &&
compare_operand_to(..)) { .. }
Then if the first one is false the second call isn't needed?
-
PR: https://git.openjdk.java.net/jdk/pull/3141
Re: RFR: 8262379: Add regression test for JDK-8257746
On Thu, 25 Feb 2021 16:27:20 GMT, Severin Gehwolf wrote: > Fails prior the patch of JDK-8257746, passes after. As expected. > > Thoughts? The new tests looks good. Thanks for adding it. Harold - Marked as reviewed by hseigel (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2725
Re: RFR: 8254001: [Metrics] Enhance parsing of cgroup interface files for version detection [v4]
On Tue, 9 Feb 2021 13:31:25 GMT, Severin Gehwolf wrote: >> This is an enhancement which solves two issues: >> >> 1. Multiple reads of relevant cgroup interface files. Now interface files >> are only read once per file (just like Hotspot). >> 2. Proxies creation of the impl specific subsystem via `determineType()` as >> before, but now reads all relevant interface files: `/proc/cgroups`, >> `/proc/self/mountinfo` and `/proc/self/cgroup`. Once read it passes the >> parsed information to the impl specific subsystem classes for instantiation. >> This allows for more flexibility of testing as interface files can be mocked >> and, thus, more cases can be tested that way without having access to these >> specific systems. For example, proper regression tests for JDK-8217766 and >> JDK-8253435 have been added now with this in place. >> >> * [x] Tested on Linux x86_64 on cgroups v1 and cgroups v2. Container tests >> pass. > > 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 seven additional > commits since the last revision: > > - Fix jcheck > - Add documentation and reduce code running in the critical section > - Add some documentation > - Merge branch 'master' into jdk-8254001-enhance-file-parsing-java-metrics > - Merge branch 'master' into jdk-8254001-enhance-file-parsing-java-metrics > - Merge branch 'master' into jdk-8254001-enhance-file-parsing-java-metrics > - 8254001: [Metrics] Enhance parsing of cgroup interface files for version > detection Hi Severin, Thanks for doing this! Sorry for taking so long to review this change. The change looks good. Before pushing it, could you add a comment explaining what the code in lines 185-194 of CgroupSubsystemFactory.java is doing? Also, please don't overwrite the fix for JDK-8257746. Thanks again! Harold - Marked as reviewed by hseigel (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1393
Integrated: 8261340: Fix 'deprecated' warnings in the vmTestbase/nsk tests
On Mon, 8 Feb 2021 19:54:03 GMT, Harold Seigel wrote: > Please review this small fix for JDK-8261340 to clean up deprecation > warnings, such as the following, in the vmTestbase/nsk tests. > > warning: [dep-ann] deprecated item is not annotated with @Deprecated > > The change was tested by running the tests locally and checking for the > warnings. It was regression tested with tiers 1-2 on Linux, Mac OS, and > Windows and tiers 3-5 on Linux x64. > > Thanks, Harold This pull request has now been integrated. Changeset: b38d5be8 Author:Harold Seigel URL: https://git.openjdk.java.net/jdk/commit/b38d5be8 Stats: 17 lines in 5 files changed: 12 ins; 0 del; 5 mod 8261340: Fix 'deprecated' warnings in the vmTestbase/nsk tests Reviewed-by: lfoltan, sspitsyn - PR: https://git.openjdk.java.net/jdk/pull/2462
Re: RFR: 8261340: Fix 'deprecated' warnings in the vmTestbase/nsk tests
On Tue, 9 Feb 2021 04:36:29 GMT, Serguei Spitsyn wrote: >> Please review this small fix for JDK-8261340 to clean up deprecation >> warnings, such as the following, in the vmTestbase/nsk tests. >> >> warning: [dep-ann] deprecated item is not annotated with @Deprecated >> >> The change was tested by running the tests locally and checking for the >> warnings. It was regression tested with tiers 1-2 on Linux, Mac OS, and >> Windows and tiers 3-5 on Linux x64. >> >> Thanks, Harold > > LGTM++ > > Thanks, > Serguei Thanks Lois and Sergei for reviewing this change! - PR: https://git.openjdk.java.net/jdk/pull/2462
RFR: 8261340: Fix 'deprecated' warnings in the vmTestbase/nsk tests
Please review this small fix for JDK-8261340 to clean up deprecation warnings, such as the following, in the vmTestbase/nsk tests. warning: [dep-ann] deprecated item is not annotated with @Deprecated The change was tested by running the tests locally and checking for the warnings. It was regression tested with tiers 1-2 on Linux, Mac OS, and Windows and tiers 3-5 on Linux x64. Thanks, Harold - Commit messages: - 8261340: Fix 'deprecated' warnings in the vmTestbase/nsk tests Changes: https://git.openjdk.java.net/jdk/pull/2462/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=2462=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8261340 Stats: 17 lines in 5 files changed: 12 ins; 0 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/2462.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2462/head:pull/2462 PR: https://git.openjdk.java.net/jdk/pull/2462
Re: RFR: 8261161: Clean up warnings in hotspot/jtreg/vmTestbase tests
On Fri, 5 Feb 2021 16:53:00 GMT, Coleen Phillimore wrote: >> Please review this change to clean up warnings, such as the following, in >> the vmTestbase tests. >> >> warning: [synchronization] attempt to synchronize on an instance of a >> value-based class >> warning: [removal] Integer(int) in Integer has been deprecated and marked >> for removal >> >> This change cleans up the warnings by using a non-value based class to >> synchronize on, and replacing calls such as Integer(int) with >> Integer.valueOf(int). >> >> The change was tested by running Mach5 tiers 1-2 on Linux, Mac OS, and >> Windows, and Mach5 tiers 3-8 on Linux x64. >> >> Thanks, Harold > > Wow. thank you! Thanks Lois and Coleen for the reviews! - PR: https://git.openjdk.java.net/jdk/pull/2427
Integrated: 8261161: Clean up warnings in hotspot/jtreg/vmTestbase tests
On Fri, 5 Feb 2021 14:48:25 GMT, Harold Seigel wrote: > Please review this change to clean up warnings, such as the following, in the > vmTestbase tests. > > warning: [synchronization] attempt to synchronize on an instance of a > value-based class > warning: [removal] Integer(int) in Integer has been deprecated and marked for > removal > > This change cleans up the warnings by using a non-value based class to > synchronize on, and replacing calls such as Integer(int) with > Integer.valueOf(int). > > The change was tested by running Mach5 tiers 1-2 on Linux, Mac OS, and > Windows, and Mach5 tiers 3-8 on Linux x64. > > Thanks, Harold This pull request has now been integrated. Changeset: db0ca2b9 Author:Harold Seigel URL: https://git.openjdk.java.net/jdk/commit/db0ca2b9 Stats: 747 lines in 129 files changed: 2 ins; 0 del; 745 mod 8261161: Clean up warnings in hotspot/jtreg/vmTestbase tests Reviewed-by: lfoltan, coleenp - PR: https://git.openjdk.java.net/jdk/pull/2427
RFR: 8261161: Clean up warnings in hotspot/jtreg/vmTestbase tests
Please review this change to clean up warnings, such as the following, in the vmTestbase tests. warning: [synchronization] attempt to synchronize on an instance of a value-based class warning: [removal] Integer(int) in Integer has been deprecated and marked for removal This change cleans up the warnings by using a non-value based class to synchronize on, and replacing calls such as Integer(int) with Integer.valueOf(int). The change was tested by running Mach5 tiers 1-2 on Linux, Mac OS, and Windows, and Mach5 tiers 3-8 on Linux x64. Thanks, Harold - Commit messages: - 8261161: Clean up warnings in hotspot/jtreg/vmTestbase tests Changes: https://git.openjdk.java.net/jdk/pull/2427/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=2427=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8261161 Stats: 747 lines in 129 files changed: 2 ins; 0 del; 745 mod Patch: https://git.openjdk.java.net/jdk/pull/2427.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2427/head:pull/2427 PR: https://git.openjdk.java.net/jdk/pull/2427
Re: RFR: 8259482: jni_Set/GetField_probe are the same as their _nh versions
On Fri, 8 Jan 2021 19:32:36 GMT, Coleen Phillimore wrote: > Remove the _nh versions. Tested with tier1 and vmTestbase/nsk/jdi,jvmti > tests. The changes look good! Thanks, Harold - Marked as reviewed by hseigel (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2008
Re: RFR: 8253797: [cgroups v2] Account for the fact that swap accounting is disabled on some systems
On Mon, 7 Dec 2020 17:48:01 GMT, Severin Gehwolf wrote: > This has been implemented for cgroups v1 with > [JDK-8250984](https://bugs.openjdk.java.net/browse/JDK-8250984) but was > lacking some tooling support for cgroups v2. With podman 2.2.0 release this > could now be implemented (and tested). The idea is the same as for the > cgroups v1 fix. If we've got no swap limit capabilities, return the memory > limit only. > > Note that for cgroups v2 doesn't implement CgroupV1Metrics (obviously) and, > thus, doesn't have `getMemoryAndSwapFailCount()` and > `getMemoryAndSwapMaxUsage()`. > > Testing: > - [x] submit testing > - [x] container tests on cgroups v2 with swapaccount=0. > - [x] Manual container tests involving `-XshowSettings:system` on cgroups v2. > > Thoughts? The changes look good. Thanks for doing this. Harold - Marked as reviewed by hseigel (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1672
Re: RFR: 8245446: vmTestbase/nsk/jvmti/RedefineClasses/StressRedefine/TestDescription.java crash intermittently
On Wed, 9 Dec 2020 16:33:22 GMT, Coleen Phillimore wrote: > This change handles redefinition during method resolution, by returning the > new method. It's not needed to reresolve the invocation. See the bug for > more information. > > Tested with tier1-3 and tier8 on linux-x64-debug and macos-x64-debug, and > running the failing tests 100x without failure. Thank you to Serguei for > confirming the testing. > > I don't now why it can't link to the issue: > https://bugs.openjdk.java.net/browse/JDK-8245446 The changes look good! - Marked as reviewed by hseigel (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1717
Re: RFR: 8252180: [JEP 390] Deprecate wrapper class constructors for removal
On Sat, 5 Dec 2020 01:46:31 GMT, Dan Smith wrote: > Integration of [JEP 390](https://bugs.openjdk.java.net/browse/JDK-8249100). > > Development has been broken into 5 tasks, each with its own JBS issue: > - Deprecate wrapper class constructors for removal (rriggs) > - Revise "value-based class" & apply to wrappers (dlsmith) > - Define & apply annotation jdk.internal.ValueBased (rriggs) > - Add 'lint' warning for @ValueBased classes (sadayapalam) > - Diagnose synchronization on @ValueBased classes (lfoltan) > > All changes have been previously reviewed and integrated into the [`jep390` > branch](https://github.com/openjdk/valhalla/tree/jep390) of the `valhalla` > repository. See the subtasks of the 5 JBS issues for these changes, including > discussion and links to reviews. (Reviewers: mchung, dlsmith, jlaskey, > rriggs, lfoltan, fparain, hseigel.) > > CSRs have also been completed or are nearly complete: > > - [JDK-8254324](https://bugs.openjdk.java.net/browse/JDK-8254324) for wrapper > class constructor deprecation > - [JDK-8254944](https://bugs.openjdk.java.net/browse/JDK-8254944) for > revisions to "value-based class" > - [JDK-8256023](https://bugs.openjdk.java.net/browse/JDK-8256023) for new > `javac` lint warnings > > Here's an overview of the files changed: > > - `src/hotspot`: implementing diagnostics when `monitorenter` is applied to > an instance of a class tagged with `jdk.internal.ValueBased`. This enhances > previous work that produced such diagnostics for the primitive wrapper > classes. > > - `src/java.base/share/classes/java/lang`: deprecating for removal the > wrapper class constructors; revising the definition of "value-based class" in > `ValueBased.html` and the description used by linking classes; applying > "value-based class" to the primitive wrapper classes; marking value-based > classes with `@jdk.internal.ValueBased`. > > - `src/java.base/share/classes/java/lang/constant`: no longer designating > these classes as "value-based", since they rely heavily on field inheritance. > > - `src/java.base/share/classes/java/time`: revising the description used to > link to `ValueBased.html`; marking value-based classes with > `@jdk.internal.ValueBased`. > > - `src/java.base/share/classes/java/util`: revising the description used to > link to `ValueBased.html`; marking value-based classes with > `@jdk.internal.ValueBased`. > > - `src/java.base/share/classes/jdk/internal`: define the > `@jdk.internal.ValueBased` annotation. > > - `src/java.management.rmi`: removing uses of wrapper class constructors. > > - `src/java.xml`: removing uses of wrapper class constructors. > > - `src/jdk.compiler`: implementing the `synchronization` lint category, which > reports attempts to synchronize on classes and interfaces annotated with > `@jdk.internal.ValueBased`. > > - `src/jdk.incubator.foreign`: revising the description used to link to > `ValueBased.html`. (Applying `@jdk.internal.ValueBased` would require a > special module export, which was not deemed necessary for an incubating API.) > > - `src/jdk.internal.vm.compiler`: suppressing `javac` deprecation and > synchronization warnings in tests > > - `src/jdk.jfr`: supplementary changes for HotSpot diagnostics > > - `test`: testing new `javac` and HotSpot behavior; removing usages of > deprecated wrapper class constructors from tests, or suppressing deprecation > warnings; revising the description used to link to `ValueBased.html`. The hotspot changes look good. In a future change, could you add additional classes, such as ProcessHandle to test TestSyncOnValueBasedClassEvent. Currently, it only tests primitive classes. - Marked as reviewed by hseigel (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1636
Integrated: 8256718: Obsolete the long term deprecated and aliased Trace flags
On Mon, 30 Nov 2020 21:13:05 GMT, Harold Seigel wrote: > Please review this change to obsolete the deprecated and aliased Trace flags. > The now empty aliased_logging_flags support was left in arguments.cpp for > use by trace flags that get deprecated and aliased in the future. > > With this change, users will get the following example messages when using > these obsolete flags, depending on whether -XX:+... or -XX:-... was specified: > > VM warning: Ignoring option TraceClassPaths; support was removed in 16.0. > Please use -Xlog:class+path=info instead. > > VM warning: Ignoring option TraceClassPaths; support was removed in 16.0. > Please use -Xlog:class+path=off instead. > > The change was tested with tiers1and 2 on Linux, Windows, and MacOS, and > tiers 3-5 on Linux x64 and with JCK lang and vm tests. > > Thanks, Harold This pull request has now been integrated. Changeset: e4497c9e Author:Harold Seigel URL: https://git.openjdk.java.net/jdk/commit/e4497c9e Stats: 326 lines in 23 files changed: 16 ins; 289 del; 21 mod 8256718: Obsolete the long term deprecated and aliased Trace flags Reviewed-by: sspitsyn, iklam, dholmes, coleenp - PR: https://git.openjdk.java.net/jdk/pull/1525
Re: RFR: 8256718: Obsolete the long term deprecated and aliased Trace flags [v3]
On Wed, 2 Dec 2020 23:00:03 GMT, David Holmes wrote: >> Harold Seigel has updated the pull request incrementally with one additional >> commit since the last revision: >> >> 8256718: Obsolete the long term deprecated and aliased Trace flags > > Looks good! > > Thanks, > David Thanks Coleen, Ioi, Serguei, and David for all the reviews. Harold - PR: https://git.openjdk.java.net/jdk/pull/1525
Re: RFR: 8256718: Obsolete the long term deprecated and aliased Trace flags [v2]
On Wed, 2 Dec 2020 02:42:18 GMT, David Holmes wrote: >> Harold Seigel has updated the pull request incrementally with one additional >> commit since the last revision: >> >> 8256718: Obsolete the long term deprecated and aliased Trace flags > > I'm still pushing for treating all flags the same and removing all the > aliased-flag code. > > Coleen seems to be okay either way. :) > > Thanks, > David Please review this latest webrev. It removes all the aliasing code for the obsolete tracing flags and does not suggest logging alternatives in their obsolete flag messages. Thanks, Harold - PR: https://git.openjdk.java.net/jdk/pull/1525
Re: RFR: 8256718: Obsolete the long term deprecated and aliased Trace flags [v3]
On Tue, 1 Dec 2020 12:02:57 GMT, Coleen Phillimore wrote:
>> Harold Seigel has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8256718: Obsolete the long term deprecated and aliased Trace flags
>
> src/hotspot/share/runtime/arguments.cpp line 617:
>
>> 615: #ifndef PRODUCT
>> 616: // These options are removed in jdk9. Remove this code for jdk10.
>> 617: static AliasedFlag const removed_develop_logging_flags[] = {
>
> I think this removed_develop_logging_flags infrastructure should be removed.
done.
-
PR: https://git.openjdk.java.net/jdk/pull/1525
Re: RFR: 8256718: Obsolete the long term deprecated and aliased Trace flags [v3]
> Please review this change to obsolete the deprecated and aliased Trace flags. > The now empty aliased_logging_flags support was left in arguments.cpp for > use by trace flags that get deprecated and aliased in the future. > > With this change, users will get the following example messages when using > these obsolete flags, depending on whether -XX:+... or -XX:-... was specified: > > VM warning: Ignoring option TraceClassPaths; support was removed in 16.0. > Please use -Xlog:class+path=info instead. > > VM warning: Ignoring option TraceClassPaths; support was removed in 16.0. > Please use -Xlog:class+path=off instead. > > The change was tested with tiers1and 2 on Linux, Windows, and MacOS, and > tiers 3-5 on Linux x64 and with JCK lang and vm tests. > > Thanks, Harold Harold Seigel has updated the pull request incrementally with one additional commit since the last revision: 8256718: Obsolete the long term deprecated and aliased Trace flags - Changes: - all: https://git.openjdk.java.net/jdk/pull/1525/files - new: https://git.openjdk.java.net/jdk/pull/1525/files/84e421f7..2c5bec9a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1525=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1525=01-02 Stats: 40 lines in 2 files changed: 3 ins; 36 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/1525.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1525/head:pull/1525 PR: https://git.openjdk.java.net/jdk/pull/1525
Re: RFR: 8256718: Obsolete the long term deprecated and aliased Trace flags [v2]
On Tue, 1 Dec 2020 12:10:59 GMT, Coleen Phillimore wrote: >> Harold Seigel has updated the pull request incrementally with one additional >> commit since the last revision: >> >> 8256718: Obsolete the long term deprecated and aliased Trace flags > > I agree with David. We should remove the helpful messages at least for most > of the obsolete Print/Trace flags. Not sure about the big 3 though. Thanks everyone for the reviews. Please review the updated commit which contains the following changes: 1. Moves most of the flags to the normal location for obsolete flags. 2. Keeps the extra -Xlog message for the three most commonly used flags. 3. Removes the removed_develop_logging_flags struct and support functions. 4. Removes the aliased_logging_flags struct and support functions based on Coleen's statement that there are no existing flags that would use it. Thanks! Harold - PR: https://git.openjdk.java.net/jdk/pull/1525
Re: RFR: 8256718: Obsolete the long term deprecated and aliased Trace flags [v2]
> Please review this change to obsolete the deprecated and aliased Trace flags. > The now empty aliased_logging_flags support was left in arguments.cpp for > use by trace flags that get deprecated and aliased in the future. > > With this change, users will get the following example messages when using > these obsolete flags, depending on whether -XX:+... or -XX:-... was specified: > > VM warning: Ignoring option TraceClassPaths; support was removed in 16.0. > Please use -Xlog:class+path=info instead. > > VM warning: Ignoring option TraceClassPaths; support was removed in 16.0. > Please use -Xlog:class+path=off instead. > > The change was tested with tiers1and 2 on Linux, Windows, and MacOS, and > tiers 3-5 on Linux x64 and with JCK lang and vm tests. > > Thanks, Harold Harold Seigel has updated the pull request incrementally with one additional commit since the last revision: 8256718: Obsolete the long term deprecated and aliased Trace flags - Changes: - all: https://git.openjdk.java.net/jdk/pull/1525/files - new: https://git.openjdk.java.net/jdk/pull/1525/files/9c373c6b..84e421f7 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1525=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1525=00-01 Stats: 206 lines in 3 files changed: 13 ins; 192 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/1525.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1525/head:pull/1525 PR: https://git.openjdk.java.net/jdk/pull/1525
RFR: 8256718: Obsolete the long term deprecated and aliased Trace flags
Please review this change to obsolete the deprecated and aliased Trace flags. The now empty aliased_logging_flags support was left in arguments.cpp for use by trace flags that get deprecated and aliased in the future. With this change, users will get the following example messages when using these obsolete flags, depending on whether -XX:+... or -XX:-... was specified: VM warning: Ignoring option TraceClassPaths; support was removed in 16.0. Please use -Xlog:class+path=info instead. VM warning: Ignoring option TraceClassPaths; support was removed in 16.0. Please use -Xlog:class+path=off instead. The change was tested with tiers1and 2 on Linux, Windows, and MacOS, and tiers 3-5 on Linux x64 and with JCK lang and vm tests. Thanks, Harold - Commit messages: - 8256718: Obsolete the long term deprecated and aliased Trace flags Changes: https://git.openjdk.java.net/jdk/pull/1525/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1525=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8256718 Stats: 190 lines in 22 files changed: 51 ins; 112 del; 27 mod Patch: https://git.openjdk.java.net/jdk/pull/1525.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1525/head:pull/1525 PR: https://git.openjdk.java.net/jdk/pull/1525
Re: RFR: 8255787: Tag container tests that use cGroups with cgroups keyword
On Wed, 11 Nov 2020 00:27:59 GMT, Serguei Spitsyn wrote: >> Please review this small change to add a cgroups keyword to tests that use >> cgroups. The fix was tested by running Mach5 container tests. > > Hi Harold, > > The fix looks good. > > Thanks, > Serguei Thanks Serguei! Harold - PR: https://git.openjdk.java.net/jdk/pull/1148
Integrated: 8255787: Tag container tests that use cGroups with cgroups keyword
On Tue, 10 Nov 2020 21:24:25 GMT, Harold Seigel wrote: > Please review this small change to add a cgroups keyword to tests that use > cgroups. The fix was tested by running Mach5 container tests. This pull request has now been integrated. Changeset: 4df8abc2 Author:Harold Seigel URL: https://git.openjdk.java.net/jdk/commit/4df8abc2 Stats: 20 lines in 15 files changed: 15 ins; 0 del; 5 mod 8255787: Tag container tests that use cGroups with cgroups keyword Reviewed-by: sspitsyn - PR: https://git.openjdk.java.net/jdk/pull/1148
RFR: 8255787: Tag container tests that use cGroups with cgroups keyword
Please review this small change to add a cgroups keyword to tests that use cgroups. The fix was tested by running Mach5 container tests. - Commit messages: - 8255787: Tag container tests that use cGroups with cgroups keyword Changes: https://git.openjdk.java.net/jdk/pull/1148/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1148=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8255787 Stats: 20 lines in 15 files changed: 15 ins; 0 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/1148.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1148/head:pull/1148 PR: https://git.openjdk.java.net/jdk/pull/1148
Re: RFR: 8138588: VerifyMergedCPBytecodes option cleanup needed
On Mon, 9 Nov 2020 23:46:04 GMT, Coleen Phillimore wrote: > This option has been removed in favor of always verifying the bytecodes in > debug mode. Tested with tier1-3. Looks good! Thanks, Harold - Marked as reviewed by hseigel (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1137
Re: RFR: 8256052: Remove unused allocation type from fieldInfo [v4]
On Mon, 9 Nov 2020 20:05:09 GMT, Frederic Parain wrote: >> Please review this small cleanup code, removing the now unused allocation >> type from the fieldInfo structure. >> >> Tested with Mach5, tiers 1 to 3 and locally by running >> test/hotspot/jtreg/serviceability/sa tests. >> >> Thank you, >> >> Fred > > Frederic Parain has updated the pull request incrementally with one > additional commit since the last revision: > > Fix comment Looks good! - Marked as reviewed by hseigel (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1130
Re: RFR: 8256052: Remove unused allocation type from fieldInfo [v3]
On Mon, 9 Nov 2020 19:54:58 GMT, Frederic Parain wrote: >> Please review this small cleanup code, removing the now unused allocation >> type from the fieldInfo structure. >> >> Tested with Mach5, tiers 1 to 3 and locally by running >> test/hotspot/jtreg/serviceability/sa tests. >> >> Thank you, >> >> Fred > > Frederic Parain has updated the pull request incrementally with one > additional commit since the last revision: > > Remove unused symbol from vmStruct src/hotspot/share/oops/fieldInfo.hpp line 61: > 59: //[--offset]01 - real field offset > 60: > 61: // Bit O indicates if the packed field contains an offset (O=1) or not > (O=1) Hi Fred, should this comment say "... or not (0=0) ? - PR: https://git.openjdk.java.net/jdk/pull/1130
Integrated: 8253476: TestUseContainerSupport.java fails on some Linux kernels w/o swap limit capabilities
On Tue, 22 Sep 2020 15:52:43 GMT, Harold Seigel wrote: > Please review this small change to remove "--memory 200m" option from > TestUseContainerSupport.java. This can cause > test failures on systems where swap accounting is disabled. This pull request has now been integrated. Changeset: 2fe0a5d7 Author:Harold Seigel URL: https://git.openjdk.java.net/jdk/commit/2fe0a5d7 Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod 8253476: TestUseContainerSupport.java fails on some Linux kernels w/o swap limit capabilities Reviewed-by: bobv, coleenp - PR: https://git.openjdk.java.net/jdk/pull/303
RFR: 8253476: TestUseContainerSupport.java fails on some Linux kernels w/o…
Please review this small change to remove "--memory 200m" option from TestUseContainerSupport.java. This can cause test failures on systems where swap accounting is disabled. - Commit messages: - 8253476: TestUseContainerSupport.java fails on some Linux kernels w/o swap limit capabilities Changes: https://git.openjdk.java.net/jdk/pull/303/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=303=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8253476 Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/303.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/303/head:pull/303 PR: https://git.openjdk.java.net/jdk/pull/303
Re: RFR: 8243290: Improve diagnostic messages for class verification and redefinition failures
+1 Thanks, Harold On 6/10/2020 10:36 AM, coleen.phillim...@oracle.com wrote: Looks good to me now. thanks, Coleen On 6/10/20 9:06 AM, Poonam Parhar wrote: Hello Harold, Thanks for your review! I fixed the null string issue, and here's the updated webrev: http://cr.openjdk.java.net/~poonam/8243290/webrev.01/ Thanks, Poonam On 6/9/20 8:38 AM, Harold Seigel wrote: Hi Poonam, Thanks for making this change. In verifier.cpp, if ex_msg is NULL, will the call to st->print_cr() at line 142 - 143, fail? Thanks, Harold On 6/9/2020 10:46 AM, Poonam Parhar wrote: Hello, Please review this simple change for improving diagnostics around class verification and linking failures: Bug: https://bugs.openjdk.java.net/browse/JDK-8243290 Webrev: http://cr.openjdk.java.net/~poonam/8243290/webrev.00/ Problem: During the class redefinition process, if a class verification fails because it could not find a class referenced in the class being redefined, the printed NoClassDefFoundError error message is not very helpful. It does not print the class name for which NoClassDefFoundError was encountered, and that makes it very hard to find the real cause of redefinition failure. The proposed solution prints the class name during class linking and verification failures. Example output produced with these changes: With 'redefine' tag: [java] [3.243s][debug][redefine,class,load ] loaded name=org.apache.commons.logging.impl.Jdk14Logger (avail_mem=819540K) [java] [3.243s][debug][redefine,class,load ] loading name=org.apache.commons.logging.impl.Log4JLogger kind=101 (avail_mem=819540K) [java] [3.244s][info ][redefine,class,load,exceptions] link_class exception: 'java/lang/NoClassDefFoundError org/apache/log4j/Priority' [java] Java Result: 1 With 'verification' tag: [java] [49.702s][info ][verification] Verification for org.apache.commons.logging.impl.Log4JLogger has exception pending 'java.lang.NoClassDefFoundError org/apache/log4j/Priority' [java] [49.702s][info ][verification] End class verification for: org.apache.commons.logging.impl.Log4JLogger Improved error message: [java] Exception in thread "main" java.lang.InternalError: class redefinition failed: invalid class [java] at java.instrument/sun.instrument.InstrumentationImpl.retransformClasses0(Native Method) [java] at java.instrument/sun.instrument.InstrumentationImpl.retransformClasses(InstrumentationImpl.java:167) [java] at Main.main(Unknown Source) Thanks, Poonam
Re: RFR(S): 8245321: refactor the redefine check that an attribute consisting of a list of classes has not changed
Hi Serguei, The change looks good. Could you add a comment to check_attribute_arrays() saying that its caller should have a ResourceMark? Also, I think that the log_trace arguments at line 724 are in the wrong order. attr_name should be after the_class->external_name(). I don't need to see a new webrev. Thanks, Harold On 6/4/2020 3:20 AM, serguei.spit...@oracle.com wrote: Please, review a fix for: https://bugs.openjdk.java.net/browse/JDK-8245321 Webrev: http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-redef-refact.1/ Summary: The jvmtiRedefineClasses.cpp functions check_nest_attributes and check_permitted_subclasses_attribute have significant common part. This fix is a refactoring which implements this common part into the function check_attribute_arrays. And this function is used in both check_nest_attributes and check_permitted_subclasses_attribute. The check_record_attributes was initially considered to be included into this refactoring. However, it has many differences in layout. I've decided, it is not worth to introduce more complexity into this refactoring in order to support this function as well. But, please. let me know if this function refactoring is still desirable. Testing: Local test runs with the RedefineNestmateAttr and RedefinePermittedSubclassesAttr tests on a Linux server are passed. In progress: submit mach5 jobs with the same Nestmates and PermittedSubclasses tests. Thanks, Serguei
Re: RFR: JDK-8225056 VM support for sealed classes
Hi David,
Thanks for reviewing the latest changes.
I'll create the follow on RFE's once the sealed classes code is in mainline.
Harold
On 5/31/2020 9:34 PM, David Holmes wrote:
Hi Harold,
On 1/06/2020 8:57 am, Harold Seigel wrote:
Thanks for the comments.
Here's version 3 of the JDK and VM changes for sealed classes.
full webrev:
http://cr.openjdk.java.net/~hseigel/sealedClasses.8225056.3/webrev/
The new webrev contains just the following three changes:
1. The sealed classes API's in Class.java (permittedSubclasses() and
isSealed()) were revised and, in particular, API
permittedSubclasses() no longer uses reflection.
For those following along we have presently abandoned the attempt to
cache the array in ReflectionData.
Current changes look okay. But I note from the CSR there appears to be
a further minor update to the javadoc coming.
2. An unneeded 'if' statement was removed from
JVM_GetPermittedSubclasses() (pointed out by David.)
Looks good.
3. VM runtime test files SealedUnnamedModuleIntfTest.java and
Permitted.java were changed to add a test case for a non-public
permitted subclass and its sealed superclass being in the same
module and package.
Looks good.
Additionally, two follow on RFE's will be filed. One to add
additional VM sealed classes tests
Thanks. I think there is a more mechanical approach to testing here
that will allow the complete matrix to be easily covered with minimal
duplication between testing for named and unnamed modules.
and one to improve the implementations of the sealed classes API's in
Class.java.
Thanks.
David
-
Thanks, Harold
On 5/28/2020 8:30 PM, David Holmes wrote:
Hi Harold,
Sorry Mandy's comment raised a couple of issues ...
On 29/05/2020 7:12 am, Mandy Chung wrote:
Hi Harold,
On 5/27/20 1:35 PM, Harold Seigel wrote:
Incremental webrev:
http://cr.openjdk.java.net/~hseigel/sealedClasses.8225056.incr.2/
full webrev:
http://cr.openjdk.java.net/~hseigel/sealedClasses.8225056.2/webrev/
Class.java
4406 ReflectionData rd = reflectionData();
4407 ClassDesc[] tmp = rd.permittedSubclasses;
4408 if (tmp != null) {
4409 return tmp;
4410 }
4411
4412 if (isArray() || isPrimitive()) {
4413 rd.permittedSubclasses = new ClassDesc[0];
4414 return rd.permittedSubclasses;
4415 }
This causes an array class or primitive type to create a
ReflectionData. It should first check if this is non-sealed class
and returns a constant empty array.
It can't check if this is a non-sealed class as the isSealed() check
calls the above code! But for arrays and primitives which can't be
sealed we should just do:
4412 if (isArray() || isPrimitive()) {
4413 return new ClassDesc[0];
4414 }
But this then made me realize that we need to be creating defensive
copies of the returned arrays, as happens with other APIs that use
ReflectionData.
Backing up a bit I complained that:
public boolean isSealed() {
return permittedSubclasses().length != 0;
}
is a very inefficient way to answer the question as to whether a
class is sealed, so I suggested that the result of
permittedSubclasses() be cached. Caching is not without its own
issues as we are discovering, and when you add in defensive copies
this seems to be trading one inefficiency for another. For nestmates
we don't cache getNestMembers() because we don;t think it will be
called often - it is there to complete the introspection API of
Class rather than being anticipated as used in a regular
programmatic sense. I expect the same is true for
permittedSubclasses(). Do we expect isSealed() to be used often or
is it too just there for completeness? If just for completeness then
perhaps a VM query would be a better compromise on the efficiency
front? Otherwise I can accept the current implementation of
isSealed(), and a non-caching permittedClasses() for this initial
implementation of sealed classes. If efficiency turns out to be a
problem for isSealed() then we can revisit it then.
Thanks,
David
In fact, ReflectionData caches the derived names and reflected
members for performance and also they may be invalidated when the
class is redefined. It might be okay to add
ReflectionData::permittedSubclasses while `PermittedSubclasses`
attribute can't be redefined and getting this attribute is not
performance sensitive. For example, the result of
`getNestMembers` is not cached in ReflectionData. It may be better
not to add it in ReflectionData for modifiable and
performance-sensitive data.
4421 tmp = new ClassDesc[subclassNames.length];
4422 int i = 0;
4423 for (String subclassName : subclassNames) {
4424 try {
4425 tmp[i++] = ClassDesc.of(subclassName.replace('/', '.'));
4426 } catch (IllegalArgumentException iae) {
4427 throw new InternalError("Invalid type in permitted subclasses
information: " + subclassName, iae);
4428 }
4429 }
Nit: rename tmp to some other name e.g. descs
I read the JVMS but it isn't clear to me t
Re: RFR: JDK-8225056 VM support for sealed classes
Thanks for the comments.
Here's version 3 of the JDK and VM changes for sealed classes.
full webrev:
http://cr.openjdk.java.net/~hseigel/sealedClasses.8225056.3/webrev/
The new webrev contains just the following three changes:
1. The sealed classes API's in Class.java (permittedSubclasses() and
isSealed()) were revised and, in particular, API
permittedSubclasses() no longer uses reflection.
2. An unneeded 'if' statement was removed from
JVM_GetPermittedSubclasses() (pointed out by David.)
3. VM runtime test files SealedUnnamedModuleIntfTest.java and
Permitted.java were changed to add a test case for a non-public
permitted subclass and its sealed superclass being in the same
module and package.
Additionally, two follow on RFE's will be filed. One to add additional
VM sealed classes tests and one to improve the implementations of the
sealed classes API's in Class.java.
Thanks, Harold
On 5/28/2020 8:30 PM, David Holmes wrote:
Hi Harold,
Sorry Mandy's comment raised a couple of issues ...
On 29/05/2020 7:12 am, Mandy Chung wrote:
Hi Harold,
On 5/27/20 1:35 PM, Harold Seigel wrote:
Incremental webrev:
http://cr.openjdk.java.net/~hseigel/sealedClasses.8225056.incr.2/
full webrev:
http://cr.openjdk.java.net/~hseigel/sealedClasses.8225056.2/webrev/
Class.java
4406 ReflectionData rd = reflectionData();
4407 ClassDesc[] tmp = rd.permittedSubclasses;
4408 if (tmp != null) {
4409 return tmp;
4410 }
4411
4412 if (isArray() || isPrimitive()) {
4413 rd.permittedSubclasses = new ClassDesc[0];
4414 return rd.permittedSubclasses;
4415 }
This causes an array class or primitive type to create a
ReflectionData. It should first check if this is non-sealed class
and returns a constant empty array.
It can't check if this is a non-sealed class as the isSealed() check
calls the above code! But for arrays and primitives which can't be
sealed we should just do:
4412 if (isArray() || isPrimitive()) {
4413 return new ClassDesc[0];
4414 }
But this then made me realize that we need to be creating defensive
copies of the returned arrays, as happens with other APIs that use
ReflectionData.
Backing up a bit I complained that:
public boolean isSealed() {
return permittedSubclasses().length != 0;
}
is a very inefficient way to answer the question as to whether a class
is sealed, so I suggested that the result of permittedSubclasses() be
cached. Caching is not without its own issues as we are discovering,
and when you add in defensive copies this seems to be trading one
inefficiency for another. For nestmates we don't cache
getNestMembers() because we don;t think it will be called often - it
is there to complete the introspection API of Class rather than being
anticipated as used in a regular programmatic sense. I expect the same
is true for permittedSubclasses(). Do we expect isSealed() to be used
often or is it too just there for completeness? If just for
completeness then perhaps a VM query would be a better compromise on
the efficiency front? Otherwise I can accept the current
implementation of isSealed(), and a non-caching permittedClasses() for
this initial implementation of sealed classes. If efficiency turns out
to be a problem for isSealed() then we can revisit it then.
Thanks,
David
In fact, ReflectionData caches the derived names and reflected
members for performance and also they may be invalidated when the
class is redefined. It might be okay to add
ReflectionData::permittedSubclasses while `PermittedSubclasses`
attribute can't be redefined and getting this attribute is not
performance sensitive. For example, the result of `getNestMembers`
is not cached in ReflectionData. It may be better not to add it in
ReflectionData for modifiable and performance-sensitive data.
4421 tmp = new ClassDesc[subclassNames.length];
4422 int i = 0;
4423 for (String subclassName : subclassNames) {
4424 try {
4425 tmp[i++] = ClassDesc.of(subclassName.replace('/', '.'));
4426 } catch (IllegalArgumentException iae) {
4427 throw new InternalError("Invalid type in permitted subclasses
information: " + subclassName, iae);
4428 }
4429 }
Nit: rename tmp to some other name e.g. descs
I read the JVMS but it isn't clear to me that the VM will validate
the names in `PermittedSubclasses`attribute are valid class
descriptors. I see ConstantPool::is_klass_or_reference check but
can't find where it validates the name is a valid class descriptor -
can you point me there? (otherwise, maybe define it to be unspecified?)
W.r.t. the APIs. I don't want to delay this review. I see that you
renamed the method to new API style as I brought up. OTOH, I expect
more discussion is needed. Below is a recent comment from John on
this topic [1]
One comment, really for the future, regarding the shape of the Java
API here: It uses Optional and omits the "get" prefix on accessors.
This is the New Style, as opposed to the Classic Style usi
Re: RFR: JDK-8225056 VM support for sealed classes
Hi Mandy,
The entries in the PermittedSubclasses attribute are constant pool
ConstantClass_info entries. These names get validated by the VM in this
code in ClassFileParser::parse_constant_pool():
for (index = 1; index < length; index++) {
const jbyte tag = cp->tag_at(index).value();
switch (tag) {
case JVM_CONSTANT_UnresolvedClass: {
const Symbol* const class_name = cp->klass_name_at(index);
// check the name, even if _cp_patches will overwrite it
*verify_legal_class_name(class_name, CHECK);*
break;
}
Thanks, Harold
On 5/28/2020 5:12 PM, Mandy Chung wrote:
I read the JVMS but it isn't clear to me that the VM will validate the
names in `PermittedSubclasses`attribute are valid class descriptors.
I see ConstantPool::is_klass_or_reference check but can't find where
it validates the name is a valid class descriptor - can you point me
there? (otherwise, maybe define it to be unspecified?)
Re: RFR: JDK-8225056 VM support for sealed classes
Hi David,
Please review this updated webrev:
Incremental webrev:
http://cr.openjdk.java.net/~hseigel/sealedClasses.8225056.incr.2/
full webrev:
http://cr.openjdk.java.net/~hseigel/sealedClasses.8225056.2/webrev/
It includes the following changes:
* Indentation and simplification changes as suggested below
* If a class is not a valid permitted subclass of its sealed super
then an IncompatibleClassChangeError exception is thrown (as
specified in the JVM Spec) instead of VerifyError.
* Added a check that a non-public subclass must be in the same package
as its sealed super. And added appropriate testing.
* Method Class.permittedSubtypes() was changed.
See also inline comments.
On 5/24/2020 10:28 PM, David Holmes wrote:
Hi Harold,
On 22/05/2020 4:33 am, Harold Seigel wrote:
Hi David,
Thanks for looking at this! Please review this new webrev:
http://cr.openjdk.java.net/~hseigel/webrev.01/webrev/
I'll list all relevant commens here rather than interspersing below so
that it is easier to track. Mostly nits below, other than the
is_permitted_subclass check in the VM, and the use of ReflectionData
in java.lang.Class.
--
src/hotspot/share/classfile/classFileParser.cpp
+ bool ClassFileParser::supports_sealed_types() {
+ return _major_version == JVM_CLASSFILE_MAJOR_VERSION &&
+ _minor_version == JAVA_PREVIEW_MINOR_VERSION &&
+ Arguments::enable_preview();
+ }
Nowe there is too little indentation - the subclauses of the
conjunction expression should align[1]
+ bool ClassFileParser::supports_sealed_types() {
+ return _major_version == JVM_CLASSFILE_MAJOR_VERSION &&
+ _minor_version == JAVA_PREVIEW_MINOR_VERSION &&
+ Arguments::enable_preview();
+ }
Fixed. Along with indentation of supports_records().
3791 if (parsed_permitted_subclasses_attribute) {
3792 classfile_parse_error("Multiple
PermittedSubclasses attributes in class file %s", CHECK);
3793 // Classes marked ACC_FINAL cannot have a
PermittedSubclasses attribute.
3794 } else if (_access_flags.is_final()) {
3795 classfile_parse_error("PermittedSubclasses
attribute in final class file %s", CHECK);
3796 } else {
3797 parsed_permitted_subclasses_attribute = true;
3798 }
The indent of the comment at L3793 is wrong, and its placement is
awkward because it relates to the next condition. But we don't have to
use if-else here as any parse error results in immediate return due to
the CHECK macro. So the above can be reformatted as:
3791 if (parsed_permitted_subclasses_attribute) {
3792 classfile_parse_error("Multiple
PermittedSubclasses attributes in class file %s", CHECK);
3793 }
3794 // Classes marked ACC_FINAL cannot have a
PermittedSubclasses attribute.
3795 if (_access_flags.is_final()) {
3796 classfile_parse_error("PermittedSubclasses
attribute in final class file %s", CHECK);
3797 }
3798 parsed_permitted_subclasses_attribute = true;
Done.
---
src/hotspot/share/oops/instanceKlass.cpp
The logic in InstanceKlass::has_as_permitted_subclass still does not
implement the rules specified in the JVMS. It only implements a "same
module" check, whereas the JVMS specifies an accessibility requirement
as well.
Done.
730 bool InstanceKlass::is_sealed() const {
731 return _permitted_subclasses != NULL &&
732 _permitted_subclasses !=
Universe::the_empty_short_array() &&
733 _permitted_subclasses->length() > 0;
734 }
Please align subclauses.
Done.
---
src/hotspot/share/prims/jvm.cpp
2159 objArrayHandle result (THREAD, r);
Please remove space after "result".
Done.
As we will always create and return an arry, if you reverse these two
statements:
2156 if (length != 0) {
2157 objArrayOop r =
oopFactory::new_objArray(SystemDictionary::String_klass(),
2158 length, CHECK_NULL);
and these two:
2169 return (jobjectArray)JNIHandles::make_local(THREAD, result());
2170 }
then you can delete
2172 // if it gets to here return an empty array, cases will be: the
class is primitive, or an array, or just not sealed
2173 objArrayOop result =
oopFactory::new_objArray(SystemDictionary::String_klass(), 0,
CHECK_NULL);
2174 return (jobjectArray)JNIHandles::make_local(env, result);
The comment there is no longer accurate anyway.
Done.
---
src/hotspot/share/prims/jvmtiRedefineClasses.cpp
857 static jvmtiError
check_permitted_subclasses_attribute(InstanceKlass* the_class,
858 InstanceKlass* scratch_class) {
Please align.
Done.
---
src/hotspot/share/prims/jvmtiRedefineClasses
Re: RFR: JDK-8225056 VM support for sealed classes
Thanks Lois!
I'll add the two ResourceMarks before the changes get pushed.
Harold
On 5/22/2020 11:07 AM, Lois Foltan wrote:
On 5/21/2020 2:33 PM, Harold Seigel wrote:
Hi David,
Thanks for looking at this! Please review this new webrev:
http://cr.openjdk.java.net/~hseigel/webrev.01/webrev/
Hi Harold,
I think this webrev looks good! A couple of minor comments:
- oops/instanceKlass.cpp:
line #236, do you need a ResourceMark here? I noticed there is one
above at line #229 for the log_trace call that uses external_name().
- prims/jvmtiRedefineClasses.cpp:
line #878, I think you need a ResourceMark for this method as well
if you invoke the external_name for the log_trace calls and for
NEW_RESOURCE_ARRAY_RETURN_NULL()?
Tests look good.
Thanks,
Lois
This webrev contains the following significant changes:
1. The format/indentation issues in classFileParser.cpp were fixed.
2. Unneeded checks in InstanceKlass::has_as_permitted_subclass() were
removed and the TRAPS parameter was removed.
3. The changes to klassVtable.* and method.* were reverted. Those
changes were from when sealed classes were marked as final, and are
no longer valid.
4. Method getPermittedSubclasses() in Class.java was renamed to
permittedSubclasses() and its implementation was changed.
5. Variables and methods for 'asm' were renamed from
'permittedSubtypes' to 'permittedSubclasses'.
6. Classes for sealed classes tests were changed to start with capital
letters.
7. Changes to langtools tests were removed from this webrev. They are
covered by the javac webrev (JDK-8244556).
8. The CSR's for JVMTI, JDWP, and JDI are in progress.
Please also see comments inline.
On 5/19/2020 1:26 AM, David Holmes wrote:
Hi Harold,
Adding serviceability-dev for the serviceability related changes.
Nit: "VM support for sealed classes"
This RFR covers the VM, core-libs, serviceability and even some
langtools tests. AFAICS only the javac changes are not included here
so when and where will they be reviewed and under what bug id?
Ideally there will be a single JBS issue for "Implementation of JEP
360: Sealed types". It's okay to break up the RFRs across different
areas, but it should be done under one bug id.
The javac changes are being reviewed as bug JDK-8227406. We
understand the need to do the reviews under one bug.
Overall this looks good. I've looked at all files and mainly have
some style nits in various places. But there are some more
significant items below.
On 14/05/2020 7:09 am, Harold Seigel wrote:
Hi,
Please review this patch for JVM and Core-libs support for the JEP
360 Sealed Classes preview feature. Code changes include the
following:
* Processing of the new PermittedSubclasses attribute to enforce
that
a class or interface, whose super is a sealed class or interface,
must be listed in the super's PermittedSubclasses attribute.
* Disallow redefinition of a sealed class or interface if its
redefinition would change its PermittedSubclasses attribute.
* Support API's to determine if a class or interface is sealed
and, if
it's sealed, return a list of its permitted subclasses.
* asm support for the PermittedSubclasses attribute
I assume Remi is providing the upstream support in ASM? :) But also
see below ...
Open Webrev:
http://cr.openjdk.java.net/~vromero/8225056/webrev.00/index.html
make/data/jdwp/jdwp.spec
There needs to be a sub-task and associated CSR request for this
JDWP spec update. I couldn't see this covered anywhere.
---
src/hotspot/share/classfile/classFileParser.cpp
3215 u2
ClassFileParser::parse_classfile_permitted_subclasses_attribute(const
ClassFileStream* const cfs,
3216 const u1* const permitted_subclasses_attribute_start,
3217 TRAPS) {
Indention on L3216/17 needs fixing after the copy'n'edit.
3515 return _major_version == JVM_CLASSFILE_MAJOR_VERSION &&
3516 _minor_version == JAVA_PREVIEW_MINOR_VERSION &&
3517 Arguments::enable_preview();
Too much indentation on L3516/17
3790 // Check for PermittedSubclasses tag
That comment (copied from my nestmates code :) is in the wrong
place. It needs to be before
3788 if (tag == vmSymbols::tag_permitted_subclasses()) {
Minor nit: I would suggest checking
parsed_permitted_subclasses_attribute before checking ACC_FINAL.
3876 if (parsed_permitted_subclasses_attribute) {
3877 const u2 num_of_subclasses =
parse_classfile_permitted_subclasses_attribute(
3878 cfs,
3879 permitted_subclasses_attribute_start,
3880 CHECK);
Although it looks odd the preceding, similarly shaped, sections all
indent to the same absolute position. Can you make L3878/78/80 match
please.
3882 guarantee_property(
3883 permitted_subclasses_attribute_length ==
3884 sizeof(num_of_subclasses) + s
Re: RFR: JDK-8225056 VM support for sealed classes
Hi Mandy,
Thanks for the suggestions. They have been incorporated in the revised
webrev.
http://cr.openjdk.java.net/~hseigel/webrev.01/webrev/
Harold
On 5/20/2020 1:05 PM, Mandy Chung wrote:
Hi Vicente,
On 5/20/20 8:40 AM, Vicente Romero wrote:
Hi David,
src/java.base/share/classes/java/lang/Class.java
There needs to be a CSR request for these changes.
yes there is one already:
https://bugs.openjdk.java.net/browse/JDK-8244556
Adding to David's comment w.r.t. @throws IAE:
The Class::getXXX APIs returns `Class` or `Class[]` if the result is
about type(s). This new `getPermittedSubclasses` returns
`ClassDesc[]`. I wonder if this should be renamed to
`permittedSubclasses` to follow the convention of the new APIs added
to support descriptors e.g. `describeConstable`
Nit: {@linkplain Class} should be {@code Class}
Mandy
