RFR: 8202082: Remove explicit CMS checks in CardTableBarrierSetAssembler

[Kim Barrett]

> On Apr 24, 2018, at 2:57 AM, Erik Österlund <erik.osterlund at oracle.com> wrote:
> 
> Hi Kim,
> 
> On 2018-04-24 04:15, Kim Barrett wrote:
>>> On Apr 20, 2018, at 10:43 AM, Erik Österlund <erik.osterlund at oracle.com> wrote:
>>> 
>>> Hi,
>>> 
>>> In CardTableBarrierSetAssembler, we sometimes need fencing due to the card table being scanned concurrently when using CMS with pre-cleaning. Rather than explicitly checking for those CMS flags in the generic CardTableBarrierSetAssembler class, it is preferrable to check the CardTable scanned_concurrently() property which already precisely reflects that.
>>> 
>>> Bug:
>>> https://bugs.openjdk.java.net/browse/JDK-8202082
>>> 
>>> Webrev:
>>> http://cr.openjdk.java.net/~eosterlund/8202082/webrev.00/
>>> 
>>> Thanks,
>>> /Erik
>> ------------------------------------------------------------------------------
>> src/hotspot/cpu/x86/gc/shared/cardTableBarrierSetAssembler_x86.cpp
>> 121     if (ct->scanned_concurrently()) {
>> 
>> I think the correct test here is card_mark_must_follow_store.  See
>> my review of 8202083 for more details.
>> 
>> Similarly for the other platforms.
> 
> I disagree. Let me explain why, and where I am trying to get with this.
> 
> We used to have a whole bunch of different ways of making decisions based on whether the card table is scanned concurrently or not, queried in various ways, depending on whether we were talking to the CollectedHeap interface, the CardTableModRefBS interface, or whether we were inside of the CardTableModRefBS class. I am trying to remove those one by one, until only one is left, the configuration point in CardTable which describes precisely that: that the card table is scanned concurrently. Here are a few examples:
> 
> * The ReduceInitialCardMarks optimization either defers card marks or issues them immediately on slowpath allocations, depending on whether the card table is scanned concurrently or not (this used to ask CollectedHeap about these queries).
> * Inside of the post barriers, StoreStore fencing between the reference store and card mark depends on whether the card table is scanned concurrently or not. (checked with explicit CMS + precleaning and sometimes conservatively just CMS checks)
> * When using UseCondCardMark, there is a StoreLoad fence to make sure the store of the reference happens-before the *load* of the card value (not the store of the card value) for filtering. Again, this is only an issue when the card table is scanned concurrently, and not very well described by whether the card mark must follow the store IMO.
> 
> The reason there is not yet a single property is that some code has previously asked CollectedHeap about these questions from outside of GC code (ReduceInitialCardMarks), while other code lived in the card table barrier set (CMS + pre-cleaning checks). Where I want to go is that no code outside of the CardTableBarrierSet classes should need to ask these questions. And code inside of the CardTableBarrierSet classes should ask its CardTable, which acts as a configuration, whether it is scanned concurrently or not.
> 
> So basically, I would prefer to now remove the card_mark_must_follow_store() method, and replace it with direct calls to whether the card table is scanned concurrently.
> 
> Do you agree?

I agree that fewer sources for what is effectively the same
information would be good.  I just don't think CardTable is the right
place for this piece of information.  CardTable is (largely) a
"simple" data structure (*), and doesn't care whether it's scanned
concurrently or not.  I think it belongs in the CTBS.

(*) I think CardTable could be simpler.  It presently contains a fair
amount of stuff that seems only needed by CMS or only needed by G1,
and should perhaps be placed elsewhere.

>> ------------------------------------------------------------------------------
>> src/hotspot/cpu/x86/gc/shared/cardTableBarrierSetAssembler_x86.cpp
>> 113     AddressLiteral cardtable((address)ctbs->card_table()->byte_map_base(), relocInfo::none);
>> 
>> [pre-existing]
>> 
>> Why isn't this using (address)disp rather than refetching the value.
>> 
>> ------------------------------------------------------------------------------
> 
> That seems unrelated to my changes.

Sort of; I did note it as pre-existing, but this line is being
changed.  And you've added a variable early in the method for the card
table, yet it isn't being used in the initializer for disp or in the
argument for cardtable construction.  Why not

  intptr_t disp = (intptr_t) ct->byte_map_base();
  ...
    AddressLiteral cardtable((address)disp, relocInfo::none);

That would have saved me some staring at these expressions to figure
out that there weren't any differences.

Although the ct variable was added for a later use that I've disagreed
with above...