Intent to force dcheck_always_on in component builds

[Gabriel Charette]

Hello all, 

for the second time in two weeks there's a DCHECK being hit on browser startup. I think we all agree that these should be considered as bad as a failing test (i.e. faulty CL should be reverted) -- in both cases those were data races caught by Sequence/ThreadCheckers.

The question is then how can we avoid them in the first place? Sure the feature is probably lacking test coverage but that barrier has already slipped by the time an unrelated dev hits a DCHECK on their local build on master (not to mention time wasted to realize it's not their branch that has the issue but their master as well...).

I propose we force "dcheck_always_on: true" on developer builds (component_build is a good proxy?). Oversight is the only reason I can think of not to want DCHECKs in a developer build. The performance side-effects are minor and real-performance profiling is already out of the question IMO in a component build so...

Barring any objections, I'll send a CL to make it so shortly.

Cheers,

Gab

[Brett Wilson]

Aren't developer builds normally debug, which would enable DCHECKS anyway? Who is using release component builds such that this is an issue?

Brett

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAJTZ7L%2BSd2hhoRMLXwihEGnjf6Px0zg0WesxfrOakOY9XtXF3w%40mail.gmail.com.

[Ken Rockot]

I almost always use release component builds for development because iteration speed on debug builds is noticeably slower.

In any case I always have dcheck_always_on=true and think it's a good idea for us to encourage its use by all chromium developers. This seems like an easy way to force it in many cases, but it's kind of a weirdly arbitrary thing to have component imply dcheck; besides, a) not all developers actually use component builds for whatever reason, and b) maybe it would be easier to just update docs to communicate the value better?

Alternative proposal: make dcheck_always_on=true by default when is_official_build=false

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

[Jeremy Roman]

Maybe we should have a bot that builds with dchecks on and starts the browser (I'd have hoped browser tests would cover such cases), but making the {release, no dchecks, component} configuration illegal seems arbitrary to me. I've used a configuration like that as a sanity check for performance before I do real measurement on an official-ish build.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CA%2BapAgFbH2Bcy%2BiPQq26ZdFTh3XF%2ByEdfdzN5u-FgZ9OuGghRA%40mail.gmail.com.

[dan...@chromium.org]

On Mon, May 15, 2017 at 5:34 PM, Ken Rockot <roc...@chromium.org> wrote:

I almost always use release component builds for development because iteration speed on debug builds is noticeably slower.

In any case I always have dcheck_always_on=true and think it's a good idea for us to encourage its use by all chromium developers. This seems like an easy way to force it in many cases, but it's kind of a weirdly arbitrary thing to have component imply dcheck; besides, a) not all developers actually use component builds for whatever reason, and b) maybe it would be easier to just update docs to communicate the value better?

Alternative proposal: make dcheck_always_on=true by default when is_official_build=false

I also regularly work in release + dcheck_always_on + component build until I reach a state where I need better symbols. Making it a default seems much better to me than forcing it to be on as well.

 

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CA%2BapAgFbH2Bcy%2BiPQq26ZdFTh3XF%2ByEdfdzN5u-FgZ9OuGghRA%40mail.gmail.com.

[Charles Harrison]

+1 to making dcheck_always_on default to true if is_official_build is false, but -1 to making {release, no dchecks, component} an illegal configuration for the reason Jeremy noted.

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev+unsubscribe@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CACuR13fJkycQeYSMRg3rgnYmnVj9UYsmeYxK4xon0Y6SR4YrLg%40mail.gmail.com.

[Peter Kasting]

On Mon, May 15, 2017 at 2:34 PM, Ken Rockot <roc...@chromium.org> wrote:

Alternative proposal: make dcheck_always_on=true by default when is_official_build=false

I like this significantly more than the original proposal.

PK 

[Jochen Eisinger]

fwiw, it's virtually impossible to use Chromium with dchecks on for regular usage which in turns makes it impossible to try to reproduce V8 issues with a release & dcheck or debug build. We're now trying to introduce a build configuration that only turns V8 dchecks on to work around this issue, but it would be really nice if dchecks would actually hold in the majority of cases..

--

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

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

[Peter Kasting]

On Mon, May 15, 2017 at 11:46 PM, Jochen Eisinger <joc...@chromium.org> wrote:

fwiw, it's virtually impossible to use Chromium with dchecks on for regular usage

This is surprising and concerning.  DCHECKs should never fire.  Failures in them should be treated at least as high as P1 crash bugs if not higher.  In my own usage, they _don't_ fire very often, and I try to file bugs when they do.  For others, please file bugs every time a DCHECK fires, and please treat your own DCHECK failures as critical bugs that are extremely urgent.

PK

[Jakob Kummerow]

Yes, Peter, I wish everyone shared your view on DCHECKs.

With today's ToT, I hit only one DCHECK in the first five minutes of trying (plus the "FATAL:canonical_cookie.cc(238)] Check failed: !name.empty()" on browser startup, which I guess is what Gab referred to?), that's better than average. I've filed issue 722761, let's see how it goes. (Historically, my experience is that about half of such bugs get traction.)

--

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAAHOzFD_GP1QvuxWRNLsimQ%3DjXfxbMG4%3Dq8oO5qvx3ScfRutMw%40mail.gmail.com.

[Gabriel Charette]

Enabling dchecks by default when is_official_build=false SGTM. Also, I didn't mean to make the {release, no dchecks, component} build illegal, merely for it to require explicitly specifying dcheck_always_on=false (I meant "default" not "force", my bad). Today I suspect many devs unintentionally run without dchecks and as such slow other people down by introducing DCHECK failures on ToT.

Re. should browser_tests run with dchecks: they already do but there are many DCHECK sites which are under tested. Sure more tests is better but that won't catch CLs that forget the very DCHECK'ing use case. Whereas perhaps they'd hit it on a local full browser run of their feature and hopefully even think of adding a regression test after fixing it locally :).

My experience matches Peter's in that it's fairly rare and should be treated as a P1 bug when it occurs (and culprit CL should be reverted right away if it can easily be identified). Hopefully it happens even less once every dev runs with dchecks enabled by default.

Making it so, cheers!

Gab

[dan...@chromium.org]

On Tue, May 16, 2017 at 5:37 AM, Jakob Kummerow <jkum...@chromium.org> wrote:

Yes, Peter, I wish everyone shared your view on DCHECKs.

With today's ToT, I hit only one DCHECK in the first five minutes of trying (plus the "FATAL:canonical_cookie.cc(238)] Check failed: !name.empty()" on browser startup, which I guess is what Gab referred to?), that's better than average. I've filed issue 722761, let's see how it goes. (Historically, my experience is that about half of such bugs get traction.)

If a DCHECK is firing and doesn't get addressed promptly, I will usually disable the DCHECK because we already know it's failing and now it's just getting in the way of developers. I think of this as similar to a test failing on the waterfall.

On Tue, May 16, 2017 at 9:00 AM, Peter Kasting <pkas...@chromium.org> wrote:

On Mon, May 15, 2017 at 11:46 PM, Jochen Eisinger <joc...@chromium.org> wrote:

fwiw, it's virtually impossible to use Chromium with dchecks on for regular usage

This is surprising and concerning.  DCHECKs should never fire.  Failures in them should be treated at least as high as P1 crash bugs if not higher.  In my own usage, they _don't_ fire very often, and I try to file bugs when they do.  For others, please file bugs every time a DCHECK fires, and please treat your own DCHECK failures as critical bugs that are extremely urgent.

PK

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAAHOzFD_GP1QvuxWRNLsimQ%3DjXfxbMG4%3Dq8oO5qvx3ScfRutMw%40mail.gmail.com.

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

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

[Colin Blundell]

On Tue, May 16, 2017 at 4:24 PM <dan...@chromium.org> wrote:

On Tue, May 16, 2017 at 5:37 AM, Jakob Kummerow <jkum...@chromium.org> wrote:

Yes, Peter, I wish everyone shared your view on DCHECKs.

With today's ToT, I hit only one DCHECK in the first five minutes of trying (plus the "FATAL:canonical_cookie.cc(238)] Check failed: !name.empty()" on browser startup, which I guess is what Gab referred to?), that's better than average. I've filed issue 722761, let's see how it goes. (Historically, my experience is that about half of such bugs get traction.)

If a DCHECK is firing and doesn't get addressed promptly, I will usually disable the DCHECK because we already know it's failing and now it's just getting in the way of developers. I think of this as similar to a test failing on the waterfall.

In response to this thread, I was thinking about introducing this explicitly as a policy. The problem however is that the resulting behavior isn't guaranteed to be any better, given our policy of not gracefully handling codepaths that are subsequent to a DCHECK. Leaving the DCHECK may actually be better for developers, depending on the case in question.

 

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

[Charles Harrison]

I think going in and removing another team's DCHECK is only a good policy if it is surely benign and the code is fine without it. Otherwise, the team should investigate the failure and treat it like a crash.

As a side note, I'll plug libfuzzer as the #1 tool I've used to catch incorrect DCHECKs that never failed with existing tests.

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

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAMGE5NE7DF3_hGMNsFArNMWJ1d277ZSi_ki3Ks-hSojUCj%3DLiA%40mail.gmail.com.

[Gabriel Charette]

Agreed, it should be treated like a crash, and the more people run DCHECKs the more likely we are to identify the culprit quickly and be able to revert it. If the culprit can't be identified, it's okay for affected developers to disable the DCHECK locally (I think that's what Dana meant?) but not to remove it from trunk.

PS: I'm moving along with https://codereview.chromium.org/2886803002/, looks like iOS build didn't even compile with DCHECKs enabled... Was a minor failure but still, it's from code landed years ago... does no one on iOS team use DCHECKs =O?! Hopefully tests pass or that config is seriously ill :(

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

[James Cook]

+1 to maintaining Release + DCHECK + component. I use this all the time and it works well for me (Chrome OS UI development).

+1 to making DCHECK on the default for most developers.

Anecdata: The DCHECKs I hit most often are in some sort of storage manager, maybe indexdb. Wiping my user data directory usually works around it.

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

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAMGE5NE7DF3_hGMNsFArNMWJ1d277ZSi_ki3Ks-hSojUCj%3DLiA%40mail.gmail.com.

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

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

[dan...@chromium.org]

On Tue, May 16, 2017 at 11:17 AM, Gabriel Charette <g...@chromium.org> wrote:

Agreed, it should be treated like a crash, and the more people run DCHECKs the more likely we are to identify the culprit quickly and be able to revert it. If the culprit can't be identified, it's okay for affected developers to disable the DCHECK locally (I think that's what Dana meant?) but not to remove it from trunk.

I will disable it locally immediately, and file a bug. But I meant that if the bug is not being addressed, I would look at the possibility of removing the DCHECK from ToT rather than leave it failing and unaddressed. I don't mean that it would be done secretly from the people who work on that code. But leaving failing DCHECKs around makes developers run without DCHECKs which only hurts our development.

 

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

[Colin Blundell]

On Tue, May 16, 2017 at 5:21 PM <dan...@chromium.org> wrote:

On Tue, May 16, 2017 at 11:17 AM, Gabriel Charette <g...@chromium.org> wrote:

Agreed, it should be treated like a crash, and the more people run DCHECKs the more likely we are to identify the culprit quickly and be able to revert it. If the culprit can't be identified, it's okay for affected developers to disable the DCHECK locally (I think that's what Dana meant?) but not to remove it from trunk.

I will disable it locally immediately, and file a bug. But I meant that if the bug is not being addressed, I would look at the possibility of removing the DCHECK from ToT rather than leave it failing and unaddressed. I don't mean that it would be done secretly from the people who work on that code. But leaving failing DCHECKs around makes developers run without DCHECKs which only hurts our development.

 

That was my understanding of what you had meant, and I think it would make sense except for the fact that removing the DCHECK leaves the code in an unpredictable state in general. I guess one could argue that this was actually already the state for devs running without DCHECKs (and users) anyway, but nonetheless, having a dev build fail with unpredictable behavior due to someone else's bug seems even worse to me than having a dev build crash predictably due to someone else's bug? Obviously this is a moot point in cases where you can look at the code around the DCHECK and reason that removing it wouldn't have this kind of effect.

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

[dan...@chromium.org]

On Tue, May 16, 2017 at 11:24 AM, Colin Blundell <blun...@chromium.org> wrote:

On Tue, May 16, 2017 at 5:21 PM <dan...@chromium.org> wrote:

On Tue, May 16, 2017 at 11:17 AM, Gabriel Charette <g...@chromium.org> wrote:

Agreed, it should be treated like a crash, and the more people run DCHECKs the more likely we are to identify the culprit quickly and be able to revert it. If the culprit can't be identified, it's okay for affected developers to disable the DCHECK locally (I think that's what Dana meant?) but not to remove it from trunk.

I will disable it locally immediately, and file a bug. But I meant that if the bug is not being addressed, I would look at the possibility of removing the DCHECK from ToT rather than leave it failing and unaddressed. I don't mean that it would be done secretly from the people who work on that code. But leaving failing DCHECKs around makes developers run without DCHECKs which only hurts our development.

 

That was my understanding of what you had meant, and I think it would make sense except for the fact that removing the DCHECK leaves the code in an unpredictable state in general. I guess one could argue that this was actually already the state for devs running without DCHECKs (and users) anyway, but nonetheless, having a dev build fail with unpredictable behavior due to someone else's bug seems even worse to me than having a dev build crash predictably due to someone else's bug? Obviously this is a moot point in cases where you can look at the code around the DCHECK and reason that removing it wouldn't have this kind of effect.

Sometimes early out instead of DCHECK crashing is better than continuing, yeah. It requires reading the code and thinking about what would happen a bit.

 

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

[Colin Blundell]

On Tue, May 16, 2017 at 5:17 PM Gabriel Charette <g...@chromium.org> wrote:

Agreed, it should be treated like a crash, and the more people run DCHECKs the more likely we are to identify the culprit quickly and be able to revert it. If the culprit can't be identified, it's okay for affected developers to disable the DCHECK locally (I think that's what Dana meant?) but not to remove it from trunk.

PS: I'm moving along with https://codereview.chromium.org/2886803002/, looks like iOS build didn't even compile with DCHECKs enabled... Was a minor failure but still, it's from code landed years ago... does no one on iOS team use DCHECKs =O?! Hopefully tests pass or that config is seriously ill :(

Slightly off-topic, but it looks like the issue on iOS there is specifically with release + dchecks enabled mode. i.e., iOS devs do use dchecks, but in debug mode. I guess the iOS trybots aren't configured to run in release + dcheck like other platforms.

[Sylvain Defresne]

As there is no gain from running is_release = true + dcheck_always_on = true vs is_debug = true, iOS developer have been running with is_debug = true for a long time. So rest assured, DCHECK have been enabled for all developer builds, the only configuration that was broken is one that nobody ever tried (the code should have used DCHECK_IS_ON() in the #if to work properly).

-- Sylvain

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

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAMGE5NE7DF3_hGMNsFArNMWJ1d277ZSi_ki3Ks-hSojUCj%3DLiA%40mail.gmail.com.

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

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

[Gabriel Charette]

Ah ok right, my bad :)

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

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAMGE5NE7DF3_hGMNsFArNMWJ1d277ZSi_ki3Ks-hSojUCj%3DLiA%40mail.gmail.com.

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

[Jochen Eisinger]

On other platforms we changed dchecks to work with release builds in order to decrease bot cycle time without losing coverage. I'd argue that this should also apply here, and we should just fix the #if

[Dirk Pranke]

Do we know how the change landed in the first place? Does it only fire on a release+dcheck+component build? If so, it's understandable that that landed, because we don't have any bots that test for it. On the other hand, if it fires on release+dcheck+static, or debug+component, that should've been caught by both trybots and on the waterfall bots, and I'd like to know if (or why) it wasn't.

Separately, we should review which configurations are tested where, and why:

On the waterfalls, we test release+static+no-dcheck and debug+dcheck+component . In addition, the perf builders use official (which is also static, no-dcheck). On the tryservers, we generally only run tests for release+static+dcheck. 

The rationale for this is that we want to run tests that will catch the most issues (debug + dcheck) and also, as much as possible, test what we ship. We don't run the tests in official everywhere because official builds take too long to compile. We also don't test release+component because adding a third configuration is too expensive to be generally worth the added coverage.

On the tryservers, running the tests in a debug configuration is believed to be too slow, and so we run in release + dcheck as a tradeoff. This occasionally leads to bugs where a problem happens only on the waterfall, or only on the tryservers.

We can either add a third configuration (either release+static+dcheck, or release+component+dcheck) to the waterfalls, or we can change the release+static+no-dcheck configuration. The downside to the former is that it's more expensive. The downsides to the latter are that (a) it'll make the tested configuration less like the released configuration, and (b) it'll probably make the waterfall builds slightly slower. An obvious upside to either changing the existing config or adding a release+static+dcheck config is that it'll eliminate the tryserver discrepancy.

-- Dirk

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

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAMGE5NE7DF3_hGMNsFArNMWJ1d277ZSi_ki3Ks-hSojUCj%3DLiA%40mail.gmail.com.

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

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

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

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

[Randy Smith]

On Tue, May 16, 2017 at 2:37 AM, Jakob Kummerow <jkum...@chromium.org> wrote:

Yes, Peter, I wish everyone shared your view on DCHECKs.

With today's ToT, I hit only one DCHECK in the first five minutes of trying (plus the "FATAL:canonical_cookie.cc(238)] Check failed: !name.empty()" on browser startup, which I guess is what Gab referred to?), that's better than average. I've filed issue 722761, let's see how it goes. (Historically, my experience is that about half of such bugs get traction.)

Hey, folks--the CanonicalCookie DCHECK sounds like it could easily be a result of some recent work I've been doing.  Even if not, I'm happy to track it down since I'm working in that code space anyway.   However, I don't get the crash when I build at r472127 on Mac and run a debug chromium.  Could someone who's seeing this file a bug with repro instructions and assign it to me?

-- Randy

On Tue, May 16, 2017 at 9:00 AM, Peter Kasting <pkas...@chromium.org> wrote:

On Mon, May 15, 2017 at 11:46 PM, Jochen Eisinger <joc...@chromium.org> wrote:

fwiw, it's virtually impossible to use Chromium with dchecks on for regular usage

This is surprising and concerning.  DCHECKs should never fire.  Failures in them should be treated at least as high as P1 crash bugs if not higher.  In my own usage, they _don't_ fire very often, and I try to file bugs when they do.  For others, please file bugs every time a DCHECK fires, and please treat your own DCHECK failures as critical bugs that are extremely urgent.

PK

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAAHOzFD_GP1QvuxWRNLsimQ%3DjXfxbMG4%3Dq8oO5qvx3ScfRutMw%40mail.gmail.com.

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

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

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

[Bruce]

If we change when DCHECKs are enabled I would request that we enable it for is_debug || is_component_build, rather than for !is_official_build. My rationale is that if a developer is running a release-non-component build then they are interested in run-time performance, and DCHECKs may interfere with that. A developer who is running a debug build or a component build has indicated that run-time performance is not their main concern, so DCHECKs are appropriate.

[Gabriel Charette]

+Brett Wilson : that was my take as well initially but I was then carried away by the sentiment on this thread that is_component_build felt arbitrary and landed it as !is_official_build (https://codereview.chromium.org/2886803002/).

I'd forgotten that part of my initial reasoning for using is_component_build as a default was also that the developer states that performance is not the goal.

Shall we change it? Looks like we'll revert for today anyways so I can make that try #2 once we've fixed the core issues identified.

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

[Sunny Sachanandani]

AFAIK is_official_build requires internal sources. A lot of us don't checkout internal sources so making that the default for performance testing seems overkill.

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

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

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/6a46ac45-c236-4c7e-903a-020885b6d0e8%40chromium.org.

[Bruce Dawson]

is_official_build is purely an optimization setting. is_chrome_branded is what uses internal sources.

I would say yes, we should trigger it on non-component/non-debug.

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

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

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

[Gabriel Charette]

Also, my bad, meant to + Bruce not Brett in previous reply (same B icon in Inbox confused me..!)

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

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

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

[Peter Kasting]

On Tue, May 16, 2017 at 1:18 PM, Bruce <bruce...@chromium.org> wrote:

If we change when DCHECKs are enabled I would request that we enable it for is_debug || is_component_build, rather than for !is_official_build. My rationale is that if a developer is running a release-non-component build then they are interested in run-time performance, and DCHECKs may interfere with that. A developer who is running a debug build or a component build has indicated that run-time performance is not their main concern, so DCHECKs are appropriate.

I think this reasoning infers too much.  Users who care a ton about perf probably don't want dcheck on; users who care somewhat but not enormously ("don't make the browser unusable, but I'm not benchmarking") probably don't mind..  Users who are is_debug likely don't care much about perf; users of release builds may care some, or may care a lot.  With users of component builds, I don't know that there's much clarity of intent.  And all of these are not hard-and-fast.

The nice thing about connecting to "official build" was that it was the closest thing we had to "I'm developing", which is the closest proxy for "are dchecks appropriate".  The connection here instead tries to infer performance concerns from other switches, then infer what the user would think about dchecks given their inference on performance.

If we really want this performance-based, it might make sense to hide this tangle of settings under some small enumerated set of modes:

• full debugging (implies debug, dchecks on, component)
  
• usable development browser (implies release, dchecks on, component)
  
• performance testing (implies release, dchecks off, monolithic)
  
• the real released, branded thing (implies release, dchecks off, monolithic, branded)
  

Then you can tweak individual settings after that if you want.  But by default you'd just pick the mode by name, not the individual settings under this, and none of these settings would imply each other.

I don't know if this is feasible to implement.

PK

[Dirk Pranke]

There are a few different dimensions: component, dcheck, debug, symbol_level, "full optimization" aka official, LTO, PGO, branding (at least). Not every combination makes sense, but there's more than four that do. I don't know if there's more than, say, eight, but at some point a simple enumeration becomes impractical. I also don't know if determining that number to see for sure would be worthwhile, which is I think I different way of saying what you said about it being feasible to implement :).

-- Dirk

[Gabriel Charette]

True (what Peter said) and for developers who really want to do performance testing on a local but non-official build, they can explicitly dcheck_always_on=false. I'm in favor of a better default for all with an extra step for those who know what they're doing.

[Gabriel Charette]

Looks like this (among other things) triggered a sizes error per adding new static initializers in the config the bots check.

Feels like it would be preferable for the sizes step to not be bothered by DCHECKs, however that step is done among many other tests on generic bots (who generally will want DCHECKs).

+Dirk Pranke mentioned that CQ/try bots use dcheck_always_on but the main waterfall intentionally doesn't, should we explicitly dcheck_always_on = false on the waterfall bots then?

[Gabriel Charette]

On Tue, May 16, 2017 at 9:23 PM Gabriel Charette <g...@chromium.org> wrote:

Looks like this (among other things) triggered a sizes error per adding new static initializers in the config the bots check.

Ref. https://crbug.com/723049, static initializers in debug.cpp. It indeed adds a static initializer.

And thinking some more about it, that should be fixed, no reason to allow tests to have static initializers more than other code.

[Nico Weber]

On Tue, May 16, 2017 at 9:36 PM, Gabriel Charette <g...@chromium.org> wrote:

On Tue, May 16, 2017 at 9:23 PM Gabriel Charette <g...@chromium.org> wrote:

Looks like this (among other things) triggered a sizes error per adding new static initializers in the config the bots check.

Ref. https://crbug.com/723049, static initializers in debug.cpp. It indeed adds a static initializer.

And thinking some more about it, that should be fixed, no reason to allow tests to have static initializers more than other code.

We definitely allow static initializers in tests and not in production code. gtest adds one static initializer per TEST_F, for example. (But maybe you mean "production code with dchecks on", not "tests"?)

 

Feels like it would be preferable for the sizes step to not be bothered by DCHECKs, however that step is done among many other tests on generic bots (who generally will want DCHECKs).

+Dirk Pranke mentioned that CQ/try bots use dcheck_always_on but the main waterfall intentionally doesn't, should we explicitly dcheck_always_on = false on the waterfall bots then?

On Tue, May 16, 2017 at 9:10 PM Gabriel Charette <g...@chromium.org> wrote:

True (what Peter said) and for developers who really want to do performance testing on a local but non-official build, they can explicitly dcheck_always_on=false. I'm in favor of a better default for all with an extra step for those who know what they're doing.

On Tue, May 16, 2017 at 8:55 PM Dirk Pranke <dpr...@chromium.org> wrote:

On Tue, May 16, 2017 at 5:11 PM, Peter Kasting <pkas...@chromium.org> wrote:

On Tue, May 16, 2017 at 1:18 PM, Bruce <bruce...@chromium.org> wrote:

If we change when DCHECKs are enabled I would request that we enable it for is_debug || is_component_build, rather than for !is_official_build. My rationale is that if a developer is running a release-non-component build then they are interested in run-time performance, and DCHECKs may interfere with that. A developer who is running a debug build or a component build has indicated that run-time performance is not their main concern, so DCHECKs are appropriate.

I think this reasoning infers too much.  Users who care a ton about perf probably don't want dcheck on; users who care somewhat but not enormously ("don't make the browser unusable, but I'm not benchmarking") probably don't mind..  Users who are is_debug likely don't care much about perf; users of release builds may care some, or may care a lot.  With users of component builds, I don't know that there's much clarity of intent.  And all of these are not hard-and-fast.

The nice thing about connecting to "official build" was that it was the closest thing we had to "I'm developing", which is the closest proxy for "are dchecks appropriate".  The connection here instead tries to infer performance concerns from other switches, then infer what the user would think about dchecks given their inference on performance.

If we really want this performance-based, it might make sense to hide this tangle of settings under some small enumerated set of modes:

• full debugging (implies debug, dchecks on, component)
  
• usable development browser (implies release, dchecks on, component)
  
• performance testing (implies release, dchecks off, monolithic)
  
• the real released, branded thing (implies release, dchecks off, monolithic, branded)
  

Then you can tweak individual settings after that if you want.  But by default you'd just pick the mode by name, not the individual settings under this, and none of these settings would imply each other.

I don't know if this is feasible to implement.

There are a few different dimensions: component, dcheck, debug, symbol_level, "full optimization" aka official, LTO, PGO, branding (at least). Not every combination makes sense, but there's more than four that do. I don't know if there's more than, say, eight, but at some point a simple enumeration becomes impractical. I also don't know if determining that number to see for sure would be worthwhile, which is I think I different way of saying what you said about it being feasible to implement :).

-- Dirk

--

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

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

[Dirk Pranke]

On Tue, May 16, 2017 at 6:23 PM, Gabriel Charette <g...@chromium.org> wrote:

Looks like this (among other things) triggered a sizes error per adding new static initializers in the config the bots check.

Feels like it would be preferable for the sizes step to not be bothered by DCHECKs, however that step is done among many other tests on generic bots (who generally will want DCHECKs).

+Dirk Pranke mentioned that CQ/try bots use dcheck_always_on but the main waterfall intentionally doesn't, should we explicitly dcheck_always_on = false on the waterfall bots then?

I took the intent of this thread as being "the release builds on both the tryservers and waterfall builders *should* have DCHECKs enabled". If we weren't going to change that, I'm not sure that changing anything at all is worth it?

-- Dirk

[Gabriel Charette]

On Tue, May 16, 2017 at 9:46 PM Nico Weber <tha...@chromium.org> wrote:

On Tue, May 16, 2017 at 9:36 PM, Gabriel Charette <g...@chromium.org> wrote:

On Tue, May 16, 2017 at 9:23 PM Gabriel Charette <g...@chromium.org> wrote:

Looks like this (among other things) triggered a sizes error per adding new static initializers in the config the bots check.

Ref. https://crbug.com/723049, static initializers in debug.cpp. It indeed adds a static initializer.

And thinking some more about it, that should be fixed, no reason to allow tests to have static initializers more than other code.

We definitely allow static initializers in tests and not in production code. gtest adds one static initializer per TEST_F, for example. (But maybe you mean "production code with dchecks on", not "tests"?)

Yes "production code with dchecks on".

[Gabriel Charette]

On Tue, May 16, 2017 at 9:50 PM Dirk Pranke <dpr...@chromium.org> wrote:

On Tue, May 16, 2017 at 6:23 PM, Gabriel Charette <g...@chromium.org> wrote:

Looks like this (among other things) triggered a sizes error per adding new static initializers in the config the bots check.

Feels like it would be preferable for the sizes step to not be bothered by DCHECKs, however that step is done among many other tests on generic bots (who generally will want DCHECKs).

+Dirk Pranke mentioned that CQ/try bots use dcheck_always_on but the main waterfall intentionally doesn't, should we explicitly dcheck_always_on = false on the waterfall bots then?

I took the intent of this thread as being "the release builds on both the tryservers and waterfall builders *should* have DCHECKs enabled". If we weren't going to change that, I'm not sure that changing anything at all is worth it?

Enabling on waterfall is fine by me. The real intent of doing this is to make sure every dev runs with DCHECKs by default. Today, too many devs unintentionally don't and this is causing other devs to hit their DCHECKs and wastes much engineering time.

[Peter Kasting]

On Tue, May 16, 2017 at 5:55 PM, Dirk Pranke <dpr...@chromium.org> wrote:

On Tue, May 16, 2017 at 5:11 PM, Peter Kasting <pkas...@chromium.org> wrote:

If we really want this performance-based, it might make sense to hide this tangle of settings under some small enumerated set of modes:

• full debugging (implies debug, dchecks on, component)
  
• usable development browser (implies release, dchecks on, component)
  
• performance testing (implies release, dchecks off, monolithic)
  
• the real released, branded thing (implies release, dchecks off, monolithic, branded)
  

Then you can tweak individual settings after that if you want.  But by default you'd just pick the mode by name, not the individual settings under this, and none of these settings would imply each other.

I don't know if this is feasible to implement.

There are a few different dimensions: component, dcheck, debug, symbol_level, "full optimization" aka official, LTO, PGO, branding (at least). Not every combination makes sense, but there's more than four that do.

Yes.  My intent was to explicitly not expose an "enumeration value" for every combo that made rational sense, but only for the most obvious and common needs, and basically say "if you want something else, start with a nearby mode and then tweak the detailed settings".  If we wanted an enum that captured every combination, I think it would be both harder to implement and less readable.

PK

[Colin Blundell]

Hi Dirk,

Assuming that you mean the compilation issue on iOS by "the change" below:

(1) component vs. static is not meaningful on iOS, as the component build is not supported.

(2) The compilation issue fires on release + dchecks. I think that the iOS release trybots simply don't have dchecks enabled (e.g., this configuration). As Jochen mentions, this just seems like a bug AFAICT.

Best,

Colin

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

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAMGE5NE7DF3_hGMNsFArNMWJ1d277ZSi_ki3Ks-hSojUCj%3DLiA%40mail.gmail.com.

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

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

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

[Sylvain Defresne]

On Wed, May 17, 2017 at 10:55 AM, Colin Blundell <blun...@chromium.org> wrote:

Hi Dirk,

Assuming that you mean the compilation issue on iOS by "the change" below:

(1) component vs. static is not meaningful on iOS, as the component build is not supported.

(2) The compilation issue fires on release + dchecks. I think that the iOS release trybots simply don't have dchecks enabled (e.g., this configuration). As Jochen mentions, this just seems like a bug AFAICT.

iOS "Release" bots do not enable DCHECK. iOS "Debug" bots have DCHECK enabled.

 

Best,

Colin

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

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAMGE5NE7DF3_hGMNsFArNMWJ1d277ZSi_ki3Ks-hSojUCj%3DLiA%40mail.gmail.com.

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

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

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

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

-- Sylvain

[John Abd-El-Malek]

(just saw this thread)

I'd like to understand exactly how this went on the bots. We already start chrome.exe through the telemetry tests on try runs, and the trybot is running tests with release+dcheck so why isn't this caught?

I don't think we should change default release (non-official) to have DCHECKs enabled. That creates yet another difference between official and what's running tests on the majority of our waterfall bots

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAEhBV%3DszvueKYFep-F%2B9P%2BVuCyzGWae%2BX17wU0pS8FjETuLLmw%40mail.gmail.com.

[Jochen Eisinger]

Unless a dcheck is hit consistently, "just crashing" doesn't turn a bot red these days. It's not uncommon to see several DCHECKs fly by on a layout tests run, however, on retry, the tests pass, so nothing is reported :/

 

Best,

Colin

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

-- Sylvain

[John Abd-El-Malek]

In this specific case, adding dchecks to release bots wouldn't change anything. i.e. just as they're ignored on debug, they'd be ignored on release.

 

Best,

Colin

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

-- Sylvain

[Jochen Eisinger]

That's true - I think the hope is to get the DCHECKs trigger more often for developers, so they'll more eagerly file bugs / fix them.

 

Best,

Colin

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

-- Sylvain

[John Abd-El-Malek]

In that case, devs who're using release can easily enable that already with the args.gn change. Many already do. Periodic reminders can help. IMO the root problem here is that DCHECKs are firing for untested code. We should have tests for whatever scenario is bringing up these DCHECKS periodically, and then it wouldn't land on the CQ.

Every change we make to release to make it different than official has risks though, so the justification to do so should be quite large. If there are other ways to fix the problem identified by this thread, I'd much prefer we focus on that instead of changing release config.

 

Best,

Colin

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

-- Sylvain

[Gabriel Charette]

I agree that the best thing is for tests to cover the use cases, not to rely on DCHECKs. But the reality is, test coverage is sometimes lacking and devs who *do* run with DCHECKs hit issues introduced by others who don't. The goal of this change is so that devs who just use the default config do run with DCHECKs as well, catching threading/etc. errors checked by many DCHECKs in //base et al. Otherwise incorrect changes land despite the developer having "tested it locally" and the burden falls back on developers who do use DCHECKs (and then many turn off DCHECKs for that very reason... that's a dangerous cycle to be in IMO).

 

Best,

Colin

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

-- Sylvain

[Sylvain Defresne]

The default is to build in debug AFAICT (as "is_debug" defaults to "!is_official_build" and "is_official_build" defaults to "false"). Maybe we should change the documentation that recommends setting "is_debug = false" in args.gn to also recommend setting "dcheck_always_on" to true. Should be pretty easy as there are only ten or so files to touch.

-- Sylvain

 

Best,

Colin

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

-- Sylvain

[John Abd-El-Malek]

On Wed, May 17, 2017 at 8:13 AM, Gabriel Charette <g...@chromium.org> wrote:

I agree that the best thing is for tests to cover the use cases, not to rely on DCHECKs. But the reality is, test coverage is sometimes lacking and devs who *do* run with DCHECKs hit issues introduced by others who don't. The goal of this change is so that devs who just use the default config do run with DCHECKs as well, catching threading/etc. errors checked by many DCHECKs in //base et al. Otherwise incorrect changes land despite the developer having "tested it locally" and the burden falls back on developers who do use DCHECKs (and then many turn off DCHECKs for that very reason... that's a dangerous cycle to be in IMO).

The root cause is missing tests. That should be our primary way of dealing with this.

However, when tests are missing and devs hit this locally, then the next solution is that we do revert changes that caused the DCHECKs. Do you have evidence that devs are moving en masse from debug to release because of DCHECKs that trigger?

The third solution to deal with this is to educate developers that they should use release-with-assert locally. I have that turned on for all my release builds, because I would find it hard to develop without it (i.e. things pass locally, but then fail on the try runs).

We have been running in this config for many years, and it has not been a problem in practice. 

 

Best,

Colin

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

-- Sylvain

[dan...@chromium.org]

On Wed, May 17, 2017 at 11:28 AM, John Abd-El-Malek <j...@chromium.org> wrote:

On Wed, May 17, 2017 at 8:13 AM, Gabriel Charette <g...@chromium.org> wrote:

I agree that the best thing is for tests to cover the use cases, not to rely on DCHECKs. But the reality is, test coverage is sometimes lacking and devs who *do* run with DCHECKs hit issues introduced by others who don't. The goal of this change is so that devs who just use the default config do run with DCHECKs as well, catching threading/etc. errors checked by many DCHECKs in //base et al. Otherwise incorrect changes land despite the developer having "tested it locally" and the burden falls back on developers who do use DCHECKs (and then many turn off DCHECKs for that very reason... that's a dangerous cycle to be in IMO).

The root cause is missing tests. That should be our primary way of dealing with this.

However, when tests are missing and devs hit this locally, then the next solution is that we do revert changes that caused the DCHECKs. Do you have evidence that devs are moving en masse from debug to release because of DCHECKs that trigger?

The third solution to deal with this is to educate developers that they should use release-with-assert locally. I have that turned on for all my release builds, because I would find it hard to develop without it (i.e. things pass locally, but then fail on the try runs).

We have been running in this config for many years, and it has not been a problem in practice.

I seem to recall multiple discussions over the last few years on this list about the high rate of occurance of DCHECKs firing for developers and for bots, such that for instance fuzzers have to disable DCHECKs because they trip them constantly. I think there is a problem in practice.

Missing tests are a problem, but a large number of tests are motivated by hearing about a bug and fixing it, rather than catching all possible errors up front. Putting developers by default into a mode that is most likely to catch their own mistakes seems like a positive thing.

Reverting changes that cause DCHECKs requires developers to see the DCHECKs happen. Education about turning them on in your GN config helps (or in all 6-8 GN configs for all different configs/platforms/etc), but defaults are much stronger.

[Charles Harrison]

On Wed, May 17, 2017 at 11:37 AM, <dan...@chromium.org> wrote:

On Wed, May 17, 2017 at 11:28 AM, John Abd-El-Malek <j...@chromium.org> wrote:

On Wed, May 17, 2017 at 8:13 AM, Gabriel Charette <g...@chromium.org> wrote:

I agree that the best thing is for tests to cover the use cases, not to rely on DCHECKs. But the reality is, test coverage is sometimes lacking and devs who *do* run with DCHECKs hit issues introduced by others who don't. The goal of this change is so that devs who just use the default config do run with DCHECKs as well, catching threading/etc. errors checked by many DCHECKs in //base et al. Otherwise incorrect changes land despite the developer having "tested it locally" and the burden falls back on developers who do use DCHECKs (and then many turn off DCHECKs for that very reason... that's a dangerous cycle to be in IMO).

The root cause is missing tests. That should be our primary way of dealing with this.

However, when tests are missing and devs hit this locally, then the next solution is that we do revert changes that caused the DCHECKs. Do you have evidence that devs are moving en masse from debug to release because of DCHECKs that trigger?

The third solution to deal with this is to educate developers that they should use release-with-assert locally. I have that turned on for all my release builds, because I would find it hard to develop without it (i.e. things pass locally, but then fail on the try runs).

We have been running in this config for many years, and it has not been a problem in practice.

I seem to recall multiple discussions over the last few years on this list about the high rate of occurance of DCHECKs firing for developers and for bots, such that for instance fuzzers have to disable DCHECKs because they trip them constantly. I think there is a problem in practice.

I was under the impression that the non-libfuzzers didn't enable DCHECKs (except SECURITY_DCHECKs) due to compilation time concerns. If those fuzzers can reliably catch bad DCHECKs that is hugely valuable and we should use that!

Missing tests are a problem, but a large number of tests are motivated by hearing about a bug and fixing it, rather than catching all possible errors up front. Putting developers by default into a mode that is most likely to catch their own mistakes seems like a positive thing.

Reverting changes that cause DCHECKs requires developers to see the DCHECKs happen. Education about turning them on in your GN config helps (or in all 6-8 GN configs for all different configs/platforms/etc), but defaults are much stronger.

+1. I I think it would also be useful to include a GN template including dcheck_always_on when you use "gn args" for the first time in a new build directory. I think that's the perfect spot to educate developers about some default/regular gn arguments. 

[John Abd-El-Malek]

On Wed, May 17, 2017 at 8:43 AM, Charles Harrison <cshar...@chromium.org> wrote:

On Wed, May 17, 2017 at 11:37 AM, <dan...@chromium.org> wrote:

On Wed, May 17, 2017 at 11:28 AM, John Abd-El-Malek <j...@chromium.org> wrote:

On Wed, May 17, 2017 at 8:13 AM, Gabriel Charette <g...@chromium.org> wrote:

I agree that the best thing is for tests to cover the use cases, not to rely on DCHECKs. But the reality is, test coverage is sometimes lacking and devs who *do* run with DCHECKs hit issues introduced by others who don't. The goal of this change is so that devs who just use the default config do run with DCHECKs as well, catching threading/etc. errors checked by many DCHECKs in //base et al. Otherwise incorrect changes land despite the developer having "tested it locally" and the burden falls back on developers who do use DCHECKs (and then many turn off DCHECKs for that very reason... that's a dangerous cycle to be in IMO).

The root cause is missing tests. That should be our primary way of dealing with this.

However, when tests are missing and devs hit this locally, then the next solution is that we do revert changes that caused the DCHECKs. Do you have evidence that devs are moving en masse from debug to release because of DCHECKs that trigger?

The third solution to deal with this is to educate developers that they should use release-with-assert locally. I have that turned on for all my release builds, because I would find it hard to develop without it (i.e. things pass locally, but then fail on the try runs).

We have been running in this config for many years, and it has not been a problem in practice.

I seem to recall multiple discussions over the last few years on this list about the high rate of occurance of DCHECKs firing for developers and for bots, such that for instance fuzzers have to disable DCHECKs because they trip them constantly. I think there is a problem in practice.

I was under the impression that the non-libfuzzers didn't enable DCHECKs (except SECURITY_DCHECKs) due to compilation time concerns. If those fuzzers can reliably catch bad DCHECKs that is hugely valuable and we should use that!

Missing tests are a problem, but a large number of tests are motivated by hearing about a bug and fixing it, rather than catching all possible errors up front. Putting developers by default into a mode that is most likely to catch their own mistakes seems like a positive thing.

Reverting changes that cause DCHECKs requires developers to see the DCHECKs happen. Education about turning them on in your GN config helps (or in all 6-8 GN configs for all different configs/platforms/etc), but defaults are much stronger.

+1. I I think it would also be useful to include a GN template including dcheck_always_on when you use "gn args" for the first time in a new build directory. I think that's the perfect spot to educate developers about some default/regular gn arguments. 

+1 to finding ways of making this default for developers' local builds. My issue is just for waterfalls.

[Brett Wilson]

There are different reasons for running release builds. Some people run benchmarks in release builds. When we were changing the implementation of assertions last year, these people (mostly Blink people) argued strenuously that release builds should be as close to official as possible. Another reason to run release builds is for a daily testing build that's not as slow as debug. The last is that people do release builds for actual releases. Since it's named "release", distros and others routinely ship our release builds to customers.

These use-cases need to be differentiated by flags as no default will satisfy all use-cases. The current argument is to flip the default use-case for release builds, in violation of what the name is ("debug check") and standard industry practice, because we believe that developers are missing these checks.

I don't think this is a good argument. I think we should follow the naming and standard practice of what "release" means as much as possible. If it was up to me, the naming of all of these things would be different, but we're stuck with what we have now. But we're beyond changing that.

The argument for doing this seems to be that some developers run on release builds and don't remember to set the debug assertion flag. The result is that we have some dchecks in the code that fire some times. But I don't buy that changing the default here will have any positive effect on the code. I believe a minority of the team runs in release, and some subset of that doesn't have the flag set.

When there's a dhceck on startup, it's not there because nobody noticed it. HUNDREDS of people have seen that, including me, and not fixed it. The bots should already be running with dchecks enabled, and if they aren't we should fix them. Adding a small additional fraction of people running a specific configuration to that set won't mean that assertions don't get added or go away. Changing this means the team overall takes a different attitude when we see such things.

Let's please keep things simple and predictable and treat debug checks like the name implies.

Brett