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

[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
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
>>
>>
>>