Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8330076: NMT: add/make a mandatory MEMFLAGS argument to family of os::reserve/commit/uncommit memory API #18745

Closed

Conversation

afshin-zafari
Copy link
Contributor

@afshin-zafari afshin-zafari commented Apr 11, 2024

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.


Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue

Issue

  • JDK-8330076: NMT: add/make a mandatory MEMFLAGS argument to family of os::reserve/commit/uncommit memory API (Enhancement - P4)

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/18745/head:pull/18745
$ git checkout pull/18745

Update a local copy of the PR:
$ git checkout pull/18745
$ git pull https://git.openjdk.org/jdk.git pull/18745/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 18745

View PR using the GUI difftool:
$ git pr show -t 18745

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/18745.diff

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Apr 11, 2024

👋 Welcome back azafari! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

openjdk bot commented Apr 11, 2024

@afshin-zafari This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

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

Reviewed-by: stefank, jsjolen, stuefe

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 302 new commits pushed to the master branch:

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the rfr Pull request is ready for review label Apr 11, 2024
@openjdk
Copy link

openjdk bot commented Apr 11, 2024

@afshin-zafari The following labels will be automatically applied to this pull request:

  • hotspot
  • shenandoah

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added hotspot hotspot-dev@openjdk.org shenandoah shenandoah-dev@openjdk.org labels Apr 11, 2024
@mlbridge
Copy link

mlbridge bot commented Apr 11, 2024

@tstuefe
Copy link
Member

tstuefe commented Apr 11, 2024

The general thrust of this is okay, and I think it makes sense. It also combines nicely with the VMATree work Johan does.

One question though, why do you want to abolish default values for MEMFLAGS? Allowing default values would reduce this patch by quite a bit, and make backporting less painful.

Another idea: To alleviate the need to pass MEMFLAGS all the time, could we have something like a "active MEMFLAGS" state per Thread, and set that stack-based with a XXMark object? That way, one could say at the entrance of Metaspace, for instance, "whatever is allocated under the scope of this function, please mark with mtMetaspace".

Just an idea. Otherwise, I think this is good, and thanks for doing this onerous work.

@afshin-zafari
Copy link
Contributor Author

One question though, why do you want to abolish default values for MEMFLAGS? Allowing default values would reduce this patch by quite a bit, and make backporting less painful.

To be sure that all the calls without MEMFLAGS are changed in PR, Ii made it mandatory to let compiler find all the instances. There are some cases hidden or not found easily if I let the arg be optional.

@afshin-zafari
Copy link
Contributor Author

afshin-zafari commented Apr 11, 2024

Another idea: To alleviate the need to pass MEMFLAGS all the time, could we have something like a "active MEMFLAGS" state per Thread, and set that stack-based with a XXMark object? That way, one could say at the entrance of Metaspace, for instance, "whatever is allocated under the scope of this function, please mark with mtMetaspace".

Not sure if I understood your idea, the question is if a thread always uses only ONE type of memory and not mix of them? For example, CDS uses both mtClass and mtClassShared. If a Thread has an active MEMFLAGS, it has to switch this flag between A and B whenever it uses type A or B.

Copy link
Member

@stefank stefank left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is good that we are fixing this. I've on the verge of doing it myself a couple of times. I've left an initial set of comments below. It's not an exhaustive list, but I think it is a good set to start with.

@@ -78,7 +78,7 @@ XPhysicalMemoryBacking::XPhysicalMemoryBacking(size_t max_capacity) :
_initialized(false) {

// Reserve address space for backing memory
_base = (uintptr_t)os::reserve_memory(max_capacity);
_base = (uintptr_t)os::reserve_memory(max_capacity, false, mtJavaHeap);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be using !MemExec and not a raw 'false' argument.

With that said, I really think it would be best if we actually split os::reserve_memory into two distinct functions:

  1. os::reserve_memory(max_capacity, mtJavaHeap) // Non-executable memory
  2. os::reserve_executable_memory(max_capacity, mtCode) // Executable

Maybe we could think about that in separate RFE.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

false/true constants are not used in executable args.
separate reserve_memory functions can be left for another RFE.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The executable argument really is only false in the original, can we keep this from doing any functional changes here and keep that to separate PR:s?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand. Are you proposing something else than what I proposed that we could do in a separate RFE?

@@ -79,7 +79,7 @@ ZPhysicalMemoryBacking::ZPhysicalMemoryBacking(size_t max_capacity)
_initialized(false) {

// Reserve address space for backing memory
_base = (uintptr_t)os::reserve_memory(max_capacity);
_base = (uintptr_t)os::reserve_memory(max_capacity, false, mtJavaHeap);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use !MemExec - this goes for all other places in the patch that fills in the exec parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@@ -4681,18 +4681,18 @@ static void workaround_expand_exec_shield_cs_limit() {
*/
char* hint = (char*)(os::Linux::initial_thread_stack_bottom() -
(StackOverflow::stack_guard_zone_size() + page_size));
char* codebuf = os::attempt_reserve_memory_at(hint, page_size);
char* codebuf = os::attempt_reserve_memory_at(hint, page_size, false, mtInternal);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these be mtInternal or is there a mtStack that is more suitable?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In line 4699, a few lines later, the original developer used mtInternal. I copied it here too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. Then this is fine for now.

@@ -3134,7 +3134,7 @@ static char* allocate_pages_individually(size_t bytes, char* addr, DWORD flags,
PAGE_READWRITE);
// If reservation failed, return null
if (p_buf == nullptr) return nullptr;
MemTracker::record_virtual_memory_reserve((address)p_buf, size_of_reserve, CALLER_PC);
MemTracker::record_virtual_memory_reserve((address)p_buf, size_of_reserve, CALLER_PC, mtNone);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this (and the other places in this file) using mtNone? Shouldn't it at least be using mtInternal?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@@ -1414,7 +1411,7 @@ void ShenandoahHeap::object_iterate(ObjectClosure* cl) {
bool ShenandoahHeap::prepare_aux_bitmap_for_iteration() {
assert(SafepointSynchronize::is_at_safepoint(), "safe iteration is only available during safepoints");

if (!_aux_bitmap_region_special && !os::commit_memory((char*)_aux_bitmap_region.start(), _aux_bitmap_region.byte_size(), false)) {
if (!_aux_bitmap_region_special && !os::commit_memory((char*)_aux_bitmap_region.start(), _aux_bitmap_region.byte_size(), false, mtJavaHeap)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this mtJavaHeap and not mtGC?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@@ -68,40 +68,40 @@ ReservedSpace::ReservedSpace(size_t size, size_t preferred_page_size) : _fd_for_

ReservedSpace::ReservedSpace(size_t size,
size_t alignment,
size_t page_size,
char* requested_address) : _fd_for_heap(-1) {
size_t page_size, MEMFLAGS flag,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this function was written to have one argument per line. You should probably keep the style.

I'm also unsure why this param is put as the next to last param instead of the last, as we do in many other places.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Put in separate line.
the last param is optional, and flag is to be mandatory.

Comment on lines 365 to 366
space.initialize_members(base, size, alignment, page_size, special, executable);
space.set_nmt_flag(flag);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this calling a set_nmt_flag instead of making initialize_member take a flag?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@@ -689,6 +690,7 @@ VirtualSpace::VirtualSpace() {
_upper_alignment = 0;
_special = false;
_executable = false;
_nmt_flag = mtNone;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weird indentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

Copy link
Member

@stefank stefank Apr 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still looks weird to me.

@@ -42,12 +42,13 @@ class ReservedSpace {
size_t _page_size;
bool _special;
int _fd_for_heap;
MEMFLAGS _nmt_flag;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation is now off.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It still looks wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be fixed now.

Comment on lines 71 to 72
inline MEMFLAGS nmt_flag() { return _nmt_flag; }
inline void set_nmt_flag(MEMFLAGS flag) { _nmt_flag = flag; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for the inline specifier here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

Copy link
Contributor

@jdksjolen jdksjolen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Afshin,

Thank you for this!

I found a couple of things.

@tstuefe , would you mind having a look at the Metaspace changes?

@@ -2264,7 +2261,7 @@ bool ShenandoahHeap::commit_bitmap_slice(ShenandoahHeapRegion* r) {
size_t len = _bitmap_bytes_per_slice;
char* start = (char*) _bitmap_region.start() + off;

if (!os::commit_memory(start, len, false)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should probably be mtGC. @shipilev, you don't happen to know whether this should be accounted under the Java heap? Thank you.

Copy link
Member

@shipilev shipilev Apr 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is bitmap slice, so it is not Java heap, it is mtGC. In the sister method, uncommit_bitmap_slice, we do the right thing: mtGC.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

assert(addr != nullptr, "Invalid address");
assert(size > 0, "Invalid size");
assert(_reserved_regions != nullptr, "Sanity check");

ReservedMemoryRegion rgn(addr, size);
ReservedMemoryRegion rgn(addr, size, NativeCallStack::empty_stack(), flag);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead, change the constructor so that it takes a flag?

  ReservedMemoryRegion(address base, size_t size, MEMFLAGS flag) :
    VirtualMemoryRegion(base, size), _stack(NativeCallStack::empty_stack()), _flag(flag) { }

Or does that break somewhere else?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. No problem.

Copy link
Contributor Author

@afshin-zafari afshin-zafari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All comments applied. Ready for another round of reviews.

@tstuefe
Copy link
Member

tstuefe commented Apr 12, 2024

Another idea: To alleviate the need to pass MEMFLAGS all the time, could we have something like a "active MEMFLAGS" state per Thread, and set that stack-based with a XXMark object? That way, one could say at the entrance of Metaspace, for instance, "whatever is allocated under the scope of this function, please mark with mtMetaspace".

Not sure if I understood your idea, the question is if a thread always uses only ONE type of memory and not mix of them? For example, CDS uses both mtClass and mtClassShared. If a Thread has an active MEMFLAGS, it has to switch this flag between A and B whenever it uses type A or B.

No, the idea was to do it stack-based with a chained mark object. But never mind, we can do this in a follow up PR. Maybe it has no merit.

Copy link
Member

@stefank stefank left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few more comments.

@@ -3134,7 +3134,7 @@ static char* allocate_pages_individually(size_t bytes, char* addr, DWORD flags,
PAGE_READWRITE);
// If reservation failed, return null
if (p_buf == nullptr) return nullptr;
MemTracker::record_virtual_memory_reserve((address)p_buf, size_of_reserve, CALLER_PC);
MemTracker::record_virtual_memory_reserve((address)p_buf, size_of_reserve, CALLER_PC, mtInternal);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that allocate_pages_individually should take a MEMFLAGS argument instead of using mtInternal here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It takes now. All corresponding functions in call hierarchy are also changed.

@@ -3195,7 +3195,7 @@ static char* allocate_pages_individually(size_t bytes, char* addr, DWORD flags,
// need to create a dummy 'reserve' record to match
// the release.
MemTracker::record_virtual_memory_reserve((address)p_buf,
bytes_to_release, CALLER_PC);
bytes_to_release, CALLER_PC, mtNone);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should ever use mtNone in code outside of the NMT code. If you follow my suggestion above that allocate_pages_individually should take a MEMFLAG arg, then it could be used here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Corrected.

@@ -3215,7 +3215,7 @@ static char* allocate_pages_individually(size_t bytes, char* addr, DWORD flags,
if ((flags & MEM_COMMIT) != 0) {
MemTracker::record_virtual_memory_reserve_and_commit((address)p_buf, bytes, CALLER_PC);
} else {
MemTracker::record_virtual_memory_reserve((address)p_buf, bytes, CALLER_PC);
MemTracker::record_virtual_memory_reserve((address)p_buf, bytes, CALLER_PC, mtNone);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use the correct MEMFLAG here instead of mtNone.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -3768,7 +3768,7 @@ bool os::protect_memory(char* addr, size_t bytes, ProtType prot,
// memory, not a big deal anyway, as bytes less or equal than 64K
if (!is_committed) {
commit_memory_or_exit(addr, bytes, prot == MEM_PROT_RWX,
"cannot commit protection page");
"cannot commit protection page", mtNone);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be something else than mtNone.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to mtInternal. When protect_memory is called with uncommitted area (is_committed == false), the flag is mtInternal.

@@ -104,7 +104,7 @@ bool JfrVirtualMemorySegment::initialize(size_t reservation_size_request_bytes)
assert(is_aligned(reservation_size_request_bytes, os::vm_allocation_granularity()), "invariant");
_rs = ReservedSpace(reservation_size_request_bytes,
os::vm_allocation_granularity(),
os::vm_page_size());
os::vm_page_size(), mtTracing);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The mtTracing should probably be on a separate line, so that it follows the style of the surrounding code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -508,7 +508,7 @@ class os: AllStatic {
// Map memory to the file referred by fd. This function is slightly different from map_memory()
// and is added to be used for implementation of -XX:AllocateHeapAt
static char* map_memory_to_file(size_t size, int fd, MEMFLAGS flag = mtNone);
static char* map_memory_to_file_aligned(size_t size, size_t alignment, int fd, MEMFLAGS flag = mtNone);
static char* map_memory_to_file_aligned(size_t size, size_t alignment, int fd, MEMFLAGS flag);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are still a few mtNone usages in this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mtNone as default value for optional arguments is removed from all function definitions.

@@ -50,7 +50,7 @@ TEST_VM(FreeRegionList, length) {
// the BOT.
size_t bot_size = G1BlockOffsetTable::compute_size(heap.word_size());
HeapWord* bot_data = NEW_C_HEAP_ARRAY(HeapWord, bot_size, mtGC);
ReservedSpace bot_rs(G1BlockOffsetTable::compute_size(heap.word_size()));
ReservedSpace bot_rs(G1BlockOffsetTable::compute_size(heap.word_size()), mtGC);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mtGC => mtTest?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -100,7 +100,7 @@ class ZForwardingTest : public Test {

_reserved = reserved;

os::commit_memory((char*)_reserved, ZGranuleSize, false /* executable */);
os::commit_memory((char*)_reserved, ZGranuleSize, !ExecMem /* executable */, mtGC);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mtGC => mtTest?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -111,7 +111,7 @@ class ZForwardingTest : public Test {
ZGeneration::_old = _old_old;
ZGeneration::_young = _old_young;
if (_reserved != nullptr) {
os::uncommit_memory((char*)_reserved, ZGranuleSize, false /* executable */);
os::uncommit_memory((char*)_reserved, ZGranuleSize, !ExecMem, mtGC);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mtGC => mtTest?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

case Disable:
case Commit:
return ReservedSpace(reserve_size_aligned,
os::vm_allocation_granularity(),
os::vm_page_size());
os::vm_page_size(), mtTest);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

newline before mtTest.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Member

@tstuefe tstuefe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly good. Small nits inside.

@@ -109,7 +109,7 @@ bool VirtualSpaceNode::commit_range(MetaWord* p, size_t word_size) {
}

// Commit...
if (os::commit_memory((char*)p, word_size * BytesPerWord, false) == false) {
if (os::commit_memory((char*)p, word_size * BytesPerWord, !ExecMem, _rs.nmt_flag()) == false) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just use mtMetaspace here, its easier

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -188,7 +188,7 @@ void VirtualSpaceNode::uncommit_range(MetaWord* p, size_t word_size) {
}

// Uncommit...
if (os::uncommit_memory((char*)p, word_size * BytesPerWord) == false) {
if (os::uncommit_memory((char*)p, word_size * BytesPerWord, !ExecMem, _rs.nmt_flag()) == false) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mtMetaspace

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -78,7 +78,7 @@ MetaspaceTestContext::MetaspaceTestContext(const char* name, size_t commit_limit
reserve_limit, Metaspace::reserve_alignment_words());
if (reserve_limit > 0) {
// have reserve limit -> non-expandable context
_rs = ReservedSpace(reserve_limit * BytesPerWord, Metaspace::reserve_alignment(), os::vm_page_size());
_rs = ReservedSpace(reserve_limit * BytesPerWord, Metaspace::reserve_alignment(), os::vm_page_size(), mtTest);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mtMetaspace

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not done yet

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be done now.

@@ -518,7 +518,7 @@ class os: AllStatic {
char *addr, size_t bytes, bool read_only = false,
bool allow_exec = false, MEMFLAGS flags = mtNone);
static bool unmap_memory(char *addr, size_t bytes);
static void free_memory(char *addr, size_t bytes, size_t alignment_hint);
static void free_memory(char *addr, size_t bytes, size_t alignment_hint, MEMFLAGS flag);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While looking at this, I noticed a couple of odd things about this function. I think it should be revised and I opened https://bugs.openjdk.org/browse/JDK-8330144. The result of that revision will be that we don't need MEMFLAGS, nor do would we need the alignment hint.

But leave the MEMFLAGS in for now. If I happen to push that change first, you can adapt the change, if you push first I'll manage.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK.