RFR: 8189871: Refactor GC barriers to use declarative semantics

[Roman Kennke]

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?

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

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

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

Roman