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

Volker Simonis volker.simonis at gmail.com
Wed Feb 27 01:18:16 PST 2013


On Tue, Feb 26, 2013 at 7:22 PM, Coleen Phillimore
<coleen.phillimore at oracle.com> wrote:
>
> 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
>

Thank you Coleen, that would be great. Please find the patch attached
to this mail.

Could you pleas also mark
http://bugs.sun.com/view_bug.do?bug_id=8005084 as duplicate of this
issue. As Christian noticed earlier in this thread, it is the same
problem and it is also fixed by this change.

Regards,
Volker

>
> 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.
>>>>>>>
>>>>>>>
>>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 8008959.patch
Type: application/octet-stream
Size: 28645 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/attachments/20130227/6f3d0bd0/8008959-0001.patch 


More information about the hotspot-runtime-dev mailing list