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

[Stefan Karlsson]

Looks good.

thanks,
StefanK

On 2013-02-26 18:35, Volker Simonis wrote:
> Hi Coleen,
>
> thanks for the info. That of course simplifies things considerably.
> I've moved the two refcount functions from "symbol.hpp" to
> "symbol.cpp" and included "atomic.inline.hpp" into "symbol.cpp".
> Here's the new webrev:
>
> http://cr.openjdk.java.net/~simonis/webrevs/8008959/
>
> Thank you and best regards,
> Volker
>
> On Tue, Feb 26, 2013 at 4:16 PM, Coleen Phillimore
> <coleen.phillimore at oracle.com> wrote:
>> I have a comment about the increment and decrement refcount functions.
>>
>>
>> On 02/26/2013 07:45 AM, Stefan Karlsson 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.
>>>>>
>> I think these functions are not performance critical.   They used to be in
>> the .cpp file and when I moved them to the .hpp file hoping they'd improve
>> performance, I got nothing.
>>
>> I think they can be moved back to .cpp file and that'd be cleaner.
>>
>> Thanks,
>> Coleen
>>
>>
>>>> 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?
>>>
>>>> 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.
>>>>>