RFR: JDK-8211955: GC abstraction for LAB reserve

[Per Liden]

Hi Roman,

On 10/11/2018 12:54 PM, Roman Kennke wrote:
[...]
>> For example, it's not obvious to me if you consider the Brooks pointer
>> to be an extension of the object header, and hence part of the object
>> and included in obj->size(), of if it's a separate thing. If it's a
>> separate thing, then a cell concept might prove useful. But if it's a
>> separate thing, then JDK-8211270 now incorrectly made
>> JvmtiEnv::GetObjectSize() and WB_GetObjectSize() return the cell size
>> instead of the object size.
>>
>> I assume you thought these things through? What's the story for
>> Shenandoah here?
> 
> We never made the distinction between cell and object size. Practically
> speaking, we treated the fwd pointer as part of the object, but made it
> invisible to the rest of the runtime, i.e. it's GC specific. We wanted
> to be honest about reporting the space that each object occupies in the
> heap, hence the changes to WB and JVMTI.

Were these two functions the only places where this was an issue, or do 
you have more patches lined up affecting shared code with similar 
problems? It would help to know to better understand if this is a 
small-ish problem that we can live with for now or if we need to do 
something bigger here, like start thinking about if we need the cell 
concept or something else.

I can't help but feel that we're piece by piece building a mess here, 
that will take a long time to clean up, instead of having a clear and 
well thought out plan from the beginning. I hope I'm wrong, of course.

> 
>>> and this could be called by JVMT and whitebox (JDK-8211270) with
>>> obj->size(), and by min_fill_size() with the static minimum obj size,
>>> and the GC would translate it to the cell size. The obj_size() from
>>> JDK-8211270 would then be removed and we'd have a single abstraction for
>>> this. Want me to make that change?
>>>
>>> Do you agree with the rest of this change? Because there's some
>>> significant changes how TLAB and PLAB calculate their alloc reserves in
>>> there too.
>>
>> I think we need to think through the basics above first.
>>
>> Generally, having PLAB call Universe::heap()->tlab_alloc_reserve() looks
>> wrong to me, since a PLAB is not a TLAB. If we really want to share this
>> code I think we should try to find some other way. Maybe just give
>> tlab_alloc_reserve() a better name, but at the same time I'm not even
>> sure this belongs in CollectedHeap.
> 
> We can change it to lab_alloc_reserve() instead. I think it should be
> shared between TLAB and PLAB because the concept is the same thing, and
> relies on the same properties: we need some reserve at the end in order
> to be able to fill it completely, and with Shenandoah this is GC
> specific. And since all the other code that deals with filling TLABs and
> PLABs resides in CollectedHeap, this naturally belongs there too, right
> next to CollectedHeap::fill_with_dummy_object(..) which is also used by
> both TLAB and PLAB.

Maybe a stupid question, but why do you even need a Brooks pointer in 
your filler objects? Is not having that maybe a solution here?

cheers,
Per

> 
> (BTW: some explanation why we care about PLAB at all: Shenandoah uses a
> sort of GCLAB for the GC workers to evacuate objects from one region to
> another. Java threads also use this for evacuations from write-barriers.
> We did use TLAB for this in the past, but this was - and still is -
> cumbersome because of the thight coupling of TLAB with Thread. For
> example TLAB needs to know about its offset in Thread. That's why we
> eventually switched to PLAB: this can be much easier used in
> GCThreadLocalData structure. It fits our purpose very well. We could
> have invented yet another LAB structure though...)
> 
> How do we want to go forward?
> 
> Roman
>