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

[Per Liden]

On 10/25/19 11:56 PM, sangheon.kim at oracle.com wrote:
> Hi Kim,
> 
> On 10/24/19 4:05 PM, Kim Barrett wrote:
>>> On Oct 23, 2019, at 12:20 PM,sangheon.kim at oracle.com  wrote:
>>>
>>> Hi Per,
>>>
>>> Thanks for taking a look at this.
>>>
>>> I agree all your comments and here's the webrev.
>>> - All comments from Per.
>>> - Move G1PageBasedVirtualSpace::page_size() near to page_start() from Kim.
>>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~sangheki/8220310/webrev.6
>>> http://cr.openjdk.java.net/~sangheki/8220310/webrev.6.inc
>>> Testing: build test for linux, solaris, windows and mac.
>>>
>>> FYI, as I think existing numa related API names and -1 stuff seem not good, I planned to refine those later after pushing. But as you said following existing rule and then refine all together later seems better.
>> The type of the argument for numa_get_group_id(void* address) should
>> be "const void*".  Sorry I didn't notice that earlier.  Of course,
>> this will require a const_cast to remove the const qualifier when
>> calling get_mempolicy, but it is better to isolate the workaround for
>> that missing qualifier to that one place.
>>
>> I'm not sure I like the overload for os::numa_get_group_id.  While
>> both are getting the numa id associated with something, the associations
>> involved seem pretty different to me.
>>
>> Spelling them out, they could be
>>
>> numa_get_group_id_for_current_thread()
>> numa_get_group_id_for_address(const void* address)
>>
>> Those seem semantically unrelated to me, so violate the usual guidance
>> of only overloading operations that are roughly equivalent (*).  Or put
>> another way, one should not need to determine which overload is selected
>> to understand a call site.
>>
>> Of course, "roughly equivalent" is in the eye of the beholder.
>>
>> (*) Operator overloading sometimes violates this on the basis that the
>> syntactic concision of using operators is more important, and there
>> are a limited set of operators.  Such violations are often used as an
>> argument against using operator overloading at all.
> I think the overload looks okay to me.
> But as you are not sure about it, I renamed the newly added one.
> 
> - static int numa_get_group_id(void* address);
> + static int numa_get_group_id_for_address(const void* address);
> 

Works for me.

/Per

> 
> webrev:
> http://cr.openjdk.java.net/~sangheki/8220310/webrev.7
> http://cr.openjdk.java.net/~sangheki/8220310/webrev.7.inc
> 
> Testing: hs-tier1
> 
> Thanks,
> Sangheon
> 
> 
>