RFR: 8247755: Leaner and more versatile GrowableArray classes

[Kim Barrett]

> On Jun 22, 2020, at 5:01 AM, Stefan Karlsson <stefan.karlsson at oracle.com> wrote:
> 
> On 2020-06-22 09:25, Kim Barrett wrote:
>> 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?
> 
> I didn't change this code, only moved it. This was done to reduce the clutter in growableArray.hpp. You don't suggest that I change this in this RFR, right?

I missed that this was moved from growableArray.hpp, having been in
one of the largish deleted chunks there.

So there's a bit of a mess here, none of it related to your change.
For example,

./share/oops/instanceKlass.cpp:    // _sort_Fn is defined in growableArray.hpp.

And worse, the semantics of extern "C" includes things like possibly
using different calling conventions. So casting a C++ function to
_sort_Fn seems technically broken, though apparently happens to work
on supported platforms.  (In which case it would seem to effectively
be a nop.)

All of that could be eliminated by using Quicksort::sort (or
std::sort, but I don't want to get into that here).

I've filed a bug:
https://bugs.openjdk.java.net/browse/JDK-8248137

>> 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. […]
> 
> Yes. I mainly wanted to use this as an example of a CHeap/CHeap-allocated GrowableArray. I also think it would make sense to change this to n on-stack CHeap array.

OK.  (Did I see a bug for cleaning this up go by?)

>> 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 >
> 
> My thinking was the just like the with the BitMapView, the backing storage is managed externally:
> 
> // The BitMapView is used when the backing storage is managed externally.
> class BitMapView : public BitMap {
> public:
>  BitMapView() : BitMap(NULL, 0) {}
>  BitMapView(bm_word_t* map, idx_t size_in_bits) : BitMap(map, size_in_bits) {}
> };
> 
> I don't think GrowableArrayData is a better name, so if I need to change this, I'm going to need some decent suggestions. I talked briefly with Kim, but we couldn't immediately find a name that both liked. Suggestions?

I haven’t come up with anything better than we discussed.

>> 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.
> 
> OK. For GrowableArrayivew, I had an idea that someone *might* want to do:
> 
> int _array[10];
> ...
> GrowableArrayView view(array, 10, 10);
> do_something(view);
> 
> but I'll change this to protected.

Classes that are used as both a base class and a concrete class can cause problems.
For example, allowing accidental slicing, which leads to UB (which may or may not
be detected in some way).

>> 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".
> 
> I thinks we view this differently. My view is that memflags belong to the CHeap allocations, but is only valid if != mtNone. Setting it to mtNone doesn't imply that resource allocations are marked with mtNone. It just means that we shouldn't care about the memflags value when allocating resource memory.

I’m OK with an interpretation that mtNone is considered an invalid memflags everywhere.
There may be some places that ought to be treating it that way but aren’t.  The aarch64 pf
function is one; you filed a bug and have a fix for that one - thanks.

>> 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.)
> 
> I think you've lost me here. Are you proposing that I split this up into more constructors. Are you suggesting that I change this:
> 
> […]
> and adjust allocate(...) and GrowableArrayMetadata(...) accordingly?

Not relevant is mtNone signals “invalid”.

>> 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?
> 
> Mainly to be usable by the tests. I can add a friend test class and revert this.

OK.  I think I’d prefer these not be in the public API; I wouldn’t want someone
writing code that was conditional on any of them… Your call.