Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Unity Builds #10578

Merged
merged 11 commits into from Feb 6, 2018
Merged

Unity Builds #10578

merged 11 commits into from Feb 6, 2018

Conversation

friedmud
Copy link
Contributor

Implement "Unity Builds" (see #10561).

With this PR building all of Framework and Modules simultaneously takes 1.5 minutes on icecream with -j 100.

I suspect that this will fail some of the apps (unity builds have already turned up quite a few C++ mistakes in Framework and moose_test).

Let's see what happens...

@mangerij
Copy link
Contributor

mangerij commented Jan 22, 2018 via email

@friedmud friedmud force-pushed the unity_10561 branch 4 times, most recently from d9384a0 to 1247a55 Compare January 23, 2018 21:02
@moosebuild
Copy link
Contributor

moosebuild commented Jan 23, 2018 •
edited

Job Documentation on e6c18f7 wanted to post the following:

View the site here

This comment will be updated on new commits.

@friedmud
Copy link
Contributor Author

BTW: With this I can rebuild all of Bison, Modules and MOOSE in 67 seconds... using -j 150 on icecream...

@dschwen
Copy link
Member

dschwen commented Jan 31, 2018

This is going in the same direction as what FParser is doing with its optimizer. It'd be great if this makes compiling faster. But it needs to be shown that this does not slow down incremental compiles and compiling after branch switches. This would pretty much make ccache ineffective. By the way one issue with the monolithic (unity) fparser build is that it takes a lot of memory when building (this can crash the build on machines with little mem and force you to underutilize the available cores).

@friedmud
Copy link
Contributor Author

@dschwen Note that it's not doing a full monolithic build. Each subdirectory is a separate unity build. It will work fine with ccache and branch switching.

I already did some testing and incremental builds didn't seem to be affected (essentially, the time to build one Kernel is the same as the time to build all Kernels together in the kernels directory... it makes sense if you think about the fact that most of the overhead of building a Kernel is doing all of the #includes... which will still be done just once with Unity build as well).

I don't see how this effects fparser... that's compiled in libMesh... I'm not changing libMesh.

On the topic of memory use I don't expect this to use much more memory for building. The reason is similar to the explanation for incremental builds. Most of the memory is used for all of the #includes each of the objects we're building are actually small. So by reading/compiling all of the #includes just once for all files in a subdirectory (like kernels) and then compiling each of the individual objects... the in-memory size won't be much more than it would be for compiling just one file by itself.

The reason this is true for MOOSE (and not necessarily true if you go read about Unity builds in general) is because our objects are grouped into subdirectories where all of the files in a subdirectory typically have nearly the exact same includes. This gives us a HUGE advantage over a "general" monolithic unity build.

Finally: you'll be able to turn this off with MOOSE_UNITY = false in your environment...

@dschwen
Copy link
Member

dschwen commented Jan 31, 2018

I didn't say this effects fparser. I was just trying to convey an experience I've had with the unity build of fpoptimizer. I'll patiently wait. Good to know you are keeping an option to build the regular way. I expect this to screw with stuff like rtags for code navigation.

@friedmud
Copy link
Contributor Author

I don't know why it would mess with rtags. The unity files are outside the normal source tree - your editor won't even see them.

Here is some timing for incremental builds:

[gastdr][~/projects/moose/test]> touch src/kernels/DotCouplingKernel.C; MOOSE_UNITY=false time make -j 8
Regenerating MooseRevision
MOOSE Compiling C++ (in opt mode) /Users/gastdr/projects/moose/test/src/kernels/DotCouplingKernel.C...
Linking Library /Users/gastdr/projects/moose/test/lib/libmoose_test-opt.la...
Linking Executable /Users/gastdr/projects/moose/test/moose_test-opt...
       17.57 real         7.30 user         1.07 sys
[gastdr][~/projects/moose/test]> touch src/kernels/DotCouplingKernel.C; MOOSE_UNITY=true time make -j 8
Regenerating MooseRevision
MOOSE Compiling C++ (in opt mode) /Users/gastdr/projects/moose/test/build/unity_src/kernels_Unity.C...
Linking Library /Users/gastdr/projects/moose/test/lib/libmoose_test-opt.la...
Linking Executable /Users/gastdr/projects/moose/test/moose_test-opt...
       18.42 real         3.62 user         1.25 sys

Takes about 1 second longer to rebuild moose_test when modifying one Kernel. And this is a pretty bad case because there are 63 Kernels in moose_test... so that Unity build is actually rebuilding all 63 of those Kernels even though you only touched one. And it only takes 1 second longer...

Any activity that involves building larger chunks of source will be faster this way.

@dschwen
Copy link
Member

dschwen commented Jan 31, 2018 •
edited

Yeah, as long as the compile_commands.json stays as it is (pointing to the original source files) it will be fine. The old way of setting up rtags was to pipe the make -n output into the control process (which would then pick up only the new unity.C files).
So hopefully the compile_commands.json:: Makefile target in moose.mk will still work.

P.S.: what about debuggers? are you inserting the preprocessor commands to make sure the debugging symbols point to the original source files?

@friedmud
Copy link
Contributor Author

Even if you did pick up the new Unity files I don't see why it would mess with rtags. But we'll just have to see what happens.

Note that compiling in Emacs with build errors and everything still works fine with the Unity files. I've actually been using this for a week or so and everything is good.

@friedmud
Copy link
Contributor Author

friedmud commented Feb 1, 2018

If the tests pass - I think this is good to go

@friedmud
Copy link
Contributor Author

friedmud commented Feb 2, 2018

No idea what's wrong with Rattlesnake. I've looked extensively at the situation and can't see that anything is amiss. It also doesn't fail to build on my Mac or on Cone.

I have attempted a "fix" here: https://hpcgitlab.inl.gov/idaholab/rattlesnake/merge_requests/330 that is really just a shot in the dark.

Can anyone else see what's going on?

This isn't something wrong with Unity itself... it's some badness with the Yak code... but I can't find it.

@YaqiWang
Copy link
Contributor

YaqiWang commented Feb 2, 2018

I have no clue, if there any option in compiler which gives more diagnosis information?

@friedmud
Copy link
Contributor Author

friedmud commented Feb 2, 2018

I cannot reproduce this - even on the build machines....

@moosebuild
Copy link
Contributor

Job App tests on 33870d7 : invalidated by @friedmud

Invalidating to see if latest RS change fixed it

@YaqiWang
Copy link
Contributor

YaqiWang commented Feb 2, 2018

It is fixed, how come?

@friedmud
Copy link
Contributor Author

friedmud commented Feb 2, 2018

Possibly because of that PR of mine you merged into RS earlier...

@YaqiWang
Copy link
Contributor

YaqiWang commented Feb 3, 2018

That pr seems doing nothing. But I am glad it is working.

@permcody permcody merged commit 42cc564 into idaholab:devel Feb 6, 2018
permcody added a commit to permcody/moose that referenced this pull request Feb 28, 2018
@permcody
Turning on Unity builds for everyone by default
1e4b5d7 
refs idaholab#10578
@permcody permcody mentioned this pull request Feb 28, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@permcody permcody permcody approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@friedmud @mangerij @moosebuild @dschwen @YaqiWang @permcody