Make code generated for histogram macros more compact.

Make code generated for histogram macros more compact.

In builds where DCHECKs are compiled in (e.g. developer
builds and Chrome Canaries), the DCHECK_EQ() in the
histogram macro was expanding to a lot of generated
machine code.

This CL moves that DCHECK() to a function instead, so that the
code is not duplicated in every call site. The function call is
wrapped behind a DCHECK_IS_ON() conditional, so that it will
still be omitted if DCHECKs are not compiled into the build.

Makes my Chromium release build smaller by over 1MB,
measuring size of Chromium Framework.

Before size: 146547380
After size:  145400692

BUG=343946
TEST=No functional changes.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=251486

[Ilya Sherman, 2014-02-14 21:45:38]

Nice.  LGTM, thanks.

[Ilya Sherman, 2014-02-14 21:45:43]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2014-02-14 21:51:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asvitkine@chromium.org/167343002/60001

[jar (doing other things), 2014-02-15 00:50:42]

https://codereview.chromium.org/167343002/diff/60001/base/metrics/histogram.h
File base/metrics/histogram.h (right):

https://codereview.chromium.org/167343002/diff/60001/base/metrics/histogram.h...
base/metrics/histogram.h:158: if (DCHECK_IS_ON()) \
Am I correct, that this is an effort to reduce the size of the debug build? 
...and it has no impact on production builds?

[Alexei Svitkine (slow), 2014-02-15 01:04:52]

See my CL description. It affects developer release builds too, and any
builds where dchecks are compiled in (e.g. I think they are compiled in on
canaries, but disabled by default).

There's still other room for improvement here for sure, but this is the
first thing that jumped out in my release build.
On 14 Feb 2014 19:50, <jar@chromium.org> wrote:

>
> https://codereview.chromium.org/167343002/diff/60001/base/
> metrics/histogram.h
> File base/metrics/histogram.h (right):
>
> https://codereview.chromium.org/167343002/diff/60001/base/
> metrics/histogram.h#newcode158
> base/metrics/histogram.h:158: if (DCHECK_IS_ON()) \
> Am I correct, that this is an effort to reduce the size of the debug
> build?  ...and it has no impact on production builds?
>
> https://codereview.chromium.org/167343002/
>

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

[commit-bot: I haz the power, 2014-02-15 03:11:50]

Change committed as 251486

[jar (doing other things), 2014-02-15 06:32:57]

FWIW I'm pretty sure we don't currently ever ship with DCHECKs compiled in,
but disabled.  I think it was experimented with, but abandoned.


On Fri, Feb 14, 2014 at 5:04 PM, Alexei Svitkine <asvitkine@chromium.org>wrote:

> See my CL description. It affects developer release builds too, and any
> builds where dchecks are compiled in (e.g. I think they are compiled in on
> canaries, but disabled by default).
>
> There's still other room for improvement here for sure, but this is the
> first thing that jumped out in my release build.
> On 14 Feb 2014 19:50, <jar@chromium.org> wrote:
>
>>
>> https://codereview.chromium.org/167343002/diff/60001/base/
>> metrics/histogram.h
>> File base/metrics/histogram.h (right):
>>
>> https://codereview.chromium.org/167343002/diff/60001/base/
>> metrics/histogram.h#newcode158
>> base/metrics/histogram.h:158: if (DCHECK_IS_ON()) \
>> Am I correct, that this is an effort to reduce the size of the debug
>> build?  ...and it has no impact on production builds?
>>
>> https://codereview.chromium.org/167343002/
>>
>

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

[asvitkine_google, 2014-02-15 14:56:53]

Seems you're right - I must have remembered wrong. (Interestingly, this is
different than NOTREACHED(), which I saw my canary hit and spew some
output.)

Still, shaving >1MB off non-official builds probably has tangible benefits
in build times for developers and our bots.

(Incidentally, here's the result of this on our perf bots:
http://build.chromium.org/f/chromium/perf/mac-release/sizes/report.html?histo....
a similar effect is seen on other platforms as well.)


On Sat, Feb 15, 2014 at 1:32 AM, Jim Roskind <jar@chromium.org> wrote:

> FWIW I'm pretty sure we don't currently ever ship with DCHECKs compiled
> in, but disabled.  I think it was experimented with, but abandoned.
>
>
> On Fri, Feb 14, 2014 at 5:04 PM, Alexei Svitkine
<asvitkine@chromium.org>wrote:
>
>> See my CL description. It affects developer release builds too, and any
>> builds where dchecks are compiled in (e.g. I think they are compiled in on
>> canaries, but disabled by default).
>>
>> There's still other room for improvement here for sure, but this is the
>> first thing that jumped out in my release build.
>> On 14 Feb 2014 19:50, <jar@chromium.org> wrote:
>>
>>>
>>> https://codereview.chromium.org/167343002/diff/60001/base/
>>> metrics/histogram.h
>>> File base/metrics/histogram.h (right):
>>>
>>> https://codereview.chromium.org/167343002/diff/60001/base/
>>> metrics/histogram.h#newcode158
>>> base/metrics/histogram.h:158: if (DCHECK_IS_ON()) \
>>> Am I correct, that this is an effort to reduce the size of the debug
>>> build?  ...and it has no impact on production builds?
>>>
>>> https://codereview.chromium.org/167343002/
>>>
>>
>

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

[asvitkine_google, 2014-02-15 15:00:26]

On Sat, Feb 15, 2014 at 9:56 AM, Alexei Svitkine <asvitkine@google.com>wrote:

> Interestingly, this is different than NOTREACHED(), which I saw my canary
> hit and spew some output.
>

(Sorry, I meant NOTIMPLEMENTED() in that case. Not sure about NOTREACHED().)

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