RFR: JDK-8211955: GC abstraction for LAB reserve

[Roman Kennke]

Thanks Per!

Roman


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

-------------- 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/b419e33b/signature.asc>