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

Wed Feb 21 13:17:09 UTC 2018

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