RFR: 8202082: Remove explicit CMS checks in CardTableBarrierSetAssembler

[Erik Österlund]

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?

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

Thanks,
/Erik