8195148: Collapse G1SATBCardTableModRefBS and G1SATBCardTableLoggingModRefBS into a single G1BarrierSet

[Erik Osterlund]

Hi Kim,

On 2 Mar 2018, at 03:43, Kim Barrett <kim.barrett at oracle.com> wrote:

>> On Mar 1, 2018, at 9:15 PM, Kim Barrett <kim.barrett at oracle.com> wrote:
>> 
>>> On Feb 26, 2018, at 8:38 AM, Erik Österlund <erik.osterlund at oracle.com> wrote:
>>> 
>>> Hi,
>>> 
>>> G1 has two barrier sets: an abstract G1SATBCardTableModRefBS barrier set that is incomplete and you can't use, and a concrete G1SATBCardTableLoggingModRefBS barrier set is what is the one actually used all over the place. The inheritance makes this code more difficult to understand than it needs to be.
>>> 
>>> There should really not be an abstract G1 barrier set that is not used - it serves no purpose. There should be a single G1BarrierSet instead reflecting the actual G1 barriers used.
>>> 
>>> Webrev:
>>> http://cr.openjdk.java.net/~eosterlund/8195148/webrev.00/
>>> 
>>> Bug:
>>> https://bugs.openjdk.java.net/browse/JDK-8195148
>>> 
>>> Thanks,
>>> /Erik
>> 
>> Mostly looks good.  One minor formatting nit, and one significant not so sure about this.
>> 
>> […]
> 
>> ------------------------------------------------------------------------------
>> 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.
> 
> Without exploring too deeply or doing any testing, it seems to me the
> only change that should be made here is to replace the use of
> oopDesc::load_heap_oop with RawAccess<MO_VOLATILE>::oop_load, e.g. it
> should be
> 
>  T heap_oop = RawAccess<MO_VOLATILE>::oop_load(field);
>  if (!oopDesc::is_null(heap_oop)) {
>    enqueue(oopDesc::decode_heap_oop_not_null(heap_oop));
>  }
> 
> That's assuming the LoadProxy can resolve to a narrowOop.

It can, but then I would still have the include dependency problem that served as primary motivator.

Thanks,
/Erik