RFR: 8322476: Remove GrowableArray C-Heap version, replace usages with GrowableArrayCHeap

[Kim Barrett]

On Tue, 19 Dec 2023 16:59:05 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

> [JDK-8247755](https://bugs.openjdk.org/browse/JDK-8247755) introduced the `GrowableArrayCHeap`. This duplicates the current C-Heap allocation capability in `GrowableArray`. I now remove that from `GrowableArray` and move all usages to `GrowableArrayCHeap`.
> 
> This has a few advantages:
> - Clear separation between arena (and resource area) allocating array and C-heap allocating array.
> - We can prevent assigning / copying between arrays of different allocation strategies already at compile time, and not only with asserts at runtime.
> - We should not have multiple implementations of the same thing (C-Heap backed array).
> - `GrowableArrayCHeap` is NONCOPYABLE. This is a nice restriction, we now know that C-Heap backed arrays do not get copied unknowingly.
> 
> **Bonus**
> We can now restrict `GrowableArray` element type `E` to be `std::is_trivially_destructible<E>::value == true`. The idea is that arena / resource allocated arrays get abandoned, often without being even cleared. Hence, the elements in the array are never destructed. But if we only use elements that are trivially destructible, then it makes no difference if the destructors are ever called, or the elements simply abandoned.
> 
> For `GrowableArrayCHeap`, we expect that the user eventually calls the destructor for the array, which in turn calls the destructors of the remaining elements. Hence, it is up to the user to ensure the cleanup. And so we can allow non-trivial destructors.
> 
> **Testing**
> Tier1-3 + stress testing: pending

pre-existing: There are a lot of non-static class data members that are pointers to
GrowableArray that seem like they would be better as direct, e.g. non-pointers. 

pre-existing: There are a lot of iterations over GrowableArray's that would be
simplified by using range-based-for.

I'm not a fan of the additional clutter in APIs that the static memory types add.
If we had a variant of GrowableArrayCHeap that was not itself dynamically allocatable
and took a memory type to use internally as a constructor argument, then I think a
lot of that clutter could be eliminated.  It could be used for ordinary data members
that are direct GAs rather than pointers to GAs.  I think there is a way to do something
similar for static data members that are pointers that are dynamically allocated later,
though that probably requires more work.

I've not yet reviewed the changes to growableArray.[ch]pp yet, nor the test changes.
But I've run out of time and energy for this for today.

src/hotspot/share/cds/dumpTimeClassInfo.hpp line 162:

> 160: private:
> 161:   template <typename T>
> 162:   static int array_length_or_zero(GrowableArrayCHeap<T, mtClass>* array) {

Argument could be `GrowableArrayView<T>*`, removing the coupling on the memory type.

Also, pre-existing: the argument should be const.

src/hotspot/share/cds/metaspaceShared.cpp line 441:

> 439: 
> 440:   void dump_java_heap_objects(GrowableArrayCHeap<Klass*, mtClassShared>* klasses) NOT_CDS_JAVA_HEAP_RETURN;
> 441:   void dump_shared_symbol_table(GrowableArrayView<Symbol*>* symbols) {

pre-existing: Perhaps the arguments to these should be const.

src/hotspot/share/cds/metaspaceShared.cpp line 840:

> 838: 
> 839: #if INCLUDE_CDS_JAVA_HEAP
> 840: void VM_PopulateDumpSharedSpace::dump_java_heap_objects(GrowableArrayCHeap<Klass*, mtClassShared>* klasses) {

pre-existing: Perhaps the argument should be const.

src/hotspot/share/classfile/compactHashtable.cpp line 54:

> 52: 
> 53:   _num_entries_written = 0;
> 54:   _buckets = NEW_C_HEAP_ARRAY(EntryBucket*, _num_buckets, mtSymbol);

pre-existing: It seems like the code could be simpler if the type of _buckets was
GrowableArrayCHeap<EntryBucket*, mtSymbol>.

src/hotspot/share/classfile/javaClasses.cpp line 1824:

> 1822:       // Pick minimum length that will cover most cases
> 1823:       int init_length = 64;
> 1824:       _methods = new GrowableArrayCHeap<Method*, mtInternal>(init_length);

Consider renaming init_length => init_capacity.

src/hotspot/share/code/codeCache.hpp line 92:

> 90:  private:
> 91:   // CodeHeaps of the cache
> 92:   typedef GrowableArrayCHeap<CodeHeap*, mtCode> CodeHeapArray;

pre-existing: Consider moving CodeHeapArray to namespace scope and prefer using it to the long-form.
If not at namespace scope, it could at least be public in this class and used throughout, including in public
APIs.

src/hotspot/share/memory/arena.hpp line 209:

> 207: 
> 208: #ifdef ASSERT
> 209: bool Arena_contains(const Arena* arena, const void* ptr);

This function doesn't seem necessary.  Directly calling arena->contains(ptr) in the one place it's being seems
like it should suffice.

src/hotspot/share/memory/heapInspection.cpp line 282:

> 280: KlassInfoHisto::KlassInfoHisto(KlassInfoTable* cit) :
> 281:   _cit(cit) {
> 282:   _elements = new GrowableArrayCHeap<KlassInfoEntry*, mtServiceability>(_histo_initial_size);

pre-existing: Why is this initialization separate from the ctor-initializer?  And this looks like an example of
where it would be better as a direct GA member rather than a pointer to GA.

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

Changes requested by kbarrett (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17160#pullrequestreview-1790925376
PR Review Comment: https://git.openjdk.org/jdk/pull/17160#discussion_r1432733463
PR Review Comment: https://git.openjdk.org/jdk/pull/17160#discussion_r1433109448
PR Review Comment: https://git.openjdk.org/jdk/pull/17160#discussion_r1433110835
PR Review Comment: https://git.openjdk.org/jdk/pull/17160#discussion_r1433129906
PR Review Comment: https://git.openjdk.org/jdk/pull/17160#discussion_r1433133733
PR Review Comment: https://git.openjdk.org/jdk/pull/17160#discussion_r1433140564
PR Review Comment: https://git.openjdk.org/jdk/pull/17160#discussion_r1433160909
PR Review Comment: https://git.openjdk.org/jdk/pull/17160#discussion_r1433164132