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

[Per Liden]

Hi,

On 11/2/18 12:05 AM, Derek Thomson wrote:
> Thanks for the review! Comments inline. And new webrev at
> http://cr.openjdk.java.net/~jcbeyler/8209802/webrev.01/

Looks like there's something wrong with this webrev, for example, the 
changes to heapRegion.hpp in

   http://cr.openjdk.java.net/~jcbeyler/8209802/webrev.01/jdk11.changeset

does not match those shown in:

 
http://cr.openjdk.java.net/~jcbeyler/8209802/webrev.01/src/hotspot/share/gc/g1/heapRegion.hpp.udiff.html


My comments here are based on what's in 
http://cr.openjdk.java.net/~jcbeyler/8209802/webrev.01/jdk11.changeset


src/hotspot/share/gc/cms/compactibleFreeListSpace.cpp
src/hotspot/share/gc/cms/compactibleFreeListSpace.hpp
src/hotspot/share/gc/serial/markSweep.inline.hpp
-----------------------------------------------------
Changes to these files should not be needed. This should instead be 
solved by having ./configure complaining, since building CMS without 
Serial is an invalid configuration.


src/hotspot/share/gc/g1/heapRegion.hpp
--------------------------------------
Remove the code, instead of commenting it out.


src/hotspot/share/gc/shared/genCollectedHeap.cpp
src/hotspot/share/jfr/periodic/jfrPeriodic.cpp
src/hotspot/share/jfr/recorder/checkpoint/types/jfrType.cpp
src/hotspot/share/jvmci/vmStructs_jvmci.cpp
------------------------------------------------
Changes to these files look ok.


cheers,
Per

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