RFR(L): 8220312: Implementation: NUMA-Aware Memory Allocation for G1, Logging (3/3)

[Kim Barrett]

> On Nov 2, 2019, at 2:08 AM, sangheon.kim at oracle.com wrote:
> 
> Here's the webrev:
> http://cr.openjdk.java.net/~sangheki/8220312/webrev.4
> http://cr.openjdk.java.net/~sangheki/8220312/webrev.4.inc
> http://cr.openjdk.java.net/~sangheki/8220312/webrev.4.inc.numa_move_pages
> 
> Testing: hs-tier 1 ~ 5 with / without UseNUMA.

I didn't spend much time looking at the actual logging output; Thomas
and Stefan have both given that a pretty thorough look.  Instead I
looked for more structural things.

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

There are several functions named "print()" which I think should have
some other name.  The classes involved (G1NUMA, G1NUMAStats, maybe
other that I didn't check for) are derived from CHeapObj<>.  In a
non-product build, CHeapObj<> is derived from AllocatedObj, which
provides a (non-virtual) "print()" function that calls the virtual
"print_on()" function.

By giving these classes their own print(), this change is overriding
this existing public API in non-product builds only. While overriding
a public non-virtual function is permitted, it is usually denigrated.
And having different overriding behavior based on build type seems
particularly confusing, especially since one is about logging and the
other isn't.

------------------------------------------------------------------------------
src/hotspot/share/gc/g1/g1ParScanThreadState.inline.hpp
 241       memset((void*)_obj_alloc_stat, 0, sizeof(size_t) * num_nodes);

Unnecessary cast to void*.

------------------------------------------------------------------------------
src/hotspot/share/gc/g1/g1RegionCounts.hpp 
  28 #include "gc/g1/heapRegion.hpp"

Unnecessary #include here; forward declaration of HeapRegion would be
sufficient.

------------------------------------------------------------------------------
src/hotspot/share/gc/g1/g1RegionCounts.hpp 

Why are add, clear, and length virtual?  This class doesn't seem to be
used as a base class, and isn't overriding anything from it's base.

If it were a base class, then the destructor should not be public and
non-virtual.

------------------------------------------------------------------------------
src/hotspot/share/gc/g1/g1RegionCounts.hpp 
  34 class G1RegionCounts : public StackObj {

The name of this class seems very generic and isn't at all suggestive
of it having anything at all to do with NUMA.

------------------------------------------------------------------------------
src/hotspot/share/gc/g1/heapRegionManager.cpp
 128     if (hr->node_index() < numa->num_active_nodes()) {
 129       numa->update_statistics(G1NUMAStats::NewRegionAlloc, requested_node_index, hr->node_index());
 130     }

Should we be doing this if NUMA disabled?

------------------------------------------------------------------------------
src/hotspot/share/gc/g1/heapRegionSet.cpp
 323 FreeRegionList::FreeRegionList(const char* name, HeapRegionSetChecker* checker):
 324                                HeapRegionSetBase(name, checker),
 325                                _node_info(G1NUMA::numa()->is_enabled() ? new NodeInfo() : NULL) {

Unusual indentatio of initializer-list.

------------------------------------------------------------------------------
src/hotspot/share/gc/g1/heapRegionSet.hpp
 189                                             const uint requested_node_index);

[pre-existing from earlier patch in this series?]
Useless const qualifier.

------------------------------------------------------------------------------
src/hotspot/share/gc/g1/heapRegionSet.hpp
 246 class NodeInfo : public CHeapObj<mtGC> {

NodeInfo is an overly generic name for the global namespace.

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