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

Wed Feb 21 16:52:45 UTC 2018

On 2/21/18 9:53 AM, Erik Österlund wrote:
> 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?

Oh, sure, I'm fine if they're going to be removed (moved?).  I still 
think SAP should make sure their platforms build before you push if they 
have time though.
thanks,
Coleen

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