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

[Kim Barrett]

> On Sep 16, 2019, at 7:12 PM, Kim Barrett <kim.barrett at oracle.com> wrote:
> 
>> On Sep 4, 2019, at 3:16 AM, sangheon.kim at oracle.com wrote:
>> 
>> Hi all,
>> 
>> Please review this patch, making G1 NUMA aware.
>> This is the first part of G1 NUMA implementation.
>> 
>> - At G1 CollectedHeap initialization time, the given heap regions will be split and touched.
>> - Internally node index(uint) is used instead of node id(int type) for easier access to arrays.
>> - Only Linux is supported.
>> - New logs are under gc+heap+numa
>> - Current memory touch implementation will be replaced with WorkGang by JDK-8230411: NUMA aware work gang.
>> 
>> CR: https://bugs.openjdk.java.net/browse/JDK-8220310
>> Webrev: http://cr.openjdk.java.net/~sangheki/8220310/webrev.0
>> Testing: hs-tier 1 ~ 5 with +- UseNUMA.
>> 
>> Thanks,
>> Sangheon
> 
> Here is an initial set of comments.  Some of these might be nonsense; I'm
> not all that familiar with NUMA support.

Here are a few more.

------------------------------------------------------------------------------
src/hotspot/share/gc/g1/g1MemoryNodeManager.hpp 
  64   // Returns node index of the given address.
  65   // If the node id of the address is invalid return randomly selected node index.
  66   virtual uint valid_index_of_address(HeapWord* o) const { return 0; }

This function is unused.

------------------------------------------------------------------------------
src/hotspot/share/gc/g1/g1MemoryNodeManager.hpp 
  70   virtual uint node_index_of_address(HeapWord* o) const { return 0; }

The only caller of this function is HeapRegionManager::allocate_free_region. 
As noted in an earlier comment, the result of that call is unused.

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

As pointed out to me offline, there *are* ways to query the address ->
associated numa node mapping.  So I'm going back and questioning the need
for _numa_id_of_pages_table and the code for managing it.

The above two comments (valid_index_of_address and node_index_of_address)
arise from that question.  If those functions were removed then the only
remaining caller of num_index_of_address is
FreeRegionList::remove_region_with_node_index, to get the numa index for a
heap region by querying the numa index of the region's bottom.  It seems
like it would be much easier to maintain that information in the region,
since that's the level at which we're interested anyway.

Among other things, that might make G1PageBasedNUMAVirtualSpace basically
moot.

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