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

build: add -DNDEBUG=1 to clang flags #48271

Closed
wants to merge 1 commit into from

Conversation

anonrig
Copy link
Member

@anonrig anonrig commented May 31, 2023

Even though we use CHECK instead of asserts in all of our codebase, we are still building and enabling assertions in standard libraries, and having those assertions comes with a cost.

cc @nodejs/build

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/gyp

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. labels May 31, 2023
@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 1, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 1, 2023
@gengjiawen
Copy link
Member

I don't think it's a good idea. We do rely on debug build to do checks like in v8. cc @nodejs/v8

@anonrig
Copy link
Member Author

anonrig commented Jun 1, 2023

@gengjiawen This effects standard library assertions as well such as assertions in std::string_view.

@joyeecheung
Copy link
Member

Have you measured how much cost there would be? I think I have rarely run into any STL assertions but if the impact is not significant it's good to keep them.

@anonrig
Copy link
Member Author

anonrig commented Jun 1, 2023

Have you measured how much cost there would be? I think I have rarely run into any STL assertions but if the impact is not significant it's good to keep them.

Any suggestions on benchmarks? I've just encountered a bug report where I realized we didn't enable NDEBUG. For reference, all other runtimes defines NDEBUG=1.

Issue: #48254

@anonrig
Copy link
Member Author

anonrig commented Jun 3, 2023

@nodejs/build Appreciate any review

@targos
Copy link
Member

targos commented Jun 4, 2023

I've just encountered a bug report where I realized we didn't enable NDEBUG

IMHO it shows that we should at least not set it for debug builds.

@targos
Copy link
Member

targos commented Jun 4, 2023

For reference, all other runtimes defines NDEBUG=1.

Can you point to a couple of them?

@anonrig
Copy link
Member Author

anonrig commented Jun 4, 2023

Can you point to a couple of them?

For example Bun: https://github.com/oven-sh/bun/blob/c4e31551f349c3ab0b15ef5adc4407848d109dd9/Makefile#L404

@anonrig
Copy link
Member Author

anonrig commented Jun 4, 2023

IMHO it shows that we should at least not set it for debug builds.

How can I not set it for debug builds?

@anonrig
Copy link
Member Author

anonrig commented Jun 13, 2023

@targos I've added NDEBUG=1 to release builds only. Can you take a look at it?

@anonrig
Copy link
Member Author

anonrig commented Jun 13, 2023

@nodejs/gyp @nodejs/build I appreciate any positive/negative feedback & review.

@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 14, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 14, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@anonrig
Copy link
Member Author

anonrig commented Aug 16, 2023

@nodejs/build @richardlau what are your thoughts on this?

@joyeecheung
Copy link
Member

Any suggestions on benchmarks?

I think the misc/startup benchmark could be a starting point to see overall impact. And maybe an HTTP benchmark that keeps lots of active connections alive, or a benchmark that targets some method that makes heavy use of STL (look for std::vector or std::map etc. in the code base should give you a clue)

@joyeecheung
Copy link
Member

Moving from slack discussion..after reading #48254 I think it's even useful to have the assertions? Calling [] /front() /back() on an empty string view should crash Node.js, IMO, it's better than having the undefined behavior (which may actually cause security issues by allowing arbitrary read..)

@anonrig
Copy link
Member Author

anonrig commented Sep 10, 2023

@joyeecheung I agree but this is a concern of the library not the implementor. In this case, I don't think we should run assertions a production application. We have debug builds just for this case. This also makes me question CHECK assertions on release builds as well.

@bnoordhuis
Copy link
Member

Assertions are excellent, they help catch bugs. They're an important reason why node and libuv are high quality projects.

W.r.t. performance, I'm very skeptical they have measurable overhead. Maybe there are one or two that perform expensive checks but you tackle those on a case-by-case basis, not by disabling all of them wholesale.

@anonrig anonrig closed this Oct 25, 2023
@anonrig anonrig deleted the build-with-ndebug branch October 25, 2023 23:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants