Progress of wtf/Assertions.h and ASSERT macros deprecation

[TAMURA, Kent]

Thank you for helping the replacement task.

The number of instances as of March 31 -> November 21:

ASSERT() : 2,191 -> 1,195

RELEASE_ASSERT() : 122 -> 88

ENABLE(ASSERT) : 301 -> 185

ASSERT_NOT_REACHED() : 584 -> 320

ASSERT_UNUSED() : 115 -> 0

ASSERT_WITH_SECURITY_IMPLICATION() : 154 -> 0

As we noticed in the previous thread, RELEASE_ASSERT() -> CHECK() replacement would have performance regression on Windows.  However I guess the replacement in directories except wtf/ won't have visible performance impact, and we should try the replacement.

On Thu, Mar 31, 2016 at 4:55 PM, TAMURA, Kent <tk...@chromium.org> wrote:

Please replace the following macros in your code, except wtf/Vector.h and wtf/HashTable.h.

ASSERT  ->  DCHECK or DCHECK_EQ/NE/LE/LT/GE/GT

RELEASE_ASSERT  ->  CHECK or CHECK_EQ/NE/LE/LT/GE/GT

ENABLE(ASSERT)  ->  DCHECK_IS_ON()

ASSERT_NO_REACHED()  ->  NOTREACHED()

ASSERT_UNUSED  ->  DCHECK* and ALLOW_UNUSED_LOCAL() if necessary.

ASSERT_WITH_SECURITY_IMPLICATION  ->  SECURITY_DCHECK

Also, please remove WTF_LOG usage, use VLOG() or DVLOG instead.

You can associate cleanup CLs for them to crbug.com/596760.

----------------------------------------------------------------------------------------

Changes we already finished:

• RELEASE_ASSERT_WITH_SECURITY_IMPLICATION was already replaced with SECURITY_CHECK.
• RELEASE_ASSERT_NOT_REACHED was renamed to RELEASE_NOTREACHED.

We removed RELEASE_NOTREACHED recently because they were rarely used.  An alternative macro is LOG(FATAL), which also crashes in release build.

 

• ASSERT_ARG was removed.  Please use DCHECK.
• BACKTRACE() was removed.  Please use WTFReportBacktrace() or base::debug::StackTrace.
  StackTrace has a downside; crbug.com/598886
• FATAL() was removed.   Please use DLOG(FATAL).
• WTF_LOG_ERROR() was removed.  Please use DLOG(ERROR).
• ASSERT_WITH_MESSAGE and RELEASE_ASSERT_WITH_MESSAGE were removed.  We can specify messages to DCHECK() and CHECK().
• platform/NotImplemented.* were removed in order to remove WTFLogVerbose() defined in Assertions.h.  Please use NOTIMPLEMENTED().

  

 

FAQ:

* Why don't we update wtf/Vector.h and wtf/HashTable.h?

Chromecast team uses Release + DCHECK_ALWAYS_ON + ENABLE_ASSERT=0  configuration daily because of device memory restriction.  Enabling ASSERTs in Blink increases the binary size significantly, and the major part of the increase comes from wtf/Vector.h and wtf/HashTable.h.  So we'd like to keep a way to disable assertions in these files.

Chromecast team doesn't use this configuration any longer.  So we can assume ENABLE(ASSERT) == DCHECK_IS_ON(), and we can replace ASSERTs in wtf/.

--

TAMURA Kent
Software Engineer, Google

[Kentaro Hara]

On Tue, Nov 22, 2016 at 12:37 PM, TAMURA, Kent <tk...@chromium.org> wrote:

Thank you for helping the replacement task.

The number of instances as of March 31 -> November 21:

ASSERT() : 2,191 -> 1,195

RELEASE_ASSERT() : 122 -> 88

ENABLE(ASSERT) : 301 -> 185

ASSERT_NOT_REACHED() : 584 -> 320

ASSERT_UNUSED() : 115 -> 0

ASSERT_WITH_SECURITY_IMPLICATION() : 154 -> 0

As we noticed in the previous thread, RELEASE_ASSERT() -> CHECK() replacement would have performance regression on Windows.  However I guess the replacement in directories except wtf/ won't have visible performance impact, and we should try the replacement.

I'm a bit concerned about this. For example, bindings/ and platform/heap/ still have RELEASE_ASSERT()s on performance-sensitive places.

Do we have any timeline for fixing the performance issue on Windows?

--

Kentaro Hara, Tokyo, Japan

[Elliott Sprehn]

Yeah I don't think we can say performance doesn't matter outside wtf/ for CHECK. It needs to be fast everywhere or we should consider switching back to RELEASE_ASSERT.

[Charles Harrison]

I'd like a little advice. There's threading code living under #if enabled(ASSERT) here. We ASSERT on these functions in various places.

I've tried migrating these to use DCHECK but DCHECK has subtly different semantics than ASSERT. Namely:

// DCHECK et al. make sure to reference |condition| regardless of
// whether DCHECKs are enabled; this is so that we don't get unused
// variable warnings if the only use of a variable is in a DCHECK.
// This behavior is different from DLOG_IF et al.
This ends up causing issues, because when DCHECK/ASSERT is not enabled we don't compile or link these debugging functions in, but we need them at the call site even if DCHECK is not enabled. What is the correct way to go here? Should we try to remove all debug-only code? If we want to DCHECK on debug-only code do we need to wrap it in a DCHECK_IS_ON()?

Also +1 on what others have said about not moving to CHECK everywhere until it is fast.

--
You received this message because you are subscribed to the Google Groups "blink-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+unsubscribe@chromium.org.

[Daniel Cheng]

On Mon, Nov 21, 2016 at 7:56 PM Charles Harrison <cshar...@chromium.org> wrote:

I'd like a little advice. There's threading code living under #if enabled(ASSERT) here. We ASSERT on these functions in various places.

I've tried migrating these to use DCHECK but DCHECK has subtly different semantics than ASSERT. Namely:

// DCHECK et al. make sure to reference |condition| regardless of
// whether DCHECKs are enabled; this is so that we don't get unused
// variable warnings if the only use of a variable is in a DCHECK.
// This behavior is different from DLOG_IF et al.
This ends up causing issues, because when DCHECK/ASSERT is not enabled we don't compile or link these debugging functions in, but we need them at the call site even if DCHECK is not enabled. What is the correct way to go here? Should we try to remove all debug-only code? If we want to DCHECK on debug-only code do we need to wrap it in a DCHECK_IS_ON()?

As you mentioned, the typical way is to wrap DCHECK-only helpers in #if DCHECK_IS_ON().

Daniel

 

To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+...@chromium.org.

[Elliott Sprehn]

On Nov 21, 2016 7:56 PM, "Charles Harrison" <cshar...@chromium.org> wrote:
>
> I'd like a little advice. There's threading code living under #if enabled(ASSERT) here. We ASSERT on these functions in various places.
>
> I've tried migrating these to use DCHECK but DCHECK has subtly different semantics than ASSERT. Namely:
>
> // DCHECK et al. make sure to reference |condition| regardless of // whether DCHECKs are enabled; this is so that we don't get unused // variable warnings if the only use of a variable is in a DCHECK. // This behavior is different from DLOG_IF et al.
>
> This ends up causing issues, because when DCHECK/ASSERT is not enabled we don't compile or link these debugging functions in, but we need them at the call site even if DCHECK is not enabled. What is the correct way to go here? Should we try to remove all debug-only code? If we want to DCHECK on debug-only code do we need to wrap it in a DCHECK_IS_ON()?

You need to use the DCHECK_IS_ON() guard around it. Yeah it's ugly :/

>> To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+...@chromium.org.
>
>

[Morten Stenshorne]

One annoying thing with DCHECK, compared to ASSERT, is that when running
inside of gdb (Linux), and a DCHECK fails, the debugger doesn't stop in
the function where the DCHECK is, but rather in "base::debug::(anonymous
namespace)::DebugBreak()", which is called by
"base::debug::BreakDebugger()", which is called by
"logging::LogMessage::~LogMessage()", which is called by the function
where the failing DCHECK is.

ASSERT, on the other hand, when it fails, stops right on that line with
the ASSERT. Nice!

Is there any way to configure DCHECK to behave a bit more like ASSERT in
this regard, or some other trick that I'm not aware of?

--
---- Morten Stenshorne, developer, Opera Software ----
------------------ http://www.opera.com/ -------------

[TAMURA, Kent]

On Tue, Nov 22, 2016 at 12:47 PM, Kentaro Hara <har...@chromium.org> wrote:

On Tue, Nov 22, 2016 at 12:37 PM, TAMURA, Kent <tk...@chromium.org> wrote:

Thank you for helping the replacement task.

The number of instances as of March 31 -> November 21:

ASSERT() : 2,191 -> 1,195

RELEASE_ASSERT() : 122 -> 88

ENABLE(ASSERT) : 301 -> 185

ASSERT_NOT_REACHED() : 584 -> 320

ASSERT_UNUSED() : 115 -> 0

ASSERT_WITH_SECURITY_IMPLICATION() : 154 -> 0

As we noticed in the previous thread, RELEASE_ASSERT() -> CHECK() replacement would have performance regression on Windows.  However I guess the replacement in directories except wtf/ won't have visible performance impact, and we should try the replacement.

I'm a bit concerned about this. For example, bindings/ and platform/heap/ still have RELEASE_ASSERT()s on performance-sensitive places.

Do we have any timeline for fixing the performance issue on Windows?

We need to wait until Microsoft fixes the compiler issue, we find a workaround, or we switch to clang on Windows.

[Primiano Tucci]

> As we noticed in the previous thread, RELEASE_ASSERT() -> CHECK() replacement would have performance regression on Windows.

I might be missing something, but as things stand today RELEASE_ASSERT and CHECK seem almost identical in officilal builds, where is the difference coming from?

From wtf/Assertions.h:

#ifndef IMMEDIATE_CRASH

#if COMPILER(GCC) || COMPILER(CLANG)

#define IMMEDIATE_CRASH() __builtin_trap()

#else

#define IMMEDIATE_CRASH() ((void)(*(volatile char*)0 = 0))

#endif

#endif

#define RELEASE_ASSERT(assertion) (UNLIKELY(!(assertion)) ? (IMMEDIATE_CRASH()) : (void)0)

From base/logging.h

#if defined(COMPILER_GCC) || __clang__

#define LOGGING_CRASH() __builtin_trap()

#else

#define LOGGING_CRASH() ((void)(*(volatile char*)0 = 0))

#endif

#define CHECK(condition) !(condition) ? LOGGING_CRASH() : EAT_STREAM_PARAMETERS

Tangentially related, I'm trying to make some small changes to CHECK as they are currently affected by crbug.com/664209 

And now that I realize, I strongly believe that that bug is hitting also the current RELEASE_ASSERT .

Out of curiosity, where does the slowdown come from on windows?

I never looked at the disasm on Windows/MSVC, but on gcc/clang  ((void)(*(volatile char*)0 = 0)) or equivalent can interleave the crash code in the main flow  (depending on the -O) and generally emits more code as the compiler doesn't treat it as a noreturn function. See http://pastebin.com/mq2Cewgd .

> We need to wait until Microsoft fixes the compiler issue, we find a workaround, or we switch to clang on Windows.

Which compiler issue?

[TAMURA, Kent]

This one: https://connect.microsoft.com/VisualStudio/Feedback/Details/3112569

EAT_STREAM_PARAMETERS causes the issue.

[dan...@chromium.org]

FWIW I filed https://bugs.chromium.org/p/chromium/issues/detail?id=668300 in which I suggest maybe the fact that we see this huge perf difference from a no-opish change to CHECK signifies we have RELEASE_ASSERTs in very hot places and we should fix that instead. Something to look at.

[Elliott Sprehn]

On Nov 23, 2016 7:12 PM, <dan...@chromium.org> wrote:
>
> FWIW I filed https://bugs.chromium.org/p/chromium/issues/detail?id=668300 in which I suggest maybe the fact that we see this huge perf difference from a no-opish change to CHECK signifies we have RELEASE_ASSERTs in very hot places and we should fix that instead. Something to look at.

Yes, we have them inside things like Vector::operator[] on purpose. CHECK just needs to be fast. If RELEASE_ASSERT is faster we should keep it.

[Kentaro Hara]

For simplicity, I'd prefer just stopping all the RELEASE_ASSERT => CHECK replacements (or even revert the replacements we've done) until the performance issue is fixed.

If we don't want to mix RELEASE_"ASSERT" and D"CHECK" in the code base, we can rename RELEASE_ASSERT to BLINK_CHECK or something.

[dan...@chromium.org]

+scottmg landed https://codereview.chromium.org/2559323007/ recently which improves the codegen of CHECK's string eating on MSVC so it should produce the same output in official builds as RELEASE_ASSERT on Windows builds now.

[Primiano Tucci]

It would be great if somebody could re-re-land crrev.com/1992873004 to check if there is anything else left or we can finally stop worry about CHECK vs RELEASE_ASSERT after the aforementioned scottmg's fix.

For the records, I tried to reconstruct the chronological evolution of all this in case somebody needs archaeology for the perf waterfall or just references to bugs.

Hoping that I didn't leave anything out (this looks like playing Risk with checks, asserts and crash reporting)

#384213 : kotenkov@ replaces all occurrences of RELEASE_ASSERT in wtf with CHECK. (crrev.com/1840163002)
#384889 : tkent@ reverts #384213 (crrev.com/1854033002) because of android perf regresson (crbug.com/599867)
#394035 : thakis@ makes CHECK use __builtin_trap to fix the regression above (crrev.com/1982123002)
#395624 : kotenkov@ re-replaces all occurrences of RELEASE_ASSERT in wtf with CHECK. (crrev.com/1992873004)
#396441 : kotenkov@ re-reverts (crrev.com/2016223002) because of windows perf regression (crbug.com/614852)
#398084 : primiano@ reverts thakis@'s #394035 beacause of crash related reasons (crrev.com/2046593002)
#404249 : primiano@ relands thakis@'s #394035, crash stuff fixed (crrev.com/2125923002)
#437763 : scottmg@ lands a perf fix for CHECK on windows (crrev.com/2559323007)
#437764 : reverted #437763 as it broke some bots (crrev.com/2569583002)
#437777 : scottmg@ re-lands the fix. It sticks (crrev.com/2559323007)

Somewhat related I'm planning to change soon LOGGING_CRASH (used by official CHECK) in https://codereview.chromium.org/2502953003/ (not landed yet) as we found some other problems related with the quality of crash reports (crbug.com/664209). By looking at the assembly shouldn't affect performance w.r.t. today's state.

[TAMURA, Kent]

That's great!  Thank you for fixing it.

[Wez]

Has anyone tried re-re-landing crrev.com/1992873004 yet?

Is there a good way to gather the relevant metrics performance via try-bots rather than risking landing and having to revert again?

[Kentaro Hara]

Given that the performance issue has been resolved, I think we can just re-land it and see how it goes.

[Wez]

OK, I'll upload a refresh of that CL for the purpose.

[Wez]

On a related note, is there something stopping us patching wtf/Assertion.h to define things in terms of base/logging.h primitives now, e.g. ASSERT->DCHECK, RELEASE_ASSERT->CHECK and so-on?

Or are there specific properties of the WTF reporting functions that we need to preserve for now?

[TAMURA, Kent]

Do you mean |#define ASSERT(condition) DCHECK(condition)|? I think no blockers for it.

[Elliott Sprehn]

On Wed, Jan 11, 2017 at 1:35 PM, Wez <w...@chromium.org> wrote:

On a related note, is there something stopping us patching wtf/Assertion.h to define things in terms of base/logging.h primitives now, e.g. ASSERT->DCHECK, RELEASE_ASSERT->CHECK and so-on?

Or are there specific properties of the WTF reporting functions that we need to preserve for now?

We should try aliasing RELEASE_ASSERT and CHECK. ASSERT is actually behaviorally different, it totally omits the code in a release build. DCHECK still has the code and depends on the compiler to omit the code. I find this results in a some pretty gross stuff like:

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/heap/TraceTraits.h?q=DCHECK+file:webkit+lang:cpp+-file:gen+ifdef&sq=package:chromium&l=207&dr=C

#if DCHECK_IS_ON()

    DCHECK(HeapObjectHeader::fromPayload(t)->checkHeader());

#endif

I think we should add a new DCHECK form that omits the code entirely and then we can alias for it.

[Wez]

Kent, Elliott: Thanks for the updates.  I'll draft a patch to start aliasing those then.

Elliott: How would you feel about ASSERT temporarily being #ifdef'd between no-op and alias-to-DCHECK to achieve that result?  I'd prefer not to try to draft an alternative DCHECK until we've cleaned up some of the accrued debt in base/logging.h first. :P

[Elliott Sprehn]

On Wed, Jan 11, 2017 at 1:51 PM, Wez <w...@chromium.org> wrote:

Kent, Elliott: Thanks for the updates.  I'll draft a patch to start aliasing those then.

Elliott: How would you feel about ASSERT temporarily being #ifdef'd between no-op and alias-to-DCHECK to achieve that result?  I'd prefer not to try to draft an alternative DCHECK until we've cleaned up some of the accrued debt in base/logging.h first. :P

You mean hide the DCHECK_IS_ON() check inside the alias? SGTM. :)

[Wez]

Exactly ;)

[Brett Wilson]

On Wed, Jan 11, 2017 at 1:40 PM, Elliott Sprehn <esp...@chromium.org> wrote:

On Wed, Jan 11, 2017 at 1:35 PM, Wez <w...@chromium.org> wrote:

On a related note, is there something stopping us patching wtf/Assertion.h to define things in terms of base/logging.h primitives now, e.g. ASSERT->DCHECK, RELEASE_ASSERT->CHECK and so-on?

Or are there specific properties of the WTF reporting functions that we need to preserve for now?

We should try aliasing RELEASE_ASSERT and CHECK. ASSERT is actually behaviorally different, it totally omits the code in a release build. DCHECK still has the code and depends on the compiler to omit the code. I find this results in a some pretty gross stuff like:

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/heap/TraceTraits.h?q=DCHECK+file:webkit+lang:cpp+-file:gen+ifdef&sq=package:chromium&l=207&dr=C

#if DCHECK_IS_ON()

    DCHECK(HeapObjectHeader::fromPayload(t)->checkHeader());

#endif

I think we should add a new DCHECK form that omits the code entirely and then we can alias for it.

I notice this behavior was only recently added

https://chromium.googlesource.com/chromium/src/+/190f7546ebe213c61874ec8885be95c0f98a84d1

to work around another type of annoyance when using DCHECKs (unused variable warnings when disabled). Either way requires some ugly ifdefs in some cases.

We should just standardize on the way that generally results in the fewest ifdefs. I don't personally care which way this is.

Brett

[dan...@chromium.org]

On Wed, Jan 11, 2017 at 5:28 PM, Brett Wilson <bre...@chromium.org> wrote:

On Wed, Jan 11, 2017 at 1:40 PM, Elliott Sprehn <esp...@chromium.org> wrote:

On Wed, Jan 11, 2017 at 1:35 PM, Wez <w...@chromium.org> wrote:

On a related note, is there something stopping us patching wtf/Assertion.h to define things in terms of base/logging.h primitives now, e.g. ASSERT->DCHECK, RELEASE_ASSERT->CHECK and so-on?

Or are there specific properties of the WTF reporting functions that we need to preserve for now?

We should try aliasing RELEASE_ASSERT and CHECK. ASSERT is actually behaviorally different, it totally omits the code in a release build. DCHECK still has the code and depends on the compiler to omit the code. I find this results in a some pretty gross stuff like:

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/heap/TraceTraits.h?q=DCHECK+file:webkit+lang:cpp+-file:gen+ifdef&sq=package:chromium&l=207&dr=C

#if DCHECK_IS_ON()

    DCHECK(HeapObjectHeader::fromPayload(t)->checkHeader());

#endif

I think we should add a new DCHECK form that omits the code entirely and then we can alias for it.

I notice this behavior was only recently added

https://chromium.googlesource.com/chromium/src/+/190f7546ebe213c61874ec8885be95c0f98a84d1

to work around another type of annoyance when using DCHECKs (unused variable warnings when disabled).

This was to make DLOG act like DCHECK in when it is enabled or not, IIUC. The DCHECK behaviour is older.

 

Either way requires some ugly ifdefs in some cases.

Yeh, also "(void)unused_except_in_dchecks;" to avoid unused warnings.

 

We should just standardize on the way that generally results in the fewest ifdefs. I don't personally care which way this is.

+1

 

Brett

[Wez]

IIUC part of the difference here is that in Blink land, many ASSERTs are conditional on data-structures which are only maintained in DEBUG builds, specifically for the purpose of verification via ASSERT.  By contrast DCHECK is most commonly used in Chrome to sanity-check the state of real non-DEBUG parameters and fields.

Note that we already have a variety of notionally-equivalent macros defined in base/logging.h, for example we have both LOG_ASSERT and CHECK, and LOG_ASSERT itself is equivalent to LOG_IF(severity, condition); lots of rarely-used syntactic sugar in there.

[TAMURA, Kent]

We finally removed all ASSERT macros.  Thank you for your contribution!

The remaining deprecated stuff in Assertions.h is only WTFLogAlways(). The following files use it:

  core/html/parser/HTMLParserScriptRunner.cpp

  core/layout/LayoutGeometryMap.cpp

  core/layout/LayoutObject.cpp

  platform/graphics/LoggingCanvas.cpp

  platform/graphics/paint/PaintController.cpp

See crbug.com/638849

On Tue, Nov 22, 2016 at 12:37 PM, TAMURA, Kent <tk...@chromium.org> wrote:

Thank you for helping the replacement task.

The number of instances as of March 31 -> November 21:

ASSERT() : 2,191 -> 1,195

RELEASE_ASSERT() : 122 -> 88

ENABLE(ASSERT) : 301 -> 185

ASSERT_NOT_REACHED() : 584 -> 320

ASSERT_UNUSED() : 115 -> 0

ASSERT_WITH_SECURITY_IMPLICATION() : 154 -> 0

As we noticed in the previous thread, RELEASE_ASSERT() -> CHECK() replacement would have performance regression on Windows.  However I guess the replacement in directories except wtf/ won't have visible performance impact, and we should try the replacement.

--

TAMURA, Kent
Software Engineer, Google

[Victor Costan]

Was there any conclusion to the idea / effort of coming up with an alternative DCHECK that doesn't require #if DCHECK_IS_ON()?

I've written some of those #ifdefs in IndexedDB code, and I'd be quite happy to make them go away.

Thank you very much,

    Victor

--
You received this message because you are subscribed to the Google Groups "blink-dev" group.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAGH7WqHjFiCNvA3%3DcXn--G4R40X7yfPO1QcLzA%3DJfcuiOrKxTQ%40mail.gmail.com.

[Wez]

Can you clarify what it is that you're asking for, Victor?  Are you talking about the fact that DCHECK references the |condition| even in Release builds?

Wez

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAP_mGKqa_t7VMuc0VfOhVzDMeq%2BhaRLnoJQDNYKtkdhmBnaAsw%40mail.gmail.com.

[Daniel Cheng]

One alternative is DLOG_IF(FATAL, ...).

Daniel

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAP_mGKqa_t7VMuc0VfOhVzDMeq%2BhaRLnoJQDNYKtkdhmBnaAsw%40mail.gmail.com.

[Victor Costan]

On Tue, May 23, 2017 at 5:48 PM, Daniel Cheng <dch...@chromium.org> wrote:

One alternative is DLOG_IF(FATAL, ...).

Thank you very much for your suggestion, Daniel!

Right now, it seems like mixing DLOG_IF with DCHECK would add an extra mental burden to my code's readers (why are you logging here and DCHECKing here?) I'll let this sink for a bit and see if it grows on me.

[Wez]

(re-adding blink-dev@)

Thanks for the clarification, Victor.  So yes it sounds like the concern is that you are ending up with code that looks like:

#if DCHECK_IS_ON();

  // Declare some stuff.

  int foo;

#endif

...

#if DCHECK_IS_ON()

  DCHECK(foo);

#endif

because of the fact that DCHECK() always references the |condition|, even in non-Debug builds.

As Daniel already responded, I think the construct you want is DLOG_IF(FATAL, ), which is equivalent to DCHECK, but with the difference that |condition| is not referenced.

On 24 May 2017 at 11:46, Victor Costan <pwn...@chromium.org> wrote:

On Tue, May 23, 2017 at 4:28 PM, Wez <w...@google.com> wrote:

Can you clarify what it is that you're asking for, Victor?  Are you talking about the fact that DCHECK references the |condition| even in Release builds?

Yes. Thank you very much for helping me clarify my question!

Specifically, I have class fields that are only used in DCHECKs. To avoid wasting memory, these fields are only declared in DCHECK-enabled builds. Concretely, the declarations are guarded by #if DCHECK_IS_ON(). While I can definitely tolerate an #if guard per field to save memory, I'm a bit sad that the guards extend to every DCHECK I use.

I think that the current DCHECK behavior is useful, as the macro can potentially turn the assertion into an optimizer hint. At the same time, I wish there was a more lightweight alternative to #if DCHECK_IS_ON()-guarded DCHECKs. Perhaps XDCHECK? XD

    Victor

[Nico Weber]

On Wed, May 24, 2017 at 2:52 PM, Wez <w...@chromium.org> wrote:

(re-adding blink-dev@)

Thanks for the clarification, Victor.  So yes it sounds like the concern is that you are ending up with code that looks like:

#if DCHECK_IS_ON();

  // Declare some stuff.

  int foo;

#endif

...

#if DCHECK_IS_ON()

  DCHECK(foo);

#endif

Let's please all just do this ^. It's 2 lines more, but it's needed fairly rarely, and it's very explicit and easy to understand.

 

because of the fact that DCHECK() always references the |condition|, even in non-Debug builds.

As Daniel already responded, I think the construct you want is DLOG_IF(FATAL, ), which is equivalent to DCHECK, but with the difference that |condition| is not referenced.

On 24 May 2017 at 11:46, Victor Costan <pwn...@chromium.org> wrote:

On Tue, May 23, 2017 at 4:28 PM, Wez <w...@google.com> wrote:

Can you clarify what it is that you're asking for, Victor?  Are you talking about the fact that DCHECK references the |condition| even in Release builds?

Yes. Thank you very much for helping me clarify my question!

Specifically, I have class fields that are only used in DCHECKs. To avoid wasting memory, these fields are only declared in DCHECK-enabled builds. Concretely, the declarations are guarded by #if DCHECK_IS_ON(). While I can definitely tolerate an #if guard per field to save memory, I'm a bit sad that the guards extend to every DCHECK I use.

I think that the current DCHECK behavior is useful, as the macro can potentially turn the assertion into an optimizer hint. At the same time, I wish there was a more lightweight alternative to #if DCHECK_IS_ON()-guarded DCHECKs. Perhaps XDCHECK? XD

    Victor

 

On 23 May 2017 at 16:20, Victor Costan <pwn...@chromium.org> wrote:

Was there any conclusion to the idea / effort of coming up with an alternative DCHECK that doesn't require #if DCHECK_IS_ON()?

I've written some of those #ifdefs in IndexedDB code, and I'd be quite happy to make them go away.

Thank you very much,

    Victor

--
You received this message because you are subscribed to the Google Groups "blink-dev" group.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CALekkJeEzqkaSEB-sBqoNs1M8SLEPd5XsVsA5fKae1NO1TUBFA%40mail.gmail.com.