RFR (tedious) 8216022: Use #pragma once

Thu Jan 3 14:55:11 UTC 2019

On 1/3/19 2:48 AM, Kim Barrett wrote:
>> On Jan 2, 2019, at 9:31 PM, coleen.phillimore at oracle.com wrote:
>>
>>
>> Here is the webrev and bug link.
>>
>> open webrev at http://cr.openjdk.java.net/~coleenp/8216022.01/webrev
>> bug link https://bugs.openjdk.java.net/browse/JDK-8216022
>>
>> On 1/2/19 9:16 PM, coleen.phillimore at oracle.com wrote:
>>> Summary: change include guards to #pragma once, except in generated header files.
>>>
>>> Tested with mach5 for linux-x64{-debug}, solaris-sparc, macosx-x64, windows-x64, built aarch64 with cross compiler, and zero.
>>>
>>> Ran tier1 and 2 tests.
>>>
>>> The webrev is huge but there are only 3 lines changed in each header file.  So click on the patch.
>>>
>>> I'll update the copyright headers with a script with the commit. Also, will do this after the shenandoah copyright headers are fixed.
>>>
>>> Adrian: I included you to check your platforms.
>>>
>>> Happy New Year!
>>> Coleen
> I think we shouldn't make this change without considering the impact
> of the following bug:
>
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58770
> GCC very slow compiling with #pragma once
>
> According to comment 3, using "#pragma once" introduces N^2 behavior
> on the number of included files, because the duplicate check uses a
> list rather than a hashtable.
>
> At the very least, a performance comparison should be made to find out
> what the impact of that bug is.

I did a performance test of the build with and without this change and 
it took the same time to build hotspot on my machine (fastdebug) with 
and without.  Both without precompiled headers.

without:
STARTED:Thu Jan  3 08:52:45 EST 2019
      DONE:Thu Jan  3 08:57:20 EST 2019
with pragma once:
STARTED:Thu Jan  3 08:58:44 EST 2019
      DONE:Thu Jan  3 09:03:19 EST 2019

It may be that more than 1700 include files might cause a slower build.  
We've done a good job fixing our includes to not include everything.

>
> A couple of comments regarding the problems with existing #include
> guards, and continuing to use #include guards instead of "#pragma once":
>
> (1) I wonder if a round trip using the following could fix the
> #include guards:
> https://github.com/cgmb/guardonce
> Utilities for converting from C/C++ include guards to #pragma once and
> back again.
>
> (2) For maintaining #include guards, clang has -Wheader-guard, which
> warns about mismatches between the #ifndef name and the #define.  Of
> course, that doesn't solve all the issues with #include guards.

Right.  It doesn't solve the ongoing maintenance problem of always 
having to edit the guards if you change a files location.

I was thinking this isn't worth doing, but maybe it is, if we don't use 
pragma once, it would be nice if the file names matched at least for one 
point in time.
>
> And now for a more general discussion of "#pragma once".
>
> The question of whether to use "#pragma once" comes up pretty often in
> various open source projects and Q&A sites.  So far, I haven't found
> any large cross-platform projects that provide headers to clients that
> have decided to go that way.  But most of HotSpot is self-contained,
> which limits exposure to some of the issues below.

Yes, the #pragma once issues in the general discussion weren't that 
convincing to me.  Maybe a general project in the general case can't use 
pragma once, didn't seem to apply to us and our build.  Maybe someone 
from our build system can comment, but I don't think we have symbolic 
links or hard links? in our build that would mess this up, or other issues.
>
> An exception to that are the C headers providing interfaces to the VM,
> e.g. the files in src/hotspot/share/include.  This suggests that
> perhaps these files should perhaps be excluded from the change, since
> they get used in whatever build environment a client uses.  It also
> suggests the #include guard names for these files need careful
> namespace consideration, which clearly didn't happen with cds.h.

I can revert these.  I was on the fence about these files.

>
> For many compilers there isn't a good performance argument for using
> "#pragma once".  gcc (for a long time), clang (always?), VS2015+ all
> do the #include guard optimization.  (I think Solaris Studio might
> still not?  And I have no idea about XLC++.)
>
> So the primary question seems to be the reduced clutter and avoidance
> of mistakes in #include guards, vs the possibility of cases where
> "#pragma once" doesn't work properly.
>
> Here's a list of some of the discussions I found:
>
> https://lists.boost.org/Archives/boost/2018/11/244423.php
> https://lists.qt-project.org/pipermail/development/2018-October/067452.html
> https://lists.qt-project.org/pipermail/development/2018-January/063932.html
> https://stackoverflow.com/questions/1143936/pragma-once-vs-include-guards
> https://www.reddit.com/r/cpp/comments/4cjjwe/come_on_guys_put_pragma_once_in_the_standard/d1j04te/
>
> The main argument against "#pragma once" (besides being non-standard,
> so possibly not sufficiently portable, though we think all platforms
> supported by HotSpot have this feature) is that it is "unreliable".
> Unfortunately, details are hard to come by.

Yeah, the details seemed vague and I was unconvinced that they were 
applicable.  The reward seems greater than the risk.
>
> I've seen claims that combining "#pragma once" with precompiled
> headers can cause problems, though the fact that Visual Studio has
> long supported both and they are commonly used together argues
> contrary.  But perhaps there are additional factors needed for
> problems to arise, and those don't happen on Windows?
>
> My impression is that having sources spread across different file
> systems might be a source of problems, possibly in conjunction with
> other factors.  Before you say "multiple file systems" is not a
> possible configuration for JDK builds, consider an out-of-tree build
> with the source and build directory on different file systems (and
> remember that our generated sources are in the build directory).
>
> I've seen suggestions that network file systems can also mess things
> up, though I didn't find details.
>
> I think an additional factor that might be relevant is the typical
> (but not specified by the standard) behavior of #include "..." first
> searching with respect to the current directory.  I think this is at
> least potentially a concern for JDK builds on (perhaps odd) file
> system configurations.
>
> Some examples are discussed in the following messages.  Having a
> bind-mount involved can mess things up, for example.  I don't know if
> that's a realistic scenario for building the JDK or HotSpot.
> https://lists.qt-project.org/pipermail/development/2018-October/067467.html
> https://lists.qt-project.org/pipermail/development/2018-October/067471.html
>
> So there seems to be some risk with this change that it will result in
> build failures or bad builds in someone's build environment, but it is
> hard to characterize what a problematic build environment looks like,
> so hard to know how "reasonable" or "sane" such a build environment
> might be.  Local testing is obviously inadequate for this change.
> Even running it through the Oracle build and test system doesn't seem
> sufficient to me.  Having it checked by the various known build farms
> (SAP, Debian, Red Hat, maybe others) seems called for.  It's good that
> the RFR specifically called out Debian to be checked.
>
Right.  I did copy Adrian because I think he has a Debian build. I'm not 
sure what to look for as a "bad build".  I would hope it would be 
catastrophic.  How do we evaluate any build changes?

Thank you for this detailed reply.  Can you put this in the bug report, 
so it doesn't get lost (and is more easily searchable)?

Thanks,
Coleen