RFR: 8332042: Move MEMFLAGS to its own include file [v2]
Stefan Karlsson
stefank at openjdk.org
Mon May 13 10:22:09 UTC 2024
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
More information about the serviceability-dev
mailing list