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 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
