RFR: JDK-8211955: GC abstraction for LAB reserve

[Per Liden]

Hi,

On 10/15/2018 04:47 PM, Roman Kennke wrote:
> Hi Per,
> 
> Thanks! I take that as 'Reviewed' except that I believe we haven't been
> clear which one. The last one I posted was did also include changes for
> whitebox and jvmti and rename to 'cell_size':
> 
> http://cr.openjdk.java.net/~rkennke/JDK-8211955/webrev.03/
> 
> but I'm not sure we wanted that.
> 
> And the one before was:
> http://cr.openjdk.java.net/~rkennke/JDK-8211955/webrev.02/
> 
> I suggest to go with that for now. Ok?

Yes, please go with webrev.02 for now and let's revisit the cell concept 
at a later time.

cheers,
Per

> 
> Roman
> 
> 
>> Hi Roman,
>>
>> On 10/15/2018 03:27 PM, Roman Kennke wrote:
>>> Hi Per,
>>>
>>> what do you think about putting this proposed change in jdk/jdk now, and
>>> work on the improvements later / on top of it? It does provide some
>>> reasonable consolidation I think, which seems worth as-is.
>>
>> Sure. I still have at least one bug to sort out in my patch, and today
>> I've been derailed by some higher priority stuff. So, go ahead and I'll
>> follow up with my patch later.
>>
>> cheers,
>> Per
>>
>>>
>>> Thanks,
>>> Roman
>>>
>>>
>>>> Hi Roman,
>>>>
>>>> On 10/11/2018 02:41 PM, Roman Kennke wrote:
>>>> [...]
>>>>> Those are the only functions where this matters. In-fact, the LAB
>>>>> reserve abstraction is the last one I have lined up. The remaining
>>>>> stuff
>>>>> that Shenandoah needs is a bunch of extra utility classes and C2
>>>>> changes.
>>>>
>>>> Thanks for clarifying. That makes me much less worried.
>>>>
>>>> For C2, the strategy we had and the requirement we put on our selves
>>>> when integrating ZGC was basically this:
>>>>
>>>> We really didn't want to screw things up in C2. If things went south, we
>>>> wanted to just be able to disable building of ZGC and C2 would go back
>>>> to do _exactly_ what it did before we merged the ZGC patch. As you maybe
>>>> remember, we _almost_ got there. We had a very small set of places in C2
>>>> where we did change/add code, which was _not_ disabled when disabling
>>>> ZGC. However, it was easy to prove that that code was safe/correct. This
>>>> strategy turned out to work well, and it forced us to think hard about
>>>> having abstractions in the right places, move as much code as possible
>>>> into the BarrierSet, etc.
>>>>
>>>> I'd recommend you do the same for Shenandoah, that way the pressure on
>>>> you (in case something breaks and results instability or a long bug
>>>> trail) will be reduced a lot. If shit hits the fan, you can always
>>>> disable building of Shenandoah until the problems are sorted out.
>>>>
>>>> [...]
>>>>>>>> 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?
>>>>>
>>>>> Filler objects are objects. Laying them out differently would make
>>>>> parsing the heap impossible.
>>>>
>>>> Check.
>>>>
>>>> I thought some more about this. How about we rip out the filler-stuff
>>>> from CollectedHeap, and put it in a Fill utility class. You could think
>>>> of Fill as a cosine to the "class Copy" we have today. Something like:
>>>>
>>>> class Fill : public AllStatic {
>>>> private:
>>>>     static int _min_size;
>>>>
>>>> public:
>>>>     static void set_min_size(int min_size) {
>>>>       _min_size = min_size;
>>>>     }
>>>>
>>>>     static int min_size() {
>>>>       return _min_size;
>>>>     }
>>>>
>>>>     static int min_reserve_size() {
>>>>       return _min_size > MinObjAlignment ? _min_size : 0;
>>>>     }
>>>>
>>>>     static void with_objects(...);
>>>>     static void with_object(...);
>>>>     ...
>>>> };
>>>>
>>>> Fill::_min_size is initialized to oopDesc::header_size() by default. A
>>>> collector that wants something different calls Fill::set_min_size(...)
>>>> during start up. TLAB/PLAB would call Fill::min_reserve_size().
>>>>
>>>> In my mind, I think this would put things where they belong. And as a
>>>> bonus we would cut out some cruft from CollectedHeap. What do you think?
>>>> I started doing a patch for this to see if it seems like a good
>>>> approach. I hope to have something to show later today.
>>>>
>>>> cheers,
>>>> Per
>>>
>