RFR: 8330076: [NMT] add/make a mandatory MEMFLAGS argument to family of os::reserve/commit/uncommit memory API [v10]
Stefan Karlsson
stefank at openjdk.org
Fri Apr 19 06:45:04 UTC 2024
On Thu, 18 Apr 2024 14:27:37 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:
>
> more improvements in style/alignments/adjustments.
Changes requested by stefank (Reviewer).
src/hotspot/os/windows/os_windows.cpp line 3401:
> 3399: // Reserve memory at an arbitrary address, only if that area is
> 3400: // available (and not reserved for something else).
> 3401: char* os::pd_attempt_reserve_memory_at(char* addr, size_t bytes, bool exec, MEMFLAGS nmt_flag) {
Most of the time the MEMFLAGS parameters are called flag but in some places they are called nmt_flag. Could they always be called flag? This might require some minor renames in some files, but I think that would be OK.
src/hotspot/share/cds/filemap.cpp line 1726:
> 1724: char *base = os::map_memory(_fd, _full_path, r->file_offset(), //file info
> 1725: addr, size, //memory info
> 1726: false /* !read_only */, r->allow_exec(), mtClassShared); // flags
Your new comments are not aligned properly.
However, I still don't think you should do this structural change. It makes the code inconsistent with the code below. If you do want to do it, I think you should change the rest of the code as well. But I don't think you should do that in this RFE, so I'd prefer if you just change this to:
Suggestion:
char *base = os::map_memory(_fd, _full_path, r->file_offset(),
addr, size, false /* !read_only */,
r->allow_exec(), mtClassShared);
src/hotspot/share/gc/shared/cardTable.cpp line 87:
> 85: ReservedSpace heap_rs(_byte_map_size, rs_align, _page_size, mtGC);
> 86:
> 87: os::trace_page_sizes("Card Table", num_bytes, num_bytes,
The indention is now messed up here.
src/hotspot/share/gc/shared/cardTable.cpp line 176:
> 174: old_committed.word_size() - new_committed.word_size());
> 175: bool res = os::uncommit_memory((char*)delta.start(),
> 176: delta.byte_size(), !ExecMem, mtGCCardSet);
Suggestion:
delta.byte_size(),
!ExecMem,
mtGCCardSet);
src/hotspot/share/memory/virtualspace.hpp line 180:
> 178: bool _executable;
> 179:
> 180: MEMFLAGS _nmt_flag;
Don't place the variable against the comment below.
Suggestion:
MEMFLAGS _nmt_flag;
src/hotspot/share/memory/virtualspace.hpp line 199:
> 197: size_t _upper_alignment;
> 198:
> 199:
Stray blankline
src/hotspot/share/nmt/virtualMemoryTracker.hpp line 306:
> 304: VirtualMemoryRegion(base, size), _stack(NativeCallStack::empty_stack()), _flag(mtNone) { }
> 305:
> 306: ReservedMemoryRegion(address base, size_t size, MEMFLAGS flag) :
(Commenting here because GitHub doesn't allow me to add comments to unchanged lines). I'd like to see a follow-up RFE that makes
ReservedMemoryRegion(address base, size_t size) :
VirtualMemoryRegion(base, size), _stack(NativeCallStack::empty_stack()), _flag(mtNone) { }
a private constructor, so that it is only used for our find operations and never accidentally used for other code.
src/hotspot/share/runtime/os.cpp line 2185:
> 2183: char* os::map_memory(int fd, const char* file_name, size_t file_offset, // file info
> 2184: char *addr, size_t bytes, // memory info
> 2185: bool read_only, bool allow_exec, MEMFLAGS flags) { // flags
I find it odd to make this documentation and separation only for this function. Could you remove that for this PR?
Suggestion:
char* os::map_memory(int fd, const char* file_name, size_t file_offset,
char *addr, size_t bytes, bool read_only
bool allow_exec, MEMFLAGS flags) {
src/hotspot/share/runtime/os.hpp line 232:
> 230: static char* pd_map_memory(int fd, const char* file_name, size_t file_offset,
> 231: char *addr, size_t bytes,
> 232: bool read_only = false, bool allow_exec = false);
I'd like to see this reverted.
src/hotspot/share/runtime/os.hpp line 518:
> 516: static char* map_memory(int fd, const char* file_name, size_t file_offset, // file info
> 517: char *addr, size_t bytes, // memory info
> 518: bool read_only, bool allow_exec, MEMFLAGS flag); // flags
Suggestion:
static char* map_memory(int fd, const char* file_name, size_t file_offset,
char *addr, size_t bytes, bool read_only,
bool allow_exec, MEMFLAGS flag);
-------------
PR Review: https://git.openjdk.org/jdk/pull/18745#pullrequestreview-2010616842
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1571879226
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1571880120
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1571885564
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1571886492
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1571892287
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1571892588
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1571896273
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1571898696
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1571899968
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1571901493
More information about the shenandoah-dev
mailing list