manifest install: Cache compiler information for change detection#362
manifest install: Cache compiler information for change detection#362autoantwort wants to merge 42 commits into
Conversation
f982f12 to
372a6f7
Compare
372a6f7 to
aac20fc
Compare
|
A few independent thoughts: MSBuild parityI think we should consider changing our CMake integration to use the same algorithm as MSBuild. Currently, MSBuild considers:
We should add "full vcpkg command line" to that list in both cases. My concern would be only that this will not pick up changes to overlay port contents or to the triplet files. LockingRight now, vcpkg generally performs a bunch of file locking in manifest mode that will cause this "lightweight" check to be globally serialized. For CMake usage this is less important since it's often called only once for the whole repo, but for more complex scenarios and other buildsystems it could be problematic. Avoid hashing file contentsIf the intent of this is to be a lightweight check, we should avoid hashing file contents altogether. Instead, I would suggest:
If there's a mismatch, perform the install. Once the install completes, write the new paths hash to the stamp (which updates the timestamp). We could potentially go even further here by instead having vcpkg actually report out the set of files considered as part of the build, a la |
Yeah I think we should also check the timestamp of the triplet files and the overlay port files.
For msbuild this is a good thing if we still use the builtin msbuild change detection. If you for example have 10 projects in your solution and a vcpkg related change is detected, vcpkg runs 10 times in parallel. If the check is done after the lock, the first vcpkg instance will install the dependencies and then all the others will detect that there are no changes and will skip installation. If we would do the check before the locking, every vcpkg instance will detect a change and tries to install everything.
We can simply check the modified date of the folders. The modified date of a folder is updated if a file is deleted, added or renamed. Otherwise sounds good :) |
# Conflicts: # locales/messages.json # src/vcpkg/base/hash.cpp
|
@ras0219-msft Are you happy with the new changes? :) |
# Conflicts: # include/vcpkg/commands.setinstalled.h # locales/messages.en.json # locales/messages.json # src/vcpkg/commands.setinstalled.cpp # src/vcpkg/install.cpp
# Conflicts: # locales/messages.en.json # locales/messages.json
# Conflicts: # src/vcpkg/install.cpp
|
It would be really neat if this were merged, this is really slowing down my builds. |
# Conflicts: # include/vcpkg/commands.setinstalled.h # locales/messages.en.json # locales/messages.json # src/vcpkg/base/strings.cpp # src/vcpkg/commands.setinstalled.cpp # src/vcpkg/install.cpp
# Conflicts: # include/vcpkg/commands.setinstalled.h # locales/messages.en.json # locales/messages.json # src/vcpkg/commands.porthistory.cpp # src/vcpkg/commands.setinstalled.cpp # src/vcpkg/install.cpp
|
@BillyONeal Any news? :) |
|
I think for some reason I had in my mind that someone else specific was to review this one. Sorry, I don't have updated news for you. |
|
Thank you for the update :) Maybe you can poke the person. |
To clarify, I had in my mind that someone was supposed to look at this but that doesn't appear to be the case |
|
Any update/news on this? |
Given that the discussion in #362 (comment) isn't resolved I don't see progress happening here. I did some more testing and it looks like the majority of the time is not SHA1-ing the compiler binaries, but is instead CMake detecting which compiler to use. Note that I intentionally used clang in this example, which is a 116MB binary, rather than cl.exe which is like 3MB: PS D:\vcpkg> Measure-Command { cmake.exe -DCURRENT_PORT_DIR=D:\vcpkg\scripts\detect_compiler -DCURRENT_BUILDTREES_DIR=D:\vcpkg\buildtrees -DCURRENT_PACKAGES_DIR=D:\vcpkg\packages\detect_compiler_x64-windows -D_HOST_TRIPLET=x64-windows -DCMD=BUILD -DDOWNLOADS=D:\vcpkg-downloads -DTARGET_TRIPLET=x64-windows -DTARGET_TRIPLET_FILE=D:\vcpkg\triplets\x64-windows.cmake -DVCPKG_ROOT_DIR=D:\vcpkg -DPACKAGES_DIR=D:\vcpkg\packages -DBUILDTREES_DIR=D:\vcpkg\buildtrees -D_VCPKG_INSTALLED_DIR=D:\vcpkg\installed -DVCPKG_MANIFEST_INSTALL=OFF -DVCPKG_BASE_VERSION=2999-12-12 -DDO_HASH=OFF -P .\scripts\ports.cmake }
[1/1] "C:/Program Files/CMake/bin/cmake.exe" -E chdir ".." "C:/Program Files/CMake/bin/cmake.exe" "D:/vcpkg/scripts/detect_compiler" "-G" "Ninja" "-DCMAKE_BUILD_TYPE=Release" "-DCMAKE_INSTALL_PREFIX=D:\vcpkg\packages\detect_compiler_x64-windows" "-DDO_HASH=OFF" "-DCMAKE_MAKE_PROGRAM=D:/vcpkg-downloads/tools/ninja/1.10.2-windows/ninja.exe" "-DBUILD_SHARED_LIBS=ON" "-DVCPKG_CHAINLOAD_TOOLCHAIN_FILE=D:/vcpkg/scripts/toolchains/windows.cmake" "-DVCPKG_TARGET_TRIPLET=x64-windows" "-DVCPKG_SET_CHARSET_FLAG=ON" "-DVCPKG_PLATFORM_TOOLSET=" "-DCMAKE_EXPORT_NO_PACKAGE_REGISTRY=ON" "-DCMAKE_FIND_PACKAGE_NO_PACKAGE_REGISTRY=ON" "-DCMAKE_FIND_PACKAGE_NO_SYSTEM_PACKAGE_REGISTRY=ON" "-DCMAKE_INSTALL_SYSTEM_RUNTIME_LIBS_SKIP=TRUE" "-DCMAKE_VERBOSE_MAKEFILE=ON" "-DVCPKG_APPLOCAL_DEPS=OFF" "-DCMAKE_TOOLCHAIN_FILE=D:/vcpkg/scripts/buildsystems/vcpkg.cmake" "-DCMAKE_ERROR_ON_ABSOLUTE_INSTALL_DESTINATION=ON" "-DVCPKG_CXX_FLAGS=" "-DVCPKG_CXX_FLAGS_RELEASE=" "-DVCPKG_CXX_FLAGS_DEBUG=" "-DVCPKG_C_FLAGS=" "-DVCPKG_C_FLAGS_RELEASE=" "-DVCPKG_C_FLAGS_DEBUG=" "-DVCPKG_CRT_LINKAGE=dynamic" "-DVCPKG_LINKER_FLAGS=" "-DVCPKG_LINKER_FLAGS_RELEASE=" "-DVCPKG_LINKER_FLAGS_DEBUG=" "-DVCPKG_TARGET_ARCHITECTURE=x64" "-DCMAKE_INSTALL_LIBDIR:STRING=lib" "-DCMAKE_INSTALL_BINDIR:STRING=bin" "-D_VCPKG_ROOT_DIR=D:\vcpkg" "-DZ_VCPKG_ROOT_DIR=D:\vcpkg" "-D_VCPKG_INSTALLED_DIR=D:\vcpkg\installed" "-DVCPKG_MANIFEST_INSTALL=OFF"
-- The C compiler identification is Clang 18.1.6 with GNU-like command-line
-- The CXX compiler identification is Clang 18.1.6 with GNU-like command-line
#COMPILER_HASH#
#COMPILER_C_HASH#
#COMPILER_C_VERSION#18.1.6
#COMPILER_C_ID#Clang
#COMPILER_C_PATH#C:/Program Files/LLVM/bin/clang.exe
#COMPILER_CXX_HASH#
#COMPILER_CXX_VERSION#18.1.6
#COMPILER_CXX_ID#Clang
#COMPILER_CXX_PATH#C:/Program Files/LLVM/bin/clang++.exe
-- Configuring done (1.2s)
-- Generating done (0.0s)
CMake Warning:
Manually-specified variables were not used by the project:
BUILD_SHARED_LIBS
CMAKE_INSTALL_BINDIR
CMAKE_INSTALL_LIBDIR
_VCPKG_ROOT_DIR
-- Build files have been written to: D:/vcpkg/buildtrees/x64-windows-rel
Days : 0
Hours : 0
Minutes : 0
Seconds : 1
Milliseconds : 332
Ticks : 13323311
TotalDays : 1.54204988425926E-05
TotalHours : 0.000370091972222222
TotalMinutes : 0.0222055183333333
TotalSeconds : 1.3323311
TotalMilliseconds : 1332.3311
PS D:\vcpkg> Measure-Command { cmake.exe -DCURRENT_PORT_DIR=D:\vcpkg\scripts\detect_compiler -DCURRENT_BUILDTREES_DIR=D:\vcpkg\buildtrees -DCURRENT_PACKAGES_DIR=D:\vcpkg\packages\detect_compiler_x64-windows -D_HOST_TRIPLET=x64-windows -DCMD=BUILD -DDOWNLOADS=D:\vcpkg-downloads -DTARGET_TRIPLET=x64-windows -DTARGET_TRIPLET_FILE=D:\vcpkg\triplets\x64-windows.cmake -DVCPKG_ROOT_DIR=D:\vcpkg -DPACKAGES_DIR=D:\vcpkg\packages -DBUILDTREES_DIR=D:\vcpkg\buildtrees -D_VCPKG_INSTALLED_DIR=D:\vcpkg\installed -DVCPKG_MANIFEST_INSTALL=OFF -DVCPKG_BASE_VERSION=2999-12-12 -DDO_HASH=ON -P .\scripts\ports.cmake }
[1/1] "C:/Program Files/CMake/bin/cmake.exe" -E chdir ".." "C:/Program Files/CMake/bin/cmake.exe" "D:/vcpkg/scripts/detect_compiler" "-G" "Ninja" "-DCMAKE_BUILD_TYPE=Release" "-DCMAKE_INSTALL_PREFIX=D:\vcpkg\packages\detect_compiler_x64-windows" "-DDO_HASH=ON" "-DCMAKE_MAKE_PROGRAM=D:/vcpkg-downloads/tools/ninja/1.10.2-windows/ninja.exe" "-DBUILD_SHARED_LIBS=ON" "-DVCPKG_CHAINLOAD_TOOLCHAIN_FILE=D:/vcpkg/scripts/toolchains/windows.cmake" "-DVCPKG_TARGET_TRIPLET=x64-windows" "-DVCPKG_SET_CHARSET_FLAG=ON" "-DVCPKG_PLATFORM_TOOLSET=" "-DCMAKE_EXPORT_NO_PACKAGE_REGISTRY=ON" "-DCMAKE_FIND_PACKAGE_NO_PACKAGE_REGISTRY=ON" "-DCMAKE_FIND_PACKAGE_NO_SYSTEM_PACKAGE_REGISTRY=ON" "-DCMAKE_INSTALL_SYSTEM_RUNTIME_LIBS_SKIP=TRUE" "-DCMAKE_VERBOSE_MAKEFILE=ON" "-DVCPKG_APPLOCAL_DEPS=OFF" "-DCMAKE_TOOLCHAIN_FILE=D:/vcpkg/scripts/buildsystems/vcpkg.cmake" "-DCMAKE_ERROR_ON_ABSOLUTE_INSTALL_DESTINATION=ON" "-DVCPKG_CXX_FLAGS=" "-DVCPKG_CXX_FLAGS_RELEASE=" "-DVCPKG_CXX_FLAGS_DEBUG=" "-DVCPKG_C_FLAGS=" "-DVCPKG_C_FLAGS_RELEASE=" "-DVCPKG_C_FLAGS_DEBUG=" "-DVCPKG_CRT_LINKAGE=dynamic" "-DVCPKG_LINKER_FLAGS=" "-DVCPKG_LINKER_FLAGS_RELEASE=" "-DVCPKG_LINKER_FLAGS_DEBUG=" "-DVCPKG_TARGET_ARCHITECTURE=x64" "-DCMAKE_INSTALL_LIBDIR:STRING=lib" "-DCMAKE_INSTALL_BINDIR:STRING=bin" "-D_VCPKG_ROOT_DIR=D:\vcpkg" "-DZ_VCPKG_ROOT_DIR=D:\vcpkg" "-D_VCPKG_INSTALLED_DIR=D:\vcpkg\installed" "-DVCPKG_MANIFEST_INSTALL=OFF"
-- The C compiler identification is Clang 18.1.6 with GNU-like command-line
-- The CXX compiler identification is Clang 18.1.6 with GNU-like command-line
#COMPILER_HASH#0f4a11f3a8b3c6eee0390888b2ca585daca373eb
#COMPILER_C_HASH#cf5d8e7225d21ffe029bdf032edaaf8a66ea0594
#COMPILER_C_VERSION#18.1.6
#COMPILER_C_ID#Clang
#COMPILER_C_PATH#C:/Program Files/LLVM/bin/clang.exe
#COMPILER_CXX_HASH#cf5d8e7225d21ffe029bdf032edaaf8a66ea0594
#COMPILER_CXX_VERSION#18.1.6
#COMPILER_CXX_ID#Clang
#COMPILER_CXX_PATH#C:/Program Files/LLVM/bin/clang++.exe
-- Configuring done (1.6s)
-- Generating done (0.0s)
CMake Warning:
Manually-specified variables were not used by the project:
BUILD_SHARED_LIBS
CMAKE_INSTALL_BINDIR
CMAKE_INSTALL_LIBDIR
_VCPKG_ROOT_DIR
-- Build files have been written to: D:/vcpkg/buildtrees/x64-windows-rel
Days : 0
Hours : 0
Minutes : 0
Seconds : 1
Milliseconds : 753
Ticks : 17538336
TotalDays : 2.0299E-05
TotalHours : 0.000487176
TotalMinutes : 0.02923056
TotalSeconds : 1.7538336
TotalMilliseconds : 1753.8336
PS D:\vcpkg>So the hashing is approximately Given that @ras0219-msft does not want to accept any change that skips the CMake compiler detection over the potential to damage binary caches, I am unsure what changes could be made to get this PR into a state we could merge. |
|
To clarify, the problematic scenario is:
so we would have to have high confidence that anything that could possibly change which compiler CMake could pick invalidated this detection, and it seems unlikely to me that we will ever reach that level of confidence given how lots of how CMake does this is a black box. |
|
So the hashing is approximately (1.33-1.75)/1.33 ~= -31% of the time. Not nothing, but not the earthshattering difference in the original PR description.
I already sketched a solution but got no real response. If this is the only thing blocking this PR I will start implementing that :) |
Sorry I missed that, can you point specifically? |
|
#362 (comment)
|
I'm not sure how such changes are going to be possible in a way that truly sells that it's safe. |
Would you be willing to do just this part? It would fix your Mac test and is clearly less controversial.. |
# Conflicts: # include/vcpkg/commands.build.h # include/vcpkg/installedpaths.h # src/vcpkg/commands.build.cpp
…fo values when building a port
| if (used_cached_compiler_info) | ||
| { | ||
| if (!is_action_plan_fulfilled(action_plan, status_db)) | ||
| { | ||
| // we have to install something, so get fresh compiler infos | ||
| for (auto& install_action : action_plan.install_actions) | ||
| { | ||
| install_action.abi_info.clear(); | ||
| } | ||
| compute_all_abis(paths, action_plan, cmake_vars, {}); | ||
| used_cached_compiler_info = false; | ||
| } | ||
| } | ||
| adjust_action_plan_to_status_db(action_plan, status_db); | ||
| if (used_cached_compiler_info) | ||
| { | ||
| Checks::check_exit(VCPKG_LINE_INFO, action_plan.empty()); | ||
| } |
There was a problem hiding this comment.
Now the cached compiler info is only used to detect if there is something to install, if something gets installed, fresh compiler info are used.
(if you update your compiler the cache is not used)
|
At the risk of annoying everyone who's posted here before: any update on this? It's been more than three years since this pull request was created... |
You should see the majority of the benefit after microsoft/vcpkg#42994 and #1558 |
a48e045 to
71ecba3
Compare
|
In absolute terms: saves 2s on Windows and ~0.5s on MacOS, which is the majority of dry-run operations. |
# Conflicts: # src/vcpkg/commands.build.cpp
|
Is this blocked because of a technical issue or by your sparse review time? |
|
Would you consider merging it when it is hidden behind a feature flag that is disabled by default? |


Fixes microsoft/vcpkg#20549 (comment), microsoft/vcpkg#14025 and microsoft/vcpkg#29857.
The noop case now needs 110 ms instead of 2.2 seconds on my arm mac.
And from 2.0 sec to 18 ms when build as release