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

Tue Feb 26 09:35:11 PST 2013

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