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

Speedup CI builds using cotire #461

Merged
merged 2 commits into from
Feb 23, 2017
Merged

Speedup CI builds using cotire #461

merged 2 commits into from
Feb 23, 2017

Conversation

tusharpm
Copy link
Contributor

All unit-*.cpp files in test/src include catch.hpp and json.hpp. Both are header-only libraries, and can benefit from using precompiled headers.

  • Use cotire for reducing the compile time using a prefix header (test/src/prefix.hpp).
  • Change travis config to use cmake instead of the hand-written Makefiles.
  • Enable parallel build in Travis config (-- -j4)

The results - comparing a sample without this change and with:

Metric Without cotire With cotire
Travis (Running time) 32 min 21 sec 28 min 32 sec
Travis (Total time) 2 hrs 3 min 43 sec 1 hr 52 min 36 sec
AppVeyor (Total time) 3 min 34 sec 2 min 55 sec

The travis build time can be further reduced by using the cmake build system for the slowest pieces (valgrind and sanitizer - these recompile the tests independently now), and the container-infrastructure.

- Add prefix header
  - Include catch.hpp
  - Include json.hpp
    - Replace private with public for all json_unit files
- Move `unit.cpp` to an object library
- cotire issue: strip whitespace from CMAKE_INCLUDE_SYSTEM_FLAG_CXX
- get CMake for XCode 8.1 image
@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.964% when pulling 5436407 on tusharpm:cotire into 0200f2d on nlohmann:develop.

@nlohmann
Copy link
Owner

I have some questions:

  • What is the role of file test/src/prefix.hpp?
  • To what extend is the speedup related to cotire and where is it just using -j4?

@tusharpm
Copy link
Contributor Author

  • test/src/prefix.hpp is the prefix header (manually maintained, instead of the one auto-generated by cotire) including the headers that need to be precompiled. As per my understanding, this (or a precompiled equivalent anyway) is added to all compilation units by cotire.
  • I triggered a build without -- -j4 just for comparing those results as well. Overall, it is a little slower than the original build (no-cotire).
    • The "special" builds are generally-speaking slower (because of the second compilation not using cmake).
    • The remaining (compile-heavy) builds have mixed results.
      • Some are a tad faster (LLVM - 3.8.0 and 3.8.1 saving 15-20 seconds in about 4 minutes, similar for Xcode 8.2).
      • Others are considerably slower (Xcode 6.4 is roughly 2 minutes slower from the original 6½ minutes).
    • I agree this analysis is not statistically sound (very few data points), but it gives a rough idea of the time taken by each variant.
    • The advantage of (only) using cotire is visible on the AppVeyor build which doesn't have an explicit parallel jobs specification.
    • If the explicit -- -j4 is the problem, should I try with a different generator/build-system (ninja), which can determine the best configuration at runtime?

@nlohmann nlohmann self-assigned this Feb 22, 2017
@nlohmann nlohmann added this to the Release 2.1.1 milestone Feb 22, 2017
@nlohmann
Copy link
Owner

I added a branch from this PR, see https://github.com/nlohmann/json/tree/tusharpm-cotire. I shall merge it when Travis/AppVeyor have finished checking it. Thanks a lot!

@nlohmann nlohmann merged commit 5436407 into nlohmann:develop Feb 23, 2017
@tusharpm tusharpm deleted the cotire branch February 23, 2017 14:27
@tusharpm
Copy link
Contributor Author

My pleasure! 👍

nlohmann added a commit that referenced this pull request Mar 12, 2017
This test case relied on logics that have been replaced by CMake with
#461. This change enables compilation and execution of the test suite
without exceptions by adding an after_success task.
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