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

[Erik Österlund]

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