RFR: 8189871: Refactor GC barriers to use declarative semantics

[Erik Osterlund]

Hi Roman,

Thanks for having a look at this.

> On 13 Nov 2017, at 18:10, Roman Kennke <rkennke at redhat.com> wrote:
> 
> Hi Erik,
> 
>> Hi Roman,
>> 
>>> On 2017-11-10 16:01, Roman Kennke wrote:
>>> Hi Erik,
>>> 
>>> This looks very good to me. It is likely that we'll need to extend it a little bit for Shenandoah, but I haven't got around to try that out yet, and will propose it when this patch percolated down to the Shenandoah project.
>> 
>> Yes. The framework should be quite flexible, and of course I will work with you on anything that needs to be updated.
> Perfect!
> 
>>> Questions (I know I've asked some of it before in private discussions):
>>> - A BarrierSet needs to declare an AccessBarrier inner class. How does this get 'registered' with the Access dispatcher?
>> 
>> Good question. Each new GC is tied together at one single point. The Access API picks up GCs from the gc/shared/barrierSetConfig.hpp and .inline.hpp files.
>> 
>> So to register a new GC, such as Shenandoah, you have to:
>> 
>> 1) Make sure you have a BarrierSet enum value which is added to the list of FOR_EACH_BARRIER_SET_DO as well as FOR_EACH_CONCRETE_BARRIER_SET_DO in barrierSetConfig.hpp.
>> 
>> The first of said lists contains all barrier sets that are known to exist at build time, and the second of said lists crucially contains concrete barrier sets that have an AccessBarrier to resolve.
>> 
>> 2) You also need to make sure in the barrierSetConfig.inline.hpp file that you #include your shenandoah BarrierSet inline.hpp file.
>> 
>> 3) You have to provide a specialization for the BarrierSet::GetName and BarrierSet::GetType metafunctions that provide an enum value for a barrier set type, and vice versa.
>> 
>> 4) Since you probably want primitive accesses in the heap to also resolve into the barrier set in a build that includes Shenandoah, you should #define SUPPORT_BARRIER_ON_PRIMITIVES in the barrierSetConfig.hpp file when building with Shenandoah. This will flick on the INTERNAL_BT_BARRIER_ON_PRIMITIVES decorator to each access so that the Access framework understands that even primitive accesses must be resolved at run-time in the barrier set. So this is a build-time switch for turning on run-time resolution of primitive accesses in the heap.
>> 
>> And now you should be set: your new ShenandoahBarrierSet::AccessBarrier will be called for each access, including primitives.
>> 
>> It works the following way:
>> 
>> 1) The barrier resolver loads the current barrier set, and checks the "name" of it (the enum value).
>> 2) Each "name" for concrete barriers that you listed in barrierSetConfig.hpp is asked for...
>> 3) ...the BarrierSet::GetType of that enum "name", and...
>> 4) The AccessBarrier of that resulting BarrierSet (your ShenandoahBarrierSet) will be called.
>> 
>> Hope that makes sense.
>> 
> 
> It is ok. But just so. ;-)
> The problem is that it is a bit mystical how stuff is set up, and I'd prefer a more explicit way to do it.
> Maybe add the above explanations (how to make new GC barriers) in a comment somewhere?

Okay, will do!

> 
>>> - I see you use namespace. I haven't seen them anywhere else in Hotspot, so this looks quite unusual :-) Not that I am against it (I would probably advocate for using more of it), but have you considered alternatives that look more common Hotspot-style (e.g. declaring an all-static AccessInternal class)?
>> 
>> We do not have any platform that has problems with namespaces. So I would prefer this basic namespace usage to a bunch of AllStatic classes. I hope nobody minds that.
>> 
> I am ok with it.

Good, thank you.

> 
>>> - The dispatching machinery looks a bit over the top, and from the outskirts like a manual re-invention of virtual method dispatch. Couldn't we do the same stuff with the usual public interface / concrete implementation idioms? I am worried that adding just one method to the interface turns into a nightmare of wiring up stuff and adding tons of boilerplate to get it going. Not to mention the learning curve involved trying to make sense of what goes where.
>> 
>> It's not quite like a normal virtual call though. It's a virtual dispatch that carries template parameters through the runtime dispatch. The template parameters are required for 1) remembering the decorators (semantics of the access), and 2) the type information of the operands for the access.
>> 
>> In my experience, when you need to drag template parameters through some form of virtual dispatch, it is easiest to use function pointers and not virtual calls.
>> I understand it might look a bit complicated, but I hope that is okay.
> Right. Templates and virtual doesn't play well (or at all) together.

Indeed.

> I've seen that the GC interface JEP just went to 'targeted' so I guess you can push it (probably needs more review?)

Yes, soon indeed.

Thanks,
/Erik

> Roman