RFR: 8318447: Move NMT source code to own subdirectory [v7]

[Stefan Karlsson]

On Tue, 24 Oct 2023 11:51:45 GMT, Johan Sjölen <jsjolen at 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
>
> Johan Sjölen has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains eight commits:
> 
>  - Merge remote-tracking branch 'upstream/master' into move-nmt
>  - Fix stefank suggestions
>  - Merge remote-tracking branch 'origin/master' into move-nmt
>  - Fix messed up include
>  - Missed this include
>  - Merge remote-tracking branch 'origin/master' into move-nmt
>  - Fixed reviewed changes
>  - Move NMT to its own subdirectory

I've approved the patch, but I think you should revert the two gtest changes I mention below. I have given suggestions for two cleanups that you might want to do.

src/hotspot/share/memory/resourceArea.inline.hpp line 30:

> 28: #include "memory/resourceArea.hpp"
> 29: 
> 30: #include "nmt/memTracker.hpp"

Another new line that should not be here. Bonus points if it gets removed.

src/hotspot/share/nmt/mallocHeader.cpp line 29:

> 27: #include "nmt/mallocHeader.inline.hpp"
> 28: 
> 29: #include "nmt/mallocSiteTable.hpp"

There should be no newline between these two includes. If you make another round of changes I think it would be good to get rid of it.

test/hotspot/gtest/nmt/test_nmt_buffer_overflow_detection.cpp line 33:

> 31: #include "testutils.hpp"
> 32: #include "unittest.hpp"
> 33: 

It is inclear to me if there is an ordering requirement between testutils and unittest. I'd prefer if you didn't change those unit test header files in this patch and make these cleanups separately.

test/hotspot/gtest/nmt/test_nmt_locationprinting.cpp line 35:

> 33: // Uncomment to get test output
> 34: //#define LOG_PLEASE
> 35: 

There seems to be an ordering requirement between LOG_PLEASE and the inclusion of LOG_PLEASE. I think you break it with this change.

-------------

Marked as reviewed by stefank (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/16276#pullrequestreview-1697824750
PR Review Comment: https://git.openjdk.org/jdk/pull/16276#discussion_r1372058081
PR Review Comment: https://git.openjdk.org/jdk/pull/16276#discussion_r1372057335
PR Review Comment: https://git.openjdk.org/jdk/pull/16276#discussion_r1372053129
PR Review Comment: https://git.openjdk.org/jdk/pull/16276#discussion_r1372051716