RFR(XL): 8220310: Implementation: NUMA-Aware Memory Allocation for G1, Mutator (1/3)
Kim Barrett
kim.barrett at oracle.com
Mon Oct 7 18:10:42 UTC 2019
> 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.
------------------------------------------------------------------------------
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.
------------------------------------------------------------------------------
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.
------------------------------------------------------------------------------
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.
------------------------------------------------------------------------------
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.
------------------------------------------------------------------------------
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);
}
}
------------------------------------------------------------------------------
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.
------------------------------------------------------------------------------
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.
------------------------------------------------------------------------------
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.
------------------------------------------------------------------------------
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.
------------------------------------------------------------------------------
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).
------------------------------------------------------------------------------
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.
------------------------------------------------------------------------------
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.)
------------------------------------------------------------------------------
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.)
------------------------------------------------------------------------------
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.)
------------------------------------------------------------------------------
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.
------------------------------------------------------------------------------
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.
------------------------------------------------------------------------------
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).
------------------------------------------------------------------------------
More information about the hotspot-runtime-dev
mailing list