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

[sangheon.kim at oracle.com]

Hi Kim,

On 10/10/19 4:34 PM, Kim Barrett wrote:
>> On Oct 9, 2019, at 12:27 AM, sangheon.kim at oracle.com wrote:
>> Webrev:
>> http://cr.openjdk.java.net/~sangheki/8220310/webrev.3
>> http://cr.openjdk.java.net/~sangheki/8220310/webrev.3.inc
>> Testing: hs-tier 1~5, with/without UseNUMA
> I agree with Stefan and Thomas; this is looking pretty good.
:)

>
> There are some naming issues that I'm not going to comment on here.
> Stefan has already commented on some, and a bit of offline discussion
> suggests there's a larger naming discussion needed, but which can
> follow getting the functionality we want.
>
> There has been further discission offline toward collapsing
> G1MemoryNodeManager to one class without virtual dispatch, and using
> G1NUMA name. I won't bother to re-iterate any of that here.
Okay.

> ------------------------------------------------------------------------------
> src/hotspot/share/gc/g1/g1Allocator.cpp
>   186   assert(Heap_lock->owner() != NULL, "Should be owned on this thread's behalf.");
>
> Use assert_lock_strong(Heap_lock).
It didn't work.
assert_lock_string() checks "lock->owned_by_self()" which is not 
equivalent to "lock::owner() != NULL". Am I missing something?

Since this is pre-existing code, I would like to leave as is.

>
> ------------------------------------------------------------------------------
> src/hotspot/share/gc/g1/g1RegionToSpaceMapper.cpp
>    82       _storage.request_memory_on_node(page, _pages_per_region, node_index);
> ...
>   153         _storage.request_memory_on_node(idx, 1, node_index);
>
> I'm not sure request_memory_on_node belongs on the _storage object.
> The current implementation just has the storage object (conditionally)
> forward the request to the memory node manager object. These places in
> the space mapper could just make the calls on the memory node manager
> object directly (it is already being used nearby).  And these places
> don't need the conditionalization.
>
> I think making the space mapper directly call the memory node manager
> here would remove the need for the proposed changes to the virtual
> space class.
Fixed to directly call G1NUMA::request_memory_on_node() (previously 
G1MemoryNodeManager).
But G1NUMA can't calculate raw address, so I had to add base address at 
G1NUMA to get that.

When I implemented it, I had similar opinion (not good fit for _storage) 
but I also wanted to avoid adding extra dependency at G1NUMA. But anyway 
I realized we can achieve it easily if we have base address.

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

>
> ------------------------------------------------------------------------------
> src/hotspot/share/gc/g1/g1MemoryNodeManager.hpp
>    81   virtual const uint max_search_depth() const { return 1; }
>
> s/const uint/uint/
>
> Similarly for other declarations and definitions.
Done.

>
> ------------------------------------------------------------------------------
> src/hotspot/share/gc/g1/g1MemoryNodeManager.hpp
>    77   virtual void request_memory_on_node(char* aligned_address, size_t size_in_bytes, uint node_index) { }
>
> Shouldn't the aligned_address argument be typed "void*" rather than "char*"?
The signature of that method changed to page based and newly added 
member is void*.
i.e. G1NUMA, void* _base_address
But eventually we need char* to call numa_make_local(char*, , ).

>
> ------------------------------------------------------------------------------
> src/hotspot/share/gc/g1/heapRegionManager.cpp
>   112   if (mgr->has_multi_nodes() && requested_node_index != G1MemoryNodeManager::AnyNodeIndex) {
>
> I think it would be better to test the requested_node_index value
> first.  The "any" case is a common case.
Done

>
> ------------------------------------------------------------------------------
> src/hotspot/share/gc/g1/heapRegionManager.cpp
>   200   if(AlwaysPreTouch) {
>
> Add space after "if".
Done

>
> ------------------------------------------------------------------------------
> src/hotspot/share/gc/g1/heapRegionManager.cpp
>   311   return region_node_index == preferred_node_index;
>
> Fix indentation.
Done

>
> ------------------------------------------------------------------------------
> src/hotspot/share/runtime/os.hpp
>   393   static const int InvalidId = -1;
>
> This should probably be "InvalidNUMAId" or something like that.
Changed to InvalidNUMAId.

FYI, I filed JDK-8232156 for further investigation of initialization 
order related to G1NUMA. i.e. about removing G1NUMA::set_region_info().

New webrev includes:
1. Addressed most comments from Kim, Stefan and Thomas.
2. Rename G1MemoryNodeManager to G1NUMA with removing virtual calls.

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

Thanks,
Sangheon


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