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

[sangheon.kim at oracle.com]

Hi Kim,

On 11/3/19 8:07 PM, Kim Barrett wrote:
>> 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.
Thanks for your review.

>
> ------------------------------------------------------------------------------
>
> 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.
Changed to have other name of 'print_statistics()'.

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

>
> ------------------------------------------------------------------------------
> src/hotspot/share/gc/g1/g1RegionCounts.hpp
>    28 #include "gc/g1/heapRegion.hpp"
>
> Unnecessary #include here; forward declaration of HeapRegion would be
> sufficient.
Done.

> ------------------------------------------------------------------------------
> 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.
Removed 'virtual'.
It was a base class before.

>
> ------------------------------------------------------------------------------
> 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?
Added G1NUMA::is_enabled().
Inside of G1NUMA::update_statistics() also checks is_enabled(), so I 
didn't add checking it before.

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

>
> ------------------------------------------------------------------------------
> src/hotspot/share/gc/g1/heapRegionSet.hpp
>   189                                             const uint requested_node_index);
>
> [pre-existing from earlier patch in this series?]
> Useless const qualifier.
Removed 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.
Moved into FreeRegionList.

I addressed all your comments but let me post the next webrev after 
reflecting Stefan's comment as well.

Thanks,
Sangheon


>
> ------------------------------------------------------------------------------
>