On Mon, 11 May 2026 16:00:48 GMT, Robert Toyonaga <[email protected]> wrote:
> ### Summary > `os::committed_in_range` is used by NMT to account for thread stacks. The > name is misleading, it actually is meant to find **live** pages not just > **committed** pages. On POSIX (except AIX) this works correctly using > `mincore`. On Windows this worked incorrectly using `VirtualQuery` (only > finds committed, not live regions). This means that the values reported on > Windows were inaccurate. > > This PR fixes the Windows implementation by using `QueryWorkingSetEx` instead > of `VirtualQuery`. This properly identifies pages that are truly live in the > working set, not just committed. I also renamed `committed_in_range` to > `live_in_range` so that the intention is clearer. I have tried to structure > the Windows implementation as similarly to Posix as possible to help with > maintainability. > > ### Testing > - Tested on windows and linux and64 > - `make test TEST=gtest:NMTCommittedVirtualMemory` (this tests > `live_in_range` directly) > - `make test > TEST=test/hotspot/jtreg/runtime/Thread/TestAlwaysPreTouchStacks.java` (this > tests `live_in_range` indirectly through NMT thread stack accounting) > - `make test TEST=gtest:os` > - `make test TEST=hotspot_nmt` > - tier 1 > --------- > - [x] I confirm that I make this contribution in accordance with the [OpenJDK > Interim AI Policy](https://openjdk.org/legal/ai). src/hotspot/os/windows/os_windows.cpp line 499: > 497: } > 498: > 499: for (int i = 0; i < pages_to_query; i++) { signed vs unsigned? Did VC++ accept this? src/hotspot/os/windows/os_windows.cpp line 518: > 516: if (live_start != nullptr) { > 517: assert(live_pages > 0, "Must have live region"); > 518: assert(live_pages <= size / page_sz, "Region cannot be smaller than > size of live pages"); Maybe a bit clearer (same on POSIX) Suggestion: assert(live_pages <= size / page_sz, "resident size exceeds region size?"); src/hotspot/share/runtime/os.hpp line 447: > 445: // The search begins at the provided start address. > 446: // Returns true after the first contiguous live region is found, > otherwise false if none found. > 447: static bool live_in_range(address start, size_t size, address& > committed_start, size_t& committed_size); We probably want to rename the output parameters too. test/hotspot/gtest/runtime/test_committed_virtualmemory.cpp line 173: > 171: bool result; > 172: size_t committed_size; > 173: address committed_start; These should probably be renamed to reflect whatever name we decide on ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/31124#discussion_r3272274290 PR Review Comment: https://git.openjdk.org/jdk/pull/31124#discussion_r3272289322 PR Review Comment: https://git.openjdk.org/jdk/pull/31124#discussion_r3272270332 PR Review Comment: https://git.openjdk.org/jdk/pull/31124#discussion_r3272305965
