RFR: 8247755: Leaner and more versatile GrowableArray classes

[Kim Barrett]

> On Jun 17, 2020, at 10:16 AM, Stefan Karlsson <stefan.karlsson at oracle.com> wrote:
> 
> Hi all,
> 
> Please review and/or comment on this patch to create leaner and more versatile GrowableArray classes.
> 
> https://cr.openjdk.java.net/~stefank/8247755/webrev.01
> https://bugs.openjdk.java.net/browse/JDK-8247755
> 
> This is a proposal to rewrite the GrowableArray classes, with two main goals:
> 
> 1) Allow a GrowableArray instance to be allocated on the stack or embedded in other objects. This removes one of the two indirections when operating on the array elements.
> 
> 2) Make the GrowableArray instances smaller, so that there's no extra overhead when they are embedded in other objects.

Those are good goals.

In earlier email I wondered whether CRTP was the best approach for
this. Never mind, I'm okay with this approach. (I wondered whether the
different non-CRTP approach taken by BitMap would serve here, but I
think it doesn't. BitMap doesn't have operations that implicitly
perform allocations; all allocations are externally requested.)

------------------------------------------------------------------------------
src/hotspot/share/utilities/globalDefinitions.hpp
 435 // Need the correct linkage to call qsort without warnings
 436 extern "C" {
 437   typedef int (*_sort_Fn)(const void *, const void *);
 438 }

Rather than messing with function pointer types, why not use
Quicksort::sort from utilities/quickSort.hpp?

------------------------------------------------------------------------------
src/hotspot/share/memory/metaspaceShared.cpp
 459 static GrowableArrayCHeap<Handle, mtClassShared>* _extra_interned_strings = NULL;
...
 462   _extra_interned_strings = new GrowableArrayCHeap<Handle, mtClassShared>(10000);

[pre-existing]

I think this vector is only needed during the preload_and_dump
operation that calls read_extra_data, and can then be discarded. It
gets allocated by read_extra_data and put in a global variable and
left there as a leak, though maybe we don't care.

I was going to suggest it might be an on-stack Resource array over
some relevant scope in the caller and passed to read_extra_data , but
that won't work because of the ResourceMark in the read_extra_data
loop.  But with the proposed changes to GrowableArray it could be an
on-stack CHeap array.

------------------------------------------------------------------------------
src/hotspot/share/utilities/growableArray.hpp 
 115 class GrowableArrayView : public GrowableArrayBase {

I don't like the name "View" for this concept. Common practice
elsewhere (including the Standard) is for "view" types to be an
adaptor over some other type, providing a limited range or otherwise
adjusted access.  (Our BitMapView does that.)

GrowableArrayData?

I'm not sure how often this class name gets used by clients, so it
might not matter that it's a little bit long or clumsy / clunky.

------------------------------------------------------------------------------
src/hotspot/share/utilities/growableArray.hpp 

Destructors for all but the most-derived classes should be non-public.
Probably also the constructors.  That is, all of GrowableArrayBase,
GrowableArrayView, and GrowableArrayWithAllocator should have
protected constructors and destructors.

------------------------------------------------------------------------------
src/hotspot/share/utilities/growableArray.hpp 

[pre-existing]

GrowableArray's use of mtNone to indicate resource allocation seems
like an abuse of that memory type. That memory type has a description
string of "Unknown", nothing to do with resources. But it's considered
an invalid type by MamTracker/MemReporter/MallocSite, and it seems to
mean something else in the VirtualMemoryTracker, like "uninitialized".

aarch64 pf (debugger print frame) passes mtNone to NEW_C_HEAP_OBJ,
which seems like it shouldn't work, given the assert in MallocSite.

But why have constructors with optional MEMFLAGS at all?  And why use
mtNone that way?  Constructors without MEMFLAGS parameter are resource
allocating, constructors with required MEMFLAGS parameter are CHeap
allocating.  (Though the random status of mtNone noted above will make
that one probably not actually usable.)  (That would require some small
adjustments to GrowableArrayMetadata.)

And if mtNone is going to be used that way, then GrowableArrayCHeap
should static assert that it's flag value is not mtNone.

------------------------------------------------------------------------------
src/hotspot/share/utilities/growableArray.hpp 
 586 public:
 587   // Where are we going to allocate memory?
 588   bool on_C_heap() { return _metadata.on_C_heap(); }
 589   bool on_stack () { return _metadata.on_stack(); }
 590   bool on_arena () { return _metadata.on_arena(); }

These were previously protected; why make them public?

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