RFR(L): 8195142: Refactor out card table from CardTableModRefBS to flatten the BarrierSet hierarchy

Wed Feb 21 14:53:35 UTC 2018

Hi Coleen,

Thank you for having a look at this.

The BarrierSet switch statements in platform specific code are going 
away relatively soon. Do you still want me to synchronize all the 
switches to handle such errors the same way now before we get there, or 
wait a few patches and have them removed?

Thanks,
/Erik

On 2018-02-21 14:17, coleen.phillimore at oracle.com wrote:
>
> Hi Erik,  I started looking at this but was quickly overwhelmed by the 
> changes.  It looks like the case for BarrierSet::ModRef is removed in 
> the stubGenerator code(s) but not in templateTable do_oop_store.   
> Should the case of BarrierSet::ModRef get a ShouldNotReachHere in 
> stubGenerator in the places where they are removed?
>
> Some platforms have code for this in do_oop_store in templateTable and 
> some platforms get ShouldNotReachHere(), which does not pattern match 
> for me.
>
> - case BarrierSet::CardTableForRS:
> - case BarrierSet::CardTableExtension:
> - case BarrierSet::ModRef:
> + case BarrierSet::CardTableModRef:
>
>
> I think SAP should test this out on the other platforms to hopefully 
> avoid any issues we've been seeing lately with multi-platform 
> changes.  CCing Thomas.
>
> thanks,
> Coleen
>
> On 2/21/18 6:33 AM, Erik Österlund wrote:
>> Hi Erik,
>>
>> Thank you for reviewing this.
>>
>> New full webrev:
>> http://cr.openjdk.java.net/~eosterlund/8195142/webrev.02/
>>
>> New incremental webrev:
>> http://cr.openjdk.java.net/~eosterlund/8195142/webrev.01_02/
>>
>> On 2018-02-21 09:18, Erik Helin wrote:
>>> Hi Erik,
>>>
>>> this is a very nice improvement, thanks for working on this!
>>>
>>> A few minor comments thus far:
>>> - in stubGenerator_ppc.cpp:
>>>   you seem to have lost a `const` in the refactoring
>>
>> Fixed.
>>
>>> - in psCardTable.hpp:
>>>   I don't think card_mark_must_follow_store() is needed, since
>>>   PSCardTable passes `false` for `conc_scan` to the CardTable
>>>   constructor
>>
>> Fixed. I took the liberty of also making the condition for 
>> card_mark_must_follow_store() more precise on CMS by making the 
>> condition for scanned_concurrently consider whether 
>> CMSPrecleaningEnabled is set or not (like other generated code does).
>>
>>> - in g1CollectedHeap.hpp:
>>>   could you store the G1CardTable as a field in G1CollectedHeap? Also,
>>>   could you name the "getter" just card_table()? (I see that
>>>   g1_hot_card_cache method above, but that one should also be 
>>> renamed to
>>>   just hot_card_cache, but in another patch)
>>
>> Fixed.
>>
>>> - in cardTable.hpp and cardTable.cpp:
>>>   could you use `hg cp` when constructing these files from
>>>   cardTableModRefBS.{hpp,cpp} so the history is preserved?
>>
>> Yes, I will do this before pushing to make sure the history is 
>> preserved.
>>
>> Thanks,
>> /Erik
>>
>>>
>>> Thanks,
>>> Erik
>>>
>>> On 02/15/2018 10:31 AM, Erik Österlund wrote:
>>>> Hi,
>>>>
>>>> Here is an updated revision of this webrev after internal feedback 
>>>> from StefanK who helped looking through my changes - thanks a lot 
>>>> for the help with that.
>>>>
>>>> The changes to the new revision are a bunch of minor clean up 
>>>> changes, e.g. copy right headers, indentation issues, sorting 
>>>> includes, adding/removing newlines, reverting an assert error 
>>>> message, fixing constructor initialization orders, and things like 
>>>> that.
>>>>
>>>> The problem I mentioned last time about the version number of our 
>>>> repo not yet being bumped to 11 and resulting awkwardness in JVMCI 
>>>> has been resolved by simply waiting. So now I changed the JVMCI 
>>>> logic to get the card values from the new location in the 
>>>> corresponding card tables when observing JDK version 11 or above.
>>>>
>>>> New full webrev (rebased onto a month fresher jdk-hs):
>>>> http://cr.openjdk.java.net/~eosterlund/8195142/webrev.01/
>>>>
>>>> Incremental webrev (over the rebase):
>>>> http://cr.openjdk.java.net/~eosterlund/8195142/webrev.00_01/
>>>>
>>>> This new version has run through hs-tier1-5 and jdk-tier1-3 without 
>>>> any issues.
>>>>
>>>> Thanks,
>>>> /Erik
>>>>
>>>> On 2018-01-17 13:54, Erik Österlund wrote:
>>>>> Hi,
>>>>>
>>>>> Today, both Parallel, CMS and Serial share the same code for its 
>>>>> card marking barrier. However, they have different requirements 
>>>>> how to manage its card tables by the GC. And as the card table 
>>>>> itself is embedded as a part of the CardTableModRefBS barrier set, 
>>>>> this has led to an unnecessary inheritance hierarchy for 
>>>>> CardTableModRefBS, where for example CardTableModRefBSForCTRS and 
>>>>> CardTableExtension are CardTableModRefBS subclasses that do not 
>>>>> change anything to do with the barriers.
>>>>>
>>>>> To clean up the code, there should really be a separate CardTable 
>>>>> hierarchy that contains the differences how to manage the card 
>>>>> table from the GC point of view, and simply let CardTableModRefBS 
>>>>> have a CardTable. This would allow removing 
>>>>> CardTableModRefBSForCTRS and CardTableExtension and their 
>>>>> references from shared code (that really have nothing to do with 
>>>>> the barriers, despite being barrier sets), and significantly 
>>>>> simplify the barrier set code.
>>>>>
>>>>> This patch mechanically performs this refactoring. A new CardTable 
>>>>> class has been created with a PSCardTable subclass for Parallel, a 
>>>>> CardTableRS for CMS and Serial, and a G1CardTable for G1. All 
>>>>> references to card tables and their values have been updated 
>>>>> accordingly.
>>>>>
>>>>> This touches a lot of platform specific code, so would be 
>>>>> fantastic if port maintainers could have a look that I have not 
>>>>> broken anything.
>>>>>
>>>>> There is a slight problem that should be pointed out. There is an 
>>>>> unfortunate interaction between Graal and hotspot. Graal needs to 
>>>>> know the values of g1 young cards and dirty cards. This is queried 
>>>>> in different ways in different versions of the JDK in the 
>>>>> ||GraalHotSpotVMConfig.java file. Now these values will move from 
>>>>> their barrier set class to their card table class. That means we 
>>>>> have at least three cases how to find the correct values. There is 
>>>>> one for JDK8, one for JDK9, and now a new one for JDK11. Except, 
>>>>> we have not yet bumped the version number to 11 in the repo, and 
>>>>> therefore it has to be from JDK10 - 11 for now and updated after 
>>>>> incrementing the version number. But that means that it will be 
>>>>> temporarily incompatible with JDK10. That is okay for our own copy 
>>>>> of Graal, but can not be used by upstream Graal as they are given 
>>>>> the choice whether to support the public JDK10 or the JDK11 that 
>>>>> does not quite admit to being 11 yet. I chose the solution that 
>>>>> works in our repository. I will notify Graal folks of this issue. 
>>>>> In the long run, it would be nice if we could have a more solid 
>>>>> interface here.
>>>>>
>>>>> However, as an added benefit, this changeset brings about a 
>>>>> hundred copyright headers up to date, so others do not have to 
>>>>> update them for a while.
>>>>>
>>>>> Bug:
>>>>> https://bugs.openjdk.java.net/browse/JDK-8195142
>>>>>
>>>>> Webrev:
>>>>> http://cr.openjdk.java.net/~eosterlund/8195142/webrev.00/
>>>>>
>>>>> Testing: mach5 hs-tier1-5 plus local AoT testing.
>>>>>
>>>>> Thanks,
>>>>> /Erik
>>>>
>>
>