RFR: 8332042: Move MEMFLAGS to its own include file [v2]

[Stefan Karlsson]

On Mon, 13 May 2024 04:55:24 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> MEMFLAGS, as well as its enum constants, should live in its own include. 
>> 
>> The constants are used throughout the code base, often without needing the allocation APIs exposed through allocation.hpp.
>> 
>> The MEMFLAGS enum def is often needed within NMT itself, again often without needing allocation.hpp.
>> 
>> ---
>> 
>> This patch moves the enum to its new file.
>> 
>> It fixes those `allocation.hpp` includes that where only needed to get MEMFLAGS. It does not fix other includes. 
>> 
>> For backward compatibility, until we straightened out the dependencies (e.g., fixing all places where we rely on indirect includes), I added memflags.hpp to allocation.hpp.
>> 
>> I tested (built) on:
>> - MacOS aarch64, no precompiled headers, fastdebug
>> - Linux x64, no precompiled headers, fastdebug, release, fastdebug crossbuild to aarch64, fastdebug minimal
>
> Thomas Stuefe has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Update mallocLimit.hpp

Changes requested by stefank (Reviewer).

src/hotspot/share/nmt/mallocTracker.hpp line 29:

> 27: #define SHARE_NMT_MALLOCTRACKER_HPP
> 28: 
> 29: #include "nmt/memflags.hpp"

Should go after mallocHeader.hpp

src/hotspot/share/nmt/memflags.cpp line 27:

> 25: #include "precompiled.hpp"
> 26: 
> 27: #include "nmt/memflags.hpp"

There should be no blankline between precompiled.hpp and the rest of the includes.

src/hotspot/share/nmt/memflags.cpp line 31:

> 29: 
> 30: // Extra insurance that MEMFLAGS truly has the same size as uint8_t.
> 31: STATIC_ASSERT(sizeof(MEMFLAGS) == sizeof(uint8_t));

I think you can remove this entire .cpp file. There's no need to check the size of an enum with a specified base type.

src/hotspot/share/nmt/memflags.hpp line 30:

> 28: #include "utilities/globalDefinitions.hpp"
> 29: 
> 30: #define MEMORY_TYPES_DO(f)                                                           \

Open-ended comment/question: We call it MEMORY_TYPE and mt, but then we call the type MEMFLAGS (with a completely non-standard UPPERCASE style). Maybe it is time to rename MEMFLAGS?

src/hotspot/share/services/mallocLimit.cpp line 28:

> 26: #include "precompiled.hpp"
> 27: 
> 28: #include "nmt/memflags.hpp"

While poking around in the includes, could you remove the blankline on 27. This style inconsistency has slowly crept into the code base.

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

PR Review: https://git.openjdk.org/jdk/pull/19172#pullrequestreview-2052269037
PR Review Comment: https://git.openjdk.org/jdk/pull/19172#discussion_r1598224275
PR Review Comment: https://git.openjdk.org/jdk/pull/19172#discussion_r1598225428
PR Review Comment: https://git.openjdk.org/jdk/pull/19172#discussion_r1598226640
PR Review Comment: https://git.openjdk.org/jdk/pull/19172#discussion_r1598229830
PR Review Comment: https://git.openjdk.org/jdk/pull/19172#discussion_r1598231329