Skip to content

Conversation

@iboB
Copy link
Contributor

@iboB iboB commented Aug 26, 2025

A new CMake-powered project is reconfigured often. Files are added and removed, the build config gets tweaked. If finufft is added as a CMake dependency (say via CPM, or FetchContent, or as a submodule) it adds an excessive amount of time on each reconfigure step.

This PR adds cmake code to cache the queries for supported compiler flags, instead of running the checks on every reconfigure. This greatly improves the reconfigure time. From a roughly 13s configure step for finufft, this takes it down to 5s. The vast majority of these 5s is in similar checks in fftw which are not properly cached (I'll probably make a pr add a PR for https://github.com/FFTW/fftw3/blob/master/CMakeLists.txt too)

The PR also include some ninja changes:

  • tweak .gitignore: group entries, add some common ide/editor artifacts
  • more concise checks for MSVC
  • rename check_arch_support to check_msvc_arch_support because this function is only used on msvc (props that its result is cached)

@DiamonDinoia
Copy link
Collaborator

Hi @iboB, thanks for the commit, I was thinking of a way to cache these checks. This seems good however, I will need to test it before approving.

It might conflict with #721. This might either wait a bit so that #721 is merged first or I might include it #721 if I have time before.

@DiamonDinoia
Copy link
Collaborator

If you can rebase this on top of master we will be able to merge (once I check that it does what is expected to).

Otherwise, I'll pick it up when I have time.

@iboB iboB force-pushed the improve-cmake-config branch from e41e37c to a11fb8f Compare October 15, 2025 04:11
@iboB iboB force-pushed the improve-cmake-config branch from a11fb8f to df5cb82 Compare October 15, 2025 04:15
@iboB
Copy link
Contributor Author

iboB commented Oct 15, 2025

Done.

I also improved the caching and based it on the hash if the input flags. Thus one can play with them without having to delete cache.

The ninja changes remain the same as described in the original PR

Copy link
Collaborator

@DiamonDinoia DiamonDinoia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the PR. I tested locally and it works.

Copy link
Collaborator

@DiamonDinoia DiamonDinoia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will wait for CI before merging.
(Now sure why it appeared twice. github lagged while typing...)

@DiamonDinoia
Copy link
Collaborator

Could you check if the problem is due to mismatched cached flags in CI now?

I will check with sanitizers later today if this an actual out of bounds read access.

@iboB
Copy link
Contributor Author

iboB commented Oct 15, 2025

To me it seems impossible that this PR would cause this. Even if there was a bug in it, it would lead to either setting flags that the compiler doesn't support (compilation error), or skipping flags that were meant to be set (missed optimizations). Moreover, all other gcc and clang configurations pass.

I'd say that this is either a runner/config issue leading to a false positive, or an actual rare out of bounds access that manifests more often with this config.

@DiamonDinoia
Copy link
Collaborator

I run the same config locally and it did not trigger.

@DiamonDinoia DiamonDinoia merged commit c131f98 into flatironinstitute:master Oct 15, 2025
41 of 42 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants