-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Enable UNITY_BUILD=ON on qgis_core (37% to 50% faster builds), qgis_gui (50% faster builds), qgis_analysis (3.7x faster) #59669
base: master
Are you sure you want to change the base?
Conversation
013ab87
to
d93ff88
Compare
ba4c5f5
to
da98dd1
Compare
🪟 Windows buildsDownload Windows builds of this PR for testing. 🪟 Windows Qt6 buildsDownload Windows Qt6 builds of this PR for testing. |
My gut feeling is this should be conditional, defaulting to off but on for ci. But I'm interested in other's thoughts! |
Interesting.
I agree, from a short read about the topic there's a risk of semantic changes (dangerous for release) and incremental builds may even suffer performance wise (?) As we do a lot of ccache for tests too, I wonder about the real effect there, is that still 37-50% faster? |
I've done that for GDAL and PROJ. Part of our CI use unity builds, part regular builds. The default is not unity builds, out of caution. I initially hit bugs, most of them caught at compilation time, a few residual ones by test suite (I've a memory of one that was really tricky to track down. It turned out to be due to code that relied on inherently undefined behavior, and with a unity build, the optimizer used that to optimize away the wrong code... OSGeo/gdal@7104ec6). So I'm inclined to make it conditioned by
That's a good question. It will depend on the content of pull requests. Typically a unity build reduces the number of compilation units by 8 or so. So if you have a pull request that touches only one .cpp file, at least 7 others will also be rebuilt. So that's something like going from a 1 second incremental build time to 8 seconds, considering all other are ccached. Not a big deal for CI purposes... But that might be a big deal for a developer wanting to quickly iterate during development and if struggling with some compilation errors (like fighting with templates, etc...) |
Sounds reasonable 👍 |
I've pushed extra changes:
For qgis_gui, unity builds for qt5 are 60% faster (from 5m 45s clock time down to 2m 12s, and 53m30s total cpu time down to 18m 8s) |
I'd love to see the impact on the windows-qt6 builds (MSVC is notoriously slow) |
d45a8c1
to
ff708f6
Compare
5eda51b
to
2680653
Compare
fedora:rawhide, qt6, qgis_gui, unity: real 6m30, user 45m20s, sys 5m1s ubuntu:22.04, qt5, qgis_analysis, unity: real 0m42s, user 5m39s, sys 0m26s
I've enabled it for windows-qt6 as well and the results are quite spectacular!
has clang for Windows been considered? It has the same C/C++ ABI as MSVC: https://clang.llvm.org/docs/MSVCCompatibility.html. |
2680653
to
0cd7f33
Compare
Wow! 🐝💨 With ccache working on current master ~40m
I've used it in the past and it performed better. I had troubles in other scenarios. I don't recall the details unfortunately, but it might be worth another try. |
0cd7f33
to
46212e0
Compare
@rouault I enabled ccache for testing reasons, I hope you don't mind |
…Woverloaded-virtual warning related to syncToLayer() method
1621b21
to
2ef66b0
Compare
Win Rebuild with ccache: 67m |
I found a master windows-qt6 build (https://github.com/qgis/QGIS/actions/runs/12070745214/job/33660951496) done by auto sipify which was 48 minute long, with following ccache stats:
and the latest build of this PR:
So for some reason unity builds would reduce the number of cacheable calls. The good and bad news is that it isn't a Windows specific behavior. I unfortunately reproduce that on ubuntu:22.04 I get a 0% cache hit when rebuilding qgis_core... |
hum trying on ubuntu 22.04, the bad news is that the incompatibility with ccache isn't related to unity builds but with the precompiled headers. We have propably merged #59667 too soon. |
9340fbf
to
4011529
Compare
… setting 'ccache -set-config sloppiness=pch_defines,time_macros'
4011529
to
075ea95
Compare
windows-qt6 build: first run 71 minutes, re-run 72 minutes with 65% cache it and 92% cacheable calls That said, on fedora:rawhide qt6, qgis_core builds with precompiled headers (independently of whather unity builds are enabled) do not trigger any ccache hits when wiping off build/src/core, in particular when removing the .gch precompiled header. It seems that its regeneration makes it to be considered as something non cacheable. Not sure if the issue is relate to the version of cmake, ccache, qt6, g++ or something else, compared to ubuntu 22.04 where things work just perfectly. I'm not sure what conclusion to draw from that. For developer use cases, where you don't normally wipe off your build directory, even the issue with fedora:rawhide qt6 shouldn't be much a practical issue. But for CI builds this is more annoying. |
@rouault does it help if you crank up the ccache max size? I've always found the default (on fedora at least) to be much too small |
no, that's not the issue. My cache has plenty of free space. The issue is that with ccache and/or g++ of fedora:rawhide the cmake_pch.hxx.gch generated file corresponding to the precompiler header causes an internal error to ccache and thus is regenerated at every fresh build when the .gch file is not already present on the file system, which, combined with the fact that gcc doesn't regenerate binary identical .gch files on repeated invocations on identical inputs (that applies even in trivial cases where the precompiled header contains nothing!), causes non-hits. Cf:
```
[root@50a8f20149e6 build_fedora_qt6]# ccache -z
Statistics zeroed
```
```
[root@50a8f20149e6 build_fedora_qt6]# ccache /usr/bin/c++ "-DCMAKE_SOURCE_DIR=\"/home/even/qgis/qgis\"" -DQT_CONCURRENT_LIB -DQT_CORE5COMPAT_LIB -DQT_CORE_LIB -DQT_DBUS_LIB -DQT_GUI_LIB -DQT_NETWORK_LIB -DQT_NO_CAST_TO_ASCII -DQT_NO_DEPRECATED_WARNINGS -DQT_NO_FOREACH -DQT_POSITIONING_LIB -DQT_PRINTSUPPORT_LIB -DQT_SERIALPORT_LIB -DQT_SQL_LIB -DQT_SVG_LIB -DQT_USE_QSTRINGBUILDER -DQT_WIDGETS_LIB -DQT_XML_LIB -DSIP_VERSION=0x060806 "-DTEST_DATA_DIR=\"/home/even/qgis/qgis/tests/testdata\"" -DWITH_COPC -DWITH_EPT -D_HAVE_PTHREAD_ -Dqgis_core_EXPORTS -I/home/even/qgis/qgis/build_fedora_qt6/src/core/qgis_core_autogen/include -I/home/even/qgis/qgis/build_fedora_qt6 -I/home/even/qgis/qgis/external/poly2tri -I/home/even/qgis/qgis/src/core/providers/ept -I/home/even/qgis/qgis/src/core/providers/copc -I/home/even/qgis/qgis/src/core/providers/vpc -I/home/even/qgis/qgis/build_fedora_qt6/src/core -I/home/even/qgis/qgis/external/meshOptimizer -I/home/even/qgis/qgis/src/core -I/home/even/qgis/qgis/src/core/3d -I/home/even/qgis/qgis/src/core/actions -I/home/even/qgis/qgis/src/core/annotations -I/home/even/qgis/qgis/src/core/auth -I/home/even/qgis/qgis/src/core/browser -I/home/even/qgis/qgis/src/core/callouts -I/home/even/qgis/qgis/src/core/classification -I/home/even/qgis/qgis/src/core/diagram -I/home/even/qgis/qgis/src/core/dxf -I/home/even/qgis/qgis/src/core/editform -I/home/even/qgis/qgis/src/core/effects -I/home/even/qgis/qgis/src/core/elevation -I/home/even/qgis/qgis/src/core/expression -I/home/even/qgis/qgis/src/core/externalstorage -I/home/even/qgis/qgis/src/core/fieldformatter -I/home/even/qgis/qgis/src/core/geometry -I/home/even/qgis/qgis/src/core/geocoding -I/home/even/qgis/qgis/src/core/gps -I/home/even/qgis/qgis/src/core/labeling -I/home/even/qgis/qgis/src/core/labeling/rules -I/home/even/qgis/qgis/src/core/layertree -I/home/even/qgis/qgis/src/core/layout -I/home/even/qgis/qgis/src/core/locator -I/home/even/qgis/qgis/src/core/maprenderer -I/home/even/qgis/qgis/src/core/mesh -I/home/even/qgis/qgis/src/core/metadata -I/home/even/qgis/qgis/src/core/network -I/home/even/qgis/qgis/src/core/numericformats -I/home/even/qgis/qgis/src/core/painting -I/home/even/qgis/qgis/src/core/pal -I/home/even/qgis/qgis/src/core/pdf -I/home/even/qgis/qgis/src/core/plot -I/home/even/qgis/qgis/src/core/pointcloud -I/home/even/qgis/qgis/src/core/pointcloud/expression -I/home/even/qgis/qgis/src/core/processing -I/home/even/qgis/qgis/src/core/processing/models -I/home/even/qgis/qgis/src/core/proj -I/home/even/qgis/qgis/src/core/project -I/home/even/qgis/qgis/src/core/providers -I/home/even/qgis/qgis/src/core/providers/arcgis -I/home/even/qgis/qgis/src/core/providers/memory -I/home/even/qgis/qgis/src/core/providers/gdal -I/home/even/qgis/qgis/src/core/providers/ogr -I/home/even/qgis/qgis/src/core/providers/meshmemory -I/home/even/qgis/qgis/src/core/providers/sensorthings -I/home/even/qgis/qgis/src/core/raster -I/home/even/qgis/qgis/src/core/renderer -I/home/even/qgis/qgis/src/core/scalebar -I/home/even/qgis/qgis/src/core/settings -I/home/even/qgis/qgis/src/core/sensor -I/home/even/qgis/qgis/src/core/stac -I/home/even/qgis/qgis/src/core/symbology -I/home/even/qgis/qgis/src/core/textrenderer -I/home/even/qgis/qgis/src/core/tiledscene -I/home/even/qgis/qgis/src/core/validity -I/home/even/qgis/qgis/src/core/vector -I/home/even/qgis/qgis/src/core/vectortile -I/home/even/qgis/qgis/src/core/web -I/home/even/qgis/qgis/external -I/home/even/qgis/qgis/external/delaunator-cpp -I/home/even/qgis/qgis/external/kdbush/include -I/home/even/qgis/qgis/external/nmea -I/home/even/qgis/qgis/external/rtree/include -I/home/even/qgis/qgis/external/tinygltf -isystem /usr/include/qt6/QtCore -isystem /usr/include/qt6 -isystem /usr/include/qt6/QtGui -isystem /usr/include/qt6/QtDBus -isystem /usr/include/qt6/QtXml -isystem /usr/include/qt6/QtWidgets -isystem /usr/include/qt6/QtSvg -isystem /usr/include/qt6/QtNetwork -isystem /usr/include/qt6/QtSql -isystem /usr/include/qt6/QtConcurrent -isystem /usr/include/qt6/QtCrypto -isystem /usr/include/qt6/QtSerialPort -isystem /usr/include/qt6/QtPrintSupport -isystem /usr/lib64/qt6/mkspecs/linux-g++ -isystem /usr/include/geos -isystem /usr/include/gdal -isystem /usr/include/qt6/QtCore5Compat -isystem /usr/include/qt6/QtPositioning -Wall -Wextra -Wno-long-long -Wformat-security -Wno-strict-aliasing -Wnon-virtual-dtor -Wno-redundant-move -Wno-misleading-indentation -Wno-deprecated-copy -g -std=gnu++17 -fPIC -fvisibility=hidden -x c++-header -include /home/even/qgis/qgis/build_fedora_qt6/src/core/CMakeFiles/qgis_core.dir/cmake_pch.hxx -MD -MT src/core/CMakeFiles/qgis_core.dir/cmake_pch.hxx.gch -MF src/core/CMakeFiles/qgis_core.dir/cmake_pch.hxx.gch.d -o src/core/CMakeFiles/qgis_core.dir/cmake_pch.hxx.gch -c /home/even/qgis/qgis/build_fedora_qt6/src/core/CMakeFiles/qgis_core.dir/cmake_pch.hxx.cxx
```
```
[root@50a8f20149e6 build_fedora_qt6]# ccache -s
Cacheable calls: 0 / 1 ( 0.00%)
Hits: 0
Direct: 0
Preprocessed: 0
Misses: 0
Errors: 1 / 1 (100.0%)
Local storage:
Cache size (GiB): 2.2 / 5.0 (44.24%)
```
I've enable ccache debugging with "ccache --set-config=debug=true" and "export CCACHE_LOGFILE=/tmp/ccache.log" but nothing in the log seems actionable. |
I just tested F41, qt5 build and I get a 100% hit rate (after the ccache config change) after a build folder wipe-and-rebuild:
For F41, qt6 build:
|
I don't know what's specific with qt6 and ccache. But I found that sometimes I manage to get hits for .pch regeneration by setting the ccache config "sloppiness = include_file_ctime, include_file_mtime, pch_defines, time_macros" (the include_file_ctime and _mtime speciically, because the pch_defines,time_macros are needed for qt5 as well), but that's not always sufficient. I don't manage to understand the exact circumstances where I get hits or misses |
ok, I finally figured out the magic needed to make ccache works properly on fedora:rawhide with QT6 and /usr/bin/c++ being the compiler which ccache doesn't understand out-of-the-box to be g++, which requires to set a ccache config item. Let's see if the changes also help for Windows Qt6... |
30994ec
to
116f81b
Compare
…MSVC, as ccache doesn't support that
I found ccache/ccache#1383 which indicates that there's no point in enabling precompiled headers with the combo MSVC + ccache, so I've aded a commit to disable them in that situation |
On top of PR #59667
Besides much faster compilation time, we also get massive reduction of the size of build artifacts !!!
That said UNITY_BUILD tend to be fragile. To debate if we want to enable them conditionally or not
With just precompiled qgis.h header:
vs with UNITY_BUILD, 37% faster:
With just precompiled qgis.h header:
vs with UNITY_BUILD, 50% faster: