Re: RFR: 8329433: Reduce nmethod header size [v4]
> This is part of changes which try to reduce size of `nmethod` and `codeblob` > data vs code in CodeCache. > These changes reduced size of `nmethod` header from 288 to 232 bytes. From > 304 to 248 in optimized VM: > > Statistics for 1282 bytecoded nmethods for C2: > total in heap = 5560352 (100%) > header = 389728 (7.009053%) > > vs > > Statistics for 1322 bytecoded nmethods for C2: > total in heap = 8307120 (100%) > header = 327856 (3.946687%) > > > Several unneeded fields in `nmethod` and `CodeBlob` were removed. Some fields > were changed from `int` to `int16_t` with added corresponding asserts to make > sure their values are fit into 16 bits. > > I did additional cleanup after recent `CompiledMethod` removal. > > Tested tier1-7,stress,xcomp and performance testing. Vladimir Kozlov has updated the pull request incrementally with one additional commit since the last revision: Use 16-bits types for header_size and frame_complete_offset arguments - Changes: - all: https://git.openjdk.org/jdk/pull/18768/files - new: https://git.openjdk.org/jdk/pull/18768/files/13744e78..6cb22e81 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18768=03 - incr: https://webrevs.openjdk.org/?repo=jdk=18768=02-03 Stats: 22 lines in 2 files changed: 0 ins; 2 del; 20 mod Patch: https://git.openjdk.org/jdk/pull/18768.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18768/head:pull/18768 PR: https://git.openjdk.org/jdk/pull/18768
Re: RFR: 8329433: Reduce nmethod header size [v3]
On Tue, 16 Apr 2024 02:28:14 GMT, Dean Long wrote: >> Vladimir Kozlov has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Union fields which usages do not overlap > > src/hotspot/share/code/nmethod.hpp line 282: > >> 280: _has_flushed_dependencies:1, // Used for maintenance of >> dependencies (under CodeCache_lock) >> 281: _is_unlinked:1, // mark during class unloading >> 282: _load_reported:1;// used by jvmti to track if an >> event has been posted for this nmethod > > It seems like the type could be changed from uint8_t to bool. Is there difference in generated code when you use bool instead of uint8_t? I used uint8_t to easy change to uint16_t in a future if needed. - PR Review Comment: https://git.openjdk.org/jdk/pull/18768#discussion_r1566653582
Re: RFR: 8329433: Reduce nmethod header size [v3]
On Tue, 16 Apr 2024 02:34:29 GMT, Dean Long wrote: >> Vladimir Kozlov has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Union fields which usages do not overlap > > src/hotspot/share/code/nmethod.hpp line 205: > >> 203: // offsets to find the receiver for non-static native wrapper >> frames. >> 204: ByteSize _native_receiver_sp_offset; >> 205: ByteSize _native_basic_lock_sp_offset; > > Don't we need an assert in the accessor functions to make sure nmethod is > native or not? I thought about that but in both places where these accessors are called (`frame::get_native_monitor()` and `frame::get_native_receiver()`) there are such asserts already: https://github.com/openjdk/jdk/blob/master/src/hotspot/share/runtime/frame.cpp#L1085 - PR Review Comment: https://git.openjdk.org/jdk/pull/18768#discussion_r1566650267
Re: RFR: 8329433: Reduce nmethod header size [v3]
On Mon, 15 Apr 2024 03:24:07 GMT, Vladimir Kozlov wrote: >> This is part of changes which try to reduce size of `nmethod` and `codeblob` >> data vs code in CodeCache. >> These changes reduced size of `nmethod` header from 288 to 232 bytes. From >> 304 to 248 in optimized VM: >> >> Statistics for 1282 bytecoded nmethods for C2: >> total in heap = 5560352 (100%) >> header = 389728 (7.009053%) >> >> vs >> >> Statistics for 1322 bytecoded nmethods for C2: >> total in heap = 8307120 (100%) >> header = 327856 (3.946687%) >> >> >> Several unneeded fields in `nmethod` and `CodeBlob` were removed. Some >> fields were changed from `int` to `int16_t` with added corresponding asserts >> to make sure their values are fit into 16 bits. >> >> I did additional cleanup after recent `CompiledMethod` removal. >> >> Tested tier1-7,stress,xcomp and performance testing. > > Vladimir Kozlov has updated the pull request incrementally with one > additional commit since the last revision: > > Union fields which usages do not overlap src/hotspot/share/code/nmethod.hpp line 205: > 203: // offsets to find the receiver for non-static native wrapper > frames. > 204: ByteSize _native_receiver_sp_offset; > 205: ByteSize _native_basic_lock_sp_offset; Don't we need an assert in the accessor functions to make sure nmethod is native or not? - PR Review Comment: https://git.openjdk.org/jdk/pull/18768#discussion_r1566634384
Re: RFR: 8329433: Reduce nmethod header size [v3]
On Mon, 15 Apr 2024 03:24:07 GMT, Vladimir Kozlov wrote: >> This is part of changes which try to reduce size of `nmethod` and `codeblob` >> data vs code in CodeCache. >> These changes reduced size of `nmethod` header from 288 to 232 bytes. From >> 304 to 248 in optimized VM: >> >> Statistics for 1282 bytecoded nmethods for C2: >> total in heap = 5560352 (100%) >> header = 389728 (7.009053%) >> >> vs >> >> Statistics for 1322 bytecoded nmethods for C2: >> total in heap = 8307120 (100%) >> header = 327856 (3.946687%) >> >> >> Several unneeded fields in `nmethod` and `CodeBlob` were removed. Some >> fields were changed from `int` to `int16_t` with added corresponding asserts >> to make sure their values are fit into 16 bits. >> >> I did additional cleanup after recent `CompiledMethod` removal. >> >> Tested tier1-7,stress,xcomp and performance testing. > > Vladimir Kozlov has updated the pull request incrementally with one > additional commit since the last revision: > > Union fields which usages do not overlap src/hotspot/share/code/nmethod.hpp line 282: > 280: _has_flushed_dependencies:1, // Used for maintenance of > dependencies (under CodeCache_lock) > 281: _is_unlinked:1, // mark during class unloading > 282: _load_reported:1;// used by jvmti to track if an > event has been posted for this nmethod It seems like the type could be changed from uint8_t to bool. - PR Review Comment: https://git.openjdk.org/jdk/pull/18768#discussion_r1566631312
Re: RFR: 8329433: Reduce nmethod header size [v3]
On Tue, 16 Apr 2024 01:30:50 GMT, Dean Long wrote: >> Vladimir Kozlov has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Union fields which usages do not overlap > > src/hotspot/share/code/codeBlob.cpp line 88: > >> 86: S390_ONLY(_ctable_offset(0) COMMA) >> 87: _header_size((uint16_t)header_size), >> 88: _frame_complete_offset((int16_t)frame_complete_offset), > > Rather than a raw cast, it would be better to use checked_cast here, or > better yet, change the incoming parameter types to match the field type. > That way, if the caller is passing a constant, the compiler can check it at > compile time. Agree and will do. In all case `sizeof(_Class_)` is used for `header_size`. - PR Review Comment: https://git.openjdk.org/jdk/pull/18768#discussion_r1566620168
Re: RFR: 8322043: HeapDumper should use parallel dump by default [v3]
On Mon, 15 Apr 2024 23:18:54 GMT, Alex Menkov wrote: >> The fix makes VM heap dumping parallel by default. >> `jcmd GC.heap_dump` and `jmap -dump` had parallel dumping by default, the >> fix affects `HotSpotDiagnosticMXBean.dumpHeap()`, >> `-XX:+HeapDumpBeforeFullGC`, `-XX:+HeapDumpAfterFullGC` and >> `-XX:+HeapDumpOnOutOfMemoryError`. >> >> Testing: >> - manually tested different heap dump scenarios with `-Xlog:heapdump`; >> - tier1,tier2,hs-tier5-svc; >> - all reg.tests that use heap dump. > > Alex Menkov has updated the pull request incrementally with one additional > commit since the last revision: > > check free_memory for OOME Looks good. Thanks for doing this! - Marked as reviewed by yyang (Committer). PR Review: https://git.openjdk.org/jdk/pull/18748#pullrequestreview-2002450373
Re: RFR: 8329433: Reduce nmethod header size [v3]
On Mon, 15 Apr 2024 03:24:07 GMT, Vladimir Kozlov wrote: >> This is part of changes which try to reduce size of `nmethod` and `codeblob` >> data vs code in CodeCache. >> These changes reduced size of `nmethod` header from 288 to 232 bytes. From >> 304 to 248 in optimized VM: >> >> Statistics for 1282 bytecoded nmethods for C2: >> total in heap = 5560352 (100%) >> header = 389728 (7.009053%) >> >> vs >> >> Statistics for 1322 bytecoded nmethods for C2: >> total in heap = 8307120 (100%) >> header = 327856 (3.946687%) >> >> >> Several unneeded fields in `nmethod` and `CodeBlob` were removed. Some >> fields were changed from `int` to `int16_t` with added corresponding asserts >> to make sure their values are fit into 16 bits. >> >> I did additional cleanup after recent `CompiledMethod` removal. >> >> Tested tier1-7,stress,xcomp and performance testing. > > Vladimir Kozlov has updated the pull request incrementally with one > additional commit since the last revision: > > Union fields which usages do not overlap src/hotspot/share/code/codeBlob.cpp line 120: > 118: > 119: _header_size((uint16_t)header_size), > 120: _frame_complete_offset((int16_t)CodeOffsets::frame_never_safe), See above. - PR Review Comment: https://git.openjdk.org/jdk/pull/18768#discussion_r1566604310
Re: RFR: 8329433: Reduce nmethod header size [v3]
On Mon, 15 Apr 2024 03:24:07 GMT, Vladimir Kozlov wrote: >> This is part of changes which try to reduce size of `nmethod` and `codeblob` >> data vs code in CodeCache. >> These changes reduced size of `nmethod` header from 288 to 232 bytes. From >> 304 to 248 in optimized VM: >> >> Statistics for 1282 bytecoded nmethods for C2: >> total in heap = 5560352 (100%) >> header = 389728 (7.009053%) >> >> vs >> >> Statistics for 1322 bytecoded nmethods for C2: >> total in heap = 8307120 (100%) >> header = 327856 (3.946687%) >> >> >> Several unneeded fields in `nmethod` and `CodeBlob` were removed. Some >> fields were changed from `int` to `int16_t` with added corresponding asserts >> to make sure their values are fit into 16 bits. >> >> I did additional cleanup after recent `CompiledMethod` removal. >> >> Tested tier1-7,stress,xcomp and performance testing. > > Vladimir Kozlov has updated the pull request incrementally with one > additional commit since the last revision: > > Union fields which usages do not overlap src/hotspot/share/code/codeBlob.cpp line 88: > 86: S390_ONLY(_ctable_offset(0) COMMA) > 87: _header_size((uint16_t)header_size), > 88: _frame_complete_offset((int16_t)frame_complete_offset), Rather than a raw cast, it would be better to use checked_cast here, or better yet, change the incoming parameter types to match the field type. That way, if the caller is passing a constant, the compiler can check it at compile time. - PR Review Comment: https://git.openjdk.org/jdk/pull/18768#discussion_r1566601934
Re: RFR: 8188784: javax/management/notification/BroadcasterSupportDeadlockTest.java - TEST FAILED: deadlock
On Tue, 9 Apr 2024 11:08:31 GMT, Kevin Walls wrote:
> This test incorrectly fails, although rarely, thinking its "thread 2" has
> deadlocked.
> A change of sleep will likely fix this, but there are other issues, so
> cleaning up the test a little.
>
> Remove the probe for the ManagementFactory class, to check we are on jdk5 or
> later. 8-)
>
> When sleeping, sleep 100, not 1ms, we don't need to spin fast and actually
> race with the other thread.
>
> We have a 1000 iteration loop, but don't seem to use it. We only check once
> then either return (pass), fail, or break (which is also fail). Use the loop
> to check for the status change, which is likely what was intended.
>
> Show the stackframes on all failures.
Marked as reviewed by lmesnik (Reviewer).
test/jdk/javax/management/notification/BroadcasterSupportDeadlockTest.java line
122:
> 120: java.util.Map traces =
> Thread.getAllStackTraces();
> 121: showStackTrace("Thread 1", traces.get(t1));
> 122: showStackTrace("Thread 2", traces.get(t2));
Could you please replace System.exit() with throwing Exception. Other looks
good.
-
PR Review: https://git.openjdk.org/jdk/pull/18687#pullrequestreview-2002384686
PR Review Comment: https://git.openjdk.org/jdk/pull/18687#discussion_r1566573453
Re: RFR: 8322043: HeapDumper should use parallel dump by default [v3]
> The fix makes VM heap dumping parallel by default. > `jcmd GC.heap_dump` and `jmap -dump` had parallel dumping by default, the fix > affects `HotSpotDiagnosticMXBean.dumpHeap()`, `-XX:+HeapDumpBeforeFullGC`, > `-XX:+HeapDumpAfterFullGC` and `-XX:+HeapDumpOnOutOfMemoryError`. > > Testing: > - manually tested different heap dump scenarios with `-Xlog:heapdump`; > - tier1,tier2,hs-tier5-svc; > - all reg.tests that use heap dump. Alex Menkov has updated the pull request incrementally with one additional commit since the last revision: check free_memory for OOME - Changes: - all: https://git.openjdk.org/jdk/pull/18748/files - new: https://git.openjdk.org/jdk/pull/18748/files/7ae4806e..f6db604f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18748=02 - incr: https://webrevs.openjdk.org/?repo=jdk=18748=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18748.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18748/head:pull/18748 PR: https://git.openjdk.org/jdk/pull/18748
Re: RFR: 8328741: serviceability/jvmti/ObjectMonitorUsage/ObjectMonitorUsage.java failed with unexpected owner
On Mon, 15 Apr 2024 06:47:24 GMT, Serguei Spitsyn wrote:
> This is the test issue. The `WaitingPT3` thread posted the `MonitorWait`
> event but has not released the `lockCheck` monitor yet. It has been fixed to
> wait for each `WaitingTask` thread to really reach the `WAITING` state. The
> same approach is used for `EnteringTask` threads. It has been fixed to wait
> for each `EnteringTask` thread to reach the `BLOCKED_ON_MONITOR` state.
>
> Testing:
> - Reran the test
> `serviceability/jvmti/ObjectMonitorUsage/ObjectMonitorUsage.java` locally
> - TBD: submit the mach5 tiers 1-6 as well
Changes requested by lmesnik (Reviewer).
test/hotspot/jtreg/serviceability/jvmti/ObjectMonitorUsage/libObjectMonitorUsage.cpp
line 183:
> 181:
> 182: static void wait_for_state(JNIEnv *jni, jthread thread, jint exp_state) {
> 183: RawMonitorLocker rml(jvmti, jni, event_lock);
The event_lock name is misleading, there are no events anymore.
Also, I am not sure if this lock is needed at all. How it is used?
I think that wait_for_state is a good candidate to be added in the library.
With some additional doc about states.
-
PR Review: https://git.openjdk.org/jdk/pull/18778#pullrequestreview-2002207716
PR Review Comment: https://git.openjdk.org/jdk/pull/18778#discussion_r1566464641
Re: RFR: 8322043: HeapDumper should use parallel dump by default [v2]
> The fix makes VM heap dumping parallel by default. > `jcmd GC.heap_dump` and `jmap -dump` had parallel dumping by default, the fix > affects `HotSpotDiagnosticMXBean.dumpHeap()`, `-XX:+HeapDumpBeforeFullGC`, > `-XX:+HeapDumpAfterFullGC` and `-XX:+HeapDumpOnOutOfMemoryError`. > > Testing: > - manually tested different heap dump scenarios with `-Xlog:heapdump`; > - tier1,tier2,hs-tier5-svc; > - all reg.tests that use heap dump. Alex Menkov has updated the pull request incrementally with one additional commit since the last revision: check free_memory for OOME - Changes: - all: https://git.openjdk.org/jdk/pull/18748/files - new: https://git.openjdk.org/jdk/pull/18748/files/8a66572e..7ae4806e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18748=01 - incr: https://webrevs.openjdk.org/?repo=jdk=18748=00-01 Stats: 10 lines in 1 file changed: 10 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/18748.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18748/head:pull/18748 PR: https://git.openjdk.org/jdk/pull/18748
Re: RFR: 8330171: Lazy W^X switch implementation
On Mon, 15 Apr 2024 08:33:25 GMT, Bernhard Urban-Forster wrote: > do you have numbers on how many transitions are done with your PR vs. the > current state when running the same program? With just simple **java -version** it is ~180 vs ~9500 (new vs old), for **java -help** ~1120 vs ~86300. For the applications the ration is about the same. - PR Comment: https://git.openjdk.org/jdk/pull/18762#issuecomment-2057280998
RFR: 8328741: serviceability/jvmti/ObjectMonitorUsage/ObjectMonitorUsage.java failed with unexpected owner
This is the test issue. The `WaitingPT3` thread posted the `MonitorWait` event but has not released the `lockCheck` monitor yet. It has been fixed to wait for each `WaitingTask` thread to really reach the `WAITING` state. The same approach is used for `EnteringTask` threads. It has been fixed to wait for each `EnteringTask` thread to reach the `BLOCKED_ON_MONITOR` state. Testing: - Reran the test `serviceability/jvmti/ObjectMonitorUsage/ObjectMonitorUsage.java` locally - TBD: submit the mach5 tiers 1-6 as well - Commit messages: - remove trailing spaces for a couple of lines - 8328741: serviceability/jvmti/ObjectMonitorUsage/ObjectMonitorUsage.java failed with unexpected owner Changes: https://git.openjdk.org/jdk/pull/18778/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18778=00 Issue: https://bugs.openjdk.org/browse/JDK-8328741 Stats: 90 lines in 2 files changed: 11 ins; 67 del; 12 mod Patch: https://git.openjdk.org/jdk/pull/18778.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18778/head:pull/18778 PR: https://git.openjdk.org/jdk/pull/18778
Re: RFR: 8330171: Lazy W^X switch implementation
On Fri, 12 Apr 2024 14:40:05 GMT, Sergey Nazarkin wrote: > An alternative for preemptively switching the W^X thread mode on macOS with > an AArch64 CPU. This implementation triggers the switch in response to the > SIGBUS signal if the *si_addr* belongs to the CodeCache area. With this > approach, it is now feasible to eliminate all WX guards and avoid potentially > costly operations. However, no significant improvement or degradation in > performance has been observed. Additionally, considering the issue with > AsyncGetCallTrace, the patched JVM has been successfully operated with > [asgct_bottom](https://github.com/parttimenerd/asgct_bottom) and > [async-profiler](https://github.com/async-profiler/async-profiler). > > Additional testing: > - [x] MacOS AArch64 server fastdebug *gtets* > - [ ] MacOS AArch64 server fastdebug *jtreg:hotspot:tier4* > - [ ] Benchmarking > > @apangin and @parttimenerd could you please check the patch on your > scenarios?? I agree that this PR effectively removes the whole idea behind JIT_MAP and thus is a bad idea security wise. Still it has some value. @snazarkin do you have numbers on how many transitions are done with your PR vs. the current state when running the same program? That would give us a lower bound on the amount of transitions needed and define a goal for future improvements in that area. - PR Comment: https://git.openjdk.org/jdk/pull/18762#issuecomment-2056182560
