RFR: 8247755: Leaner and more versatile GrowableArray classes

[Stefan Karlsson]

Latest patches:
  https://cr.openjdk.java.net/~stefank/8247755/webrev.02.delta/
  https://cr.openjdk.java.net/~stefank/8247755/webrev.02/

I haven't changed:
- GrowableArrayView to something else
- S to ALLOC or Derived. I'm leaning towards Derived.

Some comments inlined:

On 2020-06-23 11:35, Kim Barrett wrote:
>> 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

Thanks.

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

I created this:
https://bugs.openjdk.java.net/browse/JDK-8248007

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

Then the current proposal is to keep this as GrowableArrayView. It's 
also an easy fix to deal with as a follow-up RFE.

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

Noted.

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

For the record:
https://bugs.openjdk.java.net/browse/JDK-8248130

I haven't created a cleanup task for VirtualMemoryTracker oddity. I 
might get to it later.

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

OK.

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

I changed them to be private.

StefanK

> 
>