RFR: 8286739: Refactor FreeListConfig

[Kim Barrett]

On Fri, 13 May 2022 14:45:25 GMT, Albert Mingkun Yang <ayang at openjdk.org> wrote:

> This PR consists of two commits:
> 
> 1 is about making `FreeListConfig` an "interface" of `allocate/deallocate`.
> 2 is about renaming `FreeListConfig -> MemAllocator`.
> 
> Test: hotspot_gc

The bug report asks a reasonable question about the naming and/or structure of
G1SegmentedArray, but then seems to go off in what to me seems like a strange
direction to address that question.

I think the existing FreeListAllocator / FreeListConfig abstraction is fine.
The config object is not a general allocator, and should not be renamed to
suggest that it is.

I'm not sure G1SegmentedArray should be deriving from FreeListConfig. If it
should, then it's poorly named. It has it's own internal free-list that it
manages differently from FreeListAllocator. I think it used to use a copy of
what became FreeListAllocator. What appears to be a lingering artifact of that
is that it wraps the allocation from the free-list in a
GlobalCounter::CriticalSection, but there's no corresponding synchronize (at
least not that I could find). I think that's because the allocation from and
release to that free-list occur in disjoint phases (so there's no ABA issue),
so the critical section isn't needed. (Althernatively, they do overlap and the
lack of synchronize is a bug, but I think the former is the case, or the
synchronize exists but I didn't find it.)

Ivan has been working on refactoring and cleanup in this area. I think that
work is still in progress. I also think the change being proposed here is in a
very different and possibly incompatible direction from what's planned for
that in-progress work.

src/hotspot/share/gc/shared/freeListAllocator.hpp line 34:

> 32: #include "utilities/lockFreeStack.hpp"
> 33: 
> 34: class AbstractAllocator {

I think this naming is a mistake - the name suggests this could/should be used as a base for any allocator, but the `allocate()` function is not useful to most.  It's only appropriate for fixed-size chunk allocators.  I prefer the existing FreeList / FreeListConfig to what's being proposed here.

src/hotspot/share/gc/shared/freeListAllocator.hpp line 125:

> 123:   FreeListAllocator(const char* name,
> 124:                     AbstractAllocator* allocator,
> 125:                     size_t transfer_threshold = 10);

I don't see the rationale for having some configuration parts directly in the allocator (like transfer_threshold) and others (like buffer_size) in the underlying config/allocator.  I'd rather they were all in the config object.

src/hotspot/share/gc/shared/ptrQueue.hpp line 167:

> 165:   friend class TestSupport;
> 166: 
> 167:   class MemAllocator : public AbstractAllocator {

Another overly generic name.  I think the existing naming is better.

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

PR: https://git.openjdk.java.net/jdk/pull/8704