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

[sangheon.kim at oracle.com]

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);


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


>