RFR: 8247755: Leaner and more versatile GrowableArray classes
coleen.phillimore at oracle.com
coleen.phillimore at oracle.com
Mon Jun 22 13:13:29 UTC 2020
I have a couple of comments to the comments.
snip.
------------------------------------------------------------------------------
>> 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.
>>
>
> 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.
Can you file another RFE to fix this?
>
>> ------------------------------------------------------------------------------
>>
>> 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?
The name "View" looks weird to me but since this class shouldn't be used
in outside code, this doesn't matter to me.
>
>> ------------------------------------------------------------------------------
>>
>> 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.
>
>
>> ------------------------------------------------------------------------------
>>
>> 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.
>
> Just like this function shows:
> 570 static E* allocate(int max, MEMFLAGS memflags) {
> 571 if (memflags != mtNone) {
> 572 return (E*)GrowableArrayCHeapAllocator::allocate(max,
> sizeof(E), memflags);
> 573 }
> 574
> 575 return (E*)GrowableArrayResourceAllocator::allocate(max,
> sizeof(E));
> 576 }
>
> and this:
> 495 uintptr_t bits(MEMFLAGS memflags) const {
> 496 if (memflags == mtNone) {
> 497 // Stack allocation
> 498 return 0;
> 499 }
> 500
> 501 // CHeap allocation
> 502 return (uintptr_t(memflags) << 1) | 1;
> 503 }
>
> (Probably should have turned that the other way around to have the
> same code layout)
>
> W.r.t. VirtualMemoryTracker, I think that's odd and probably should be
> cleaned up.
>
>> aarch64 pf (debugger print frame) passes mtNone to NEW_C_HEAP_OBJ,
>> which seems like it shouldn't work, given the assert in MallocSite.
>>
>
> I'll see if I can provoke this and will create a JBS entry if I do.
>
>> 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:
>
> GrowableArray(int initial_max = 2, MEMFLAGS memflags = mtNone) :
> GrowableArrayWithAllocator<E, GrowableArray<E> >(
> allocate(initial_max, memflags),
> initial_max),
> _metadata(memflags) {
> init_checks();
> }
>
> GrowableArray(int initial_max, int initial_len, const E& filler,
> MEMFLAGS memflags = mtNone) :
> GrowableArrayWithAllocator<E, GrowableArray<E> >(
> allocate(initial_max, memflags),
> initial_max, initial_len, filler),
> _metadata(memflags) {
> init_checks();
> }
>
> to (say):
> GrowableArray(int initial_max = 2) :
> GrowableArrayWithAllocator<E, GrowableArray<E> >(
> allocate(initial_max),
> initial_max),
> _metadata() {
> init_checks();
> }
>
> GrowableArray(int initial_max, MEMFLAGS memflags) :
> GrowableArrayWithAllocator<E, GrowableArray<E> >(
> allocate(initial_max, memflags),
> initial_max),
> _metadata(memflags) {
> init_checks();
> }
>
> GrowableArray(int initial_max, int initial_len, const E& filler) :
> GrowableArrayWithAllocator<E, GrowableArray<E> >(
> allocate(initial_max),
> initial_max, initial_len, filler),
> _metadata() {
> init_checks();
> }
>
> GrowableArray(int initial_max, int initial_len, const E& filler,
> MEMFLAGS memflags) :
> GrowableArrayWithAllocator<E, GrowableArray<E> >(
> allocate(initial_max, memflags),
> initial_max, initial_len, filler),
> _metadata(memflags) {
> init_checks();
> }
>
> and adjust allocate(...) and GrowableArrayMetadata(...) accordingly?
No, please don't do this. Passing mtNone as a default parameter for
resource allocated GrowableArrays and !mtNone if not, seemed like a nice
compact change, with obvious meaning.
Thanks,
Coleen
>
>>
>> And if mtNone is going to be used that way, then GrowableArrayCHeap
>> should static assert that it's flag value is not mtNone.
>>
>
> Sure.
>
>> ------------------------------------------------------------------------------
>>
>> 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.
>
>> ------------------------------------------------------------------------------
>>
>>
>
> Thanks for reviewing this,
> StefanK
More information about the hotspot-dev
mailing list