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

Vladimir Kozlov vladimir.kozlov at oracle.com
Thu Feb 22 18:41:02 UTC 2018 

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

More information about the hotspot-dev mailing list