936912 - Build xpcom in unified mode

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[(no longer active)]

This helps with building the code faster, similar to the way that we currently build WebIDL bindings, IPDL protocols, etc.

Try run: https://tbpl.mozilla.org/?tree=Try&rev=44ccb4514136

Comment 1

[(no longer active)]

Attachment: Build xpcom in unified mode

Comment 2

[Nathan Froyd [:froydnj]]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #0)
> This helps with building the code faster, similar to the way that we
> currently build WebIDL bindings, IPDL protocols, etc.

Do you have any numbers showing that things build faster?

Comment 3

[(no longer active)]

(In reply to Nathan Froyd (:froydnj) from comment #2)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #0)
> > This helps with building the code faster, similar to the way that we
> > currently build WebIDL bindings, IPDL protocols, etc.
> 
> Do you have any numbers showing that things build faster?

On my machine, it makes a |make -j1 -C xpcom/| go down from 26s to 19s.  Note that this command also involves a bunch of linking, etc.  Eyeballing the stdout output, the C++ compilation with this patch almost takes no visible time, while you can see seconds pass by in the C++ compilation without this patch.  But I don't know how to get only compilation times here.

Comment 4

[Benjamin Smedberg]

What are the guidelines for deciding which directories should be built in unified mode and which shouldn't? Why wouldn't we do this across the entire tree, or for everything in general?

Comment 5

[(no longer active)]

My goal is to move to a point where we build most of the tree in unified mode.  The switch over is a manual process though since sometimes the contents of the translation units collide with each other.  It's often easy to fix those cases but sometimes it's not possible (see for example how plarena.h uses PL_ARENA_CONST_ALIGN_MASK, which means that two files using that macro cannot be unified together) and sometimes it's not possible because we may not want to fix the code to build in unified mode (Porting libjpg to unified builds would mean us having to change lots of its code, which is not great for an imported library.)

I have some patches landed and many others in flight, and will continue to work on this in my spare time.

Comment 6

[Benjamin Smedberg]

Is unified mode is significantly master than sending multiple C++ files to the compiler in one pass?

The downside of unified mode AIUI is that if you change a file, you're going to trigger rebuilding more things. Have we discussed in dev-builds whether that's the tradeoff we actually want? I don't remember seeing that we were going to expand unified builds to more of the tree before this bug.

Comment 7

[(no longer active)]

(In reply to comment #6)
> Is unified mode is significantly master than sending multiple C++ files to the
> compiler in one pass?

I'm not sure if you can do that with gcc and clang.  With Visual C++, I think you can do that, but I doubt that our current build system supports it.

> The downside of unified mode AIUI is that if you change a file, you're going to
> trigger rebuilding more things.

That's true.  Note that we currently have a maximum of 16 files per unified cpp file, so that situation is at least not as bad as it could be.

> Have we discussed in dev-builds whether that's
> the tradeoff we actually want? I don't remember seeing that we were going to
> expand unified builds to more of the tree before this bug.

I don't think so, but I have mostly stopped reading that mailing list.  I've been acting under the assumption that if you touch just one .cpp file, then the increase in the compilation time if that file is part of a unified build will be overshadowed by the link time, and if you touch things like headers, you're likely to trigger rebuilds of files especially ones in the same directory, which seems like a nice property for unified builds.

Comment 8

[Gregory Szorc [:gps]]

AIUI we've only really discussed unified mode for things like auto-generated sources and external projects [which rarely change].

If we're pursuing unified mode as a general solution (as Ehsan suggests), I'd like to have a dev.platform and/or dev.builds discussion about it. From the build system perspective, I don't think we care about unified vs regular too much. Although it may have implications on incremental build times in automation. IMO the bigger impact is felt by Gecko developers editing the .cpp files being built in unified mode. You are trading gains in clobber build times for losses in incremental build times. Perhaps we can make unified mode a build flag?

FWIW, I hypothesize that we're seeing significant compile time decreases in unified mode because the compiler spends so much time processing included files. All this redundant processing across separate compiler invocations really adds up. Mitigating include hell and using pre-compiled headers should minimize the benefits of unified mode. Perhaps compiling in unified mode is a necessary bridge until we get there? 

Let's take this to a mailing list.

Comment 9

[(no longer active)]

(In reply to comment #8)
> AIUI we've only really discussed unified mode for things like auto-generated
> sources and external projects [which rarely change].

That was only because it was easier to get those cases to work initially.

> If we're pursuing unified mode as a general solution (as Ehsan suggests), I'd
> like to have a dev.platform and/or dev.builds discussion about it. From the
> build system perspective, I don't think we care about unified vs regular too
> much. Although it may have implications on incremental build times in
> automation. IMO the bigger impact is felt by Gecko developers editing the .cpp
> files being built in unified mode. You are trading gains in clobber build times
> for losses in incremental build times. Perhaps we can make unified mode a build
> flag?

No you don't want to do that, otherwise people may do something which builds locally but will fail to build once they land it.

> FWIW, I hypothesize that we're seeing significant compile time decreases in
> unified mode because the compiler spends so much time processing included
> files. All this redundant processing across separate compiler invocations
> really adds up. Mitigating include hell and using pre-compiled headers should
> minimize the benefits of unified mode. Perhaps compiling in unified mode is a
> necessary bridge until we get there? 

Minimizing the include graph and unfied builds have different goals, the former tries to minimize the dependencies causing stuff to get rebuilt, the latter tries to build things faster.  FWIW I have measured between 6-15x faster builds in unified mode depending on the code in question.

> Let's take this to a mailing list.

It's not clear to me what we want to discuss.  Can someone else start the thread with a set of questions they'd like to see answers to?  I can jump on and try to answer the questions.

(Also, note that dev-platform would be the right forum for that discussion, as this has nothing to do with the build system.)

Comment 10

[Gregory Szorc [:gps]]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #9)

> > FWIW, I hypothesize that we're seeing significant compile time decreases in
> > unified mode because the compiler spends so much time processing included
> > files. All this redundant processing across separate compiler invocations
> > really adds up. Mitigating include hell and using pre-compiled headers should
> > minimize the benefits of unified mode. Perhaps compiling in unified mode is a
> > necessary bridge until we get there? 
> 
> Minimizing the include graph and unfied builds have different goals, the
> former tries to minimize the dependencies causing stuff to get rebuilt, the
> latter tries to build things faster.  FWIW I have measured between 6-15x
> faster builds in unified mode depending on the code in question.

I'm just saying that I *hypothesize* that a reason unified mode is so much faster is because the overhead of dealing with the excessive amount of included files. You are making the preprocessor, AST, and codegen compute the included files N times instead of 1. Unified should *always* be faster because of this property. But I think the difference is more pronounced than it could be because of our include hell problem. This is why fixing include hell can reduce clobber times. Precompiled headers can help for the same reasons. Unified compilation is effectively PCH - AST + 1 codegen (instead of N). I'll concede to not knowing if more time is spent in preprocessing, AST, or codegen. We can and should measure this!

At the end of the day, I can't say unified compilation isn't faster (it is). I'm mostly concerned about the developer tradeoffs.

> > Let's take this to a mailing list.
> 
> It's not clear to me what we want to discuss.  Can someone else start the
> thread with a set of questions they'd like to see answers to?  I can jump on
> and try to answer the questions.

I think you should make sure Gecko developers don't form a lynch mob and come after you for making their incremental build times worse :)

Comment 11

[Gregory Szorc [:gps]]

Let's compare CPU time for xpcom/base/nsMessageLoop.cpp with Clang:

0.074s          -E (preprocessor only)
0.256s (+0.182) -fsyntax-only (parser + type checking)
0.313s (+0.057) -S (most codegen)
0.313s          -c (+assembly into object file)

Not sure where I'm going with this. But this one file shows that we do spend a lot more time in AST than in codegen - ~3.2x more. Therefore anything we can do to speed up AST acquisition will benefit clobber compile times. Unified compilation, PCH, and reducing include hell will all do this. Unified sure looks like the easiest path.

Comment 12

[(no longer active)]

(In reply to comment #10)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #9)
> 
> > > FWIW, I hypothesize that we're seeing significant compile time decreases in
> > > unified mode because the compiler spends so much time processing included
> > > files. All this redundant processing across separate compiler invocations
> > > really adds up. Mitigating include hell and using pre-compiled headers should
> > > minimize the benefits of unified mode. Perhaps compiling in unified mode is a
> > > necessary bridge until we get there? 
> > 
> > Minimizing the include graph and unfied builds have different goals, the
> > former tries to minimize the dependencies causing stuff to get rebuilt, the
> > latter tries to build things faster.  FWIW I have measured between 6-15x
> > faster builds in unified mode depending on the code in question.
> 
> I'm just saying that I *hypothesize* that a reason unified mode is so much
> faster is because the overhead of dealing with the excessive amount of included
> files. You are making the preprocessor, AST, and codegen compute the included
> files N times instead of 1. Unified should *always* be faster because of this
> property. But I think the difference is more pronounced than it could be
> because of our include hell problem. This is why fixing include hell can reduce
> clobber times. Precompiled headers can help for the same reasons. Unified
> compilation is effectively PCH - AST + 1 codegen (instead of N). I'll concede
> to not knowing if more time is spent in preprocessing, AST, or codegen. We can
> and should measure this!

Yeah, this is mostly accurate.  Also please note that the amount of time that the semantic analysis and codegen phases take can also depend on the code being compiled, so you will see different ratios depending on what the code in question is doing.

> At the end of the day, I can't say unified compilation isn't faster (it is).
> I'm mostly concerned about the developer tradeoffs.
> 
> > > Let's take this to a mailing list.
> > 
> > It's not clear to me what we want to discuss.  Can someone else start the
> > thread with a set of questions they'd like to see answers to?  I can jump on
> > and try to answer the questions.
> 
> I think you should make sure Gecko developers don't form a lynch mob and come
> after you for making their incremental build times worse :)

Lol, ok, should I write an information post explaining what I'm up to and what the trade-offs are then? (and let people ask questions if they have them, of course)

Comment 13

[Gregory Szorc [:gps]]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #12)
> > I think you should make sure Gecko developers don't form a lynch mob and come
> > after you for making their incremental build times worse :)
> 
> Lol, ok, should I write an information post explaining what I'm up to and
> what the trade-offs are then? (and let people ask questions if they have
> them, of course)

Yes.

Comment 14

[Mike Hommey [:glandium]]

Note that on linux, unified building makes dwarf smaller. Which also means faster link times.

Comment 15

[(no longer active)]

(In reply to comment #13)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #12)
> > > I think you should make sure Gecko developers don't form a lynch mob and come
> > > after you for making their incremental build times worse :)
> > 
> > Lol, ok, should I write an information post explaining what I'm up to and
> > what the trade-offs are then? (and let people ask questions if they have
> > them, of course)
> 
> Yes.

Done.

Comment 16

[Trevor Saunders (:tbsaunde)]

(In reply to Mike Hommey [:glandium] from comment #14)
> Note that on linux, unified building makes dwarf smaller. Which also means
> faster link times.

only when you don't use -gsplit-dwarf right? although maybe less .dwo files -> faster gdb start up?

Comment 17

[Nathan Froyd [:froydnj]]

Comment on attachment 829881 [details] [diff] [review]
Build xpcom in unified mode

Review of attachment 829881 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, though I'm curious about your reasoning for the below.  I think the unconditional #undef approach is better.

::: xpcom/ds/nsPersistentProperties.cpp
@@ +8,5 @@
>  #include "nsCOMArray.h"
>  #include "nsUnicharInputStream.h"
>  #include "nsPrintfCString.h"
>  
> +#undef PL_ARENA_CONST_ALIGN_MASK

Is there a reason that you prefer straight #undefs here...

::: xpcom/io/nsInputStreamTee.cpp
@@ +19,5 @@
>  using namespace mozilla;
>  
> +#ifdef LOG
> +#undef LOG
> +#endif

...but check for a definition here?  Seems like you might as well #undef unconditionally.

Comment 18

[(no longer active)]

Hmm, I think that's actually wrong...  I may need to take those files out of the unified build.

IIRC some compilers will warn if the name being undefined is not previously defined, which is why I prefer the #ifdef guards like this.

Comment 19

[(no longer active)]

Attachment: Build xpcom in unified mode; r=froydnj

Comment 20

[(no longer active)]

Comment on attachment 8334258 [details] [diff] [review]
Build xpcom in unified mode; r=froydnj

Review of attachment 8334258 [details] [diff] [review]:
-----------------------------------------------------------------

Not sure if you want to rubber stamp this again or not...

Comment 21

[Nathan Froyd [:froydnj]]

Comment on attachment 8334258 [details] [diff] [review]
Build xpcom in unified mode; r=froydnj

Review of attachment 8334258 [details] [diff] [review]:
-----------------------------------------------------------------

I enjoy using my shiny new rubber stamp.

Comment 22

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0d469a8e2208

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0d469a8e2208

Comment 24

[Benoit Girard (:BenWa)]

We should fix this warning:

 0:03.66 In file included from /Users/bgirard/mozilla/mozilla-central/builds/obj-ff-64gdb/xpcom/io/Unified_cpp_xpcom_io0.cpp:12:
 0:03.66 In file included from /Users/bgirard/mozilla/mozilla-central/tree/xpcom/io/nsMultiplexInputStream.cpp:23:
 0:03.66 In file included from ../../dist/include/mozilla/ipc/InputStreamUtils.h:8:
 0:03.66 In file included from /Users/bgirard/mozilla/mozilla-central/builds/obj-ff-64gdb/ipc/ipdl/_ipdlheaders/mozilla/ipc/InputStreamParams.h:12:
 0:03.66 In file included from ../../dist/include/ipc/IPCMessageUtils.h:10:
 0:03.66 In file included from /Users/bgirard/mozilla/mozilla-central/tree/ipc/chromium/src/base/process_util.h:28:
 0:03.66 In file included from /Users/bgirard/mozilla/mozilla-central/tree/ipc/chromium/src/base/command_line.h:27:
 0:03.66 /Users/bgirard/mozilla/mozilla-central/tree/ipc/chromium/src/base/logging.h:95:9: warning: 'LOG' macro redefined
 0:03.66 #define LOG(info) mozilla::LogWrapper(mozilla::LOG_ ## info, __FILE__, __LINE__)
 0:03.66         ^
 0:03.66 /Users/bgirard/mozilla/mozilla-central/tree/xpcom/io/nsInputStreamTee.cpp:35:9: note: previous definition is here
 0:03.67 #define LOG(args)
 0:03.67         ^

Comment 25

[(no longer active)]

Done: https://hg.mozilla.org/integration/mozilla-inbound/rev/430d402e2c45

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/430d402e2c45