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

[Lindenmaier, Goetz]

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.

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

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
>