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

[sangheon.kim at oracle.com]

Hi Kim,

On 10/11/19 1:12 PM, Kim Barrett wrote:
>> On Oct 11, 2019, at 2:30 PM, Kim Barrett <kim.barrett at oracle.com> wrote:
>>
>>>> src/hotspot/share/gc/g1/heapRegion.cpp
>>>> 464     st->print("|Node ID %02d", node_ids[this->node_index()]);
>>>>
>>>> The unchecked use of node_index() here can run afoul of an unset (so
>>>> UnknownNodeIndex) index.
>>> Added such checking.
>>>> Also, no need for `this->` in `this->node_index()`.
>>> Removed.
>>> I'm aware but tried to follow local style which uses 'this->' in that code.
>> There is one other use of this-> in that function (and one additional one in the whole file).
>> The *vast* majority of accesses use the implicit this.  So I wouldn’t describe that as the
>> local style, rather a couple of weirdnesses that probably should be cleaned up.
> Looks like this has been fixed in the latest version.
Yes

>
>>> webrev:
>>> http://cr.openjdk.java.net/~sangheki/8220310/webrev.4
>>> http://cr.openjdk.java.net/~sangheki/8220310/webrev.4.inc
>>>
>>> Testing: hs-tier 1 ~ 5 with/without UseNUMA
>> I’ve started looking at the new webrev.  Looking good, and no comments yet, but not done yet either.
> The only other thing I found was this:
>
> ------------------------------------------------------------------------------
> src/hotspot/share/gc/g1/g1NUMA.hpp
>    85   // Print current active memory node count.
>    86   uint num_active_nodes() const;
>   
> "Print"?  Also, "current"? It doesn't change, I think.
Okay, changed 'Returns active memory node count'.

>
> ------------------------------------------------------------------------------
>
> Other than that, looks good to me.  I don't need another webrev for
> a fix to that comment.
Nice to hear!
Many thanks for your thorough all reviews.

Thanks,
Sangheon