Re: RFR: 8337511: Implement JEP 404: Generational Shenandoah (Experimental) [v11]
On Tue, 26 Nov 2024 00:37:32 GMT, William Kemper wrote: >> This PR merges JEP 404, a generational mode for the Shenandoah garbage >> collector. The JEP can be viewed here: https://openjdk.org/jeps/404. We >> would like to target JDK24 with this PR. > > William Kemper has updated the pull request incrementally with two additional > commits since the last revision: > > - Merge remote-tracking branch 'shenandoah/master' into > great-genshen-pr-redux > - 8344985: GenShen: Refactor arraycopy barrier for generational mode > >Reviewed-by: shade Marked as reviewed by phh (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/21273#pullrequestreview-2466068316
Re: RFR: 8305895: Implement JEP 450: Compact Object Headers (Experimental) [v57]
On Thu, 7 Nov 2024 17:25:40 GMT, Roman Kennke wrote: >> This is the main body of the JEP 450: Compact Object Headers (Experimental). >> >> It is also a follow-up to #20640, which now also includes (and supersedes) >> #20603 and #20605, plus the Tiny Class-Pointers parts that have been >> previously missing. >> >> Main changes: >> - Introduction of the (experimental) flag UseCompactObjectHeaders. All >> changes in this PR are protected by this flag. The purpose of the flag is to >> provide a fallback, in case that users unexpectedly observe problems with >> the new implementation. The intention is that this flag will remain >> experimental and opt-in for at least one release, then make it on-by-default >> and diagnostic (?), and eventually deprecate and obsolete it. However, there >> are a few unknowns in that plan, specifically, we may want to further >> improve compact headers to 4 bytes, we are planning to enhance the Klass* >> encoding to support virtually unlimited number of Klasses, at which point we >> could also obsolete UseCompressedClassPointers. >> - The compressed Klass* can now be stored in the mark-word of objects. In >> order to be able to do this, we are add some changes to GC forwarding (see >> below) to protect the relevant (upper 22) bits of the mark-word. Significant >> parts of this PR deal with loading the compressed Klass* from the mark-word. >> This PR also changes some code paths (mostly in GCs) to be more careful when >> accessing Klass* (or mark-word or size) to be able to fetch it from the >> forwardee in case the object is forwarded. >> - Self-forwarding in GCs (which is used to deal with promotion failure) now >> uses a bit to indicate 'self-forwarding'. This is needed to preserve the >> crucial Klass* bits in the header. This also allows to get rid of >> preserved-header machinery in SerialGC and G1 (Parallel GC abuses >> preserved-marks to also find all other relevant oops). >> - Full GC forwarding now uses an encoding similar to compressed-oops. We >> have 40 bits for that, and can encode up to 8TB of heap. When exceeding 8TB, >> we turn off UseCompressedClassPointers (except in ZGC, which doesn't use the >> GC forwarding at all). >> - Instances can now have their base-offset (the offset where the field >> layouter starts to place fields) at offset 8 (instead of 12 or 16). >> - Arrays will now store their length at offset 8. >> - CDS can now write and read archives with the compressed header. However, >> it is not possible to read an archive that has been written with an opposite >> setting of UseCompactObjectHeaders. Some build machinery is added so that >> _co... > > Roman Kennke has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 107 commits: > > - Merge branch 'master' into JDK-8305895-v4 > - Merge tag 'jdk-25+23' into JDK-8305895-v4 > >Added tag jdk-24+23 for changeset c0e6c3b9 > - Fix gen-ZGC removal > - Merge tag 'jdk-24+22' into JDK-8305895-v4 > >Added tag jdk-24+22 for changeset 388d44fb > - Enable riscv in CompressedClassPointersEncodingScheme test > - s390 port > - Conditionalize platform specific parts of > CompressedClassPointersEncodingScheme test > - Update copyright > - Avoid assert/endless-loop in JFR code > - Update copyright headers > - ... and 97 more: https://git.openjdk.org/jdk/compare/d3c042f9...c1a6323b Marked as reviewed by phh (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/20677#pullrequestreview-2424210008
Re: RFR: 8335124: com/sun/management/ThreadMXBean/ThreadCpuTimeArray.java failed with CPU time out of expected range
On Thu, 27 Jun 2024 08:54:16 GMT, Kevin Walls wrote: > This test has had occasional failures for years, possibly forever. > A previous update made the test "othervm" which removed some interruptions, > but a time accounting problem remains. > > This change adds a simple sleep after observing that the test threads are all > sleeping. > > The idea is that threads may not have actually started sleeping when we > observe their java.lang.Thread.State as WAITING, they may use some CPU time > after that in order to actually get to sleep. Makes sense. - Marked as reviewed by phh (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19924#pullrequestreview-2146005577
Re: RFR: 8328592: hprof tests fail with -XX:-CompactStrings [v2]
On Wed, 20 Mar 2024 17:19:34 GMT, Aleksey Shipilev wrote: >> See the bug for symptoms. The tests are failing because hprof test library >> is confused about non-compact strings. >> >> Additional testing: >> - [x] `serviceability/HeapDump lib-test:all` with `-XX:-CompactStrings` now >> pass >> - [x] `serviceability/HeapDump lib-test:all` with `-XX:+CompactStrings` >> still pass > > Aleksey Shipilev 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: > > - Handle platform endianness better > - Merge branch 'master' into JDK-8328592-hprof-compact-strings > - Fix Marked as reviewed by phh (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18394#pullrequestreview-1950715132
Re: RFR: 8328592: hprof tests fail with -XX:-CompactStrings
On Wed, 20 Mar 2024 09:53:36 GMT, Aleksey Shipilev wrote: > See the bug for symptoms. The tests are failing because hprof test library is > confused about non-compact strings. > > Additional testing: > - [x] `serviceability/HeapDump lib-test:all` with `-XX:-CompactStrings` now > pass > - [x] `serviceability/HeapDump lib-test:all` with `-XX:+CompactStrings` > still pass Are we guaranteed that non-compact string char component bytes are stored in little-endian order? - PR Review: https://git.openjdk.org/jdk/pull/18394#pullrequestreview-1949113961
Re: RFR: 8324241: Always record evol_method deps to avoid excessive method flushing
On Sat, 20 Jan 2024 19:48:07 GMT, Volker Simonis wrote:
> Currently we don't record dependencies on redefined methods (i.e.
> `evol_method` dependencies) in JIT compiled methods if none of the
> `can_redefine_classes`, `can_retransform_classes` or
> `can_generate_breakpoint_events` JVMTI capabalities is set. This means that
> if a JVMTI agent which requests one of these capabilities is dynamically
> attached, all the methods which have been JIT compiled until that point, will
> be marked for deoptimization and flushed from the code cache. For large,
> warmed-up applications this mean deoptimization and instant recompilation of
> thousands if not then-thousands of methods, which can lead to dramatic
> performance/latency drop-downs for several minutes.
>
> One could argue that dynamic agent attach is now deprecated anyway (see [JEP
> 451: Prepare to Disallow the Dynamic Loading of
> Agents](https://openjdk.org/jeps/451)) and this problem could be solved by
> making the recording of `evol_method` dependencies dependent on the new
> `-XX:+EnableDynamicAgentLoading` flag isntead of the concrete JVMTI
> capabilities (because the presence of the flag indicates that an agent will
> be loaded eventually).
>
> But there a single, however important exception to this rule and that's JFR.
> JFR is advertised as low overhead profiler which can be enabled in production
> at any time. However, when JFR is started dynamically (e.g. through JCMD or
> JMX) it will silently load a HotSpot internl JVMTI agent which requests the
> `can_retransform_classes` and retransforms some classes. This will inevitably
> trigger the deoptimization of all compiled methods as described above.
>
> I'd therefor like to propose to *always* and unconditionally record
> `evol_method` dependencies in JIT compiled code by exporting the relevant
> properties right at startup in `init_globals()`:
> ```c++
> jint init_globals() {
>management_init();
>JvmtiExport::initialize_oop_storage();
> +#if INCLUDE_JVMTI
> + JvmtiExport::set_can_hotswap_or_post_breakpoint(true);
> + JvmtiExport::set_all_dependencies_are_recorded(true);
> +#endif
>
>
> My measurements indicate that the overhead of doing so is minimal (around 1%
> increase of nmethod size) and justifies the benefit. E.g. a Spring Petclinic
> application started with `-Xbatch -XX:+UnlockDiagnosticVMOptions
> -XX:+LogCompilation` compiles about ~11500 methods (~9000 with C1 and ~2500
> with C2) resulting in an aggregated nmethod size of around ~40bm.
> Additionally recording `evol_method` dependencies only increases this size be
> about 400kb. The ration remains about the same i...
Marked as reviewed by phh (Reviewer).
-
PR Review: https://git.openjdk.org/jdk/pull/17509#pullrequestreview-1836891644
Re: RFR: 8314029: Add file name parameter to Compiler.perfmap [v8]
On Mon, 11 Dec 2023 22:41:56 GMT, Yi-Fan Tsai wrote: >> `jcmd Compiler.perfmap` uses the hard-coded file name for a perf map: >> `/tmp/perf-%d.map`. This change adds an optional argument for specifying a >> file name. >> >> `jcmd PID help Compiler.perfmap` shows the following usage. >> >> >> Compiler.perfmap >> Write map file for Linux perf tool. >> >> Impact: Low >> >> Syntax : Compiler.perfmap [] >> >> Arguments: >> filename : [optional] Name of the map file (STRING, no default value) >> >> >> The following section of man page is also updated. (`man -l >> src/jdk.jcmd/share/man/jcmd.1`) >> >> >>Compiler.perfmap [arguments] (Linux only) >> Write map file for Linux perf tool. >> >> Impact: Low >> >> arguments: >> >> · filename: (Optional) Name of the map file (STRING, no >> default value) >> >> If filename is not specified, a default file name is chosen >> using the pid of the target JVM process. For example, if the pid is 12345, >> then >> the default filename will be /tmp/perf-12345.map. > > Yi-Fan Tsai has updated the pull request incrementally with one additional > commit since the last revision: > > Update copyright of PerfMapTest Shall I revert it? - PR Comment: https://git.openjdk.org/jdk/pull/15871#issuecomment-1861334362
Re: RFR: 8314029: Add file name parameter to Compiler.perfmap [v8]
On Mon, 11 Dec 2023 22:41:56 GMT, Yi-Fan Tsai wrote: >> `jcmd Compiler.perfmap` uses the hard-coded file name for a perf map: >> `/tmp/perf-%d.map`. This change adds an optional argument for specifying a >> file name. >> >> `jcmd PID help Compiler.perfmap` shows the following usage. >> >> >> Compiler.perfmap >> Write map file for Linux perf tool. >> >> Impact: Low >> >> Syntax : Compiler.perfmap [] >> >> Arguments: >> filename : [optional] Name of the map file (STRING, no default value) >> >> >> The following section of man page is also updated. (`man -l >> src/jdk.jcmd/share/man/jcmd.1`) >> >> >>Compiler.perfmap [arguments] (Linux only) >> Write map file for Linux perf tool. >> >> Impact: Low >> >> arguments: >> >> · filename: (Optional) Name of the map file (STRING, no >> default value) >> >> If filename is not specified, a default file name is chosen >> using the pid of the target JVM process. For example, if the pid is 12345, >> then >> the default filename will be /tmp/perf-12345.map. > > Yi-Fan Tsai has updated the pull request incrementally with one additional > commit since the last revision: > > Update copyright of PerfMapTest Thanks. - PR Comment: https://git.openjdk.org/jdk/pull/15871#issuecomment-1861689670
Integrated: 8313081: MonitoringSupport_lock should be unconditionally initialized after 8304074
On Tue, 25 Jul 2023 21:48:24 GMT, Paul Hohensee wrote: > MonitoringSupport_lock is initialized only when UseG1GC is true, but > [JDK-8304074](https://bugs.openjdk.org/browse/JDK-8304074) uses it to > implement getTotalThreadAllocatedBytes, which is available for all garbage > collectors. While the current code sets UseG1GC regardless of which collector > is specified, see FLAG_SET_ERGO_IF_DEFAULT(UseG1GC, true) in gcConfig.cpp, if > G1 isn't included in the Hotspot build or Hotspot is not running on a server > class machine (unlikely these days), the lock will not be initialized. The > lock's initialization should be unconditional. > > I updated ThreadAllocatedMemory.java to run the test using both G1 and Serial > collectors. This pull request has now been integrated. Changeset: a9d21c61 Author:Paul Hohensee URL: https://git.openjdk.org/jdk/commit/a9d21c61fb12a11e18c6bb8aa903e5a8e42473f1 Stats: 17 lines in 3 files changed: 12 ins; 2 del; 3 mod 8313081: MonitoringSupport_lock should be unconditionally initialized after 8304074 Reviewed-by: dholmes, sspitsyn, shade - PR: https://git.openjdk.org/jdk/pull/15028
Re: RFR: 8313081: MonitoringSupport_lock should be unconditionally initialized after 8304074 [v2]
On Wed, 26 Jul 2023 15:19:02 GMT, Paul Hohensee wrote: >> MonitoringSupport_lock is initialized only when UseG1GC is true, but >> [JDK-8304074](https://bugs.openjdk.org/browse/JDK-8304074) uses it to >> implement getTotalThreadAllocatedBytes, which is available for all garbage >> collectors. While the current code sets UseG1GC regardless of which >> collector is specified, see FLAG_SET_ERGO_IF_DEFAULT(UseG1GC, true) in >> gcConfig.cpp, if G1 isn't included in the Hotspot build or Hotspot is not >> running on a server class machine (unlikely these days), the lock will not >> be initialized. The lock's initialization should be unconditional. >> >> I updated ThreadAllocatedMemory.java to run the test using both G1 and >> Serial collectors. > > Paul Hohensee has updated the pull request incrementally with one additional > commit since the last revision: > > 8313081: MonitoringSupport_lock should be unconditionally initialized after > 8304074 linux-x86 langtools/tools/javac/varargs/warning/Warn4.java presubmit test failure is a crash in G1ConcurrentMark.cpp which appears unrelated. Rerun on my test box succeeded. - PR Comment: https://git.openjdk.org/jdk/pull/15028#issuecomment-1652255976
Re: RFR: 8313081: MonitoringSupport_lock should be unconditionally initialized after 8304074 [v2]
On Wed, 26 Jul 2023 15:19:02 GMT, Paul Hohensee wrote: >> MonitoringSupport_lock is initialized only when UseG1GC is true, but >> [JDK-8304074](https://bugs.openjdk.org/browse/JDK-8304074) uses it to >> implement getTotalThreadAllocatedBytes, which is available for all garbage >> collectors. While the current code sets UseG1GC regardless of which >> collector is specified, see FLAG_SET_ERGO_IF_DEFAULT(UseG1GC, true) in >> gcConfig.cpp, if G1 isn't included in the Hotspot build or Hotspot is not >> running on a server class machine (unlikely these days), the lock will not >> be initialized. The lock's initialization should be unconditional. >> >> I updated ThreadAllocatedMemory.java to run the test using both G1 and >> Serial collectors. > > Paul Hohensee has updated the pull request incrementally with one additional > commit since the last revision: > > 8313081: MonitoringSupport_lock should be unconditionally initialized after > 8304074 I filed [JDK-8313200](https://bugs.openjdk.org/browse/JDK-8313200) to track the windows-aarch64 build issue. - PR Comment: https://git.openjdk.org/jdk/pull/15028#issuecomment-1652116355
Re: RFR: 8313081: MonitoringSupport_lock should be unconditionally initialized after 8304074 [v2]
On Wed, 26 Jul 2023 15:19:02 GMT, Paul Hohensee wrote: >> MonitoringSupport_lock is initialized only when UseG1GC is true, but >> [JDK-8304074](https://bugs.openjdk.org/browse/JDK-8304074) uses it to >> implement getTotalThreadAllocatedBytes, which is available for all garbage >> collectors. While the current code sets UseG1GC regardless of which >> collector is specified, see FLAG_SET_ERGO_IF_DEFAULT(UseG1GC, true) in >> gcConfig.cpp, if G1 isn't included in the Hotspot build or Hotspot is not >> running on a server class machine (unlikely these days), the lock will not >> be initialized. The lock's initialization should be unconditional. >> >> I updated ThreadAllocatedMemory.java to run the test using both G1 and >> Serial collectors. > > Paul Hohensee has updated the pull request incrementally with one additional > commit since the last revision: > > 8313081: MonitoringSupport_lock should be unconditionally initialized after > 8304074 Thanks, Aleksey. Will push once pre-submit tests finish. - PR Comment: https://git.openjdk.org/jdk/pull/15028#issuecomment-1652050296
Re: RFR: 8313081: MonitoringSupport_lock should be unconditionally initialized after 8304074 [v2]
> MonitoringSupport_lock is initialized only when UseG1GC is true, but > [JDK-8304074](https://bugs.openjdk.org/browse/JDK-8304074) uses it to > implement getTotalThreadAllocatedBytes, which is available for all garbage > collectors. While the current code sets UseG1GC regardless of which collector > is specified, see FLAG_SET_ERGO_IF_DEFAULT(UseG1GC, true) in gcConfig.cpp, if > G1 isn't included in the Hotspot build or Hotspot is not running on a server > class machine (unlikely these days), the lock will not be initialized. The > lock's initialization should be unconditional. > > I updated ThreadAllocatedMemory.java to run the test using both G1 and Serial > collectors. Paul Hohensee has updated the pull request incrementally with one additional commit since the last revision: 8313081: MonitoringSupport_lock should be unconditionally initialized after 8304074 - Changes: - all: https://git.openjdk.org/jdk/pull/15028/files - new: https://git.openjdk.org/jdk/pull/15028/files/e84cb69c..f83eccf7 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15028&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15028&range=00-01 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/15028.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15028/head:pull/15028 PR: https://git.openjdk.org/jdk/pull/15028
Re: RFR: 8313081: MonitoringSupport_lock should be unconditionally initialized after 8304074
On Tue, 25 Jul 2023 21:48:24 GMT, Paul Hohensee wrote: > MonitoringSupport_lock is initialized only when UseG1GC is true, but > [JDK-8304074](https://bugs.openjdk.org/browse/JDK-8304074) uses it to > implement getTotalThreadAllocatedBytes, which is available for all garbage > collectors. While the current code sets UseG1GC regardless of which collector > is specified, see FLAG_SET_ERGO_IF_DEFAULT(UseG1GC, true) in gcConfig.cpp, if > G1 isn't included in the Hotspot build or Hotspot is not running on a server > class machine (unlikely these days), the lock will not be initialized. The > lock's initialization should be unconditional. > > I updated ThreadAllocatedMemory.java to run the test using both G1 and Serial > collectors. Aleksey, no, it doesn't break now because (1) we don't appear to test on non-server class machines, and (2) FLAG_SET_ERGO_IF_DEFAULT sets UseG1GC unconditionally regardless of which GC is specified on the command line because it (UseG1GC) hasn't also been specified on the command line. So, the assert will have no effect. - PR Comment: https://git.openjdk.org/jdk/pull/15028#issuecomment-1651986014
Re: RFR: 8313081: MonitoringSupport_lock should be unconditionally initialized after 8304074
On Tue, 25 Jul 2023 21:48:24 GMT, Paul Hohensee wrote: > MonitoringSupport_lock is initialized only when UseG1GC is true, but > [JDK-8304074](https://bugs.openjdk.org/browse/JDK-8304074) uses it to > implement getTotalThreadAllocatedBytes, which is available for all garbage > collectors. While the current code sets UseG1GC regardless of which collector > is specified, see FLAG_SET_ERGO_IF_DEFAULT(UseG1GC, true) in gcConfig.cpp, if > G1 isn't included in the Hotspot build or Hotspot is not running on a server > class machine (unlikely these days), the lock will not be initialized. The > lock's initialization should be unconditional. > > I updated ThreadAllocatedMemory.java to run the test using both G1 and Serial > collectors. windows-aarch64 pre-submit build failure is due to an unrelated aarch64-specific issue. - PR Comment: https://git.openjdk.org/jdk/pull/15028#issuecomment-1651808313
RFR: 8313081: MonitoringSupport_lock should be unconditionally initialized after 8304074
MonitoringSupport_lock is initialized only when UseG1GC is true, but [JDK-8304074](https://bugs.openjdk.org/browse/JDK-8304074) uses it to implement getTotalThreadAllocatedBytes, which is available for all garbage collectors. While the current code sets UseG1GC regardless of which collector is specified, see FLAG_SET_ERGO_IF_DEFAULT(UseG1GC, true) in gcConfig.cpp, if G1 isn't included in the Hotspot build or Hotspot is not running on a server class machine (unlikely these days), the lock will not be initialized. The lock's initialization should be unconditional. I updated ThreadAllocatedMemory.java to run the test using both G1 and Serial collectors. - Commit messages: - 8313081: MonitoringSupport_lock should be unconditionally initialized after 8304074 Changes: https://git.openjdk.org/jdk/pull/15028/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=15028&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8313081 Stats: 16 lines in 2 files changed: 11 ins; 2 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/15028.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15028/head:pull/15028 PR: https://git.openjdk.org/jdk/pull/15028
Re: RFR: 8309271: A way to align already compiled methods with compiler directives
On Wed, 24 May 2023 00:38:27 GMT, Dmitry Chuyko wrote: > Compiler Control (https://openjdk.org/jeps/165) provides method-context > dependent control of the JVM compilers (C1 and C2). The active directive > stack is built from the directive files passed with the > `-XX:CompilerDirectivesFile` diagnostic command-line option and the > Compiler.add_directives diagnostic command. It is also possible to clear all > directives or remove the top from the stack. > > A matching directive will be applied at method compilation time when such > compilation is started. If directives are added or changed, but compilation > does not start, then the state of compiled methods doesn't correspond to the > rules. This is not an error, and it happens in long running applications when > directives are added or removed after compilation of methods that could be > matched. For example, the user decides that C2 compilation needs to be > disabled for some method due to a compiler bug, issues such a directive but > this does not affect the application behavior. In such case, the target > application needs to be restarted, and such an operation can have high costs > and risks. Another goal is testing/debugging compilers. > > It would be convenient to optionally reconcile at least existing matching > nmethods to the current stack of compiler directives. Methods in general are > often inlined, and this information is hard to track down. > > Natural way to eliminate the discrepancy between the result of compilation > and the broken rule is to discard the compilation result, i.e. > deoptimization. Obviously there is a performance penalty, so it should be > applied with care. Hot code will most likely be recompiled soon, as nothing > happens to its hotness. > > A new flag '`-d`' has beed introduced for some directives related to compile > commands: `Compiler.add_directives`, `Compiler.remove_directives`, > `Compiler.clear_directives`. The default behavior has not changed (no flag). > If the new flag is present, the command scans already compiled methods and > marks for deoptimization those methods that have any active non-default > matching compiler directives. There is currently no distinction which > directives are found. In particular, this means that if there are rules for > inlining into some method, it will be deoptimized. On the other hand, if > there are rules for a method and it was inlined, top-level methods won't be > deoptimized, but this can be achieved by having rules for them. > > In addition, a new diagnistic command `Compiler.replace_directives`, has been > added for convenience. It's like a combinatio... "refresh" (-r) would be better than "deoptimize" (-d). The latter implies a specific implementation, the former is generic. If the method is to be recompiled, perhaps rather than deopt and wait, add it to the compile queue immediately and deopt the old version when the new compilation is complete, similar to what happens when the c1 version of the method is replaced by the c2 version. - PR Comment: https://git.openjdk.org/jdk/pull/14111#issuecomment-1583199824
Re: RFR: JDK-8307314: Implementation: Generational Shenandoah (Experimental) [v5]
On Sun, 4 Jun 2023 21:39:58 GMT, Kelvin Nilsen wrote: >> OpenJDK Colleagues: >> >> Please review this proposed integration of Generational mode for Shenandoah >> GC under https://bugs.openjdk.org/browse/JDK-8307314. >> >> Generational mode of Shenandoah is enabled by adding >> `-XX:+UnlockExperimentalVMOptions -XX:ShenandoahGCMode=generational` to a >> command line that already specifies ` -XX:+UseShenandoahGC`. The >> implementation automatically adjusts the sizes of old generation and young >> generation to efficiently utilize the entire heap capacity. Generational >> mode of Shenandoah resembles G1 in the following regards: >> >> 1. Old-generation marking runs concurrently during the time that multiple >> young generation collections run to completion. >> 2. After old-generation marking completes, we perform a sequence of mixed >> collections. Each mixed collection combines collection of young generation >> with evacuation of a portion of the old-generation regions identified for >> collection based on old-generation marking information. >> 3. Unlike G1, young-generation collections and evacuations are entirely >> concurrent, as with single-generation Shenandoah. >> 4. As with single-generation Shenandoah, there is no explicit notion of eden >> and survivor space within the young generation. In practice, regions that >> were most recently allocated tend to have large amounts of garbage and these >> regions tend to be collected with very little effort. Young-generation >> objects that survive garbage collection tend to accumulate in regions that >> hold survivor objects. These regions tend to have smaller amounts of >> garbage, and are less likely to be collected. If they survive a sufficient >> number of young-generation collections, the “survivor” regions are promoted >> into the old generation. >> >> We expect to refine heuristics as we gain experience with more production >> workloads. In the future, we plan to remove the “experimental” qualifier >> from generational mode, at which time we expect that generational mode will >> become the default mode for Shenandoah. >> >> **Testing**: We continuously run jtreg tiers 1-4 + hotspot_gc_shenandoah, >> gcstress, jck compiler, jck runtime, Dacapo, SpecJBB, SpecVM, Extremem, >> HyperAlloc, and multiple AWS production workload simulators. We test on >> Linux x64 and aarch64, Alpine x64 and aarch64, macOS x64 and aarch64, and >> Windows x64. > > Kelvin Nilsen has updated the pull request incrementally with one additional > commit since the last revision: > > Remove three asserts making comparisons between atomic volatile variables > > Though changes to the volatile variables are individually protected by > Atomic load and store operations, these asserts were not assuring > atomic access to multiple volatile variables, each of which could be > modified independently of the others. The asserts were therefore not > trustworthy, as has been confirmed by more extensive testing. We understand, and would not have proposed the last chance split-directory alternative without the level of isolation Hotspot's GC interface enables. We've added single-gen performance enhancements on the way and would like to keep them! - PR Comment: https://git.openjdk.org/jdk/pull/14185#issuecomment-1579439750
Re: RFR: JDK-8307314: Implementation: Generational Shenandoah (Experimental) [v5]
On Sun, 4 Jun 2023 21:39:58 GMT, Kelvin Nilsen wrote: >> OpenJDK Colleagues: >> >> Please review this proposed integration of Generational mode for Shenandoah >> GC under https://bugs.openjdk.org/browse/JDK-8307314. >> >> Generational mode of Shenandoah is enabled by adding >> `-XX:+UnlockExperimentalVMOptions -XX:ShenandoahGCMode=generational` to a >> command line that already specifies ` -XX:+UseShenandoahGC`. The >> implementation automatically adjusts the sizes of old generation and young >> generation to efficiently utilize the entire heap capacity. Generational >> mode of Shenandoah resembles G1 in the following regards: >> >> 1. Old-generation marking runs concurrently during the time that multiple >> young generation collections run to completion. >> 2. After old-generation marking completes, we perform a sequence of mixed >> collections. Each mixed collection combines collection of young generation >> with evacuation of a portion of the old-generation regions identified for >> collection based on old-generation marking information. >> 3. Unlike G1, young-generation collections and evacuations are entirely >> concurrent, as with single-generation Shenandoah. >> 4. As with single-generation Shenandoah, there is no explicit notion of eden >> and survivor space within the young generation. In practice, regions that >> were most recently allocated tend to have large amounts of garbage and these >> regions tend to be collected with very little effort. Young-generation >> objects that survive garbage collection tend to accumulate in regions that >> hold survivor objects. These regions tend to have smaller amounts of >> garbage, and are less likely to be collected. If they survive a sufficient >> number of young-generation collections, the “survivor” regions are promoted >> into the old generation. >> >> We expect to refine heuristics as we gain experience with more production >> workloads. In the future, we plan to remove the “experimental” qualifier >> from generational mode, at which time we expect that generational mode will >> become the default mode for Shenandoah. >> >> **Testing**: We continuously run jtreg tiers 1-4 + hotspot_gc_shenandoah, >> gcstress, jck compiler, jck runtime, Dacapo, SpecJBB, SpecVM, Extremem, >> HyperAlloc, and multiple AWS production workload simulators. We test on >> Linux x64 and aarch64, Alpine x64 and aarch64, macOS x64 and aarch64, and >> Windows x64. > > Kelvin Nilsen has updated the pull request incrementally with one additional > commit since the last revision: > > Remove three asserts making comparisons between atomic volatile variables > > Though changes to the volatile variables are individually protected by > Atomic load and store operations, these asserts were not assuring > atomic access to multiple volatile variables, each of which could be > modified independently of the others. The asserts were therefore not > trustworthy, as has been confirmed by more extensive testing. Thanks for finding the single-gen regression, we're very happy you took the time to run it and write up your results. We're very concerned about single-gen regressions too because we have single-gen Shen in production for several critical services. We'd like to propose to push now, and tackle/fix the single-gen issue you identified during RDP1, as well as any other significant single-gen regressions that may come up. We have four Shen experts on board, Roman, Aleksey, Kelvin, and William, so believe it's doable before RDP2 in July. In the worst case that we fail, we'd emulate ZGC and move GenShen to it's own directory as an entirely separate collector before RDP2. Make sense? - PR Comment: https://git.openjdk.org/jdk/pull/14185#issuecomment-1579351993
Re: RFR: 8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM [v23]
On Tue, 30 May 2023 14:20:12 GMT, Volker Simonis wrote: >> Fixed using a high water mark. > > A simple fix for guaranteeing monotonicity, would be to add another field > (e.g. `max_allocated_bytes`) which keeps the last result returned by > `getTotalThreadAllocatedBytes()` and the latter would then return the maximum > of the current computation and `max_allocated_bytes`. That's what I did. :) Thanks for your review! - PR Review Comment: https://git.openjdk.org/jdk/pull/13814#discussion_r1210491161
Integrated: 8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM
On Thu, 4 May 2023 19:54:57 GMT, Paul Hohensee wrote: > Please review this addition to com.sun.management.ThreadMXBean that returns > the total number of bytes allocated on the Java heap since JVM launch by both > terminated and live threads. > > Because this PR adds a new interface method, I've updated the JMM_VERSION to > 4, but would be happy to update it to 3_1 instead. This pull request has now been integrated. Changeset: 3eced01f Author:Paul Hohensee URL: https://git.openjdk.org/jdk/commit/3eced01f9efe2567a07b63343f8559683a2d0517 Stats: 298 lines in 11 files changed: 219 ins; 45 del; 34 mod 8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM Reviewed-by: dholmes, mchung - PR: https://git.openjdk.org/jdk/pull/13814
Re: RFR: 8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM [v29]
On Mon, 29 May 2023 17:51:17 GMT, Paul Hohensee wrote: >> Please review this addition to com.sun.management.ThreadMXBean that returns >> the total number of bytes allocated on the Java heap since JVM launch by >> both terminated and live threads. >> >> Because this PR adds a new interface method, I've updated the JMM_VERSION to >> 4, but would be happy to update it to 3_1 instead. > > Paul Hohensee has updated the pull request incrementally with one additional > commit since the last revision: > > 8304074: Change reportUnsupported argument name to when, add comment to > MonitoringSupport_lock declaration Thanks for your in-depth review! - PR Comment: https://git.openjdk.org/jdk/pull/13814#issuecomment-1568460763
Re: RFR: 8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM [v28]
On Mon, 29 May 2023 02:02:21 GMT, David Holmes wrote:
>> Paul Hohensee has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 100 commits:
>>
>> - 8304074: Remove extra file
>> - 8304074: Add getTotalAllocatedBytes high water mark
>> - Merge branch 'master' into 8304074
>> - 8304074: Refactor test exception handling
>> - Merge master
>> - Merge master
>> - 8304074: Change UnsupportedOperationException in javadoc comment to
>> {@code UnsupportedOperationException}
>> - 8306507: [linux] Print number of memory mappings in error reports
>>
>>Reviewed-by: adinn, sgehwolf
>> - 8300086: Replace NULL with nullptr in share/c1/
>>
>>Reviewed-by: thartmann, chagedorn
>> - 8308465: Reduce memory accesses in AArch64 MD5 intrinsic
>>
>>Reviewed-by: aph, phh
>> - ... and 90 more: https://git.openjdk.org/jdk/compare/77c5adb0...5f2f86bb
>
> test/jdk/com/sun/management/ThreadMXBean/ThreadAllocatedMemory.java line 305:
>
>> 303: }
>> 304:
>> 305: private static void reportUnexpected(Exception e, String reason) {
>
> nit: 'when' or 'where' seems a more appropriate name than 'reason'
Changed to "when".
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13814#discussion_r1209483911
Re: RFR: 8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM [v28]
On Fri, 26 May 2023 16:41:32 GMT, Paul Hohensee wrote:
>> Please review this addition to com.sun.management.ThreadMXBean that returns
>> the total number of bytes allocated on the Java heap since JVM launch by
>> both terminated and live threads.
>>
>> Because this PR adds a new interface method, I've updated the JMM_VERSION to
>> 4, but would be happy to update it to 3_1 instead.
>
> Paul Hohensee has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains 100 commits:
>
> - 8304074: Remove extra file
> - 8304074: Add getTotalAllocatedBytes high water mark
> - Merge branch 'master' into 8304074
> - 8304074: Refactor test exception handling
> - Merge master
> - Merge master
> - 8304074: Change UnsupportedOperationException in javadoc comment to {@code
> UnsupportedOperationException}
> - 8306507: [linux] Print number of memory mappings in error reports
>
>Reviewed-by: adinn, sgehwolf
> - 8300086: Replace NULL with nullptr in share/c1/
>
>Reviewed-by: thartmann, chagedorn
> - 8308465: Reduce memory accesses in AArch64 MD5 intrinsic
>
>Reviewed-by: aph, phh
> - ... and 90 more: https://git.openjdk.org/jdk/compare/77c5adb0...5f2f86bb
Changed the MonitoringSupport_lock comment to "Protects updates to the
serviceability memory pools and allocated memory high water mark". The lock was
introduced to serialize updates to the G1 memory pool stats, but given that
doesn't happen very often, and the name, imo it's reasonable to use it in a
more general fashion.
-
PR Comment: https://git.openjdk.org/jdk/pull/13814#issuecomment-1567380618
Re: RFR: 8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM [v29]
> Please review this addition to com.sun.management.ThreadMXBean that returns > the total number of bytes allocated on the Java heap since JVM launch by both > terminated and live threads. > > Because this PR adds a new interface method, I've updated the JMM_VERSION to > 4, but would be happy to update it to 3_1 instead. Paul Hohensee has updated the pull request incrementally with one additional commit since the last revision: 8304074: Change reportUnsupported argument name to when, add comment to MonitoringSupport_lock declaration - Changes: - all: https://git.openjdk.org/jdk/pull/13814/files - new: https://git.openjdk.org/jdk/pull/13814/files/5f2f86bb..48a5852e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13814&range=28 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13814&range=27-28 Stats: 5 lines in 3 files changed: 0 ins; 0 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/13814.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13814/head:pull/13814 PR: https://git.openjdk.org/jdk/pull/13814
Re: RFR: 8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM [v28]
On Fri, 26 May 2023 16:41:32 GMT, Paul Hohensee wrote:
>> Please review this addition to com.sun.management.ThreadMXBean that returns
>> the total number of bytes allocated on the Java heap since JVM launch by
>> both terminated and live threads.
>>
>> Because this PR adds a new interface method, I've updated the JMM_VERSION to
>> 4, but would be happy to update it to 3_1 instead.
>
> Paul Hohensee has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains 100 commits:
>
> - 8304074: Remove extra file
> - 8304074: Add getTotalAllocatedBytes high water mark
> - Merge branch 'master' into 8304074
> - 8304074: Refactor test exception handling
> - Merge master
> - Merge master
> - 8304074: Change UnsupportedOperationException in javadoc comment to {@code
> UnsupportedOperationException}
> - 8306507: [linux] Print number of memory mappings in error reports
>
>Reviewed-by: adinn, sgehwolf
> - 8300086: Replace NULL with nullptr in share/c1/
>
>Reviewed-by: thartmann, chagedorn
> - 8308465: Reduce memory accesses in AArch64 MD5 intrinsic
>
>Reviewed-by: aph, phh
> - ... and 90 more: https://git.openjdk.org/jdk/compare/77c5adb0...5f2f86bb
Thanks for your review, Mandy.
-
PR Comment: https://git.openjdk.org/jdk/pull/13814#issuecomment-1564800041
Re: RFR: 8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM [v23]
On Thu, 25 May 2023 16:38:43 GMT, Mandy Chung wrote: >> The original implementation grabbed this code from, e.g., >> test/jdk/java/lang/management/ThreadMXBean/ThreadUserTime.java. In the >> latter, InterruptedException causes the test to fail, but for some reason >> now lost, ThreadAllocatedMemory.java doesn't do that, but >> ThreadAllocatedMemoryArray.java does. > > Since you are on this file, it may be good to clean them up. Done. - PR Review Comment: https://git.openjdk.org/jdk/pull/13814#discussion_r1207104630
Re: RFR: 8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM [v23]
On Thu, 25 May 2023 09:01:14 GMT, Kevin Walls wrote: >> src/hotspot/share/services/management.cpp line 2104: >> >>> 2102: // the final result can only be short due to (1) threads that >>> start after >>> 2103: // the TLH is created, or (2) terminating threads that escape TLH >>> creation >>> 2104: // and don't update exited_allocated_bytes before we initialize >>> result. >> >> Okay this is why you can't do monotonicity check in the tests. Imagine we >> have 10 threads all live and we call this function and so tally up all 10 >> threads. The next time we call this 5 of the threads are in the process of >> terminating - they have escaped the TLH but not yet updated the >> exited_allocated_bytes - so now we only tally up 5 threads and so the total >> appears to be far less than last time. > > The tests and all apps might thank us for keeping last known value, and not > returning anything lower, ensuring montonicity. Fixed using a high water mark. - PR Review Comment: https://git.openjdk.org/jdk/pull/13814#discussion_r1207049618
Re: RFR: 8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM [v28]
> Please review this addition to com.sun.management.ThreadMXBean that returns
> the total number of bytes allocated on the Java heap since JVM launch by both
> terminated and live threads.
>
> Because this PR adds a new interface method, I've updated the JMM_VERSION to
> 4, but would be happy to update it to 3_1 instead.
Paul Hohensee has updated the pull request with a new target base due to a
merge or a rebase. The pull request now contains 100 commits:
- 8304074: Remove extra file
- 8304074: Add getTotalAllocatedBytes high water mark
- Merge branch 'master' into 8304074
- 8304074: Refactor test exception handling
- Merge master
- Merge master
- 8304074: Change UnsupportedOperationException in javadoc comment to {@code
UnsupportedOperationException}
- 8306507: [linux] Print number of memory mappings in error reports
Reviewed-by: adinn, sgehwolf
- 8300086: Replace NULL with nullptr in share/c1/
Reviewed-by: thartmann, chagedorn
- 8308465: Reduce memory accesses in AArch64 MD5 intrinsic
Reviewed-by: aph, phh
- ... and 90 more: https://git.openjdk.org/jdk/compare/77c5adb0...5f2f86bb
-
Changes: https://git.openjdk.org/jdk/pull/13814/files
Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=13814&range=27
Stats: 297 lines in 10 files changed: 219 ins; 45 del; 33 mod
Patch: https://git.openjdk.org/jdk/pull/13814.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/13814/head:pull/13814
PR: https://git.openjdk.org/jdk/pull/13814
Re: RFR: 8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM [v27]
On Fri, 26 May 2023 15:37:32 GMT, Paul Hohensee wrote: >> Please review this addition to com.sun.management.ThreadMXBean that returns >> the total number of bytes allocated on the Java heap since JVM launch by >> both terminated and live threads. >> >> Because this PR adds a new interface method, I've updated the JMM_VERSION to >> 4, but would be happy to update it to 3_1 instead. > > Paul Hohensee has updated the pull request incrementally with one additional > commit since the last revision: > > 8304074: Refactor test exception handling I added a high water mark to getTotalThreadAllocatedBytes (under a lock, lest we have a monotonicity issue on the high water mark), and updated the tests per Mandy's request. - PR Comment: https://git.openjdk.org/jdk/pull/13814#issuecomment-1564648234
Re: RFR: 8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM [v27]
> Please review this addition to com.sun.management.ThreadMXBean that returns > the total number of bytes allocated on the Java heap since JVM launch by both > terminated and live threads. > > Because this PR adds a new interface method, I've updated the JMM_VERSION to > 4, but would be happy to update it to 3_1 instead. Paul Hohensee has updated the pull request incrementally with one additional commit since the last revision: 8304074: Refactor test exception handling - Changes: - all: https://git.openjdk.org/jdk/pull/13814/files - new: https://git.openjdk.org/jdk/pull/13814/files/96cb0e5b..c2473337 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13814&range=26 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13814&range=25-26 Stats: 38 lines in 2 files changed: 17 ins; 13 del; 8 mod Patch: https://git.openjdk.org/jdk/pull/13814.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13814/head:pull/13814 PR: https://git.openjdk.org/jdk/pull/13814
Re: RFR: 8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM [v26]
On Wed, 24 May 2023 16:50:55 GMT, Paul Hohensee wrote: >> Please review this addition to com.sun.management.ThreadMXBean that returns >> the total number of bytes allocated on the Java heap since JVM launch by >> both terminated and live threads. >> >> Because this PR adds a new interface method, I've updated the JMM_VERSION to >> 4, but would be happy to update it to 3_1 instead. > > Paul Hohensee has updated the pull request incrementally with one additional > commit since the last revision: > > Merge master Yes, I saw your comment and have been thinking about how to fix it, but haven't come up with anything. Ramki (ysr) pointed out that walking the thread list using SMR is very expensive when there are 1000s of threads, and that there isn't much overhead to collecting per-thread statistics: a thread local counter is incremented when TLABs are retired or there are direct allocations of large objects in eden or the old gen. So, he suggested adding an atomic increment of a global counter whenever a thread local counter is incremented. Atomic operations have been completing in the LLC (i.e., on-chip) for some time now, so the overhead is pretty small. What do you think, shall I reimplement that way? - PR Comment: https://git.openjdk.org/jdk/pull/13814#issuecomment-1562983030
Re: RFR: 8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM [v23]
On Tue, 23 May 2023 18:25:26 GMT, Mandy Chung wrote:
>> Paul Hohensee has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8304074: atomic load needed in exited_allocated_bytes
>
> test/jdk/com/sun/management/ThreadMXBean/ThreadAllocatedMemory.java line 160:
>
>> 158: try {
>> 159: curThread.join();
>> 160: } catch (InterruptedException e) {
>
> should it just let `InterruptedException` be thrown rather than swallowing
> it? This pattern appears multiple places in this test and
> ThreadAllocatedMemoryArray.java test.
The original implementation grabbed this code from, e.g.,
test/jdk/java/lang/management/ThreadMXBean/ThreadUserTime.java. In the latter,
InterruptedException causes the test to fail, but for some reason now lost,
ThreadAllocatedMemory.java doesn't do that, but ThreadAllocatedMemoryArray.java
does.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13814#discussion_r1204504529
Re: RFR: 8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM [v26]
> Please review this addition to com.sun.management.ThreadMXBean that returns > the total number of bytes allocated on the Java heap since JVM launch by both > terminated and live threads. > > Because this PR adds a new interface method, I've updated the JMM_VERSION to > 4, but would be happy to update it to 3_1 instead. Paul Hohensee has updated the pull request incrementally with one additional commit since the last revision: Merge master - Changes: - all: https://git.openjdk.org/jdk/pull/13814/files - new: https://git.openjdk.org/jdk/pull/13814/files/7b90091b..96cb0e5b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13814&range=25 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13814&range=24-25 Stats: 17 lines in 1 file changed: 0 ins; 16 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/13814.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13814/head:pull/13814 PR: https://git.openjdk.org/jdk/pull/13814
Re: RFR: 8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM [v25]
> Please review this addition to com.sun.management.ThreadMXBean that returns
> the total number of bytes allocated on the Java heap since JVM launch by both
> terminated and live threads.
>
> Because this PR adds a new interface method, I've updated the JMM_VERSION to
> 4, but would be happy to update it to 3_1 instead.
Paul Hohensee 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 95 additional commits
since the last revision:
- Merge master
- 8304074: Change UnsupportedOperationException in javadoc comment to {@code
UnsupportedOperationException}
- 8306507: [linux] Print number of memory mappings in error reports
Reviewed-by: adinn, sgehwolf
- 8300086: Replace NULL with nullptr in share/c1/
Reviewed-by: thartmann, chagedorn
- 8308465: Reduce memory accesses in AArch64 MD5 intrinsic
Reviewed-by: aph, phh
- 8302218: CHeapBitMap::free frees with incorrect size
Reviewed-by: aboldtch, iklam, tschatzl
- 8308388: Update description of SourceVersion.RELEASE_21
Reviewed-by: iris
- 8307190: Refactor ref_at methods in Constant Pool
Reviewed-by: coleenp, iklam
- 8308458: Windows build failure with disassembler.cpp(792): warning C4267:
'=': conversion from 'size_t' to 'int'
Reviewed-by: jiefu
- 8308034: Some CDS tests need to use @requires vm.flagless
Reviewed-by: iklam
- ... and 85 more: https://git.openjdk.org/jdk/compare/20185d2c...7b90091b
-
Changes:
- all: https://git.openjdk.org/jdk/pull/13814/files
- new: https://git.openjdk.org/jdk/pull/13814/files/638756de..7b90091b
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=13814&range=24
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=13814&range=23-24
Stats: 10880 lines in 222 files changed: 7954 ins; 1723 del; 1203 mod
Patch: https://git.openjdk.org/jdk/pull/13814.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/13814/head:pull/13814
PR: https://git.openjdk.org/jdk/pull/13814
Re: RFR: 8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM [v24]
On Wed, 24 May 2023 14:40:21 GMT, Paul Hohensee wrote:
>> Please review this addition to com.sun.management.ThreadMXBean that returns
>> the total number of bytes allocated on the Java heap since JVM launch by
>> both terminated and live threads.
>>
>> Because this PR adds a new interface method, I've updated the JMM_VERSION to
>> 4, but would be happy to update it to 3_1 instead.
>
> Paul Hohensee has updated the pull request incrementally with 63 additional
> commits since the last revision:
>
> - 8304074: Change UnsupportedOperationException in javadoc comment to {@code
> UnsupportedOperationException}
> - 8306507: [linux] Print number of memory mappings in error reports
>
>Reviewed-by: adinn, sgehwolf
> - 8300086: Replace NULL with nullptr in share/c1/
>
>Reviewed-by: thartmann, chagedorn
> - 8308465: Reduce memory accesses in AArch64 MD5 intrinsic
>
>Reviewed-by: aph, phh
> - 8302218: CHeapBitMap::free frees with incorrect size
>
>Reviewed-by: aboldtch, iklam, tschatzl
> - 8308388: Update description of SourceVersion.RELEASE_21
>
>Reviewed-by: iris
> - 8307190: Refactor ref_at methods in Constant Pool
>
>Reviewed-by: coleenp, iklam
> - 8308458: Windows build failure with disassembler.cpp(792): warning C4267:
> '=': conversion from 'size_t' to 'int'
>
>Reviewed-by: jiefu
> - 8308034: Some CDS tests need to use @requires vm.flagless
>
>Reviewed-by: iklam
> - 8307573: Implementation of JEP 449: Deprecate the Windows 32-bit x86 Port
> for Removal
>
>Reviewed-by: erikj
> - ... and 53 more: https://git.openjdk.org/jdk/compare/7ee35da0...638756de
Yes, it seems to have happened when I did a merge of this branch with jdk
master, which latter must have been in a strange state. Unfortunately, git
merge didn't complain at all, so I didn't check before pushing.
-
PR Comment: https://git.openjdk.org/jdk/pull/13814#issuecomment-1561553206
Re: RFR: 8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM [v23]
On Tue, 23 May 2023 18:21:18 GMT, Mandy Chung wrote:
>> Paul Hohensee has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8304074: atomic load needed in exited_allocated_bytes
>
> src/jdk.management/share/classes/com/sun/management/ThreadMXBean.java line
> 116:
>
>> 114: * recorded.
>> 115: *
>> 116: * @implSpec The default implementation throws
>> UnsupportedOperationException
>
> Suggestion:
>
> * @implSpec The default implementation throws {@code
> UnsupportedOperationException}
Done.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13814#discussion_r1204267595
Re: RFR: 8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM [v24]
> Please review this addition to com.sun.management.ThreadMXBean that returns
> the total number of bytes allocated on the Java heap since JVM launch by both
> terminated and live threads.
>
> Because this PR adds a new interface method, I've updated the JMM_VERSION to
> 4, but would be happy to update it to 3_1 instead.
Paul Hohensee has updated the pull request incrementally with 63 additional
commits since the last revision:
- 8304074: Change UnsupportedOperationException in javadoc comment to {@code
UnsupportedOperationException}
- 8306507: [linux] Print number of memory mappings in error reports
Reviewed-by: adinn, sgehwolf
- 8300086: Replace NULL with nullptr in share/c1/
Reviewed-by: thartmann, chagedorn
- 8308465: Reduce memory accesses in AArch64 MD5 intrinsic
Reviewed-by: aph, phh
- 8302218: CHeapBitMap::free frees with incorrect size
Reviewed-by: aboldtch, iklam, tschatzl
- 8308388: Update description of SourceVersion.RELEASE_21
Reviewed-by: iris
- 8307190: Refactor ref_at methods in Constant Pool
Reviewed-by: coleenp, iklam
- 8308458: Windows build failure with disassembler.cpp(792): warning C4267:
'=': conversion from 'size_t' to 'int'
Reviewed-by: jiefu
- 8308034: Some CDS tests need to use @requires vm.flagless
Reviewed-by: iklam
- 8307573: Implementation of JEP 449: Deprecate the Windows 32-bit x86 Port
for Removal
Reviewed-by: erikj
- ... and 53 more: https://git.openjdk.org/jdk/compare/7ee35da0...638756de
-
Changes:
- all: https://git.openjdk.org/jdk/pull/13814/files
- new: https://git.openjdk.org/jdk/pull/13814/files/7ee35da0..638756de
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=13814&range=23
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=13814&range=22-23
Stats: 22538 lines in 726 files changed: 12148 ins; 6106 del; 4284 mod
Patch: https://git.openjdk.org/jdk/pull/13814.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/13814/head:pull/13814
PR: https://git.openjdk.org/jdk/pull/13814
Re: RFR: 8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM [v23]
On Fri, 19 May 2023 13:54:13 GMT, Paul Hohensee wrote: >> Please review this addition to com.sun.management.ThreadMXBean that returns >> the total number of bytes allocated on the Java heap since JVM launch by >> both terminated and live threads. >> >> Because this PR adds a new interface method, I've updated the JMM_VERSION to >> 4, but would be happy to update it to 3_1 instead. > > Paul Hohensee has updated the pull request incrementally with one additional > commit since the last revision: > > 8304074: atomic load needed in exited_allocated_bytes Nope, test failures went away with the latest revision. I'm at a loss on that one. Perhaps it was the atomic load. - PR Comment: https://git.openjdk.org/jdk/pull/13814#issuecomment-1557466320
Re: RFR: 8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM [v22]
On Fri, 19 May 2023 03:02:33 GMT, David Holmes wrote:
>> Paul Hohensee has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8304074: Need acquire/release in incr_exited_allocated_bytes
>
> src/hotspot/share/services/threadService.hpp line 109:
>
>> 107: static jlong get_daemon_thread_count() { return
>> _atomic_daemon_threads_count; }
>> 108:
>> 109: static jlong exited_allocated_bytes() { return
>> _exited_allocated_bytes; }
>
> This needs to be an atomic::load too .
Done.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13814#discussion_r1199651489
Re: RFR: 8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM [v22]
On Fri, 19 May 2023 03:08:40 GMT, David Holmes wrote: >> Paul Hohensee has updated the pull request incrementally with one additional >> commit since the last revision: >> >> 8304074: Need acquire/release in incr_exited_allocated_bytes > > src/hotspot/share/services/threadService.hpp line 117: > >> 115: // store to _exited_allocated_bytes above this one. >> 116: jlong old = Atomic::load_acquire(&_exited_allocated_bytes); >> 117: Atomic::release_store(&_exited_allocated_bytes, old + size); > > First yes Atomic::load is not needed here due to the lock, it is needed in > the accessor above - sorry that wasn't clear. The Atomic store is needed of > course. > > But the load_acquire is definitely not needed as there is an implicit acquire > when the lock is acquired, so any load of the field here must see the last > value written under the lock. The release_store is also not needed because a > release is for ensuring visibility of previous stores of which there are > none, and has no affect on the visibility of the store done by the > `release_store`. > > So lets imagine a really weakly-ordered piece of hardware where all of the > writing threads are completely properly synchronized by the use of the lock > (else the hw is broken), but a lock-free reader can potentially see any of > those writes out-of-order. I think the only way to guarantee seeing the most > recent write would be to issue a full fence() before the load. But I would be > very surprised if any of our hardware actually operates in such a fashion. Thanks for the explanation. I reverted the acquire/release and the atomic load in incr_exited_allocated_bytes and added an atomic load in exited_allocated_bytes. - PR Review Comment: https://git.openjdk.org/jdk/pull/13814#discussion_r1198986781
Re: RFR: 8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM [v23]
> Please review this addition to com.sun.management.ThreadMXBean that returns > the total number of bytes allocated on the Java heap since JVM launch by both > terminated and live threads. > > Because this PR adds a new interface method, I've updated the JMM_VERSION to > 4, but would be happy to update it to 3_1 instead. Paul Hohensee has updated the pull request incrementally with one additional commit since the last revision: 8304074: atomic load needed in exited_allocated_bytes - Changes: - all: https://git.openjdk.org/jdk/pull/13814/files - new: https://git.openjdk.org/jdk/pull/13814/files/7d8252ff..7ee35da0 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13814&range=22 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13814&range=21-22 Stats: 6 lines in 1 file changed: 0 ins; 3 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/13814.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13814/head:pull/13814 PR: https://git.openjdk.org/jdk/pull/13814
Re: RFR: 8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM [v19]
On Thu, 18 May 2023 12:41:49 GMT, Paul Hohensee wrote: >> src/hotspot/share/services/threadService.hpp line 113: >> >>> 111: // No need for atomicity, method is called under the Threads_lock >>> 112: _exited_allocated_bytes += size; >>> 113: } >> >> As there is a lock-free read however you need to use Atomic::load and >> Atomic::store - especially for 32-bit systems. > > Changed. Not sure atomic load is needed. Even though this version passed the pre-submit tests, which include the new versions of ThreadAllocatedBytes.java, running it more often on a host with many hardware threads resulted in failures where getTotalAllocatedBytes did not monotonically increase. I believe what happened was that without an acquire/release, the hardware can float earlier stores to _exited_allocated_bytes above later ones. Adding acquire/release fixed the issue. - PR Review Comment: https://git.openjdk.org/jdk/pull/13814#discussion_r1197862479
Re: RFR: 8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM [v22]
> Please review this addition to com.sun.management.ThreadMXBean that returns > the total number of bytes allocated on the Java heap since JVM launch by both > terminated and live threads. > > Because this PR adds a new interface method, I've updated the JMM_VERSION to > 4, but would be happy to update it to 3_1 instead. Paul Hohensee has updated the pull request incrementally with one additional commit since the last revision: 8304074: Need acquire/release in incr_exited_allocated_bytes - Changes: - all: https://git.openjdk.org/jdk/pull/13814/files - new: https://git.openjdk.org/jdk/pull/13814/files/86be9237..7d8252ff Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13814&range=21 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13814&range=20-21 Stats: 5 lines in 1 file changed: 2 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/13814.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13814/head:pull/13814 PR: https://git.openjdk.org/jdk/pull/13814
Re: RFR: 8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM [v19]
On Tue, 16 May 2023 04:34:29 GMT, David Holmes wrote: >> Paul Hohensee 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 26 additional >> commits since the last revision: >> >> - 8304704: _exited_allocated_bytes increment does not have to be atomic >> - Merge branch 'master' into 8304074 >> - 8304704: _exited_allocated_bytes increment does not have to be atomic >> - 8304704: In getTotalThreadAllocatedBytes javadoc, 'launched' -> 'started' >> - 8304704: Typos >> - 8304074: fetch_then_add, not fetch_and_add >> - 8304074: Assign to _add_long_func in add_long_bootstrap >> - Merge branch 'master' into 8304074 >> - 8304074: Define FULL_MEM_BARRIER for linux_arm >> - 8304074: set _atomic_add_long_entry = ShouldNotCallThisStub() >> - ... and 16 more: https://git.openjdk.org/jdk/compare/1517b64a...effdefc6 > > src/hotspot/share/services/threadService.hpp line 113: > >> 111: // No need for atomicity, method is called under the Threads_lock >> 112: _exited_allocated_bytes += size; >> 113: } > > As there is a lock-free read however you need to use Atomic::load and > Atomic::store - especially for 32-bit systems. Changed. Not sure atomic load is needed. > test/jdk/com/sun/management/ThreadMXBean/ThreadAllocatedMemory.java line 228: > >> 226: >> 227: // start threads >> 228: done = false; done1 = false; > > Nit: separate lines please Fixed. - PR Review Comment: https://git.openjdk.org/jdk/pull/13814#discussion_r1197767814 PR Review Comment: https://git.openjdk.org/jdk/pull/13814#discussion_r1197767956
Re: RFR: 8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM [v21]
> Please review this addition to com.sun.management.ThreadMXBean that returns > the total number of bytes allocated on the Java heap since JVM launch by both > terminated and live threads. > > Because this PR adds a new interface method, I've updated the JMM_VERSION to > 4, but would be happy to update it to 3_1 instead. Paul Hohensee 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 29 additional commits since the last revision: - 8304074: initialize done and done1 on separate lines - Merge branch 'master' into 8304074 - 8304074: incr_exited_allocated_bytes needs atomic load/store - 8304704: _exited_allocated_bytes increment does not have to be atomic - Merge branch 'master' into 8304074 - 8304704: _exited_allocated_bytes increment does not have to be atomic - 8304704: In getTotalThreadAllocatedBytes javadoc, 'launched' -> 'started' - 8304704: Typos - 8304074: fetch_then_add, not fetch_and_add - 8304074: Assign to _add_long_func in add_long_bootstrap - ... and 19 more: https://git.openjdk.org/jdk/compare/95e39e85...86be9237 - Changes: - all: https://git.openjdk.org/jdk/pull/13814/files - new: https://git.openjdk.org/jdk/pull/13814/files/3c543e60..86be9237 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13814&range=20 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13814&range=19-20 Stats: 7860 lines in 341 files changed: 4624 ins; 989 del; 2247 mod Patch: https://git.openjdk.org/jdk/pull/13814.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13814/head:pull/13814 PR: https://git.openjdk.org/jdk/pull/13814
Re: RFR: 8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM [v20]
> Please review this addition to com.sun.management.ThreadMXBean that returns > the total number of bytes allocated on the Java heap since JVM launch by both > terminated and live threads. > > Because this PR adds a new interface method, I've updated the JMM_VERSION to > 4, but would be happy to update it to 3_1 instead. Paul Hohensee has updated the pull request incrementally with one additional commit since the last revision: 8304074: incr_exited_allocated_bytes needs atomic load/store - Changes: - all: https://git.openjdk.org/jdk/pull/13814/files - new: https://git.openjdk.org/jdk/pull/13814/files/effdefc6..3c543e60 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13814&range=19 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13814&range=18-19 Stats: 5 lines in 1 file changed: 3 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/13814.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13814/head:pull/13814 PR: https://git.openjdk.org/jdk/pull/13814
Re: RFR: 8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM [v5]
On Mon, 8 May 2023 02:15:03 GMT, David Holmes wrote: >> Paul Hohensee has updated the pull request incrementally with one additional >> commit since the last revision: >> >> 8304074: [JMX] Add an approximation of total bytes allocated on the Java >> heap by the JVM > > src/jdk.management/share/classes/com/sun/management/ThreadMXBean.java line > 111: > >> 109: * Returns an approximation of the total amount of memory, in bytes, >> 110: * allocated in heap memory since the Java virtual machine was >> launched, >> 111: * including the amount allocated by terminated threads. > > This "including ..." part seems redundant - it is the value allocated since > JVM launch. Removed. - PR Review Comment: https://git.openjdk.org/jdk/pull/13814#discussion_r1194446883
Re: RFR: 8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM [v11]
On Tue, 9 May 2023 01:40:20 GMT, Paul Hohensee wrote: >> src/hotspot/share/runtime/atomic.hpp line 310: >> >>> 308: >>> 309: // Platform-specific implementation of add. Support for sizes of 4 >>> 310: // and 8 bytes are required. The class must be default >>> constructable, >> >> Comment change seems unnecessary. > > I found that I had to add 64-bit atomic add support on 32-bit platforms > (working on it), so I changed the comment to match the similar cmpxchg one. Reverted, since 64-bit atomic add is no longer needed. - PR Review Comment: https://git.openjdk.org/jdk/pull/13814#discussion_r1194447125
Re: RFR: 8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM [v18]
On Mon, 15 May 2023 08:34:53 GMT, David Holmes wrote: >> Paul Hohensee has updated the pull request incrementally with one additional >> commit since the last revision: >> >> 8304704: In getTotalThreadAllocatedBytes javadoc, 'launched' -> 'started' > > src/hotspot/share/services/#allocationSite.hpp# line 1: > >> (failed to retrieve contents of file, check the PR for context) > Shouldn't be here. Removed. > src/hotspot/share/services/.#allocationSite.hpp line 1: > >> (failed to retrieve contents of file, check the PR for context) > Shouldn't be here. Removed. > src/hotspot/share/services/threadService.hpp line 65: > >> 63: >> 64: // As could this... >> 65: // Number of heap bytes allocated by termianted threads. > > typo: termianted Fixed. - PR Review Comment: https://git.openjdk.org/jdk/pull/13814#discussion_r1194447382 PR Review Comment: https://git.openjdk.org/jdk/pull/13814#discussion_r1194447452 PR Review Comment: https://git.openjdk.org/jdk/pull/13814#discussion_r1194447500
Re: RFR: 8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM [v19]
On Mon, 15 May 2023 23:12:52 GMT, Paul Hohensee wrote: >> Please review this addition to com.sun.management.ThreadMXBean that returns >> the total number of bytes allocated on the Java heap since JVM launch by >> both terminated and live threads. >> >> Because this PR adds a new interface method, I've updated the JMM_VERSION to >> 4, but would be happy to update it to 3_1 instead. > > Paul Hohensee 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 26 additional > commits since the last revision: > > - 8304704: _exited_allocated_bytes increment does not have to be atomic > - Merge branch 'master' into 8304074 > - 8304704: _exited_allocated_bytes increment does not have to be atomic > - 8304704: In getTotalThreadAllocatedBytes javadoc, 'launched' -> 'started' > - 8304704: Typos > - 8304074: fetch_then_add, not fetch_and_add > - 8304074: Assign to _add_long_func in add_long_bootstrap > - Merge branch 'master' into 8304074 > - 8304074: Define FULL_MEM_BARRIER for linux_arm > - 8304074: set _atomic_add_long_entry = ShouldNotCallThisStub() > - ... and 16 more: https://git.openjdk.org/jdk/compare/6d9db9f6...effdefc6 Makes things very much simpler without needing 64-bit atomic add. Hopefully all cleaned up. - PR Comment: https://git.openjdk.org/jdk/pull/13814#issuecomment-1548745902
Re: RFR: 8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM [v19]
> Please review this addition to com.sun.management.ThreadMXBean that returns > the total number of bytes allocated on the Java heap since JVM launch by both > terminated and live threads. > > Because this PR adds a new interface method, I've updated the JMM_VERSION to > 4, but would be happy to update it to 3_1 instead. Paul Hohensee 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 26 additional commits since the last revision: - 8304704: _exited_allocated_bytes increment does not have to be atomic - Merge branch 'master' into 8304074 - 8304704: _exited_allocated_bytes increment does not have to be atomic - 8304704: In getTotalThreadAllocatedBytes javadoc, 'launched' -> 'started' - 8304704: Typos - 8304074: fetch_then_add, not fetch_and_add - 8304074: Assign to _add_long_func in add_long_bootstrap - Merge branch 'master' into 8304074 - 8304074: Define FULL_MEM_BARRIER for linux_arm - 8304074: set _atomic_add_long_entry = ShouldNotCallThisStub() - ... and 16 more: https://git.openjdk.org/jdk/compare/7f45a6df...effdefc6 - Changes: - all: https://git.openjdk.org/jdk/pull/13814/files - new: https://git.openjdk.org/jdk/pull/13814/files/998a72ad..effdefc6 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13814&range=18 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13814&range=17-18 Stats: 4747 lines in 164 files changed: 2973 ins; 690 del; 1084 mod Patch: https://git.openjdk.org/jdk/pull/13814.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13814/head:pull/13814 PR: https://git.openjdk.org/jdk/pull/13814
Re: RFR: 8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM [v18]
> Please review this addition to com.sun.management.ThreadMXBean that returns > the total number of bytes allocated on the Java heap since JVM launch by both > terminated and live threads. > > Because this PR adds a new interface method, I've updated the JMM_VERSION to > 4, but would be happy to update it to 3_1 instead. Paul Hohensee has updated the pull request incrementally with one additional commit since the last revision: 8304704: In getTotalThreadAllocatedBytes javadoc, 'launched' -> 'started' - Changes: - all: https://git.openjdk.org/jdk/pull/13814/files - new: https://git.openjdk.org/jdk/pull/13814/files/5e5ec87a..998a72ad Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13814&range=17 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13814&range=16-17 Stats: 3 lines in 3 files changed: 2 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/13814.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13814/head:pull/13814 PR: https://git.openjdk.org/jdk/pull/13814
Re: RFR: 8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM [v17]
> Please review this addition to com.sun.management.ThreadMXBean that returns > the total number of bytes allocated on the Java heap since JVM launch by both > terminated and live threads. > > Because this PR adds a new interface method, I've updated the JMM_VERSION to > 4, but would be happy to update it to 3_1 instead. Paul Hohensee has updated the pull request incrementally with two additional commits since the last revision: - 8304704: Typos - 8304074: fetch_then_add, not fetch_and_add - Changes: - all: https://git.openjdk.org/jdk/pull/13814/files - new: https://git.openjdk.org/jdk/pull/13814/files/ab3b9080..5e5ec87a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13814&range=16 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13814&range=15-16 Stats: 94 lines in 4 files changed: 0 ins; 88 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/13814.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13814/head:pull/13814 PR: https://git.openjdk.org/jdk/pull/13814
Re: RFR: 8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM [v16]
> Please review this addition to com.sun.management.ThreadMXBean that returns > the total number of bytes allocated on the Java heap since JVM launch by both > terminated and live threads. > > Because this PR adds a new interface method, I've updated the JMM_VERSION to > 4, but would be happy to update it to 3_1 instead. Paul Hohensee 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 20 additional commits since the last revision: - 8304074: Assign to _add_long_func in add_long_bootstrap - Merge branch 'master' into 8304074 - 8304074: Define FULL_MEM_BARRIER for linux_arm - 8304074: set _atomic_add_long_entry = ShouldNotCallThisStub() - 8304074: update jmm_GetTotalAllocatedBytes comment - 8304074: Use __atomic_add_fetch rather than TBD assembler stub - 8304074: getTotalThreadAllocatedBytes name change, increment exited_allocated_bytes in ThreadService::remove_thread, add 64-bit atomic add for linux/bsd_x86, stub out for other platforms - Implement 32-bit linux Atomic::add() - Implement 32-bit linux Atomic::add() - Implement 32-bit linux Atomic::add() - ... and 10 more: https://git.openjdk.org/jdk/compare/e38628f9...ab3b9080 - Changes: - all: https://git.openjdk.org/jdk/pull/13814/files - new: https://git.openjdk.org/jdk/pull/13814/files/4ce0ede7..ab3b9080 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13814&range=15 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13814&range=14-15 Stats: 108483 lines in 1798 files changed: 86806 ins; 8865 del; 12812 mod Patch: https://git.openjdk.org/jdk/pull/13814.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13814/head:pull/13814 PR: https://git.openjdk.org/jdk/pull/13814
Re: RFR: 8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM [v12]
On Tue, 9 May 2023 14:30:06 GMT, Volker Simonis wrote: >> src/hotspot/share/services/threadService.cpp line 224: >> >>> 222: >>> 223: decrement_thread_counts(jt, daemon); >>> 224: >>> ThreadService::incr_exited_allocated_bytes(jt->cooked_allocated_bytes()); >> >> Again this is too soon. This should be deferred until after the thread has >> removed itself from the threads-list. > > Here I agree. This method (i.e. `ThreadService::current_thread_exiting()`) > is called from `JavaThread::exit()` *before* the call to `Threads::remove()`. > I think this call here to `ThreadService::incr_exited_allocated_bytes()` > could be removed completely in favor of an unconditional call to > `ThreadService::incr_exited_allocated_bytes()` in > `ThreadService::remove_thread()` (which will be called by `Threads::remove()` > *after* it removed the thread from the thread list). See my comment there. Done. - PR Review Comment: https://git.openjdk.org/jdk/pull/13814#discussion_r1192493634
Re: RFR: 8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM [v5]
On Tue, 9 May 2023 01:04:02 GMT, David Holmes wrote:
>> Do you mean something like:
>>
>> JavaThreadIteratorWithHandle jtiwh;
>> jlong result = ThreadService::exited_allocated_bytes();
>> while (JavaThread *thread = jtiwh.next()) {
>> ...
>>
>> That would be fine for me. Otherwise I agree with the current compromise
>> between accuracy and speed.
>
> Yes. Though I realize my comment needed to make clear that you need a thread
> to update the count after it has removed itself from the thread-list, for
> this to be an improvement.
Done as Volker suggests.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13814#discussion_r1192495715
Re: RFR: 8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM [v5]
On Tue, 9 May 2023 14:21:05 GMT, Volker Simonis wrote:
>> @dcubed-ojdk This is the current thread acting on itself
>
> This method (i.e. `ThreadService::remove_thread()`) is called from
> `Threads::remove()` *after* the thread was removed from the thread list:
>
> void Threads::remove(JavaThread* p, bool is_daemon) {
> ...
> // Maintain fast thread list
> ThreadsSMRSupport::remove_thread(p);
> ...
> ThreadService::remove_thread(p, is_daemon);
>
>
> But if we reach here from
> `JavaThread::cleanup_failed_attach_current_thread()` as the comment implies
> (`JavaThread::cleanup_failed_attach_current_thread()` calls
> `Threads::remove()` in the case of an attach failure), calling
> `ThreadService::incr_exited_allocated_bytes()` is probably irrelevant,
> because a thread which failed to attach can't allocate anyway.
>
> So instead of doing the call to
> `ThreadService::incr_exited_allocated_bytes()` here you can do it
> unconditionally and move it up before the check for `is_hidden_thread()`.
> This would then also account for regular thread exits where you arrive here
> from `JavaThread::exit()` and make the call to
> `ThreadService::incr_exited_allocated_bytes()` in
> `ThreadService::current_thread_exiting()` obsolete (also see my comment
> there).
Done.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13814#discussion_r1192495985
Re: RFR: 8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM [v15]
On Mon, 8 May 2023 02:07:25 GMT, David Holmes wrote: >> Afaiu, SMR/TLH keeps a terminated thread's TLS accessible, but doesn't stop >> the termination process. >> >> If we initialize result with exited_allocated_bytes, the "too small" >> possibility is still there. We still have a window between that >> initialization and the creation of the threads iterator where threads may >> terminate and thus not be included in the iteration. And new threads may >> become active during the iteration and not be included. As Aleksey observes, >> an advantage is that threads that terminate during the iteration will be >> counted only once and we don't have to check for terminated threads. >> >> If we stick with adding exited_allocated_bytes to result after the >> iteration, Volker is correct that we shouldn't include terminated threads in >> the sum because their allocated bytes values will have been added to >> exited_allocated_bytes. We have the same undercount possibility that >> initializing result with exited_allocated_bytes has, so this (fixed) >> approach can result in "too small" also, but "too large" goes away. >> >> Looks like both approaches are equivalent, so let's go with initializing >> result with exited_allocated_bytes because it avoids a comparison in the >> loop. > >> Afaiu, SMR/TLH keeps a terminated thread's TLS accessible, but doesn't stop >> the termination process. > > Incorrect. A thread cannot complete the termination process if it is > contained by a TLH - see ` ThreadsSMRSupport::smr_delete` and the call to > `wait_until_not_protected`. > > But not sure that helps with the zero-else-double accounting problem. Any > read of the "total accumulated bytes written to date" value is racing with > terminating threads. Resolved, see below. - PR Review Comment: https://git.openjdk.org/jdk/pull/13814#discussion_r1192494925
Re: RFR: 8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM [v11]
On Tue, 9 May 2023 01:12:06 GMT, David Holmes wrote: >> Paul Hohensee has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Implement 32-bit linux Atomic::add() > > src/hotspot/share/services/management.cpp line 2107: > >> 2105: // when result is initialized. The iterator is created and >> initialized >> 2106: // before the call to exited_allocated_bytes to ensure we don't >> miss >> 2107: // accounting for threads that are just about to terminate. > > Sorry this is only accurate if a thread updates `exited_allocated_bytes` > after it removes itself from the Threads-list. Fixed, see below. > src/jdk.management/share/classes/com/sun/management/ThreadMXBean.java line > 119: > >> 117: * @implSpec The default implementation throws >> UnsupportedOperationException >> 118: * if the Java virtual machine implementation does not support >> thread >> 119: * memory allocation measuremust, and otherwise acts as though >> thread > > Typo: measuremust Fixed. - PR Review Comment: https://git.openjdk.org/jdk/pull/13814#discussion_r1192494127 PR Review Comment: https://git.openjdk.org/jdk/pull/13814#discussion_r1192494309
Re: RFR: 8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM [v15]
> Please review this addition to com.sun.management.ThreadMXBean that returns > the total number of bytes allocated on the Java heap since JVM launch by both > terminated and live threads. > > Because this PR adds a new interface method, I've updated the JMM_VERSION to > 4, but would be happy to update it to 3_1 instead. Paul Hohensee has updated the pull request incrementally with one additional commit since the last revision: 8304074: Define FULL_MEM_BARRIER for linux_arm - Changes: - all: https://git.openjdk.org/jdk/pull/13814/files - new: https://git.openjdk.org/jdk/pull/13814/files/5515fc59..4ce0ede7 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13814&range=14 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13814&range=13-14 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/13814.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13814/head:pull/13814 PR: https://git.openjdk.org/jdk/pull/13814
Re: RFR: 8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM [v13]
On Thu, 11 May 2023 03:41:46 GMT, David Holmes wrote: >> src/hotspot/share/services/management.cpp line 2107: >> >>> 2105: // twice, once in the loop and agsin in exited_allocated_bytes if >>> it's >>> 2106: // removed from the list after it's encountered in the loop but >>> before >>> 2107: // adding exited_allocated_bytes. >> >> This is not an accurate description. >> >> Once we have the TLH we have a fixed set of threads and none of them can >> update exited_allocated_bytes. If we read exited_allocated_bytes while the >> TLH is held then the count can be short for two reasons: >> 1. Newly started threads >> 2. Terminating threads that already escaped the TLH but which hadn't updated >> exited_allocated_bytes by the time we read it. >> AFAICS with the current logic there is no possibility of double-accounting. > > Sorry I thought you had made the change to only update exited_allocated_bytes > after the thread removed itself from the threads-list. It really makes things > so much simpler. I made that change. The comment now reads: // A thread increments exited_allocated_bytes in ThreadService::remove_thread // only after it removes itself from the threads list, and once a TLH is // created, no thread it references can remove itself from the threads // list, so none can update exited_allocated_bytes. We therefore initialize // result with exited_allocated_bytes after after we create the TLH so that // the final result can only be short due to (1) threads that start after // the TLH is created, or (2) terminating threads that escape TLH creation // and don't update exited_allocated_bytes before we initialize result. - PR Review Comment: https://git.openjdk.org/jdk/pull/13814#discussion_r1192473858
Re: RFR: 8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM [v14]
> Please review this addition to com.sun.management.ThreadMXBean that returns > the total number of bytes allocated on the Java heap since JVM launch by both > terminated and live threads. > > Because this PR adds a new interface method, I've updated the JMM_VERSION to > 4, but would be happy to update it to 3_1 instead. Paul Hohensee has updated the pull request incrementally with three additional commits since the last revision: - 8304074: set _atomic_add_long_entry = ShouldNotCallThisStub() - 8304074: update jmm_GetTotalAllocatedBytes comment - 8304074: Use __atomic_add_fetch rather than TBD assembler stub - Changes: - all: https://git.openjdk.org/jdk/pull/13814/files - new: https://git.openjdk.org/jdk/pull/13814/files/ab830a80..5515fc59 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13814&range=13 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13814&range=12-13 Stats: 42 lines in 3 files changed: 16 ins; 15 del; 11 mod Patch: https://git.openjdk.org/jdk/pull/13814.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13814/head:pull/13814 PR: https://git.openjdk.org/jdk/pull/13814
Re: RFR: 8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM [v13]
> Please review this addition to com.sun.management.ThreadMXBean that returns > the total number of bytes allocated on the Java heap since JVM launch by both > terminated and live threads. > > Because this PR adds a new interface method, I've updated the JMM_VERSION to > 4, but would be happy to update it to 3_1 instead. Paul Hohensee has updated the pull request incrementally with one additional commit since the last revision: 8304074: getTotalThreadAllocatedBytes name change, increment exited_allocated_bytes in ThreadService::remove_thread, add 64-bit atomic add for linux/bsd_x86, stub out for other platforms - Changes: - all: https://git.openjdk.org/jdk/pull/13814/files - new: https://git.openjdk.org/jdk/pull/13814/files/fbe3c114..ab830a80 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13814&range=12 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13814&range=11-12 Stats: 159 lines in 11 files changed: 112 ins; 13 del; 34 mod Patch: https://git.openjdk.org/jdk/pull/13814.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13814/head:pull/13814 PR: https://git.openjdk.org/jdk/pull/13814
Re: RFR: 8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM [v11]
On Tue, 9 May 2023 01:08:19 GMT, David Holmes wrote: >> Paul Hohensee has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Implement 32-bit linux Atomic::add() > > src/hotspot/share/runtime/atomic.hpp line 310: > >> 308: >> 309: // Platform-specific implementation of add. Support for sizes of 4 >> 310: // and 8 bytes are required. The class must be default constructable, > > Comment change seems unnecessary. I found that I had to add 64-bit atomic add support on 32-bit platforms (working on it), so I changed the comment to match the similar cmpxchg one. - PR Review Comment: https://git.openjdk.org/jdk/pull/13814#discussion_r1188034535
Re: RFR: 8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM [v12]
> Please review this addition to com.sun.management.ThreadMXBean that returns > the total number of bytes allocated on the Java heap since JVM launch by both > terminated and live threads. > > Because this PR adds a new interface method, I've updated the JMM_VERSION to > 4, but would be happy to update it to 3_1 instead. Paul Hohensee has updated the pull request incrementally with one additional commit since the last revision: Implement 32-bit linux Atomic::add() - Changes: - all: https://git.openjdk.org/jdk/pull/13814/files - new: https://git.openjdk.org/jdk/pull/13814/files/75630a07..fbe3c114 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13814&range=11 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13814&range=10-11 Stats: 10 lines in 1 file changed: 1 ins; 0 del; 9 mod Patch: https://git.openjdk.org/jdk/pull/13814.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13814/head:pull/13814 PR: https://git.openjdk.org/jdk/pull/13814
Re: RFR: 8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM [v11]
> Please review this addition to com.sun.management.ThreadMXBean that returns > the total number of bytes allocated on the Java heap since JVM launch by both > terminated and live threads. > > Because this PR adds a new interface method, I've updated the JMM_VERSION to > 4, but would be happy to update it to 3_1 instead. Paul Hohensee has updated the pull request incrementally with one additional commit since the last revision: Implement 32-bit linux Atomic::add() - Changes: - all: https://git.openjdk.org/jdk/pull/13814/files - new: https://git.openjdk.org/jdk/pull/13814/files/c592c8a1..75630a07 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13814&range=10 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13814&range=09-10 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/13814.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13814/head:pull/13814 PR: https://git.openjdk.org/jdk/pull/13814
Re: RFR: 8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM [v10]
> Please review this addition to com.sun.management.ThreadMXBean that returns > the total number of bytes allocated on the Java heap since JVM launch by both > terminated and live threads. > > Because this PR adds a new interface method, I've updated the JMM_VERSION to > 4, but would be happy to update it to 3_1 instead. Paul Hohensee has updated the pull request incrementally with one additional commit since the last revision: Implement 32-bit linux Atomic::add() - Changes: - all: https://git.openjdk.org/jdk/pull/13814/files - new: https://git.openjdk.org/jdk/pull/13814/files/0bf27fed..c592c8a1 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13814&range=09 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13814&range=08-09 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/13814.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13814/head:pull/13814 PR: https://git.openjdk.org/jdk/pull/13814
Re: RFR: 8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM [v9]
> Please review this addition to com.sun.management.ThreadMXBean that returns > the total number of bytes allocated on the Java heap since JVM launch by both > terminated and live threads. > > Because this PR adds a new interface method, I've updated the JMM_VERSION to > 4, but would be happy to update it to 3_1 instead. Paul Hohensee has updated the pull request incrementally with one additional commit since the last revision: Implement 32-bit linux Atomic::add() - Changes: - all: https://git.openjdk.org/jdk/pull/13814/files - new: https://git.openjdk.org/jdk/pull/13814/files/fcabf083..0bf27fed Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13814&range=08 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13814&range=07-08 Stats: 4 lines in 1 file changed: 1 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/13814.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13814/head:pull/13814 PR: https://git.openjdk.org/jdk/pull/13814
Re: RFR: 8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM [v8]
> Please review this addition to com.sun.management.ThreadMXBean that returns > the total number of bytes allocated on the Java heap since JVM launch by both > terminated and live threads. > > Because this PR adds a new interface method, I've updated the JMM_VERSION to > 4, but would be happy to update it to 3_1 instead. Paul Hohensee has updated the pull request incrementally with one additional commit since the last revision: CSR comment updates - Changes: - all: https://git.openjdk.org/jdk/pull/13814/files - new: https://git.openjdk.org/jdk/pull/13814/files/d9039f3f..fcabf083 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13814&range=07 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13814&range=06-07 Stats: 6 lines in 1 file changed: 0 ins; 0 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/13814.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13814/head:pull/13814 PR: https://git.openjdk.org/jdk/pull/13814
Re: RFR: 8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM [v7]
> Please review this addition to com.sun.management.ThreadMXBean that returns > the total number of bytes allocated on the Java heap since JVM launch by both > terminated and live threads. > > Because this PR adds a new interface method, I've updated the JMM_VERSION to > 4, but would be happy to update it to 3_1 instead. Paul Hohensee has updated the pull request incrementally with one additional commit since the last revision: CSR comment updates - Changes: - all: https://git.openjdk.org/jdk/pull/13814/files - new: https://git.openjdk.org/jdk/pull/13814/files/4161677c..d9039f3f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13814&range=06 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13814&range=05-06 Stats: 6 lines in 2 files changed: 0 ins; 0 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/13814.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13814/head:pull/13814 PR: https://git.openjdk.org/jdk/pull/13814
Re: RFR: 8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM [v6]
> Please review this addition to com.sun.management.ThreadMXBean that returns > the total number of bytes allocated on the Java heap since JVM launch by both > terminated and live threads. > > Because this PR adds a new interface method, I've updated the JMM_VERSION to > 4, but would be happy to update it to 3_1 instead. Paul Hohensee has updated the pull request incrementally with two additional commits since the last revision: - 8304074: Implement 32-bit linux Atomic::add() - 8304074: CSR comment updates - Changes: - all: https://git.openjdk.org/jdk/pull/13814/files - new: https://git.openjdk.org/jdk/pull/13814/files/7b922263..4161677c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13814&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13814&range=04-05 Stats: 61 lines in 8 files changed: 41 ins; 3 del; 17 mod Patch: https://git.openjdk.org/jdk/pull/13814.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13814/head:pull/13814 PR: https://git.openjdk.org/jdk/pull/13814
Re: RFR: 8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM [v5]
On Fri, 5 May 2023 16:43:10 GMT, Aleksey Shipilev wrote:
>> Paul Hohensee has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8304074: [JMX] Add an approximation of total bytes allocated on the Java
>> heap by the JVM
>
> src/hotspot/share/include/jmm.h line 55:
>
>> 53: JMM_VERSION_2 = 0x2002, // JDK 10
>> 54: JMM_VERSION_3 = 0x2003, // JDK 14
>> 55: JMM_VERSION_3_0 = 0x2003,
>
> What's `JMM_VERSION_3_0`?
Removed.
> src/hotspot/share/services/management.cpp line 2115:
>
>> 2113: result += size;
>> 2114: }
>> 2115: return result + ThreadService::exited_allocated_bytes();;
>
> Double `;;`.
Fixed.
> src/hotspot/share/services/threadService.hpp line 111:
>
>> 109: static jlong exited_allocated_bytes() { return
>> _exited_allocated_bytes; }
>> 110: static void incr_exited_allocated_bytes(jlong size) {
>> 111: Atomic::add(&_exited_allocated_bytes, size);
>
> `Atomic::add(&_exited_allocated_bytes, size, memory_order_relaxed);`, please.
> No need for overly-strict memory effects for this counter.
Fixed.
> src/java.management/share/classes/sun/management/ThreadImpl.java line 535:
>
>> 533: private static native long getThreadAllocatedMemory0(long id);
>> 534: private static native void getThreadAllocatedMemory1(long[] ids,
>> long[] result);
>> 535: private static native long getThreadAllocatedMemory2();
>
> We can call this one `getAllThreadAllocatedMemory`, which obviates the need
> for `2` as the suffix.
Fixed.
> src/jdk.management/share/classes/com/sun/management/ThreadMXBean.java line
> 159:
>
>> 157: *
>> 158: * @return an approximation of the total memory allocated, in
>> bytes, in
>> 159: * heap memory for the current thread,
>
> I am not sure if typos changes in the public API requires a CSR (albeit
> trivial one). Maybe skip these updates?
Fixed.
> test/jdk/com/sun/management/ThreadMXBean/ThreadAllocatedMemory.java line 221:
>
>> 219: // baseline should be positive
>> 220: Thread curThread = Thread.currentThread();
>> 221: long cumulative_size = mbean.getAllThreadAllocatedBytes();
>
> Java style for variables is camel-case, `cumulativeSize`.
Fixed.
> test/jdk/com/sun/management/ThreadMXBean/ThreadAllocatedMemory.java line 377:
>
>> 375: throw new RuntimeException(getName() +
>> 376: " ThreadAllocatedBytes before = " + size1 +
>> 377: " > ThreadAllocatedBytes after = " + size2);
>
> Is this replaceable with `checkResult(...)`?
Yes. Done.
> test/jdk/com/sun/management/ThreadMXBean/ThreadAllocatedMemoryArray.java line
> 120:
>
>> 118: long[] sizes1 = mbean.getThreadAllocatedBytes(ids);
>> 119: for (int i = 0; i < NUM_THREADS; i++) {
>> 120: checkResult(threads[i], sizes[i], sizes1[i]);
>
> Since we are cleaning up the test anyway, can we / should we rename `sizes`
> -> `before`, `size1` -> `after`?
I kept "sizes" since it's use before isn't really "before". I renamed sizes1 to
afterSizes.
> test/jdk/com/sun/management/ThreadMXBean/ThreadAllocatedMemoryArray.java line
> 164:
>
>> 162:
>> 163: private static void checkResult(Thread curThread,
>> 164: long prev_size, long curr_size) {
>
> camelCase arguments.
Fixed.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13814#discussion_r1186514932
PR Review Comment: https://git.openjdk.org/jdk/pull/13814#discussion_r1186515039
PR Review Comment: https://git.openjdk.org/jdk/pull/13814#discussion_r1186513534
PR Review Comment: https://git.openjdk.org/jdk/pull/13814#discussion_r1186513639
PR Review Comment: https://git.openjdk.org/jdk/pull/13814#discussion_r1186513735
PR Review Comment: https://git.openjdk.org/jdk/pull/13814#discussion_r1186513794
PR Review Comment: https://git.openjdk.org/jdk/pull/13814#discussion_r1186514728
PR Review Comment: https://git.openjdk.org/jdk/pull/13814#discussion_r1186514463
PR Review Comment: https://git.openjdk.org/jdk/pull/13814#discussion_r1186514571
Re: RFR: 8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM [v5]
On Fri, 5 May 2023 15:11:26 GMT, Volker Simonis wrote: >> Paul Hohensee has updated the pull request incrementally with one additional >> commit since the last revision: >> >> 8304074: [JMX] Add an approximation of total bytes allocated on the Java >> heap by the JVM > > src/hotspot/share/include/jmm.h line 55: > >> 53: JMM_VERSION_2 = 0x2002, // JDK 10 >> 54: JMM_VERSION_3 = 0x2003, // JDK 14 >> 55: JMM_VERSION_3_0 = 0x2003, > > Why do we need `JMM_VERSION_3_0`? We haven't defined `JMM_VERSION_2_0` either. Removed. > src/hotspot/share/include/jmm.h line 321: > >> 319: jstring flag_name, >> 320: jvalue new_value); >> 321: jlong(JNICALL *GetAllThreadAllocatedMemory) > > I'm not sure here, but I think there's no need to "overwrite" a *reserved* > slot if you add this functionality to a new major release as you do. You also > haven't done it when you've added `GetOneThreadAllocatedMemory()` with > [JDK-8231209](https://bugs.openjdk.org/browse/JDK-8231209). > > I think we should keep these *reserved* slots for the case when we eventually > have to downport new functionality from a later release. Done. > src/hotspot/share/services/management.cpp line 2282: > >> 2280: jmm_FindDeadlockedThreads, >> 2281: jmm_SetVMGlobal, >> 2282: jmm_GetAllThreadAllocatedMemory, > > See comment on overwriting the `reserved6` slot above. Done. - PR Review Comment: https://git.openjdk.org/jdk/pull/13814#discussion_r1186515133 PR Review Comment: https://git.openjdk.org/jdk/pull/13814#discussion_r1186515257 PR Review Comment: https://git.openjdk.org/jdk/pull/13814#discussion_r1186515422
Re: RFR: 8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM [v5]
> Please review this addition to com.sun.management.ThreadMXBean that returns > the total number of bytes allocated on the Java heap since JVM launch by both > terminated and live threads. > > Because this PR adds a new interface method, I've updated the JMM_VERSION to > 4, but would be happy to update it to 3_1 instead. Paul Hohensee has updated the pull request incrementally with one additional commit since the last revision: 8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM - Changes: - all: https://git.openjdk.org/jdk/pull/13814/files - new: https://git.openjdk.org/jdk/pull/13814/files/2e2adc0b..7b922263 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13814&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13814&range=03-04 Stats: 11 lines in 1 file changed: 0 ins; 4 del; 7 mod Patch: https://git.openjdk.org/jdk/pull/13814.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13814/head:pull/13814 PR: https://git.openjdk.org/jdk/pull/13814
Re: RFR: 8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM [v4]
On Fri, 5 May 2023 17:29:30 GMT, Paul Hohensee wrote: >> I agree we should strive to get the value as accurate as possible. I think >> for operational use at scale, we need to avoid doing safepoints. Holding a >> `ThreadLock` might also penalize other code that (ab)uses threading (we >> frequently see thousands of threads coming and going, don't ask). >> >> But I have a fundamental question here: since SMR/TLH gives us a snapshot of >> currently live threads, and it also protects us from seeing an exiting >> thread in bad state (ultimately, a `delete`-d one), why can't we just trust >> its `cooked_allocated_bytes`, and avoid adding allocated bytes on exit path? >> If we cannot trust that, can we make it trustable while thread is protected >> by SMR/TLH? > > I thought about doing those, but both would slow down the app/JVM and very > likely introduce p99.9/p100 latency outliers that we'd rather not see just > because we're sampling. Also, > > 1. The existing thread allocated bytes implementation isn't particularly > accurate either, in the sense that reality quickly gets away from the method > return values. That was a conscious decision to go with speed and efficiency > over stability/accuracy, so I went the same way for this implementation. > 2. Cloud services sample at fairly long intervals in order to avoid overhead: > 1 minute is common and 5 minutes not unheard of. Stability/accuracy within > short time frames is unneeded with such long sampling intervals. Afaiu, SMR/TLH keeps a terminated thread's TLS accessible, but doesn't stop the termination process. If we initialize result with exited_allocated_bytes, the "too small" possibility is still there. We still have a window between that initialization and the creation of the threads iterator where threads may terminate and thus not be included in the iteration. And new threads may become active during the iteration and not be included. As Aleksey observes, an advantage is that threads that terminate during the iteration will be counted only once and we don't have to check for terminated threads. If we stick with adding exited_allocated_bytes to result after the iteration, Volker is correct that we shouldn't include terminated threads in the sum because their allocated bytes values will have been added to exited_allocated_bytes. We have the same undercount possibility that initializing result with exited_allocated_bytes has, so this (fixed) approach can result in "too small" also, but "too large" goes away. Looks like both approaches are equivalent, so let's go with initializing result with exited_allocated_bytes because it avoids a comparison in the loop. - PR Review Comment: https://git.openjdk.org/jdk/pull/13814#discussion_r1186508768
Re: RFR: 8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM [v4]
On Fri, 5 May 2023 06:43:20 GMT, David Holmes wrote: >> Paul Hohensee has updated the pull request incrementally with one additional >> commit since the last revision: >> >> 8304074: [JMX] Add an approximation of total bytes allocated on the Java >> heap by the JVM > > src/hotspot/share/services/management.cpp line 2106: > >> 2104: // the loop gets to it and thus not be counted. If, on the other >> hand and done >> 2105: // here, exited_allocated_bytes is added after the loop, the final >> result might be >> 2106: // "too large" because a thread might be counted twice, once in >> the loop and agsin > > typo agsin Fixed. - PR Review Comment: https://git.openjdk.org/jdk/pull/13814#discussion_r1186498560
Re: RFR: 8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM [v4]
> Please review this addition to com.sun.management.ThreadMXBean that returns > the total number of bytes allocated on the Java heap since JVM launch by both > terminated and live threads. > > Because this PR adds a new interface method, I've updated the JMM_VERSION to > 4, but would be happy to update it to 3_1 instead. Paul Hohensee has updated the pull request incrementally with one additional commit since the last revision: 8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM - Changes: - all: https://git.openjdk.org/jdk/pull/13814/files - new: https://git.openjdk.org/jdk/pull/13814/files/1001b667..2e2adc0b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13814&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13814&range=02-03 Stats: 6 lines in 1 file changed: 3 ins; 3 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/13814.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13814/head:pull/13814 PR: https://git.openjdk.org/jdk/pull/13814
Re: RFR: 8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM [v3]
On Fri, 5 May 2023 16:16:37 GMT, Aleksey Shipilev wrote: >> Paul Hohensee has updated the pull request incrementally with one additional >> commit since the last revision: >> >> 8304074: [JMX] Add an approximation of total bytes allocated on the Java >> heap by the JVM > > test/jdk/com/sun/management/ThreadMXBean/ThreadAllocatedMemory.java line 286: > >> 284: } >> 285: >> 286: private static long checkResult(Thread curThread, > > There is another `checkResult` below? Should they be replaced by a single > method? No, there's only one checkResult method. I changed the result type from void to long. - PR Review Comment: https://git.openjdk.org/jdk/pull/13814#discussion_r1186491906
Re: RFR: 8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM [v3]
> Please review this addition to com.sun.management.ThreadMXBean that returns > the total number of bytes allocated on the Java heap since JVM launch by both > terminated and live threads. > > Because this PR adds a new interface method, I've updated the JMM_VERSION to > 4, but would be happy to update it to 3_1 instead. Paul Hohensee has updated the pull request incrementally with one additional commit since the last revision: 8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM - Changes: - all: https://git.openjdk.org/jdk/pull/13814/files - new: https://git.openjdk.org/jdk/pull/13814/files/460c00e4..1001b667 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13814&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13814&range=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/13814.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13814/head:pull/13814 PR: https://git.openjdk.org/jdk/pull/13814
Re: RFR: 8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM [v2]
> Please review this addition to com.sun.management.ThreadMXBean that returns > the total number of bytes allocated on the Java heap since JVM launch by both > terminated and live threads. > > Because this PR adds a new interface method, I've updated the JMM_VERSION to > 4, but would be happy to update it to 3_1 instead. Paul Hohensee has updated the pull request incrementally with one additional commit since the last revision: 8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM - Changes: - all: https://git.openjdk.org/jdk/pull/13814/files - new: https://git.openjdk.org/jdk/pull/13814/files/d78ec8fa..460c00e4 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13814&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13814&range=00-01 Stats: 55 lines in 8 files changed: 6 ins; 14 del; 35 mod Patch: https://git.openjdk.org/jdk/pull/13814.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13814/head:pull/13814 PR: https://git.openjdk.org/jdk/pull/13814
Re: RFR: 8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM
On Fri, 5 May 2023 16:42:29 GMT, Aleksey Shipilev wrote: >> The API specification clearly states that this method returns "*an >> approximation of the total amount of memory allocated in heap*" so in my >> opinion it is OK to keep it simple here and don't start messing with looks >> and safepoints. >> >> But can't we make this a little more accurate by only adding a threads >> allocated bytes if it is not `thread->is_terminated()`? Wouldn't that >> prevent double counting most of the time? > > I agree we should strive to get the value as accurate as possible. I think > for operational use at scale, we need to avoid doing safepoints. Holding a > `ThreadLock` might also penalize other code that (ab)uses threading (we > frequently see thousands of threads coming and going, don't ask). > > But I have a fundamental question here: since SMR/TLH gives us a snapshot of > currently live threads, and it also protects us from seeing an exiting thread > in bad state (ultimately, a `delete`-d one), why can't we just trust its > `cooked_allocated_bytes`, and avoid adding allocated bytes on exit path? If > we cannot trust that, can we make it trustable while thread is protected by > SMR/TLH? I thought about doing those, but both would slow down the app/JVM and very likely introduce p99.9/p100 latency outliers that we'd rather not see just because we're sampling. Also, 1. The existing thread allocated bytes implementation isn't particularly accurate either, in the sense that reality quickly gets away from the method return values. That was a conscious decision to go with speed and efficiency over stability/accuracy, so I went the same way for this implementation. 2. Cloud services sample at fairly long intervals in order to avoid overhead: 1 minute is common and 5 minutes not unheard of. Stability/accuracy within short time frames is unneeded with such long sampling intervals. - PR Review Comment: https://git.openjdk.org/jdk/pull/13814#discussion_r1186341500
RFR: 8304074: [JMX] Add an approximation of JVM process allocated bytes
Please review this addition to com.sun.management.ThreadMXBean that returns the total number of bytes allocated on the Java heap since JVM launch by both terminated and live threads. Because this PR adds a new interface method, I've updated the JMM_VERSION to 4, but would be happy to update it to 3_1 instead. - Commit messages: - 8304074: [JMX] Add an approximation of JVM process allocated bytes Changes: https://git.openjdk.org/jdk/pull/13814/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=13814&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8304074 Stats: 222 lines in 10 files changed: 173 ins; 16 del; 33 mod Patch: https://git.openjdk.org/jdk/pull/13814.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13814/head:pull/13814 PR: https://git.openjdk.org/jdk/pull/13814
