Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1937613002:
Stop compiling and printing logging strings for CHECK() on Android. (Closed)
DescriptionStop compiling and printing logging strings for CHECK() on Android.
This is a performance regression in general, and causes visible
regression when Blink used CHECK instead of their own RELEASE_ASSERT.
This behaviour was introduced in https://codereview.chromium.org/336413005
If Android continues to need something special here (radio silence
from previously-interested parties for the last few weeks), we should
print __FILE__ and __LINE__ or something, and not support the full
string logging that this patch reverts.
R=haraken, thakis@chromium.org
BUG=599867, 596760, 378974
Committed: https://crrev.com/b9d5931295b7eab7bd2f029b8d896082b911d776
Cr-Commit-Position: refs/heads/master@{#391613}
Patch Set 1 #Patch Set 2 : android-check: . #Messages
Total messages: 50 (17 generated)
The CQ bit was checked by danakj@chromium.org to run a CQ dry run
Grace, please shout if you think this will cause issues. I think this lgtm. (Dana, maybe you can link to the CL that added this in the CL description of this change.) Thanks!
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1937613002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1937613002/20001
Description was changed from ========== Stop compiling and printing logging strings for CHECK() on Android. This is a performance regression in general, and causes visible regression when Blink used CHECK instead of their own RELEASE_ASSERT. If Android continues to need something special here (radio silence from previously-interested parties for the last few weeks), we should print __FILE__ and __LINE__ or something, and not support the full string logging that this patch reverts. R=haraken, thakis@chromium.org BUG=599867,596760 ========== to ========== Stop compiling and printing logging strings for CHECK() on Android. This is a performance regression in general, and causes visible regression when Blink used CHECK instead of their own RELEASE_ASSERT. This behaviour was introduced in https://codereview.chromium.org/336413005 If Android continues to need something special here (radio silence from previously-interested parties for the last few weeks), we should print __FILE__ and __LINE__ or something, and not support the full string logging that this patch reverts. R=haraken, thakis@chromium.org BUG=599867,596760, 378974 ==========
Done, and added that bug to the BUG= too.
On 2016/04/29 21:10:47, commit-bot: I haz the power wrote: > Dry run: CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/1937613002/20001 > View timeline at > https://chromium-cq-status.appspot.com/patch-timeline/1937613002/20001 sorry for not chiming in earlier but this may make some of our stability efforts more difficult. A lot of our stability work is improved by getting logcat alongside the crash and there the CHECK message can theoretically help. I'm not sure how helpful the actual log message is but I can go back through a few crashes and see how relevant it's been. The specific case I'm worried about is one that is part of our triage instructions in jni_android.cc - base::android::CheckException(). Of course, we could do a targeted fix there. Can I ask to hold for one-two days and get back to you early next week while I review some recent triage work?
On 2016/04/29 21:21:28, Yaron wrote: > On 2016/04/29 21:10:47, commit-bot: I haz the power wrote: > > Dry run: CQ is trying da patch. Follow status at > > https://chromium-cq-status.appspot.com/patch-status/1937613002/20001 > > View timeline at > > https://chromium-cq-status.appspot.com/patch-timeline/1937613002/20001 > > sorry for not chiming in earlier but this may make some of our stability efforts > more difficult. A lot of our stability work is improved by getting logcat > alongside the crash and there the CHECK message can theoretically help. I'm not > sure how helpful the actual log message is but I can go back through a few > crashes and see how relevant it's been. The specific case I'm worried about is > one that is part of our triage instructions in jni_android.cc - > base::android::CheckException(). Of course, we could do a targeted fix there. > > Can I ask to hold for one-two days and get back to you early next week while I > review some recent triage work? Sure, I'll ping again early next week for updates. Thanks for replying.
yfriedman@chromium.org changed reviewers: + torne@chromium.org, yfriedman@chromium.org
+torne for thoughts and whether this affects webview
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ping :) Anything to add?
Honestly, I'm a bit torn but probably ok with it. I'd like to reap the benefits of standardizing with other platforms and getting rid of the strings but looks like someone employed this technique as recently as last week: https://codereview.chromium.org/1914983002/ Is it common knowledge that CHECK strings are discarded on other platforms? Perhaps Android dev is unique in this regard in that developers are used to have a meaningful logcat with crashes to aid debugging and this would break that expectation? Certainly, we'd have to announce this change. One mitigating factor is that Feng's CL predates our inclusion of microdumps in logs which addresses Nico's justification for allowing his CL in the first place here: https://codereview.chromium.org/336413005 As a way of naively checking impact I tried: $ git grep "[^D]CHECK(.*<<" | grep android Of course, that's not multi-line so I'm possibly severely under counting. Scanning the list - I think it'd be helpful to explicitly log the three instances in jni_android. The others shouldn't be an issue. So long story short, I am concerned about this making some OEM-specific problems which we only discover on stable harder to pin down but there are mitigating factors. It's worth noting that we only get signal for these kinds of issues on Stable and re-spinning on Android is much more costly than other patforms, especially for webview. If torne is ok with this for webview then I guess I'm ok with it. On Mon, May 2, 2016 at 4:44 PM <danakj@chromium.org> wrote: > ping :) Anything to add? > > https://codereview.chromium.org/1937613002/ > -- 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.
On 2016/05/02 21:38:00, Yaron wrote: > Honestly, I'm a bit torn but probably ok with it. I'd like to reap the > benefits of standardizing with other platforms and getting rid of the > strings but looks like someone employed this technique as recently as last > week: https://codereview.chromium.org/1914983002/ > > Is it common knowledge that CHECK strings are discarded on other platforms? > Perhaps Android dev is unique in this regard in that developers are used to > have a meaningful logcat with crashes to aid debugging and this would break > that expectation? Certainly, we'd have to announce this change. > > One mitigating factor is that Feng's CL predates our inclusion of > microdumps in logs which addresses Nico's justification for allowing his CL > in the first place here: https://codereview.chromium.org/336413005 > > As a way of naively checking impact I tried: > $ git grep "[^D]CHECK(.*<<" | grep android > > Of course, that's not multi-line so I'm possibly severely under counting. > > Scanning the list - I think it'd be helpful to explicitly log the three > instances in jni_android. The others shouldn't be an issue. > > So long story short, I am concerned about this making some OEM-specific > problems which we only discover on stable harder to pin down but there are > mitigating factors. It's worth noting that we only get signal for these > kinds of issues on Stable and re-spinning on Android is much more costly > than other patforms, especially for webview. If torne is ok with this for > webview then I guess I'm ok with it. There's a webview-specific problem here, which is that webview crash reports *don't come with logcat output*, and so the *only* information we get is: 1) A microdump, if it managed to get produced and included (it works ~85% of the time?) 2) The message passed to abort(), which currently includes the CHECK() << "message", but wouldn't any more after this change. Printing errors to the log explicitly means that WebView will *never see them* in the field, unfortunately. We do usually have microdumps as Yaron mentioned, which are often more useful than the CHECK message, but sometimes there's information in the CHECK message which can't be deduced from the stack. WebView, like chrome on android, has to debug any non-locally-reproducible problem in the field using official release builds, and only gets significant signal on the stable channel, just as Yaron mentions. I think we could live with this being removed, because it's true that we don't normally need them, but they are sometimes useful. I'm hesitant to propose that we have a NOISYCHECK() or something that doesn't have its messages stripped for cases where we really, really want it, but that might help?
Hm. If you do LOG(FATAL) << "foo"; does the "foo" end up in the abort message on android? If so then any case where we really need the output we could just use a regular conditional around a LOG(FATAL) instead of CHECK().
On 2016/05/03 13:08:28, Torne wrote: > Hm. If you do LOG(FATAL) << "foo"; does the "foo" end up in the abort message on > android? If so then any case where we really need the output we could just use a > regular conditional around a LOG(FATAL) instead of CHECK(). LOG(ERROR) goes to log cat right? LOG(FATAL) should too. It sounds like this isn't something we maintain and we can work around other things if needed. LOG() should probably be enough. Thanks! I'm going to land this to unblock blink folks from using CHECKs.
The CQ bit was checked by danakj@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1937613002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1937613002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply the patch.
The CQ bit was unchecked by commit-bot@chromium.org
The CQ bit was checked by danakj@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1937613002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1937613002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply the patch.
The CQ bit was unchecked by commit-bot@chromium.org
The CQ bit was checked by danakj@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1937613002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1937613002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply the patch.
The CQ bit was unchecked by commit-bot@chromium.org
klobag@chromium.org changed reviewers: + klobag@chromium.org
Sorry to chime in late. We don't want the string because of the size. Can we print out the file name and line number?
On Tue, May 3, 2016 at 6:19 PM, <klobag@chromium.org> wrote: > Sorry to chime in late. > > We don't want the string because of the size. Can we print out the file > name and > line number? > I think that it's possible, it may still be a perf regression as I didn't try investigate it. But it sounds like LOG(FATAL) does everything we need, so there's no need to diverge on Android here, or do you have other concerns with that? -- 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.
On 2016/05/04 01:22:23, danakj wrote: > On Tue, May 3, 2016 at 6:19 PM, <mailto:klobag@chromium.org> wrote: > > > Sorry to chime in late. > > > > We don't want the string because of the size. Can we print out the file > > name and > > line number? > > > > I think that it's possible, it may still be a perf regression as I didn't > try investigate it. (Changing RELEASE_ASSERT in blink to CHECK was a performance hit, not just size, due to the strings.) > But it sounds like LOG(FATAL) does everything we need, so there's no need > to diverge on Android here, or do you have other concerns with that?
We only need to get filename and line number right before we crash. Why it cause performance regression?
I don't think this is the correct fix, we need regular release builds (the ones we're profiling locally and perf testing on the bots) to be fast, not just official builds.
I'll follow up a bit more on the bug (http://crbug.com/599867), but just quickly: On 2016/05/04 01:22:23, danakj wrote: > On Tue, May 3, 2016 at 6:19 PM, <mailto:klobag@chromium.org> wrote: > > > Sorry to chime in late. > > > > We don't want the string because of the size. Can we print out the file > > name and > > line number? I'm not sure printing the file name and line number is actually useful to Android at this point. Our crash pipeline is now pretty mature and it should now be rare that we can't symbolise the crashes correctly. The only reason to keep Android's nonstandard behaviour is if we think the error message is going to be useful often enough to pay the binary size cost - I don't know what that cost actually is, offhand. > I think that it's possible, it may still be a perf regression as I didn't > try investigate it. My quick look at the bug suggests it will still be a perf regression, though maybe a smaller one. > But it sounds like LOG(FATAL) does everything we need, so there's no need > to diverge on Android here, or do you have other concerns with that? I tested just to be sure and replacing: CHECK(condition) << "message"; with: if (condition) LOG(FATAL) << "condition failed, message"; does indeed do the right thing on Android (for both chrome and webview) - the fatal log message is passed to libc and appears in debuggerd output, not just in logcat, which is what we'd want. This means that if we really need a particular message for in-the-field debugging, we can accomplish this with LOG(FATAL), but it still only applies to the cases where we actually use that, and we won't get any details for CHECK().
On 2016/05/04 05:28:17, esprehn wrote: > I don't think this is the correct fix, we need regular release builds (the ones > we're profiling locally and perf testing on the bots) The perf bots all run official builds, and everyone working on performance should too. Optimization settings are different, and if you're profiling with a non-official build you're profiling a config that's different from what we ship. Torne's comment on the bug is interesting though.
The CQ bit was checked by danakj@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1937613002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1937613002/20001
On 2016/05/04 14:43:45, Nico (hiding Wed May 4) wrote: > On 2016/05/04 05:28:17, esprehn wrote: > > I don't think this is the correct fix, we need regular release builds (the > ones > > we're profiling locally and perf testing on the bots) > > The perf bots all run official builds, and everyone working on performance > should too. Optimization settings are different, and if you're profiling with a > non-official build you're profiling a config that's different from what we ship. +1. Always official build if you are looking at performance. > Torne's comment on the bug is interesting though. I think he's looking at the analysis of what we're removing here.
Message was sent while issue was closed.
Description was changed from ========== Stop compiling and printing logging strings for CHECK() on Android. This is a performance regression in general, and causes visible regression when Blink used CHECK instead of their own RELEASE_ASSERT. This behaviour was introduced in https://codereview.chromium.org/336413005 If Android continues to need something special here (radio silence from previously-interested parties for the last few weeks), we should print __FILE__ and __LINE__ or something, and not support the full string logging that this patch reverts. R=haraken, thakis@chromium.org BUG=599867,596760, 378974 ========== to ========== Stop compiling and printing logging strings for CHECK() on Android. This is a performance regression in general, and causes visible regression when Blink used CHECK instead of their own RELEASE_ASSERT. This behaviour was introduced in https://codereview.chromium.org/336413005 If Android continues to need something special here (radio silence from previously-interested parties for the last few weeks), we should print __FILE__ and __LINE__ or something, and not support the full string logging that this patch reverts. R=haraken, thakis@chromium.org BUG=599867,596760, 378974 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Stop compiling and printing logging strings for CHECK() on Android. This is a performance regression in general, and causes visible regression when Blink used CHECK instead of their own RELEASE_ASSERT. This behaviour was introduced in https://codereview.chromium.org/336413005 If Android continues to need something special here (radio silence from previously-interested parties for the last few weeks), we should print __FILE__ and __LINE__ or something, and not support the full string logging that this patch reverts. R=haraken, thakis@chromium.org BUG=599867,596760, 378974 ========== to ========== Stop compiling and printing logging strings for CHECK() on Android. This is a performance regression in general, and causes visible regression when Blink used CHECK instead of their own RELEASE_ASSERT. This behaviour was introduced in https://codereview.chromium.org/336413005 If Android continues to need something special here (radio silence from previously-interested parties for the last few weeks), we should print __FILE__ and __LINE__ or something, and not support the full string logging that this patch reverts. R=haraken, thakis@chromium.org BUG=599867,596760, 378974 Committed: https://crrev.com/b9d5931295b7eab7bd2f029b8d896082b911d776 Cr-Commit-Position: refs/heads/master@{#391613} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/b9d5931295b7eab7bd2f029b8d896082b911d776 Cr-Commit-Position: refs/heads/master@{#391613}
Message was sent while issue was closed.
On a local official build this change saves 252KiB of binary size for arm32, a 0.6% reduction. It presumably also has some performance benefit, but I haven't tried to measure that.
Message was sent while issue was closed.
Thanks all for working on this! The performance regression is gone with this CL, so can we now start using CHECK in the Blink code base?
Message was sent while issue was closed.
On 2016/05/20 05:27:09, haraken wrote: > Thanks all for working on this! > > The performance regression is gone with this CL, so can we now start using CHECK > in the Blink code base? Nico also did https://codereview.chromium.org/1982123002/ so they should really be equivilent now. SGTM |
