base: Remove the string logging from CHECK().

base: Remove the string logging from CHECK().

The string is omitted entirely from official builds which means it has
no effect on the build... except on Windows with MSVC. There it affects
the code generation in some cases which produces slightly slower code
that ends up being caught by a layout micro benchmark.

To avoid forever blocking productive work on this impossible-to-fix
problem, just remove the string from CHECK().

R=dcheng@chromium.org
BUG=672699
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

[danakj, 2016-12-09 04:41:46]

Description was changed from

==========
base: Remove the string logging from CHECK().

The string is omitted entirely from official builds which means it has
no effect on the build... except on Windows with MSVC. There it affects
the code generation in some cases which produces slightly slower code
that ends up being caught by a layout micro benchmark.

To avoid forever blocking productive work on this impossible-to-fix
problem, just remove the string from CHECK().

R=dcheng@chromium.org
BUG=672699
==========

to

==========
base: Remove the string logging from CHECK().

The string is omitted entirely from official builds which means it has
no effect on the build... except on Windows with MSVC. There it affects
the code generation in some cases which produces slightly slower code
that ends up being caught by a layout micro benchmark.

To avoid forever blocking productive work on this impossible-to-fix
problem, just remove the string from CHECK().

R=dcheng@chromium.org
BUG=672699
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[danakj, 2016-12-09 04:41:48]

Description was changed from

==========
base: Remove the string logging from CHECK().

The string is omitted entirely from official builds which means it has
no effect on the build... except on Windows with MSVC. There it affects
the code generation in some cases which produces slightly slower code
that ends up being caught by a layout micro benchmark.

To avoid forever blocking productive work on this impossible-to-fix
problem, just remove the string from CHECK().

R=dcheng@chromium.org
BUG=672699
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
base: Remove the string logging from CHECK().

The string is omitted entirely from official builds which means it has
no effect on the build... except on Windows with MSVC. There it affects
the code generation in some cases which produces slightly slower code
that ends up being caught by a layout micro benchmark.

To avoid forever blocking productive work on this impossible-to-fix
problem, just remove the string from CHECK().

R=dcheng@chromium.org
BUG=672699
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[danakj, 2016-12-09 04:41:49]

Description was changed from

==========
base: Remove the string logging from CHECK().

The string is omitted entirely from official builds which means it has
no effect on the build... except on Windows with MSVC. There it affects
the code generation in some cases which produces slightly slower code
that ends up being caught by a layout micro benchmark.

To avoid forever blocking productive work on this impossible-to-fix
problem, just remove the string from CHECK().

R=dcheng@chromium.org
BUG=672699
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
base: Remove the string logging from CHECK().

The string is omitted entirely from official builds which means it has
no effect on the build... except on Windows with MSVC. There it affects
the code generation in some cases which produces slightly slower code
that ends up being caught by a layout micro benchmark.

To avoid forever blocking productive work on this impossible-to-fix
problem, just remove the string from CHECK().

R=dcheng@chromium.org
BUG=672699
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[danakj, 2016-12-09 04:41:51]

Description was changed from

==========
base: Remove the string logging from CHECK().

The string is omitted entirely from official builds which means it has
no effect on the build... except on Windows with MSVC. There it affects
the code generation in some cases which produces slightly slower code
that ends up being caught by a layout micro benchmark.

To avoid forever blocking productive work on this impossible-to-fix
problem, just remove the string from CHECK().

R=dcheng@chromium.org
BUG=672699
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
base: Remove the string logging from CHECK().

The string is omitted entirely from official builds which means it has
no effect on the build... except on Windows with MSVC. There it affects
the code generation in some cases which produces slightly slower code
that ends up being caught by a layout micro benchmark.

To avoid forever blocking productive work on this impossible-to-fix
problem, just remove the string from CHECK().

R=dcheng@chromium.org
BUG=672699
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

[danakj, 2016-12-09 04:41:52]

Description was changed from

==========
base: Remove the string logging from CHECK().

The string is omitted entirely from official builds which means it has
no effect on the build... except on Windows with MSVC. There it affects
the code generation in some cases which produces slightly slower code
that ends up being caught by a layout micro benchmark.

To avoid forever blocking productive work on this impossible-to-fix
problem, just remove the string from CHECK().

R=dcheng@chromium.org
BUG=672699
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

to

==========
base: Remove the string logging from CHECK().

The string is omitted entirely from official builds which means it has
no effect on the build... except on Windows with MSVC. There it affects
the code generation in some cases which produces slightly slower code
that ends up being caught by a layout micro benchmark.

To avoid forever blocking productive work on this impossible-to-fix
problem, just remove the string from CHECK().

R=dcheng@chromium.org
BUG=672699
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[danakj, 2016-12-09 04:41:56]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-09 04:42:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-09 04:49:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-09 04:49:32]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[danakj, 2016-12-09 05:04:15]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-09 05:04:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-09 05:24:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-09 05:24:05]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Alexander Alekseev, 2016-12-09 06:12:02]

alemate@chromium.org changed reviewers:
+ alemate@chromium.org

[Alexander Alekseev, 2016-12-09 06:12:04]

Can we replace them with LOG_FATAL instead?

[Alexander Alekseev, 2016-12-09 06:14:02]

https://codereview.chromium.org/2561963002/diff/20001/ui/base/template_expres...
File ui/base/template_expressions.cc (right):

https://codereview.chromium.org/2561963002/diff/20001/ui/base/template_expres...
ui/base/template_expressions.cc:57: CHECK(value != replacements.end());
I just came across this CHECK and it turned out that I need debug build to debug
HTML!

Could we replace this with LOG_FATAL?

[dcheng, 2016-12-09 06:17:24]

https://codereview.chromium.org/2561963002/diff/20001/ui/base/template_expres...
File ui/base/template_expressions.cc (right):

https://codereview.chromium.org/2561963002/diff/20001/ui/base/template_expres...
ui/base/template_expressions.cc:57: CHECK(value != replacements.end());
On 2016/12/09 06:14:02, Alexander Alekseev wrote:
> I just came across this CHECK and it turned out that I need debug build to
debug
> HTML!
> 
> Could we replace this with LOG_FATAL?

LOG(FATAL) isn't equivalent: CHECK attempts (but fails on MSVC) to optimize out
the string completely in release builds. LOG(FATAL) will include the string no
matter what.

[Alexander Alekseev, 2016-12-09 07:14:50]

https://codereview.chromium.org/2561963002/diff/20001/ui/base/template_expres...
File ui/base/template_expressions.cc (right):

https://codereview.chromium.org/2561963002/diff/20001/ui/base/template_expres...
ui/base/template_expressions.cc:57: CHECK(value != replacements.end());
On 2016/12/09 06:17:24, dcheng wrote:
>
https://codereview.chromium.org/2561963002/diff/20001/ui/base/template_expres...
> File ui/base/template_expressions.cc (right):
> 
>
https://codereview.chromium.org/2561963002/diff/20001/ui/base/template_expres...
> ui/base/template_expressions.cc:57: CHECK(value != replacements.end());
> On 2016/12/09 06:14:02, Alexander Alekseev wrote:
> > I just came across this CHECK and it turned out that I need debug build to
> debug
> > HTML!
> > 
> > Could we replace this with LOG_FATAL?
> 
> LOG(FATAL) isn't equivalent: CHECK attempts (but fails on MSVC) to optimize
out
> the string completely in release builds. LOG(FATAL) will include the string no
> matter what.

This means that that most people used CHECK() with log message incorrectly. As
you see, a lot of changes are in tests, where you would naturally expect error
messages to be always visible.

I used to think that it will display log message always, even in release builds.

I would suggest changing CHECK() so that it will always print error message. But
if it is unacceptable due to some reason, let's change all these occurrences to
LOG(FATAL).
Or, probably, you should ask the owners of individual components of what to do
with the error strings.
My chrome has just crashed here and I was very surprised that I received no
error message. So I believe that owners of this file also expect that message to
be always visible. You cannot understand what is wrong in your html without this
message.

[dcheng, 2016-12-09 07:29:37]

https://codereview.chromium.org/2561963002/diff/20001/ui/base/template_expres...
File ui/base/template_expressions.cc (right):

https://codereview.chromium.org/2561963002/diff/20001/ui/base/template_expres...
ui/base/template_expressions.cc:57: CHECK(value != replacements.end());
On 2016/12/09 07:14:50, Alexander Alekseev wrote:
> On 2016/12/09 06:17:24, dcheng wrote:
> >
>
https://codereview.chromium.org/2561963002/diff/20001/ui/base/template_expres...
> > File ui/base/template_expressions.cc (right):
> > 
> >
>
https://codereview.chromium.org/2561963002/diff/20001/ui/base/template_expres...
> > ui/base/template_expressions.cc:57: CHECK(value != replacements.end());
> > On 2016/12/09 06:14:02, Alexander Alekseev wrote:
> > > I just came across this CHECK and it turned out that I need debug build to
> > debug
> > > HTML!
> > > 
> > > Could we replace this with LOG_FATAL?
> > 
> > LOG(FATAL) isn't equivalent: CHECK attempts (but fails on MSVC) to optimize
> out
> > the string completely in release builds. LOG(FATAL) will include the string
no
> > matter what.
> 
> This means that that most people used CHECK() with log message incorrectly. As
> you see, a lot of changes are in tests, where you would naturally expect error
> messages to be always visible.
> 
> I used to think that it will display log message always, even in release
builds.
> 
> I would suggest changing CHECK() so that it will always print error message.
But
> if it is unacceptable due to some reason, let's change all these occurrences
to
> LOG(FATAL).
> Or, probably, you should ask the owners of individual components of what to do
> with the error strings.
> My chrome has just crashed here and I was very surprised that I received no
> error message. So I believe that owners of this file also expect that message
to
> be always visible. You cannot understand what is wrong in your html without
this
> message.
> 
> 
> 

1. We shouldn't unconditionally emit the string: we pay a fair amount in binary
size to have a bunch of constant strings sitting around for no real reason. I
don't think anyone has numbers but it's non-trivial.

2. Similarly, LOG(...) is highly discouraged for that reason: it generates log
spam and puts a bunch of strings in the official binary that aren't generally
useful for end-users, yet take up a bunch of binary space. In fact, there's a
PRESUBMIT that tries to prevent LOG(...) statements.

For these particular CHECKs, they certainly look like they should be DCHECKs or
properly handled: they almost certainly shouldn't be CHECKs. Either we know what
we're passing into our templates, and we should never violate those invariants,
or we are processing untrusted data, and we need to properly handle invalid
inputs.

[Alexander Alekseev, 2016-12-09 07:41:23]

https://codereview.chromium.org/2561963002/diff/20001/ui/base/template_expres...
File ui/base/template_expressions.cc (right):

https://codereview.chromium.org/2561963002/diff/20001/ui/base/template_expres...
ui/base/template_expressions.cc:57: CHECK(value != replacements.end());
On 2016/12/09 07:29:36, dcheng wrote:
> On 2016/12/09 07:14:50, Alexander Alekseev wrote:
> > On 2016/12/09 06:17:24, dcheng wrote:
> > >
> >
>
https://codereview.chromium.org/2561963002/diff/20001/ui/base/template_expres...
> > > File ui/base/template_expressions.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2561963002/diff/20001/ui/base/template_expres...
> > > ui/base/template_expressions.cc:57: CHECK(value != replacements.end());
> > > On 2016/12/09 06:14:02, Alexander Alekseev wrote:
> > > > I just came across this CHECK and it turned out that I need debug build
to
> > > debug
> > > > HTML!
> > > > 
> > > > Could we replace this with LOG_FATAL?
> > > 
> > > LOG(FATAL) isn't equivalent: CHECK attempts (but fails on MSVC) to
optimize
> > out
> > > the string completely in release builds. LOG(FATAL) will include the
string
> no
> > > matter what.
> > 
> > This means that that most people used CHECK() with log message incorrectly.
As
> > you see, a lot of changes are in tests, where you would naturally expect
error
> > messages to be always visible.
> > 
> > I used to think that it will display log message always, even in release
> builds.
> > 
> > I would suggest changing CHECK() so that it will always print error message.
> But
> > if it is unacceptable due to some reason, let's change all these occurrences
> to
> > LOG(FATAL).
> > Or, probably, you should ask the owners of individual components of what to
do
> > with the error strings.
> > My chrome has just crashed here and I was very surprised that I received no
> > error message. So I believe that owners of this file also expect that
message
> to
> > be always visible. You cannot understand what is wrong in your html without
> this
> > message.
> > 
> > 
> > 
> 
> 1. We shouldn't unconditionally emit the string: we pay a fair amount in
binary
> size to have a bunch of constant strings sitting around for no real reason. I
> don't think anyone has numbers but it's non-trivial.
> 
> 2. Similarly, LOG(...) is highly discouraged for that reason: it generates log
> spam and puts a bunch of strings in the official binary that aren't generally
> useful for end-users, yet take up a bunch of binary space. In fact, there's a
> PRESUBMIT that tries to prevent LOG(...) statements.
> 
> For these particular CHECKs, they certainly look like they should be DCHECKs
or
> properly handled: they almost certainly shouldn't be CHECKs. Either we know
what
> we're passing into our templates, and we should never violate those
invariants,
> or we are processing untrusted data, and we need to properly handle invalid
> inputs.

1) Do you mean that a single explanation message before chrome crash should be
considered as spam?

2) Do you think that CHECKs in test are spam? It is probably difficult to
replace them by ASSERT because of gtest limitatoins, but removing test failure
reasons completely seems weird.

[dcheng, 2016-12-09 07:56:11]

https://codereview.chromium.org/2561963002/diff/20001/ui/base/template_expres...
File ui/base/template_expressions.cc (right):

https://codereview.chromium.org/2561963002/diff/20001/ui/base/template_expres...
ui/base/template_expressions.cc:57: CHECK(value != replacements.end());
On 2016/12/09 07:41:23, Alexander Alekseev wrote:
> On 2016/12/09 07:29:36, dcheng wrote:
> > On 2016/12/09 07:14:50, Alexander Alekseev wrote:
> > > On 2016/12/09 06:17:24, dcheng wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/2561963002/diff/20001/ui/base/template_expres...
> > > > File ui/base/template_expressions.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2561963002/diff/20001/ui/base/template_expres...
> > > > ui/base/template_expressions.cc:57: CHECK(value != replacements.end());
> > > > On 2016/12/09 06:14:02, Alexander Alekseev wrote:
> > > > > I just came across this CHECK and it turned out that I need debug
build
> to
> > > > debug
> > > > > HTML!
> > > > > 
> > > > > Could we replace this with LOG_FATAL?
> > > > 
> > > > LOG(FATAL) isn't equivalent: CHECK attempts (but fails on MSVC) to
> optimize
> > > out
> > > > the string completely in release builds. LOG(FATAL) will include the
> string
> > no
> > > > matter what.
> > > 
> > > This means that that most people used CHECK() with log message
incorrectly.
> As
> > > you see, a lot of changes are in tests, where you would naturally expect
> error
> > > messages to be always visible.
> > > 
> > > I used to think that it will display log message always, even in release
> > builds.
> > > 
> > > I would suggest changing CHECK() so that it will always print error
message.
> > But
> > > if it is unacceptable due to some reason, let's change all these
occurrences
> > to
> > > LOG(FATAL).
> > > Or, probably, you should ask the owners of individual components of what
to
> do
> > > with the error strings.
> > > My chrome has just crashed here and I was very surprised that I received
no
> > > error message. So I believe that owners of this file also expect that
> message
> > to
> > > be always visible. You cannot understand what is wrong in your html
without
> > this
> > > message.
> > > 
> > > 
> > > 
> > 
> > 1. We shouldn't unconditionally emit the string: we pay a fair amount in
> binary
> > size to have a bunch of constant strings sitting around for no real reason.
I
> > don't think anyone has numbers but it's non-trivial.
> > 
> > 2. Similarly, LOG(...) is highly discouraged for that reason: it generates
log
> > spam and puts a bunch of strings in the official binary that aren't
generally
> > useful for end-users, yet take up a bunch of binary space. In fact, there's
a
> > PRESUBMIT that tries to prevent LOG(...) statements.
> > 
> > For these particular CHECKs, they certainly look like they should be DCHECKs
> or
> > properly handled: they almost certainly shouldn't be CHECKs. Either we know
> what
> > we're passing into our templates, and we should never violate those
> invariants,
> > or we are processing untrusted data, and we need to properly handle invalid
> > inputs.
> 
> 1) Do you mean that a single explanation message before chrome crash should be
> considered as spam?

Not necessarily, but there's a binary size tradeoff involved here.

> 
> 2) Do you think that CHECKs in test are spam? It is probably difficult to
> replace them by ASSERT because of gtest limitatoins, but removing test failure
> reasons completely seems weird.

If something is a CHECK just so we can have a death test... it probably
shouldn't be a CHECK. Perhaps I'm misunderstanding this though.

[grt (UTC plus 2), 2016-12-09 08:13:13]

grt@chromium.org changed reviewers:
+ grt@chromium.org, scottmg@chromium.org

[grt (UTC plus 2), 2016-12-09 08:13:14]

+scottmg to weigh on on whether or not this is a good thing. Crashpad includes
the CHECK message in a crash key to help with diagnosing crashes
(https://cs.chromium.org/chromium/src/components/crash/content/app/crashpad.cc...).

[grt (UTC plus 2), 2016-12-09 08:14:44]

Description was changed from

==========
base: Remove the string logging from CHECK().

The string is omitted entirely from official builds which means it has
no effect on the build... except on Windows with MSVC. There it affects
the code generation in some cases which produces slightly slower code
that ends up being caught by a layout micro benchmark.

To avoid forever blocking productive work on this impossible-to-fix
problem, just remove the string from CHECK().

R=dcheng@chromium.org
BUG=672699
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
base: Remove the string logging from CHECK().

The string is omitted entirely from official builds which means it has
no effect on the build... except on Windows with MSVC. There it affects
the code generation in some cases which produces slightly slower code
that ends up being caught by a layout micro benchmark.

To avoid forever blocking productive work on this impossible-to-fix
problem, just remove the string from CHECK().

R=dcheng@chromium.org
BUG=672699
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[grt (UTC plus 2), 2016-12-09 08:14:44]

grt@chromium.org changed reviewers:
- grt@chromium.org

[dcheng, 2016-12-09 08:14:46]

On 2016/12/09 08:13:14, grt (UTC plus 1) wrote:
> +scottmg to weigh on on whether or not this is a good thing. Crashpad includes
> the CHECK message in a crash key to help with diagnosing crashes
>
(https://cs.chromium.org/chromium/src/components/crash/content/app/crashpad.cc...).

I'm pretty sure that's just for LOG(FATAL), which CHECK does not use.

[vabr (Chromium), 2016-12-09 08:29:10]

vabr@chromium.org changed reviewers:
+ vabr@chromium.org

[vabr (Chromium), 2016-12-09 08:29:13]

chrome/browser/autofill/ LGTM

As for the main change, I support dropping the strings to spare the binary size,
as long as one is still able to use LOG(FATAL) for temporary investigations of
hard-to-reproduce crashes.

Cheers,
Vaclav

https://codereview.chromium.org/2561963002/diff/20001/chrome/browser/autofill...
File chrome/browser/autofill/form_structure_browsertest.cc (right):

https://codereview.chromium.org/2561963002/diff/20001/chrome/browser/autofill...
chrome/browser/autofill/form_structure_browsertest.cc:41: // Input file is not
UTF-8.
This comment does not make sense. Thanks to the IsStringUTF8 function name, the
CHECK is self-documenting, please drop the comment.

[inferno, 2016-12-09 14:47:25]

inferno@chromium.org changed reviewers:
+ inferno@chromium.org

[inferno, 2016-12-09 14:47:27]

Check failure strings are needed for testcase deduplication during fuzzing on
ClusterFuzz. Please don't remove the strings, those are useful for debugging as
well.

[mmenke, 2016-12-09 15:22:32]

On 2016/12/09 14:47:27, inferno wrote:
> Check failure strings are needed for testcase deduplication during fuzzing on
> ClusterFuzz. Please don't remove the strings, those are useful for debugging
as
> well.

Random driveby suggestions:  Can we only enable the strings if DCHECK is on for
the build or something?

[inferno, 2016-12-09 15:27:52]

On 2016/12/09 15:22:32, mmenke wrote:
> On 2016/12/09 14:47:27, inferno wrote:
> > Check failure strings are needed for testcase deduplication during fuzzing
on
> > ClusterFuzz. Please don't remove the strings, those are useful for debugging
> as
> > well.
> 
> Random driveby suggestions:  Can we only enable the strings if DCHECK is on
for
> the build or something?

I think it should be ok to strip for OFFICIAL_BUILD, but for regular chromium
build, we need this for debugging. Fuzzing builds are both release and debug, so
we need in release builds as well.

[scottmg, 2016-12-09 16:21:42]

Hearty +1 to the cl based on the description, it's always seemed crazy to me
that we ship debug strings in billions of binaries. (I didn't look at the
logging.h changes enough to understand them though.)

[scottmg, 2016-12-09 16:23:49]

On 2016/12/09 16:21:42, scottmg wrote:
> Hearty +1 to the cl based on the description, it's always seemed crazy to me
> that we ship debug strings in billions of binaries. (I didn't look at the
> logging.h changes enough to understand them though.)

Oh. I see now (from the enormous CL size!) we have to manually drop them. That's
pretty weak. There's really no way to get the << to optimize away? :/

[dcheng, 2016-12-09 16:27:17]

On 2016/12/09 16:23:49, scottmg wrote:
> On 2016/12/09 16:21:42, scottmg wrote:
> > Hearty +1 to the cl based on the description, it's always seemed crazy to me
> > that we ship debug strings in billions of binaries. (I didn't look at the
> > logging.h changes enough to understand them though.)
> 
> Oh. I see now (from the enormous CL size!) we have to manually drop them.
That's
> pretty weak. There's really no way to get the << to optimize away? :/

logging.h has a bunch of optimizations that's supposed to achieve this, and it
works everywhere except MSVC =/

[scottmg, 2016-12-09 16:28:17]

scottmg@chromium.org changed reviewers:
+ mark@chromium.org

[scottmg, 2016-12-09 16:28:20]

+mark will likely want to do something different for third_party/crashpad to not
lose those strings there.

[[[ LOG << "str" has always smelled so bad. What the heck was wrong with
LOG("str")? ]]]

[scottmg, 2016-12-09 16:31:15]

On 2016/12/09 16:27:17, dcheng wrote:
> On 2016/12/09 16:23:49, scottmg wrote:
> > On 2016/12/09 16:21:42, scottmg wrote:
> > > Hearty +1 to the cl based on the description, it's always seemed crazy to
me
> > > that we ship debug strings in billions of binaries. (I didn't look at the
> > > logging.h changes enough to understand them though.)
> > 
> > Oh. I see now (from the enormous CL size!) we have to manually drop them.
> That's
> > pretty weak. There's really no way to get the << to optimize away? :/
> 
> logging.h has a bunch of optimizations that's supposed to achieve this, and it
> works everywhere except MSVC =/

I completely believe you and am not overly surprised, but do we have an
standalone or specific file I could fiddle with? At the very least we should
complain to Microsoft as it presumably means there's a pretty easy optimization
that's being missed.

[dcheng, 2016-12-09 16:42:54]

On 2016/12/09 16:31:15, scottmg wrote:
> On 2016/12/09 16:27:17, dcheng wrote:
> > On 2016/12/09 16:23:49, scottmg wrote:
> > > On 2016/12/09 16:21:42, scottmg wrote:
> > > > Hearty +1 to the cl based on the description, it's always seemed crazy
to
> me
> > > > that we ship debug strings in billions of binaries. (I didn't look at
the
> > > > logging.h changes enough to understand them though.)
> > > 
> > > Oh. I see now (from the enormous CL size!) we have to manually drop them.
> > That's
> > > pretty weak. There's really no way to get the << to optimize away? :/
> > 
> > logging.h has a bunch of optimizations that's supposed to achieve this, and
it
> > works everywhere except MSVC =/
> 
> I completely believe you and am not overly surprised, but do we have an
> standalone or specific file I could fiddle with? At the very least we should
> complain to Microsoft as it presumably means there's a pretty easy
optimization
> that's being missed.

See https://bugs.chromium.org/p/chromium/issues/detail?id=672699 and
https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/TL0NkNXIT1w/mb...
for some previous context about this. I believe we've filed a connect issue as
well.

[Primiano Tucci (use gerrit), 2016-12-09 16:44:47]

Just throwing an idea there:
the entire problem here are not the strings themselves (they are already NOT
part of the official binary).
The problem is that supporting all the << operator as it is today is quite
complicated and as danakj says it becomes zero-overhead everywhere but not on
MSVC.

Now, having said that, IMHO a softer alternative for this would be switching
from:
CHECK(condition) << crazy << stream << that << maybe << few << pepole << use;

to

CHECK(condition, "static string");
And drop all the fancy stream case?

I am pretty confident that is possible to write the latter in a way that
excludes the string from the binary in official builds AND has really zero
overhead everywhere.

[Mark Mentovai, 2016-12-09 17:02:39]

scottmg wrote:
> On 2016/12/09 16:21:42, scottmg wrote:
> > Hearty +1 to the cl based on the description, it's always seemed crazy to me
> > that we ship debug strings in billions of binaries. (I didn't look at the
> > logging.h changes enough to understand them though.)
> 
> Oh. I see now (from the enormous CL size!) we have to manually drop them.
That's
> pretty weak. There's really no way to get the << to optimize away? :/

This is pretty sad to me too. These strings have value, even if we’re not
intending them to have value in “official” builds. The fact that there are so
many of them scattered throughout the codebase is a bit of an indication that
many other developers also expect them to behave a certain way.

What’s the real impact here, compressed downloadable installer-wise?

[dcheng, 2016-12-09 17:08:47]

On 2016/12/09 17:02:39, Mark Mentovai wrote:
> scottmg wrote:
> > On 2016/12/09 16:21:42, scottmg wrote:
> > > Hearty +1 to the cl based on the description, it's always seemed crazy to
me
> > > that we ship debug strings in billions of binaries. (I didn't look at the
> > > logging.h changes enough to understand them though.)
> > 
> > Oh. I see now (from the enormous CL size!) we have to manually drop them.
> That's
> > pretty weak. There's really no way to get the << to optimize away? :/
> 
> This is pretty sad to me too. These strings have value, even if we’re not
> intending them to have value in “official” builds. The fact that there are so
> many of them scattered throughout the codebase is a bit of an indication that
> many other developers also expect them to behave a certain way.
> 
> What’s the real impact here, compressed downloadable installer-wise?

The core issue here is that supporting << costs us a non-trivial amount in perf.
When we replaced all RELEASE_ASSERT with CHECK in Blink, we took a hit on layout
perf: https://bugs.chromium.org/p/chromium/issues/detail?id=614852#c5

I think part of the issue is there are probably RELEASE_ASSERTS in inappropriate
paths in Blink, but it would also be good if we could make CHECK() more
efficient (on MSVC). This came up in the CL to move PartitionAlloc into //base:
there were objections to using CHECK() inside the allocator, since
PartitionAlloc previously only used RELEASE_ASSERT.

[scottmg, 2016-12-09 17:12:04]

On 2016/12/09 17:02:39, Mark Mentovai wrote:
> scottmg wrote:
> > On 2016/12/09 16:21:42, scottmg wrote:
> > > Hearty +1 to the cl based on the description, it's always seemed crazy to
me
> > > that we ship debug strings in billions of binaries. (I didn't look at the
> > > logging.h changes enough to understand them though.)
> > 
> > Oh. I see now (from the enormous CL size!) we have to manually drop them.
> That's
> > pretty weak. There's really no way to get the << to optimize away? :/
> 
> This is pretty sad to me too. These strings have value, even if we’re not
> intending them to have value in “official” builds. The fact that there are so
> many of them scattered throughout the codebase is a bit of an indication that
> many other developers also expect them to behave a certain way.
> 
> What’s the real impact here, compressed downloadable installer-wise?

OK, so to resummarize the bug and blink-dev (and what everyone else probably
already knows, but I talk too much :) the *actual* problem is that on
Windows-only, x86-only, we generate a few extra instructions in functions that
do a CHECK() << "blah";. As Primiano pointed out, the strings are already
stripped, it's just that the the EAT_STREAM_PARAMETERS doesn't effectively drop
everything. (That's my understanding anyway).

Reading tkent's blink post, it seems that he's identified a fix though, no? I'll
muck around some more with his code, but doesn't it seem like we just need a
better EAT_STREAM_PARAMETERS for Windows?

[dcheng, 2016-12-09 17:22:55]

On 2016/12/09 17:12:04, scottmg wrote:
> On 2016/12/09 17:02:39, Mark Mentovai wrote:
> > scottmg wrote:
> > > On 2016/12/09 16:21:42, scottmg wrote:
> > > > Hearty +1 to the cl based on the description, it's always seemed crazy
to
> me
> > > > that we ship debug strings in billions of binaries. (I didn't look at
the
> > > > logging.h changes enough to understand them though.)
> > > 
> > > Oh. I see now (from the enormous CL size!) we have to manually drop them.
> > That's
> > > pretty weak. There's really no way to get the << to optimize away? :/
> > 
> > This is pretty sad to me too. These strings have value, even if we’re not
> > intending them to have value in “official” builds. The fact that there are
so
> > many of them scattered throughout the codebase is a bit of an indication
that
> > many other developers also expect them to behave a certain way.
> > 
> > What’s the real impact here, compressed downloadable installer-wise?
> 
> OK, so to resummarize the bug and blink-dev (and what everyone else probably
> already knows, but I talk too much :) the *actual* problem is that on
> Windows-only, x86-only, we generate a few extra instructions in functions that
> do a CHECK() << "blah";. As Primiano pointed out, the strings are already
> stripped, it's just that the the EAT_STREAM_PARAMETERS doesn't effectively
drop
> everything. (That's my understanding anyway).
> 
> Reading tkent's blink post, it seems that he's identified a fix though, no?
I'll
> muck around some more with his code, but doesn't it seem like we just need a
> better EAT_STREAM_PARAMETERS for Windows?

My understanding (possibly outdated) is that we haven't been able to figure out
how to trick MSVC into optimizing it away completely. The bug hints that this is
related to destructors (destructor for the unused stream object, I assume?). I
think LOG_STREAM() always returns a std::ostream object, generating some extra
prolog / epilog instructions that end up being unnecessary.

I wonder what would happen if we returned a POD object to stand in for the
stream with a templated operator<< that matched everything...

[scottmg, 2016-12-09 17:25:11]

On 2016/12/09 17:22:55, dcheng wrote:
> On 2016/12/09 17:12:04, scottmg wrote:
> > On 2016/12/09 17:02:39, Mark Mentovai wrote:
> > > scottmg wrote:
> > > > On 2016/12/09 16:21:42, scottmg wrote:
> > > > > Hearty +1 to the cl based on the description, it's always seemed crazy
> to
> > me
> > > > > that we ship debug strings in billions of binaries. (I didn't look at
> the
> > > > > logging.h changes enough to understand them though.)
> > > > 
> > > > Oh. I see now (from the enormous CL size!) we have to manually drop
them.
> > > That's
> > > > pretty weak. There's really no way to get the << to optimize away? :/
> > > 
> > > This is pretty sad to me too. These strings have value, even if we’re not
> > > intending them to have value in “official” builds. The fact that there are
> so
> > > many of them scattered throughout the codebase is a bit of an indication
> that
> > > many other developers also expect them to behave a certain way.
> > > 
> > > What’s the real impact here, compressed downloadable installer-wise?
> > 
> > OK, so to resummarize the bug and blink-dev (and what everyone else probably
> > already knows, but I talk too much :) the *actual* problem is that on
> > Windows-only, x86-only, we generate a few extra instructions in functions
that
> > do a CHECK() << "blah";. As Primiano pointed out, the strings are already
> > stripped, it's just that the the EAT_STREAM_PARAMETERS doesn't effectively
> drop
> > everything. (That's my understanding anyway).
> > 
> > Reading tkent's blink post, it seems that he's identified a fix though, no?
> I'll
> > muck around some more with his code, but doesn't it seem like we just need a
> > better EAT_STREAM_PARAMETERS for Windows?
> 
> My understanding (possibly outdated) is that we haven't been able to figure
out
> how to trick MSVC into optimizing it away completely. The bug hints that this
is
> related to destructors (destructor for the unused stream object, I assume?). I
> think LOG_STREAM() always returns a std::ostream object, generating some extra
> prolog / epilog instructions that end up being unnecessary.
> 
> I wonder what would happen if we returned a POD object to stand in for the
> stream with a templated operator<< that matched everything...

Yeah, that's what I was thinking too. I'll test that out.

[Primiano Tucci (use gerrit), 2016-12-09 17:29:49]

On 2016/12/09 17:12:04, scottmg wrote:
> On 2016/12/09 17:02:39, Mark Mentovai wrote:
> > scottmg wrote:
> > > On 2016/12/09 16:21:42, scottmg wrote:
> > > > Hearty +1 to the cl based on the description, it's always seemed crazy
to
> me
> > > > that we ship debug strings in billions of binaries. (I didn't look at
the
> > > > logging.h changes enough to understand them though.)
> > > 
> > > Oh. I see now (from the enormous CL size!) we have to manually drop them.
> > That's
> > > pretty weak. There's really no way to get the << to optimize away? :/
> > 
> > This is pretty sad to me too. These strings have value, even if we’re not
> > intending them to have value in “official” builds. The fact that there are
so
> > many of them scattered throughout the codebase is a bit of an indication
that
> > many other developers also expect them to behave a certain way.
> > 
> > What’s the real impact here, compressed downloadable installer-wise?
> 
> OK, so to resummarize the bug and blink-dev (and what everyone else probably
> already knows, but I talk too much :) the *actual* problem is that on
> Windows-only, x86-only
(not sure about x86-only or also x86-64)

>, we generate a few extra instructions in functions that
> do a CHECK() << "blah";.
Correct. Specifically the problem seems to be that the Dtor of LogMessage (not
LogMessageVodify) causes extra instructions to be emitted, essentially a missed
compiler optimization.

The MSVC issue is
https://connect.microsoft.com/VisualStudio/Feedback/Details/3112569 but it takes
a MSDN subscription.
I got a copy @ https://ghostbin.com/paste/4c48q and I've been told MS didn't
reply to that.
The issue repros on MSVC15 (the current one in depot_tools) and tkent tried also
MSVC2017 (there it repros in a slighly different way but still repros)

Copy/pasting form a private conversation I had with tkent to get more context.
The CHECK in official build works as follows:

#define CHECK(condition) \
  !(condition) ? IMMEDIATE_CRASH() : EAT_STREAM_PARAMETERS

#define EAT_STREAM_PARAMETERS                                           \
  true ? (void) 0 : ::logging::LogMessageVoidify() & LOG_STREAM(FATAL)

#define LOG_STREAM(severity) COMPACT_GOOGLE_LOG_ ## severity.stream()

#define COMPACT_GOOGLE_LOG_FATAL \
  COMPACT_GOOGLE_LOG_EX_FATAL(LogMessage)

#define COMPACT_GOOGLE_LOG_EX_FATAL(ClassName, ...) \
  ::logging::ClassName(__FILE__, __LINE__, ::logging::LOG_FATAL, ##__VA_ARGS__)


So, collapsing all the macros, CHECK(foo) << "bar" becomes (take a breath and
keep calm for operator precedence)

!(foo) ? IMMEDIATE_CRASH() : true ? void(0) : ::logging::LogMessageVoidify() &
LogMessage(__FILE__, __LINE__) << "bar";

the problem here seems to be the dtor of LogMessage.


> As Primiano pointed out, the strings are already
> stripped, it's just that the the EAT_STREAM_PARAMETERS doesn't effectively
drop
> everything. (That's my understanding anyway).
> 
> Reading tkent's blink post, it seems that he's identified a fix though, no?
I'll
> muck around some more with his code, 
Don't think there is a known fix yet. only an unanswered bug with MS

> but doesn't it seem like we just need a better EAT_STREAM_PARAMETERS for
Windows?
Right. If you can come with one I think this becomes entirely a non-issue.

[scottmg, 2016-12-09 18:08:06]

On 2016/12/09 17:29:49, Primiano Tucci wrote:
> On 2016/12/09 17:12:04, scottmg wrote:
> > On 2016/12/09 17:02:39, Mark Mentovai wrote:
> > > scottmg wrote:
> > > > On 2016/12/09 16:21:42, scottmg wrote:
> > > > > Hearty +1 to the cl based on the description, it's always seemed crazy
> to
> > me
> > > > > that we ship debug strings in billions of binaries. (I didn't look at
> the
> > > > > logging.h changes enough to understand them though.)
> > > > 
> > > > Oh. I see now (from the enormous CL size!) we have to manually drop
them.
> > > That's
> > > > pretty weak. There's really no way to get the << to optimize away? :/
> > > 
> > > This is pretty sad to me too. These strings have value, even if we’re not
> > > intending them to have value in “official” builds. The fact that there are
> so
> > > many of them scattered throughout the codebase is a bit of an indication
> that
> > > many other developers also expect them to behave a certain way.
> > > 
> > > What’s the real impact here, compressed downloadable installer-wise?
> > 
> > OK, so to resummarize the bug and blink-dev (and what everyone else probably
> > already knows, but I talk too much :) the *actual* problem is that on
> > Windows-only, x86-only
> (not sure about x86-only or also x86-64)
> 
> >, we generate a few extra instructions in functions that
> > do a CHECK() << "blah";.
> Correct. Specifically the problem seems to be that the Dtor of LogMessage (not
> LogMessageVodify) causes extra instructions to be emitted, essentially a
missed
> compiler optimization.
> 
> The MSVC issue is
> https://connect.microsoft.com/VisualStudio/Feedback/Details/3112569 but it
takes
> a MSDN subscription.
> I got a copy @ https://ghostbin.com/paste/4c48q and I've been told MS didn't
> reply to that.
> The issue repros on MSVC15 (the current one in depot_tools) and tkent tried
also
> MSVC2017 (there it repros in a slighly different way but still repros)
> 
> Copy/pasting form a private conversation I had with tkent to get more context.
> The CHECK in official build works as follows:
> 
> #define CHECK(condition) \
>   !(condition) ? IMMEDIATE_CRASH() : EAT_STREAM_PARAMETERS
> 
> #define EAT_STREAM_PARAMETERS                                           \
>   true ? (void) 0 : ::logging::LogMessageVoidify() & LOG_STREAM(FATAL)
> 
> #define LOG_STREAM(severity) COMPACT_GOOGLE_LOG_ ## severity.stream()
> 
> #define COMPACT_GOOGLE_LOG_FATAL \
>   COMPACT_GOOGLE_LOG_EX_FATAL(LogMessage)
> 
> #define COMPACT_GOOGLE_LOG_EX_FATAL(ClassName, ...) \
>   ::logging::ClassName(__FILE__, __LINE__, ::logging::LOG_FATAL,
##__VA_ARGS__)
> 
> 
> So, collapsing all the macros, CHECK(foo) << "bar" becomes (take a breath and
> keep calm for operator precedence)
> 
> !(foo) ? IMMEDIATE_CRASH() : true ? void(0) : ::logging::LogMessageVoidify() &
> LogMessage(__FILE__, __LINE__) << "bar";
> 
> the problem here seems to be the dtor of LogMessage.
> 
> 
> > As Primiano pointed out, the strings are already
> > stripped, it's just that the the EAT_STREAM_PARAMETERS doesn't effectively
> drop
> > everything. (That's my understanding anyway).
> > 
> > Reading tkent's blink post, it seems that he's identified a fix though, no?
> I'll
> > muck around some more with his code, 
> Don't think there is a known fix yet. only an unanswered bug with MS
> 
> > but doesn't it seem like we just need a better EAT_STREAM_PARAMETERS for
> Windows?
> Right. If you can come with one I think this becomes entirely a non-issue.

(Am I completely missing something? (maybe operator precedence?)
https://codereview.chromium.org/2559323007/)

[Primiano Tucci (use gerrit), 2016-12-09 18:29:39]

So I got a minified a repro here:
https://ghostbin.com/paste/njn2d

On 2016/12/09 18:08:06, scottmg wrote:
> (Am I completely missing something? (maybe operator precedence?)
> https://codereview.chromium.org/2559323007/)

Not that I know about, in fact I ended up doing pretty much the same here :D
https://codereview.chromium.org/2532513002/

Let's see what trybot say. If this goes green I believe that this should be
enough to bring back CHECK on-par with RELEASE_ASSERT (either my trick or yours
shouldn't matter)
Ah, realized only now, we still miss a UNLIKELY() around the condition.

I'm going to leave the office now. would be great if somebody can get a
variation of scott's or the above + UNLIKELY so we get a reply from perf bots
next week.

[Alexander Alekseev, 2016-12-10 01:08:14]

Could we introduce ASSERT macro (from Blink) to Chrome?

I think it is very useful if you have debug-only members and methods (which
Blink uses a lot).

[dcheng, 2016-12-10 03:45:13]

On 2016/12/10 01:08:14, Alexander Alekseev wrote:
> Could we introduce ASSERT macro (from Blink) to Chrome?
> 
> I think it is very useful if you have debug-only members and methods (which
> Blink uses a lot).

DCHECK == ASSERT. In any case, I think we're going to land primiano's fix, so we
shouldn't need to do this.

(CHECK is still not going to emit a string in official builds; sorry, but that
can't change)

[Alexander Alekseev, 2016-12-10 04:49:11]

On 2016/12/10 03:45:13, dcheng wrote:
> On 2016/12/10 01:08:14, Alexander Alekseev wrote:
> > Could we introduce ASSERT macro (from Blink) to Chrome?
> > 
> > I think it is very useful if you have debug-only members and methods (which
> > Blink uses a lot).
> 
> DCHECK == ASSERT. In any case, I think we're going to land primiano's fix, so
we
> shouldn't need to do this.
> 
> (CHECK is still not going to emit a string in official builds; sorry, but that
> can't change)

ASSERT (in a way it is defined in Blink) expands into empty string in release
builds.
This simplifies handling of debug variables which do not exist in release builds
(less #ifdefs).
So probably we can have this macro also in chrome, if the data that is checked
does not exist in release build.

[jbroman, 2016-12-10 15:23:13]

On 2016/12/10 at 04:49:11, alemate wrote:
> On 2016/12/10 03:45:13, dcheng wrote:
> > On 2016/12/10 01:08:14, Alexander Alekseev wrote:
> > > Could we introduce ASSERT macro (from Blink) to Chrome?
> > > 
> > > I think it is very useful if you have debug-only members and methods
(which
> > > Blink uses a lot).
> > 
> > DCHECK == ASSERT. In any case, I think we're going to land primiano's fix,
so we
> > shouldn't need to do this.
> > 
> > (CHECK is still not going to emit a string in official builds; sorry, but
that
> > can't change)
> 
> ASSERT (in a way it is defined in Blink) expands into empty string in release
builds.
> This simplifies handling of debug variables which do not exist in release
builds (less #ifdefs).
> So probably we can have this macro also in chrome, if the data that is checked
does not exist in release build.

I'd rather not have both. Ultimately either:
- the expression is "used" (as with DCHECK), and so you need to add preprocessor
guards if the variable is not defined in debug builds, or
- the expression is "unused" (as with ASSERT), and so you need to suppress
unused variable warnings in non-debug builds (using ASSERT_UNUSED, or (void)(),
or similar)

I don't think it's a big difference either way (though I mildly prefer DCHECK's
approach), but having both doesn't seem to add much.

If you're maintaining variables that don't exist in non-debug builds, you're
going to need to have "#if DCHECK_IS_ON()" at the places where you define and
update it, so it doesn't seem unreasonable to have it also at the place where
you read it.

[Alexander Alekseev, 2016-12-11 09:59:24]

On 2016/12/10 15:23:13, jbroman wrote:
> On 2016/12/10 at 04:49:11, alemate wrote:
> > On 2016/12/10 03:45:13, dcheng wrote:
> > > On 2016/12/10 01:08:14, Alexander Alekseev wrote:
> > > > Could we introduce ASSERT macro (from Blink) to Chrome?
> > > > 
> > > > I think it is very useful if you have debug-only members and methods
> (which
> > > > Blink uses a lot).
> > > 
> > > DCHECK == ASSERT. In any case, I think we're going to land primiano's fix,
> so we
> > > shouldn't need to do this.
> > > 
> > > (CHECK is still not going to emit a string in official builds; sorry, but
> that
> > > can't change)
> > 
> > ASSERT (in a way it is defined in Blink) expands into empty string in
release
> builds.
> > This simplifies handling of debug variables which do not exist in release
> builds (less #ifdefs).
> > So probably we can have this macro also in chrome, if the data that is
checked
> does not exist in release build.
> 
> I'd rather not have both. Ultimately either:
> - the expression is "used" (as with DCHECK), and so you need to add
preprocessor
> guards if the variable is not defined in debug builds, or
> - the expression is "unused" (as with ASSERT), and so you need to suppress
> unused variable warnings in non-debug builds (using ASSERT_UNUSED, or
(void)(),
> or similar)
> 
> I don't think it's a big difference either way (though I mildly prefer
DCHECK's
> approach), but having both doesn't seem to add much.
> 
> If you're maintaining variables that don't exist in non-debug builds, you're
> going to need to have "#if DCHECK_IS_ON()" at the places where you define and
> update it, so it doesn't seem unreasonable to have it also at the place where
> you read it.

I just replaced ASSERT with DCHECK in a few places in Blink code, and instead of
a single #ifdef when declaring debug reference counter, there are now #ifdefs
around 10 DCHECKS. I.e. we tripled number of lines with preprocessor directives.
Yes, this doesn't always happen, but I'd rather have both of them.

[dcheng, 2016-12-11 19:24:50]

On 2016/12/11 09:59:24, Alexander Alekseev wrote:
> On 2016/12/10 15:23:13, jbroman wrote:
> > On 2016/12/10 at 04:49:11, alemate wrote:
> > > On 2016/12/10 03:45:13, dcheng wrote:
> > > > On 2016/12/10 01:08:14, Alexander Alekseev wrote:
> > > > > Could we introduce ASSERT macro (from Blink) to Chrome?
> > > > > 
> > > > > I think it is very useful if you have debug-only members and methods
> > (which
> > > > > Blink uses a lot).
> > > > 
> > > > DCHECK == ASSERT. In any case, I think we're going to land primiano's
fix,
> > so we
> > > > shouldn't need to do this.
> > > > 
> > > > (CHECK is still not going to emit a string in official builds; sorry,
but
> > that
> > > > can't change)
> > > 
> > > ASSERT (in a way it is defined in Blink) expands into empty string in
> release
> > builds.
> > > This simplifies handling of debug variables which do not exist in release
> > builds (less #ifdefs).
> > > So probably we can have this macro also in chrome, if the data that is
> checked
> > does not exist in release build.
> > 
> > I'd rather not have both. Ultimately either:
> > - the expression is "used" (as with DCHECK), and so you need to add
> preprocessor
> > guards if the variable is not defined in debug builds, or
> > - the expression is "unused" (as with ASSERT), and so you need to suppress
> > unused variable warnings in non-debug builds (using ASSERT_UNUSED, or
> (void)(),
> > or similar)
> > 
> > I don't think it's a big difference either way (though I mildly prefer
> DCHECK's
> > approach), but having both doesn't seem to add much.
> > 
> > If you're maintaining variables that don't exist in non-debug builds, you're
> > going to need to have "#if DCHECK_IS_ON()" at the places where you define
and
> > update it, so it doesn't seem unreasonable to have it also at the place
where
> > you read it.
> 
> I just replaced ASSERT with DCHECK in a few places in Blink code, and instead
of
> a single #ifdef when declaring debug reference counter, there are now #ifdefs
> around 10 DCHECKS. I.e. we tripled number of lines with preprocessor
directives.
> Yes, this doesn't always happen, but I'd rather have both of them.

This is a good point, but unrelated to this review. Can you file a bug and we
can continue the discussion there?

[Sergey Ulanov, 2016-12-11 23:11:42]

sergeyu@chromium.org changed reviewers:
+ sergeyu@chromium.org

[Sergey Ulanov, 2016-12-11 23:11:46]

https://codereview.chromium.org/2561963002/diff/20001/remoting/host/curtain_m...
File remoting/host/curtain_mode_mac.cc (right):

https://codereview.chromium.org/2561963002/diff/20001/remoting/host/curtain_m...
remoting/host/curtain_mode_mac.cc:135: // Error activating curtain-mode:
CGSessionCopyCurrentDictionary() returned
This comment would be confusing, especially given that we have more detailed
comments above. I would prefer just removing the error messages from CHECKs
instead of moving them to comments.

[dcheng, 2016-12-11 23:15:03]

https://codereview.chromium.org/2561963002/diff/20001/remoting/host/curtain_m...
File remoting/host/curtain_mode_mac.cc (right):

https://codereview.chromium.org/2561963002/diff/20001/remoting/host/curtain_m...
remoting/host/curtain_mode_mac.cc:135: // Error activating curtain-mode:
CGSessionCopyCurrentDictionary() returned
On 2016/12/11 23:11:45, Sergey Ulanov wrote:
> This comment would be confusing, especially given that we have more detailed
> comments above. I would prefer just removing the error messages from CHECKs
> instead of moving them to comments. 

We've landed a change which should make this CL unnecessary. I'm going to go
ahead and close this CL to avoid further confusion

[dcheng, 2016-12-11 23:15:59]

Description was changed from

==========
base: Remove the string logging from CHECK().

The string is omitted entirely from official builds which means it has
no effect on the build... except on Windows with MSVC. There it affects
the code generation in some cases which produces slightly slower code
that ends up being caught by a layout micro benchmark.

To avoid forever blocking productive work on this impossible-to-fix
problem, just remove the string from CHECK().

R=dcheng@chromium.org
BUG=672699
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
base: Remove the string logging from CHECK().

The string is omitted entirely from official builds which means it has
no effect on the build... except on Windows with MSVC. There it affects
the code generation in some cases which produces slightly slower code
that ends up being caught by a layout micro benchmark.

To avoid forever blocking productive work on this impossible-to-fix
problem, just remove the string from CHECK().

R=dcheng@chromium.org
BUG=672699
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========