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

[Kim Barrett]

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

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

------------------------------------------------------------------------------

Other than that, looks good to me.  I don't need another webrev for
a fix to that comment.