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

[Stefan Johansson]

Hi Sangheon (and Kim),

On 2019-10-11 19:34, sangheon.kim at oracle.com wrote:
>> ------------------------------------------------------------------------------ 
>>
>> 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.
> Fixed to directly call G1NUMA::request_memory_on_node() (previously 
> G1MemoryNodeManager).
> But G1NUMA can't calculate raw address, so I had to add base address at 
> G1NUMA to get that.
> 
> When I implemented it, I had similar opinion (not good fit for _storage) 
> but I also wanted to avoid adding extra dependency at G1NUMA. But anyway 
> I realized we can achieve it easily if we have base address.

I don't fully I agree here. I think having the storage do the call to 
G1NUMA does make sense because it knows how to translate a page index to 
a real address. It also goes along the same lines as the pretouch() call 
in commit_regions(), but I won't object if we want to leave it in the 
mapper.

If we do that, there are still some changes required, because we 
currently will call G1NUMA::request_memory_on_node() for all mappers and 
all mappers will then use the heaps base address when calling 
numa_make_local(). So I propose two changes:
1. Expose G1PageBasedVirtualSpace::page_start() or use 
G1CollectedHeap::bottom_addr_for_region(uint index) and let the mapper 
use it to call request_memory_on_node() with a real address rather than 
a page index. Another solution could be to change the function even more 
and call it request_heap_region_on_node() and just pass in the region 
index and then use G1CollectedHeap::bottom_addr_for_region(uint index) 
in G1NUMA.
2. Add a state to the mappers to say if they are NUMA aware or not, and 
currently only the heap mapper should be NUMA aware. We could either set 
this state to true using the mtJavaHeap type as we have checked before 
or add an explicit setter that we only call for the heap mapper.

I know that only doing 2) will fix the current problem, but I think it 
would be nice to avoid having the base address in G1NUMA, thoughts?

> 
> 
> FYI, I filed JDK-8232156 for further investigation of initialization 
> order related to G1NUMA. i.e. about removing G1NUMA::set_region_info().
> 
Thanks for filing this.

> New webrev includes:
> 1. Addressed most comments from Kim, Stefan and Thomas.
> 2. Rename G1MemoryNodeManager to G1NUMA with removing virtual calls.
> 
> webrev:
> http://cr.openjdk.java.net/~sangheki/8220310/webrev.4
> http://cr.openjdk.java.net/~sangheki/8220310/webrev.4.inc
Apart from my comment above I think this looks really good, just one 
small additional comment:
src/hotspot/os/linux/os_linux.cpp
---
3021 #endif
3022
3023   int id = InvalidNUMAId;

Extra whitespace on line 3022.
---

Thanks,
Stefan

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