Skip to content
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

Correct CMAKE_BUILD_TYPE in end user Docker images #2894

Merged
merged 3 commits into from
Nov 18, 2023

Conversation

jorgensd
Copy link
Sponsor Member

Related to the nightly timings #2891.
We still have a regression with the introduction of nanobind, but now we are down to a halving in performance.

@jhale
Copy link
Member

jhale commented Nov 17, 2023

I would propose we remove DOLFINX_CMAKE_CXX_FLAGS=-O2 as RelWithDebInfo is usually picked as -O2 -g -DNDEBUG anyway.

Perhaps we should go for Release on these images, that typically enables -O3. How useful really is the DebugInfo for users?

Edit: I'm thinking the tagged versions e.g. :v0.7.2 should be done with Release, and :nightly should have RelWithDebInfo. It's not urgent, let's switch to RelWithDebInfo as you suggest for now.

@jhale
Copy link
Member

jhale commented Nov 17, 2023

We can discuss the nanobind performance issues separately.

@jorgensd
Copy link
Sponsor Member Author

I would propose we remove DOLFINX_CMAKE_CXX_FLAGS=-O2 as RelWithDebInfo is usually picked as -O2 -g -DNDEBUG anyway.

Perhaps we should go for Release on these images, that typically enables -O3. How useful really is the DebugInfo for users?

Edit: I'm thinking the tagged versions e.g. :v0.7.2 should be done with Release, and :nightly should have RelWithDebInfo. It's not urgent, let's switch to RelWithDebInfo as you suggest for now.

Agreed, this is a separate issue. It is mostly important to get some better performance on the nightly images.

@michalhabera
Copy link
Contributor

michalhabera commented Nov 17, 2023

Remove CMAKE_CXX_FLAGS, it does not work anyway because --config-settings=cmake.define.-DCMAKE_CXX_FLAGS is wrong and should be --config-settings=cmake.define.CMAKE_CXX_FLAGS. Moreover, build type overwrites these, so we'd need --config-settings=cmake.define.CMAKE_CXX_FLAGS_RELEASE for Release build etc.

@francesco-ballarin
Copy link
Member

Once we are done with the change proposed by @michalhabera , can @jorgensd manually trigger https://github.com/FEniCS/dolfinx/actions/workflows/docker-end-user.yml ?
After the nightly image is manually rebuilt, I'll trigger a handful of runs in the multiphenicsx repo to see if it is still affected by #2884

@jorgensd
Copy link
Sponsor Member Author

Once we are done with the change proposed by @michalhabera , can @jorgensd manually trigger https://github.com/FEniCS/dolfinx/actions/workflows/docker-end-user.yml ? After the nightly image is manually rebuilt, I'll trigger a handful of runs in the multiphenicsx repo to see if it is still affected by #2884

I can also trigger my own pipelines, as I've got several tests timing out because of this

@michalhabera
Copy link
Contributor

@jorgensd Can you also remove --config-settings=cmake.define.-DCMAKE_CXX_FLAGS?

@garth-wells
Copy link
Member

Let's improve issue and PR names. This is a Docker-only change but the title is very broad.

@jorgensd
Copy link
Sponsor Member Author

Let's improve issue and PR names. This is a Docker-only change but the title is very broad.

The issue is still a regression issue due to transfering to nanobind. Happy to rename this PR though.

@jorgensd jorgensd changed the title Fix end user build type Correct CMAKE_BUILD_TYPE in end user Docker images Nov 17, 2023
@francesco-ballarin
Copy link
Member

Can #2895 be merged before this, and then merge the updated main into this branch? I'll test the combined effect of both.

@garth-wells garth-wells added this pull request to the merge queue Nov 18, 2023
Merged via the queue into main with commit 067843d Nov 18, 2023
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants