RFR(M): 8038498: Fix includes and C inlining after 8035330

[Lindenmaier, Goetz]

Hi David, 

Well, the only reason for .inline.hpp files is to break include cycles.
If you include them in the corresponding .hpp files, you reintroduce
the cycles.

If you want encapsulation, don't use inline methods.

> There's a flow 
> on affect that an inline function can't use other inline functions 
> unless they all get moved to .inline.hpp files.
That's exactly the rule that should be enforced.

Yes, .inline.hpp files may include .inline.hpp files.  That will 
not cause cycles.  This way .hpp files will all be placed in the
beginning of a (preprocessed) cpp file, then the .include.hpp files
and last the code of the .cpp file.

Best regards,
  Goetz.



-----Original Message-----
From: David Holmes [mailto:david.holmes at oracle.com] 
Sent: Dienstag, 1. April 2014 09:36
To: Lindenmaier, Goetz; hotspot-dev at openjdk.java.net; hotspot-gc-dev at openjdk.java.net
Subject: Re: RFR(M): 8038498: Fix includes and C inlining after 8035330

On 1/04/2014 5:30 PM, Lindenmaier, Goetz wrote:
> Hi David,
>
>> Personally I don't see why a cpp file should have to be aware that a
>> member function it uses just happens to have been defined in an
>> inline.hpp file by its author. Seems to break encapsulation to me. :(
> Because the .cpp file will not compile if the .inline.hpp header with
> the implementation is not visible during compilation.

Understood - but there is more than one way to make the implementation 
visible to the cpp file eg cpp include .hpp and .hpp includes 
.inline.hpp (I suppose that is a "bad" thing to do?)

And that still doesn't change the fact it is breaking encapsulation :)

>> I see 277 occurrences of this in the existing OpenJDK hotspot code, so I
> There are a lot of .hpp files with include methods that don't have
> a corresponding .inline.hpp file.  Like taskqueue.hpp, which includes
> the atomic headers.  Would be nice if this was cleaned up.
> Also, oop.inline.hpp often makes problems, because methods defined
> there are used in .hpp files, which can not include oop.inline.hpp
> as it causes cycles.  If you include that header in a cpp file, you also have
> to include oop.inline.hpp, which is really unintuitive.

I find the inline situation to be unintuitive in general. There's a flow 
on affect that an inline function can't use other inline functions 
unless they all get moved to .inline.hpp files. I hope it is okay for 
one .inline.hpp file to include another one? Otherwise you have to 
expose the internal implementation detail of one class to its clients so 
they know which set of .inline.hpp files have to be included!

Cheers,
David

> Best regards,
>    Goetz.
>
> -----Original Message-----
> From: David Holmes [mailto:david.holmes at oracle.com]
> Sent: Dienstag, 1. April 2014 08:03
> To: Lindenmaier, Goetz; hotspot-dev at openjdk.java.net; hotspot-gc-dev at openjdk.java.net
> Subject: Re: RFR(M): 8038498: Fix includes and C inlining after 8035330
>
> Hi Goetz,
>
> Not a review ...
>
> On 31/03/2014 10:42 PM, Lindenmaier, Goetz wrote:
>> Hi,
>>
>> could I please get reviews for this change?  I please also need a sponsor.
>> I would like to get this into 8u20, as 8036330 was downported, too.
>>
>> I'd like to make the point of this change clear once more, independent
>> of which compiler does what.
>>
>> I think we agree that basically
>>     - a file should include all headers that define symbols or include
>>       method bodies used in that file
>
> Personally I don't see why a cpp file should have to be aware that a
> member function it uses just happens to have been defined in an
> inline.hpp file by its author. Seems to break encapsulation to me. :(
>
>>     - a .hpp header should not include a .inline.hpp header
>
> I see 277 occurrences of this in the existing OpenJDK hotspot code, so I
> don't think we can take this as a general rule either.
>
> Note I'm not saying this fix isn't right or needed, just that the
> "rules" you are trying to embody don't seem to generally apply.
>
> David
> -----
>
>> This is violated
>>
>> 1.)    by defining
>>    template <class T> void immediate_rs_update(HeapRegion* from, T* p, int tid) {
>>      if (!from->is_survivor()) {
>>         _g1_rem->par_write_ref(from, p, tid);
>>       }
>> }
>>    in g1CollectedHeap.hpp, as par_write_ref in declared 'inline' and only defined in
>>       g1RemSet.inline.hpp - which is not included and should not be included
>>       in g1CollectedHeap.hpp.
>> 2. by including g1CollectedHeap.inline.hpp in sparsePRT.hpp
>>
>> This change fixes these two points by moving functions into .inline.hpp files
>> and fixing the include.
>>
>> Best regards,
>>     Goetz.
>>
>>
>>
>>
>> From: Lindenmaier, Goetz
>> Sent: Donnerstag, 27. März 2014 17:46
>> To: hotspot-dev at openjdk.java.net; hotspot-gc-dev at openjdk.java.net
>> Subject: RFR(M): 8038498: Fix includes and C inlining after 8035330
>>
>> Hi,
>>
>> Please review and test this change.  I please need a sponsor:
>> http://cr.openjdk.java.net/~goetz/webrevs/8038498-incl/
>>
>> Change 8035330: Remove G1ParScanPartialArrayClosure and G1ParScanHeapEvacClosure
>> broke the dbg build on AIX. That's because do_oop_partial_array() is added in
>> a header, but requires inline function par_write_ref() through several inlined
>> calls. In some cpp files, like arguments.cpp, par_write_ref() is not defined
>> as the corresponding inline header is not included.  The aix debug VM does not start.
>>
>> This can be solved by including g1RemSet.inline.hpp in g1CollectedHeap.inline.hpp.
>>
>> Unfortunately this now causes a cyclic dependency that breaks the linux build.
>> A inline.hpp file is included ahead of a .hpp file, so that the code in the inline.hpp file
>> can not see the class declaration.
>> This is caused because g1CollectedHeap.inline.hpp is included in sparsePRT.hpp.
>> But .inline.hpp files never should be included in .hpp files.
>>
>> To resolve this, I changed this inlcude to g1CollectedHeap.hpp.  As consequence,
>> I had to move a row of functions to existing .inline.hpp files.
>>
>> I did debug, fastdebug and product builds on linux_ppc64, aix_ppc64,
>> sun_64, bsd_x86_64 and linux_x86_64 and tested that the VM starts up.
>>
>> Best regards,
>>     Goetz
>>