RFR: 8247755: Leaner and more versatile GrowableArray classes
Stefan Karlsson
stefan.karlsson at oracle.com
Mon Jun 22 09:01:15 UTC 2020
On 2020-06-22 09:25, Kim Barrett wrote:
>> 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?
>
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?
> ------------------------------------------------------------------------------
> 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.
> ------------------------------------------------------------------------------
> 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?
> ------------------------------------------------------------------------------
> 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?
>
> 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