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

Issue warnings for non-Release builds #701

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

stephenswat
Copy link
Member

A common pitfall when using traccc seems to be that people build it in non-Release mode and then get poor performance. This commit adds a bunch of warning prints to make sure that the code will inform you if you try to run it in non-Release mode.

@stephenswat stephenswat added the feature New feature or request label Sep 16, 2024
@krasznaa
Copy link
Member

So, you pretty much add this printout to every single executable as I can see it. 🤔 Would it not be easier to just print a warning when traccc::core is loaded? Then we wouldn't need to remember to put this macro into all the executables. Not to mention that I'm not even that big of a fan of how the macro is set up...

What I'm thinking of is a file called optimization_warning.cxx in core/src, with something like:

int checkOptimization() {
#if ...
    std::cerr << ...;
#endif
}
static const int dummy = checkOptimization();

It's a bit of a hack, but I've sort of enjoyed how RooFit has been printing a greeting message to its users with the same "trick" since forever. 🤔

@stephenswat
Copy link
Member Author

No, putting this straight into core is not a good idea; this is really just for the examples executables. Also, I would prefer this message to be at the same place as the performance results. Otherwise they will simply be overlooked.

@stephenswat
Copy link
Member Author

Updated to affect only the throughput examples.

A common pitfall when using traccc seems to be that people build it in
non-Release mode and then get poor performance. This commit adds a bunch
of warning prints to make sure that the code will inform you if you try
to run it in non-Release mode.
Copy link
Member

@krasznaa krasznaa left a comment

Choose a reason for hiding this comment

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

Shoot, wanted to make all comments part of the review...

You sure want users to notice these messages. 😆 3 printouts per job. 😮 (I'm on board though.)

@@ -0,0 +1,27 @@
/** TRACCC library, part of the ACTS project (R&D line)
*
* (c) 2022 CERN for the benefit of the ACTS project
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* (c) 2022 CERN for the benefit of the ACTS project
* (c) 2024 CERN for the benefit of the ACTS project

Comment on lines +12 to +27
#if !defined(TRACCC_BUILD_TYPE_IS_RELEASE) || !defined(NDEBUG) || \
defined(_DEBUG)
#define TRACCC_OPTIMIZATION_WARNING() \
do { \
std::cout \
<< "WARNING: traccc was built without Release mode, without the " \
"`NDEBUG` flag, or (on MSVC) with the `_DEBUG` flag. " \
"Performance is guaranteed to be much lower and compute " \
"performance results should be considered unreliable!" \
<< std::endl; \
} while (false)
#else
#define TRACCC_OPTIMIZATION_WARNING() \
do { \
} while (false)
#endif
Copy link
Member

Choose a reason for hiding this comment

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

Coming back to this... Why a macro at this point? A void optimization_warning() function would seem perfectly adequate here. The extra function call is not really an issue, it's all happening during the initialization of the throughput applications. 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Okay, I have no idea what the GitHub UI is doing now... 😕

@krasznaa
Copy link
Member

krasznaa commented Oct 1, 2024

Ping! It would be nice to put this in after some further changes.

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

Successfully merging this pull request may close these issues.

2 participants