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

[Stefan Johansson]

> 29 okt. 2019 kl. 21:39 skrev sangheon.kim at oracle.com:
> 
> Hi Kim and Per,
> 
> Thanks for your reviews.
> 
> -----------
> To all reviewers,
> 
> Stefan suggested a safer handling of node index so here's another webrev.
> Basically when we enable AlwaysPreTouch, we expect to get actual node id of the address.
> However, in theory we still may get something unknown id. So below change is added to have safer handling of node index.
> 
> uint G1NUMA::index_for_region(HeapRegion* hr) const {
>   if (!is_enabled()) {
>     return 0;
>   }
> 
> 
>    if (AlwaysPreTouch) {
>      // If we already pretouched, we can check actual node index here.
> -  return index_of_address(hr->bottom());
> 
> +    // However, if node index is still unknown, use preferred node index.
> +    uint node_index = index_of_address(hr->bottom());
> +    if (node_index != UnknownNodeIndex) {
> +      return node_index;
> +    }
> 
> Webrev:
> http://cr.openjdk.java.net/~sangheki/8220310/webrev.8
> http://cr.openjdk.java.net/~sangheki/8220310/webrev.8.inc
Looks good,
Stefan
> Testing: local build
> 
> Thanks,
> Sangheon
> 
> 
> On 10/26/19 1:36 AM, Per Liden wrote:
>> 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 
>>> 
>>> 
>>> 
>