RFR: JDK-8211955: GC abstraction for LAB reserve

[Roman Kennke]

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?

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20181015/b1e58a19/signature.asc>