RFR: 8202082: Remove explicit CMS checks in CardTableBarrierSetAssembler

[Erik Österlund]

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.

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

Okay. I will fix it then.

/Erik