PSA: try jobs now go to _rel bots

[Nico Weber]

Hi,

I just landed 113979, which lets `git try` and `gcl try` tryjobs to to
the _rel bots by default. This means that your patches are now (by
default) run using release builds – don't worry the _rel bots have
DCHECKs enabled, so you don't lose DCHECK coverage. The main
observable change should be that your tryjobs should now be 2-2.5x as
fast.

You can still pass -b to choose specific bots.

If this sticks, this might free up enough resources that we can add a
shared build bot to the default try set and/or the commit queue –
we'll see.

Nico

[Mike Reed]

Our std practice has been something like the following (for linux only in this example)

gcl try foo --bot=linux,linux_layout,linux_layout_rel -c -r ####

How does your change affect this? Should we specify less, or differently?

Nico

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
   http://groups.google.com/a/chromium.org/group/chromium-dev

[Nico Weber]

Hi Mike,

On Mon, Dec 12, 2011 at 5:14 AM, Mike Reed <re...@google.com> wrote:
> Our std practice has been something like the following (for linux only in
> this example)
>
> gcl try foo --bot=linux,linux_layout,linux_layout_rel -c -r ####
>
> How does your change affect this? Should we specify less, or differently?

you're unaffected by this change. I changed only the default set. If
you pass -b / --bot, nothing changes.

Nico

[Jonathan Dixon]

Any thoughts on setting these _rel try builds to *not* have  NDEBUG defined?

AIUI the main saving of 'release' builds is from having compiler optimization enabled, not from the relatively small additional overhead of runtime diagnostics that hide under ifndef NDEBUG.

I was just close to submitting a change I thought was safe as it was passing code with new CHECKs and DCHECKs on the try bots, but in fact the function I was calling inside the checks is a no-op on NDEBUG builds and so wasn't actually checking anything at all.

FWIW in my own sub-project we find using 'release' builds with #undef NDEBUG to be a sweet spot for developer builds -- lighter weight, but with all the extra runtime checks still in place. 

Nico

[Jói Sigurðsson]

> Any thoughts on setting these _rel try builds to *not* have  NDEBUG defined?

+1, I committed a change yesterday that passed the _rel trybots but
DCHECKed on the tree (thread checker DCHECK). It seems like it would
be cheap enough to not have NDEBUG defined on these bots.

Cheers,
Jói

[Nico Weber]

On Tue, Dec 20, 2011 at 7:59 AM, Jói Sigurðsson <j...@chromium.org> wrote:
>> Any thoughts on setting these _rel try builds to *not* have  NDEBUG defined?

> +1, I committed a change yesterday that passed the _rel trybots but
> DCHECKed on the tree (thread checker DCHECK).  It seems like it would
> be cheap enough to not have NDEBUG defined on these bots.

The _rel builds do have DCHECKs enabled already, so I'd guess your
change had flaky behavior and passed on the trybots by (bad) luck.

joth: I don't have an opinion on not setting NDEBUG. I don't know how
MSVC chooses if it should use its debug c library – if that's not done
through NDEBUG, I guess I'm fine with giving it a try. But changing
NDEBUG-covered code is fairly rare, so requiring explicit jobs on the
non-_rel bots doesn't sound terrible to me. If we wouldn't set NDEBUG
on the _rel bots, we'd need to add bots that people could send jobs to
if they needed to test a change with NDEBUG set. So maybe it's not
worth it.

Nico

[Mike Reed]

Hmm. Skia as configured today triggers its debug-only code based on the absence of NDEBUG. Seems like a hit to performance for sure.

[Jonathan Dixon]

On 20 December 2011 16:20, Nico Weber <tha...@chromium.org> wrote:

On Tue, Dec 20, 2011 at 7:59 AM, Jói Sigurðsson <j...@chromium.org> wrote:
>> Any thoughts on setting these _rel try builds to *not* have  NDEBUG defined?

> +1, I committed a change yesterday that passed the _rel trybots but
> DCHECKed on the tree (thread checker DCHECK).  It seems like it would
> be cheap enough to not have NDEBUG defined on these bots.

The _rel builds do have DCHECKs enabled already, so I'd guess your
change had flaky behavior and passed on the trybots by (bad) luck.
 

joth: I don't have an opinion on not setting NDEBUG. I don't know how
MSVC chooses if it should use its debug c library – if that's not done
through NDEBUG, I guess I'm fine with giving it a try. But changing
NDEBUG-covered code is fairly rare,

I think it's actually more common than you might imagine, e.g. :

http://src.chromium.org/viewvc/chrome/trunk/src/base/threading/non_thread_safe.h?view=markup

uses NDEBUG to determine whether to enable thread-checking. Likewise 

http://src.chromium.org/viewvc/chrome/trunk/src/base/threading/thread_restrictions.h?revision=95618&view=markup

so virtually any code change in content or chrome that calls a new API may fail if it happens to touch an API in non-thread safe or a non-permitted way (e.g. doing IO on the IO thread)

[Jonathan Dixon]

another option is introduce a new half-way house macro, and have things that check conditionally pull in extra checking code use that, without impacting any third party code.

 e.g. DIAGNOSTICS_ENABLED, or DCHECK_ENABLED if we want to tie it tightly to DCHECK state

[Jói Sigurðsson]

Looks like we already have ENABLE_DCHECK set to 1 or 0 in
base/logging.h. I will prepare a change to use that in ThreadChecker
/ NonThreadSafe since that is the problem that bit me (and joth@ as
well I think).

Cheers,
Jói

[Jonathan Dixon]

Cool.

...Only, it looks like logging.h is a good citizen and undefs if again at the end :)

what ever we use, it should probably have a doc that it is 'public' API preferred to ndef NDEBUG for this sort of thing. And preferably be arranged to be a compile break if the associated header is missed.

[Nico Weber]

On Tue, Dec 20, 2011 at 8:48 AM, Jonathan Dixon <jo...@google.com> wrote:
> Cool.
>
> ...Only, it looks like logging.h is a good citizen and undefs if again at
> the end :)

It's set here: http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/build/common.gypi&exact_package=chromium&q=DCHECK_ALWAYS_ON&type=cs&l=1102

[Marc-Antoine Ruel]

I disagree, we are at the point were we need to simplify our configuration matrix, not extend it.

The current state is a mess. IMHO, 90% of the GYP_DEFINE's should die and become proper target configurations. Here's a non official list OTTOMH:
debug o-none
debug o-min tsan
debug o-min nosym (replaces fastbuild)
release o-min dcheck
release o-max (was official)

I'm definitely missing some. "o-" means optimization level. The only diff between debug and release is NDEBUG is defined and the corresponding CRT is used.

I could file a bug but it'll take at least 3 stable releases to complete conversion so we need a highly motivated or highly fed up guy. :)

M-A

[Jonathan Dixon]

On 20 December 2011 16:50, Marc-Antoine Ruel <mar...@chromium.org> wrote:

I disagree, we are at the point were we need to simplify our configuration matrix, not extend it.

Agree with the aim here. I think there's two things pulling against this here:

1/ an argument the "our" try bots should be stricter (follow slower paths) with "our" code than our upstream dependencies (which, we assume, have their own test & release cycle before we even pull them into our code). if we use NDEBUG it impacts our code and skia the same (maybe this is a non-issue)

2/ Several places assume that NDEBUG and DCHECK disabled are the same, this is ever decreasingly so, and this try bot change has deepened that.

i.e. we need a general solultion for:

#if DCHECK_WILL_DO_SOMETHING_USEFUL

  .. complex code using objects that don't exist in release build ...

  DCHECK(result_of_the_above);

#endif

... and have it work in all places that "DCHECKs are enabled" (specifically, try bots).

ISTM to do this while reducing (or at least, not increasing) configurations, we either need to move back closer to DCHECK and ndef NDBUG equivalence (conflicts with 1), or introduce some thing new and consistently use that in our code in place of NDEBUG. 

[Jonathan Dixon]

On 20 December 2011 16:49, Nico Weber <tha...@chromium.org> wrote:

On Tue, Dec 20, 2011 at 8:48 AM, Jonathan Dixon <jo...@google.com> wrote:
> Cool.
>
> ...Only, it looks like logging.h is a good citizen and undefs if again at
> the end :)

It's set here: http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/build/common.gypi&exact_package=chromium&q=DCHECK_ALWAYS_ON&type=cs&l=1102

That sets DCHECK_ALWAYS_ON, not ENABLE_DCHECK...

Not sure DCHECK_ALWAYS_ON is (currently) a good choice, as afaict nothing ensures that is set on a debug build?

[Marc-Antoine Ruel]

So you are only opposed to the configuration "release o-min dcheck", right?

It will continue to exist since it's compiled in and off-by-default in canary releases IIRC.

As such, I'll assume you are in agreement with me. :D

M-A

[Scott Hess]

On Tue, Dec 20, 2011 at 10:35 AM, Jonathan Dixon <jo...@google.com> wrote:
> On 20 December 2011 16:50, Marc-Antoine Ruel <mar...@chromium.org> wrote:
>> I disagree, we are at the point were we need to simplify our configuration
>> matrix, not extend it.
>
> Agree with the aim here. I think there's two things pulling against this
> here:
> 1/ an argument the "our" try bots should be stricter (follow slower paths)
> with "our" code than our upstream dependencies (which, we assume, have their
> own test & release cycle before we even pull them into our code). if we use
> NDEBUG it impacts our code and skia the same (maybe this is a non-issue)
> 2/ Several places assume that NDEBUG and DCHECK disabled are the same, this
> is ever decreasingly so, and this try bot change has deepened that.
>
> i.e. we need a general solultion for:
>
> #if DCHECK_WILL_DO_SOMETHING_USEFUL
>   .. complex code using objects that don't exist in release build ...
>   DCHECK(result_of_the_above);
> #endif
>
> ... and have it work in all places that "DCHECKs are enabled" (specifically,
> try bots).

A general solution is:

static bool MyExpensiveFunction(Params& my_parameters) {
// ...
return result;
}

DCHECK(MyExpensiveFunction(my_parameters));

Not only should it only run when DCHECK can run, it WILL only run when
DCHECK can run.

-scott

[Fred Akalin]

Perhaps it's not easily discoverable, but the macro to use is DCHECK_IS_ON(), like:

if (DCHECK_IS_ON()) {

  // do expensive computation
}

similar macros are LOG_IS_ON(severity) and VLOG_IS_ON(n).

[Alexei Svitkine]

On Tue, Dec 20, 2011 at 1:59 PM, Fred Akalin <aka...@chromium.org> wrote:
> Perhaps it's not easily discoverable, but the macro to use is
> DCHECK_IS_ON(), like:
>
> if (DCHECK_IS_ON()) {
>   // do expensive computation
> }
>
> similar macros are LOG_IS_ON(severity) and VLOG_IS_ON(n).

If we can all agree that this should be The One True Way, can it be
added to our style guide page?

-Alexei

[Jonathan Dixon]

This doesn't work for compiling out members based on DCHECK_IS_ON

e.g. couldn't replace the usage of NDEBUG in

http://src.chromium.org/viewvc/chrome/trunk/src/base/threading/thread_checker.h?revision=76833&view=markup and

http://codesearch.google.com/#OAMlx_jo-ck/src/base/debug/stack_trace.h&exact_package=chromium&q=NDEBUG%20file:stack_trace&type=cs&l=23

On 20 December 2011 18:53, Scott Hess <sh...@chromium.org> wrote:

A general solution is:

static bool MyExpensiveFunction(Params& my_parameters) {
 // ...
 return result;
}

DCHECK(MyExpensiveFunction(my_parameters));

Not only should it only run when DCHECK can run, it WILL only run when
DCHECK can run.

ditto

[Jói Sigurðsson]

> This doesn't work for compiling out members based on DCHECK_IS_ON>> e.g. couldn't replace the usage of NDEBUG in> http://src.chromium.org/viewvc/chrome/trunk/src/base/threading/thread_checker.h?revision=76833&view=markup and> http://codesearch.google.com/#OAMlx_jo-ck/src/base/debug/stack_trace.h&exact_package=chromium&q=NDEBUG%20file:stack_trace&type=cs&l=23

I have a fix pending that turns ThreadChecker on for the *_rel bots:
http://crbug.com/108227
http://codereview.chromium.org/9020008/

Cheers,
Jói