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

Fri Feb 23 10:21:31 UTC 2018

Hi Erik,

Thank you for the review. I will apply your proposed tweaks before pushing.

Thanks,
/Erik

On 2018-02-23 11:15, Erik Helin wrote:
>
>
> On 02/21/2018 12:33 PM, 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/
>
> The changes looks good, just a few very minor nits:
>
> - g1CollectedHeap.hpp:
>   please make the method card_table in G1CollectedHeap const, as in:
>   G1CardTable* card_table() const {
>     return _card_table;
>   }
>
> - g1CollectedHeap.cpp:
>   when you are changing methods in G1CollectedHeap, and have access to a
>   private field, please use the field instead of the getter. For
>   example:
>
>   + _card_table->initialize(cardtable_storage);
>
>   instead of:
>
>   +  card_table()->initialize(cardtable_storage);
>
> - stubGenerator_ppc.cpp
>   maybe add a space before the const qualifier?
>
>   + CardTableModRefBS*const ctbs =
>   + CardTable*const ct =
>
>   That is, change the above to:
>
>   + CardTableModRefBS* const ctbs =
>   + CardTable* const ct =
>
>   This is just my personal preference, but the code gets a bit dense
>   otherwise IMHO :)
>
> I don't need to see a new webrev for the above changes, just please do 
> these changes before you push. I also had a look at the patch after 
> the comments from Coleen and Vladimir, and it looks good. Reviewed 
> from my part.
>
> Thanks,
> Erik
>
>> 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
>>>>
>>