RFR(XL): 8220310: Implementation: NUMA-Aware Memory Allocation for G1, Mutator (1/3)

[Kim Barrett]

> On Oct 9, 2019, at 12:27 AM, sangheon.kim at oracle.com wrote:
> Webrev:
> http://cr.openjdk.java.net/~sangheki/8220310/webrev.3
> http://cr.openjdk.java.net/~sangheki/8220310/webrev.3.inc
> Testing: hs-tier 1~5, with/without UseNUMA

I agree with Stefan and Thomas; this is looking pretty good.

There are some naming issues that I'm not going to comment on here.
Stefan has already commented on some, and a bit of offline discussion
suggests there's a larger naming discussion needed, but which can
follow getting the functionality we want.

There has been further discission offline toward collapsing
G1MemoryNodeManager to one class without virtual dispatch, and using
G1NUMA name. I won't bother to re-iterate any of that here.

------------------------------------------------------------------------------
src/hotspot/share/gc/g1/g1Allocator.cpp
 186   assert(Heap_lock->owner() != NULL, "Should be owned on this thread's behalf.");

Use assert_lock_strong(Heap_lock).

------------------------------------------------------------------------------
src/hotspot/share/gc/g1/g1RegionToSpaceMapper.cpp
  82       _storage.request_memory_on_node(page, _pages_per_region, node_index);
...
 153         _storage.request_memory_on_node(idx, 1, node_index);

I'm not sure request_memory_on_node belongs on the _storage object.
The current implementation just has the storage object (conditionally)
forward the request to the memory node manager object. These places in
the space mapper could just make the calls on the memory node manager
object directly (it is already being used nearby).  And these places
don't need the conditionalization.

I think making the space mapper directly call the memory node manager
here would remove the need for the proposed changes to the virtual
space class.

------------------------------------------------------------------------------
src/hotspot/share/gc/g1/heapRegion.cpp
 464     st->print("|Node ID %02d", node_ids[this->node_index()]);

The unchecked use of node_index() here can run afoul of an unset (so
UnknownNodeIndex) index.

Also, no need for `this->` in `this->node_index()`.

------------------------------------------------------------------------------
src/hotspot/share/gc/g1/g1MemoryNodeManager.hpp 
  81   virtual const uint max_search_depth() const { return 1; }

s/const uint/uint/

Similarly for other declarations and definitions.

------------------------------------------------------------------------------
src/hotspot/share/gc/g1/g1MemoryNodeManager.hpp 
  77   virtual void request_memory_on_node(char* aligned_address, size_t size_in_bytes, uint node_index) { }

Shouldn't the aligned_address argument be typed "void*" rather than "char*"?

------------------------------------------------------------------------------
src/hotspot/share/gc/g1/heapRegionManager.cpp
 112   if (mgr->has_multi_nodes() && requested_node_index != G1MemoryNodeManager::AnyNodeIndex) {

I think it would be better to test the requested_node_index value
first.  The "any" case is a common case.

------------------------------------------------------------------------------
src/hotspot/share/gc/g1/heapRegionManager.cpp
 200   if(AlwaysPreTouch) {

Add space after "if".

------------------------------------------------------------------------------
src/hotspot/share/gc/g1/heapRegionManager.cpp
 311   return region_node_index == preferred_node_index;

Fix indentation.

------------------------------------------------------------------------------
src/hotspot/share/runtime/os.hpp
 393   static const int InvalidId = -1;

This should probably be "InvalidNUMAId" or something like that.

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