RFR (XS) 8209802: JFR failed to build when certain garbage collectors are disabled

[Derek Thomson]

Thanks for the review! Comments inline. And new webrev at
http://cr.openjdk.java.net/~jcbeyler/8209802/webrev.01/

On Tue, Oct 23, 2018 at 4:43 AM Thomas Schatzl <thomas.schatzl at oracle.com>
wrote:

> Hi Derek,
>
> On Fri, 2018-10-19 at 15:04 -0700, Derek Thomson wrote:
> > Hi all,
> >
> > I saw this bug and thought I could take a stab at it. I tweaked the
> > #ifs to make it work for -g1gc, then decided to keep going and make
> > sure all valid GC exclusions work.
> >
> > It's worth noting that you can't say '-serialgc' without also using
> > '-cmsgc', as CMS relies on serial.
> >
> > Could someone review it please?
> >
> > Webrev: http://cr.openjdk.java.net/~jcbeyler/8209802/webrev.00/
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8209802
>
>   - wouldn't it be more useful to tweak the makefiles so that setting
> -serialgc implies -cmsgc, and conversely, +cmsgc implies +serialgc?
>

Curiously there's a test in hotspot.m4, line 327, that explicitly forbids
this combination.
I was going to add this exactly just now. It turns out it only checks if
you explicitly
say "-serialgc,cmsgc," and not by implication with simply "-serialgc". Not
sure how to
make a check that does block the implied case, or if it's possible.


>
> Or, for me as fine, if conflicting options are set, return with an
> error.
>
> Because it looks like with this change, with -serialgc +cmsgc you will
> get a non-functioning CMS anyway. I.e. generate a nonsensical
> situation.
>
> This removes a lot of #ifdef SERIALGC I think.
>
> I am actually not sure it is actually possible to compile (and
> succesfully run any of the "old" gcs) without serial gc at the moment.
>

I'll let others comment on how far to take this i.e. do we remove the option
to build without serialgc entirely. I'd prefer to be minimal for now. It at
least
builds with all valid combinations.


>
>   - the friend declaration in heapRegion.hpp can be removed instead of
> ifdef'ed. G1 does not use these methods. HeapRegion is still a
> CompactibleSpace/Space, but it actually should not any more.
>
> (And applying the Serial gc full gc to G1 will crash the VM)
>
> Anyway, for this change, just remove the friend declaration completely.
>

Done.


>
>   - in jfr.cpp, I would prefer the ifdef's completely removed the
> TRACE_REQUEST_FUNC(G1HeapRegionInformation), if possible. Otherwise, I
> think it is better to remove starting from the body of this method.
>

Sorry, did you mean jfrPeriodic.cpp? In which case not sure what method you
mean here. Can you clarify for me please?


>
>   - in jfrType.cpp the question is if compiling without g1 could remove
> the entire declaration of G1HeapRegionTypeConstant.
>
> Cc'ing hotspot-jfr-dev at openjdk.java.net as the jfr devs might have an
> idea how to accomplish that.
>

Sure.



>
> Thanks,
>   Thomas
>
>
>