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

Drop support for NDEBUG #3172

Merged
merged 1 commit into from
Dec 15, 2023
Merged

Drop support for NDEBUG #3172

merged 1 commit into from
Dec 15, 2023

Conversation

oschuett
Copy link
Member

@oschuett oschuett commented Dec 13, 2023

In CP2K we strive to not have asserts in tight loops.
So while there is no performance penalty, these asserts have saved the day countless times.

@oschuett oschuett force-pushed the current5 branch 2 times, most recently from 38e134d to 373c79c Compare December 13, 2023 21:12
@oschuett oschuett merged commit 0dfeee1 into cp2k:master Dec 15, 2023
34 checks passed
@hfp
Copy link
Member

hfp commented Dec 17, 2023

In the past we had not enough distinction in CP2K's support macros like using assertions to exit as well as side-effects in using certain macros. The worst I found was pointer navigation for some of the iterators (maybe DBCSR at the time). The "support" for NDEBUG was originally to test for side-effects or misuse/repurpose of certain support-macros. Also, using assertions in tight loops can hinder for instance vectorization since assert is like a system call.

@oschuett
Copy link
Member Author

In my opinion there are two big risks to using NDEBUG:

  1. Side-effects: While you sweeped for side-effcects in 2018, I don't think anyone has done so since. Presumably some have creeped back in.
  2. Wrong results: The CPASSERTS are often part of the regular user interface and protect against wrong inputs or unimplemented methods. And occasionally they even catch a bug that made it to production ;-)

On the other hand, the possible upside, namely performance gains, doesn't apply to CP2K. We don't have asserts in tight loops and generally keep a close eye on our performance. These days most of our inner loops are either in 3rd party libraries or the Fortran runtime anyways.

The bottom line is that we either have to support NDEBUG properly (incl. regular testing) or explicitly not support it.
Give the pros and cons above, I've decided to drop support for NDEBUG.

@hfp
Copy link
Member

hfp commented Dec 21, 2023

Side-effects: While you sweeped for side-effcects in 2018, I don't think anyone has done so since. Presumably some have creeped back in.

An alternative could be removing the #error directives and at least run "some tests" with NDEBUG. I agree, trying to ban NDEBUG (as merged by this PR) is the most rigorous way of controlling things. We can be lucky that CP2K's complexity locks-in on one of the supported build-systems, i.e., we actually control things like if not people are on their own anyway.

Regarding "some tests", I wonder if having one test-set being "randomized" is useful. I used non-deterministic test to increase coverage which was not affordable otherwise. In my case, I used selecting compilers on a randomized basis out of a set which was too large to run as a whole.

And occasionally they even catch a bug that made it to production ;-)

That's your point indeed. Otherwise it's endless to discuss if assertions are "error handlers". They are advanced debug tools, which convinces me saying one may want to deal with NDEBUG as a fact.

We don't have asserts in tight loops and generally keep a close eye on our performance. These days most of our inner loops are either in 3rd party libraries or the Fortran runtime anyways.

Indeed, CP2K's performance critical code is supposed to be covered elsewhere (which is a great achievement) like CP2K may be glue code or controlled in terms of assertions (DBM, GRID and a few other components).

It could be beneficial to increase tested configs beyond what we have based on randomized selections, e.g., compilers, NDEBUG, and other pieces.

@oschuett
Copy link
Member Author

We can be lucky that CP2K's complexity locks-in on one of the supported build-systems, i.e., we actually control things like if not people are on their own anyway.

It took 10+ years to get CMake support to where it's now. I seriously doubt anybody will use another build system.

Of course, people are free to tinker - that's the beauty of open source. It's just that we as a developer community, shouldn't spend resources supporting a feature that doesn't serve our users.

Regarding "some tests", I wonder if having one test-set being "randomized" is useful.

Yes, I've also toyed with the idea of fuzzing our configurations. It's certainly an intriguing approach.

Currently, we're heading more in the direction of fostering a few standard configrations.

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.

None yet

2 participants