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

[sangheon.kim at oracle.com]

Hi Stefan,

On 10/9/19 2:40 PM, Stefan Johansson wrote:
> 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.
For the record, Stefan provided me a patch showing above idea of 
'non-NUMA case work exactly as a the NUMA case with one node'. The next 
webrev will include this change.

>
>> 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.
Done

> —
>
> src/hotspot/share/gc/g1/g1Allocator.hpp
> —
>   44   G1MemoryNodeManager* _mnm;
>
> I would prefer a more descriptive name like _memory_node_manager.
After changing to G1NUMA, all members will be _numa.

> —
>
> 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().
Same as above.

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

> —
>
> 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”.
I don't strong opinion on this but as Thomas suggests not to have such 
word, I removed it.
It will print something like, "| 00". Hope you are okay with this.

> —
>
> 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.
Changed as you suggested.
I had same opinion but the reason that I didn't choose was I wanted to 
avoid dependency for HeapRegion at G1NUMA.

> —
>
> 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.
N/A

> —
> 57   virtual bool has_multi_nodes() const { return false; }
>
> Same as above I would prefer has_multiple_nodes()
N/A

I will post next webrev after applying others' comments.

Thanks,
Sangheon


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