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

Thu Feb 22 11:13:42 UTC 2018

Hi Coleen,

Okay great. Thanks for the review.

/Erik

On 2018-02-21 17:52, coleen.phillimore at oracle.com wrote:
>
>
> 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
>>>>>>
>>>>
>>>
>>
>