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.

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

Adding possibility of faster builds using ccache #3381

Merged
merged 3 commits into from
Oct 9, 2019

Conversation

kunaltyagi
Copy link
Member

Compile times should be shorter now

This solution should work for Unix Makefile, Ninja and XCode. It'll not work for MSVC since ccache doesn't support it.

There may be need for changing the ${PCL_DISABLE_CCACHE} if MSVC is used (when ccache is installed), but it'll need testing. Any volunteers?

@kunaltyagi kunaltyagi added needs: testing Specify why not closed/merged yet module: cmake labels Sep 27, 2019
Copy link
Member

@taketwo taketwo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can give this a try on Ubuntu, though I don't have experience with ccache. Do we expect shorter first-time compilations?

cmake/pcl_options.cmake Outdated Show resolved Hide resolved
cmake/pcl_ccache.cmake Outdated Show resolved Hide resolved
@kunaltyagi
Copy link
Member Author

Do we expect shorter first-time compilations

Most likely, no. Unless the compiler for some reason doesn't reuse the compiled object (which can happen based on CMake generation and rules)

I do forsee developers getting faster recompiles after branch changes (or if they have 2 different versions in 2 different locations). Plus, if ccache is shared between CI, then the CI builds will also be faster (lots of reasons to start a clean build every time and not share the build dir)

@taketwo
Copy link
Member

taketwo commented Sep 27, 2019

I get endless warnings of this kind:

[1/1521] Building CXX object filters/CMakeFiles/pcl_filters.dir/src/passthrough.cpp.o
/home/sergey/Workspace/Libraries/pcl/build/launch-cxx: 5: /home/sergey/Workspace/Libraries/pcl/build/launch-cxx: [[: not found
c++: warning: /usr/bin/c++: linker input file unused because linking not done

@kunaltyagi
Copy link
Member Author

I see. Your output seems to be truncated

[[: not found is that the end? The error seems to be from the compiler, not ccache. Linking should not be done for the very first target too.

Could you report the command by setting VERBOSE=1 for make utility (seems like ninja).

@taketwo
Copy link
Member

taketwo commented Sep 27, 2019

I see. Your output seems to be truncated
[[: not found is that the end?

Yep. And then a warning from the next file comes [2/1521]. And so on. Here is verbose output:

$ ninja -v
[1/1518] /home/sergey/Workspace/Libraries/pcl/build/launch-cxx /usr/bin/c++  -DPCLAPI_EXPORTS -Dqh_QHpointer -DvtkRenderingContext2D_AUTOINIT="1(vtkRenderingContextOpenGL)" -DvtkRenderingCore_AUTOINIT="3(vtkInteractionStyle,vtkRenderingFreeType,vtkRenderingOpenGL)" -isystem /usr/include/eigen3 -isystem /usr/include/ni -isystem /usr/include/openni2 -isystem ../recognition/include/pcl/recognition/3rdparty -I/usr/include/vtk-6.3 -I/usr/include/freetype2 -I/usr/include/x86_64-linux-gnu -Iinclude -I../common/include -Wabi=11 -Wall -Wextra -Wno-unknown-pragmas -fno-strict-aliasing -Wno-format-extra-args -Wno-sign-compare -Wno-invalid-offsetof -Wno-conversion -march=native -msse4.2 -mfpmath=sse -fopenmp  -O2 -g -DNDEBUG -fPIC   -DBOOST_DISABLE_ASSERTS -DEIGEN_NO_DEBUG -std=c++14 -MD -MT common/CMakeFiles/pcl_common.dir/src/print.cpp.o -MF common/CMakeFiles/pcl_common.dir/src/print.cpp.o.d -o common/CMakeFiles/pcl_common.dir/src/print.cpp.o -c ../common/src/print.cpp
/home/sergey/Workspace/Libraries/pcl/build/launch-cxx: 5: /home/sergey/Workspace/Libraries/pcl/build/launch-cxx: [[: not found
c++: warning: /usr/bin/c++: linker input file unused because linking not done

I checked the contents of the launch files and the problem is clear: [[ is a Bash thing, so either the shebang should point to bash, or we should use [ tests. (I prefer the latter.)

With this fix everything works.

@kunaltyagi
Copy link
Member Author

[[ is a Bash thing,

Of course. That'll bite me. I use bash as my driver, not dash (not Ubuntu). Could you push your fix? I'll have to test and fiddle with bracket expressions.

@kunaltyagi
Copy link
Member Author

Thanks. That was trivial (and the bash test macro was useless)

@SunBlack
Copy link
Contributor

SunBlack commented Sep 27, 2019

Just as note: We are using it in our project, too, but our way is a lot of easier:

# Configure CCache if available
find_program(CCACHE_FOUND ccache)
if(CCACHE_FOUND)
	set_property(GLOBAL PROPERTY RULE_LAUNCH_COMPILE ccache)
	set_property(GLOBAL PROPERTY RULE_LAUNCH_LINK ccache)
endif(CCACHE_FOUND)

I'm using ccache already since month for PCL via:

CMAKE_CXX_COMPILER:FILEPATH=/usr/lib/ccache/clang++
CMAKE_C_COMPILER:FILEPATH=/usr/lib/ccache/clang

But in general: CCache should be default to OFF here, because it is not necessary to spam most users cache with files from a library which they only compile once.

Further note: For MSVC clcache exists.

//Edit: Ah well, now I see, why it looks so complicated compared to our short solution.

cmake/pcl_ccache.cmake Outdated Show resolved Hide resolved
cmake/launch-cxx.in Outdated Show resolved Hide resolved
@kunaltyagi
Copy link
Member Author

For MSVC clcache exists

Thanks. It's interface is almost a mirror too. I added some modification for MSVC based on this

@kunaltyagi
Copy link
Member Author

I found this document for instructions to use ccache for azure pipelines. This should being down CI build times by a lot.

On my system, ccache takes 6.1 GB space.

$ ninja clean; time ninja
real    1m7.459s
user    4m57.092s
sys     0m42.055s

@kunaltyagi kunaltyagi marked this pull request as ready for review September 28, 2019 09:03
@taketwo
Copy link
Member

taketwo commented Sep 29, 2019

I will rebuild and push the env:16.04 image.

@kunaltyagi
Copy link
Member Author

kunaltyagi commented Sep 29, 2019

Is pip or chocolatey available for installing clcache on Windows machines?

@SunBlack have you used clcache before? Should there be something specific to clcache (compared to ccache)?

@taketwo
Copy link
Member

taketwo commented Sep 29, 2019

Here is the Windows image contents. Chocolatey is the first item on the list.


After adding ccache to the Ubuntu image, I ran the job two times. Here is the first run and here is the second. The cache was uploaded and then downloaded successfully, so that part works. However, I don't observe any decrease in build time*. The statistics reported after the second run are not encouraging, especially because this was a re-run without any code change:

cache hit (direct)                    16
cache hit (preprocessed)              60
cache miss                          1780
files in cache                       898

*compared to pre-ccache jobs where the build step took around 1:50. If the entire job duration (including upload/download of cache) is taken into account, then the time is even approximately 10 minutes worse.

@SunBlack
Copy link
Contributor

@SunBlack have you used clcache before? Should there be something specific to clcache (compared to ccache)?

We are using clcache on our Build server on this way (without CMake):

cmake .. -G "Visual Studio 15 Win64" ...
msbuild pcr.sln /p:Configuration=Release /ds /m /verbosity:m /nologo /p:CLToolExe=clcache.exe /p:CLToolPath="C:\Program Files\Python37\Scripts"

Thinks you should know about clcache:

  • Debug mode is not supported
  • The speed improvement is not as high as with with ccache, but might we in short future
  • Development is slow, as the project is open for a new maintainer (see comment here)

@kunaltyagi
Copy link
Member Author

kunaltyagi commented Sep 30, 2019

cache miss 1780

This is really high and perhaps due to insufficient cache (and also since the second run was worse compared to the first)

EDIT:
So, the build time fell to 1 hour, from 1 hour 52 minutes on Ubuntu. It rose on Mac, for reasons unknown to me. I'll keep sleuthing. Might be because of increased IO (Azure disks are known to be slow)

The cache misses are still huge, which I'm hoping will be resolved by a step to zero the stats before every build so we get per-build data, not accumulated.

@kunaltyagi kunaltyagi force-pushed the ccache branch 2 times, most recently from 3181dfe to 2bbc00d Compare September 30, 2019 07:10
@taketwo
Copy link
Member

taketwo commented Sep 30, 2019

So, the build time fell to 1 hour, from 1 hour 52 minutes on Ubuntu.

Wow, that's great. Now I am convinced :)

However, I tend to agree with @SunBlack regarding the default state:

But in general: CCache should be default to OFF here, because it is not necessary to spam most users cache with files from a library which they only compile once.

@kunaltyagi
Copy link
Member Author

Now I am convinced

Try to compile on your workstation and feel the difference already 😛

agree with @SunBlack regarding the default state

PTAL at the recent rebase. I've applied the ON to PCL_DISABLE_CCACHE so only manually flipping it will enable ccache (which is also what I did in the build-ubuntu.yaml)

We could tune this more (for Mac too), with slopiness for clang_index_store, file_stat_matches (and time_macros+file_macro for CI) debug using CCACHE_DEBUG, but I don't think that's required now. I feel misses (~50%) are still a lot but not an easy issue to solve.

Experimenting on the CI has a severe lag of 😰 for every tiny modification. Esp since Windows takes 2 hours to build. Any ideas on how to test? I don't have access to Azure free anymore

@taketwo
Copy link
Member

taketwo commented Sep 30, 2019

Try to compile on your workstation and feel the difference already 😛

My workstation has 24 threads, so PCL is built in no time anyway 😛

PTAL at the recent rebase. I've applied the ON to PCL_DISABLE_CCACHE

I overlooked that, indeed. But then I will bring my proposal to rename to PCL_ENABLE_CCACHE to the table again. Your argument against was basically that ENABLE_CCACHE=ON without ccache doesn't make sense. But isn't it the same for DISABLE_CCACHE=OFF? And if so, then having an opt-in feature called ENABLE and defaulting to OFF seems more natural to me.

Experimenting on the CI has a severe lag of cold_sweat for every tiny modification. Esp since Windows takes 2 hours to build. Any ideas on how to test? I don't have access to Azure free anymore

I added you to the Azure organization a couple of days ago, so you should be able to cancel/reschedule jobs.

cmake/pcl_ccache.cmake Outdated Show resolved Hide resolved
.ci/azure-pipelines/build-ubuntu.yml Outdated Show resolved Hide resolved
.ci/azure-pipelines/build-windows.yml Outdated Show resolved Hide resolved
.ci/azure-pipelines/build-windows.yml Outdated Show resolved Hide resolved
choco install clcache --source=.
- task: CacheBeta@0
inputs:
key: ccache | msvc | $(Agent.OS)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we have different keys for x86 and x64 jobs? Or is it okay to mix their caches?

Copy link
Member Author

@kunaltyagi kunaltyagi Oct 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought the platform is different so would the $(Agent.OS) string, but that's not what happened. I don't know if it'll work, but I'll push a change with the GENERATOR variable as part of the cache string

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a step that prints all environment variables. Looking at it, I think it would be appropriate to append Agent.OSArchitecture to the key.

Copy link
Member Author

@kunaltyagi kunaltyagi Oct 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, and no, it's not ok. Not an issue since the cache is fortunately for reasons unknown to me empty. Maybe clcache doesn't want the command to include the compiler .... 🤷‍♂

@taketwo
Copy link
Member

taketwo commented Oct 1, 2019

With the latest change we have different keys for different jobs, that's good. But it seems like clcache was not used (despite being detected by CMake):

clcache statistics:
  current cache dir         : Disk cache at d:\a\1\clcache
  cache size                : 0 bytes
  maximum cache size        : 1,073,741,824 bytes
  cache entries             : 0
  cache hits                : 0
  cache misses
    total                      : 0
    evicted                    : 0
    header changed             : 0
    source changed             : 0
  passed to real compiler
    called w/ invalid argument : 0
    called for preprocessing   : 0
    called for linking         : 0
    called for external debug  : 0
    called w/o source          : 0
    called w/ multiple sources : 0
    called w/ PCH              : 0

If you think about it, this is of no surprise since compiler launcher properties are only supported for Ninja and Makefiles, not MSVC. According to this page the only way to use clcache with CMake is to set the CXX enrivorment variable. This means, however, that we can not do this automatically from within CMake. Sad, but at least in CI jobs, we are in control of the environment and can still benefit from caching.

@taketwo
Copy link
Member

taketwo commented Oct 1, 2019

One other thing. I found this implementation, also based on the blog post you referenced. In comparison to the scripts in this PR it:

  • supports CUDA
  • has launch files embedded into the script

What're your feelings about adopting it?

@kunaltyagi
Copy link
Member Author

kunaltyagi commented Oct 1, 2019

supports CUDA

Yay

has launch files embedded into the script

Overall positive but somewhat mixed feelings. ✅ On one hand, one single file. 🔻 On the other hand, CMake isn't sh and it looks bad. But it works ¯\_(ツ)_/¯ ✅ And we can just copy-paste and edit to add the link to the original till KDE lives 😆

we can not do this automatically from within CMake

That's the truly sad part of this entire chain 😆

CMake is to set the CXX enrivorment variable

Are we running in powershell or what? I don't know what syntax to use to set the env variable

@taketwo
Copy link
Member

taketwo commented Oct 1, 2019

I don't know what syntax to use to set the env variable

Try to add the following step before CMake configuration:

- script: |
    echo ##vso[task.setvariable variable=CXX]clcache.exe
  displayName: 'Set default C++ compiler to clcache'

Overall positive but somewhat mixed feelings.

Same here. Not sure if not having separate files is a big plus. So I'll leave it up to you to decide. But at any rate, copy over the CUDA part.

@kunaltyagi
Copy link
Member Author

So I'll leave it up to you to decide

Single file. Least headache.

Also, since the setting is done globally for clcache, I'll create a warning saying: Set the CXX/C env variable, since clcache doesn't behave like ccache.

@taketwo
Copy link
Member

taketwo commented Oct 2, 2019

Also, since the setting is done globally for clcache, I'll create a warning saying: Set the CXX/C env variable, since clcache doesn't behave like ccache.

This I don't get. To enable clcache on Windows the user needs to modify environmental variables, which is outside of CMake control. And that's the only thing that needs to be done. Thus what's the point of having a CMake option, doing find_program, etc.?

@kunaltyagi
Copy link
Member Author

kunaltyagi commented Oct 4, 2019

add the following step before CMake configuration

This didn't seem to work as evident by CMake config log

-- The C compiler identification is MSVC 19.16.27034.0
-- The CXX compiler identification is MSVC 19.16.27034.0

Also, I've changed the file contents. PTAL

@kunaltyagi kunaltyagi closed this Oct 4, 2019
@kunaltyagi kunaltyagi reopened this Oct 4, 2019
Copy link
Member

@taketwo taketwo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, setting the environment variable does not help. What about leaving Windows CI support out of this pull request for now?

cmake/Modules/UseCompilerCache.cmake Outdated Show resolved Hide resolved
cmake/launch-c.in Outdated Show resolved Hide resolved
cmake/pcl_options.cmake Outdated Show resolved Hide resolved
Copy link
Member

@taketwo taketwo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, wrong button.

@kunaltyagi
Copy link
Member Author

I'll try to push a change today or tomorrow. Really wanted to reduce Windows build time down to 1 hour or so from 2 hours

@taketwo
Copy link
Member

taketwo commented Oct 7, 2019

Sure, if you haven't run out of ideas yet, let's keep on trying.

@kunaltyagi
Copy link
Member Author

I gave up after another experiment on Windows. Unnoticed to me, the change to new script resulted in ccache not being used. I'm working on it.

Ignore MSVC + Recommended change

Replace [[ with [ to be compatible with sh

Fix indentation

disable ccache by default

Add clcache support

Modifications to save variables in parent scope
Setting a high limit on cache size

Per-run stats only

Removed ccache from Mac, negative performance impact
@kunaltyagi
Copy link
Member Author

PTAL.

Works on Mac, Ubuntu and Arch. On the CI, enabled only for ubuntu 16.04

Build time ~1 hour on Ubuntu CI

@taketwo taketwo removed the needs: testing Specify why not closed/merged yet label Oct 9, 2019
@taketwo taketwo merged commit 9018def into PointCloudLibrary:master Oct 9, 2019
@taketwo
Copy link
Member

taketwo commented Oct 9, 2019

This is a great contribution, thanks for working on it.

@kunaltyagi kunaltyagi deleted the ccache branch October 10, 2019 01:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants