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

[Stefan Johansson]

Hi Sangheon,

Thanks again for a much improved version. Some comments below.

> 9 okt. 2019 kl. 06:27 skrev sangheon.kim at oracle.com:
> 
> ...
> 
> Here's the major change list at the webrev. Or arguable list :)
> 1) Verification at HRM::allocate_free_region() is removed and it will be added somewhere at safepoint by JDK-8220312 (3/3 which is part of this JEP). Probably at the end of young gc?
> 2) Node id printing is changed. Removed old one and added at HeapRegion::print_on() with new column. Node id is only printed when UseNUMA is enabled and gc+heap+region=trace. If there's single active node, it will print the node id and this is intentional. Another approach would be printing only if there are multiple nodes.
> 3) If AlwaysPreTouch is enabled, HeapRegion will have actual node index instead of preferred node index.
> 4) HeapRegion::_node_index is set at HRM::make_regions_available() as there is the only place initializing HeapRegion. Another approach would be setting the index at HeapRegion::initialize(we have to pollute HR with G1MNM stuff) or conditionally(*) setting the index at HeapRegion::node_index(). (*) if the index is unknown etc..
> 5) G1NUMA class is merged into G1MemoryNodeManager.

I saw your comment above about suggestions around this area and I can try out one thought I had, something I think Thomas mentioned as well. Making the non-NUMA case work exactly as a the NUMA case with one node. I’ll need some more time for that, but below are my comments on the current patch.

> 
> Webrev:
> http://cr.openjdk.java.net/~sangheki/8220310/webrev.3
> http://cr.openjdk.java.net/~sangheki/8220310/webrev.3.inc

src/hotspot/os/linux/os_linux.cpp
—
3026     warning("Failed to get numa id at " PTR_FORMAT " with errno=%d", p2i((void*)address), errno);

The cast here is no longer needed.
—

src/hotspot/share/gc/g1/g1Allocator.hpp
—
 44   G1MemoryNodeManager* _mnm;

I would prefer a more descriptive name like _memory_node_manager.
—

src/hotspot/share/gc/g1/g1CollectedHeap.hpp
—
 196   // Manages single or multi node memory.
 197   G1MemoryNodeManager* _mem_node_mgr;
 ...
 558   G1MemoryNodeManager* mem_node_mgr() const { return _mem_node_mgr; }

As above, I would prefer spelling out the names to memory_node_manager().
—

src/hotspot/share/gc/g1/g1_globals.hpp
—
Last line still removed a ’\’, please revert this change.
—

src/hotspot/share/gc/g1/heapRegion.cpp
—
 462   if (UseNUMA) {
 463     const int* node_ids = G1MemoryNodeManager::mgr()->node_ids();
 464     st->print("|Node ID %02d", node_ids[this->node_index()]);
 465   }
 466   st->print_cr("”);

I would prefer having a function that returns the node id given the index. Like the inverse of index_of_node_id(). 

I also think it would be more informative to say "NUMA id" or "NUMA node”.
—

src/hotspot/share/gc/g1/heapRegionManager.cpp
—
 195 // Set node index of the given HeapRegion.
 196 // If AlwaysPreTouch is enabled, set with actual node index.
 197 // If it is disabled, set with preferred node index which is already decided.
 198 static void set_heapregion_node_index(HeapRegion* hr) {
 199   uint node_index;
 200   if(AlwaysPreTouch) {
 201     // If we already pretouched, we can check actual node index here.
 202     node_index = G1MemoryNodeManager::mgr()->index_of_address(hr->bottom());
 203   } else {
 204     node_index = G1MemoryNodeManager::mgr()->preferred_node_index_for_index(hr->hrm_index());
 205   }
 206   hr->set_node_index(node_index);
 207 }

I would prefer to have a helper for calculating the index to set not a helper for setting the index. If you agree, you could move this logic to G1MemoryNodeManager::index_for_region() and then you can change:
 233     // Set node index of the heap region after initialization but before inserting
 234     // to free list.
 235     set_heapregion_node_index(hr);

To just:
 235     hr->set_node_index(G1MemoryNodeManager::mgr()->index_for_region(hr));
—
 309  bool HeapRegionManager::is_on_preferred_index(uint region_index, uint preferred_node_index) {
 310    uint region_node_index = G1MemoryNodeManager::mgr()->preferred_node_index_for_index(region_index);
 311   return region_node_index == preferred_node_index;
 312  }

Indentation on row 311.
—

src/hotspot/share/gc/g1/g1MemoryNodeManager.hpp
—
 44   static G1MemoryNodeManager* mgr() { return _inst; }

I think we should change the name of this getter to manager(), to avoid unnecessary shortenings. 
—
57   virtual bool has_multi_nodes() const { return false; }

Same as above I would prefer has_multiple_nodes()
—

Thanks,
Stefan

> Testing: hs-tier 1~5, with/without UseNUMA
> 
> Thanks,
> Sangheon