RFR: JDK-8211955: GC abstraction for LAB reserve

[Per Liden]

Hi,

On 10/11/2018 11:24 AM, Roman Kennke wrote:
[...]
>>> Ok, cool. Here's the updated webrevs:
>>> Incremental:
>>> http://cr.openjdk.java.net/~rkennke/JDK-8211955/webrev.02.diff/
>>> Full:
>>> http://cr.openjdk.java.net/~rkennke/JDK-8211955/webrev.02/
>>>
>>> Good now?
>>
>> I'm not sure I understand why we need yet another abstraction for this.
>> I'm thinking the stuff you did in JDK-8211270 should be enough? We
>> already have CollectedHeap::min_fill_size() to answer the question what
>> the min filler size is, so adding a new function doesn't make sense to
>> me. What am I missing?

I see now that I missed the second half of the JDK-8211270 mail thread.

> 
> min_fill_size() is static and cannot be overridden by GC. obj_size()
> from JDK-8211270 could be used if we'd pass a specific object. I guess
> we could make a better abstraction like what Erik suggested in the other
> thread:
> 
> virtual size_t cell_size(size_t obj_size) { return obj_size; }

I don't think we should introduced the concept of cell in this patch. 
That's a much larger concept/change that needs to be carefully thought 
out first.

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?

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

cheers,
Per