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

[Kim Barrett]

> On Oct 1, 2019, at 12:43 PM, sangheon.kim at oracle.com wrote:
> 
Here are my inline responses to yours.

> 
> On 9/24/19 6:44 PM, Kim Barrett wrote:
>>> On Sep 21, 2019, at 1:19 AM, sangheon.kim at oracle.com
>>>  wrote:
>>> 
>>> webrev:
>>> 
>>> http://cr.openjdk.java.net/~sangheki/8220310/webrev.1
>>> http://cr.openjdk.java.net/~sangheki/8220310/webrev.1.inc
>>>  (this may not help much! :) )
>>> Testing: hs-tier 1 ~ 5 (with/without UseNUMA)
>>> 
>> ------------------------------------------------------------------------------
>> src/hotspot/share/gc/g1/g1AllocRegion.hpp
>>   96   uint _node_index;
>> 
>> Protected; should be private.
>> 
> _node_index is used from derived classes.
> Are you suggesting to add a getter?

Oops, missed that it was used in derived classes.

I usually try to avoid non-private data members, and would add a getter here,
but that’s not a universal style in our code.

>> src/hotspot/share/gc/g1/g1Allocator.cpp
>>   53 G1Allocator::~G1Allocator() {
>>   54   for (uint i = 0; i < _num_alloc_region; i++) {
>>   55     _mutator_alloc_region[i].~MutatorAllocRegion();
>>   56   }
>>   57   FREE_C_HEAP_ARRAY(MutatorAllocRegion, _mutator_alloc_region);
>>   58 }
>> 
>> --- should also be calling _mutator_alloc_region[i].release() ??
>> --- or does destructor do that?
>> 
> No, release() is never called.
> release() is not actually releasing allocated resources but sets null to pointers and inc/dec some numbers such as used bytes. So I was thinking we don't need to call release().

Thanks for clarifying that.  That was a reminder for me to go figure that out,
but I forgot to do so before sending off that round of comments.

>> src/hotspot/share/gc/g1/g1PageBasedVirtualSpace.cpp
>>   83 G1PageBasedVirtualSpace::~G1PageBasedVirtualSpace() {
>> ...
>>   92   _numa                   = NULL;
>>   93 }
>> 
>> [pre-existing] Destructors are for resource management. Nulling out /
>> zeroing out members in a destructor generally isn't useful. This is
>> really a comment on the existing code rather than a request to change
>> anything. The addition of line 92 is okay in context, just the context
>> is not good.
>> 
> Agreed on pre-existing.
> The intent here is to align with existing context, so leave as is?

You can leave as is.

>> src/hotspot/share/gc/g1/g1NUMA.cpp
>>   42   memset(_numa_id_to_index_map, 
>>   43          G1MemoryNodeManager::InvalidNodeIndex,
>>   44          sizeof(uint) * _len_numa_id_to_index_map);
>> 
>> memset only works here because all bytes of InvalidNodeIndex happen to
>> have the same value.  I would prefer an explicit fill loop rather than
>> memset here.  Or a static assert on the value, but that's probably
>> more code.
>> 
> Changed to fill during loop.
> I'm aware of this and the only reason of changing InvalidNodeIndex from 0xfffe to 0xffff was to use memset here.
> I was thinking you are okay with memset as you commented to use memset from your previous email. :) 

Seems like my earlier suggestion to use memset was a bad idea…