On Thu, 19 Oct 2023 20:06:50 GMT, Johan Sjölen <jsjo...@openjdk.org> wrote:
> I think that NMT is deserving of its own subdirectory. Can we do a review of > the changes before I fix the merge conflicts? > > 1. Moved all the nmt source code from services/ to nmt/ > 2. Renamed all the include statements and sorted them > 3. Fixed the include guards Changes requested by stefank (Reviewer). Changes requested by stefank (Reviewer). src/hotspot/share/gc/parallel/psParallelCompact.cpp line 33: > 31: #include "code/codeCache.hpp" > 32: #include "compiler/oopMap.hpp" > 33: #include "gc/parallel/parMarkBitMap.inline.hpp" This uses a case-sensitive sort, whereas I think that when we add includes without using a sorting tool we would do so in a case-insensitive manner. I'm not sure we should make this change here. (The same goes for similar cases in the rest of the patch) src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 89: > 87: #include "runtime/safepointMechanism.hpp" > 88: #include "runtime/vmThread.hpp" > 89: #include "nmt/memTracker.hpp" This included end up at the wrong place. src/hotspot/share/nmt/memBaseline.cpp line 32: > 30: #include "runtime/safepoint.hpp" > 31: #include "nmt/memBaseline.hpp" > 32: #include "nmt/memTracker.hpp" Sort order src/hotspot/share/nmt/nmtPreInit.hpp line 33: > 31: #include "runtime/atomic.hpp" > 32: #endif > 33: #include "nmt/memTracker.hpp" The ASSERT Section should be moved down to after the main include block, or even better just include it without the guard. Might be nice to fix this while you are moving around the includes. src/hotspot/share/nmt/threadStackTracker.cpp line 31: > 29: #include "nmt/memTracker.hpp" > 30: #include "nmt/threadStackTracker.hpp" > 31: #include "nmt/virtualMemoryTracker.hpp" Sort order src/hotspot/share/nmt/virtualMemoryTracker.hpp line 33: > 31: #include "nmt/allocationSite.hpp" > 32: #include "nmt/nmtCommon.hpp" > 33: #include "utilities/linkedlist.hpp" This file didn't get an update to the include guard src/hotspot/share/services/mallocTracker.inline.hpp line 30: > 28: > 29: #include "services/mallocLimit.hpp" > 30: #include "services/mallocTracker.hpp" Missing include guard rename src/hotspot/share/services/nmtPreInit.hpp line 33: > 31: #include "runtime/atomic.hpp" > 32: #endif > 33: #include "services/memTracker.hpp" Missing include guard rename test/hotspot/gtest/nmt/test_nmt_buffer_overflow_detection.cpp line 31: > 29: #include "utilities/debug.hpp" > 30: #include "utilities/ostream.hpp" > 31: You make this change in some files but not all. Maybe revert this unrelated change? test/hotspot/gtest/nmt/test_nmt_cornercases.cpp line 26: > 24: > 25: #include "precompiled.hpp" > 26: You make this change in some files but not all. Maybe revert this unrelated change? ------------- PR Review: https://git.openjdk.org/jdk/pull/16276#pullrequestreview-1689312655 PR Review: https://git.openjdk.org/jdk/pull/16276#pullrequestreview-1689327479 PR Review Comment: https://git.openjdk.org/jdk/pull/16276#discussion_r1366529454 PR Review Comment: https://git.openjdk.org/jdk/pull/16276#discussion_r1366521538 PR Review Comment: https://git.openjdk.org/jdk/pull/16276#discussion_r1366522881 PR Review Comment: https://git.openjdk.org/jdk/pull/16276#discussion_r1366524596 PR Review Comment: https://git.openjdk.org/jdk/pull/16276#discussion_r1366524886 PR Review Comment: https://git.openjdk.org/jdk/pull/16276#discussion_r1366525477 PR Review Comment: https://git.openjdk.org/jdk/pull/16276#discussion_r1366530660 PR Review Comment: https://git.openjdk.org/jdk/pull/16276#discussion_r1366530318 PR Review Comment: https://git.openjdk.org/jdk/pull/16276#discussion_r1366526935 PR Review Comment: https://git.openjdk.org/jdk/pull/16276#discussion_r1366527038
