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

Add WarpX_UNITY_BUILD #5702

Open
wants to merge 2 commits into
base: development
Choose a base branch
from
Open

Conversation

ax3l
Copy link
Member

@ax3l ax3l commented Feb 24, 2025

Unity builds combine all .cpp files into a single one (a single translation unit, TU). This is useful for performance optimization, computer science experiments, or differentiable programming.

FYI: A CPU unity build of ImpactX is around 18sec on my laptop (BLAST-ImpactX/impactx#861), for WarpX around 3:30min. In both cases, this refers to the lib* of ImpactX/WarpX part.

Unity builds combine all `.cpp` files into a single one
(a single translation unit, TU). This is useful for
performance optimization, computer science experiments,
or differentiable programming.
- Errors: redefinitions (fixed)
- Warnings: shadowing (started)

Ref.:
https://cmake.org/cmake/help/latest/prop_tgt/UNITY_BUILD_UNIQUE_ID.html
@ax3l ax3l requested review from EZoni, aeriforme and RemiLehe February 24, 2025 23:13
@@ -846,13 +846,13 @@
for (auto idx=0; idx<pc->NumRealComps(); idx++) {
if (write_real_comp[idx]) {
// handle scalar and non-scalar records by name
const auto [record_name, component_name] = detail::name2openPMD(real_comp_names[idx]);
const auto [record_name, component_name] = ::detail::name2openPMD(real_comp_names[idx]);

Check notice

Code scanning / CodeQL

Unused local variable Note

Variable component_name is not used.
@@ -869,13 +869,13 @@
for (auto idx=0; idx<int_counter; idx++) {
if (write_int_comp[idx]) {
// handle scalar and non-scalar records by name
const auto [record_name, component_name] = detail::name2openPMD(int_comp_names[idx]);
const auto [record_name, component_name] = ::detail::name2openPMD(int_comp_names[idx]);

Check notice

Code scanning / CodeQL

Unused local variable Note

Variable component_name is not used.
@lucafedeli88
Copy link
Member

Uh! We have to be careful with anonymous namespaces if we use unity builds!

@EZoni
Copy link
Member

EZoni commented Mar 1, 2025

@ax3l @lucafedeli88

This comment by Luca,

Uh! We have to be careful with anonymous namespaces if we use unity builds!

sounds like something we need to discuss properly before we proceed with moving more functions to anonymous namespaces. At this time I don't have enough knowledge of unity builds to have an informed understanding of Luca's statement, but if you do, it is probably a good idea to touch base on this and see what is the best way to move forward.

@ax3l
Copy link
Member Author

ax3l commented Mar 1, 2025

To clarify: anonymous namespaces are not the problem, identically named global variables repeated with the same name in different .cpp files are what one wants to avoid (in or outside global namespaces).

@lucafedeli88
Copy link
Member

To clarify: anonymous namespaces are not the problem, identically named global variables repeated with the same name in different .cpp files are what one wants to avoid (in or outside global namespaces).

Yes, sorry, I should have explained that better.... My point is that we are using anonymous namespaces more often than before, so the risk of conflicts may increase in the future. Maybe we could consider not using anonymous namespaces, but parent_namespace::details in the .cpp file to reduce the risk of conflicts. What do you think?

@ax3l
Copy link
Member Author

ax3l commented Mar 3, 2025

That is possible for sure and clean :)

Anonymous namespaces are generally still great to use, because what they do is they do not create an unnecessary symbol in the final binary and thus keep the binary a bit smaller. (That said, we can use compiler symbol hiding flags when we want to optimize for that, we do that in pybind11 for non-debug builds.)

Another solution I could have done for this one collision here: move the global variable to a header file (in a regular namespace) and define it inline or static.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants