Re: RFR: 8322043: HeapDumper should use parallel dump by default [v3]
On Tue, 16 Apr 2024 19:45:12 GMT, Alex Menkov wrote: >> src/hotspot/share/services/heapDumper.hpp line 63: >> >>> 61: // additional info is written to out if not null. >>> 62: // compression >= 0 creates a gzipped file with the given compression >>> level. >>> 63: // parallel_thread_num >= 0 indicates thread numbers of parallel >>> object dump, -1 means "auto select". >> >> I don't understand why you need to add `-1` to mean "auto-select" instead of >> just setting the default parameter to be `default_num_of_dump_threads()`? > > I think it makes the code more flexible - it allows to distinguish between > "use default value" and "I don't care" cases. I'm not sure it is a worthwhile distinction. Not passing an actual parameter means "I don't care - take the default". - PR Review Comment: https://git.openjdk.org/jdk/pull/18748#discussion_r1568350751
Re: RFR: 8322043: HeapDumper should use parallel dump by default [v3]
On Wed, 17 Apr 2024 07:22:55 GMT, David Holmes wrote: >> I think it makes the code more flexible - it allows to distinguish between >> "use default value" and "I don't care" cases. > > I'm not sure it is a worthwhile distinction. Not passing an actual parameter > means "I don't care - take the default". I agree with @dholmes-ora. Let's keep things simple. - PR Review Comment: https://git.openjdk.org/jdk/pull/18748#discussion_r1568833440
Re: RFR: 8329433: Reduce nmethod header size [v6]
On Wed, 17 Apr 2024 00:56:33 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 two > additional commits since the last revision: > > - remove trailing space > - Shuffle fields initialization src/hotspot/share/code/nmethod.hpp line 259: > 257: int _orig_pc_offset; > 258: > 259: int _compile_id;// which compilation made this > nmethod NIT: are these fields always needed? - PR Review Comment: https://git.openjdk.org/jdk/pull/18768#discussion_r1569185473
Re: RFR: 8329433: Reduce nmethod header size [v6]
On Wed, 17 Apr 2024 17:10:50 GMT, Cesar Soares Lucas wrote: >> Vladimir Kozlov has updated the pull request incrementally with two >> additional commits since the last revision: >> >> - remove trailing space >> - Shuffle fields initialization > > src/hotspot/share/code/nmethod.hpp line 259: > >> 257: int _orig_pc_offset; >> 258: >> 259: int _compile_id;// which compilation made this >> nmethod > > NIT: are these fields always needed? Yes, they are needed for debugging issues. They are important for error reporting, logs and events recording. And they do not take much space: CompLevel and CompilerType are one byte size. - PR Review Comment: https://git.openjdk.org/jdk/pull/18768#discussion_r1569237659
RFR: 8330388: Remove invokedynamic cache index encoding
Before [JDK-8307190](https://bugs.openjdk.org/browse/JDK-8307190), [JDK-8309673](https://bugs.openjdk.org/browse/JDK-8309673), and [JDK-8301995](https://bugs.openjdk.org/browse/JDK-8301995), invokedynamic operands needed to be rewritten to encoded values to better distinguish indy entries from other cp cache entries. The above changes now distinguish between entries with `to_cp_index()` using the bytecode, which is now propagated by the callers. The encoding flips the bits of the index so the encoded index is always negative, leading to access errors if there is no matching decode call. These calls are removed with some methods adjusted to distinguish between indices with the bytecode. Verified with tier 1-5 tests. - Commit messages: - 8330388: Remove invokedynamic cache index encoding Changes: https://git.openjdk.org/jdk/pull/18819/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18819=00 Issue: https://bugs.openjdk.org/browse/JDK-8330388 Stats: 220 lines in 37 files changed: 15 ins; 136 del; 69 mod Patch: https://git.openjdk.org/jdk/pull/18819.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18819/head:pull/18819 PR: https://git.openjdk.org/jdk/pull/18819
Re: RFR: 8328741: serviceability/jvmti/ObjectMonitorUsage/ObjectMonitorUsage.java failed with unexpected owner [v3]
On Tue, 16 Apr 2024 21:55:54 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 > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > review: replaced wait with timeout with sleep_ms; fixed one test to use > sleep_sec from test lib Marked as reviewed by lmesnik (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18778#pullrequestreview-2007023177
RFR: 8330534: Update nsk/jdwp tests to use driver instead of othervm
The jdwp tests use debugger and debugee. There is no goal to execute debugger part with all VM flags, they are needed to be used with debugee VM only. The change is all tests is to don't use System.exit() and use 'driver' instead of othervm. test/hotspot/jtreg/vmTestbase/nsk/share/jpda/DebugeeBinder.java is updated to correctly set classpath for debugee - Commit messages: - 8330534: Update nsk/jdwp tests to use driver instead of othervm Changes: https://git.openjdk.org/jdk/pull/18826/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18826=00 Issue: https://bugs.openjdk.org/browse/JDK-8330534 Stats: 824 lines in 219 files changed: 342 ins; 0 del; 482 mod Patch: https://git.openjdk.org/jdk/pull/18826.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18826/head:pull/18826 PR: https://git.openjdk.org/jdk/pull/18826
Re: RFR: 8329433: Reduce nmethod header size [v3]
On Tue, 16 Apr 2024 16:09:21 GMT, Vladimir Kozlov wrote: >> src/hotspot/share/code/nmethod.cpp line 1441: >> >>> 1439: int deps_size = align_up((int)dependencies->size_in_bytes(), >>> oopSize); >>> 1440: int sum_size = oops_size + metadata_size + deps_size; >>> 1441: assert((sum_size >> 16) == 0, "data size is bigger than 64Kb: >>> %d", sum_size); >> >> I suggest using checked_cast for the assignment below, rather than >> special-purpose checks here. > > Okay. But I will put above code under `#ifdef ASSERT` then. The ASSERT block above looks unnecessary, now that field assignments below are using checked_cast. - PR Review Comment: https://git.openjdk.org/jdk/pull/18768#discussion_r1569496753
Re: RFR: 8329433: Reduce nmethod header size [v7]
> 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 with a new target base due to a merge or a rebase. The pull request now contains eight commits: - Merge master - remove trailing space - Shuffle fields initialization - Address comments. Used checked_cast. - Use 16-bits types for header_size and frame_complete_offset arguments - Union fields which usages do not overlap - Moved some fields initialization into init_defaults() - 8329433: Reduce nmethod header size - Changes: https://git.openjdk.org/jdk/pull/18768/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18768=06 Stats: 528 lines in 15 files changed: 140 ins; 178 del; 210 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 [v7]
On Wed, 17 Apr 2024 22:23:47 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 with a new target base due to a > merge or a rebase. The pull request now contains eight commits: > > - Merge master > - remove trailing space > - Shuffle fields initialization > - Address comments. Used checked_cast. > - Use 16-bits types for header_size and frame_complete_offset arguments > - Union fields which usages do not overlap > - Moved some fields initialization into init_defaults() > - 8329433: Reduce nmethod header size I remove `ASSERT` blocks to address the last @dean-long comment. - PR Comment: https://git.openjdk.org/jdk/pull/18768#issuecomment-2062784402
Re: RFR: 8322043: HeapDumper should use parallel dump by default [v5]
> 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: removed unneeded cast - Changes: - all: https://git.openjdk.org/jdk/pull/18748/files - new: https://git.openjdk.org/jdk/pull/18748/files/482d9ffe..641bedc5 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18748=04 - incr: https://webrevs.openjdk.org/?repo=jdk=18748=03-04 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: 8330388: Remove invokedynamic cache index encoding
On Wed, 17 Apr 2024 15:26:52 GMT, Matias Saavedra Silva wrote: > Before [JDK-8307190](https://bugs.openjdk.org/browse/JDK-8307190), > [JDK-8309673](https://bugs.openjdk.org/browse/JDK-8309673), and > [JDK-8301995](https://bugs.openjdk.org/browse/JDK-8301995), invokedynamic > operands needed to be rewritten to encoded values to better distinguish indy > entries from other cp cache entries. The above changes now distinguish > between entries with `to_cp_index()` using the bytecode, which is now > propagated by the callers. > > The encoding flips the bits of the index so the encoded index is always > negative, leading to access errors if there is no matching decode call. These > calls are removed with some methods adjusted to distinguish between indices > with the bytecode. Verified with tier 1-5 tests. SA changes look good. - Marked as reviewed by cjplummer (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18819#pullrequestreview-2007176260
Re: RFR: 8329433: Reduce nmethod header size [v3]
On Wed, 17 Apr 2024 20:27:53 GMT, Dean Long wrote: >> Okay. But I will put above code under `#ifdef ASSERT` then. > > The ASSERT block above looks unnecessary, now that field assignments below > are using checked_cast. Agree, but I need to change how I use checked_cast below to get the same check as above. I will do it in next update: _dependencies_offset = _metadata_offset + checked_cast(align_up(code_buffer->total_metadata_size(), wordSize)); _scopes_pcs_offset= _dependencies_offset + checked_cast(align_up((int)dependencies->size_in_bytes(), oopSize)); --- _dependencies_offset = checked_cast(_metadata_offset + align_up(code_buffer->total_metadata_size(), wordSize)); _scopes_pcs_offset= checked_cast(_dependencies_offset + align_up((int)dependencies->size_in_bytes(), oopSize)); - PR Review Comment: https://git.openjdk.org/jdk/pull/18768#discussion_r1569563003
Re: RFR: 8330388: Remove invokedynamic cache index encoding
On Wed, 17 Apr 2024 15:26:52 GMT, Matias Saavedra Silva
wrote:
> Before [JDK-8307190](https://bugs.openjdk.org/browse/JDK-8307190),
> [JDK-8309673](https://bugs.openjdk.org/browse/JDK-8309673), and
> [JDK-8301995](https://bugs.openjdk.org/browse/JDK-8301995), invokedynamic
> operands needed to be rewritten to encoded values to better distinguish indy
> entries from other cp cache entries. The above changes now distinguish
> between entries with `to_cp_index()` using the bytecode, which is now
> propagated by the callers.
>
> The encoding flips the bits of the index so the encoded index is always
> negative, leading to access errors if there is no matching decode call. These
> calls are removed with some methods adjusted to distinguish between indices
> with the bytecode. Verified with tier 1-5 tests.
src/hotspot/share/ci/ciEnv.cpp line 1513:
> 1511: // process the BSM
> 1512: int pool_index = indy_info->constant_pool_index();
> 1513: BootstrapInfo bootstrap_specifier(cp, pool_index, indy_index);
Why not just change the incoming parameter name to `index`?
src/hotspot/share/classfile/resolutionErrors.hpp line 60:
> 58:
> 59: // This function is used to encode an invokedynamic index to
> differentiate it from a
> 60: // constant pool index. It assumes it is being called with a index
> that is less than 0
Is this comment still correct?
src/hotspot/share/interpreter/bootstrapInfo.cpp line 77:
> 75: return true;
> 76: } else if (indy_entry->resolution_failed()) {
> 77: int encoded_index =
> ResolutionErrorTable::encode_indy_index(_indy_index);
That's an improvement, from two levels of encoding to only one!
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18819#discussion_r1569625123
PR Review Comment: https://git.openjdk.org/jdk/pull/18819#discussion_r1569626519
PR Review Comment: https://git.openjdk.org/jdk/pull/18819#discussion_r1569627219
Re: RFR: 8330388: Remove invokedynamic cache index encoding
On Wed, 17 Apr 2024 15:26:52 GMT, Matias Saavedra Silva wrote: > Before [JDK-8307190](https://bugs.openjdk.org/browse/JDK-8307190), > [JDK-8309673](https://bugs.openjdk.org/browse/JDK-8309673), and > [JDK-8301995](https://bugs.openjdk.org/browse/JDK-8301995), invokedynamic > operands needed to be rewritten to encoded values to better distinguish indy > entries from other cp cache entries. The above changes now distinguish > between entries with `to_cp_index()` using the bytecode, which is now > propagated by the callers. > > The encoding flips the bits of the index so the encoded index is always > negative, leading to access errors if there is no matching decode call. These > calls are removed with some methods adjusted to distinguish between indices > with the bytecode. Verified with tier 1-5 tests. @dougxc should check JVMCI changes. - Marked as reviewed by dlong (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18819#pullrequestreview-2007498087
Re: RFR: 8322043: HeapDumper should use parallel dump by default [v4]
> 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: feedback - Changes: - all: https://git.openjdk.org/jdk/pull/18748/files - new: https://git.openjdk.org/jdk/pull/18748/files/f6db604f..482d9ffe Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18748=03 - incr: https://webrevs.openjdk.org/?repo=jdk=18748=02-03 Stats: 18 lines in 3 files changed: 0 ins; 3 del; 15 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: 8330388: Remove invokedynamic cache index encoding
On Wed, 17 Apr 2024 15:26:52 GMT, Matias Saavedra Silva wrote: > Before [JDK-8307190](https://bugs.openjdk.org/browse/JDK-8307190), > [JDK-8309673](https://bugs.openjdk.org/browse/JDK-8309673), and > [JDK-8301995](https://bugs.openjdk.org/browse/JDK-8301995), invokedynamic > operands needed to be rewritten to encoded values to better distinguish indy > entries from other cp cache entries. The above changes now distinguish > between entries with `to_cp_index()` using the bytecode, which is now > propagated by the callers. > > The encoding flips the bits of the index so the encoded index is always > negative, leading to access errors if there is no matching decode call. These > calls are removed with some methods adjusted to distinguish between indices > with the bytecode. Verified with tier 1-5 tests. Did you consider minimizing changes by leaving decode_invokedynamic_index/encode_invokedynamic_index calls in place, but having the implementations not change the value? - PR Comment: https://git.openjdk.org/jdk/pull/18819#issuecomment-2062609288
Re: RFR: 8329433: Reduce nmethod header size [v7]
On Wed, 17 Apr 2024 22:23:47 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 with a new target base due to a > merge or a rebase. The pull request now contains eight commits: > > - Merge master > - remove trailing space > - Shuffle fields initialization > - Address comments. Used checked_cast. > - Use 16-bits types for header_size and frame_complete_offset arguments > - Union fields which usages do not overlap > - Moved some fields initialization into init_defaults() > - 8329433: Reduce nmethod header size Merge [#18637](https://github.com/openjdk/jdk/pull/18637) added an other `short` field `_num_stack_arg_slots` which pushed `nmethod` size back to 240 bytes in product VM. I will not do changes in **this** PR to compensate it. - PR Comment: https://git.openjdk.org/jdk/pull/18768#issuecomment-2062779812
Re: RFR: 8329433: Reduce nmethod header size [v8]
> 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: Address comment - Changes: - all: https://git.openjdk.org/jdk/pull/18768/files - new: https://git.openjdk.org/jdk/pull/18768/files/adc17594..5f5f30de Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18768=07 - incr: https://webrevs.openjdk.org/?repo=jdk=18768=06-07 Stats: 16 lines in 1 file changed: 0 ins; 13 del; 3 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 [v8]
On Thu, 18 Apr 2024 00:41:03 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: > > Address comment Marked as reviewed by dlong (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18768#pullrequestreview-2007632424
