Build xpcom in unified mode

RESOLVED FIXED in mozilla28

Status

()

Core
XPCOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

unspecified
mozilla28
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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
(Assignee)

Comment 1

5 years ago
Created attachment 829881 [details] [diff] [review]
Build xpcom in unified mode
(Assignee)

Updated

5 years ago
Assignee: nobody → ehsan
(Assignee)

Updated

5 years ago
Attachment #829881 - Flags: review?(benjamin)
(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?
(Assignee)

Comment 3

5 years ago
(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

5 years ago
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?
Flags: needinfo?(ehsan)
(Assignee)

Comment 5

5 years ago
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.
Flags: needinfo?(ehsan)

Comment 6

5 years ago
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.
(Assignee)

Comment 7

5 years ago
(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

5 years ago
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.
(Assignee)

Comment 9

5 years ago
(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.)
(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 :)
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.
(Assignee)

Comment 12

5 years ago
(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)
(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.
Note that on linux, unified building makes dwarf smaller. Which also means faster link times.
(Assignee)

Comment 15

5 years ago
(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.
(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?
(Assignee)

Updated

5 years ago
Blocks: 939583
(Assignee)

Updated

5 years ago
Attachment #829881 - Flags: review?(benjamin) → review?(nfroyd)
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.
Attachment #829881 - Flags: review?(nfroyd) → review+
(Assignee)

Comment 18

5 years ago
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.
(Assignee)

Comment 19

5 years ago
Created attachment 8334258 [details] [diff] [review]
Build xpcom in unified mode; r=froydnj
(Assignee)

Updated

5 years ago
Attachment #829881 - Attachment is obsolete: true
(Assignee)

Comment 20

5 years ago
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...
Attachment #8334258 - Flags: review?(nfroyd)
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.
Attachment #8334258 - Flags: review?(nfroyd) → review+
https://hg.mozilla.org/mozilla-central/rev/0d469a8e2208
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
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         ^

Updated

5 years ago
Flags: needinfo?(ehsan)

Updated

5 years ago
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.