<div dir="ltr"><div dir="ltr">Thanks for the review! Comments inline. And new webrev at <a href="http://cr.openjdk.java.net/~jcbeyler/8209802/webrev.01/">http://cr.openjdk.java.net/~jcbeyler/8209802/webrev.01/</a><div><br><div class="gmail_quote"><div dir="ltr">On Tue, Oct 23, 2018 at 4:43 AM Thomas Schatzl <<a href="mailto:thomas.schatzl@oracle.com" target="_blank">thomas.schatzl@oracle.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Derek,<br>
<br>
On Fri, 2018-10-19 at 15:04 -0700, Derek Thomson wrote:<br>
> Hi all,<br>
> <br>
> I saw this bug and thought I could take a stab at it. I tweaked the<br>
> #ifs to make it work for -g1gc, then decided to keep going and make<br>
> sure all valid GC exclusions work.<br>
> <br>
> It's worth noting that you can't say '-serialgc' without also using<br>
> '-cmsgc', as CMS relies on serial.<br>
> <br>
> Could someone review it please?<br>
> <br>
> Webrev: <a href="http://cr.openjdk.java.net/~jcbeyler/8209802/webrev.00/" rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~jcbeyler/8209802/webrev.00/</a><br>
> Bug: <a href="https://bugs.openjdk.java.net/browse/JDK-8209802" rel="noreferrer" target="_blank">https://bugs.openjdk.java.net/browse/JDK-8209802</a><br>
<br>
  - wouldn't it be more useful to tweak the makefiles so that setting<br>
-serialgc implies -cmsgc, and conversely, +cmsgc implies +serialgc?<br></blockquote><div><br></div><div>Curiously there's a test in hotspot.m4, line 327, that explicitly forbids this combination.</div><div>I was going to add this exactly just now. It turns out it only checks if you explicitly</div><div>say "-serialgc,cmsgc," and not by implication with simply "-serialgc". Not sure how to</div><div>make a check that does block the implied case, or if it's possible.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Or, for me as fine, if conflicting options are set, return with an<br>
error.<br>
<br>
Because it looks like with this change, with -serialgc +cmsgc you will<br>
get a non-functioning CMS anyway. I.e. generate a nonsensical<br>
situation.<br>
<br>
This removes a lot of #ifdef SERIALGC I think.<br>
<br>
I am actually not sure it is actually possible to compile (and<br>
succesfully run any of the "old" gcs) without serial gc at the moment.<br></blockquote><div><br></div><div>I'll let others comment on how far to take this i.e. do we remove the option</div><div>to build without serialgc entirely. I'd prefer to be minimal for now. It at least</div><div>builds with all valid combinations.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
  - the friend declaration in heapRegion.hpp can be removed instead of<br>
ifdef'ed. G1 does not use these methods. HeapRegion is still a<br>
CompactibleSpace/Space, but it actually should not any more.<br>
<br>
(And applying the Serial gc full gc to G1 will crash the VM)<br>
<br>
Anyway, for this change, just remove the friend declaration completely.<br></blockquote><div><br></div><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
  - in jfr.cpp, I would prefer the ifdef's completely removed the<br>
TRACE_REQUEST_FUNC(G1HeapRegionInformation), if possible. Otherwise, I<br>
think it is better to remove starting from the body of this method.<br></blockquote><div><br></div><div>Sorry, did you mean jfrPeriodic.cpp? In which case not sure what method you mean here. Can you clarify for me please?</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
  - in jfrType.cpp the question is if compiling without g1 could remove<br>
the entire declaration of G1HeapRegionTypeConstant.<br>
<br>
Cc'ing <a href="mailto:hotspot-jfr-dev@openjdk.java.net" target="_blank">hotspot-jfr-dev@openjdk.java.net</a> as the jfr devs might have an<br>
idea how to accomplish that.<br></blockquote><div><br></div><div>Sure.</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Thanks,<br>
  Thomas<br>
<br>
<br>
</blockquote></div></div></div></div>