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

Fri Feb 23 09:31:36 UTC 2018

Hi Vladimir,

Thank you for the review.

/Erik

On 2018-02-22 19:41, Vladimir Kozlov wrote:
> On 2/22/18 10:30 AM, Vladimir Kozlov wrote:
>> Thank you, Erik
>>
>> I am old C++ guy and using template for just casting is overkill to 
>> me. You still specify <type> when you can just use cast (type). So 
>> what benefit template has in this case?
>
> Never mind my comment - I missed reinterpret_cast<> you need to cast a 
> pointer to basic types.
>
> Changes are good.
>
> Thanks,
> Vladimir
>
>>
>> Otherwise looks good.
>>
>> Thanks,
>> Vladimir
>>
>> On 2/22/18 3:45 AM, Erik Österlund wrote:
>>> Hi Vladimir,
>>>
>>> Thank you for having a look at this.
>>>
>>> I created some utility functions in ci/ciUtilities.hpp to get the 
>>> card table:
>>>
>>> jbyte* ci_card_table_address()
>>> template<T> T ci_card_table_address_as()
>>>
>>> The compiler code has been updated to use these helpers instead to 
>>> fetch the card table in a consistent way.
>>>
>>> Hope this is kind of what you had in mind?
>>>
>>> New full webrev:
>>> http://cr.openjdk.java.net/~eosterlund/8195142/webrev.03/
>>>
>>> New incremental webrev:
>>> http://cr.openjdk.java.net/~eosterlund/8195142/webrev.02_03/
>>>
>>> Thanks,
>>> /Erik
>>>
>>> On 2018-02-21 18:12, Vladimir Kozlov wrote:
>>>> Hi Erik,
>>>>
>>>> I looked on compiler and aot changes. I noticed repeated sequence 
>>>> in several files to get byte_map_base()
>>>>
>>>> +  BarrierSet* bs = Universe::heap()->barrier_set();
>>>> +  CardTableModRefBS* ctbs = barrier_set_cast<CardTableModRefBS>(bs);
>>>> +  CardTable* ct = ctbs->card_table();
>>>> +  assert(sizeof(*(ct->byte_map_base())) == sizeof(jbyte), "adjust 
>>>> this code");
>>>> +  LIR_Const* card_table_base = new LIR_Const(ct->byte_map_base());
>>>>
>>>> But sometimes it has the assert (graphKit.cpp) and sometimes does 
>>>> not (aotCodeHeap.cpp).
>>>>
>>>> Can you factor this sequence into one method which can be used in 
>>>> all such places?
>>>>
>>>> Thanks,
>>>> Vladimir
>>>>
>>>> On 2/21/18 3: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
>>>>>>>
>>>>>
>>>