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

Derek Thomson dthomson at google.com
Thu Nov 1 23:05:56 UTC 2018

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

-------------- next part -------------- An HTML attachment was scrubbed... URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20181101/9e7e88da/attachment.htm>

More information about the hotspot-gc-dev mailing list