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

Change nanobind build options to remove -Os #2895

Merged
merged 2 commits into from
Nov 18, 2023
Merged

Conversation

jorgensd
Copy link
Sponsor Member

@jorgensd jorgensd commented Nov 17, 2023

With gcc, -Os (recommended by nanobind) can lead to significantly slower binaries than -O2. DOLFINx has considerable templated code, which gets compiled by the nanobind wrappers. This makes performance of the wrapper library sensitive to the compiler options.

This improves the run-time of #2891 to be better than in 0.7.1

Fixes #2891.

@IgorBaratta
Copy link
Member

IgorBaratta commented Nov 18, 2023

I confirm that the changes fix #2891 for me .

But:

Size optimizations can be disabled by specifying the optional NOMINSIZE argument, though doing so is not recommended.

from https://nanobind.readthedocs.io/en/latest/api_cmake.html

Is it then a bug in nanobind?

Copy link
Contributor

@chrisrichardson chrisrichardson left a comment

Choose a reason for hiding this comment

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

Do we need MODULE?

@chrisrichardson
Copy link
Contributor

@IgorBaratta - not a bug in nanobind, but a feature of the compiler. Each compiler may treat -Os differently.

@jorgensd
Copy link
Sponsor Member Author

Do we need MODULE?

Reading the docs Igor linked to,

nanobind_add_module() performs the following steps to produce bindings.

It creates a CMake library via add_library(target_name MODULE ...) and enables the use of C++17 features during compilation.

@garth-wells
Copy link
Member

With macOS/clang, the difference between -Os and -O3 is negligible for the code in #2891 (comment).

@jorgensd
Copy link
Sponsor Member Author

With macOS/clang, the difference between -Os and -O3 is negligible for the code in #2891 (comment).

I clearly has a large effect on linux with gcc.

@chrisrichardson
Copy link
Contributor

add_library needs MODULE - I don't think nanobind_add_module does.

@jorgensd
Copy link
Sponsor Member Author

add_library needs MODULE - I don't think nanobind_add_module does.

Feel free to test it. I'm currently rewriting dolfinx_mpc to support nanobind

@garth-wells garth-wells changed the title Change nanobind configuration to avoid minimizing binary size Change nanobind build options to remove -Os Nov 18, 2023
@garth-wells garth-wells added the build Build system and compiler issues label Nov 18, 2023
@garth-wells garth-wells added this pull request to the merge queue Nov 18, 2023
Merged via the queue into main with commit 764263c Nov 18, 2023
20 checks passed
@garth-wells garth-wells deleted the dokken/nanobind-config branch November 18, 2023 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Build system and compiler issues performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression in Python assemble_vector performance after nanobind migration
5 participants