RFR: 8247755: Leaner and more versatile GrowableArray classes
Stefan Karlsson
stefan.karlsson at oracle.com
Mon Jun 22 13:46:33 UTC 2020
On 2020-06-22 15:13, coleen.phillimore at oracle.com wrote:
>
>
> 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?
Done:
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?
>
> 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.
I don't know how strong Kim's objection to this is. I'll wait for him to
comment on it.
StefanK
>
> 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