Optimisation of compilation speed with CMake/Ninja #2877

daljit46 · 2024-04-15T09:23:28Z

This PR aims to significantly reduce the time to compile MRtrix3 and address the issue raised in #2876.

I have tested the changes locally on my laptop on Fedora Asahi and Mac OS 14 and, roughly speaking, I see an improvement of almost 30% compared to dev (without using ccache).

Here is a table showing indicative build times (in seconds) on my Macbook M2 Pro 2023 (I took the best of 3 successive builds).

	dev	new
MacOS 14 (AppleClang)	124s	89s
Fedora Asahi (clang 17)	139s	91s
Fedora Asahi (gcc 13)	154s	110s

This was achieved with a lot of trial and error and the help of ClangBuildAnalyzer. @MRtrix3/mrtrix3-devs it'd be great if you could test this on your machines to see if you can reproduce similar results. One caveat is that to achieve the most optimal results, you must use the latest version of Ninja(v 1.12), released last week (see first point below). On MacOS, it's just a matter of running brew upgrade. On Linux you will probably need to download it directly from their Github releases page and then run export PATH=/ninja_dir/:$PATH (it's unlikely that your distro has packaged the latest version). Before rebuilding, you obviously need to clear your build directory and compiler cache (ccache -C).

More in detail, there are three changes in this PR:

Increased build parallelism. During the initial stages of our CMake effort, it was pointed out by @jdtournier that the CPU load during the build phase didn't stay 100% throughout its entirety. So looking more into this, I found that when building dev, the Ninja build graph looks like this:
If you look at the end of the graph, you can see that mrregister.cpp.o takes a long time to compile (about 45s) and in fact the rest of the object files finish compiling much earlier. So there is a period when the compiler is effectively operating using a single thread. Ideally, the compilation of mrregister should start at the beginning of the compilation phase, however, there is effectively no way to control the order of compilation in CMake (or in Ninja). To get around this, I used a simple trick (see cmd/CMakeLists.txt): create a dummy executable and then add mrregister as a dependency of this dummy (this is the relevant change in Ninja 1.12 that makes this possible). This is enough to trick Ninja and change the build graph to the following:
Now, you see that we are operating multi-threaded throughout the entire compilation phase.
Precompiled headers. Precompiled headers speed up compilation by storing parsed header information in a special file. During subsequent compilations, the compiler reuses this precompiled data instead of re-parsing headers. They are particularly suited for "stable" header files, i.e. files that do not change very often (e.g. STL headers). If files listed in a precompiled header file change, then a target using the precompiled header will need to be recompiled from scratch. In this PR, I only included the following headers (other than STL and third parties) in precompiled headers: gl.h and exception.h. The first one is only used for mrview/shview and the second one is for all the code. If a dev needs to frequently edit these files, it'd be best to turn off MRTRIX_USE_PCH as otherwise, any change in those files will trigger a rebuild of the whole project.
Explicit template instantiations. C++11 introduced the extern template syntax, which lets you say, "Hey, the template's definition? It's elsewhere!". So if we use this in a header file, we can tell the compiler to avoid automatically instantiating a specific template in every .cpp file that includes the header (of course we have to make sure that the template is instantiated somewhere, e.g. in the corresponding .cpp for the header).
After looking at the report by ClangBuildAnalyzer, I used this feature to explicitly instantiate some class and function templates to reduce compilation time. I did this fairly conservatively (I really don't like dealing with the complexity that templates bring), but the few changes I made still made a significant difference in build time.

github-actions · 2024-04-15T09:27:23Z

clang-tidy review says "All clean, LGTM! 👍"

github-actions · 2024-04-15T10:29:17Z

clang-tidy review says "All clean, LGTM! 👍"

daljit46 · 2024-04-15T18:37:58Z

Managed to obtain further improvements by enabling PCHs for executables too (previously they were only used for the shared libraries).
The new timings look like below:

	dev	new
MacOS 14 (AppleClang)	124s	84s
Fedora Asahi (clang 17)	139s	84s
Fedora Asahi (gcc 13)	154s	103s

github-actions · 2024-04-15T18:41:57Z

clang-tidy review says "All clean, LGTM! 👍"

github-actions · 2024-04-15T19:12:20Z

clang-tidy review says "All clean, LGTM! 👍"

cmd/CMakeLists.txt

Lestropie · 2024-04-15T22:19:34Z

core/core_pch.h

@@ -0,0 +1,3 @@
+#include "exception.h"


Should decide whether inclusion of MRtrix header files here is to be based on pragmatism (ie. what provides the greatest speedup vs. is least likely to be modified) vs. some sort of principle (ie. what are "the most fundamental" core header files). There might be an argument to be made for core/*.h (ie. root of core/ but not its sub-directories). But I'm not currently able to form a robust opinion on this.

I think a nice balance between stability and speed gain should be prioritised. Additionally, we should probably include header files that are likely to affect most of the codebase. For example, this was the reasoning to add #include "exception.h", since it showed up in ClangBuilderAnalyzer's report and I think there are little chances for a developer to modify it. Furthermore, this header is indirectly included in the majority of our codebase and thus even without precompiled headers, its modification will trigger a recompilation of a substantial chunk of the project (on this point, it's worth noting that you can always turn off PCHs by setting cmake -DMRTRIX_USE_PCH=OFF).
Overall, I do think that we should be very conservative with the inclusion of user headers in a PCH.

core/image.h

src/dwi/tractography/roi.cpp

Lestropie · 2024-04-15T22:40:38Z

Are you able to glean insight into what it is that makes the mrregister / mrtransform compilation so long? That would potentially be the place to be looking into precompiled headers / precompiled template instantiations.

daljit46 · 2024-04-16T09:11:53Z

Are you able to glean insight into what it is that makes the mrregister / mrtransform compilation so long? That would potentially be the place to be looking into precompiled headers / precompiled template instantiations.

Looking at the output of ClangBuildAnalyzer, for mrregister it shows that only 20% of the time is spent in the compiler's front-end (parsing and instantiation templates), while the rest of the time the compiler is busy optimising the code (most likely on inlined templated functions).

Here is the json trace file for the command obtained by compiling with CMAKE_CXX_FLAGS="-ftime-trace" (you can read it using https://ui.perfetto.dev)
mrregister.cpp.json

Hence I think the best course of action would be to split this file into multiple files (or at least move parts of its content somewhere else) so that the compiler can operate multithreaded.

daljit46 · 2024-04-16T16:50:49Z

Further finding on mrregister: on my desk PC (a 20-core Intel(R) Xeon(R) CPU E5-2687W v3 @ 3.10GHz), the compilation of mrregister.cpp.o takes more than the time required to build all other the object files in the project (nearly 4 minutes)!

github-actions

clang-tidy made some suggestions

src/registration/transform/base.cpp

github-actions · 2024-04-17T11:04:29Z

src/registration/transform/base.cpp

+namespace MR::Registration::Transform {
+
+Base::Base(size_t number_of_parameters)
+    : number_of_parameters(number_of_parameters), optimiser_weights(number_of_parameters), nonsymmetric(false) {


warning: narrowing conversion from 'size_t' (aka 'unsigned long') to signed type 'Eigen::VectorXd' (aka 'int') is implementation-defined [bugprone-narrowing-conversions]

: number_of_parameters(number_of_parameters), optimiser_weights(number_of_parameters), nonsymmetric(false) { ^

src/registration/transform/base.cpp

github-actions · 2024-04-17T11:04:29Z

src/registration/transform/base.cpp

+
+Eigen::Matrix<default_type, 4, 1> Base::get_jacobian_vector_wrt_params(const Eigen::Vector3d &p) const {
+  throw Exception("FIXME: get_jacobian_vector_wrt_params not implemented for this metric");
+  Eigen::Matrix<default_type, 4, 1> jac;


warning: variable 'jac' is not initialized [cppcoreguidelines-init-variables]

Suggested change

Eigen::Matrix<default_type, 4, 1> jac;

Eigen::Matrix<default_type, 4, 1> jac = 0;

github-actions · 2024-04-17T11:04:29Z

src/registration/transform/base.cpp

+}
+
+Eigen::Transform<Base::ParameterType, 3, Eigen::AffineCompact> Base::get_transform() const {
+  Eigen::Transform<ParameterType, 3, Eigen::AffineCompact> transform;


warning: variable 'transform' is not initialized [cppcoreguidelines-init-variables]

Suggested change

Eigen::Transform<ParameterType, 3, Eigen::AffineCompact> transform;

Eigen::Transform<ParameterType, 3, Eigen::AffineCompact> transform = 0;

github-actions · 2024-04-17T11:04:29Z

src/registration/transform/base.cpp

+
+void Base::compute_offset() { trafo.translation() = (trafo.translation() + centre - trafo.linear() * centre).eval(); }
+
+void Base::compute_halfspace_transformations() {


warning: method 'compute_halfspace_transformations' can be made const [readability-make-member-function-const]

Suggested change

void Base::compute_halfspace_transformations() {

void Base::compute_halfspace_transformations() const {

src/registration/transform/base.h:177:

- void compute_halfspace_transformations(); + void compute_halfspace_transformations() const;

github-actions · 2024-04-17T11:04:29Z

src/registration/transform/base.cpp

+void Base::compute_offset() { trafo.translation() = (trafo.translation() + centre - trafo.linear() * centre).eval(); }
+
+void Base::compute_halfspace_transformations() {
+  Eigen::Matrix<ParameterType, 4, 4> tmp;


warning: variable 'tmp' is not initialized [cppcoreguidelines-init-variables]

Suggested change

Eigen::Matrix<ParameterType, 4, 4> tmp;

Eigen::Matrix<ParameterType, 4, 4> tmp = 0;

daljit46 · 2024-04-17T11:26:57Z

I noticed that registration/transform/base.h took a very long time to parse. I moved some functions from the header to a new corresponding source file and this provided a nice build time improvement for mrregister (about 10%). On my work desk PC compile times went down from 235 seconds to 212 seconds.
This is a pattern I noticed a lot in our codebase: there are many instances where (non-templated) functions are unnecessarily defined in header files. We should try to improve on this (at least for new code).

Another finding is that the main culprit of large compilation times for mrregister is the compilation of all instantiations of Linear::run_masked (in src/registration/linear.h) which is responsible for at least 50% of the compilation times.

mrregister and mrtransform can take a long time to build. If their compilation starts towards the end of the build, Ninja may be compiling a single source file for quite sometime. This may result in suboptimal compilation times (due to part of the compilation not being multi-threaded). This hack tries to avoid this by starting their compilation as soon as possible (atm there's no alternative nice way of doing this in Ninja or CMake).

We use extern template to reduce the number of instantiations of some templates to reduce compilation time.

daljit46 · 2024-04-22T15:21:44Z

This is now ready for review. I've cleaned up a few things and added more explicit instantiations to reduce the build time for the registration code.
I've tested the changes on three different machines. Generally speaking, you should observe an improvement of between 30-50% in compile times (the changes are most effective when compiling with Clang rather than GCC).

The compilation of mrregister, while significantly faster than on dev, remains a bottleneck. This is because mrregister.cpp contains many template instantiations that take a lot of time to compile. The only viable strategy to mitigate this is to move these instantiations to separate .cpp files. I've already done this for some templates, but to see even further improvements one would need to (very tediously) take this further. However, this PR is already beyond the scope of what I initially intended. So, I would prefer, if this strategy is pursued further, to implement those changes in a future PR.

A point of concern, raised by @jdtournier, was that explicit instantiation of a templated function in a .cpp file will lead the compiler to not inline that function. While it's true that extern template can hinder inlining, I don't think this is a problem. Firstly, trying to inline a function may not always lead to the compiler producing faster code. Inlining only really makes sense for small and inexpensive functions. On this note, I do think that we are slightly abusing this feature in our codebase as it mostly likely doesn't lead to better code, I wouldn't be surprised if it has the opposite effect. Secondly, Link Time Optimisation (cmake -DCMAKE_INTERPROCEDURAL_OPTIMIZATION=ON) can obviate this issue when using extern template (for a demonstration see this very nice video).

@Lestropie @jdtournier any comments are welcome.

Lestropie · 2024-04-22T23:48:26Z

MR::Registration::Transform::Base::Base(size_t) should be easy to fix the clang-tidy warnings; those Eigen member variables have corresponding initialisation syntax. Not essential for merge, just what I saw when reviewing.

I would have just pushed the change but I wasn't able to verify compilation because of:

robertes@9520l-003953-l:~/src/mrtrix3$ cmake --build build_debug/
[29/498] Linking CXX executable cmd/pch_cmd
FAILED: cmd/pch_cmd 
: && /usr/bin/c++ -g -fuse-ld=lld cmd/CMakeFiles/pch_cmd.dir/pch_cmd.cpp.o -o cmd/pch_cmd   && :
ld.lld: error: undefined symbol: MR::File::Config::config[abi:cxx11]
>>> referenced by config.h:34 (/home/unimelb.edu.au/robertes/src/mrtrix3/cmd/../core/file/config.h:34)
>>>               cmd/CMakeFiles/pch_cmd.dir/pch_cmd.cpp.o:(MR::File::Config::get(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&))
>>> referenced by config.h:35 (/home/unimelb.edu.au/robertes/src/mrtrix3/cmd/../core/file/config.h:35)
>>>               cmd/CMakeFiles/pch_cmd.dir/pch_cmd.cpp.o:(MR::File::Config::get(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&))
collect2: error: ld returned 1 exit status
[50/498] Building CXX object src/CMakeFiles/mrtrix-headless.dir/dwi/fmls.cpp.o
ninja: build stopped: subcommand failed.

This avoids their recompilation every time the project is configured

github-actions

clang-tidy made some suggestions

core/app.cpp

github-actions · 2024-04-23T10:25:56Z

src/registration/transform/base.cpp

+  centre.setZero();
+}
+
+Eigen::Matrix<default_type, 4, 1> Base::get_jacobian_vector_wrt_params(const Eigen::Vector3d &) {


warning: all parameters should be named in a function [readability-named-parameter]

Suggested change

Eigen::Matrix<default_type, 4, 1> Base::get_jacobian_vector_wrt_params(const Eigen::Vector3d &) {

Eigen::Matrix<default_type, 4, 1> Base::get_jacobian_vector_wrt_params(const Eigen::Vector3d & /*unused*/) {

github-actions · 2024-04-23T10:25:56Z

src/registration/transform/base.h

-  Eigen::Transform<ParameterType, 3, Eigen::AffineCompact> trafo_half;
-  Eigen::Transform<ParameterType, 3, Eigen::AffineCompact> trafo_half_inverse;
-  Eigen::Vector3d centre;
+  Eigen::Transform<ParameterType, 3, Eigen::AffineCompact> trafo{};


warning: member variable 'trafo' has protected visibility [cppcoreguidelines-non-private-member-variables-in-classes]

Eigen::Transform<ParameterType, 3, Eigen::AffineCompact> trafo{}; ^

github-actions · 2024-04-23T10:25:56Z

src/registration/transform/base.h

-  Eigen::Transform<ParameterType, 3, Eigen::AffineCompact> trafo_half_inverse;
-  Eigen::Vector3d centre;
+  Eigen::Transform<ParameterType, 3, Eigen::AffineCompact> trafo{};
+  Eigen::Transform<ParameterType, 3, Eigen::AffineCompact> trafo_half{};


warning: member variable 'trafo_half' has protected visibility [cppcoreguidelines-non-private-member-variables-in-classes]

Eigen::Transform<ParameterType, 3, Eigen::AffineCompact> trafo_half{}; ^

github-actions · 2024-04-23T10:25:57Z

src/registration/transform/base.h

-  Eigen::Vector3d centre;
+  Eigen::Transform<ParameterType, 3, Eigen::AffineCompact> trafo{};
+  Eigen::Transform<ParameterType, 3, Eigen::AffineCompact> trafo_half{};
+  Eigen::Transform<ParameterType, 3, Eigen::AffineCompact> trafo_half_inverse{};


warning: member variable 'trafo_half_inverse' has protected visibility [cppcoreguidelines-non-private-member-variables-in-classes]

Eigen::Transform<ParameterType, 3, Eigen::AffineCompact> trafo_half_inverse{}; ^

I'm not sure about this check. While I understand that having classes with private and public data members can be undesirable, I don't really find this to be a huge concern. Especially in a codebase like ours.

github-actions · 2024-04-23T10:25:57Z

src/registration/transform/base.h

+  Eigen::Transform<ParameterType, 3, Eigen::AffineCompact> trafo{};
+  Eigen::Transform<ParameterType, 3, Eigen::AffineCompact> trafo_half{};
+  Eigen::Transform<ParameterType, 3, Eigen::AffineCompact> trafo_half_inverse{};
+  Eigen::Vector3d centre{};


warning: member variable 'centre' has protected visibility [cppcoreguidelines-non-private-member-variables-in-classes]

Eigen::Vector3d centre{}; ^

github-actions

clang-tidy made some suggestions

There were too many comments to post at once. Showing the first 25 out of 56. Check the log or trigger a new build to see more.

github-actions · 2024-04-24T01:47:55Z

core/file/config.cpp

+}
+
+std::string Config::get(const std::string &key, const std::string &default_value) {
+  KeyValues::iterator i = config.find(key);


warning: variable 'i' of type 'KeyValues::iterator' (aka '_Rb_tree_iterator<std::pair<const std::basic_string, std::basic_string>>') can be declared 'const' [misc-const-correctness]

Suggested change

KeyValues::iterator i = config.find(key);

KeyValues::iterator const i = config.find(key);

github-actions · 2024-04-24T01:47:55Z

core/file/utils.cpp

+
+namespace {
+inline char random_char() {
+  char c = rand() % 62;


warning: variable 'c' of type 'char' can be declared 'const' [misc-const-correctness]

Suggested change

char c = rand() % 62;

char const c = rand() % 62;

github-actions · 2024-04-24T01:47:55Z

core/file/utils.cpp

+
+namespace {
+inline char random_char() {
+  char c = rand() % 62;


warning: narrowing conversion from 'int' to signed type 'char' is implementation-defined [bugprone-narrowing-conversions]

char c = rand() % 62; ^

github-actions · 2024-04-24T01:47:55Z

core/file/utils.cpp

+
+namespace {
+inline char random_char() {
+  char c = rand() % 62;


warning: function is not thread safe [concurrency-mt-unsafe]

char c = rand() % 62; ^

github-actions · 2024-04-24T01:47:55Z

core/file/utils.cpp

+inline char random_char() {
+  char c = rand() % 62;
+  if (c < 10)
+    return c + 48;


warning: narrowing conversion from 'int' to signed type 'char' is implementation-defined [bugprone-narrowing-conversions]

return c + 48; ^

github-actions · 2024-04-24T01:47:56Z

core/file/utils.cpp

+
+void remove(const std::string &file) {
+  if (std::remove(file.c_str()))
+    throw Exception("error deleting file \"" + file + "\": " + strerror(errno));


warning: function is not thread safe [concurrency-mt-unsafe]

throw Exception("error deleting file \"" + file + "\": " + strerror(errno)); ^

github-actions · 2024-04-24T01:47:56Z

core/file/utils.cpp

+  DEBUG(std::string("creating ") + (size ? "" : "empty ") + "file \"" + filename + "\"" +
+        (size ? " with size " + str(size) : ""));
+
+  int fid;


warning: variable 'fid' is not initialized [cppcoreguidelines-init-variables]

Suggested change

int fid;

int fid = 0;

github-actions · 2024-04-24T01:47:56Z

core/file/utils.cpp

+        (size ? " with size " + str(size) : ""));
+
+  int fid;
+  while ((fid = open(filename.c_str(),


warning: do not call c-style vararg functions [cppcoreguidelines-pro-type-vararg]

while ((fid = open(filename.c_str(), ^

github-actions · 2024-04-24T01:47:56Z

core/file/utils.cpp

+      INFO("file \"" + filename + "\" already exists - removing");
+      remove(filename);
+    } else
+      throw Exception("error creating output file \"" + filename + "\": " + std::strerror(errno));


warning: function is not thread safe [concurrency-mt-unsafe]

throw Exception("error creating output file \"" + filename + "\": " + std::strerror(errno)); ^

github-actions · 2024-04-24T01:47:56Z

core/file/utils.cpp

+      throw Exception("error creating output file \"" + filename + "\": " + std::strerror(errno));
+  }
+  if (fid < 0) {
+    std::string mesg("error creating file \"" + filename + "\": " + strerror(errno));


warning: function is not thread safe [concurrency-mt-unsafe]

std::string mesg("error creating file \"" + filename + "\": " + strerror(errno)); ^

github-actions

clang-tidy made some suggestions

There were too many comments to post at once. Showing the first 25 out of 31. Check the log or trigger a new build to see more.

github-actions · 2024-04-24T10:23:29Z

core/file/utils.cpp

+    throw Exception(mesg);
+  }
+
+  if (size)


warning: implicit conversion 'int64_t' (aka 'long') -> bool [readability-implicit-bool-conversion]

Suggested change

if (size)

if (size != 0)

github-actions · 2024-04-24T10:23:29Z

core/file/utils.cpp

+    size = ftruncate(fid, size);
+  close(fid);
+
+  if (size)


warning: implicit conversion 'int64_t' (aka 'long') -> bool [readability-implicit-bool-conversion]

Suggested change

if (size)

if (size != 0)

github-actions · 2024-04-24T10:23:29Z

core/file/utils.cpp

+  close(fid);
+
+  if (size)
+    throw Exception("cannot resize file \"" + filename + "\": " + strerror(errno));


warning: function is not thread safe [concurrency-mt-unsafe]

throw Exception("cannot resize file \"" + filename + "\": " + strerror(errno)); ^

github-actions · 2024-04-24T10:23:29Z

core/file/utils.cpp

+void resize(const std::string &filename, int64_t size) {
+  DEBUG("resizing file \"" + filename + "\" to " + str(size));
+
+  int fd = open(filename.c_str(), O_RDWR, S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH | S_IWOTH);


github-actions · 2024-04-24T10:23:29Z

core/file/utils.cpp

+void resize(const std::string &filename, int64_t size) {
+  DEBUG("resizing file \"" + filename + "\" to " + str(size));
+
+  int fd = open(filename.c_str(), O_RDWR, S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH | S_IWOTH);


github-actions · 2024-04-24T10:23:30Z

core/file/utils.cpp

+
+  int status = size ? ftruncate(fid, size) : 0;
+  close(fid);
+  if (status)


warning: implicit conversion 'int' -> bool [readability-implicit-bool-conversion]

Suggested change

if (status)

if (status != 0)

github-actions · 2024-04-24T10:23:30Z

core/file/utils.cpp

+  int status = size ? ftruncate(fid, size) : 0;
+  close(fid);
+  if (status)
+    throw Exception("cannot resize file \"" + filename + "\": " + strerror(errno));


warning: function is not thread safe [concurrency-mt-unsafe]

throw Exception("cannot resize file \"" + filename + "\": " + strerror(errno)); ^

github-actions · 2024-04-24T10:23:30Z

core/file/utils.cpp

+}
+
+void mkdir(const std::string &folder) {
+  if (::mkdir(folder.c_str()


warning: implicit conversion 'int' -> bool [readability-implicit-bool-conversion]

core/file/utils.cpp:204:

- )) + ) != 0)

github-actions · 2024-04-24T10:23:30Z

core/file/utils.cpp

+              0777
+#endif
+              ))
+    throw Exception("error creating folder \"" + folder + "\": " + strerror(errno));


warning: function is not thread safe [concurrency-mt-unsafe]

throw Exception("error creating folder \"" + folder + "\": " + strerror(errno)); ^

github-actions · 2024-04-24T10:23:30Z

core/file/utils.cpp

+    throw Exception("error creating folder \"" + folder + "\": " + strerror(errno));
+}
+
+void rmdir(const std::string &folder, bool recursive) {


warning: function 'rmdir' is within a recursive call chain [misc-no-recursion]

void rmdir(const std::string &folder, bool recursive) { ^

Additional context

core/file/utils.cpp:208: example recursive call chain, starting from function 'rmdir'

void rmdir(const std::string &folder, bool recursive) { ^

core/file/utils.cpp:215: Frame #1: function 'rmdir' calls function 'rmdir' here:

rmdir(path, true); ^

core/file/utils.cpp:215: ... which was the starting point of the recursive call chain; there may be other cycles

rmdir(path, true); ^

CMake seems to have issue with using PCHs together with sccache on Mac. Disabling them also is a good test to ensure that we can compile the project with MRTRIX_USE_PCH=OFF

Lestropie · 2024-05-09T10:42:46Z

Windows 11 machine through MSYS2, 16 cores, Ninja 1.12 (already available on MSYS2), average of 3 runs:

	`dev`	new
GCC	202s	118s
Clang	183s	106s

Can't currently test on my work Linux laptop as it's partway through a script doing 72 bedpostx runs and I don't particularly want to interrupt it.

I agree regarding abuse of inlining / too many implementations in header files / would benefit from use of extern template. Chances are most of the benefits have already been obtained here, and there may be somewhat diminishing returns hunting for more. So I'm not sure it's worth delaying the merge. I will myself try to keep in mind whenever any given area of code is under modification to consider rectifying such things on a case-by-case basis.

Conflicts: core/file/utils.h

Lestropie

Merge conflicts resolved; should be good to go. Nice work!

github-actions

clang-tidy made some suggestions

github-actions · 2024-05-09T11:23:47Z

core/file/utils.cpp

+
+namespace {
+inline char random_char() {
+  const char c = rand() % 62;


warning: narrowing conversion from 'int' to signed type 'char' is implementation-defined [bugprone-narrowing-conversions]

const char c = rand() % 62; ^

github-actions · 2024-05-09T11:23:47Z

core/file/utils.cpp

+
+namespace {
+inline char random_char() {
+  const char c = rand() % 62;


warning: function is not thread safe [concurrency-mt-unsafe]

const char c = rand() % 62; ^

github-actions · 2024-05-09T11:23:47Z

core/file/utils.cpp

+void resize(const std::string &filename, int64_t size) {
+  DEBUG("resizing file \"" + filename + "\" to " + str(size));
+
+  const int fd = open(filename.c_str(), O_RDWR, S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH | S_IWOTH);


github-actions · 2024-05-09T11:23:48Z

core/file/utils.cpp

+  DEBUG("creating temporary file of size " + str(size));
+
+  std::string filename(Path::join(tmpfile_dir(), tmpfile_prefix()) + "XXXXXX.");
+  const int rand_index = filename.size() - 7;


warning: narrowing conversion from 'size_type' (aka 'unsigned long') to signed type 'int' is implementation-defined [bugprone-narrowing-conversions]

const int rand_index = filename.size() - 7; ^

github-actions · 2024-05-09T11:23:48Z

core/file/utils.cpp

+    Path::Dir dir(folder);
+    std::string entry;
+    while (!(entry = dir.read_name()).empty()) {
+      std::string path = Path::join(folder, entry);


warning: variable 'path' of type 'std::string' (aka 'basic_string') can be declared 'const' [misc-const-correctness]

Suggested change

std::string path = Path::join(folder, entry);

std::string const path = Path::join(folder, entry);

github-actions · 2024-05-09T11:23:48Z

core/file/utils.cpp

+    }
+  }
+  DEBUG("deleting folder \"" + folder + "\"...");
+  if (::rmdir(folder.c_str()))


warning: implicit conversion 'int' -> bool [readability-implicit-bool-conversion]

Suggested change

if (::rmdir(folder.c_str()))

if (::rmdir(folder.c_str()) != 0)

github-actions · 2024-05-09T11:23:48Z

core/file/utils.cpp

+  }
+  DEBUG("deleting folder \"" + folder + "\"...");
+  if (::rmdir(folder.c_str()))
+    throw Exception("error deleting folder \"" + folder + "\": " + strerror(errno));


warning: function is not thread safe [concurrency-mt-unsafe]

throw Exception("error deleting folder \"" + folder + "\": " + strerror(errno)); ^

github-actions · 2024-05-09T11:23:48Z

src/registration/transform/base.cpp

+      trafo_half(decltype(trafo_half)::Identity()),
+      trafo_half_inverse(decltype(trafo_half_inverse)::Identity()),
+      centre(decltype(centre)::Zero()),
+      optimiser_weights(decltype(optimiser_weights)::Zero(number_of_parameters)),


warning: narrowing conversion from 'size_t' (aka 'unsigned long') to signed type 'Index' (aka 'long') is implementation-defined [bugprone-narrowing-conversions]

optimiser_weights(decltype(optimiser_weights)::Zero(number_of_parameters)), ^

daljit46 added the build label Apr 15, 2024

daljit46 self-assigned this Apr 15, 2024

daljit46 mentioned this pull request Apr 15, 2024

Improving build times #2876

Closed

daljit46 force-pushed the improve_build branch from 980bdaf to ba0f104 Compare April 15, 2024 10:25

daljit46 force-pushed the improve_build branch from ba0f104 to 3d27bf5 Compare April 15, 2024 18:36

daljit46 force-pushed the improve_build branch from 3d27bf5 to ef2a884 Compare April 15, 2024 18:38

daljit46 force-pushed the improve_build branch from ef2a884 to 4be3391 Compare April 15, 2024 19:08

Lestropie reviewed Apr 15, 2024

View reviewed changes

cmd/CMakeLists.txt Show resolved Hide resolved

Lestropie reviewed Apr 15, 2024

View reviewed changes

core/image.h Outdated Show resolved Hide resolved

Lestropie reviewed Apr 15, 2024

View reviewed changes

src/dwi/tractography/roi.cpp Show resolved Hide resolved

daljit46 marked this pull request as draft April 17, 2024 08:27

github-actions bot reviewed Apr 17, 2024

View reviewed changes

daljit46 force-pushed the improve_build branch 4 times, most recently from 612a14a to 8a827a8 Compare April 22, 2024 14:41

daljit46 added 4 commits April 22, 2024 15:54

Enable the use of precompiled headers

97eb211

Minimise number of instantiation of heavy templates

db5f24b

We use extern template to reduce the number of instantiations of some templates to reduce compilation time.

Enable PCHs for executables

d23cbab

Move non-templated functions in separate source

5b40ea4

daljit46 force-pushed the improve_build branch from 8a827a8 to 5b40ea4 Compare April 22, 2024 15:01

daljit46 requested a review from jdtournier April 22, 2024 15:22

daljit46 marked this pull request as ready for review April 22, 2024 15:22

This comment was marked as outdated.

Sign in to view

*/CMakeLists.txt: Indentation fixes

5be0b49

daljit46 added 2 commits April 23, 2024 10:49

Address some clang-tidy warnings

276b8c0

Use file GENERATE to create dummy cpp files

ee1cbb5

This avoids their recompilation every time the project is configured

daljit46 force-pushed the improve_build branch from cc149f5 to ee1cbb5 Compare April 23, 2024 10:04

github-actions bot reviewed Apr 23, 2024

View reviewed changes

Fixes to compilation with precompiled headers

480bebb

github-actions bot reviewed Apr 24, 2024

View reviewed changes

daljit46 force-pushed the improve_build branch from c5a6332 to 600ff63 Compare April 24, 2024 09:59

github-actions bot reviewed Apr 24, 2024

View reviewed changes

daljit46 mentioned this pull request Apr 26, 2024

Simplify CI for merging to dev #1559

Closed

Lestropie mentioned this pull request May 9, 2024

Unable to build debug preset on Windows #2896

Closed

Lestropie and others added 3 commits May 9, 2024 10:16

Registration::Transform::Base: Change Eigen member initialisation

b536100

Disable PCHs on MacOS CI

a55d6fd

CMake seems to have issue with using PCHs together with sccache on Mac. Disabling them also is a good test to ensure that we can compile the project with MRTRIX_USE_PCH=OFF

Add missing include

14da966

daljit46 force-pushed the improve_build branch from 600ff63 to 14da966 Compare May 9, 2024 09:16

Lestropie mentioned this pull request May 9, 2024

Universal revisions based on clang-tidy #2828

Open

10 tasks

Merge remote-tracking branch 'origin/dev' into improve_build

4f6f64d

Conflicts: core/file/utils.h

Lestropie approved these changes May 9, 2024

View reviewed changes

Lestropie enabled auto-merge May 9, 2024 10:59

Lestropie merged commit 5dc1c7b into dev May 9, 2024
6 checks passed

Lestropie deleted the improve_build branch May 9, 2024 11:20

github-actions bot reviewed May 9, 2024

View reviewed changes

	Eigen::Matrix<default_type, 4, 1> jac;
	Eigen::Matrix<default_type, 4, 1> jac = 0;

	Eigen::Transform<ParameterType, 3, Eigen::AffineCompact> transform;
	Eigen::Transform<ParameterType, 3, Eigen::AffineCompact> transform = 0;


		void Base::compute_offset() { trafo.translation() = (trafo.translation() + centre - trafo.linear() * centre).eval(); }

		void Base::compute_halfspace_transformations() {

	Eigen::Matrix<ParameterType, 4, 4> tmp;
	Eigen::Matrix<ParameterType, 4, 4> tmp = 0;

	Eigen::Matrix<default_type, 4, 1> Base::get_jacobian_vector_wrt_params(const Eigen::Vector3d &) {
	Eigen::Matrix<default_type, 4, 1> Base::get_jacobian_vector_wrt_params(const Eigen::Vector3d & /unused/) {

	KeyValues::iterator i = config.find(key);
	KeyValues::iterator const i = config.find(key);

	int fd = open(filename.c_str(), O_RDWR, S_IRUSR \| S_IWUSR \| S_IRGRP \| S_IWGRP \| S_IROTH \| S_IWOTH);
	int const fd = open(filename.c_str(), O_RDWR, S_IRUSR \| S_IWUSR \| S_IRGRP \| S_IWGRP \| S_IROTH \| S_IWOTH);

	std::string path = Path::join(folder, entry);
	std::string const path = Path::join(folder, entry);

	if (::rmdir(folder.c_str()))
	if (::rmdir(folder.c_str()) != 0)

Optimisation of compilation speed with CMake/Ninja #2877

Optimisation of compilation speed with CMake/Ninja #2877

Conversation

daljit46 commented Apr 15, 2024 • edited Loading

github-actions bot commented Apr 15, 2024

github-actions bot commented Apr 15, 2024

daljit46 commented Apr 15, 2024 • edited Loading

github-actions bot commented Apr 15, 2024

github-actions bot commented Apr 15, 2024

Lestropie Apr 15, 2024

Choose a reason for hiding this comment

daljit46 Apr 16, 2024 • edited Loading

Choose a reason for hiding this comment

Lestropie commented Apr 15, 2024

daljit46 commented Apr 16, 2024 • edited Loading

daljit46 commented Apr 16, 2024 • edited Loading

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Apr 17, 2024

Choose a reason for hiding this comment

github-actions bot Apr 17, 2024

Choose a reason for hiding this comment

github-actions bot Apr 17, 2024

Choose a reason for hiding this comment

github-actions bot Apr 17, 2024

Choose a reason for hiding this comment

github-actions bot Apr 17, 2024

Choose a reason for hiding this comment

daljit46 commented Apr 17, 2024 • edited Loading

daljit46 commented Apr 22, 2024 • edited Loading

This comment was marked as outdated.

Lestropie commented Apr 22, 2024

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Apr 23, 2024

Choose a reason for hiding this comment

github-actions bot Apr 23, 2024

Choose a reason for hiding this comment

github-actions bot Apr 23, 2024

Choose a reason for hiding this comment

github-actions bot Apr 23, 2024

Choose a reason for hiding this comment

daljit46 Apr 23, 2024

Choose a reason for hiding this comment

github-actions bot Apr 23, 2024

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Apr 24, 2024

Choose a reason for hiding this comment

github-actions bot Apr 24, 2024

Choose a reason for hiding this comment

github-actions bot Apr 24, 2024

Choose a reason for hiding this comment

github-actions bot Apr 24, 2024

Choose a reason for hiding this comment

github-actions bot Apr 24, 2024

Choose a reason for hiding this comment

github-actions bot Apr 24, 2024

Choose a reason for hiding this comment

github-actions bot Apr 24, 2024

Choose a reason for hiding this comment

github-actions bot Apr 24, 2024

Choose a reason for hiding this comment

github-actions bot Apr 24, 2024

Choose a reason for hiding this comment

github-actions bot Apr 24, 2024

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Apr 24, 2024

Choose a reason for hiding this comment

github-actions bot Apr 24, 2024

Choose a reason for hiding this comment

github-actions bot Apr 24, 2024

Choose a reason for hiding this comment

github-actions bot Apr 24, 2024

Choose a reason for hiding this comment

github-actions bot Apr 24, 2024

Choose a reason for hiding this comment

daljit46 commented Apr 15, 2024 •

edited

Loading

daljit46 commented Apr 15, 2024 •

edited

Loading

daljit46 Apr 16, 2024 •

edited

Loading

daljit46 commented Apr 16, 2024 •

edited

Loading

daljit46 commented Apr 16, 2024 •

edited

Loading

daljit46 commented Apr 17, 2024 •

edited

Loading

daljit46 commented Apr 22, 2024 •

edited

Loading