8195148: Collapse G1SATBCardTableModRefBS and G1SATBCardTableLoggingModRefBS into a single G1BarrierSet

[Erik Osterlund]

Hi Kim,

Thanks for the review.

On 2 Mar 2018, at 03:15, 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.cpp
> 35 G1BarrierSet::G1BarrierSet(
> 36   G1CardTable* card_table) :
> 37   CardTableModRefBS(card_table, BarrierSet::FakeRtti(BarrierSet::G1BarrierSet)),
> 38   _dcqs(JavaThread::dirty_card_queue_set())
> 39 { }
> 
> Move the argument to the same line as the constructor name, so it's
> easier to tell what are arguments and what are initializers.

Will fix.

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

Thanks,
/Erik

> ------------------------------------------------------------------------------
>