All graphs for this bug:
  https://chromeperf.appspot.com/group_report?bug_id=832678

(For debugging:) Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?sid=bd2844166fc28c03131d5c8e2043b6feb59c9ee7b9abf2155500392806159d74


Bot(s) for this bug's original alert(s):

Android Builder Perf

Assigning to jdoerrie@chromium.org because this is the only CL in range:
[base] Fail CHECK when dereferencing an empty optional

This change promotes the DCHECKs in base::Optional::value() to CHECKs
in order to match std::optional more closely. Furthermore, it also
CHECKs when dereferencing a base::Optional via operator* or operator->.

Bug: 817982
Change-Id: Ib2bfdd3a863e8bade21fa1b6b1c10c7c4d2ca135
Reviewed-on: https://chromium-review.googlesource.com/997335
Commit-Queue: Jan Wilken Dörrie <jdoerrie@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550197}

See attached diff.

Not sure if there's anything we can do to avoid this though (short of removing the CHECKs).. Maybe we need to revisit our CHECK implementation to see if it can get any smaller?

Deleted: diff_results.txt 144 KB

diff_results.txt 144 KB View Download

Working graph link: https://chromeperf.appspot.com/report?sid=e9a395446072c0ec9e8fb904774a84c07ad868b552721960a3e7097d7c009b7d&num_points=10&rev=550197

Looks like this is indeed my change. Re #c3: I'm not sure either, as the CHECK implementation for release builds is already quite minimal: https://codesearch.chromium.org/chromium/src/base/logging.h?l=507-600&rcl=b2a818025f27b8fbeaa1b3573c9d561b9fa18214

Apologies for the naive question, but is MonochromePublic.apk part of the official Clank build? If not, do you happen if you experience the same regression there?

Adding primiano@ and thakis@, who worked on making CHECKs smaller in the past and might be able to provide some insight.

MonochromePublic.apk is not the official apk. The official one is "Monochrome.apk". In terms of native code size, the only difference between the two is that Monochrome.apk uses a linker orderfile, so size-wise they are about the same.

Can you paste in this bug the assembly before and after the change to try to figure out what's inflating the binary and how we can avoid that?

A CHECK, from what I can see, ultimately expands to (on arm):
if !(condition) ? asm volatile("bkpt #0; udf %0;" ::"i"(__COUNTER__ % 256))

So, there are two options here:
1) something is not working as intended, and something else is expanding
2) There are just lot of calls to base::Optional::operatorXX in the cosebase and this is the cost of expanding those 3-4 instruction in thousands of call sites.

I suspect 2 is what's happening, you are paying the cost of ~4 bytes * 5900 base::Optional (As per codesearch) ~ 23 KB

If you add code to a heavily used template, binary size goes up, i.e. primiano's 2. So let's not add code to heavily used templates permanently. The CHECK probably found a few crashes which we can act on, and we can turn it on for a short while every now and then, but we probably shouldn't keep it in permanently due to binary size.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a000101f58501dac2d090ba09f61cb292f7c9f6b

commit a000101f58501dac2d090ba09f61cb292f7c9f6b
Author: jdoerrie <jdoerrie@chromium.org>
Date: Fri Jun 08 19:52:09 2018

[base] Remove CHECKs in base::Optional operator* and ->

This change partially reverts r550197 which introduced CHECKs to
base::Optional's operator*, operator-> and value(). This is done to
reduce binary size and to be more standard's compliant, as std::optional
also doesn't perform checks for operator* and operator->. Lastly, the
CHECKs in value() are kept, as here CHECKing is desired, and also
matches std::optional's behaviour.

Bug:  832678 
Change-Id: I467c7d7623c2880ee761b8a58a74738c09e0ba2a
Reviewed-on: https://chromium-review.googlesource.com/1093314
Reviewed-by: Nico Weber <thakis@chromium.org>
Commit-Queue: Jan Wilken Dörrie <jdoerrie@chromium.org>
Cr-Commit-Position: refs/heads/master@{#565720}
[modify] https://crrev.com/a000101f58501dac2d090ba09f61cb292f7c9f6b/base/optional.h

r565720 mostly reverted r550197, except for the CHECKs in base::Optional::value(), where they are deliberate. This should address most of the binary bloat introduced by the initial change. Therefore, I'm marking this as fixed now.

Would it be possible to put this change back in? There have been a number of security issues and fuzz crashes related to empty Optional instances lately, so it would be good if accidentally de-referencing an empty optional wasn't an exploitable crash. 

Eventually we'll use std::optional and that doesn't have that CHECK. What's your suggestion on what to do about that?