build: add -DNDEBUG=1 to clang flags

[anonrig]

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]

Review requested:

• @nodejs/gyp

[gengjiawen]

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]

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

[joyeecheung]

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]

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]

@nodejs/build Appreciate any review

[targos]

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]

For reference, all other runtimes defines NDEBUG=1.

Can you point to a couple of them?

[anonrig]

Can you point to a couple of them?

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

[anonrig]

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

How can I not set it for debug builds?

[anonrig]

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

[anonrig]

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

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/52249/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/52431/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/52480/

[anonrig]

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

[joyeecheung]

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]

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]

@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]

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.