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

[Thomas Stuefe]

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?

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.

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.

----

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

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.

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