RFR(XL): 8220310: Implementation: NUMA-Aware Memory Allocation for G1, Mutator (1/3)
sangheon.kim at oracle.com
sangheon.kim at oracle.com
Wed Oct 9 04:27:03 UTC 2019
Hi Kim,
On 10/7/19 11:10 AM, Kim Barrett wrote:
>> On Oct 1, 2019, at 12:43 PM, sangheon.kim at oracle.com wrote:
>> webrev:
>> http://cr.openjdk.java.net/~sangheki/8220310/webrev.2/
>> http://cr.openjdk.java.net/~sangheki/8220310/webrev.2.inc
>> Testing: hs-tier1 ~ 5 +-UseNUMA
> I like the direction of this. I think there are some additional simplifications possible
> around G1NUMA, which are discussed below.
>
> I still need to respond to your earlier individual responses. That will be in another email.
OK!
>
> ------------------------------------------------------------------------------
> src/hotspot/share/gc/g1/g1MemoryNodeManager.cpp
> 67 LINUX_ONLY(if (UseNUMA) {
>
> Maybe instead use #ifdef LINUX. Either way, add a trailing comment at
> the end of the conditional block.
Changed to use
#ifdef LINUX
>
> ------------------------------------------------------------------------------
> src/hotspot/share/gc/g1/g1NUMA.cpp
> 79 // If we don't have preferred numa id, touch the given area with round-robin manner.
>
> This comment seems out of place / obsolete.
OK
>
> ------------------------------------------------------------------------------
> src/hotspot/share/gc/g1/g1NUMA.cpp
> 138 uint region_index = G1CollectedHeap::heap()->addr_to_region(address);
>
> This requires the address be in the range reserved for the heap.
> That's okay; that's what we decided we want to do. But that should be
> part of the function's description, e.g. it should be mentioned as a
> precondition for prefered_index_for_address.
I think I addressed your point.
>
> ------------------------------------------------------------------------------
> src/hotspot/share/gc/g1/g1NUMA.hpp
> 87 // Returns numa id of the given numa index.
> 88 inline int numa_id_of_index(uint numa_index) const;
>
> Unused function.
Removed.
> ------------------------------------------------------------------------------
> src/hotspot/share/gc/g1/g1NUMA.hpp
> 83 inline uint index_of_numa_id(int numa_id) const;
>
> This function should be private. It is only needed in the
> implementation of index_of_current_thread and index_of_address.
> It should have a precondition that the argument is an active numa id,
> e.g. a definition something like
>
> uint G1NUMA::index_of_numa_id(int numa_id) const {
> assert(numa_id >= 0, "invalid numa id %d", numa_id);
> assert(numa_id < _len_numa_id_to_index_map, "invalid numa id %d", numa_id);
> uint numa_index = _numa_id_to_index_map[numa_id];
> assert(numa_index != G1MemoryNodeManager::InvalidNodeIndex,
> "invalid numa id %d", numa_id);
> return numa_index;
> }
>
> To make this work, index_of_address should also be changed, to
> something like:
>
> uint G1NUMA::index_of_address(HeapWord* address) const {
> int numa_id = os::numa_get_address_id((uintptr_t)address);
> if (numa_id == os::InvalidId) {
> return G1MemoryNodeManager::InvalidNodeIndex;
> } else {
> return index_of_numa_id(numa_id);
> }
> }
Changed as your patch.
> ------------------------------------------------------------------------------
> src/hotspot/share/gc/g1/g1NUMA.cpp
> 31 void G1NUMA::init_numa_id_to_index_map(const int* numa_ids, uint num_numa_ids) {
>
> This function is only called from one place, G1NUMA::initialize. The
> code would be simpler and more clear if the body of this function were
> just directly inlined into initialize and this function eliminated.
>
> And once that's done it becomes apparent that initialize could be
> hoisted into the (moved out of line) constructor.
>
> This also lets num_active_numa_ids just be a trivial accessor function
> in the header; there's no possibility of finding it uninitialized
> after the constructor returns, so no need for the assert that it has
> been set.
Changed as your patch.
>
> ------------------------------------------------------------------------------
> src/hotspot/share/gc/g1/g1NUMA.inline.hpp
> 32 inline bool G1NUMA::is_valid_numa_id(int numa_id) {
>
> Only called by init_numa_to_index_map in a guarantee that would be
> more obviously vacuous after the earlier suggested merge of that
> function into initialize.
Done.
>
> ------------------------------------------------------------------------------
> src/hotspot/share/runtime/os.hpp
> 393 enum NumaIdState {
> 394 InvalidId = -1,
> 395 AnyId = -2
> 396 };
>
> The type NumaIdState is unused.
> The AnyId enumerator is unused.
>
> Suggest making InvalidId just a static const int in the class.
Changed to static const int.
>
> ------------------------------------------------------------------------------
> src/hotspot/share/runtime/os.hpp
> 398 static int numa_get_address_id(uintptr_t address);
>
> Why is the type of address uintptr_t rather than a pointer type?
>
> I see that the underlying Linux syscall (get_mempolicy) wants an
> unsigned long, but that detail ought to be isolated to the Linux
> implementation layer. Callers are going to want to pass in addresses
> (pointers) and should not need to cast. That cast should happen at
> the point where the syscall is being made.
Changed to void*.
>
> ------------------------------------------------------------------------------
> src/hotspot/share/gc/g1/g1Allocator.inline.hpp
> 37 inline MutatorAllocRegion* G1Allocator::mutator_alloc_region(uint node_index) {
> 38 assert(_g1h->mem_node_mgr()->is_valid_node_index(node_index), "Invariant, index %u", node_index);
> 39 return &_mutator_alloc_regions[node_index];
> 40 }
>
> I think the assert here should be that node_index < _num_alloc_regions.
>
> is_valid_node_index gives a somewhat indirect (so weak) check of the
> validity of the array access.
>
> Such a change would also eliminate one of the two callers of
> is_valid_node_index, which I think can be eliminated (see next comment).
Done.
>
> ------------------------------------------------------------------------------
> src/hotspot/share/gc/g1/heapRegionManager.cpp
> 126 HeapRegion* HeapRegionManager::allocate_free_region(HeapRegionType type, uint requested_node_index) {
> ...
> 131 if (mgr->num_active_nodes() > 1 && mgr->is_valid_node_index(requested_node_index)) {
>
> I think a better test here would be
> if ((requested_node_index != G1MemoryNodeManager::AnyNodeIndex) &&
> (mgr->num_active_nodes() > 1)) {
>
> This eliminates one of two calls to is_valid_node_index (which I think
> can be eliminated, see previous comment). And callers should not be
> passing in actually invalid indices. I think there are asserts lower
> down in the stack (in G1NUMA) to complain about such, but they
> shouldn't be getting in here anyway.
Done, but introduced G1MemoryNodeManager::has_multi_node() which is
Thomas' comment.
>
> ------------------------------------------------------------------------------
> src/hotspot/share/gc/g1/g1MemoryNodeManager.hpp
> 42 static const uint InvalidNodeIndex = UINT_MAX;
> 43 static const uint AnyNodeIndex = InvalidNodeIndex - 1;
>
> These seem misplaced to me. Shouldn't they be in G1NUMA? Possibly
> reexported here for convenience? (Assuming it actually is convenient.)
Yes, for convenience.
But G1NUMA is merged into G1MemoryNodeManager so no more argue here.
>
> ------------------------------------------------------------------------------
> src/hotspot/share/gc/g1/g1MemoryNodeManager.hpp
> 42 static const uint InvalidNodeIndex = UINT_MAX;
>
> I think the only place this arises is as the result of
> index_of_address when the numa id for the location isn't known. Which
> suggests the name should be "UnknownNodeIndex" rather than
> "InvalidNodeIndex". And the description of index_of_address should
> mention that it can return that value (whatever its name ends up being.)
Good idea.
Changed to UnknownNodeIndex.
>
> ------------------------------------------------------------------------------
> src/hotspot/share/gc/g1/g1MemoryNodeManager.hpp
>
> I'm not sure G1MemoryNodeManager is useful. It seems to be just a thin
> wrapper over the G1NUMA API, with a virtual dispatch between a
> non-NUMA or single-node implementation and the multi-node
> implementation that uses a G1NUMA that is only created for multi-node
> support. The virtual dispatch can't be eliminated in most (all or
> nearly all?) cases.
>
> But I think most of the single-node implementation would just fall out
> as a 1-node boundary case for multi-node G1MemoryNodeManager / G1NUMA.
>
> So I think this might all be collapsed down to a G1NUMA that always
> exists. If there are any places that require actual distinction, that
> class can have a private member to select the appropriate behavior.
> (Or maybe it's just the number of active nodes.)
G1NUMA is merged to G1MemoryNodeManager.
Previously G1MNM owned G1NUMA so I tried to keep this relation. Now
G1MultiMemoryNodeManager has NUMA related implementations.
Thomas also suggested merging these two.
We discussed about virtual dispatch stuff, but I couldn't find anything
better than now.
More than welcome if you have any suggestion. Or keep for later
enhancements.
>
> ------------------------------------------------------------------------------
> src/hotspot/share/gc/g1/g1NUMA.inline.hpp
>
> I think that with the changes I've proposed above, I think there's not
> much left in this file, and it might not be worth having it. Consider
> moving any lingering remnents to the .hpp or .cpp file as appropriate.
Removed g1NUMA.inline.hpp
>
> ------------------------------------------------------------------------------
> src/hotspot/share/gc/g1/g1NUMA.hpp
>
> Consider adding a page_size() accessor function (private for now) that
> asserts the associated data member is > 0 (e.g. initialized), since it
> is initialized after construction. Use that instead of direct uses of
> the data member.
Added page_size().
>
> ------------------------------------------------------------------------------
> src/hotspot/share/runtime/arguments.cpp
> 4108 // such as Parallel GC for Linux and Solaris or G1 GC for Linux will
> ...
> 4111 // Non NUMA-aware collectors such as CMS and Serial-GC on
> 4112 // all platforms and ParallelGC on Windows will interleave all
>
> I think that these comments about which configurations do or don't
> support NUMA are just a maintenance headache. I think it would be
> better here to just say
>
> NUMA-aware collectors will interleave ...
> Non NUMA-aware collectors will interleave ...
>
> And leave out mentions of configurations that may change (as is being
> done here) or be removed (as soon expected for CMS).
I just removed mentioning of configurations.
// UseNUMAInterleaving is set to ON for all collectors and
platforms when
// UseNUMA is set to ON. NUMA-aware collectors will interleave old
gen and
// survivor spaces on top of NUMA allocation policy for the eden space.
// Non NUMA-aware collectors will interleave all of the heap spaces
across
// NUMA nodes.
Here's the major change list at the webrev. Or arguable list :)
1) Verification at HRM::allocate_free_region() is removed and it will be
added somewhere at safepoint by JDK-8220312 (3/3 which is part of this
JEP). Probably at the end of young gc?
2) Node id printing is changed. Removed old one and added at
HeapRegion::print_on() with new column. Node id is only printed when
UseNUMA is enabled and gc+heap+region=trace. If there's single active
node, it will print the node id and this is intentional. Another
approach would be printing only if there are multiple nodes.
3) If AlwaysPreTouch is enabled, HeapRegion will have actual node index
instead of preferred node index.
4) HeapRegion::_node_index is set at HRM::make_regions_available() as
there is the only place initializing HeapRegion. Another approach would
be setting the index at HeapRegion::initialize(we have to pollute HR
with G1MNM stuff) or conditionally(*) setting the index at
HeapRegion::node_index(). (*) if the index is unknown etc..
5) G1NUMA class is merged into G1MemoryNodeManager.
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
Thanks,
Sangheon
>
> ------------------------------------------------------------------------------
>
More information about the hotspot-gc-dev
mailing list