[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR: 8322476: Remove GrowableArray C-Heap version, replace usages with GrowableArrayCHeap
From:       Kim Barrett <kim.barrett () oracle ! com>
Date:       2023-12-22 23:22:24
Message-ID: 300914D8-7893-4859-A32F-3250CF7C3C84 () oracle ! com
[Download RAW message or body]

[Kim Barret wrote:]
> > > pre-existing: There are a lot of non-static class data members that are \
> > > pointers to GrowableArray that seem like they would be better as direct, e.g. \
> > > non-pointers. 
> > > pre-existing: There are a lot of iterations over GrowableArray's that would be
> > > simplified by using range-based-for.
> > > 
> > > I'm not a fan of the additional clutter in APIs that the static memory types \
> > > add. If we had a variant of GrowableArrayCHeap that was not itself dynamically \
> > > allocatable and took a memory type to use internally as a constructor argument, \
> > > then I think a lot of that clutter could be eliminated.  It could be used for \
> > > ordinary data members that are direct GAs rather than pointers to GAs.  I think \
> > > there is a way to do something similar for static data members that are \
> > > pointers that are dynamically allocated later, though that probably requires \
> > > more work. 
> > 
> > On Fri, 22 Dec 2023 13:22:19 GMT, Stefan Karlsson <stefank@openjdk.org> wrote:
> > FWIW, I added the GrowableArrayCHeap and the static memory type. I did that \
> > because there was a perceived need to minimize the memory usage, because we were \
> > going to use an extreme amount of these arrays for one of our subsystems in ZGC. \
> > It later turned out that we really didn't need to squeeze out the last bit of \
> > memory for that use-case. I would really like to get rid of the the static memory \
> > type from GrowableArrayCHeap, and just add it as an instance member.

Currently the basic overhead for a static-MEMTYPE GA is 16 bytes (data
pointer, int length and capacity).  So 16 bytes on 64bit platforms.  Adding
the memory type adds 8 bytes (due to alignment), so 50% increase.  Though it
may not matter for dynamically allocated GAs, since std::max_align_t is
probably at least 32 bytes.  Though I think we seriously overuse dynamic
allocation for GAs.

Does it really matter.  I don't know.

StringDedup uses a lot of GAs. (Though it could use 1/2 as many with some work,
since they are used in pairs that always have the same length and capacity. At
the time I was working on it I was feeling lazy and just used pairs of GAs, with the
intention of coming back to that at some point if it proved to be a significant \
problem.)

> On Dec 22, 2023, at 8:33 AM, Emanuel Peter <epeter@openjdk.org> wrote:
> @stefank Maybe it would then make sense to do that before this change here? Because \
> we will really have to touch all these changes here again for that. 
> @kimbarrett @stefank @jdksjolen
> Or alternatively, we just say that we want to keep the C-Heap functionality inside \
> `GrowableArray`, and simply remove `GrowableArrayCHeap`? Though I honestly would \
> prefer a world where every allocation strategy has its own `GrowableArray*` \
> variant, so that it is clear statically that they cannot be assigned to each other. \
> So my preference would even be to have these: 
> CHeapGrowableArray
> ArenaGrowableArray
> ResourceAreaGrowableArray
> 
> What do you think?

I think having distinct CHeap, Resource, and Arena allocated types would be an
improvement. If we were using std::vector with associated allocators we'd have
something like that. (The allocator type is a template parameter for
std::vector.) Refactoring GA in that direction is certainly possible, and
might be an improvement.

We could also have two variants of CHeap GAs, one with the memtype being a
constructor argument (and possibly different from the memtype used to
dynamically allocate the GA, as is currently the case, although I think that
feature is never used), and a more compact one with a static memtype.

I did some work on HotSpot allocators for standard containers like std::vector
a while ago that could be applied to GA.  It includes support for dynamic and
static memtypes, and shows that isn't hard to do.

I thought about whether this PR should go ahead without some of that stuff in
place. It touches a lot of places that would probably be touched again if we
went in that sort of direction. OTOH, a lot of these same places would
probably need to be touched repeatedly anyway, one way or another. For
example, dealing with what appears to be a large amount of unnecessary dynamic
allocation of GAs would also touch a lot of these places, and I'm suspicious
of combining the type changes and the allocation changes in one PR.

> And how important is it that we do not use `virtual` with `GrowableArray`? Because \
> the implementation and testing is much nastier without it. Is the V-table still \
> such a overhead that we need to avoid it?

I don't think there is any need for virtual functions in GA.


["signature.asc" (signature.asc)]

-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEErsON5iYRKpuGOrjBMSl6JxmzxBAFAmWGGiYACgkQMSl6Jxmz
xBBI4xAAuYSMwIqwQPih4DGr+fwABw5BMqsJUCWZRmnnigufGtFY0JZH44EaT6R8
UOH/X71u/FS/+HDAUUZdlfllRjPRAHNjvO8eJJb8Q2U7EhDsHBoP/d+ZNp0y/Kc4
Ps5fpnHEo0PwDwYxNFmSpqGMBcrrksx7pXK61gYJpJ6yyneLHshr946cHaMNMeVX
1vptimiHosfuFAYikwa0arc/vr/vEhw1xXlOot7XYS12Bl1230RGzBN+s0YzmyW/
oYfSah/6bfAAJP/XHBACYmcNdzs4oMNmk5Y0o7ThFzjyGsXVKBMY1E6+dUL4sAxd
YqgUPXWcqZAR881VnM4rw1ORcEdM8l/UhCVwEpLTk4C3UxVPhxgit2N/gCnYJ94/
B3JThpsRkEQspThwuuw876iF/2zQ5o1JyrWWC+oEgF0Q7ZHXW/rC3ygXTv6p/lx7
MYmp8XRVTAjt9DQwQtl0YSWKcoZMmvyq9L2xiTeguo6jkpIq46FP0SbUs2EiTpnP
fC1VjWramFclU9qzW1uXZBfPU8SLR0NGUoBe/2pZZtP9IO6nuCd7ZpkdyZEB79QY
eLrLvAiILpyIJQUMHDb0GEyrhsFM8X8cA7scnKRuR2hj/rghQvsqClM/OOL43xd0
Rg2q1XAgdtIeGtINVgb/dpaqt9E+0TyQkqkqNeKko1nEefpZZWc=
=mYvZ
-----END PGP SIGNATURE-----


[prev in list] [next in list] [prev in thread] [next in thread]