On Tue, 19 Dec 2023 16:59:05 GMT, Emanuel Peter <epe...@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
