Add macros to throw in debug builds and log in production #534
Conversation
|
CC @byule this is the macro we discussed recently for debug/test only asserts. |
|
Our internal code has a very similar macro called |
src/workerd/jsg/jsg.h
Outdated
| #ifdef KJ_DEBUG | ||
| void assertInvariantImpl(); | ||
| #endif | ||
| void assertInvariant(); |
There was a problem hiding this comment.
This method was intentionally designed to be optimized out in release mode, since it is called every time a Data is moved (note that Data underlies jsg::Value and V8Ref).
There was a problem hiding this comment.
Ah, so we should definitely be documenting that. Is the concern that isEmpty() will be expensive or that throwing an exception will prevent the move ctor from being elided?
There was a problem hiding this comment.
I think just having an out-of-line function call in a move constructor likely makes life difficult for the compiler. Like I would hope the compiler can see when a variable is moved-away and then subsequently destroyed, the destructor can be elided because the move nulled out all the pointers. But if there's an out-of-line function call in the middle of the move then probably the compiler can't reason about it at all anymore.
Both performance and code bloat are concerns since we pass V8Refs by move in quite a few places.
Of course, if actual measurements show this has negligible impact, then I don't have a problem. But I dunno if it's worth spending time to determine that, vs. just leaving this as it is. I feel like the check here should provide sufficient coverage running only in debug mode, and I'm not even sure if I've ever seen it fail in any case.
There was a problem hiding this comment.
Ahhhhhh, got it. Even if I felt confident that each rvalue ctor would be elided (it is allowed despite side effects after all), I wouldn't want to do this anyway since we'd run the risk of big log bursts if the assertion was incorrect when invoking the move assignment operator.
Hmmmm, yeah, I'd be fine with using that more generally instead. (The situations I care about do feel like ERROR to me, but I was on the fence about providing a severity anyway.) Its use implies that we are not supporting debug releases of workerd for non-testing usage. Is that accurate? |
Yeah, I'd say so. |
We already had a near identical macro in use in source files, let's provide it for more general usage with some warnings. Co-authored-by: Milan Miladinovic <milan@cloudflare.com>
bcaimano
force-pushed
the
bcaimano/sentry-log-assert
branch
from
2d89cb9 to
a94ac02
Compare
|
Fyi, @MellowYarker I tagged you as a coauthor since you wrote our internal version. |
We already had a near identical macro in use in source files, let's provide it for more general usage with some warnings.