Re: RFR: 8261242: [Linux] OSContainer::is_containerized() returns true when run outside a container [v2]
On Tue, 16 Apr 2024 15:17:33 GMT, Severin Gehwolf wrote: > The idea here is to use this property to tune OpenJDK for in-container, > specifically k8s, use. In such a setup it's custom to run a single process > within set resource constraints. The in-container tuning means to use all the available resources. Containers in the real world have some memory limits set which is where my modified patch still correctly identifies it as a container to use all the available resources of the node which is the whole goal of the container detection code. > In order to do this, we need a reliable way to distinguish that vs. > non-containerized setup. I expect it should have been written "We need a reliable way to distinguish real world in-container vs. non-containerized setup. We do not mind behavior for artificial containers on OpenJDK development machines.". Which is what my patch does in an easier and less error-prone way. > If somebody really wants to run OpenJDK in a container expecting it to run > like a physical OpenJDK deployment, that's when `-XX:-UseContainerSupport` > should be used. That behaves still the same with my patch. Could you give a countercase where my patch behaves wrongly? - PR Comment: https://git.openjdk.org/jdk/pull/18201#issuecomment-2060158409
Re: RFR: 8329433: Reduce nmethod header size [v6]
> 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 - Changes: - all: https://git.openjdk.org/jdk/pull/18768/files - new: https://git.openjdk.org/jdk/pull/18768/files/8405a23d..6a164b11 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18768=05 - incr: https://webrevs.openjdk.org/?repo=jdk=18768=04-05 Stats: 158 lines in 3 files changed: 73 ins; 75 del; 10 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
RFR: 8330303: Crash: assert(_target_jt == nullptr || _target_jt->vthread() == target_h()) failed
This is a simple fix of three similar asserts.
The `_target_jt->jvmti_vthread()` has to be used instead of
`_target_jt->vthread()`.
The `_target_jt->vthread()` can be outdated in some specific contexts as shown
in the `hs_err` stack trace.
I've seen similar issue and already fixed it in this fragment of code:
class GetCurrentLocationClosure : public JvmtiUnitedHandshakeClosure {
. . .
void do_vthread(Handle target_h) {
assert(_target_jt == nullptr || !_target_jt->is_exiting(), "sanity check");
// use jvmti_vthread() as vthread() can be outdated
assert(_target_jt == nullptr || _target_jt->jvmti_vthread() == target_h(),
"sanity check");
. . .
The issue above was fixed by replacing `_target_jt->vthread()` with
`_target_jt->jvmti_vthread()`.
There are three places which need to be fixed the same way:
- `GetSingleStackTraceClosure::do_vthread(Handle target_h)`
- `SetForceEarlyReturn::do_vthread(Handle target_h)`
- `UpdateForPopTopFrameClosure::do_vthread(Handle target_h)`
Testing:
- Run mach5 tiers 1-6
-
Commit messages:
- add comments explaining that the vthread() can return outdated oop
- 8330303: Crash: assert(_target_jt == nullptr || _target_jt->vthread() ==
target_h()) failed
Changes: https://git.openjdk.org/jdk/pull/18806/files
Webrev: https://webrevs.openjdk.org/?repo=jdk=18806=00
Issue: https://bugs.openjdk.org/browse/JDK-8330303
Stats: 6 lines in 2 files changed: 3 ins; 0 del; 3 mod
Patch: https://git.openjdk.org/jdk/pull/18806.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/18806/head:pull/18806
PR: https://git.openjdk.org/jdk/pull/18806
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 Thank you for review, Leonid! - PR Comment: https://git.openjdk.org/jdk/pull/18778#issuecomment-2060066992
Re: RFR: 8329433: Reduce nmethod header size [v4]
On Tue, 16 Apr 2024 19:47:06 GMT, Vladimir Kozlov wrote: >> Okay. > > It is tempting to do for `nmethod` to replace `init_defaults()`. I will look > what can be done. It does not work. It does not allow fields initialization after delegation: src/hotspot/share/code/nmethod.cpp: In constructor 'nmethod::nmethod(Method*, CompilerType, int, int, CodeOffsets*, CodeBuffer*, int, ByteSize, ByteSize, OopMapSet*)': src/hotspot/share/code/nmethod.cpp:1232:56: error: mem-initializer for 'nmethod::_native_receiver_sp_offset' follows constructor delegation Yes, I can move some fields initialization into body - but then it will be duplicated initialization. >From reading net, the delegated constructor should take the largest number of >arguments which default values are set by delegating constructors. It is not >easy applicable to `nmethod` constructors - it will add more lines of code >than remove. `nmethod` constructors we have are too big and different enough >that delegation will not improve code. I will keep `init_defaults()` and extend it to reduce common code in constructors. - PR Review Comment: https://git.openjdk.org/jdk/pull/18768#discussion_r1567992881
Re: RFR: 8328741: serviceability/jvmti/ObjectMonitorUsage/ObjectMonitorUsage.java failed with unexpected owner [v3]
> 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 - Changes: - all: https://git.openjdk.org/jdk/pull/18778/files - new: https://git.openjdk.org/jdk/pull/18778/files/9941c492..68cdd8c7 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18778=02 - incr: https://webrevs.openjdk.org/?repo=jdk=18778=01-02 Stats: 9 lines in 2 files changed: 0 ins; 6 del; 3 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: 8328741: serviceability/jvmti/ObjectMonitorUsage/ObjectMonitorUsage.java failed with unexpected owner [v2]
On Tue, 16 Apr 2024 19:21:16 GMT, Leonid Mesnik wrote: > The fix looks good, just one nit. rml.wait(100); might be changed to plain > sleep(100) Right, thanks. Will fix it. - PR Comment: https://git.openjdk.org/jdk/pull/18778#issuecomment-2059950982
Re: RFR: 8318026: jcmd should provide access to detailed JVM object information [v16]
> Introduce the jcmd "VM.inspect" to implement access to detailed JVM object > information. > > Not recommended for live production use. Requires UnlockDiagnosticVMOptions > and not included in jcmd help output, to remind us this is not a > general-purpose customer-facing tool. Kevin Walls 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 33 additional commits since the last revision: - Merge remote-tracking branch 'upstream/master' into 8318026_jcmd_VMdebug_command - Update XX option - Update decode_raw usage to decode_without_asserts, after 8328698 - nits - Merge remote-tracking branch 'upstream/master' into 8318026_jcmd_VMdebug_command - Update XX flag requirement - VMInspectTest update - Merge remote-tracking branch 'upstream/master' into 8318026_jcmd_VMdebug_command - One dbg_is_good_oop method - Test more pointer types: compiled method and metadata. - ... and 23 more: https://git.openjdk.org/jdk/compare/e5180fba...bb92a849 - Changes: - all: https://git.openjdk.org/jdk/pull/17655/files - new: https://git.openjdk.org/jdk/pull/17655/files/7e6c7b97..bb92a849 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17655=15 - incr: https://webrevs.openjdk.org/?repo=jdk=17655=14-15 Stats: 16719 lines in 562 files changed: 8816 ins; 4016 del; 3887 mod Patch: https://git.openjdk.org/jdk/pull/17655.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17655/head:pull/17655 PR: https://git.openjdk.org/jdk/pull/17655
Re: RFR: 8329433: Reduce nmethod header size [v4]
On Tue, 16 Apr 2024 19:20:37 GMT, Vladimir Kozlov wrote: >> Delegating constructors are the answer to having some common 'init' >> functions. It would simply save lines of code that look the same in both >> constructor initializer lists. But it's a drive-by comment. > > Okay. It is tempting to do for `nmethod` to replace `init_defaults()`. I will look what can be done. - PR Review Comment: https://git.openjdk.org/jdk/pull/18768#discussion_r1567866651
Re: RFR: 8322043: HeapDumper should use parallel dump by default [v3]
On Tue, 16 Apr 2024 09:08:20 GMT, David Holmes wrote: >> Alex Menkov has updated the pull request incrementally with one additional >> commit since the last revision: >> >> check free_memory for OOME > > 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. - PR Review Comment: https://git.openjdk.org/jdk/pull/18748#discussion_r1567864860
Re: RFR: 8329433: Reduce nmethod header size [v4]
On Tue, 16 Apr 2024 19:03:01 GMT, Coleen Phillimore wrote: >> Thank you, @coleenp, foe looking on these changes. >> >> Which fields are initialized twice? Only `_oop_maps` is set to `nullptr` >> before we proper build oop maps in first constructor. >> >> The only saving could be lines of code but then I would have to check that >> `cb != nullptr` and do other additional checks which I don't think will save >> much lines. >> >> Separation of `nmethod` constructor for native wrappers is helping clear see >> the difference and I would like to keep them separate. We have >> `init_defaults()` method for similar code and I can move more code into it >> from both constructors. > > Delegating constructors are the answer to having some common 'init' > functions. It would simply save lines of code that look the same in both > constructor initializer lists. But it's a drive-by comment. Okay. - PR Review Comment: https://git.openjdk.org/jdk/pull/18768#discussion_r1567841490
Re: RFR: 8328741: serviceability/jvmti/ObjectMonitorUsage/ObjectMonitorUsage.java failed with unexpected owner [v2]
On Tue, 16 Apr 2024 06:08:58 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: removed event_lock; moved wait_for_state() to jvmti_common.hpp Marked as reviewed by lmesnik (Reviewer). The fix looks good, just one nit. rml.wait(100); might be changed to plain sleep(100) - PR Review: https://git.openjdk.org/jdk/pull/18778#pullrequestreview-2004437310 PR Comment: https://git.openjdk.org/jdk/pull/18778#issuecomment-2059776412
Re: RFR: 8329433: Reduce nmethod header size [v4]
On Tue, 16 Apr 2024 18:54:40 GMT, Vladimir Kozlov wrote: >> src/hotspot/share/code/codeBlob.cpp line 106: >> >>> 104: >>> 105: // Simple CodeBlob used for simple BufferBlob. >>> 106: CodeBlob::CodeBlob(const char* name, CodeBlobKind kind, int size, >>> uint16_t header_size) : >> >> Just a drive-by comment. You might be able to use delegating constructors >> for CodeBlob so you don't have to have the field initializations twice. >> Maybe the same for nmethod ? > > Thank you, @coleenp, foe looking on these changes. > > Which fields are initialized twice? Only `_oop_maps` is set to `nullptr` > before we proper build oop maps in first constructor. > > The only saving could be lines of code but then I would have to check that > `cb != nullptr` and do other additional checks which I don't think will save > much lines. > > Separation of `nmethod` constructor for native wrappers is helping clear see > the difference and I would like to keep them separate. We have > `init_defaults()` method for similar code and I can move more code into it > from both constructors. Delegating constructors are the answer to having some common 'init' functions. It would simply save lines of code that look the same in both constructor initializer lists. But it's a drive-by comment. - PR Review Comment: https://git.openjdk.org/jdk/pull/18768#discussion_r1567823393
Re: RFR: 8329433: Reduce nmethod header size [v4]
On Tue, 16 Apr 2024 16:33:18 GMT, Coleen Phillimore wrote: >> 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 > > src/hotspot/share/code/codeBlob.cpp line 106: > >> 104: >> 105: // Simple CodeBlob used for simple BufferBlob. >> 106: CodeBlob::CodeBlob(const char* name, CodeBlobKind kind, int size, >> uint16_t header_size) : > > Just a drive-by comment. You might be able to use delegating constructors > for CodeBlob so you don't have to have the field initializations twice. > Maybe the same for nmethod ? Thank you, @coleenp, foe looking on these changes. Which fields are initialized twice? Only `_oop_maps` is set to `nullptr` before we proper build oop maps in first constructor. The only saving could be lines of code but then I would have to check that `cb != nullptr` and do other additional checks which I don't think will save much lines. Separation of `nmethod` constructor for native wrappers is helping clear see the difference and I would like to keep them separate. We have `init_defaults()` method for similar code and I can move more code into it from both constructors. - PR Review Comment: https://git.openjdk.org/jdk/pull/18768#discussion_r1567814189
Re: RFR: 8329433: Reduce nmethod header size [v5]
> 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 comments. Used checked_cast. - Changes: - all: https://git.openjdk.org/jdk/pull/18768/files - new: https://git.openjdk.org/jdk/pull/18768/files/6cb22e81..8405a23d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18768=04 - incr: https://webrevs.openjdk.org/?repo=jdk=18768=03-04 Stats: 32 lines in 3 files changed: 4 ins; 11 del; 17 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: 8261242: [Linux] OSContainer::is_containerized() returns true when run outside a container [v2]
On Sat, 13 Apr 2024 18:29:59 GMT, Thomas Stuefe wrote:
>> 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 ten additional
>> commits since the last revision:
>>
>> - Merge branch 'master' into jdk-8261242-is-containerized-fix
>> - jcheck fixes
>> - Fix tests
>> - Implement Metrics.isContainerized()
>> - Some clean-up
>> - Drop cgroups testing on plain Linux
>> - Implement fall-back logic for non-ro controller mounts
>> - Make find_ro static and local to compilation unit
>> - 8261242: [Linux] OSContainer::is_containerized() returns true
>
> src/hotspot/os/linux/cgroupSubsystem_linux.cpp line 170:
>
>> 168: }
>> 169: }
>> 170: return false;
>
> An alternative, simpler, no need for modifying source string:
>
> static bool find_ro_opt(const char* o) {
> return strcmp(o, "ro") || strstr(o, ",ro") || strstr(o, "ro,");
> }
Please disregard my comment. Albeit longer, your version is clearer to read and
more fault tolerant.
> src/hotspot/os/linux/cgroupSubsystem_linux.cpp line 351:
>
>> 349: //
>> 350: // We collect the read only mount option in the cgroup infos so as
>> to have that
>> 351: // info ready when determining is_containerized().
>
> Here, and in other places: a comment indicating the line format we scan would
> be appreciated, possibly with argument numbers. Saves the casual code reader
> from looking into proc man page. Even just pasting the example line for proc
> manpage would be fine (https://man7.org/linux/man-pages/man5/proc.5.html)
> (but with order adapted to your scanf call, they count major:minor as one)
Trying to parse the `%s%*[^-]-`
So, %s parses the mount options, until we encounter whitespace. Then %*[^-]-
parses everything that is not a dash, until we encounter the dash? Then we eat
the dash? This is to skip the optionals?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18201#discussion_r1567754861
PR Review Comment: https://git.openjdk.org/jdk/pull/18201#discussion_r1567767209
Re: RFR: 8261242: [Linux] OSContainer::is_containerized() returns true when run outside a container [v2]
On Thu, 11 Apr 2024 12:08:02 GMT, Severin Gehwolf wrote:
>> Please review this enhancement to the container detection code which allows
>> it to figure out whether the JVM is actually running inside a container
>> (`podman`, `docker`, `crio`), or with some other means that enforces
>> memory/cpu limits by means of the cgroup filesystem. If neither of those
>> conditions hold, the JVM runs in not containerized mode, addressing the
>> issue described in the JBS tracker. For example, on my Linux system
>> `is_containerized() == false" is being indicated with the following trace
>> log line:
>>
>>
>> [0.001s][debug][os,container] OSContainer::init: is_containerized() = false
>> because no cpu or memory limit is present
>>
>>
>> This state is being exposed by the Java `Metrics` API class using the new
>> (still JDK internal) `isContainerized()` method. Example:
>>
>>
>> java -XshowSettings:system --version
>> Operating System Metrics:
>> Provider: cgroupv1
>> System not containerized.
>> openjdk 23-internal 2024-09-17
>> OpenJDK Runtime Environment (fastdebug build
>> 23-internal-adhoc.sgehwolf.jdk-jdk)
>> OpenJDK 64-Bit Server VM (fastdebug build
>> 23-internal-adhoc.sgehwolf.jdk-jdk, mixed mode, sharing)
>>
>>
>> The basic property this is being built on is the observation that the cgroup
>> controllers typically get mounted read only into containers. Note that the
>> current container tests assert that `OSContainer::is_containerized() ==
>> true` in various tests. Therefore, using the heuristic of "is any memory or
>> cpu limit present" isn't sufficient. I had considered that in an earlier
>> iteration, but many container tests failed.
>>
>> Overall, I think, with this patch we improve the current situation of
>> claiming a containerized system being present when it's actually just a
>> regular Linux system.
>>
>> Testing:
>>
>> - [x] GHA (risc-v failure seems infra related)
>> - [x] Container tests on Linux x86_64 of cgroups v1 and cgroups v2
>> (including gtests)
>> - [x] Some manual testing using cri-o
>>
>> Thoughts?
>
> Severin Gehwolf has updated the pull request with a new target base due to a
> merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains ten additional
> commits since the last revision:
>
> - Merge branch 'master' into jdk-8261242-is-containerized-fix
> - jcheck fixes
> - Fix tests
> - Implement Metrics.isContainerized()
> - Some clean-up
> - Drop cgroups testing on plain Linux
> - Implement fall-back logic for non-ro controller mounts
> - Make find_ro static and local to compilation unit
> - 8261242: [Linux] OSContainer::is_containerized() returns true
I am not enough of a container expert to judge if the basic approach is right -
I trust you on this. This is just a technical code review.
src/hotspot/os/linux/cgroupSubsystem_linux.cpp line 170:
> 168: }
> 169: }
> 170: return false;
An alternative, simpler, no need for modifying source string:
static bool find_ro_opt(const char* o) {
return strcmp(o, "ro") || strstr(o, ",ro") || strstr(o, "ro,");
}
src/hotspot/os/linux/cgroupSubsystem_linux.cpp line 351:
> 349: //
> 350: // We collect the read only mount option in the cgroup infos so as
> to have that
> 351: // info ready when determining is_containerized().
Here, and in other places: a comment indicating the line format we scan would
be appreciated, possibly with argument numbers. Saves the casual code reader
from looking into proc man page. Even just pasting the example line for proc
manpage would be fine (https://man7.org/linux/man-pages/man5/proc.5.html) (but
with order adapted to your scanf call, they count major:minor as one)
src/hotspot/os/linux/osContainer_linux.cpp line 78:
> 76: const char *reason;
> 77: bool any_mem_cpu_limit_present = false;
> 78: bool ctrl_ro = cgroup_subsystem->is_containerized();
nit: naming? what does ctrl mean in this case? Maybe use
"cgroup_is_containerized"?
src/java.base/share/classes/sun/launcher/LauncherHelper.java line 375:
> 373: if (!c.isContainerized()) {
> 374: ostream.println(INDENT + "System not containerized.");
> 375: return;
Why return here? Would this not cut the output short in the non-containerized
case?
And if this not intended, the not-containerized-`-XshowSettings:system` test
below should test and catch this (e.g. scan for CPU set)
-
PR Review: https://git.openjdk.org/jdk/pull/18201#pullrequestreview-1999328503
PR Review Comment: https://git.openjdk.org/jdk/pull/18201#discussion_r1564182879
PR Review Comment: https://git.openjdk.org/jdk/pull/18201#discussion_r1567756663
PR Review Comment: https://git.openjdk.org/jdk/pull/18201#discussion_r1567774124
PR Review Comment: https://git.openjdk.org/jdk/pull/18201#discussion_r1567779248
Re: RFR: 8329433: Reduce nmethod header size [v4]
On Tue, 16 Apr 2024 03:31:25 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: > > Use 16-bits types for header_size and frame_complete_offset arguments src/hotspot/share/code/codeBlob.cpp line 106: > 104: > 105: // Simple CodeBlob used for simple BufferBlob. > 106: CodeBlob::CodeBlob(const char* name, CodeBlobKind kind, int size, > uint16_t header_size) : Just a drive-by comment. You might be able to use delegating constructors for CodeBlob so you don't have to have the field initializations twice. Maybe the same for nmethod ? - PR Review Comment: https://git.openjdk.org/jdk/pull/18768#discussion_r1567658758
Re: RFR: 8329433: Reduce nmethod header size [v3]
On Tue, 16 Apr 2024 06:13:59 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.cpp line 1235:
>
>> 1233: int skipped_insts_size =
>> code_buffer->total_skipped_instructions_size();
>> 1234: #ifdef ASSERT
>> 1235: assert(((skipped_insts_size >> 16) == 0), "size is bigger than
>> 64Kb: %d", skipped_insts_size);
>
> Suggestion:
>
>
> I think it's simpler just to use checked_cast below.
Agree
> src/hotspot/share/code/nmethod.cpp line 1240:
>
>> 1238: int consts_offset =
>> code_buffer->total_offset_of(code_buffer->consts());
>> 1239: assert(consts_offset == 0, "const_offset: %d", consts_offset);
>> 1240: #endif
>
> Suggestion:
I assume you are suggesting to remove `#ifdef ASSERT`.
Done.
> src/hotspot/share/code/nmethod.cpp line 1241:
>
>> 1239: assert(consts_offset == 0, "const_offset: %d", consts_offset);
>> 1240: #endif
>> 1241: _skipped_instructions_size = (uint16_t)skipped_insts_size;
>
> Suggestion:
>
> _skipped_instructions_size =
> checked_cast(code_buffer->total_skipped_instructions_size());
Done.
> 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.
> src/hotspot/share/code/nmethod.cpp line 1445:
>
>> 1443: _metadata_offset = (uint16_t)oops_size;
>> 1444: _dependencies_offset = _metadata_offset +
>> (uint16_t)metadata_size;
>> 1445: _scopes_pcs_offset= _dependencies_offset +
>> (uint16_t)deps_size;
>
> Use checked_cast instead of raw casts.
okay
> src/hotspot/share/code/nmethod.cpp line 1459:
>
>> 1457: assert((data_offset() + data_end_offset) <= nmethod_size, "wrong
>> nmethod's size: %d < %d", nmethod_size, (data_offset() + data_end_offset));
>> 1458:
>> 1459: _entry_offset =
>> (uint16_t)offsets->value(CodeOffsets::Entry);
>
> Use checked_cast.
done
> src/hotspot/share/memory/heap.hpp line 58:
>
>> 56: void set_length(size_t length) {
>> 57: LP64_ONLY( assert(((length >> 32) == 0), "sanity"); )
>> 58: _header._length = (uint32_t)length;
>
> Suggestion:
>
> _header._length = checked_castlength;
Done.
> src/hotspot/share/memory/heap.hpp line 63:
>
>> 61: // Accessors
>> 62: void* allocated_space() const { return (void*)(this +
>> 1); }
>> 63: size_t length() const { return
>> (size_t)_header._length; }
>
> This cast looks unnecessary.
Agree.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18768#discussion_r1567619512
PR Review Comment: https://git.openjdk.org/jdk/pull/18768#discussion_r1567620520
PR Review Comment: https://git.openjdk.org/jdk/pull/18768#discussion_r1567620735
PR Review Comment: https://git.openjdk.org/jdk/pull/18768#discussion_r1567627565
PR Review Comment: https://git.openjdk.org/jdk/pull/18768#discussion_r1567636013
PR Review Comment: https://git.openjdk.org/jdk/pull/18768#discussion_r1567638682
PR Review Comment: https://git.openjdk.org/jdk/pull/18768#discussion_r1567644204
PR Review Comment: https://git.openjdk.org/jdk/pull/18768#discussion_r1567645140
Re: RFR: 8329433: Reduce nmethod header size [v4]
On Tue, 16 Apr 2024 03:31:25 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: > > Use 16-bits types for header_size and frame_complete_offset arguments @dean-long, thank you for review. I applied all your suggestions and push it after testing. - PR Review: https://git.openjdk.org/jdk/pull/18768#pullrequestreview-2004070926
Re: RFR: 8329433: Reduce nmethod header size [v3]
On Tue, 16 Apr 2024 06:48:05 GMT, Dean Long wrote: >> 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 > > OK, but I'd rather see it in the accessors too. Some users are checking for > method()->is_native() and others are checking for is_osr_method(), so we need > to make sure those are always mutually exclusive: method()->is_native() != > is_osr_method(). We have separate `nmethod()` constructor for native method wrappers and it sets `_entry_bci = InvocationEntryBci;`. So it is impossible to have OSR native method wrapper. But I agree with adding assert into accessors to catch accidental usage for not native methods. - PR Review Comment: https://git.openjdk.org/jdk/pull/18768#discussion_r1567603094
Re: RFR: 8261242: [Linux] OSContainer::is_containerized() returns true when run outside a container [v2]
On Tue, 16 Apr 2024 14:40:46 GMT, Jan Kratochvil wrote: > IMHO `is_containerized()` is OK to return `false` even when running in a > container but with no limitations set. The idea here is to use this property to tune OpenJDK for in-container, specifically k8s, use. In such a setup it's custom to run a single process within set resource constraints. In order to do this, we need a reliable way to distinguish that vs. non-containerized setup. If somebody really wants to run OpenJDK in a container expecting it to run like a physical OpenJDK deployment, that's when `-XX:-UseContainerSupport` should be used. - PR Comment: https://git.openjdk.org/jdk/pull/18201#issuecomment-2059344194
Re: RFR: 8261242: [Linux] OSContainer::is_containerized() returns true when run outside a container [v2]
On Thu, 11 Apr 2024 12:08:02 GMT, Severin Gehwolf wrote: >> Please review this enhancement to the container detection code which allows >> it to figure out whether the JVM is actually running inside a container >> (`podman`, `docker`, `crio`), or with some other means that enforces >> memory/cpu limits by means of the cgroup filesystem. If neither of those >> conditions hold, the JVM runs in not containerized mode, addressing the >> issue described in the JBS tracker. For example, on my Linux system >> `is_containerized() == false" is being indicated with the following trace >> log line: >> >> >> [0.001s][debug][os,container] OSContainer::init: is_containerized() = false >> because no cpu or memory limit is present >> >> >> This state is being exposed by the Java `Metrics` API class using the new >> (still JDK internal) `isContainerized()` method. Example: >> >> >> java -XshowSettings:system --version >> Operating System Metrics: >> Provider: cgroupv1 >> System not containerized. >> openjdk 23-internal 2024-09-17 >> OpenJDK Runtime Environment (fastdebug build >> 23-internal-adhoc.sgehwolf.jdk-jdk) >> OpenJDK 64-Bit Server VM (fastdebug build >> 23-internal-adhoc.sgehwolf.jdk-jdk, mixed mode, sharing) >> >> >> The basic property this is being built on is the observation that the cgroup >> controllers typically get mounted read only into containers. Note that the >> current container tests assert that `OSContainer::is_containerized() == >> true` in various tests. Therefore, using the heuristic of "is any memory or >> cpu limit present" isn't sufficient. I had considered that in an earlier >> iteration, but many container tests failed. >> >> Overall, I think, with this patch we improve the current situation of >> claiming a containerized system being present when it's actually just a >> regular Linux system. >> >> Testing: >> >> - [x] GHA (risc-v failure seems infra related) >> - [x] Container tests on Linux x86_64 of cgroups v1 and cgroups v2 >> (including gtests) >> - [x] Some manual testing using cri-o >> >> Thoughts? > > Severin Gehwolf has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains ten additional > commits since the last revision: > > - Merge branch 'master' into jdk-8261242-is-containerized-fix > - jcheck fixes > - Fix tests > - Implement Metrics.isContainerized() > - Some clean-up > - Drop cgroups testing on plain Linux > - Implement fall-back logic for non-ro controller mounts > - Make find_ro static and local to compilation unit > - 8261242: [Linux] OSContainer::is_containerized() returns true IMHO `is_containerized()` is OK to return `false` even when running in a container but with no limitations set. Container detection is IIUC/AFAIK being used to maximize resource usage by OpenJDK. But if OpenJDK runs in a container with the same limits as the hardware box OpenJDK should still use reduced resources as it is sharing them with other processes on the hardware box. [is-containerized.patch.txt](https://github.com/openjdk/jdk/files/14998503/is-containerized.patch.txt) - PR Comment: https://git.openjdk.org/jdk/pull/18201#issuecomment-2059261807
Integrated: 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. This pull request has now been integrated. Changeset: 58911ccc Author:Kevin Walls URL: https://git.openjdk.org/jdk/commit/58911ccc2c48b4b5dd2ebc9d2a44dcf3737eca50 Stats: 42 lines in 1 file changed: 9 ins; 21 del; 12 mod 8188784: javax/management/notification/BroadcasterSupportDeadlockTest.java - TEST FAILED: deadlock Reviewed-by: cjplummer, lmesnik - PR: https://git.openjdk.org/jdk/pull/18687
Re: RFR: 8188784: javax/management/notification/BroadcasterSupportDeadlockTest.java - TEST FAILED: deadlock [v2]
On Tue, 16 Apr 2024 09:26:58 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. > > Kevin Walls has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains three additional > commits since the last revision: > > - Remove System.exit calls > - Merge remote-tracking branch 'upstream/master' into > 8188784_BroadcasterSupportDeadlockTest > - 8188784: javax/management/notification/BroadcasterSupportDeadlockTest.java > - TEST FAILED: deadlock Thanks for the reviews! - PR Comment: https://git.openjdk.org/jdk/pull/18687#issuecomment-2058745011
Re: RFR: 8188784: javax/management/notification/BroadcasterSupportDeadlockTest.java - TEST FAILED: deadlock [v2]
On Tue, 16 Apr 2024 00:31:10 GMT, Leonid Mesnik wrote:
>> Kevin Walls has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains three additional
>> commits since the last revision:
>>
>> - Remove System.exit calls
>> - Merge remote-tracking branch 'upstream/master' into
>> 8188784_BroadcasterSupportDeadlockTest
>> - 8188784:
>> javax/management/notification/BroadcasterSupportDeadlockTest.java - TEST
>> FAILED: deadlock
>
> 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.
OK sure.
FYI I count 137 System.exit calls in test/jdk/javax/management
That's after I take out the existing 3 that are in this test. 8-)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18687#discussion_r1567098465
Re: RFR: 8188784: javax/management/notification/BroadcasterSupportDeadlockTest.java - TEST FAILED: deadlock [v2]
> 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. Kevin Walls has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision: - Remove System.exit calls - Merge remote-tracking branch 'upstream/master' into 8188784_BroadcasterSupportDeadlockTest - 8188784: javax/management/notification/BroadcasterSupportDeadlockTest.java - TEST FAILED: deadlock - Changes: - all: https://git.openjdk.org/jdk/pull/18687/files - new: https://git.openjdk.org/jdk/pull/18687/files/5237f1d8..a77b3c85 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18687=01 - incr: https://webrevs.openjdk.org/?repo=jdk=18687=00-01 Stats: 16713 lines in 561 files changed: 8813 ins; 4016 del; 3884 mod Patch: https://git.openjdk.org/jdk/pull/18687.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18687/head:pull/18687 PR: https://git.openjdk.org/jdk/pull/18687
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 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()`? - PR Review Comment: https://git.openjdk.org/jdk/pull/18748#discussion_r1567017789
Re: RFR: 8329433: Reduce nmethod header size [v3]
On Tue, 16 Apr 2024 03:12:48 GMT, Vladimir Kozlov wrote: >> 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. I don't think uint8_t vs uint16_t matters, only if it is signed, unsigned, or bool. So if we have more than 8 individual :1 fields, it will expand to a 2nd byte. - PR Review Comment: https://git.openjdk.org/jdk/pull/18768#discussion_r1566814258
Re: RFR: 8329433: Reduce nmethod header size [v3]
On Tue, 16 Apr 2024 03:06:13 GMT, Vladimir Kozlov wrote: >> 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 OK, but I'd rather see it in the accessors too. Some users are checking for method()->is_native() and others are checking for is_osr_method(), so we need to make sure those are always mutually exclusive: method()->is_native() != is_osr_method(). - PR Review Comment: https://git.openjdk.org/jdk/pull/18768#discussion_r1566806754
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/memory/heap.hpp line 58:
> 56: void set_length(size_t length) {
> 57: LP64_ONLY( assert(((length >> 32) == 0), "sanity"); )
> 58: _header._length = (uint32_t)length;
Suggestion:
_header._length = checked_castlength;
src/hotspot/share/memory/heap.hpp line 63:
> 61: // Accessors
> 62: void* allocated_space() const { return (void*)(this +
> 1); }
> 63: size_t length() const { return
> (size_t)_header._length; }
This cast looks unnecessary.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18768#discussion_r1566784458
PR Review Comment: https://git.openjdk.org/jdk/pull/18768#discussion_r1566784587
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.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. src/hotspot/share/code/nmethod.cpp line 1445: > 1443: _metadata_offset = (uint16_t)oops_size; > 1444: _dependencies_offset = _metadata_offset + > (uint16_t)metadata_size; > 1445: _scopes_pcs_offset= _dependencies_offset + (uint16_t)deps_size; Use checked_cast instead of raw casts. src/hotspot/share/code/nmethod.cpp line 1459: > 1457: assert((data_offset() + data_end_offset) <= nmethod_size, "wrong > nmethod's size: %d < %d", nmethod_size, (data_offset() + data_end_offset)); > 1458: > 1459: _entry_offset = > (uint16_t)offsets->value(CodeOffsets::Entry); Use checked_cast. - PR Review Comment: https://git.openjdk.org/jdk/pull/18768#discussion_r1566771026 PR Review Comment: https://git.openjdk.org/jdk/pull/18768#discussion_r1566772567 PR Review Comment: https://git.openjdk.org/jdk/pull/18768#discussion_r1566773477
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.cpp line 1235: > 1233: int skipped_insts_size = > code_buffer->total_skipped_instructions_size(); > 1234: #ifdef ASSERT > 1235: assert(((skipped_insts_size >> 16) == 0), "size is bigger than > 64Kb: %d", skipped_insts_size); Suggestion: I think it's simpler just to use checked_cast below. src/hotspot/share/code/nmethod.cpp line 1240: > 1238: int consts_offset = > code_buffer->total_offset_of(code_buffer->consts()); > 1239: assert(consts_offset == 0, "const_offset: %d", consts_offset); > 1240: #endif Suggestion: src/hotspot/share/code/nmethod.cpp line 1241: > 1239: assert(consts_offset == 0, "const_offset: %d", consts_offset); > 1240: #endif > 1241: _skipped_instructions_size = (uint16_t)skipped_insts_size; Suggestion: _skipped_instructions_size = checked_cast(code_buffer->total_skipped_instructions_size()); - PR Review Comment: https://git.openjdk.org/jdk/pull/18768#discussion_r1566764300 PR Review Comment: https://git.openjdk.org/jdk/pull/18768#discussion_r1566765068 PR Review Comment: https://git.openjdk.org/jdk/pull/18768#discussion_r1566759786
Re: RFR: 8328741: serviceability/jvmti/ObjectMonitorUsage/ObjectMonitorUsage.java failed with unexpected owner [v2]
> 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: removed event_lock; moved wait_for_state() to jvmti_common.hpp - Changes: - all: https://git.openjdk.org/jdk/pull/18778/files - new: https://git.openjdk.org/jdk/pull/18778/files/1a1cde71..9941c492 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18778=01 - incr: https://webrevs.openjdk.org/?repo=jdk=18778=00-01 Stats: 38 lines in 2 files changed: 18 ins; 18 del; 2 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: 8328741: serviceability/jvmti/ObjectMonitorUsage/ObjectMonitorUsage.java failed with unexpected owner [v2]
On Mon, 15 Apr 2024 21:34:48 GMT, Leonid Mesnik wrote:
>> Serguei Spitsyn has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> review: removed event_lock; moved wait_for_state() to jvmti_common.hpp
>
> 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.
Good suggestions, thanks. Implemented now.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18778#discussion_r1566755184
