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

eme64 · 2023-12-19T16:59:05Z

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

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Integration blockers

⚠️ Too few reviewers with at least role reviewer found (have 0, need at least 1) (failed with updated jcheck configuration in pull request)
⚠️ Whitespace errors (failed with updated jcheck configuration in pull request)

Issue

JDK-8322476: Remove GrowableArray C-Heap version, replace usages with GrowableArrayCHeap (Enhancement - P4)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/17160/head:pull/17160
$ git checkout pull/17160

Update a local copy of the PR:
$ git checkout pull/17160
$ git pull https://git.openjdk.org/jdk.git pull/17160/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 17160

View PR using the GUI difftool:
$ git pr show -t 17160

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/17160.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2023-12-19T17:00:07Z

👋 Welcome back epeter! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2023-12-19T17:01:09Z

@eme64 The following labels will be automatically applied to this pull request:

graal
hotspot
serviceability

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

mlbridge · 2023-12-20T12:57:09Z

Webrevs

00: Full (68baaf3e)

kimbarrett

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.

kimbarrett · 2023-12-20T13:44:44Z

src/hotspot/share/cds/dumpTimeClassInfo.hpp

 private:
  template <typename T>
-  static int array_length_or_zero(GrowableArray<T>* array) {
+  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.

kimbarrett · 2023-12-20T19:36:07Z

src/hotspot/share/cds/metaspaceShared.cpp

-  void dump_java_heap_objects(GrowableArray<Klass*>* klasses) NOT_CDS_JAVA_HEAP_RETURN;
-  void dump_shared_symbol_table(GrowableArray<Symbol*>* symbols) {
+  void dump_java_heap_objects(GrowableArrayCHeap<Klass*, mtClassShared>* klasses) NOT_CDS_JAVA_HEAP_RETURN;
+  void dump_shared_symbol_table(GrowableArrayView<Symbol*>* symbols) {


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

kimbarrett · 2023-12-20T19:37:52Z

src/hotspot/share/cds/metaspaceShared.cpp


 #if INCLUDE_CDS_JAVA_HEAP
-void VM_PopulateDumpSharedSpace::dump_java_heap_objects(GrowableArray<Klass*>* klasses) {
+void VM_PopulateDumpSharedSpace::dump_java_heap_objects(GrowableArrayCHeap<Klass*, mtClassShared>* klasses) {


pre-existing: Perhaps the argument should be const.

pre-existing and can't attach comment to line#50: int i; is dead variable.

kimbarrett · 2023-12-20T19:58:09Z

src/hotspot/share/classfile/compactHashtable.cpp


  _num_entries_written = 0;
-  _buckets = NEW_C_HEAP_ARRAY(GrowableArray<Entry>*, _num_buckets, mtSymbol);
+  _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>.

kimbarrett · 2023-12-20T20:02:16Z

src/hotspot/share/classfile/javaClasses.cpp

      int init_length = 64;
-      _methods = new (mtInternal) GrowableArray<Method*>(init_length, mtInternal);
-      _bcis = new (mtInternal) GrowableArray<int>(init_length, mtInternal);
+      _methods = new GrowableArrayCHeap<Method*, mtInternal>(init_length);


Consider renaming init_length => init_capacity.

kimbarrett · 2023-12-20T20:11:11Z

src/hotspot/share/code/codeCache.hpp

-  static GrowableArray<CodeHeap*>* _compiled_heaps;
-  static GrowableArray<CodeHeap*>* _nmethod_heaps;
-  static GrowableArray<CodeHeap*>* _allocable_heaps;
+  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.

kimbarrett · 2023-12-20T20:35:05Z

src/hotspot/share/memory/arena.hpp

  NEW_ARENA_ARRAY(arena, type, 1)

+#ifdef ASSERT
+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.

@kimbarrett the reason was that I need to call this from the hpp file, and I encountered some circular dependency I did could not resolve. So I needed to move something off to the cpp files. Either I put it in arena.cpp, or in growableArray.cpp. But If I put things off to growableArray.cpp from the GrowableArray class, then it will not easily instantiate the templates, so that is not possible then. Hence I have to put it into arena.cpp

I don't think a global API is warranted to support that local debug-only
implementation detail.

The inclusion problem arises because the PR eliminates GrowableArrayMetadata
(a non-templated class), thereby forcing the init_checks helper to be moved to
GrowableArray (a class template). That forced moving the implementation from
the .cpp to the .hpp.

One solution might be to move the new version of init_checks to
growableArray.inline.hpp. After all, breaking circularities is one of the
reasons one might have an inline.hpp file. However, that file doesn't
currently exist, and I think introducing it would have too much fannout. And
it might not even work without even more effort; there might be .hpp files
that contain allocations of GrowableArray.

Much simpler, and I think probably better, is to keep GrowableArrayMetadata
(perhaps under a different name - GrowableArrayArenaHolder?), now holding the
allocation arena and providing init_checks (and anything else that seems
appropriate), with init_checks defined similarly to the current definition in
the .cpp file.

kimbarrett · 2023-12-20T20:39:27Z

src/hotspot/share/memory/heapInspection.cpp

 KlassInfoHisto::KlassInfoHisto(KlassInfoTable* cit) :
  _cit(cit) {
-  _elements = new (mtServiceability) GrowableArray<KlassInfoEntry*>(_histo_initial_size, mtServiceability);
+  _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.

Can name be changed to _histo_initial_capacity?

eme64 · 2023-12-21T06:43:26Z

@kimbarrett Thanks for looking at the PR!
I see you address a lot of "pre-existing" issues. And you would like GrowableArrayCHeap not have the MEMFLAGS in the template argument but maybe as a constructor argument instead. Or maybe a GACH version that only allocates once, though I guess that would limit what kinds of methods you could call on it...
Can we address these issues as separate RFE's?

jdksjolen

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.

jdksjolen · 2023-12-21T10:39:58Z

src/hotspot/share/jfr/leakprofiler/chains/edgeStore.cpp

  assert(edge != nullptr, "invariant");
  if (_leak_context_edges == nullptr) {
-    _leak_context_edges = new (mtTracing) GrowableArray<const StoredEdge*>(initial_size, mtTracing);
+    _leak_context_edges = new GrowableArrayCHeap<const StoredEdge*, mtTracing>(initial_size);


Pre-existing: initial_capacity is a better name.

jdksjolen · 2023-12-21T10:40:16Z

src/hotspot/share/jfr/leakprofiler/checkpoint/objectSampleCheckpoint.cpp

-static GrowableArray<T>* c_heap_allocate_array(int size = initial_array_size) {
-  return new (mtTracing) GrowableArray<T>(size, mtTracing);
+static GrowableArrayCHeap<T, mtTracing>* c_heap_allocate_array(int size = initial_array_size) {
+  return new GrowableArrayCHeap<T, mtTracing>(size);


capacity instead of size

jdksjolen · 2023-12-21T10:40:29Z

src/hotspot/share/jfr/recorder/checkpoint/types/jfrThreadGroup.cpp


 JfrThreadGroup::JfrThreadGroup() :
-  _list(new (mtTracing) GrowableArray<JfrThreadGroupEntry*>(initial_array_size, mtTracing)) {}
+  _list(new GrowableArrayCHeap<JfrThreadGroupEntry*, mtTracing>(initial_array_size)) {}


jdksjolen · 2023-12-21T10:40:48Z

src/hotspot/share/jfr/recorder/jfrRecorder.cpp

  assert(length >= 1, "invariant");
  assert(dcmd_recordings_array == nullptr, "invariant");
-  dcmd_recordings_array = new (mtTracing) GrowableArray<JfrStartFlightRecordingDCmd*>(length, mtTracing);
+  dcmd_recordings_array = new GrowableArrayCHeap<JfrStartFlightRecordingDCmd*, mtTracing>(length);


jdksjolen · 2023-12-21T10:41:03Z

src/hotspot/share/jfr/support/jfrKlassUnloading.cpp

 template <typename T>
-static GrowableArray<T>* c_heap_allocate_array(int size = initial_array_size) {
-  return new (mtTracing) GrowableArray<T>(size, mtTracing);
+static GrowableArrayCHeap<T, mtTracing>* c_heap_allocate_array(int size = initial_array_size) {


jdksjolen · 2023-12-21T10:45:57Z