Disable Blink assertions on Chromecast device builds

Disable Blink assertions on Chromecast device eng builds

Chromecast device eng builds are release builds with dcheck_always_on
to take advantage of DCHECKS.  By default this also enables Blink
assertions, which add ~20% to our binary size.  Recently the eng OTA
package has become too large to push to the device, so space savings
are required, and the Blink assertions haven't provided a lot of value
historically.  They are still enabled for buildbots, desktop builds and
Android TV.

BUG=internal b/26142354

Committed: https://crrev.com/25419b09eee2221bde1ac0320e29ca32026a1f3e
Cr-Commit-Position: refs/heads/master@{#367363}

[slan, 2015-12-17 02:44:12]

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

[slan, 2015-12-17 02:44:13]

https://codereview.chromium.org/1535633002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/wtf/BUILD.gn (right):

https://codereview.chromium.org/1535633002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/wtf/BUILD.gn:24: defines = [
This needs to be defines += or GN will yell at you on Win builds.

https://codereview.chromium.org/1535633002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/wtf/BUILD.gn:50: defines += [ "ENABLE_ASSERT=0" ]
Any target that has this macro in *.h files should include this as a
public_config. Any target which has this macro in just *.cc files should include
it as a config.

[halliwell, 2015-12-17 20:37:07]

Description was changed from

==========
Disable WTF assertions on Chromecast device builds

Assertions in Blink, when enabled, add 20% to cast_shell binary size,
making our OTA package too large to push to a device for developer
builds.  We still need DCHECKs, so need a way to enable those without
Blink asserts.

BUG=
==========

to

==========
Disable Blink assertions on Chromecast device eng builds

Chromecast device eng builds are release builds with dcheck_always_on
to take advantage of DCHECKS.  By default this also enables Blink
assertions, which add ~20% to our binary size.  Recently the eng OTA
package has become too large to push to the device, so space savings
are required, and the Blink assertions haven't provided a lot of value
historically.  They are still enabled for buildbots, desktop builds and
Android TV.

BUG=internal b/26142354
==========

[halliwell, 2015-12-17 20:38:44]

halliwell@chromium.org changed reviewers:
+ esprehn@chromium.org, thakis@chromium.org

[slan, 2015-12-17 23:25:59]

This looks pretty good to me. I think the GN logic would ideally be expressed
best as: if enable_assertions is defined in args.gn, use that value. Otherwise,
use the value of (!NDEBUG or DCHECK_ALWAYS_ON). Unfortunately, GN does not have
a super-clean mechanism of checking whether an arg has been defined in args.gn,
so this isn't a favorable solution.

lgtm

[esprehn, 2015-12-18 00:22:37]

lgtm, as long as the bots running the tests have them turned on this seems fine
for eng builds.

[halliwell, 2015-12-29 18:13:56]

On 2015/12/18 00:22:37, esprehn wrote:
> lgtm, as long as the bots running the tests have them turned on this seems
fine
> for eng builds.

Nico: friendly reminder on this :)

[Nico, 2015-12-29 18:38:50]

Would it be an option to include fewer strings in DCHECKs and blink assertions
in cast builds instead?

[halliwell, 2015-12-29 21:33:25]

On 2015/12/29 18:38:50, Nico wrote:
> Would it be an option to include fewer strings in DCHECKs and blink assertions
> in cast builds instead?

Seems like all the strings in DCHECKs are important (file,line,assertion).

We could live without the function name in blink assertions.  That appears to
give about 10% saving, which would probably be enough in the short term, but the
20% saving in this change gives us more breathing room.

What do you think?

[Nico, 2015-12-29 21:41:00]

Sounds simpler to me. Elliot, do you think we could unconditionally stop using
WTF_PRETTY_FUNCTION in WTF_ASSERT? Having a demangled function name when you
already have file name and line number doesn't seem suuuuuper useful; Chromium
seems to be able to live without that, and it'd probably make linking a bit
faster too.

[esprehn, 2015-12-29 22:31:04]

That means anything looking at asserts needs to do a line number, file and
revision lookup to know the function.

Will it still contain the ASSERT expression?

That also means a release with asserts build and a release assert are both
much less useful. I actually find chromium infuriating to debug because it
never has symbols on linux for stacks, asserts on the bots are often
difficult to figure out and release asserts in the wild don't tell you
much...

I'd rather chromium adopt the function names like blink. :)

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

[esprehn, 2015-12-29 22:31:07]

That means anything looking at asserts needs to do a line number, file and
revision lookup to know the function.

Will it still contain the ASSERT expression?

That also means a release with asserts build and a release assert are both
much less useful. I actually find chromium infuriating to debug because it
never has symbols on linux for stacks, asserts on the bots are often
difficult to figure out and release asserts in the wild don't tell you
much...

I'd rather chromium adopt the function names like blink. :)

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

[Nico, 2015-12-29 22:35:34]

Yes, it'll still contain the ASSERT expression.

You need to look at the code anyway to debug, no?

On Tue, Dec 29, 2015 at 5:31 PM, Elliott Sprehn <esprehn@chromium.org>
wrote:

> That means anything looking at asserts needs to do a line number, file and
> revision lookup to know the function.
>
> Will it still contain the ASSERT expression?
>
> That also means a release with asserts build and a release assert are both
> much less useful. I actually find chromium infuriating to debug because it
> never has symbols on linux for stacks, asserts on the bots are often
> difficult to figure out and release asserts in the wild don't tell you
> much...
>
> I'd rather chromium adopt the function names like blink. :)
>

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

[Nico, 2015-12-29 22:35:35]

Yes, it'll still contain the ASSERT expression.

You need to look at the code anyway to debug, no?

On Tue, Dec 29, 2015 at 5:31 PM, Elliott Sprehn <esprehn@chromium.org>
wrote:

> That means anything looking at asserts needs to do a line number, file and
> revision lookup to know the function.
>
> Will it still contain the ASSERT expression?
>
> That also means a release with asserts build and a release assert are both
> much less useful. I actually find chromium infuriating to debug because it
> never has symbols on linux for stacks, asserts on the bots are often
> difficult to figure out and release asserts in the wild don't tell you
> much...
>
> I'd rather chromium adopt the function names like blink. :)
>

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

[esprehn, 2015-12-29 22:40:36]

Not if the person is filing bugs about flaky crashes on bots or someone
sees a crash in the wild.

Anyway I'd rather we not mess with the blink output unless we're aliasing
to CHECK and DCHECK.

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

[esprehn, 2015-12-29 22:40:36]

Not if the person is filing bugs about flaky crashes on bots or someone
sees a crash in the wild.

Anyway I'd rather we not mess with the blink output unless we're aliasing
to CHECK and DCHECK.

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

[halliwell, 2016-01-04 17:20:55]

On 2015/12/29 22:40:36, esprehn wrote:
> Not if the person is filing bugs about flaky crashes on bots or someone
> sees a crash in the wild.
> 
> Anyway I'd rather we not mess with the blink output unless we're aliasing
> to CHECK and DCHECK.
> 
> -- 
> 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.

Nico: just wanted to bump this thread again post holidays, thoughts on eprehn's
last comment?

[Nico, 2016-01-04 17:21:43]

lgtm

[halliwell, 2016-01-04 17:51:55]

The CQ bit was checked by halliwell@chromium.org

[halliwell, 2016-01-04 17:51:56]

The patchset sent to the CQ was uploaded after l-g-t-m from slan@chromium.org,
esprehn@chromium.org, thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/1535633002/#ps60001
(title: "Rebase")

[commit-bot: I haz the power, 2016-01-04 17:52:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1535633002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1535633002/60001

[commit-bot: I haz the power, 2016-01-04 21:05:41]

Description was changed from

==========
Disable Blink assertions on Chromecast device eng builds

Chromecast device eng builds are release builds with dcheck_always_on
to take advantage of DCHECKS.  By default this also enables Blink
assertions, which add ~20% to our binary size.  Recently the eng OTA
package has become too large to push to the device, so space savings
are required, and the Blink assertions haven't provided a lot of value
historically.  They are still enabled for buildbots, desktop builds and
Android TV.

BUG=internal b/26142354
==========

to

==========
Disable Blink assertions on Chromecast device eng builds

Chromecast device eng builds are release builds with dcheck_always_on
to take advantage of DCHECKS.  By default this also enables Blink
assertions, which add ~20% to our binary size.  Recently the eng OTA
package has become too large to push to the device, so space savings
are required, and the Blink assertions haven't provided a lot of value
historically.  They are still enabled for buildbots, desktop builds and
Android TV.

BUG=internal b/26142354
==========

[commit-bot: I haz the power, 2016-01-04 21:05:42]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-01-04 21:06:59]

Description was changed from

==========
Disable Blink assertions on Chromecast device eng builds

Chromecast device eng builds are release builds with dcheck_always_on
to take advantage of DCHECKS.  By default this also enables Blink
assertions, which add ~20% to our binary size.  Recently the eng OTA
package has become too large to push to the device, so space savings
are required, and the Blink assertions haven't provided a lot of value
historically.  They are still enabled for buildbots, desktop builds and
Android TV.

BUG=internal b/26142354
==========

to

==========
Disable Blink assertions on Chromecast device eng builds

Chromecast device eng builds are release builds with dcheck_always_on
to take advantage of DCHECKS.  By default this also enables Blink
assertions, which add ~20% to our binary size.  Recently the eng OTA
package has become too large to push to the device, so space savings
are required, and the Blink assertions haven't provided a lot of value
historically.  They are still enabled for buildbots, desktop builds and
Android TV.

BUG=internal b/26142354

Committed: https://crrev.com/25419b09eee2221bde1ac0320e29ca32026a1f3e
Cr-Commit-Position: refs/heads/master@{#367363}
==========

[commit-bot: I haz the power, 2016-01-04 21:06:59]

Patchset 4 (id:??) landed as
https://crrev.com/25419b09eee2221bde1ac0320e29ca32026a1f3e
Cr-Commit-Position: refs/heads/master@{#367363}