RFR: 8330076: [NMT] add/make a mandatory MEMFLAGS argument to family of os::reserve/commit/uncommit memory API [v7]

[Thomas Stuefe]

On Mon, 15 Apr 2024 16:11:13 GMT, Afshin Zafari <azafari at openjdk.org> wrote:

>> `MEMFLAGS flag` is used to hold/show the type of the memory regions in NMT. Each call of NMT API requires a search through the list of memory regions.
>> The Hotspot code reserves/commits/uncommits memory regions and later calls explicitly NMT API with a specific memory type (e.g., `mtGC`, `mtJavaHeap`) for that region.  Therefore, there are two search in the list of regions per reserve/commit/uncommit operations, one for the operation and another for setting the type of the region.  
>> When the memory type is passed in during reserve/commit/uncommit operations, NMT can use it and avoid the extra search for setting the memory type.
>> 
>> Tests: tiers1-5 passed on linux-x64, macosx-aarch64 and windows-x64 for debug and non-debug builds.
>
> Afshin Zafari has updated the pull request incrementally with one additional commit since the last revision:
> 
>   alignment in coding style changed.

src/hotspot/share/cds/metaspaceShared.cpp line 1322:

> 1320:                                        os::vm_page_size(), mtClassShared, (char*)base_address);
> 1321:       class_space_rs   = ReservedSpace(class_space_size, class_space_alignment,
> 1322:                                        os::vm_page_size(), mtClass, (char*)ccs_base);

Note that here, we place two spaces atop of a region that has been previously mapped with mtClass (see e.g. src/hotspot/cpu/aarch64/compressedKlass_aarch64.cpp). I assume this is not a problem?

src/hotspot/share/memory/virtualspace.cpp line 57:

> 55: }
> 56: 
> 57: ReservedSpace::ReservedSpace(size_t size, size_t preferred_page_size, MEMFLAGS flag) : _fd_for_heap(-1), _nmt_flag(flag) {

Small nit: Mixture of styles. As much as I dislike it, current style is to initialize things via dedicated initialize methods. I'd rather stay consistent.

That said, I would be more than happy for someone to give these classes a once-over and convert them to the more usual style - using initializer lists. Then, we also can make members const that should be const, e.g. _nmt_flags. Not in this PR though.

src/hotspot/share/memory/virtualspace.cpp line 623:

> 621:   }
> 622:   // _nmt_flag is used internally by initialize_compressed_heap
> 623:   _nmt_flag = mtJavaHeap;

Nit, we use a mixture of directly accessing _nmt_flag and accessing it via getter. Hotspot seems to prefer getters/setters. Can we use setters here?

src/hotspot/share/memory/virtualspace.hpp line 46:

> 44:   int      _fd_for_heap;
> 45:   bool     _executable;
> 46:   MEMFLAGS _nmt_flag;

See my remark below. This member, and probably others (e.g. page size and size) could and should probably be const. Food for follow up PRs.

src/hotspot/share/memory/virtualspace.hpp line 71:

> 69:  public:
> 70: 
> 71:   MEMFLAGS nmt_flag() { return _nmt_flag; }

const method

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1584163635
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1584153176
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1584158038
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1584154689
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1584150480