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

[Erik Helin]

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
- 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
- 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)
- in cardTable.hpp and cardTable.cpp:
   could you use `hg cp` when constructing these files from
   cardTableModRefBS.{hpp,cpp} so the history is preserved?

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
>