RFR: JDK-8211955: GC abstraction for LAB reserve

Roman Kennke rkennke at redhat.com
Fri Oct 12 08:02:12 UTC 2018


Hi Per,

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

Thanks for your suggestions and support! That's basically the approach
we're following with Shenandoah too: 1. put as much stuff (that makes
sense) behind a reasonable BarrierSetC2 interface 2. wrap the rest in
#ifdef INCLUDE_SHENANDOAHGC and/or if (UseShenandoahGC) and
justify/upstream stuff ahead of time that's not Shenandoah-specific and
otherwise useful.

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

That seems like a good idea. Not sure if it should be AllStatic? Maybe
put the abstractions (that are currently in CH, plus the one I proposed
with this patch) into the Fill class?

I'm wondering if I should push this patch ahead of this, and then we can
build the Fill refactoring on top of it? Otherwise I'll just wait for
you to come up with a proposal.

Thanks & Cheers,
Roman

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/attachments/20181012/116130a8/signature.asc>


More information about the hotspot-gc-dev mailing list