RFR: 8295475: Move non-resource allocation strategies out of ResourceObj [v2]

[Thomas Stüfe]

Hi Coleen,


On Thu, Oct 20, 2022 at 4:26 PM <coleen.phillimore at oracle.com> wrote:

>
>
> On 10/19/22 2:41 AM, Thomas Stuefe wrote:
> > On Tue, 18 Oct 2022 13:42:40 GMT, Stefan Karlsson <stefank at openjdk.org>
> wrote:
> >
> >>> Background to this patch:
> >>>
> >>> This prototype/patch has been discussed with a few HotSpot devs, and
> I've gotten feedback that I should send it out for broader
> discussion/review. It could be a first step to make it easier to talk about
> our allocation super classes and strategies. This in turn would make it
> easier to have further discussions around how to make our allocation
> strategies more flexible. E.g. do we really need to tie down utility
> classes to a specific allocation strategy? Do we really have to provide
> MEMFLAGS as compile time flags? Etc.
> >>>
> >>> PR RFC:
> >>>
> >>> HotSpot has a few allocation classes that other classes can inherit
> from to get different dynamic-allocation strategies:
> >>>
> >>> MetaspaceObj - allocates in the Metaspace
> >>> CHeap - uses malloc
> >>> ResourceObj - ...
> >>>
> >>> The last class sounds like it provide an allocation strategy to
> allocate inside a thread's resource area. This is true, but it also
> provides functions to allow the instances to be allocated in Areanas or
> even CHeap allocated memory.
> >>>
> >>> This is IMHO misleading, and often leads to confusion among HotSpot
> developers.
> >>>
> >>> I propose that we simplify ResourceObj to only provide an allocation
> strategy for resource allocations, and move the multi-allocation strategy
> feature to another class, which isn't named ResourceObj.
> >>>
> >>> In my proposal and prototype I've used the name AnyObj, as short,
> simple name. I'm open to changing the name to something else.
> >>>
> >>> The patch also adds a new class named ArenaObj, which is for objects
> only allocated in provided arenas.
> >>>
> >>> The patch also removes the need to provide ResourceObj/AnyObj::C_HEAP
> to `operator new`. If you pass in a MEMFLAGS argument it now means that you
> want to allocate on the CHeap.
> >> Stefan Karlsson has updated the pull request incrementally with one
> additional commit since the last revision:
> >>
> >>    Fix Shenandoah
> > So, `AnyObj` is the holdover from the old ResourceObj? Is the intent to
> move most objects to a clear allocation type in followup RFEs?
>
> Yes.
> >
> > I'm not sure that we need `AnyObj` at all. Or, we could get rid of it in
> the future. The purpose of `AnyObj` is to place an object into a different
> area than its class designer intended. That can be done simply via holder
> objects, see my earlier comment. It would remove the runtime cost of
> tracking the allocation type per allocation. It would be a bit less
> convenient since you have to go thru the holder when accessing the object.
> But we don't want to have many classes like these anyway.
>
> Yes, we're trying to find a nice way to remove AnyObj in favor of a
> factory or holder object for these classes.  There are fewer than we
> thought.  This change is nice because we can easily find them by looking
> for AnyObj, rather than the ResourceObjs.
>

This makes sense. We can do it piece by piece, with individual RFEs.


> >
> > I think that many cases where we today have a ResourceObj allocated in
> C-Heap can actually be made CHeapObj. I'm not sure how many real cases
> there are where we mix C-heap and RA for the same class.
>
> The not well named anymore class ResourceHashtable is one such class
> that may be better always CHeap obj allocated, but we have to see. There
> are a couple of AnyObj classes that I don't know anything about so they
> can be looked at also.
> >
> > ----
> >
> > If we go with `AnyObj`, I have a smaller concern:
> >
> > `AnyObj` sounds like the typical absolute base class many frameworks
> have. But here, it has a specific role, and we probably want to discourage
> its use for new classes. It should only be used where its really needed. It
> does not have to be super convenient, and maybe should be renamed to
> something like `MultipleAllocationObj` or similar.
> >
> > Then, allocation area for `AnyObj` is determined by the overloaded new I
> use:
> >
> >       - `new X;` // RA
> >       - `new (mtTest) X;` // C-Heap
> >       - `new (arena) X;` // lives in Arena
> >
> > but this can be confusing, especially for newcomers. The default of RA
> is surprising, in `ResourceObj` it was right in the name. Since RA
> allocation is a bit dangerous, I think RA as default can trip over people.
>
> I think this RA default allocation is something we want to fix further
> by making AnyObj not be an allocation class but these objects use a
> factory.  For this change, we're just separating AnyObj out from true
> ResourceObjs.  We have further work that we will do.
> >
> > I also dislike the MEMFLAGS==CHeap association. MEMFLAGS is semantically
> different from where the object lives and this may bite us later.
> >
> > For example, today we don't require MEMFLAG for RA allocation because
> the arena is tagged with a single flag. But we could easily switch to
> per-allocation tracking instead by allowing an arena to carry multiple
> MEMFLAGS. It would have some advantages.
>
> Having arena and ResourceArea tracking have multiple MEMFLAGS would
> increase space and overhead though.  ResourceObjs/ArenaObjs are supposed
> to be minimally invasive.  We'd have to talk about a change like that.
>

Sure. But note that the overhead would be *per Arena*, not per Object. Per
Object would be terrible :) You'd have a vector of counters per arena where
today you have a single counter. Also, only if NMT is on of course. We are
talking about dozens, at most hundreds of bytes per physical Thread (per
RA) if NMT is on.

What we gain are complexity reduction in NMT and increased tracking
resolution. RAs accumulate a lot of allocations from different use spaces.
But we track all this as just "RA". Or, in compiler threads, as "compiler"
(there is this weird hack that basically re-accounts RAs for compiler
threads to "compiler" - we could get rid of all these ugly hacks).


> >
> > Therefore I would require the user to hand in the allocation type when
> calling new AnyObj. It would be clearer, and since we don't want to have
> many of these classes anyway, I think that would be okay.
>
> Agree that we don't want to have many of these classes anyway. This
> AnyObj patch cuts down the number of classes we're looking at, which is
> why I like it so much.  We're not done though, and your ideas about its
> replacement are welcome.
>
>
Thanks for these explanations! It is unfortunate that outside Oracle one is
often out of the loop, but this patch makes a lot of sense to me and is a
nice complexity reduction.

Cheers, Thomas

Thanks,
> Coleen
> >
> > src/hotspot/share/asm/codeBuffer.hpp line 386:
> >
> >> 384:   // CodeBuffers must be allocated on the stack except for a single
> >> 385:   // special case during expansion which is handled internally.
> This
> >> 386:   // is done to guarantee proper cleanup of resources.
> > Not your patch, but comment is misleading. Probably should say "on the
> ResourceArea".
> >
> > -------------
> >
> > PR: https://git.openjdk.org/jdk/pull/10745
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-dev/attachments/20221020/6b7e7674/attachment.htm>