RFR(M): Fix non-PCH build on Linux, Windows and MacOS X

Tue Feb 26 06:29:00 PST 2013

On Tue, Feb 26, 2013 at 1:45 PM, Stefan Karlsson
<stefan.karlsson at oracle.com> wrote:
> On 02/26/2013 12:59 PM, Volker Simonis wrote:
>>
>> Hi Stefan,
>>
>> thank you for the review. Please see my comments inline:
>>
>> On Tue, Feb 26, 2013 at 10:03 AM, Stefan Karlsson
>> <stefan.karlsson at oracle.com> wrote:
>>>
>>> Volker,
>>>
>>> Thanks for fixing this. See inlined:
>>>
>>>
>>> On 02/25/2013 07:09 PM, Volker Simonis wrote:
>>>>
>>>> Recent changes have broken the no-PCH build again.
>>>>
>>>> The fix requires some bigger changes this time but before going into
>>>> the details - PLEASE, PLEASE add a no-PCH build to your JPRT (or
>>>> nightly build or whatever) system to catch such errors. On platforms
>>>> which don't support PCH (i.e. Solaris) the normal build 'naturally'
>>>> works and I would consider it 'good practice' for a project to be able
>>>> to build without such an esoteric:) feature like precompiled headers.
>>>> Actually this only means that all the dependencies of a compilation
>>>> unit are explicitly recorded in that compilation unit (which I'd
>>>> consider self-evident).
>>>>
>>>> Tested 'product' and 'jvmg' builds on Linux/x86_64, Linux/x86_32,
>>>> Windows/x86_64, Solaris/Sparc_64, Solaris/Sparc_32, and MacOS
>>>> X/x86_64.
>>>>
>>>> Please be so kind to open a BugID for this issue and review the change:
>>>>
>>>> http://cr.openjdk.java.net/~simonis/webrevs/fix_nonPCH_build/
>>>
>>>
>>> BugID created: JDK-8008959.
>>>
>>>
>>>> Regards,
>>>> Volker
>>>>
>>>> PS: following some details of this change:
>>>>
>>>> The following places were easy to fix (just include the appropriate
>>>> ".inline.hpp" files):
>>>>
>>>> src/share/vm/oops/symbol.hpp
>>>
>>>
>>> I think we should strive to avoid including .inline.hpp files in .hpp
>>> files.
>>> The inline.hpp files tend to bring a lot of extra dependencies that are
>>> exposed to the includers of the .hpp  file.
>>>
>>> Could you move Symbol::increment_refcount() and
>>> Symbol::decrement_refcount
>>> into a symbol.inline.hpp file instead? This will have some ripple
>>> effects,
>>> but I think it's worth it.
>>>
>> Generally, I'm very much in favour of your proposal (see my previous
>> mail). However in this case that would require huge changes. I've just
>> checked that Symbol::increment_refcount() and
>> Symbol::decrement_refcount() are used in 22 files (17 .cpp and 5 .hpp
>> files). For the .cpp files it would be easy - just add an include
>> statement for the new "symbol.inline.hpp". But for the 5 header files
>> you'd get another 6 functions which would have to be moved to
>> corresponding (potentially new) .inline.hpp files. And then
>> transitively ...
>
>
> These are the functions I could find:
>
> dictionary.hpp:
>   SymbolPropertyTable::new_entry
>   SymbolPropertyTable::free_entry
>
> symbolTable.hpp
>   TempNewSymbol::operator=
>   TempNewSymbol::clear
>
> loaderConstraints.hpp:
>   LoaderConstraintEntry::set_name
>
> placeloaders.hpp
>   ~FieldArrayInfo
>
> The easiest would be to just move the implementation to corresponding cpp
> file. Then you wouldn't have to change any other functions. Could someone in
> the Runtime team say if these functions are in a performance critical path
> and actually need to be inlined?
>

OK, I'll wait for another reviewer and if she/he agrees, I'll move
Symbol::increment_refcount() and Symbol::decrement_refcount() into the
new file symbol.inline.hpp and their callers from the .hpp files
mentioned above into the corresponding  .cpp files.

Thank you and best regards,
Volker

>
>>
>> So while I still think that would be a good change in general, I think
>> it is out of scope of this change which only wants to fix the non-PCH
>> build. As I wrote, there's already a RFE for the "inline include
>> problem": "8003990 : Clean up inline #includes" -
>> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8003990. But as
>> this small example shows, such a kind of clean up will be disruptive
>> and only makes sense if it is done "at a stretch" because it will
>> probably touch all the files and create a lot of new files (i.e.
>> .inline.hpp files).
>>
>> That would be comparable to the includeDB change and it should be
>> probably combined with the introduction of "shadow" include files
>> which bundle the inclusion of os/cpu dependent files in order to avoid
>> the current #ifdef cascades. Maybe we should file a JEP for this?
>>
>> So finally, what do you think - should I really move
>> Symbol::increment_refcount() and Symbol::decrement_refcount to a newly
>> created  symbol.inline.hpp?
>
>
> I think you should do it, if you get the permission from the Runtime team to
> move the functions mentioned above to the cpp files.
>
> thanks,
> StefanK
>
>
>>
>> Regards,
>> Volker
>>
>>> Otherwise, this looks good to me.
>>>
>>> thanks,
>>> StefanK
>>>
>>>
>>>> - uses Atomic inline functions (inc() and dec()) without including
>>>> "runtime/atomic.inline.hpp"
>>>>
>>>> src/share/vm/memory/allocation.inline.hpp
>>>>
>>>> - uses Atomic inline functions (load() and store()) without including
>>>> "runtime/atomic.inline.hpp"
>>>>
>>>> src/os/windows/vm/decoder_windows.cpp
>>>>
>>>> - uses Arguments::get_java_home() without including
>>>> "runtime/arguments.hpp"
>>>>
>>>> src/os_cpu/linux_x86/vm/orderAccess_linux_x86.inline.hpp
>>>> src/os_cpu/bsd_x86/vm/orderAccess_bsd_x86.inline.hpp
>>>> src/os_cpu/solaris_x86/vm/orderAccess_solaris_x86.inline.hpp
>>>> src/os_cpu/solaris_sparc/vm/orderAccess_solaris_sparc.inline.hpp
>>>> src/os_cpu/windows_x86/vm/orderAccess_windows_x86.inline.hpp
>>>>
>>>> - use the Atomic inline functions (load() and store()) without
>>>> including "runtime/atomic.inline.hpp"
>>>>
>>>> src/os_cpu/bsd_x86/vm/atomic_bsd_x86.inline.hpp
>>>> src/os_cpu/linux_x86/vm/atomic_linux_x86.inline.hpp
>>>> src/os_cpu/linux_zero/vm/atomic_linux_zero.inline.hpp
>>>> src/os_cpu/linux_sparc/vm/atomic_linux_sparc.inline.hpp
>>>> src/os_cpu/windows_x86/vm/atomic_windows_x86.inline.hpp
>>>> src/os_cpu/bsd_zero/vm/atomic_bsd_zero.inline.hpp
>>>> src/os_cpu/solaris_sparc/vm/atomic_solaris_sparc.inline.hpp
>>>> src/os_cpu/solaris_x86/vm/atomic_solaris_x86.inline.hpp
>>>>
>>>> - include "orderAccess_<os>_<cpu>.inline.hpp" without using any
>>>> OrderAccess method. I removed this includes to avoid a cyclic
>>>> dependency between "orderAccess_<os>_<cpu>.inline.hpp" and
>>>> "atomic_<os>_<cpu>.inline.hpp" via "atomic.inline.hpp"
>>>>
>>>> src/os_cpu/linux_x86/vm/orderAccess_linux_x86.inline.hpp
>>>> src/os_cpu/bsd_x86/vm/orderAccess_bsd_x86.inline.hpp
>>>> src/os_cpu/windows_x86/vm/orderAccess_windows_x86.inline.hpp
>>>>
>>>> - use os::is_MP() without including os.hpp
>>>>
>>>> The following fix was a little more cumbersome:
>>>>
>>>> src/os/bsd/vm/os_bsd.hpp
>>>> src/os/linux/vm/os_linux.hpp
>>>>
>>>> - The methods "set_suspended()" and "clear_suspended()" use
>>>> Atomic::cmpxchg without "atomic.inline.hpp" being included.
>>>> Unfortunately "os_bsd.hpp" and "os_linux.hpp" are not self-contained
>>>> files. They are themselves included into "os.hpp" right into the class
>>>> definition of the "os" class. It is therefore not possible to simply
>>>> include "atomic.inline.hpp" into "os_bsd.hpp" and "os_linux.hpp".
>>>>
>>>> - The natural solution was to transfer the implementations of
>>>> "set_suspended()" and "clear_suspended()" to the corresponding
>>>> "os_bsd.inline.hpp" and "os_linux.inline.hpp" files. Unfortunately
>>>> this is not trivial as well, because both methods use preprocessor
>>>> defined constants from their enclosing class "SuspendResume" which are
>>>> defined right before there definition and undefined right thereafter.
>>>> They are therefore not available in "os_bsd.inline.hpp" and
>>>> "os_linux.inline.hpp".
>>>>
>>>> - The solution for the last problem is to change the mentioned
>>>> preprocessor constants into an C++ enumeration which also results in a
>>>> nicer overall code. The drawback is that all the users of the
>>>> preprocessor constants have to be updated to use the new, qualified
>>>> enum-values (i.e. 'os::Bsd::SuspendResume::SR_CONTINUE' instead of
>>>> 'SR_CONTINUE'). It's a little more verbose but definitely a lot
>>>> cleaner this way.
>>>>
>>>> I also did the following clean-up because I touched the corresponding
>>>> files anyway:
>>>>
>>>> src/os_cpu/windows_x86/vm/orderAccess_windows_x86.inline.hpp
>>>>
>>>> - remove "#pragma warning(disable: 4035)" because there don't seem to
>>>> be any missing return statement in this file anymore
>>>>
>>>> src/os/bsd/vm/os_bsd.inline.hpp
>>>> src/os/linux/vm/os_linux.inline.hpp
>>>> src/os/solaris/vm/os_solaris.inline.hpp
>>>> src/os/windows/vm/os_windows.inline.hpp
>>>>
>>>> - include both "atomic.hpp" and "atomic.inline.hpp". This is
>>>> unnecessary, because "XXX.inline.hpp" always includes "XXX.hpp"
>>>>
>>>> Finally, I've also updated the copyright line to 2013 in every file
>>>> touched by this change.
>>>
>>>
>