RFR: 8337217: Port VirtualMemoryTracker to use VMATree [v21]
Johan Sjölen
jsjolen at openjdk.org
Mon Feb 10 14:17:39 UTC 2025
On Thu, 6 Feb 2025 15:51:41 GMT, Afshin Zafari <azafari at openjdk.org> wrote:
>> - `VMATree` is used instead of `SortedLinkList` in new class `VirtualMemoryTrackerWithTree`.
>> - A wrapper/helper `RegionTree` is made around VMATree to make some calls easier.
>> - Both old and new versions exist in the code and can be selected via `MemTracker::set_version()`
>> - `find_reserved_region()` is used in 4 cases, it will be removed in further PRs.
>> - All tier1 tests pass except one ~that expects a 50% increase in committed memory but it does not happen~ https://bugs.openjdk.org/browse/JDK-8335167.
>> - Adding a runtime flag for selecting the old or new version can be added later.
>> - Some performance tests are added for new version, VMATree and Treap, to show the idea and should be improved later. Based on the results of comparing speed of VMATree and VMT, VMATree shows ~40x faster response time.
>
> Afshin Zafari has updated the pull request incrementally with one additional commit since the last revision:
>
> fixed merge problems
Hi!
I've looked through some more of the PR and I've found multiple renamings from `type` to `tag`. It's great that you've done this, but can these changes be moved out into a separate PR for mainline? Keeping the diff minimal allows for much easier reviewing, and will let your hard work renaming type/flag to tag to be integrated sooner. I commented all the cases I found with "Move out into mainline PR".
For `vmtCommon.hpp`: If I remember correctly this refactoring was made because we had 2 separate implementations, but now we only have one. Restoring the code such that the content of `vmtCommon.hpp` is put into `virtualMemoryTracker.hpp` will further make the diff smaller and make it easier to find the actual changes to the code.
We're currently at +1528/-1846, that's a fairly large PR size. If we can make this smaller by separating out parts into multiple PRs and avoiding unnecessary refactoring of directory structure, then this can become quite a lot smaller and easier to review.
I hope you'll consider making these changes, as I think it makes it easier on the reviewers.
Thanks and all the best,
Johan
src/hotspot/share/nmt/mallocTracker.hpp line 163:
> 161: }
> 162:
> 163: inline const MallocMemory* by_tag(MemTag mem_tag) const {
Move out into mainline PR
src/hotspot/share/nmt/mallocTracker.hpp line 241:
> 239:
> 240: static inline void record_arena_size_change(ssize_t size, MemTag mem_tag) {
> 241: as_snapshot()->by_tag(mem_tag)->record_arena_size_change(size);
Move out into mainline PR
src/hotspot/share/nmt/mallocTracker.inline.hpp line 55:
> 53: l = MallocLimitHandler::category_limit(mem_tag);
> 54: if (l->sz > 0) {
> 55: const MallocMemory* mm = as_snapshot()->by_tag(mem_tag);
Move out into mainline PR
src/hotspot/share/nmt/memBaseline.cpp line 64:
> 62:
> 63: // Sort into allocation site addresses and memory tag order for baseline comparison
> 64: int compare_malloc_site_and_tag(const MallocSite& s1, const MallocSite& s2) {
Move out into mainline PR
src/hotspot/share/nmt/memBaseline.cpp line 235:
> 233: break;
> 234: case by_site_and_tag:
> 235: malloc_sites_to_allocation_site_and_tag_order();
Move out into mainline PR
src/hotspot/share/nmt/memBaseline.cpp line 275:
> 273:
> 274: void MemBaseline::malloc_sites_to_allocation_site_order() {
> 275: if (_malloc_sites_order != by_site && _malloc_sites_order != by_site_and_tag) {
Move out into mainline PR
src/hotspot/share/nmt/memBaseline.cpp line 292:
> 290: _malloc_sites.set_head(tmp.head());
> 291: tmp.set_head(nullptr);
> 292: _malloc_sites_order = by_site_and_tag;
Move out into mainline PR
src/hotspot/share/nmt/memBaseline.hpp line 56:
> 54: by_size, // by memory size
> 55: by_site, // by call site where the memory is allocated from
> 56: by_site_and_tag // by call site and memory tag
Move out into mainline PR (and indentation of comment seems wrong)
src/hotspot/share/nmt/memBaseline.hpp line 154:
> 152: VirtualMemory* virtual_memory(MemTag mem_tag) {
> 153: assert(baseline_type() != Not_baselined, "Not yet baselined");
> 154: return _virtual_memory_snapshot.by_tag(mem_tag);
Move out into mainline PR
src/hotspot/share/nmt/memBaseline.hpp line 207:
> 205: void malloc_sites_to_allocation_site_order();
> 206: // Sort allocation sites in call site address and memory tag order
> 207: void malloc_sites_to_allocation_site_and_tag_order();
Move out into mainline PR
src/hotspot/share/nmt/memReporter.cpp line 192:
> 190: }
> 191:
> 192: void MemSummaryReporter::report_summary_of_tag(MemTag mem_tag,
Move out into mainline PR
src/hotspot/share/nmt/memReporter.cpp line 201:
> 199: if (mem_tag == mtThread) {
> 200: const VirtualMemory* thread_stack_usage =
> 201: (const VirtualMemory*)_vm_snapshot->by_tag(mtThreadStack);
Move out into mainline PR
src/hotspot/share/nmt/memReporter.cpp line 243:
> 241: } else if (mem_tag == mtThread) {
> 242: const VirtualMemory* thread_stack_usage =
> 243: _vm_snapshot->by_tag(mtThreadStack);
Move out into mainline PR
src/hotspot/share/nmt/memReporter.cpp line 538:
> 536: // thread stack is reported as part of thread category
> 537: if (mem_tag == mtThreadStack) continue;
> 538: diff_summary_of_tag(mem_tag,
Move out into mainline PR
src/hotspot/share/nmt/memReporter.cpp line 608:
> 606:
> 607:
> 608: void MemSummaryDiffReporter::diff_summary_of_tag(MemTag mem_tag,
Move out into mainline PR
src/hotspot/share/nmt/memReporter.cpp line 810:
> 808: void MemDetailDiffReporter::diff_malloc_sites() const {
> 809: MallocSiteIterator early_itr = _early_baseline.malloc_sites(MemBaseline::by_site_and_tag);
> 810: MallocSiteIterator current_itr = _current_baseline.malloc_sites(MemBaseline::by_site_and_tag);
Move out into mainline PR
src/hotspot/share/nmt/memoryFileTracker.cpp line 47:
> 45: VMATree::SummaryDiff diff = file->_tree.commit_mapping(offset, size, regiondata);
> 46: for (int i = 0; i < mt_number_of_tags; i++) {
> 47: VirtualMemory* summary = file->_summary.by_tag(NMTUtil::index_to_tag(i));
Move out into mainline PR
src/hotspot/share/nmt/memoryFileTracker.cpp line 56:
> 54: VMATree::SummaryDiff diff = file->_tree.release_mapping(offset, size);
> 55: for (int i = 0; i < mt_number_of_tags; i++) {
> 56: VirtualMemory* summary = file->_summary.by_tag(NMTUtil::index_to_tag(i));
Move out into mainline PR
src/hotspot/share/nmt/memoryFileTracker.cpp line 187:
> 185: snap->commit_memory(current->committed());
> 186: }
> 187: }
Revert this change
src/hotspot/share/nmt/memoryFileTracker.hpp line 81:
> 79: const MemoryFile* file = _files.at(d);
> 80: for (int i = 0; i < mt_number_of_tags; i++) {
> 81: f(NMTUtil::index_to_tag(i), file->_summary.by_tag(NMTUtil::index_to_tag(i)));
Move out into mainline PR
src/hotspot/share/nmt/nmtCommon.cpp line 33:
> 31:
> 32: #define MEMORY_TAG_DECLARE_NAME(tag, human_readable) \
> 33: { #tag, human_readable },
Move out into mainline PR
src/hotspot/share/nmt/nmtCommon.hpp line 91:
> 89: // Map memory tag to index
> 90: static inline int tag_to_index(MemTag mem_tag) {
> 91: assert(tag_is_valid(mem_tag), "Invalid tag (%u)", (unsigned)mem_tag);
Move out into mainline PR
-------------
PR Review: https://git.openjdk.org/jdk/pull/20425#pullrequestreview-2605844859
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1949121262
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1949121527
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1949120937
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1949120631
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1949120394
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1949120180
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1949119941
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1949118303
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1949118619
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1949118841
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1949116400
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1949115914
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1949115644
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1949110432
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1949110306
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1949110184
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1949109029
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1949108903
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1949107753
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1949105112
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1949104485
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1949104091
More information about the hotspot-dev
mailing list