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

Lindenmaier, Goetz goetz.lindenmaier at sap.com
Mon Feb 26 14:07:26 UTC 2018 

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

More information about the hotspot-dev mailing list