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

[Coleen Phillimore]

Yes, this looks great.   If you hg commit it, hg export the patch, I 
will send it through JPRT for you.  Unless you have another sponsor.
Thanks,
Coleen

On 02/26/2013 01:11 PM, Stefan Karlsson wrote:
> 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.
>>>>>>
>