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

[coleen.phillimore at oracle.com]

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.
>
> 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.
>
> 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,
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