Make sure DCHECK compiles out completely if DCHECKS aren't enabled.

Make sure DCHECK compiles out completely if DCHECKS aren't enabled.

The MSVC optimizer can get confused when expressions that create
temporaries are completely optimized out.

Before:
2017-01-24  05:59 PM         1,139,712 chrome.exe
2017-01-24  05:53 PM        48,618,496 chrome.dll
2017-01-24  05:59 PM        68,739,584 chrome_child.dll

After:
2017-01-24  06:51 PM         1,136,128 chrome.exe
2017-01-24  06:45 PM        48,265,216 chrome.dll
2017-01-24  06:51 PM        68,245,504 chrome_child.dll

This results in a total savings of ~830KB in a mostly official build.
These numbers don't include the effect of full_wpo_on_official=true,
but it's been confirmed that this codegen bug happens either way.

BUG=684105
R=scottmg@chromium.org

Review-Url: https://codereview.chromium.org/2653073002
Cr-Commit-Position: refs/heads/master@{#446054}
Committed: https://chromium.googlesource.com/chromium/src/+/fc670f47ae174fb59497fe5d3315b10cc7601cad

[dcheng, 2017-01-24 20:55:49]

I think this should work, but I don't have a local test case.

We'll also want something similar for DPCHECk and DCHECK_OP, I believe.

[danakj, 2017-01-24 20:58:02]

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

[danakj, 2017-01-24 20:58:03]

Seems cool, since DCHECK_IS_ON() is no longer runtime, there's no reason to use
it like so.

[dcheng, 2017-01-24 21:17:05]

OK, I think I managed to trick DCHECK_OP into using this optimization as well.

[scottmg, 2017-01-24 21:25:58]

lgtm

https://codereview.chromium.org/2653073002/diff/60001/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/2653073002/diff/60001/base/logging.h#newcode742
base/logging.h:742: #define DCHECK(condition) EAT_STREAM_PARAMETERS <<
!(condition)
!'ing condition seems a bit odd here.

[scottmg, 2017-01-24 21:28:21]

As discussed offline, a test would be nice, but would only work in Official, and
I don't think the Official bots currently run all the tests, so it would be
challenging.

... I suppose we could have a more conservative Release-only test, in that if it
gets removed in plain-Release then we could be pretty confident that it'll get
removed in Official.

... Oh, but all the bots run with dcheck_always_on, soooooo... probably not.

So it's definitely going to be tricky to add a test for this. Maybe just delete
base/logging.h and see what goes wrong?

[danakj, 2017-01-24 21:30:22]

On Tue, Jan 24, 2017 at 4:28 PM, <scottmg@chromium.org> wrote:

> As discussed offline, a test would be nice, but would only work in
> Official, and
> I don't think the Official bots currently run all the tests, so it would be
> challenging.
>

I think they run base unittests?

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

[scottmg, 2017-01-24 21:36:16]

On 2017/01/24 21:30:22, danakj (slow) wrote:
> On Tue, Jan 24, 2017 at 4:28 PM, <mailto:scottmg@chromium.org> wrote:
> 
> > As discussed offline, a test would be nice, but would only work in
> > Official, and
> > I don't think the Official bots currently run all the tests, so it would be
> > challenging.
> >
> 
> I think they run base unittests?

Oh, you're right! They do now. OK, test please. :)

Might still be a bit tricky in there to get the disassembly to confirm the
function is empty. One possibility -- two functions with obscure signatures,
containing the same bodies other than one with a DCHECK and one without should
get COMDAT'd to the same address.

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

[dcheng, 2017-01-24 21:37:44]

On 2017/01/24 21:30:22, danakj (slow) wrote:
> On Tue, Jan 24, 2017 at 4:28 PM, <mailto:scottmg@chromium.org> wrote:
> 
> > As discussed offline, a test would be nice, but would only work in
> > Official, and
> > I don't think the Official bots currently run all the tests, so it would be
> > challenging.
> >
> 
> I think they run base unittests?

I was just talking with nick@ offline, and he pointed out we can write a
perftest to exercise these codepaths directly: wasn't it a Blink microbenchmark
the caught the original regression with CHECK()?

So it's a bit indirect, but hopefully will suffice. The main problem is for
DCHECK, the conditions for this not getting optimized aren't as straightforward.
So I'll try in a followup, if that's OK.

[dcheng, 2017-01-24 21:44:50]

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

[commit-bot: I haz the power, 2017-01-24 21:45:10]

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

[ncarter (slow), 2017-01-24 21:52:06]

nick@chromium.org changed reviewers:
+ nick@chromium.org, wez@chromium.org

[ncarter (slow), 2017-01-24 21:52:06]

+wez as an FYI

[ncarter (slow), 2017-01-24 21:52:25]

lgtm

[ncarter (slow), 2017-01-24 22:04:43]

I patched this in, and verified that it fixes the repro I reported.

[commit-bot: I haz the power, 2017-01-24 22:38:22]

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

[commit-bot: I haz the power, 2017-01-24 22:38:23]

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-...)

[dcheng, 2017-01-24 23:26:34]

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

[commit-bot: I haz the power, 2017-01-24 23:27:15]

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

[dcheng, 2017-01-24 23:27:43]

Patchset #6 (id:100001) has been deleted

[dcheng, 2017-01-24 23:28:48]

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

[commit-bot: I haz the power, 2017-01-24 23:29:40]

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, 2017-01-24 23:42:04]

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

[commit-bot: I haz the power, 2017-01-24 23:42:05]

Dry run: Try jobs failed on following builders:
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[dcheng, 2017-01-24 23:51:49]

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

[commit-bot: I haz the power, 2017-01-24 23:52:44]

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, 2017-01-25 00:05:56]

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

[commit-bot: I haz the power, 2017-01-25 00:05:57]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[dcheng, 2017-01-25 00:52:38]

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

[commit-bot: I haz the power, 2017-01-25 00:53:29]

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

[dcheng, 2017-01-25 01:08:03]

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

[commit-bot: I haz the power, 2017-01-25 01:08:39]

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

[dcheng, 2017-01-25 01:11:26]

OK, I patched it in, built, and confirmed it works. And as a bonus, it even
compiles now.

[Wez, 2017-01-25 01:57:24]

https://codereview.chromium.org/2653073002/diff/120003/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/2653073002/diff/120003/base/logging.h#newcode774
base/logging.h:774: ::logging::g_swallow_stream, val2), \
nit: Add a comment to clarify why we're doing this weird
(MakeCheckOpValueString, MCOVS, (val1)op(val2)) thing here?

[dcheng, 2017-01-25 03:11:58]

Also updated the CL description with some numbers.

https://codereview.chromium.org/2653073002/diff/120003/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/2653073002/diff/120003/base/logging.h#newcode774
base/logging.h:774: ::logging::g_swallow_stream, val2), \
On 2017/01/25 01:57:24, Wez wrote:
> nit: Add a comment to clarify why we're doing this weird
> (MakeCheckOpValueString, MCOVS, (val1)op(val2)) thing here?

Done.

[dcheng, 2017-01-25 03:14:52]

Description was changed from

==========
Make sure DCHECK compiles out completely if DCHECKS aren't enabled.

The MSVC optimizer can get confused when expressions that create
temporaries are completely optimized out.

BUG=684105
R=scottmg@chromium.org
==========

to

==========
Make sure DCHECK compiles out completely if DCHECKS aren't enabled.

The MSVC optimizer can get confused when expressions that create
temporaries are completely optimized out.

Before:
2017-01-24  05:59 PM         1,139,712 chrome.exe
2017-01-24  05:53 PM        48,618,496 chrome.dll
2017-01-24  05:59 PM        68,739,584 chrome_child.dll

After:
2017-01-24  06:51 PM         1,136,128 chrome.exe
2017-01-24  06:45 PM        48,265,216 chrome.dll
2017-01-24  06:51 PM        68,245,504 chrome_child.dll

BUG=684105
R=scottmg@chromium.org
==========

[dcheng, 2017-01-25 03:17:39]

Description was changed from

==========
Make sure DCHECK compiles out completely if DCHECKS aren't enabled.

The MSVC optimizer can get confused when expressions that create
temporaries are completely optimized out.

Before:
2017-01-24  05:59 PM         1,139,712 chrome.exe
2017-01-24  05:53 PM        48,618,496 chrome.dll
2017-01-24  05:59 PM        68,739,584 chrome_child.dll

After:
2017-01-24  06:51 PM         1,136,128 chrome.exe
2017-01-24  06:45 PM        48,265,216 chrome.dll
2017-01-24  06:51 PM        68,245,504 chrome_child.dll

BUG=684105
R=scottmg@chromium.org
==========

to

==========
Make sure DCHECK compiles out completely if DCHECKS aren't enabled.

The MSVC optimizer can get confused when expressions that create
temporaries are completely optimized out.

Before:
2017-01-24  05:59 PM         1,139,712 chrome.exe
2017-01-24  05:53 PM        48,618,496 chrome.dll
2017-01-24  05:59 PM        68,739,584 chrome_child.dll

After:
2017-01-24  06:51 PM         1,136,128 chrome.exe
2017-01-24  06:45 PM        48,265,216 chrome.dll
2017-01-24  06:51 PM        68,245,504 chrome_child.dll

This results in a total savings of ~830KB in a mostly official build.
These numbers don't include the effect of full_wpo_on_official=true,
but it's been confirmed that this codegen bug happens either way.

BUG=684105
R=scottmg@chromium.org
==========

[dcheng, 2017-01-25 04:59:38]

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

[commit-bot: I haz the power, 2017-01-25 05:00:00]

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, 2017-01-25 07:19:52]

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

[commit-bot: I haz the power, 2017-01-25 07:19:53]

Dry run: This issue passed the CQ dry run.

[danakj, 2017-01-25 16:03:42]

LGTM

https://codereview.chromium.org/2653073002/diff/160001/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/2653073002/diff/160001/base/logging.h#newcode735
base/logging.h:735: #if DCHECK_IS_ON()
can you elif instead of else\nif?

[dcheng, 2017-01-25 17:42:53]

https://codereview.chromium.org/2653073002/diff/160001/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/2653073002/diff/160001/base/logging.h#newcode735
base/logging.h:735: #if DCHECK_IS_ON()
On 2017/01/25 16:03:42, danakj (slow) wrote:
> can you elif instead of else\nif?

We discussed offline and I'll leave it as nested #ifs for now.

[dcheng, 2017-01-25 17:43:02]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2017-01-25 17:43:02]

The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org,
scottmg@chromium.org
Link to the patchset: https://codereview.chromium.org/2653073002/#ps160001
(title: "commented")

[commit-bot: I haz the power, 2017-01-25 17:43:42]

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, 2017-01-25 17:48:47]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1485366182216040,
"parent_rev": "43117a2e3860fed908e46bed85624b17358fe63b", "commit_rev":
"fc670f47ae174fb59497fe5d3315b10cc7601cad"}

[commit-bot: I haz the power, 2017-01-25 17:49:20]

Description was changed from

==========
Make sure DCHECK compiles out completely if DCHECKS aren't enabled.

The MSVC optimizer can get confused when expressions that create
temporaries are completely optimized out.

Before:
2017-01-24  05:59 PM         1,139,712 chrome.exe
2017-01-24  05:53 PM        48,618,496 chrome.dll
2017-01-24  05:59 PM        68,739,584 chrome_child.dll

After:
2017-01-24  06:51 PM         1,136,128 chrome.exe
2017-01-24  06:45 PM        48,265,216 chrome.dll
2017-01-24  06:51 PM        68,245,504 chrome_child.dll

This results in a total savings of ~830KB in a mostly official build.
These numbers don't include the effect of full_wpo_on_official=true,
but it's been confirmed that this codegen bug happens either way.

BUG=684105
R=scottmg@chromium.org
==========

to

==========
Make sure DCHECK compiles out completely if DCHECKS aren't enabled.

The MSVC optimizer can get confused when expressions that create
temporaries are completely optimized out.

Before:
2017-01-24  05:59 PM         1,139,712 chrome.exe
2017-01-24  05:53 PM        48,618,496 chrome.dll
2017-01-24  05:59 PM        68,739,584 chrome_child.dll

After:
2017-01-24  06:51 PM         1,136,128 chrome.exe
2017-01-24  06:45 PM        48,265,216 chrome.dll
2017-01-24  06:51 PM        68,245,504 chrome_child.dll

This results in a total savings of ~830KB in a mostly official build.
These numbers don't include the effect of full_wpo_on_official=true,
but it's been confirmed that this codegen bug happens either way.

BUG=684105
R=scottmg@chromium.org

Review-Url: https://codereview.chromium.org/2653073002
Cr-Commit-Position: refs/heads/master@{#446054}
Committed:
https://chromium.googlesource.com/chromium/src/+/fc670f47ae174fb59497fe5d3315...
==========

[commit-bot: I haz the power, 2017-01-25 17:49:25]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/fc670f47ae174fb59497fe5d3315...

[scottmg, 2017-01-25 18:19:27]

[ FWIW, I experienced a 7M discrepancy between full_wpo_on_official=true and
=false in https://bugs.chromium.org/p/chromium/issues/detail?id=581766 . So
unfortunately in general, it's not really worth doing size comparisons without
it on. ]