RFR: 8343893: Test jdk/jfr/event/runtime/TestNativeMemoryUsageEvents.java failed: heap should have grown and NMT should show that: expected 0 > 0
# Background With the implementation of [JDK-8312132](https://bugs.openjdk.org/browse/JDK-8312132) we added the `MemoryFileTracker` (MFT). Unfortunately, this work failed to implement JFR integration. This, in turn, meant that (generational) ZGC has not correctly reported its heap usage via the NMT JFR events. We (as in Oracle) never saw this issue in testing, as we didn't run this test for all possible GC configurations. With an update of our test configurations this is no longer the case: We run this test for all possible GCs and the test now fails for generational ZGC. # The fix I implemented JFR events for the MFT by adding all of its reserved and committed memory into the JFR data, similarly to the `VirtualMemoryTracker`. I also added an explicit `run` using `ZGC` to the failing test, to ensure that any actual regressions are found. ------------- Commit messages: - Add MemoryFileTracker support to NMTUsage Changes: https://git.openjdk.org/jdk/pull/22204/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=22204&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8343893 Stats: 40 lines in 5 files changed: 31 ins; 4 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/22204.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/22204/head:pull/22204 PR: https://git.openjdk.org/jdk/pull/22204
# Background
With the implementation of [JDK-8312132](https://bugs.openjdk.org/browse/JDK-8312132) we added the `MemoryFileTracker` (MFT). Unfortunately, this work failed to implement JFR integration. This, in turn, meant that (generational) ZGC has not correctly reported its heap usage via the NMT JFR events. We (as in Oracle) never saw this issue in testing, as we didn't run this test for all possible GC configurations. With an update of our test configurations this is no longer the case: We run this test for all possible GCs and the test now fails for generational ZGC.
# The fix
I implemented JFR events for the MFT by adding all of its reserved and committed memory into the JFR data, similarly to the `VirtualMemoryTracker`. I also added an explicit `run` using `ZGC` to the failing test, to ensure that any actual regressions are found.
Johan Sjölen has updated the pull request incrementally with one additional commit since the last revision: Add include ------------- Changes: - all: https://git.openjdk.org/jdk/pull/22204/files - new: https://git.openjdk.org/jdk/pull/22204/files/7164f615..76f8bff4 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=22204&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=22204&range=00-01 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/22204.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/22204/head:pull/22204 PR: https://git.openjdk.org/jdk/pull/22204
# Background
With the implementation of [JDK-8312132](https://bugs.openjdk.org/browse/JDK-8312132) we added the `MemoryFileTracker` (MFT). Unfortunately, this work failed to implement JFR integration. This, in turn, meant that (generational) ZGC has not correctly reported its heap usage via the NMT JFR events. We (as in Oracle) never saw this issue in testing, as we didn't run this test for all possible GC configurations. With an update of our test configurations this is no longer the case: We run this test for all possible GCs and the test now fails for generational ZGC.
# The fix
I implemented JFR events for the MFT by adding all of its reserved and committed memory into the JFR data, similarly to the `VirtualMemoryTracker`. I also added an explicit `run` using `ZGC` to the failing test, to ensure that any actual regressions are found.
Johan Sjölen 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 four additional commits since the last revision: - Un-problemlist the test - Merge remote-tracking branch 'openjdk/master' into mft-jfr - Add include - Add MemoryFileTracker support to NMTUsage ------------- Changes: - all: https://git.openjdk.org/jdk/pull/22204/files - new: https://git.openjdk.org/jdk/pull/22204/files/76f8bff4..fcf5db52 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=22204&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=22204&range=01-02 Stats: 171879 lines in 3514 files changed: 59750 ins; 100147 del; 11982 mod Patch: https://git.openjdk.org/jdk/pull/22204.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/22204/head:pull/22204 PR: https://git.openjdk.org/jdk/pull/22204
On Mon, 18 Nov 2024 12:42:14 GMT, Johan Sjölen <jsjolen@openjdk.org> wrote:
# Background
With the implementation of [JDK-8312132](https://bugs.openjdk.org/browse/JDK-8312132) we added the `MemoryFileTracker` (MFT). Unfortunately, this work failed to implement JFR integration. This, in turn, meant that (generational) ZGC has not correctly reported its heap usage via the NMT JFR events. We (as in Oracle) never saw this issue in testing, as we didn't run this test for all possible GC configurations. With an update of our test configurations this is no longer the case: We run this test for all possible GCs and the test now fails for generational ZGC.
# The fix
I implemented JFR events for the MFT by adding all of its reserved and committed memory into the JFR data, similarly to the `VirtualMemoryTracker`. I also added an explicit `run` using `ZGC` to the failing test, to ensure that any actual regressions are found.
Johan Sjölen 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 four additional commits since the last revision:
- Un-problemlist the test - Merge remote-tracking branch 'openjdk/master' into mft-jfr - Add include - Add MemoryFileTracker support to NMTUsage
Looks good. ------------- Marked as reviewed by mgronlun (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/22204#pullrequestreview-2442912454
On Mon, 18 Nov 2024 12:42:14 GMT, Johan Sjölen <jsjolen@openjdk.org> wrote:
# Background
With the implementation of [JDK-8312132](https://bugs.openjdk.org/browse/JDK-8312132) we added the `MemoryFileTracker` (MFT). Unfortunately, this work failed to implement JFR integration. This, in turn, meant that (generational) ZGC has not correctly reported its heap usage via the NMT JFR events. We (as in Oracle) never saw this issue in testing, as we didn't run this test for all possible GC configurations. With an update of our test configurations this is no longer the case: We run this test for all possible GCs and the test now fails for generational ZGC.
# The fix
I implemented JFR events for the MFT by adding all of its reserved and committed memory into the JFR data, similarly to the `VirtualMemoryTracker`. I also added an explicit `run` using `ZGC` to the failing test, to ensure that any actual regressions are found.
Johan Sjölen 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 four additional commits since the last revision:
- Un-problemlist the test - Merge remote-tracking branch 'openjdk/master' into mft-jfr - Add include - Add MemoryFileTracker support to NMTUsage
Changes requested by lmesnik (Reviewer). test/jdk/jdk/jfr/event/runtime/TestNativeMemoryUsageEvents.java line 47:
45: * @modules jdk.jfr 46: * jdk.management 47: * @run main/othervm -XX:+UseZGC -XX:NativeMemoryTracking=summary -Xms16m -Xmx128m -XX:-UseLargePages -Xlog:gc jdk.jfr.event.runtime.TestNativeMemoryUsageEvents true
It is not needed, we'll catch them anyway. But if you want to add separate run, please add it as a separate testcase and requires vm.gc.G1. So we have separate test results and can run tests with any GC. ------------- PR Review: https://git.openjdk.org/jdk/pull/22204#pullrequestreview-2443100829 PR Review Comment: https://git.openjdk.org/jdk/pull/22204#discussion_r1846881639
On Mon, 18 Nov 2024 16:15:23 GMT, Leonid Mesnik <lmesnik@openjdk.org> wrote:
But if you want to add separate run, please add it as a separate testcase and requires vm.gc.G1.
Sorry Leonid, could you help me out with writing this? I'm not very good at JTReg yet :). ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/22204#discussion_r1846925542
On Mon, 18 Nov 2024 12:42:14 GMT, Johan Sjölen <jsjolen@openjdk.org> wrote:
# Background
With the implementation of [JDK-8312132](https://bugs.openjdk.org/browse/JDK-8312132) we added the `MemoryFileTracker` (MFT). Unfortunately, this work failed to implement JFR integration. This, in turn, meant that (generational) ZGC has not correctly reported its heap usage via the NMT JFR events. We (as in Oracle) never saw this issue in testing, as we didn't run this test for all possible GC configurations. With an update of our test configurations this is no longer the case: We run this test for all possible GCs and the test now fails for generational ZGC.
# The fix
I implemented JFR events for the MFT by adding all of its reserved and committed memory into the JFR data, similarly to the `VirtualMemoryTracker`. I also added an explicit `run` using `ZGC` to the failing test, to ensure that any actual regressions are found.
Johan Sjölen 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 four additional commits since the last revision:
- Un-problemlist the test - Merge remote-tracking branch 'openjdk/master' into mft-jfr - Add include - Add MemoryFileTracker support to NMTUsage
Leonid convinced me that it's unnecessary to have the ZGC case for the test, so I removed it. ------------- PR Comment: https://git.openjdk.org/jdk/pull/22204#issuecomment-2483600906
# Background
With the implementation of [JDK-8312132](https://bugs.openjdk.org/browse/JDK-8312132) we added the `MemoryFileTracker` (MFT). Unfortunately, this work failed to implement JFR integration. This, in turn, meant that (generational) ZGC has not correctly reported its heap usage via the NMT JFR events. We (as in Oracle) never saw this issue in testing, as we didn't run this test for all possible GC configurations. With an update of our test configurations this is no longer the case: We run this test for all possible GCs and the test now fails for generational ZGC.
# The fix
I implemented JFR events for the MFT by adding all of its reserved and committed memory into the JFR data, similarly to the `VirtualMemoryTracker`. I also added an explicit `run` using `ZGC` to the failing test, to ensure that any actual regressions are found.
Johan Sjölen has updated the pull request incrementally with one additional commit since the last revision: Remove ZGC run test ------------- Changes: - all: https://git.openjdk.org/jdk/pull/22204/files - new: https://git.openjdk.org/jdk/pull/22204/files/fcf5db52..4a11a22a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=22204&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=22204&range=02-03 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/22204.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/22204/head:pull/22204 PR: https://git.openjdk.org/jdk/pull/22204
On Mon, 18 Nov 2024 16:59:56 GMT, Johan Sjölen <jsjolen@openjdk.org> wrote:
# Background
With the implementation of [JDK-8312132](https://bugs.openjdk.org/browse/JDK-8312132) we added the `MemoryFileTracker` (MFT). Unfortunately, this work failed to implement JFR integration. This, in turn, meant that (generational) ZGC has not correctly reported its heap usage via the NMT JFR events. We (as in Oracle) never saw this issue in testing, as we didn't run this test for all possible GC configurations. With an update of our test configurations this is no longer the case: We run this test for all possible GCs and the test now fails for generational ZGC.
# The fix
I implemented JFR events for the MFT by adding all of its reserved and committed memory into the JFR data, similarly to the `VirtualMemoryTracker`. I also added an explicit `run` using `ZGC` to the failing test, to ensure that any actual regressions are found.
Johan Sjölen has updated the pull request incrementally with one additional commit since the last revision:
Remove ZGC run test
Marked as reviewed by lmesnik (Reviewer). ------------- PR Review: https://git.openjdk.org/jdk/pull/22204#pullrequestreview-2443206355
On Mon, 18 Nov 2024 17:03:54 GMT, Johan Sjölen <jsjolen@openjdk.org> wrote:
# Background
With the implementation of [JDK-8312132](https://bugs.openjdk.org/browse/JDK-8312132) we added the `MemoryFileTracker` (MFT). Unfortunately, this work failed to implement JFR integration. This, in turn, meant that (generational) ZGC has not correctly reported its heap usage via the NMT JFR events. We (as in Oracle) never saw this issue in testing, as we didn't run this test for all possible GC configurations. With an update of our test configurations this is no longer the case: We run this test for all possible GCs and the test now fails for generational ZGC.
# The fix
I implemented JFR events for the MFT by adding all of its reserved and committed memory into the JFR data, similarly to the `VirtualMemoryTracker`. I also added an explicit `run` using `ZGC` to the failing test, to ensure that any actual regressions are found.
Johan Sjölen has updated the pull request incrementally with one additional commit since the last revision:
Remove ZGC run test
Just and observation - you used here the acronym `MFT` and it just jumped out at me, then I looked at `memoryFileTracker` and it feels inconsistent to me. Did we ever consider naming it `nativeMemoryFileTracker`? Then we would have `NMFT`, which extends `NMT` better, IMHO. Just something to keep in mind if we ever feel like cleaning up the names in `NMT` area. LGTM src/hotspot/share/nmt/nmtUsage.cpp line 105:
103: _vm_total.committed += vm->committed(); 104: }); 105: }
I am missing something here, in memoryFileTracker.hpp you say: // All memory is accounted as committed, there is no reserved memory. // Any reserved memory is expected to exist in the VirtualMemoryTracker. but here we use MFT to account for both reserved and committed? ------------- PR Review: https://git.openjdk.org/jdk/pull/22204#pullrequestreview-2446030052 Marked as reviewed by gziemski (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/22204#pullrequestreview-2446053916 PR Review Comment: https://git.openjdk.org/jdk/pull/22204#discussion_r1848701088
On Tue, 19 Nov 2024 16:41:00 GMT, Gerard Ziemski <gziemski@openjdk.org> wrote:
Johan Sjölen has updated the pull request incrementally with one additional commit since the last revision:
Remove ZGC run test
src/hotspot/share/nmt/nmtUsage.cpp line 105:
103: _vm_total.committed += vm->committed(); 104: }); 105: }
I am missing something here, in memoryFileTracker.hpp you say:
// All memory is accounted as committed, there is no reserved memory. // Any reserved memory is expected to exist in the VirtualMemoryTracker.
but here we use MFT to account for both reserved and committed?
Ah, we actually do use VirtualMemory here to track the memory. Never mind! ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/22204#discussion_r1848714196
On Tue, 19 Nov 2024 16:48:28 GMT, Gerard Ziemski <gziemski@openjdk.org> wrote:
src/hotspot/share/nmt/nmtUsage.cpp line 105:
103: _vm_total.committed += vm->committed(); 104: }); 105: }
I am missing something here, in memoryFileTracker.hpp you say:
// All memory is accounted as committed, there is no reserved memory. // Any reserved memory is expected to exist in the VirtualMemoryTracker.
but here we use MFT to account for both reserved and committed?
Ah, we actually do use VirtualMemory here to track the memory. Never mind!
Yeah, but it is actually 0 :). I wanted to future proof this code, if we ever do use `reserved` here. I'll assert that `reserved` is 0 with a good message, then we can fix the code if our assumptions change. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/22204#discussion_r1849851012
On Wed, 20 Nov 2024 08:39:31 GMT, Johan Sjölen <jsjolen@openjdk.org> wrote:
Ah, we actually do use VirtualMemory here to track the memory. Never mind!
Yeah, but it is actually 0 :). I wanted to future proof this code, if we ever do use `reserved` here. I'll assert that `reserved` is 0 with a good message, then we can fix the code if our assumptions change.
Sorry, I was pretty confused here. We need to remove the reserved additions, otherwise we double-account reserved memory. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/22204#discussion_r1851785179
# Background
With the implementation of [JDK-8312132](https://bugs.openjdk.org/browse/JDK-8312132) we added the `MemoryFileTracker` (MFT). Unfortunately, this work failed to implement JFR integration. This, in turn, meant that (generational) ZGC has not correctly reported its heap usage via the NMT JFR events. We (as in Oracle) never saw this issue in testing, as we didn't run this test for all possible GC configurations. With an update of our test configurations this is no longer the case: We run this test for all possible GCs and the test now fails for generational ZGC.
# The fix
I implemented JFR events for the MFT by adding all of its reserved and committed memory into the JFR data, similarly to the `VirtualMemoryTracker`. I also added an explicit `run` using `ZGC` to the failing test, to ensure that any actual regressions are found.
Johan Sjölen has updated the pull request incrementally with one additional commit since the last revision: Do not add reserved memory. ------------- Changes: - all: https://git.openjdk.org/jdk/pull/22204/files - new: https://git.openjdk.org/jdk/pull/22204/files/4a11a22a..ef9a0511 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=22204&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=22204&range=03-04 Stats: 2 lines in 1 file changed: 0 ins; 2 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/22204.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/22204/head:pull/22204 PR: https://git.openjdk.org/jdk/pull/22204
On Thu, 21 Nov 2024 10:32:37 GMT, Johan Sjölen <jsjolen@openjdk.org> wrote:
# Background
With the implementation of [JDK-8312132](https://bugs.openjdk.org/browse/JDK-8312132) we added the `MemoryFileTracker` (MFT). Unfortunately, this work failed to implement JFR integration. This, in turn, meant that (generational) ZGC has not correctly reported its heap usage via the NMT JFR events. We (as in Oracle) never saw this issue in testing, as we didn't run this test for all possible GC configurations. With an update of our test configurations this is no longer the case: We run this test for all possible GCs and the test now fails for generational ZGC.
# The fix
I implemented JFR events for the MFT by adding all of its reserved and committed memory into the JFR data, similarly to the `VirtualMemoryTracker`. I also added an explicit `run` using `ZGC` to the failing test, to ensure that any actual regressions are found.
Johan Sjölen has updated the pull request incrementally with one additional commit since the last revision:
Do not add reserved memory.
That matches the comments from `src/hotspot/share/nmt/memoryFileTracker.hpp` I've seen the issue of overcounting pop-up couple of times recently - can we write a test that checks and would have caught the issue we ran into just here? ------------- Marked as reviewed by gziemski (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/22204#pullrequestreview-2451845562
On Thu, 21 Nov 2024 10:32:37 GMT, Johan Sjölen <jsjolen@openjdk.org> wrote:
# Background
With the implementation of [JDK-8312132](https://bugs.openjdk.org/browse/JDK-8312132) we added the `MemoryFileTracker` (MFT). Unfortunately, this work failed to implement JFR integration. This, in turn, meant that (generational) ZGC has not correctly reported its heap usage via the NMT JFR events. We (as in Oracle) never saw this issue in testing, as we didn't run this test for all possible GC configurations. With an update of our test configurations this is no longer the case: We run this test for all possible GCs and the test now fails for generational ZGC.
# The fix
I implemented JFR events for the MFT by adding all of its reserved and committed memory into the JFR data, similarly to the `VirtualMemoryTracker`. I also added an explicit `run` using `ZGC` to the failing test, to ensure that any actual regressions are found.
Johan Sjölen has updated the pull request incrementally with one additional commit since the last revision:
Do not add reserved memory.
Cheers Gerard. I'm not entirely sure of how to write a test for catching this, if you have any ideas that'd be great. ------------- PR Comment: https://git.openjdk.org/jdk/pull/22204#issuecomment-2493228546
On Mon, 18 Nov 2024 11:44:41 GMT, Johan Sjölen <jsjolen@openjdk.org> wrote:
# Background
With the implementation of [JDK-8312132](https://bugs.openjdk.org/browse/JDK-8312132) we added the `MemoryFileTracker` (MFT). Unfortunately, this work failed to implement JFR integration. This, in turn, meant that (generational) ZGC has not correctly reported its heap usage via the NMT JFR events. We (as in Oracle) never saw this issue in testing, as we didn't run this test for all possible GC configurations. With an update of our test configurations this is no longer the case: We run this test for all possible GCs and the test now fails for generational ZGC.
# The fix
I implemented JFR events for the MFT by adding all of its reserved and committed memory into the JFR data, similarly to the `VirtualMemoryTracker`. I also added an explicit `run` using `ZGC` to the failing test, to ensure that any actual regressions are found.
This pull request has now been integrated. Changeset: 2ea0364b Author: Johan Sjölen <jsjolen@openjdk.org> URL: https://git.openjdk.org/jdk/commit/2ea0364b6e3f10977f7b607d239c29ee616a8f7c Stats: 40 lines in 5 files changed: 29 ins; 6 del; 5 mod 8343893: Test jdk/jfr/event/runtime/TestNativeMemoryUsageEvents.java failed: heap should have grown and NMT should show that: expected 0 > 0 Reviewed-by: gziemski, mgronlun, lmesnik ------------- PR: https://git.openjdk.org/jdk/pull/22204
participants (4)
-
Gerard Ziemski
-
Johan Sjölen
-
Leonid Mesnik
-
Markus Grönlund