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

Mon Feb 26 15:54:36 UTC 2018

Hi Goetz,

Thank you for letting me know. This builds with and without precompiled 
headers in our builds and I did not make any changes to graphKit.hpp and 
the affected code seemingly has nothing to do with the new CardTable 
class. Yet it does indeed not seem right and looks like a pre-existing 
include dependency problem. I will file a bug and fix this.

Thanks,
/Erik

On 2018-02-26 15:07, Lindenmaier, Goetz wrote:
> Hi Eric,
>
> your change seems to break the build without precompiled headers:
> opto/graphKit.hpp:760:32: error: CardTableModRefBS was not declared in this scope
>              && barrier_set_cast<CardTableModRefBS>(bs)->can_elide_tlab_store_barriers()
>
> This fixes it:
>
> --- a/src/hotspot/share/opto/graphKit.hpp	Mon Feb 26 14:36:14 2018 +0100
> +++ b/src/hotspot/share/opto/graphKit.hpp	Mon Feb 26 15:06:23 2018 +0100
> @@ -27,6 +27,7 @@
>   
>   #include "ci/ciEnv.hpp"
>   #include "ci/ciMethodData.hpp"
> +#include "gc/shared/cardTableModRefBS.hpp"
>   #include "opto/addnode.hpp"
>   #include "opto/callnode.hpp"
>   #include "opto/cfgnode.hpp"
>
> Best regards,
>    Goetz.
>
>> -----Original Message-----
>> From: hotspot-dev [mailto:hotspot-dev-bounces at openjdk.java.net] On Behalf
>> Of Erik Österlund
>> Sent: Freitag, 23. Februar 2018 11:22
>> To: Erik Helin <erik.helin at oracle.com>; hotspot-dev developers <hotspot-
>> dev at openjdk.java.net>
>> Subject: Re: RFR(L): 8195142: Refactor out card table from CardTableModRefBS
>> to flatten the BarrierSet hierarchy
>>
>> 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