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

[Stefan Johansson]

On 2019-10-07 10:45, Thomas Schatzl wrote:
> Hi,
> 
> On 07.10.19 10:25, Stefan Johansson wrote:
>> Hi Thomas and Sangheon,
>>
>> I have one comment on Thomas comments =)
>>
>> On 2019-10-04 14:11, Thomas Schatzl wrote:
>>> Hi Sangheon,
> [...]
>>>
>>> I.e. we eventually end up with the actual node index reported by the 
>>> OS in HeapRegion::_node_index.
>>>
>>
>> I like the idea of being able to get the correct node index from 
>> HeapRegion, but I have two concerns about the above idea. First, this 
>> will cause us to do a syscall while holding the lock to get a new 
>> region. This might not be a big deal, but I would prefer to do this
> I am not completely into the actual code flow right now, but I do not 
> think there is a need to get the node index in this code path from the 
> HeapRegion. Maybe when allocating from the free list later?

Allocating from the free list is also under the lock, but I think we are 
on the same page, just asking for the hr->node_index() should not cause 
a syscall.

> 
>> update during a safepoint. The second thing is that if pages get 
> 
> Fine with me too to piggyback it on some existing region iteration to be 
> 100% sure.
Yes, that should make the cost fairly low.

> 
>> migrated by the OS we would not see this if we only request the actual 
>> node index one time.
> 
> This is what the logging/verification is for I guess at this time. If 
> the migration is significant, we need to handle this and update the node 
> index - but I think we can do this node index update as RFE.

Yes, I have no idea if migration is a real problem, so separate RFE is ok.

> 
> Above update of the actual node index values during safepoint could also 
> "always" do the summary logging then (with gc+numa=debug or something) 
> if NUMA is enabled.
Sounds reasonable.

Thanks,
Stefan
> 
> Overall I would agree with that too.
> 
>> It's possible that both those concerns can be ignored, but I wanted to 
>> bring them up to hear others opinions.
> 
> Thanks,
>    Thomas