3D slicer incremental build takes 8 minutes on my Linux PC if I simply put my git branch on another commit (even if this commit does not change any c++ code).
Config: i7 12 cores and 32GB RAM
Build launch with this command in Slicer-build directory: make -j12
It is caused by Slicer-build/vtkSlicerVersionConfigure.h which is modified each time the commit is changed in the source directory.
We could store the constants of vtkSlicerVersionConfigure.h in a file read once when Slicer is launched to save build time without any performance drawback.
vtkSlicerVersionConfigure.h is needed for some #ifdefs that are used at compile time, so it would not be sufficient to make it available in a resource file only.
That said, a change in vtkSlicerVersionConfigure.h should not cause much rebuilding, as it is just used in a few cxx files (also in qSlicerAbstractModule.h, which could be probably removed), so it will just build and link a handful of libraries.
Could you please check if build takes a long time if you just update vtkSlicerVersionConfigure.h?
If that does not take too long then it may be something else triggers the lenghty recompilations/linkings.
Thanks for your quick answer.
Incremental build still takes the same time if I just update vtkSlicerVersionConfigure.h.
The first file recompiled is vtkMRMLVolumeSequenceStorageNode.cxx which is compiled quite quickly but then many linking must be done since it is in libMRMLCore which is used in many libs.
In the next days I will try to minimize the use of #include “vtkSlicerVersionConfigure.h” and propose a pull request.
libMRMLCore is supposed to be just an import library that loads the shared library. If the public interface of the library does not change then the import library does not change and no re-linking should occur. That’s why it is very important to avoid including vtkSlicerVersionConfigure.h in any header file. Try removing the include in qSlicerAbstractModule.h, it might solve the excessive rebuild issue.
I modify vtkMRMLVolumeSequenceStorageNode.cxx (simply add a space to a comment)
launch build in Slicer-build (make -j12)
A log screenshot is attached (I can provide the full log as a text file if you need it)
Removing vtkSlicerVersionConfigure.h in qSlicerAbstractModule.h is nice but it does not make significant change because my problem is really linking time of each library which depends on libraries modified by vtkSlicerVersionConfigure.h.
I would like to propose this improvement:
use a resource file everywhere it is possible
if information is required at build time: include vtkSlicerVersionConfigureInternal.h rather than vtkSlicerVersionConfigure.h
Removing references to vtkSlicerVersionConfigure.h in implementation (.cxx) files should not matter, as they should not trigger any relinking in other libraries. The .lib file is only modified when the public API (exported symbols in header files) is changed. When you change the content of a .cxx file then only the .so file is updated, which is not used during the linking (the content will only matter at runtime).
That said, I could reproduce the issue: touching vtkMRMLVolumeSequenceStorageNode.cxx and then running make -j20 took 52 seconds (while without this change, make took 2 seconds).
@jcfr Do you know what can cause this unnecessary re-linking?
On Windows, the build works as it is supposed to. After modifying vtkMRMLVolumeSequenceStorageNode.cxx (not just whitespace change but actual implementation change) the build time is just 3 seconds longer. If no file is modified then build takes 14 seconds (due to Python hierarchy files are updated at every build); after modifying the storage node implementation, build takes just slightly longer - 17 seconds.
@pieper could you test it on macOS? How long make -jN takes for you on a non-modified build tree and how long does it take after you make a small change in vtkMRMLVolumeSequenceStorageNode.cxx?
@aymeric.chataigner The problem can be fixed by adding set(CMAKE_LINK_DEPENDS_NO_SHARED TRUE) to the top-level CMakeLists.txt (near the top, for example right below the C++ standard section)!
If only the implementation (vtkMRMLVolumeSequenceStorageNode.cxx) is changed then rebuild takes a couple of seconds. If the header file (public interface) is changed then the rebuild takes about 30 seconds.
There is a very old discussion about enabling this flag by default. Some people brought up some contrived examples and some tests failed (maybe just on Windows?), so at that time they did not enable the CMAKE_LINK_DEPENDS_NO_SHARED flag by default, and it seems that it is still disabled by default.
@jcfr do you think we should enable CMAKE_LINK_DEPENDS_NO_SHARED or we should set target linking properties more carefully to prevent the unnecessary relinking on linux?
[…]vtkSlicerVersionConfigure.h […] in qSlicerAbstractModule.h […] could be probably removed,
Indeed, this was added back in 2012 in the context of Slicer@b6aab58a2.
Since then, in mid-2021, the approach has been revisited in Slicer@ae2e56b49 and there is no need have macro Slicer_VERSION_MAJOR and Slicer_VERSION_MINOR defined for module to compile.
Removing the include of vtkSlicerVersionConfigure.h in qSlicerAbstractModule.h should not be an issue for Slicer core.
And while it may affect some extension, it will be easy to fix.
Ideally yes, this will happen organically as we start transitioning the build system to fully adopt the modern cmake paradigm where the usage requirements are all associated with our targets. See here
In the meantime, enabling this option to improve the develop experience seems reasonable.
I just sent a quick note to our Kitware CMake channel to ask if there any caveats folks may be aware of.
I set CMAKE_LINK_DEPENDS_NO_SHARED to ON and I removed #include “vtkSlicerVersionConfigure.h” in qSlicerAbstractModule
Now if I build after a change in vtkSlicerVersionConfigure.h (like it is done by cmake when my git branch goes onto the next commit which just changes README.md) it takes 2min30s rather than 8min.
Should I propose a Pull Request for this simple changes ?
Naturally It would be nicer if a simple commit change should cause NO compilation, NO link.
As I said before, could I propose a pull request which:
use a resource file everywhere it is possible
if information is required at build time: include vtkSlicerVersionConfigureInternal.h rather than vtkSlicerVersionConfigure.h if it is possible
@aymeric.chataigner Can you confirm that building after touching vtkMRMLVolumeSequenceStorageNode.cxx just takes a couple of seconds now? That means that we solved a large part of the problem!
Cleaning up vtkSlicerVersionConfigure.h includes would be nice, it would be great if you could work on it. I think there are a couple of places where it is simply not needed.
Using a resource file will probably not be necessary, because it is simpler to get version information at runtime using qSlicerCoreApplication methods.
As its name (and comment in the file header) suggests, vtkSlicerVersionConfigureInternal.h is for internal use - only to be included in vtkSlicerVersionConfigure.h and not to be used in modules. We could rename and repurpose this .h file, but I’m not sure if it is worth it (if it can lead to significant build time reduction). What classes you would replace the include with the internal?
vtkMRMLVolumeSequenceStorageNode → The version check can be removed and we can just use the code snippet in the Slicer_VERSION_MAJOR > 4 branch.
vtkMRMLMarkupsFiducialStorageNode → it uses Slicer_VERSION, which is not available in vtkSlicerVersionConfigureInternal.h, so I think we need to keep using vtkSlicerVersionConfigure.h. It is just an implementation file in a small library, so recompiling&linking should not take more than a few seconds anyway.
qSlicerApplicationHelper → it uses Slicer_MAIN_PROJECT_VERSION_FULL, which is not available in vtkSlicerVersionConfigureInternal.h, so I think we need to keep using vtkSlicerVersionConfigure.h. If you find the recompilation of this file makes a huge difference then we can have a look how to avoid it.
I proposed these PR to continue to save more time during incremetal build:
On my PC with ubuntu 20.04 running an intel i7 6 hyperthreaded cores:
If I do make -j12:
Build slicer incremental: 35 seconds rather than 40 seconds
Build slicerRT incremental: 30 seconds rather than 1 minute
