8195148: Collapse G1SATBCardTableModRefBS and G1SATBCardTableLoggingModRefBS into a single G1BarrierSet

[Erik Österlund]

Hi Kim,

New full webrev:
http://cr.openjdk.java.net/~eosterlund/8195148/webrev.01/

Incremental:
http://cr.openjdk.java.net/~eosterlund/8195148/webrev.00_01/

On 2018-03-02 23:51, Kim Barrett wrote:
>> On Mar 2, 2018, at 2:31 AM, Erik Osterlund <erik.osterlund at oracle.com> wrote:
>>> src/hotspot/share/gc/g1/g1BarrierSet.inline.hpp
>>> 32 inline void G1BarrierSet::write_ref_field_pre(T* field) {
>>>
>>> The change here doesn't seem to have anything to do with the renaming.
>>> Rather, it looks like a separate bug fix?
>>>
>>> The old code deferred the decode until after the null check, with the
>>> decoding benefitting from having already done the null check.  At
>>> first glance, the new code seems like it might not perform as well.
>>>
>>> I do see why adding volatile is needed here though.
>> I understand this might look unrelated. Here is my explanation:
>>
>> There has been an unfortunate implicit dependency to oop.inline.hpp. Now with some headers included in different order, it no longer compiles without adding that include. But including oop.inline.hpp causes an unfortunate include cycle that causes other problems. By loading the oop with RawAccess instead, those issues are solved.
>>
>> As for MO_VOLATILE, I thought I might as well correct that while I am at it. Some compilers can and actually do reload the oop after the null check, at which point they may be NULL and break the algorithm. I couldn’t not put that decorator in there to solve that.
>>
>> So you see how that started as a necessary include dependency fix (I had to do something or it would not compile) but ended up fixing more things. Hope that is okay.
> I tried this out, to try to get a better understanding of the issues,
> and I don't know what problems you are referring to.
>
> I used the variant I suggested, e.g. RawAccess and conversion to T
> rather than collapsing to oop, with the original oopDesc-based null
> check and decoding.  That did indeed fail to compile (not too
> surpisingly).  But adding an #include of oop.inline.hpp seemed to just
> work.
>
> While thinking about this before trying any experiments, it occurred
> to me that we might have a usage mistake around .inline.hpp files, but
> that didn't seem to arise here.
>
> So no, I'm not (yet) okay with this part of the change.

The problem does not manifest in our repository when including 
oop.inline.hpp in g1BarrierSet.inline.hpp. You are right about that. 
There is however still a problem. If you were to add a new barrier set 
with a name that comes after g1 in lexicographical order, and plug it in 
to the Access API through the barrierSetConfig files, the problem does 
manifest when oop.inline.hpp is included in g1BarrierSet.inline.hpp then.

The problem seems to be that access.inline.hpp includes 
barrierSet.inline.hpp, which includes barrierSetConfig.inline.hpp, which 
includes all the relevant barrier sets. If files other than Access 
include barrierSet.inline.hpp directly, then you will end up with a 
compilation error that the metafunction for getting the barrier set type 
from the enum number is not defined.

The good news is that I found a way of untangling this and going with 
your proposed solution of using RawAccess<MO_VOLATILE> with 
oop.inline.hpp. The solution is  to let access.inline.hpp include 
barrierSetConfig.inline.hpp directly (instead of barrierSet.inline.hpp), 
and remove the include of barrierSetConfig.inline.hpp in 
barrierSet.inline.hpp.

I hope you like the solution.

Thanks,
/Erik