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

[Johan Sjölen]

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

Wow! Thank you for this Emanuel.

I went through your changes and I am happy with them. There are some spelling issues and my opinions on how to write the doc strings. I also asked for some "length"/"size" naming to be changed to "capacity", you don't have to do this as it's pre-existing, but it would make that code clearer.

src/hotspot/share/jfr/leakprofiler/chains/edgeStore.cpp line 287:

> 285:   assert(edge != nullptr, "invariant");
> 286:   if (_leak_context_edges == nullptr) {
> 287:     _leak_context_edges = new GrowableArrayCHeap<const StoredEdge*, mtTracing>(initial_size);

Pre-existing: `initial_capacity` is a better name.

src/hotspot/share/jfr/leakprofiler/checkpoint/objectSampleCheckpoint.cpp line 55:

> 53: template <typename T>
> 54: static GrowableArrayCHeap<T, mtTracing>* c_heap_allocate_array(int size = initial_array_size) {
> 55:   return new GrowableArrayCHeap<T, mtTracing>(size);

`capacity` instead of `size`

src/hotspot/share/jfr/recorder/checkpoint/types/jfrThreadGroup.cpp line 266:

> 264: 
> 265: JfrThreadGroup::JfrThreadGroup() :
> 266:   _list(new GrowableArrayCHeap<JfrThreadGroupEntry*, mtTracing>(initial_array_size)) {}

`capacity`

src/hotspot/share/jfr/recorder/jfrRecorder.cpp line 151:

> 149:   assert(length >= 1, "invariant");
> 150:   assert(dcmd_recordings_array == nullptr, "invariant");
> 151:   dcmd_recordings_array = new GrowableArrayCHeap<JfrStartFlightRecordingDCmd*, mtTracing>(length);

`capacity`

src/hotspot/share/jfr/support/jfrKlassUnloading.cpp line 38:

> 36: 
> 37: template <typename T>
> 38: static GrowableArrayCHeap<T, mtTracing>* c_heap_allocate_array(int size = initial_array_size) {

`capacity`

src/hotspot/share/utilities/growableArray.hpp line 618:

> 616: 
> 617: // The GrowableArray internal data is allocated from either:
> 618: //  - Resrouce area (default)

Spelling

src/hotspot/share/utilities/growableArray.hpp line 621:

> 619: //  - Arena
> 620: //
> 621: // Itself, it can be embedded, on stack, resource_arena or arena allocated.

"Itself can be allocated on stack, resource area or arena allocated."

src/hotspot/share/utilities/growableArray.hpp line 629:

> 627: // For C-Heap allocation use GrowableArrayCHeap.
> 628: //
> 629: // Note, that with GrowableArray does not deallocate the allocated memory from

"that the" not "that with"

src/hotspot/share/utilities/growableArray.hpp line 638:

> 636: // GrowableArray is copyable, but it only creates a shallow copy. Hence, one has
> 637: // to be careful not to duplicate the state and then diverge while sharing the
> 638: // underlying data.

Sad but true :-(

src/hotspot/share/utilities/growableArray.hpp line 644:

> 642:   friend class GrowableArrayWithAllocator<E, GrowableArray<E> >;
> 643: 
> 644:   // Since GrowableArray is arena / resource area allocated, it is a custom to

"it is a custom to" can basically be removed

"Since Growable array is arena/resource area allocated it does not destruct its elements. Therefore, ..." is sufficient.

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

PR Review: https://git.openjdk.org/jdk/pull/17160#pullrequestreview-1792708591
PR Review Comment: https://git.openjdk.org/jdk/pull/17160#discussion_r1433894629
PR Review Comment: https://git.openjdk.org/jdk/pull/17160#discussion_r1433894947
PR Review Comment: https://git.openjdk.org/jdk/pull/17160#discussion_r1433895133
PR Review Comment: https://git.openjdk.org/jdk/pull/17160#discussion_r1433895741
PR Review Comment: https://git.openjdk.org/jdk/pull/17160#discussion_r1433896219
PR Review Comment: https://git.openjdk.org/jdk/pull/17160#discussion_r1433901496
PR Review Comment: https://git.openjdk.org/jdk/pull/17160#discussion_r1433904262
PR Review Comment: https://git.openjdk.org/jdk/pull/17160#discussion_r1433916326
PR Review Comment: https://git.openjdk.org/jdk/pull/17160#discussion_r1433918209
PR Review Comment: https://git.openjdk.org/jdk/pull/17160#discussion_r1433920731