RFR: 8329113: Deprecate -XX:+UseNotificationThread
The fix deprecates -XX:+UseNotificationThread VM option Testing: tier1-3 - Commit messages: - fix Changes: https://git.openjdk.org/jdk/pull/18852/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18852=00 Issue: https://bugs.openjdk.org/browse/JDK-8329113 Stats: 3 lines in 3 files changed: 2 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18852.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18852/head:pull/18852 PR: https://git.openjdk.org/jdk/pull/18852
RFR: 8320215: HeapDumper can use DumpWriter buffer during merge
The fix updates HeapMerger to use writer buffer (no need to copy memory, also writer buffer is 1MB instead of 4KB). Additionally fixed small issue in FileWriter (looks like `ssize_t` instead of `size_t` is a typo, the argument should be unsigned) Testing: all HeapDump-related tests on Oracle supported platforms - Commit messages: - fix Changes: https://git.openjdk.org/jdk/pull/18850/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18850=00 Issue: https://bugs.openjdk.org/browse/JDK-8320215 Stats: 22 lines in 3 files changed: 9 ins; 4 del; 9 mod Patch: https://git.openjdk.org/jdk/pull/18850.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18850/head:pull/18850 PR: https://git.openjdk.org/jdk/pull/18850
Re: RFR: 8303689: javac -Xlint could/should report on "dangling" doc comments [v6]
> Please review the updates to support a proposed new
> `-Xlint:dangling-doc-comments` option.
>
> The work can be thought of as in 3 parts:
>
> 1. An update to the `javac` internal class `DeferredLintHandler` so that it
> is possible to specify the appropriately configured `Lint` object when it is
> time to consider whether to generate the diagnostic or not.
>
> 2. Updates to the `javac` front end to record "dangling docs comments" found
> near the beginning of a declaration, and to report them using an instance of
> `DeferredLintHandler`. This allows the warnings to be enabled or disabled
> using the standard mechanisms for `-Xlint` and `@SuppressWarnings`. The
> procedure for handling dangling doc comments is described in this comment in
> `JavacParser`.
>
> * Dangling documentation comments are handled as follows.
> * 1. {@code Scanner} adds all doc comments to a queue of
> * recent doc comments. The queue is flushed whenever
> * it is known that the recent doc comments should be
> * ignored and should not cause any warnings.
> * 2. The primary documentation comment is the one obtained
> * from the first token of any declaration.
> * (using {@code token.getDocComment()}.
> * 3. At the end of the "signature" of the declaration
> * (that is, before any initialization or body for the
> * declaration) any other "recent" comments are saved
> * in a map using the primary comment as a key,
> * using this method, {@code saveDanglingComments}.
> * 4. When the tree node for the declaration is finally
> * available, and the primary comment, if any,
> * is "attached", (in {@link #attach}) any related
> * dangling comments are also attached to the tree node
> * by registering them using the {@link #deferredLintHandler}.
> * 5. (Later) Warnings may be genereated for the dangling
> * comments, subject to the {@code -Xlint} and
> * {@code @SuppressWarnings}.
>
>
> 3. Updates to the make files to disable the warnings in modules for which
> the
> warning is generated. This is often because of the confusing use of `/**` to
> create box or other standout comments.
Jonathan Gibbons has updated the pull request with a new target base due to a
merge or a rebase. The pull request now contains seven commits:
- Merge with upstream/master
- update test
- improve handling of ignorable doc comments
suppress warning when building test code
- Merge remote-tracking branch 'upstream/master' into 8303689.dangling-comments
- call `saveDanglingDocComments` for local variable declarations
- adjust call for `saveDanglingDocComments` for enum members
- JDK-8303689: javac -Xlint could/should report on "dangling" doc comments
-
Changes: https://git.openjdk.org/jdk/pull/18527/files
Webrev: https://webrevs.openjdk.org/?repo=jdk=18527=05
Stats: 488 lines in 61 files changed: 389 ins; 3 del; 96 mod
Patch: https://git.openjdk.org/jdk/pull/18527.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/18527/head:pull/18527
PR: https://git.openjdk.org/jdk/pull/18527
Re: RFR: 8328741: serviceability/jvmti/ObjectMonitorUsage/ObjectMonitorUsage.java failed with unexpected owner [v4]
> 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: tweak in wait_for_state func to allow exp_state bit mask - Changes: - all: https://git.openjdk.org/jdk/pull/18778/files - new: https://git.openjdk.org/jdk/pull/18778/files/68cdd8c7..0b2500d5 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18778=03 - incr: https://webrevs.openjdk.org/?repo=jdk=18778=02-03 Stats: 3 lines in 1 file changed: 2 ins; 0 del; 1 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: 8303689: javac -Xlint could/should report on "dangling" doc comments [v5]
> Please review the updates to support a proposed new
> `-Xlint:dangling-doc-comments` option.
>
> The work can be thought of as in 3 parts:
>
> 1. An update to the `javac` internal class `DeferredLintHandler` so that it
> is possible to specify the appropriately configured `Lint` object when it is
> time to consider whether to generate the diagnostic or not.
>
> 2. Updates to the `javac` front end to record "dangling docs comments" found
> near the beginning of a declaration, and to report them using an instance of
> `DeferredLintHandler`. This allows the warnings to be enabled or disabled
> using the standard mechanisms for `-Xlint` and `@SuppressWarnings`. The
> procedure for handling dangling doc comments is described in this comment in
> `JavacParser`.
>
> * Dangling documentation comments are handled as follows.
> * 1. {@code Scanner} adds all doc comments to a queue of
> * recent doc comments. The queue is flushed whenever
> * it is known that the recent doc comments should be
> * ignored and should not cause any warnings.
> * 2. The primary documentation comment is the one obtained
> * from the first token of any declaration.
> * (using {@code token.getDocComment()}.
> * 3. At the end of the "signature" of the declaration
> * (that is, before any initialization or body for the
> * declaration) any other "recent" comments are saved
> * in a map using the primary comment as a key,
> * using this method, {@code saveDanglingComments}.
> * 4. When the tree node for the declaration is finally
> * available, and the primary comment, if any,
> * is "attached", (in {@link #attach}) any related
> * dangling comments are also attached to the tree node
> * by registering them using the {@link #deferredLintHandler}.
> * 5. (Later) Warnings may be genereated for the dangling
> * comments, subject to the {@code -Xlint} and
> * {@code @SuppressWarnings}.
>
>
> 3. Updates to the make files to disable the warnings in modules for which
> the
> warning is generated. This is often because of the confusing use of `/**` to
> create box or other standout comments.
Jonathan Gibbons has updated the pull request incrementally with one additional
commit since the last revision:
update test
-
Changes:
- all: https://git.openjdk.org/jdk/pull/18527/files
- new: https://git.openjdk.org/jdk/pull/18527/files/f3670e7a..8ad8b818
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=18527=04
- incr: https://webrevs.openjdk.org/?repo=jdk=18527=03-04
Stats: 6 lines in 1 file changed: 6 ins; 0 del; 0 mod
Patch: https://git.openjdk.org/jdk/pull/18527.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/18527/head:pull/18527
PR: https://git.openjdk.org/jdk/pull/18527
Re: RFR: 8303689: javac -Xlint could/should report on "dangling" doc comments [v4]
> Please review the updates to support a proposed new
> `-Xlint:dangling-doc-comments` option.
>
> The work can be thought of as in 3 parts:
>
> 1. An update to the `javac` internal class `DeferredLintHandler` so that it
> is possible to specify the appropriately configured `Lint` object when it is
> time to consider whether to generate the diagnostic or not.
>
> 2. Updates to the `javac` front end to record "dangling docs comments" found
> near the beginning of a declaration, and to report them using an instance of
> `DeferredLintHandler`. This allows the warnings to be enabled or disabled
> using the standard mechanisms for `-Xlint` and `@SuppressWarnings`. The
> procedure for handling dangling doc comments is described in this comment in
> `JavacParser`.
>
> * Dangling documentation comments are handled as follows.
> * 1. {@code Scanner} adds all doc comments to a queue of
> * recent doc comments. The queue is flushed whenever
> * it is known that the recent doc comments should be
> * ignored and should not cause any warnings.
> * 2. The primary documentation comment is the one obtained
> * from the first token of any declaration.
> * (using {@code token.getDocComment()}.
> * 3. At the end of the "signature" of the declaration
> * (that is, before any initialization or body for the
> * declaration) any other "recent" comments are saved
> * in a map using the primary comment as a key,
> * using this method, {@code saveDanglingComments}.
> * 4. When the tree node for the declaration is finally
> * available, and the primary comment, if any,
> * is "attached", (in {@link #attach}) any related
> * dangling comments are also attached to the tree node
> * by registering them using the {@link #deferredLintHandler}.
> * 5. (Later) Warnings may be genereated for the dangling
> * comments, subject to the {@code -Xlint} and
> * {@code @SuppressWarnings}.
>
>
> 3. Updates to the make files to disable the warnings in modules for which
> the
> warning is generated. This is often because of the confusing use of `/**` to
> create box or other standout comments.
Jonathan Gibbons has updated the pull request with a new target base due to a
merge or a rebase. The incremental webrev excludes the unrelated changes
brought in by the merge/rebase. The pull request contains five additional
commits since the last revision:
- improve handling of ignorable doc comments
suppress warning when building test code
- Merge remote-tracking branch 'upstream/master' into 8303689.dangling-comments
- call `saveDanglingDocComments` for local variable declarations
- adjust call for `saveDanglingDocComments` for enum members
- JDK-8303689: javac -Xlint could/should report on "dangling" doc comments
-
Changes:
- all: https://git.openjdk.org/jdk/pull/18527/files
- new: https://git.openjdk.org/jdk/pull/18527/files/3f745431..f3670e7a
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=18527=03
- incr: https://webrevs.openjdk.org/?repo=jdk=18527=02-03
Stats: 42074 lines in 1058 files changed: 18282 ins; 15937 del; 7855 mod
Patch: https://git.openjdk.org/jdk/pull/18527.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/18527/head:pull/18527
PR: https://git.openjdk.org/jdk/pull/18527
Re: RFR: 8261242: [Linux] OSContainer::is_containerized() returns true when run outside a container [v2]
On 4/18/24 2:54 AM, Severin Gehwolf wrote: On Wed, 17 Apr 2024 01:07:04 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. 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? @jankratochvil I believe this boils down to what we actually want. Should `OSContainer::is_containerized()` return `false` when run *inside* a container? If so, when is it OK to do that? Should `OSContainer::is_containerized()` return `true` on a physical Linux deployment? IMO, the read-only property of the mount points was something that fit naturally since, we already scan those anyway for (cgv1 vs cgv2 detection). Therefore it was something to consider to make heuristics more accurate. The truth table of the patch in this PR looks like this: | `OSContainer::is_containerized()` value | Actual deployment scenario | | - | - | | `true` | OpenJDK runs in an unprivileged container **without** a cpu/memory limit | | `true` | OpenJDK runs in an unprivileged container **with** a cpu/memory limit | | `true` | OpenJDK runs in a privileged container **with** a cpu/memory limit | | `false` | OpenJDK runs in a privileged container **without** a cpu/memory limit | | `false` | OpenJDK runs in a systemd slice **without** a cpu/memory limit | | `true` | OpenJDK runs in a systemd slice **with** a cpu/memory limit | | `false` | OpenJDK runs on a physical Linux system (VM or bare metal) | As you can see, the case of "OpenJDK runs in a privileged container *without* a cpu/memory limit" gives the wrong result. However, I consider this a fairly uncommon setup and there isn't really anything we can do to detect this kind of setup. Even if we did manage to detect it, why would we care? It's a question of probability. Could you give a countercase where my patch behaves wrongly? Again, it's not a case of right or wrong IMO. Since we are in the land of heuristics, they will be wrong in some cases. We should make them so that we cover the common cases and, perhaps, are able to use those in serviceability tools to help guide diagnosis and/or further tuning. So far the existing capabilities were OK, but prevent further out-of-the-box tuning and/or accurate data collection. I think (I am agreeing with you Severin) that the goal of the heuristic is to inform the JVM (and any associated serviceability tools) that the JVM is in a resource constrained/managed execution context... Rgds - Larry Your proposed patch (it's one I had in a previous iteration too, fwiw) would also return `false` for the case of "OpenJDK runs in an unprivileged container **without** a cpu/memory limit", which seems counterintuitive if OpenJDK actually runs in a container! What's more, it seems a fairly common case. Also, there is a chance of the OpenJDK heuristics to fail memory/cpu limit detection because of bugs and new developments. It seems the safer option to know that OpenJDK is containerized (using other heuristics) in that case. Maybe that's just me. Let's have that discussion more broadly and see if we can reach consensus! - PR Comment: https://git.openjdk.org/jdk/pull/18201#issuecomment-2063477204
Re: RFR: 8261242: [Linux] OSContainer::is_containerized() returns true when run outside a container [v2]
On 4/18/24 9:38 AM, Severin Gehwolf wrote: On Thu, 18 Apr 2024 13:27:38 GMT, Jan Kratochvil wrote: Could not we rename `is_containerized()` to `use_container_limit()` ? As that is the current only purpose of `is_containerized()`. I'm not sure. There is value to have `is_containerized()` like it would behave after this patch. Specifically the first table row difference in [your comment](https://github.com/openjdk/jdk/pull/18201#issuecomment-2063868908) concerns me. JVMs running in a container without limit wouldn't be detected as "containerized". That seems a large share of deployments to miss. agree 100% - PR Comment: https://git.openjdk.org/jdk/pull/18201#issuecomment-2064487567
Re: RFR: 8261242: [Linux] OSContainer::is_containerized() returns true when run outside a container [v2]
On Thu, 18 Apr 2024 13:27:38 GMT, Jan Kratochvil wrote: > Could not we rename `is_containerized()` to `use_container_limit()` ? As that > is the current only purpose of `is_containerized()`. I'm not sure. There is value to have `is_containerized()` like it would behave after this patch. Specifically the first table row difference in [your comment](https://github.com/openjdk/jdk/pull/18201#issuecomment-2063868908) concerns me. JVMs running in a container without limit wouldn't be detected as "containerized". That seems a large share of deployments to miss. - PR Comment: https://git.openjdk.org/jdk/pull/18201#issuecomment-2064487567
Re: RFR: 8330388: Remove invokedynamic cache index encoding [v2]
On Wed, 17 Apr 2024 22:41:08 GMT, Dean Long wrote: >> Matias Saavedra Silva has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Dean and Gilles comments > > 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`? `indy_index` is used frequently even in functions that are only used in the context of invokedynamic. I think it helps with clarity. - PR Review Comment: https://git.openjdk.org/jdk/pull/18819#discussion_r1571043673
Re: RFR: 8330388: Remove invokedynamic cache index encoding [v2]
> 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. Matias Saavedra Silva has updated the pull request incrementally with one additional commit since the last revision: Dean and Gilles comments - Changes: - all: https://git.openjdk.org/jdk/pull/18819/files - new: https://git.openjdk.org/jdk/pull/18819/files/87926aee..3ef92512 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18819=01 - incr: https://webrevs.openjdk.org/?repo=jdk=18819=00-01 Stats: 6 lines in 2 files changed: 0 ins; 1 del; 5 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: 8330388: Remove invokedynamic cache index encoding
On Wed, 17 Apr 2024 22:48:16 GMT, Dean Long wrote: > Did you consider minimizing changes by leaving > decode_invokedynamic_index/encode_invokedynamic_index calls in place, but > having the implementations not change the value? The intention from the start was to remove the encode/decode methods because they have been made unnecessary thanks to the changes mentioned in the description. As the author of the previously mentioned changes that overhauled the cpcache, this change should have been included in one of those PRs, but I must have forgotten! Leaving the calls in even if they did nothing would just make the code confusing and might become a red herring if other issues in the area come up. - PR Comment: https://git.openjdk.org/jdk/pull/18819#issuecomment-2064263944
Re: RFR: 8330388: Remove invokedynamic cache index encoding
On Wed, 17 Apr 2024 22:43:38 GMT, Dean Long 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/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? The last sentence is no longer valid, thanks for catching this! - PR Review Comment: https://git.openjdk.org/jdk/pull/18819#discussion_r1570969645
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 Waiting second review. - PR Comment: https://git.openjdk.org/jdk/pull/18768#issuecomment-2064136498
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 Could not we rename `is_containerized()` to `use_container_limit()` ? As that is the current only purpose of `is_containerized()`. I did not test it but I expect the values will be: | your patch | my trivial patch | Actual deployment scenario | |||| | `true` | `false` |OpenJDK runs in an unprivileged container **without** a cpu/memory limit | | `true` | `true` | OpenJDK runs in an unprivileged container **with** a cpu/memory limit | | `false` | `false` | OpenJDK runs in a privileged container **without** a cpu/memory limit | | `true` | `true` | OpenJDK runs in a privileged container **with** a cpu/memory limit | | `false` | `false` | OpenJDK runs in a systemd slice **without** a cpu/memory limit | | `true` | `true` | OpenJDK runs in a systemd slice **with** a cpu/memory limit | | `false` | `false` | OpenJDK runs on a physical Linux system (VM or bare metal) | - PR Comment: https://git.openjdk.org/jdk/pull/18201#issuecomment-2063868908
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. To borrow @shipilev's comment from a different PR, Good Riddance! - Marked as reviewed by coleenp (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18819#pullrequestreview-2008602360
Re: RFR: 8261242: [Linux] OSContainer::is_containerized() returns true when run outside a container [v2]
On Wed, 17 Apr 2024 01:07:04 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. > >> 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? @jankratochvil I believe this boils down to what we actually want. Should `OSContainer::is_containerized()` return `false` when run *inside* a container? If so, when is it OK to do that? Should `OSContainer::is_containerized()` return `true` on a physical Linux deployment? IMO, the read-only property of the mount points was something that fit naturally since, we already scan those anyway for (cgv1 vs cgv2 detection). Therefore it was something to consider to make heuristics more accurate. The truth table of the patch in this PR looks like this: | `OSContainer::is_containerized()` value | Actual deployment scenario | | - | - | | `true` | OpenJDK runs in an unprivileged container **without** a cpu/memory limit | | `true` | OpenJDK runs in an unprivileged container **with** a cpu/memory limit | | `true` | OpenJDK runs in a privileged container **with** a cpu/memory limit | | `false` | OpenJDK runs in a privileged container **without** a cpu/memory limit | | `false` | OpenJDK runs in a systemd slice **without** a cpu/memory limit | | `true` | OpenJDK runs in a systemd slice **with** a cpu/memory limit | | `false` | OpenJDK runs on a physical Linux system (VM or bare metal) | As you can see, the case of "OpenJDK runs in a privileged container *without* a cpu/memory limit" gives the wrong result. However, I consider this a fairly uncommon setup and there isn't really anything we can do to detect this kind of setup. Even if we did manage to detect it, why would we care? It's a question of probability. > Could you give a countercase where my patch behaves wrongly? Again, it's not a case of right or wrong IMO. Since we are in the land of heuristics, they will be wrong in some cases. We should make them so that we cover the common cases and, perhaps, are able to use those in serviceability tools to help guide diagnosis and/or further tuning. So far the existing capabilities were OK, but prevent further out-of-the-box tuning and/or accurate data collection. Your proposed patch (it's one I had in a previous iteration too, fwiw) would also return `false` for the case of "OpenJDK runs in an unprivileged container **without** a cpu/memory limit", which seems counterintuitive if OpenJDK actually runs in a container! What's more, it seems a fairly common case. Also, there is a chance of the OpenJDK heuristics to fail memory/cpu limit detection because of bugs and new developments. It seems the safer option to know that OpenJDK is containerized (using other heuristics) in that case. Maybe that's just me. Let's have that discussion more broadly and see if we can reach consensus! - PR Comment: https://git.openjdk.org/jdk/pull/18201#issuecomment-2063477204
Re: RFR: 8330388: Remove invokedynamic cache index encoding
On Wed, 17 Apr 2024 22:58:21 GMT, Dean Long wrote: > @dougxc should check JVMCI changes. Thanks for the heads up. I've asked @matias9927 to double check these changes against libgraal which should address any concerns about how this change impacts Graal. - PR Comment: https://git.openjdk.org/jdk/pull/18819#issuecomment-2063308834
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/jdk.internal.vm.ci/share/classes/jdk/vm/ci/hotspot/HotSpotConstantPool.java
line 720:
> 718: @Override
> 719: public JavaMethod lookupMethod(int rawIndex, int opcode,
> ResolvedJavaMethod caller) {
> 720: int which = rawIndex;
We could get rid of that intermediate variable now and just use `rawIndex`
below.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18819#discussion_r1570192972
