RFR: 8202082: Remove explicit CMS checks in CardTableBarrierSetAssembler

[Kim Barrett]

> On Apr 24, 2018, at 3:17 PM, Erik Österlund <erik.osterlund at oracle.com> wrote:
> 
> Hi Kim,
> 
> On 2018-04-24 20:23, Kim Barrett wrote:
>>> 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.
> 
> Whether the CardTable is scanned concurrently or not is the property we are consistently interested in. That is a natural property of the CardTable to me, and hence belongs in the CardTable. I don't see a convincing argument why it would not be a natural property of the CardTable. Conversely, I find that it would mess up GenCollectedHeap if it was not. Currently, the GenCollectedHeaps unconditionally instantiates a CardTableBarrierSet, with a single configuration parameter: the CardTable, which is created with a virtual call as either CardTableRS or CMSCardTable. With your proposal to move the property of whether the CardTable is scanned concurrently or not, out from the CardTable, that would inevitably result in more virtual calls only for configuring the CardTableBarrierSet with this property, or make the CardTableBarrierSet itself into a configuration (created with virtual calls). That does not look like a win to me. Do you agree?
> 
>> (*) 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.
> 
> Sure. Those things should be moved into CMSCardTable and G1CardTable respectively.

Or stated differently:
Whether the collector scans the card table concurrently or not is the
property we are consistently interested in.  That is a natural
property of the collector to me...

I see the card table as data, that can/should be largely independent
of the collector / barrier set. The collector / barrier set operate on
that data, and different collectors operate on it in different ways.
It's the collector / barrier set that determines the policies and
behaviors. In particular, there's no intrinsic reason for the card
table to have any knowledge at all about whether it is scanned
concurrently or not.

I'm also doubtful about the hierarchy based on CardTable, though I
understand how we got to where we are; I once took a stab at
extracting the card table stuff from the barrier set stuff, and all I
got was a mess. You did better, but it's still pretty messy and could
use more work.

That said, an argument that the messiness that we presently have makes
the card table an easier place to put this bit of information does
carry weight.  So okay, I guess.

>>>> src/hotspot/cpu/x86/gc/shared/cardTableBarrierSetAssembler_x86.cpp
>>>> 113     AddressLiteral cardtable((address)ctbs->card_table()->byte_map_base(), relocInfo::none);
>> 
>> 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.
> 
> Okay. I will fix it then.

Thanks.