Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1918403004:
Add Check and DCheck java classes to base (Closed)
DescriptionAdd Check and DCheck java classes to base
These classes expose CHECK and DCHECK like functionality for Chrome's Java
code. They have static methods like those in org.junit.Assert to reduce code
written by consumers and reduce code run in Release builds. All methods
in DCheck.java are marked @RemoveableInRelase, which are treated as
assumenosideeffects by proguard, removing the calls from the Release
builds. The only code left over is the code passed into the calls. Methods
called inside the argument list will still exist unless assumenosideeffect is
placed on them. Callers can take advantage of methods like isEqual to prevent
the equality check from occurring in Release builds.
BUG=462676
Patch Set 1 #
Total comments: 3
Patch Set 2 : Addressed dgn's comments #Patch Set 3 : Changed Debugger wait to use a flag instead #
Total comments: 1
Patch Set 4 : Removed DCheck and added error message formatting #Patch Set 5 : Added use of formatted message to Check #
Total comments: 3
Patch Set 6 : Converted crashing mechanism to throw an AssertionError instead #
Messages
Total messages: 62 (10 generated)
Description was changed from ========== Add Check and DCheck java classes to base These classes expose CHECK and DCHECK like functionality for Chrome's Java code. They have static methods like those in org.junit.Assert to reduce code written by consumers and reduce code run in Release builds. o All methods in DCheck.java are marked @RemoveableInRelase, which are treated as assumenosideeffects by proguard, removing the calls from the Release builds. The only code left over is the code passed into the calls. Methods called inside the argument list will still exist unless assumenosideeffect is placed on them. Callers can take advantage of methods like isEqual to prevent the equality check from occurring in Release builds. For computationally expensive calls that want to be checked, they can wrapped in a Runnable and passed to DCheck.run() to reduce the performance impact down to an anonymous instance instantiation. BUG=462676 ========== to ========== Add Check and DCheck java classes to base These classes expose CHECK and DCHECK like functionality for Chrome's Java code. They have static methods like those in org.junit.Assert to reduce code written by consumers and reduce code run in Release builds. o All methods in DCheck.java are marked @RemoveableInRelase, which are treated as assumenosideeffects by proguard, removing the calls from the Release builds. The only code left over is the code passed into the calls. Methods called inside the argument list will still exist unless assumenosideeffect is placed on them. Callers can take advantage of methods like isEqual to prevent the equality check from occurring in Release builds. For computationally expensive calls that want to be checked, they can wrapped in a Runnable and passed to DCheck.run() to reduce the performance impact down to an anonymous instance instantiation. BUG=462676 ==========
doloffd@amazon.com changed reviewers: + thakis@chromium.org
doloffd@amazon.com changed reviewers: + jbudorick@chromium.org
Description was changed from ========== Add Check and DCheck java classes to base These classes expose CHECK and DCHECK like functionality for Chrome's Java code. They have static methods like those in org.junit.Assert to reduce code written by consumers and reduce code run in Release builds. o All methods in DCheck.java are marked @RemoveableInRelase, which are treated as assumenosideeffects by proguard, removing the calls from the Release builds. The only code left over is the code passed into the calls. Methods called inside the argument list will still exist unless assumenosideeffect is placed on them. Callers can take advantage of methods like isEqual to prevent the equality check from occurring in Release builds. For computationally expensive calls that want to be checked, they can wrapped in a Runnable and passed to DCheck.run() to reduce the performance impact down to an anonymous instance instantiation. BUG=462676 ========== to ========== Add Check and DCheck java classes to base These classes expose CHECK and DCHECK like functionality for Chrome's Java code. They have static methods like those in org.junit.Assert to reduce code written by consumers and reduce code run in Release builds. o All methods in DCheck.java are marked @RemoveableInRelase, which are treated as assumenosideeffects by proguard, removing the calls from the Release builds. The only code left over is the code passed into the calls. Methods called inside the argument list will still exist unless assumenosideeffect is placed on them. Callers can take advantage of methods like isEqual to prevent the equality check from occurring in Release builds. BUG=462676 ==========
Description was changed from ========== Add Check and DCheck java classes to base These classes expose CHECK and DCHECK like functionality for Chrome's Java code. They have static methods like those in org.junit.Assert to reduce code written by consumers and reduce code run in Release builds. o All methods in DCheck.java are marked @RemoveableInRelase, which are treated as assumenosideeffects by proguard, removing the calls from the Release builds. The only code left over is the code passed into the calls. Methods called inside the argument list will still exist unless assumenosideeffect is placed on them. Callers can take advantage of methods like isEqual to prevent the equality check from occurring in Release builds. BUG=462676 ========== to ========== Add Check and DCheck java classes to base These classes expose CHECK and DCHECK like functionality for Chrome's Java code. They have static methods like those in org.junit.Assert to reduce code written by consumers and reduce code run in Release builds. All methods in DCheck.java are marked @RemoveableInRelase, which are treated as assumenosideeffects by proguard, removing the calls from the Release builds. The only code left over is the code passed into the calls. Methods called inside the argument list will still exist unless assumenosideeffect is placed on them. Callers can take advantage of methods like isEqual to prevent the equality check from occurring in Release builds. BUG=462676 ==========
doloffd@amazon.com changed reviewers: - jbudorick@chromium.org
PTAL
dgn@chromium.org changed reviewers: + dgn@chromium.org
There is an issue with @RemovableInRelease. Proguard optimizations are disable in WebView builds, and maybe Monochrome builds too (see https://crbug.com/583143). So in the current state the DCHECKs would stay in Release builds in WebView. I think agrieve@ is looking into the proguard issue and might have more information.
I don't have access to that bug, could you give me access to it? If not, are the debug checks inside the DCheck calls not enough on their own for those builds and so the function calls are a performance concern?
On 2016/04/28 17:51:35, doloffd wrote: > I don't have access to that bug, could you give me access to it? If not, are the > debug checks inside the DCheck calls not enough on their own for those builds > and so the function calls are a performance concern? First - thanks for taking a stab at this! It's an annoying bug that asserts are not working for us right now :( I don't think we're able to unmark a bug once it's been marked private, but the takeaway is that some proguard optimizations were found to break webview, so have been disabled Given our lack-of-faith in proguard doing the right thing, our large number of existing asserts in the codebase, and the promise that jack will make them work again, we should almost certainly pursue integrating jack in our builds rather than replacing all assert statements.
Jack doesn't seem like it will solve the problem entirely, even once it is used. The three biggest advantages to using custom classes like Check and DCheck are that 1. We have a consistent way of invoking crashes in both Debug and Release builds. 2. We have a familiar way for developers to write assertions. 3. We have a centralized way of crashing in Debug builds so if certain actions should be taken during the crash, such as cleanup or custom logging, they can occur. For 1, there are numerous cases where where RuntimeExceptions are thrown, most likely with the intent of causing crashes, but could be foiled by code blindly catching Exception or Throwable. These uses crashes might not actually be desired on release builds but are expected to happen so rarely, that it was deemed acceptable to throw them Examples of times where RuntimeExceptions are used to cause crashes in release builds: //base/android/java/src/org/chromium/base/ContextUtils.java //components/cronet/android/java/src/org/chromium/net/CronetLibraryLoader.java //chrome/android/java/src/org/chromium/chrome/browser/preferences/Preferences.java For 2, Java developers are familiar with JUnit tests and writing assertions in the format of the static methods org.junit.Assert. It also familiar for Chromium developers that have written assertion in C++ code where it is common to use DCHECK and CHECK macros for making assertions.
thakis@chromium.org changed reviewers: + yfriedman@chromium.org
adding a base/android/OWNER
Sure, having this class is a good idea to standardize the checks across the native and java code, and all the advantages you cited. But so far, we know that DCHECKs would stay enabled at least in WebView, even in Release builds. And using proguard is not as reliable as c++ preprocessor conditionals. Devs will have to be extra careful about how they write the checks. So you might want to make sure everything works as intended with WebView and Monochrome before landing this. Currently I doubt it does. On Tue, 3 May 2016, 01:14 , <thakis@chromium.org> wrote: > adding a base/android/OWNER > > https://codereview.chromium.org/1918403004/ > -- 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.
> But so far, we know that DCHECKs would stay enabled at least in WebView, > even in Release builds. And using proguard is not as reliable as c++ > preprocessor conditionals. Devs will have to be extra careful about how > they write the checks. > > So you might want to make sure everything works as intended with WebView > and Monochrome before landing this. Currently I doubt it does. Perhaps I misunderstand and you mean this from a performance perspective, but all the methods in DCheck are guarded with a debug check to prevent them from running on Debug builds. This will turn the DCheck calls into no-ops if proguard doesn't strip them out. I believe the debug checks work on both the builds you refer to as the file is built from a template that uses the preprocessor. With this in mind, what were you concerned developers would have to worry about? Was it the potential cost of the function call and if-statement being scattered around the codebase and run on certain Release builds?
Oh my bad, you're right. I should have looked better. Please disregard my earlier comments. https://codereview.chromium.org/1918403004/diff/1/base/android/java/src/org/c... File base/android/java/src/org/chromium/base/Check.java (right): https://codereview.chromium.org/1918403004/diff/1/base/android/java/src/org/c... base/android/java/src/org/chromium/base/Check.java:1227: System.err.flush(); nit: System.err seems unused in Android: "By default, the Android system sends stdout and stderr (System.out and System.err) output to /dev/null." (from http://developer.android.com/tools/debugging/debugging-log.html#viewingStd) https://codereview.chromium.org/1918403004/diff/1/base/android/junit/src/org/... File base/android/junit/src/org/chromium/base/CheckTest.java (right): https://codereview.chromium.org/1918403004/diff/1/base/android/junit/src/org/... base/android/junit/src/org/chromium/base/CheckTest.java:34: public void setUp() { Statics are not cleared between individual tests. How about adding an @After to clear the callback? https://codereview.chromium.org/1918403004/diff/1/base/android/junit/src/org/... base/android/junit/src/org/chromium/base/CheckTest.java:393: } catch (final MockitoAssertionError e) { nit: why catching the error here? Wouldn't mockito properly log that it expected a function call here, and then stop?
> https://codereview.chromium.org/1918403004/diff/1/base/android/java/src/org/c... > base/android/java/src/org/chromium/base/Check.java:1227: System.err.flush(); > nit: System.err seems unused in Android: > "By default, the Android system sends stdout and stderr (System.out and > System.err) output to /dev/null." (from > http://developer.android.com/tools/debugging/debugging-log.html#viewingStd) Will remove > https://codereview.chromium.org/1918403004/diff/1/base/android/junit/src/org/... > base/android/junit/src/org/chromium/base/CheckTest.java:34: public void setUp() > { > Statics are not cleared between individual tests. How about adding an @After to > clear the callback? Added an @After tearDown method and reset the callback to null > https://codereview.chromium.org/1918403004/diff/1/base/android/junit/src/org/... > base/android/junit/src/org/chromium/base/CheckTest.java:393: } catch (final > MockitoAssertionError e) { > nit: why catching the error here? Wouldn't mockito properly log that it expected > a function call here, and then stop? The reason I catch and re-throw an exception is that it's easier to figure how what part of the test broke if you know what the last fail() was. I found when writing the tests, it was easier to figure out where the failure was by looking at the last known failure than looking up line numbers. I will remove if you don't think it adds value. I have an implementation question for you. I currently call Check#waitForDebuggerInDebug during Check#fail, which will cause the app to hang for about 10 seconds while it waits for a debugger. Would it be better to not include this as all crashes on Debug builds will look like an ANR and instead rely on the command line flag --wait-for-java-debugger? Keep in mind that Check/DCheck could be used anywhere, including in CommandLine.java as it is now, and this line flag might not have been processed yet.
> > https://codereview.chromium.org/1918403004/diff/1/base/android/junit/src/org/... > > base/android/junit/src/org/chromium/base/CheckTest.java:393: } catch (final > > MockitoAssertionError e) { > > nit: why catching the error here? Wouldn't mockito properly log that it expected > > a function call here, and then stop? > The reason I catch and re-throw an exception is that it's easier to figure how what part of the test broke if you know what the last fail() was. I found when writing the tests, it was easier to figure out where the failure was by looking at the last known failure than looking up line numbers. I will remove if you don't think it adds value. Mockito might show the string here already, but I don't feel strongly about it. Maybe add to the error message that the exception trace displayed is the one for the previous successful failure, since for the current one we didn't get one. > I have an implementation question for you. I currently call Check#waitForDebuggerInDebug during Check#fail, which will cause the app to hang for about 10 seconds while it waits for a debugger. Would it be better to not include this as all crashes on Debug builds will look like an ANR and instead rely on the command line flag --wait-for-java-debugger? Keep in mind that Check/DCheck could be used anywhere, including in CommandLine.java as it is now, and this line flag might not have been processed yet. How about both? Would that work to check the flag is we are using the native implementation of CommandLine, and always wait if we have the java only one? Junit tests will still hit the timeout though, as it looks like Robolectric properly implements waitForDebugger
On 2016/05/05 09:46:13, dgn wrote: > > > > https://codereview.chromium.org/1918403004/diff/1/base/android/junit/src/org/... > > > base/android/junit/src/org/chromium/base/CheckTest.java:393: } catch (final > > > MockitoAssertionError e) { > > > nit: why catching the error here? Wouldn't mockito properly log that it > expected > > > a function call here, and then stop? > > The reason I catch and re-throw an exception is that it's easier to figure how > what part of the test broke if you know what the last fail() was. I found when > writing the tests, it was easier to figure out where the failure was by looking > at the last known failure than looking up line numbers. I will remove if you > don't think it adds value. > > Mockito might show the string here already, but I don't feel strongly about it. > Maybe add to the error message that the exception trace displayed is the one for > the previous successful failure, since for the current one we didn't get one. That's what I've done in the second patch set. > > I have an implementation question for you. I currently call > Check#waitForDebuggerInDebug during Check#fail, which will cause the app to hang > for about 10 seconds while it waits for a debugger. Would it be better to not > include this as all crashes on Debug builds will look like an ANR and instead > rely on the command line flag --wait-for-java-debugger? Keep in mind that > Check/DCheck could be used anywhere, including in CommandLine.java as it is now, > and this line flag might not have been processed yet. > > How about both? Would that work to check the flag is we are using the native > implementation of CommandLine, and always wait if we have the java only one? > Junit tests will still hit the timeout though, as it looks like Robolectric > properly implements waitForDebugger This can't rely on native being initialized because there is plenty of code that runs before the native bridge is built, which is the code that you would need this waitForDebugger for. Once the app has started, the debugger can connect fine. This line is to make it possible to debug code that occurs during initialization. I just realized I can't use the command line because there are DChecks in it that would result in an infinite loop. This helper function is only useful before most things are initialized so it is most likely not going to be useful for most developers and only cause frustration on the Debug builds if it causes it to hang. What if I simply made a constant at the top for developers enable in code if they find they need the debugger to attach very early in the lifecycle, similar to the ones in other parts of the code that are always checked in false but centralized for easy developer toggling.
> > > I have an implementation question for you. I currently call > > Check#waitForDebuggerInDebug during Check#fail, which will cause the app to hang > > for about 10 seconds while it waits for a debugger. Would it be better to not > > include this as all crashes on Debug builds will look like an ANR and instead > > rely on the command line flag --wait-for-java-debugger? Keep in mind that > > Check/DCheck could be used anywhere, including in CommandLine.java as it is now, > > and this line flag might not have been processed yet. > > > > How about both? Would that work to check the flag is we are using the native > > implementation of CommandLine, and always wait if we have the java only one? > > Junit tests will still hit the timeout though, as it looks like Robolectric > > properly implements waitForDebugger > > This can't rely on native being initialized because there is plenty of code that runs before the native bridge is built, which is the code that you would need this waitForDebugger for. Once the app has started, the debugger can connect fine. This line is to make it possible to debug code that occurs during initialization. I just realized I can't use the command line because there are DChecks in it that would result in an infinite loop. This helper function is only useful before most things are initialized so it is most likely not going to be useful for most developers and only cause frustration on the Debug builds if it causes it to hang. > > What if I simply made a constant at the top for developers enable in code if they find they need the debugger to attach very early in the lifecycle, similar to the ones in other parts of the code that are always checked in false but centralized for easy developer toggling. That sounds like a good alternative, yes :)
I didn't realize I hadn't published my latest drafts. Sorry about the delay
On 2016/05/12 16:07:57, doloffd wrote: > I didn't realize I hadn't published my latest drafts. Sorry about the delay How about using a GN flag to make a build wait for the debugger, instead of changing the source? https://code.google.com/p/chromium/codesearch#chromium/src/base/android/java/... Also, I'm not owner, so you'll need it approved my someone else. Yaron, Nico, PTAL.
> How about using a GN flag to make a build wait for the debugger, instead of > changing the source? > > https://code.google.com/p/chromium/codesearch#chromium/src/base/android/java/... If I understand you correctly, you are suggesting adding a new build flag that will be exposed to java via the file you linked to so that the code waits for the debugger. Is this correct? If so, then it seems like overkill since that would trigger rebuilding a lot more code to adjust for the new #define being added. Another thing to keep in mind is that many Activities already use a command line flag to wait for debugger in java so that once the command line is initialized and some of the early classes are starting up, the debugger will most likely already be attached if the user wanted it. This flag is only helpful if there is a check that occurs even earlier in the startup.
sorry for the delay. +torne as I shouldn't approve this on behalf of webview. First off: I like this change. However, it still does concern me without additional safety measures. I like the use of BuildConfig to ensure that DCheck itself doesn't add overhead but the main issue I think is the one that's been discussed ad nauseam on the linked bug: how do you ensure that developers don't inadvertently slow down release builds? Let's make this concrete instead of talking about fud: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav... That assert is expensive - in the ballpark of 10s to 100ms for some users. I only know this from doing a perf analysis of related code but it's otherwise a seemingly innocuous android framework api call. If we start getting a bunch of these through DChecks being added, it's a bad place to be in. That's why I've insisted that we should programmatically enforce this - either by using Jack so asserts work "for free" or some sort of static analysis - errorprone,proguard,checkstyle(?) (I frankly don't care which) to ensure that discarded computations aren't left in release code. https://codereview.chromium.org/1918403004/diff/40001/base/android/java/src/o... File base/android/java/src/org/chromium/base/BaseChromiumApplication.java (right): https://codereview.chromium.org/1918403004/diff/40001/base/android/java/src/o... base/android/java/src/org/chromium/base/BaseChromiumApplication.java:161: Check.isTrue(Proxy.isProxyClass(activity.getWindow().getCallback().getClass()) why'd you choose Check in this case? I would expect the default replacement for assert is DCheck. If this is just to introduce a sample scenario of where to use it, I think the cases of RuntimeExceptions you highlighted are more appropriate
yfriedman@chromium.org changed reviewers: + torne@chromium.org
+torne for real
On 2016/05/25 15:39:55, Yaron wrote: > +torne for real Hmm. I suppose that adding Check should be fairly harmless though. In those cases we do want the code to remain even in release mode but admittedly it's probably not that useful to add and DCheck is what we really want.
On 2016/05/25 15:39:39, Yaron wrote: > sorry for the delay. > +torne as I shouldn't approve this on behalf of webview. Thanks Yaron :) > First off: I like this change. However, it still does concern me without > additional safety measures. > > I like the use of BuildConfig to ensure that DCheck itself doesn't add overhead I didn't know we actually had any mechanism to know in Java whether we were in debug or release. Does this work in all configurations? I'm curious now ;) > but the main issue I think is the one that's been discussed ad nauseam on the > linked bug: how do you ensure that developers don't inadvertently slow down > release builds? > > Let's make this concrete instead of talking about fud: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav... > > That assert is expensive - in the ballpark of 10s to 100ms for some users. I > only know this from doing a perf analysis of related code but it's otherwise a > seemingly innocuous android framework api call. If we start getting a bunch of > these through DChecks being added, it's a bad place to be in. That's why I've > insisted that we should programmatically enforce this - either by using Jack so > asserts work "for free" or some sort of static analysis - > errorprone,proguard,checkstyle(?) (I frankly don't care which) to ensure that > discarded computations aren't left in release code. Yeah, I'm pretty concerned about performance here, especially as a replacement for assert, which has totally different semantics to DCHECK() in native. Guarding with BuildConfig means we won't *spam the logs* in release builds where the stripping didn't work, but it won't solve performance problems caused by anything other than the actual logging, as those costs have already been paid before you hit the debug check. I don't think it's acceptable to introduce this unless we have a solid mechanism for ensuring that it's *always* stripped at compile time. Making it easy for people to write code that has a disproportionate performance impact on WebView (where we have less robust performance monitoring at present, though we're working on it) isn't okay :( Sorry. Not LGTM. :( > https://codereview.chromium.org/1918403004/diff/40001/base/android/java/src/o... > File base/android/java/src/org/chromium/base/BaseChromiumApplication.java > (right): > > https://codereview.chromium.org/1918403004/diff/40001/base/android/java/src/o... > base/android/java/src/org/chromium/base/BaseChromiumApplication.java:161: > Check.isTrue(Proxy.isProxyClass(activity.getWindow().getCallback().getClass()) > why'd you choose Check in this case? I would expect the default replacement for > assert is DCheck. If this is just to introduce a sample scenario of where to use > it, I think the cases of RuntimeExceptions you highlighted are more appropriate Yeah, that really shouldn't be a Check.
On 2016/05/25 17:29:11, Torne wrote: > On 2016/05/25 15:39:39, Yaron wrote: > > sorry for the delay. > > +torne as I shouldn't approve this on behalf of webview. > > Thanks Yaron :) > > > First off: I like this change. However, it still does concern me without > > additional safety measures. > > > > I like the use of BuildConfig to ensure that DCheck itself doesn't add > overhead > > I didn't know we actually had any mechanism to know in Java whether we were in > debug or release. Does this work in all configurations? I'm curious now ;) It should: https://codereview.chromium.org/1597273005 > > > but the main issue I think is the one that's been discussed ad nauseam on the > > linked bug: how do you ensure that developers don't inadvertently slow down > > release builds? > > > > Let's make this concrete instead of talking about fud: > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav... > > > > That assert is expensive - in the ballpark of 10s to 100ms for some users. I > > only know this from doing a perf analysis of related code but it's otherwise a > > seemingly innocuous android framework api call. If we start getting a bunch of > > these through DChecks being added, it's a bad place to be in. That's why I've > > insisted that we should programmatically enforce this - either by using Jack > so > > asserts work "for free" or some sort of static analysis - > > errorprone,proguard,checkstyle(?) (I frankly don't care which) to ensure that > > discarded computations aren't left in release code. > > Yeah, I'm pretty concerned about performance here, especially as a replacement > for assert, which has totally different semantics to DCHECK() in native. > Guarding with BuildConfig means we won't *spam the logs* in release builds where > the stripping didn't work, but it won't solve performance problems caused by > anything other than the actual logging, as those costs have already been paid > before you hit the debug check. > > I don't think it's acceptable to introduce this unless we have a solid mechanism > for ensuring that it's *always* stripped at compile time. Making it easy for > people to write code that has a disproportionate performance impact on WebView > (where we have less robust performance monitoring at present, though we're > working on it) isn't okay :( > > Sorry. Not LGTM. :( > > > > https://codereview.chromium.org/1918403004/diff/40001/base/android/java/src/o... > > File base/android/java/src/org/chromium/base/BaseChromiumApplication.java > > (right): > > > > > https://codereview.chromium.org/1918403004/diff/40001/base/android/java/src/o... > > base/android/java/src/org/chromium/base/BaseChromiumApplication.java:161: > > Check.isTrue(Proxy.isProxyClass(activity.getWindow().getCallback().getClass()) > > why'd you choose Check in this case? I would expect the default replacement > for > > assert is DCheck. If this is just to introduce a sample scenario of where to > use > > it, I think the cases of RuntimeExceptions you highlighted are more > appropriate > > Yeah, that really shouldn't be a Check.
> > but the main issue I think is the one that's been discussed ad nauseam on the > > linked bug: how do you ensure that developers don't inadvertently slow down > > release builds? > > > > Let's make this concrete instead of talking about fud: > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav... > > > > That assert is expensive - in the ballpark of 10s to 100ms for some users. I > > only know this from doing a perf analysis of related code but it's otherwise a > > seemingly innocuous android framework api call. If we start getting a bunch of > > these through DChecks being added, it's a bad place to be in. That's why I've > > insisted that we should programmatically enforce this - either by using Jack > so > > asserts work "for free" or some sort of static analysis - > > errorprone,proguard,checkstyle(?) (I frankly don't care which) to ensure that > > discarded computations aren't left in release code. > > Yeah, I'm pretty concerned about performance here, especially as a replacement > for assert, which has totally different semantics to DCHECK() in native. > Guarding with BuildConfig means we won't *spam the logs* in release builds where > the stripping didn't work, but it won't solve performance problems caused by > anything other than the actual logging, as those costs have already been paid > before you hit the debug check. > > I don't think it's acceptable to introduce this unless we have a solid mechanism > for ensuring that it's *always* stripped at compile time. Making it easy for > people to write code that has a disproportionate performance impact on WebView > (where we have less robust performance monitoring at present, though we're > working on it) isn't okay :( > > Sorry. Not LGTM. :( I'm not entirely sure what your concern is. Is it: 1) If not proguarded out, the performance of DCheck on release builds will add up as there are more and more DChecks added. 2) Developers will make expensive calls inside DCheck that will be evaluated in Release builds. e.g. `DCheck.isTrue(obj.getExpensiveBoolean())` 3) Something else entirely --- For 1, I wrote some instrumentation tests and ran them on this Kindle (http://specdevice.com/showspec.php?id=3b0c-6365-0033-c5870033c587) and got the following results with `sIsDebug = false` private static final int ITERATIONS = 100000; // Average duration of 100ns public void testEmptyLoop() { final long startTime = System.nanoTime(); for (int i = 0; i < ITERATIONS; i++); printAverage(startTime, System.nanoTime()); } // Average duration of 2973ns public void testDCheckLoop() { final long startTime = System.nanoTime(); for (int i = 0; i < ITERATIONS; i++) { DCheck.isFalse(false); } printAverage(startTime, System.nanoTime()); } // Average duration of 6112ns public void testCheckLoop() { final long startTime = System.nanoTime(); for (int i = 0; i < ITERATIONS; i++) { Check.isFalse(false); } printAverage(startTime, System.nanoTime()); } // Average duration of 129091ns public void testLogLoop() { final long startTime = System.nanoTime(); for (int i = 0; i < ITERATIONS; i++) { Log.i("tag", "message"); } printAverage(startTime, System.nanoTime()); } These costs are very low with ~3ms per DCheck, while a simple log statement is 43x as expensive at ~129ms --- For 2, I can't think of clean way to ensure developers don't make that expensive call, though it can be mitigated with if (BuildConfig.sIsDebug) { DCheck.isTrue(obj.getExpensiveBoolean()); } If this is the concern, I don't believe Jack will solve the problem of release checks such as those the use `throw new RuntimeException()`. If DChecks are unacceptable because of the risk of developer misuse or extra burden from wrapping them in if-statements, what if this change only included the Check class to centralize how to induce a crash in relase builds? > > > https://codereview.chromium.org/1918403004/diff/40001/base/android/java/src/o... > > File base/android/java/src/org/chromium/base/BaseChromiumApplication.java > > (right): > > > > > https://codereview.chromium.org/1918403004/diff/40001/base/android/java/src/o... > > base/android/java/src/org/chromium/base/BaseChromiumApplication.java:161: This was just a typo, will change to Check
> These costs are very low with ~3ms per DCheck, while a simple log statement is > 43x as expensive at ~129ms I did my math wrong for these numbers and was off by an order of magnitude, those numbers should be ~0.003ms and ~0.129ms respectively
On 2016/05/26 21:16:02, doloffd wrote: > > > but the main issue I think is the one that's been discussed ad nauseam on > the > > > linked bug: how do you ensure that developers don't inadvertently slow down > > > release builds? > > > > > > Let's make this concrete instead of talking about fud: > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav... > > > > > > That assert is expensive - in the ballpark of 10s to 100ms for some users. I > > > only know this from doing a perf analysis of related code but it's otherwise > a > > > seemingly innocuous android framework api call. If we start getting a bunch > of > > > these through DChecks being added, it's a bad place to be in. That's why > I've > > > insisted that we should programmatically enforce this - either by using Jack > > so > > > asserts work "for free" or some sort of static analysis - > > > errorprone,proguard,checkstyle(?) (I frankly don't care which) to ensure > that > > > discarded computations aren't left in release code. > > > > Yeah, I'm pretty concerned about performance here, especially as a replacement > > for assert, which has totally different semantics to DCHECK() in native. > > Guarding with BuildConfig means we won't *spam the logs* in release builds > where > > the stripping didn't work, but it won't solve performance problems caused by > > anything other than the actual logging, as those costs have already been paid > > before you hit the debug check. > > > > I don't think it's acceptable to introduce this unless we have a solid > mechanism > > for ensuring that it's *always* stripped at compile time. Making it easy for > > people to write code that has a disproportionate performance impact on WebView > > (where we have less robust performance monitoring at present, though we're > > working on it) isn't okay :( > > > > Sorry. Not LGTM. :( > > I'm not entirely sure what your concern is. Is it: > 1) If not proguarded out, the performance of DCheck on release builds will add > up as there are more and more DChecks added. Yes, this is one concern, though the overhead of DCheck itself should be pretty cheap. > 2) Developers will make expensive calls inside DCheck that will be evaluated in > Release builds. e.g. `DCheck.isTrue(obj.getExpensiveBoolean())` Yes, this is another concern. assert doesn't evaluate its parameter when disabled, so switching from assert to DCheck is a behavioural change here that developers aren't necessarily going to be aware of. It's not just the condition itself, also: there's also the message parameter, which might be constructed by formatting a string with other values (both the formatting, and the fetching of the values used in formatting, will take time). > 3) Something else entirely > > --- > For 1, I wrote some instrumentation tests and ran them on this Kindle > (http://specdevice.com/showspec.php?id=3b0c-6365-0033-c5870033c587) and got the > following results with `sIsDebug = false` > > private static final int ITERATIONS = 100000; > > // Average duration of 100ns > public void testEmptyLoop() { > final long startTime = System.nanoTime(); > for (int i = 0; i < ITERATIONS; i++); > printAverage(startTime, System.nanoTime()); > } > > // Average duration of 2973ns > public void testDCheckLoop() { > final long startTime = System.nanoTime(); > for (int i = 0; i < ITERATIONS; i++) { > DCheck.isFalse(false); > } > printAverage(startTime, System.nanoTime()); > } > > // Average duration of 6112ns > public void testCheckLoop() { > final long startTime = System.nanoTime(); > for (int i = 0; i < ITERATIONS; i++) { > Check.isFalse(false); > } > printAverage(startTime, System.nanoTime()); > } > > // Average duration of 129091ns > public void testLogLoop() { > final long startTime = System.nanoTime(); > for (int i = 0; i < ITERATIONS; i++) { > Log.i("tag", "message"); > } > printAverage(startTime, System.nanoTime()); > } > > These costs are very low with ~3ms per DCheck, while a simple log statement is > 43x as expensive at ~129ms Microbenchmarks like this aren't very representative on VMs that use optimising compilers. These should get compiled to pretty efficient code that won't necessarily be as good in real use cases. I do agree that the overhead is low, thoguh. > > --- > For 2, I can't think of clean way to ensure developers don't make that expensive > call, though it can be mitigated with > > if (BuildConfig.sIsDebug) { > DCheck.isTrue(obj.getExpensiveBoolean()); > } If we have to write code like this then it doesn't seem like introducing DCheck is adding a lot of value. > If this is the concern, I don't believe Jack will solve the problem of release > checks such as those the use `throw new RuntimeException()`. I'm not sure what you mean. If a release check is going to call something expensive in its condition then that's unavoidable no matter what we do (other than to try to avoid that in the first place). If we can switch to jack and have assert gain the semantics we actually want then this is inevitably better than the proposed DCheck here. > If DChecks are > unacceptable because of the risk of developer misuse or extra burden from > wrapping them in if-statements, what if this change only included the Check > class to centralize how to induce a crash in relase builds? Having a defined mechanism for asserting in release builds would be nice, yes, though it's a bit unfortunate if this ends up being totally different to asserting in debug - but I guess that's unavoidable since Java doesn't have language support for this. One thing I'd point out is that currently having a custom message requires you to format any possible parameters before making the call, which is a cost that will be incurred even if the condition is fine. Many log/assert systems support passing formatting parameters instead of a formatted message to avoid this; is there a way to do that here? > > > > > > https://codereview.chromium.org/1918403004/diff/40001/base/android/java/src/o... > > > File base/android/java/src/org/chromium/base/BaseChromiumApplication.java > > > (right): > > > > > > > > > https://codereview.chromium.org/1918403004/diff/40001/base/android/java/src/o... > > > base/android/java/src/org/chromium/base/BaseChromiumApplication.java:161: > > This was just a typo, will change to Check
On 2016/05/31 11:22:28, Torne wrote:
> > 2) Developers will make expensive calls inside DCheck that will be evaluated
> in
> > Release builds. e.g. `DCheck.isTrue(obj.getExpensiveBoolean())`
>
> Yes, this is another concern. assert doesn't evaluate its parameter when
> disabled, so switching from assert to DCheck is a behavioural change here that
> developers aren't necessarily going to be aware of.
>
> It's not just the condition itself, also: there's also the message parameter,
> which might be constructed by formatting a string with other values (both the
> formatting, and the fetching of the values used in formatting, will take
time).
See my response at the end about using `String.format()` instead
> > For 2, I can't think of clean way to ensure developers don't make that
> expensive
> > call, though it can be mitigated with
> >
> > if (BuildConfig.sIsDebug) {
> > DCheck.isTrue(obj.getExpensiveBoolean());
> > }
>
> If we have to write code like this then it doesn't seem like introducing
DCheck
> is adding a lot of value.
>
> > If this is the concern, I don't believe Jack will solve the problem of
release
> > checks such as those the use `throw new RuntimeException()`.
>
> I'm not sure what you mean. If a release check is going to call something
> expensive in its condition then that's unavoidable no matter what we do (other
> than to try to avoid that in the first place).
>
> If we can switch to jack and have assert gain the semantics we actually want
> then this is inevitably better than the proposed DCheck here.
>
> > If DChecks are
> > unacceptable because of the risk of developer misuse or extra burden from
> > wrapping them in if-statements, what if this change only included the Check
> > class to centralize how to induce a crash in relase builds?
>
> Having a defined mechanism for asserting in release builds would be nice, yes,
> though it's a bit unfortunate if this ends up being totally different to
> asserting in debug - but I guess that's unavoidable since Java doesn't have
> language support for this.
Is it not possible for the Jack team to expose an additional keyword in Java so
that you could use `assert` for release and `dassert` for debug and compile out
`dassert` or something like it in debug builds? It seems like it would be
cleaner and less confusing to either use Check and DCheck or only use Java
keywords and not mix them. Do you agree or should I move forward with this
review, removing the DCheck code?
> One thing I'd point out is that currently having a custom message requires you
> to format any possible parameters before making the call, which is a cost that
> will be incurred even if the condition is fine. Many log/assert systems
support
> passing formatting parameters instead of a formatted message to avoid this; is
> there a way to do that here?
Sure, I could use `String.format()` like I do in Check and is used in
org.chromium.base.Log. I could extend the functions to something like:
public static void isTrue(boolean value, String messageFormat, Object...
messageParams);
On 2016/06/04 20:57:23, doloffd wrote: > Is it not possible for the Jack team to expose an additional keyword in Java so > that you could use `assert` for release and `dassert` for debug and compile out > `dassert` or something like it in debug builds? It seems like it would be > cleaner and less confusing to either use Check and DCheck or only use Java > keywords and not mix them. Do you agree or should I move forward with this > review, removing the DCheck code? I think it would be *extremely* weird to try to introduce a new Java keyword for this. It would break basically all editors/IDEs/static analysers/etc, and we are likely to continue to rely on compiling at least some parts of the code with javac in order to run host-side tests (on a regular, non-Android JVM). So, yeah, due to the nature of Java here I think we should go with assert + Check.
OK, sorry for the delay, have been really busy with things. Proguard for webview is now fixed and we can remove things in release builds, so it's probably possible to reintroduce DCheck here, but first we should get a Check we're happy with so that DCheck can just be defined similarly :) General question: for a given case, what's the bytecode size of compiling a call to Check vs just checking the condition and throwing an exception at the callsite manually? Also, how much gets executed in the case where the assertion is fine? (i.e. what's the performance cost when we aren't crashing) https://codereview.chromium.org/1918403004/diff/80001/base/android/java/src/o... File base/android/java/src/org/chromium/base/Check.java (right): https://codereview.chromium.org/1918403004/diff/80001/base/android/java/src/o... base/android/java/src/org/chromium/base/Check.java:31: "Not equal. Expected [%s], but recieved [%s]"; spelling: received https://codereview.chromium.org/1918403004/diff/80001/base/android/java/src/o... base/android/java/src/org/chromium/base/Check.java:475: fail(stringFormat("Expected comparables to be not equal but were. [%s] [%s]", This message is pretty hard to follow.. not sure how to phrase it concisely though. https://codereview.chromium.org/1918403004/diff/80001/base/android/java/src/o... base/android/java/src/org/chromium/base/Check.java:1380: Log.wtf(TAG, formattedMessage, assertionError); This doesn't seem like an ideal way to crash at all - this won't get reported as an uncaught exception, and so we won't get data from the field about it (unless i'm missing something here). wtf() is pretty heavyweight also (it logs a LOT, and on debug devices makes a tombstone), plus the tag you're using is too generic: since base is shared by webview it's best if log tags contain some indication that chromium is involved, to avoid confusing developers. Why not just construct the Error with the formatted message and then throw it?
On 2016/07/25 15:19:20, Torne wrote: > Proguard for webview is now fixed and we can remove things in release builds, so > it's probably possible to reintroduce DCheck here, but first we should get a > Check we're happy with so that DCheck can just be defined similarly :) What about your comment in https://codereview.chromium.org/1918403004/#msg33 where you stated that Chromium was going do use Jack to remove or force enable asserts in the builds? Are you saying that it would be cleaner to have identical ways of doing DChecks as Checks? > General question: for a given case, what's the bytecode size of compiling a call > to Check vs just checking the condition and throwing an exception at the > callsite manually? Also, how much gets executed in the case where the assertion > is fine? (i.e. what's the performance cost when we aren't crashing) In https://codereview.chromium.org/1918403004/#msg31 I ran some micro-benchmarks to get an idea of the cost of passing Checks in release builds. I didn't compare the bytecode at the time, do you still want me to investigate that for you? > https://codereview.chromium.org/1918403004/diff/80001/base/android/java/src/o... > base/android/java/src/org/chromium/base/Check.java:31: "Not equal. Expected > [%s], but recieved [%s]"; > spelling: received Okay, will do > https://codereview.chromium.org/1918403004/diff/80001/base/android/java/src/o... > base/android/java/src/org/chromium/base/Check.java:475: > fail(stringFormat("Expected comparables to be not equal but were. [%s] [%s]", > This message is pretty hard to follow.. not sure how to phrase it concisely > though. How about changing it back to "Expected compareTo to return 0 but returned %d. [%s] [%s]" > https://codereview.chromium.org/1918403004/diff/80001/base/android/java/src/o... > base/android/java/src/org/chromium/base/Check.java:1380: Log.wtf(TAG, > formattedMessage, assertionError); > This doesn't seem like an ideal way to crash at all - this won't get reported as > an uncaught exception, and so we won't get data from the field about it (unless > i'm missing something here). > > wtf() is pretty heavyweight also (it logs a LOT, and on debug devices makes a > tombstone), plus the tag you're using is too generic: since base is shared by > webview it's best if log tags contain some indication that chromium is involved, > to avoid confusing developers. I didn't realize wtf() could induce a crash. A little further down I induce the crash with `Runtime.getRuntime().halt(1);`. Isn't the idea of these types of crashes to print a lot of information to make the crash easier to debug? How can I check how/if this type of crash will be reported? I'll change the tag to "CHROMIUM_CHECK_ASSERTION" > Why not just construct the Error with the formatted message and then throw it? The problem with throwing any Throwable is that it could be caught for the wrong reason. There are already 32 examples so far of catches for all Throwables in the code. https://cs.chromium.org/search/?q=%22catch+(Throwable%22+lang:java&sq=package... The fail() function is designed to cause an unblockable crash, like C++'s CHECK.
On 2016/07/26 17:10:39, doloffd wrote: > On 2016/07/25 15:19:20, Torne wrote: > > Proguard for webview is now fixed and we can remove things in release builds, > so > > it's probably possible to reintroduce DCheck here, but first we should get a > > Check we're happy with so that DCheck can just be defined similarly :) > > What about your comment in https://codereview.chromium.org/1918403004/#msg33 > where you stated that Chromium was going do use Jack to remove or force enable > asserts in the builds? Are you saying that it would be cleaner to have identical > ways of doing DChecks as Checks? Oh, I forgot about that :/ I'm not sure which is nicer, to be honest. We should probably start by getting Check into a state where everyone is happy with it, land it, and see how people feel about using it, and also keep an eye on how jack stuff is going, and re-evaluate what to do about dcheck vs assert when there's been a bit of experience here? > > General question: for a given case, what's the bytecode size of compiling a > call > > to Check vs just checking the condition and throwing an exception at the > > callsite manually? Also, how much gets executed in the case where the > assertion > > is fine? (i.e. what's the performance cost when we aren't crashing) > > In https://codereview.chromium.org/1918403004/#msg31 I ran some micro-benchmarks > to get an idea of the cost of passing Checks in release builds. I didn't compare > the bytecode at the time, do you still want me to investigate that for you? Yeah, the overhead in a microbenchmark didn't look too terrible; sorry, I forgot you already did it. It'd be interesting to see the size difference for some plausible cases, because if we're going to insert as many of them as we have CHECKs in native, the multiplier might be quite big. It'll vary on a case by case basis, though.. > > > https://codereview.chromium.org/1918403004/diff/80001/base/android/java/src/o... > > base/android/java/src/org/chromium/base/Check.java:31: "Not equal. Expected > > [%s], but recieved [%s]"; > > spelling: received > > Okay, will do > > > > https://codereview.chromium.org/1918403004/diff/80001/base/android/java/src/o... > > base/android/java/src/org/chromium/base/Check.java:475: > > fail(stringFormat("Expected comparables to be not equal but were. [%s] [%s]", > > This message is pretty hard to follow.. not sure how to phrase it concisely > > though. > > How about changing it back to "Expected compareTo to return 0 but returned %d. > [%s] [%s]" Yeah, that's at least clear about what happened. > > > > https://codereview.chromium.org/1918403004/diff/80001/base/android/java/src/o... > > base/android/java/src/org/chromium/base/Check.java:1380: Log.wtf(TAG, > > formattedMessage, assertionError); > > This doesn't seem like an ideal way to crash at all - this won't get reported > as > > an uncaught exception, and so we won't get data from the field about it > (unless > > i'm missing something here). > > > > wtf() is pretty heavyweight also (it logs a LOT, and on debug devices makes a > > tombstone), plus the tag you're using is too generic: since base is shared by > > webview it's best if log tags contain some indication that chromium is > involved, > > to avoid confusing developers. > > I didn't realize wtf() could induce a crash. It doesn't actually induce a crash, but on some debug configurations it captures the same information that an actual crash would have done and saves it, which is pretty expensive. We're crashing anyway so the cost isn't a huge deal, but it's still considerably more involved than just printing a message. > A little further down I induce the > crash with `Runtime.getRuntime().halt(1);`. This is not actually a crash at all in Android or Chrome's view. This is a clean, normal program termination as far as the OS is concerned - the process exits, without receiving a signal. Android doesn't care about process termination and doesn't check exit codes. > Isn't the idea of these types of > crashes to print a lot of information to make the crash easier to debug? The point of having a CHECK rather than a DCHECK is that we want it to crash out in the field on real users' devices. This means we can't rely on a developer ever seeing the logs, because users rarely report this kind of thing, and rarely include logs. It needs to be a type of crash which is directly reported back to Google for users who have enabled crash reporting, and the automated report needs to include the right information to make it easy to do stats on. We want as much of the information about why we crashed to be in the machine-parseable crash metadata, and not printed into logs (we do get logs in most cases, but maybe not enough of the log, and it's harder to do analysis on). > How can I check how/if this type of crash will be reported? Two kinds of things are reported by breakpad, basically: 1) Native crashes caused by unhandled signals of a few types: SEGV, BUS, ILL (and others that indicate a memory/program error), as well as ABRT (caused by calls to abort() in the code, which is what CHECK() does). These are reported with their *native* crash stack, which is not a very convenient way to report a Java exception; our crash backend won't be able to handle them sensibly and process their real, Java, cause. 2) Uncaught Java exceptions. If nobody catches an exception and it causes a thread to die, we report the exception, in a nice way that captures the Java stack and exception details properly. This is what would make Check valuable and usable on the crash backend. Android also has its own crash reporting mechanism which we rely on as well as breakpad's own one (they have different coverage, since user consent is handled in different ways), and it also reports these same two classes of thing in pretty much the same way. > I'll change the tag to "CHROMIUM_CHECK_ASSERTION" Maximum tag length is 23 characters. I think we have a convention of using cr_ as a prefix, but i'm not sure what the current recommendations are; people have been trying to make our tags consistent/rational. > > > Why not just construct the Error with the formatted message and then throw it? > > The problem with throwing any Throwable is that it could be caught for the wrong > reason. There are already 32 examples so far of catches for all Throwables in > the code. > https://cs.chromium.org/search/?q=%22catch+(Throwable%22+lang:java&sq=package... > The fail() function is designed to cause an unblockable crash, like C++'s > CHECK. Right, so unfortunately right now there's no sensible way for Java code to cause an unblockable crash that will get reported in a usable way to our crash backend. In theory we could introduce a magic way to report the assertion failure separately from the crash, but that seems pretty complicated, and would also only work for breakpad and not the regular android crash handling mechanism, which isn't under our control. Anywhere that is catching Throwable without rethrowing it is likely to be a big issue already: there are lots of VM conditions that really, really don't make sense to catch. Even "assert" is caught if you do this, sicne it just throws AssertionError. I'm not convinced trying to avoid this is worthwhile.
> I'm not sure which is nicer, to be honest. We should probably start by getting > Check into a state where everyone is happy with it, land it, and see how people > feel about using it, and also keep an eye on how jack stuff is going, and > re-evaluate what to do about dcheck vs assert when there's been a bit of > experience here? I'll leave DCheck out for now then > > > General question: for a given case, what's the bytecode size of compiling a > > call > > > to Check vs just checking the condition and throwing an exception at the > > > callsite manually? Also, how much gets executed in the case where the > > assertion > > > is fine? (i.e. what's the performance cost when we aren't crashing) > > > > In https://codereview.chromium.org/1918403004/#msg31 I ran some > micro-benchmarks > > to get an idea of the cost of passing Checks in release builds. I didn't > compare > > the bytecode at the time, do you still want me to investigate that for you? > > Yeah, the overhead in a microbenchmark didn't look too terrible; sorry, I forgot > you already did it. It'd be interesting to see the size difference for some > plausible cases, because if we're going to insert as many of them as we have > CHECKs in native, the multiplier might be quite big. It'll vary on a case by > case basis, though.. I haven't done the work to look at the bytecode but the differences in code would be if (badState) { throw new RuntimeException("Bad state encountered"); } vs Check.isFalse(badState, "Bad state encountered"); The bytecode generated depends on the compiler so Jack might produce different bytecode than javac does currently > https://codereview.chromium.org/1918403004/diff/80001/base/android/java/src/o... > > > base/android/java/src/org/chromium/base/Check.java:1380: Log.wtf(TAG, > > > formattedMessage, assertionError); > > > This doesn't seem like an ideal way to crash at all - this won't get > reported > > as > > > an uncaught exception, and so we won't get data from the field about it > > (unless > > > i'm missing something here). > > > > > > wtf() is pretty heavyweight also (it logs a LOT, and on debug devices makes > a > > > tombstone), plus the tag you're using is too generic: since base is shared > by > > > webview it's best if log tags contain some indication that chromium is > > involved, > > > to avoid confusing developers. > > > > I didn't realize wtf() could induce a crash. > > It doesn't actually induce a crash, but on some debug configurations it captures > the same information that an actual crash would have done and saves it, which is > pretty expensive. We're crashing anyway so the cost isn't a huge deal, but it's > still considerably more involved than just printing a message. > > > A little further down I induce the > > crash with `Runtime.getRuntime().halt(1);`. > > This is not actually a crash at all in Android or Chrome's view. This is a > clean, normal program termination as far as the OS is concerned - the process > exits, without receiving a signal. Android doesn't care about process > termination and doesn't check exit codes. > > > Isn't the idea of these types of > > crashes to print a lot of information to make the crash easier to debug? > > The point of having a CHECK rather than a DCHECK is that we want it to crash out > in the field on real users' devices. This means we can't rely on a developer > ever seeing the logs, because users rarely report this kind of thing, and rarely > include logs. > > It needs to be a type of crash which is directly reported back to Google for > users who have enabled crash reporting, and the automated report needs to > include the right information to make it easy to do stats on. We want as much of > the information about why we crashed to be in the machine-parseable crash > metadata, and not printed into logs (we do get logs in most cases, but maybe not > enough of the log, and it's harder to do analysis on). > > > How can I check how/if this type of crash will be reported? > > Two kinds of things are reported by breakpad, basically: > > 1) Native crashes caused by unhandled signals of a few types: SEGV, BUS, ILL > (and others that indicate a memory/program error), as well as ABRT (caused by > calls to abort() in the code, which is what CHECK() does). These are reported > with their *native* crash stack, which is not a very convenient way to report a > Java exception; our crash backend won't be able to handle them sensibly and > process their real, Java, cause. > > 2) Uncaught Java exceptions. If nobody catches an exception and it causes a > thread to die, we report the exception, in a nice way that captures the Java > stack and exception details properly. This is what would make Check valuable and > usable on the crash backend. > > Android also has its own crash reporting mechanism which we rely on as well as > breakpad's own one (they have different coverage, since user consent is handled > in different ways), and it also reports these same two classes of thing in > pretty much the same way. How about this, first it tries to make a JNI call to induce a `CHECK(FALSE)` then throws if it can't? This is in preference to NOTREACHED() since that will not crash on release builds and we've already done the debug/release checking in java. If JNI hasn't been set up yet, then I'll throw a Throwable and hope for the best. That way 99% of the time we will get a forced crash and we have a much smaller risk that crash can be caught. > > I'll change the tag to "CHROMIUM_CHECK_ASSERTION" > > Maximum tag length is 23 characters. I think we have a convention of using cr_ > as a prefix, but i'm not sure what the current recommendations are; people have > been trying to make our tags consistent/rational. Chromium's logger already prepends "cr_" to the tag so I'll leave the tag as is.
On 2016/08/03 22:45:42, doloffd wrote: > > > > General question: for a given case, what's the bytecode size of compiling > a > > > call > > > > to Check vs just checking the condition and throwing an exception at the > > > > callsite manually? Also, how much gets executed in the case where the > > > assertion > > > > is fine? (i.e. what's the performance cost when we aren't crashing) > > > > > > In https://codereview.chromium.org/1918403004/#msg31 I ran some > > micro-benchmarks > > > to get an idea of the cost of passing Checks in release builds. I didn't > > compare > > > the bytecode at the time, do you still want me to investigate that for you? > > > > Yeah, the overhead in a microbenchmark didn't look too terrible; sorry, I > forgot > > you already did it. It'd be interesting to see the size difference for some > > plausible cases, because if we're going to insert as many of them as we have > > CHECKs in native, the multiplier might be quite big. It'll vary on a case by > > case basis, though.. > > I haven't done the work to look at the bytecode but the differences in code > would be > > if (badState) { > throw new RuntimeException("Bad state encountered"); > } > > vs > > Check.isFalse(badState, "Bad state encountered"); > > The bytecode generated depends on the compiler so Jack might produce different > bytecode than javac does currently That's the most trivial case; what about more complex ones? And it's likely to compile to about the same thing no matter what; there's not a lot of room for interpretation. > > > https://codereview.chromium.org/1918403004/diff/80001/base/android/java/src/o... > > > > base/android/java/src/org/chromium/base/Check.java:1380: Log.wtf(TAG, > > > > formattedMessage, assertionError); > > > > This doesn't seem like an ideal way to crash at all - this won't get > > reported > > > as > > > > an uncaught exception, and so we won't get data from the field about it > > > (unless > > > > i'm missing something here). > > > > > > > > wtf() is pretty heavyweight also (it logs a LOT, and on debug devices > makes > > a > > > > tombstone), plus the tag you're using is too generic: since base is shared > > by > > > > webview it's best if log tags contain some indication that chromium is > > > involved, > > > > to avoid confusing developers. > > > > > > I didn't realize wtf() could induce a crash. > > > > It doesn't actually induce a crash, but on some debug configurations it > captures > > the same information that an actual crash would have done and saves it, which > is > > pretty expensive. We're crashing anyway so the cost isn't a huge deal, but > it's > > still considerably more involved than just printing a message. > > > > > A little further down I induce the > > > crash with `Runtime.getRuntime().halt(1);`. > > > > This is not actually a crash at all in Android or Chrome's view. This is a > > clean, normal program termination as far as the OS is concerned - the process > > exits, without receiving a signal. Android doesn't care about process > > termination and doesn't check exit codes. > > > > > Isn't the idea of these types of > > > crashes to print a lot of information to make the crash easier to debug? > > > > The point of having a CHECK rather than a DCHECK is that we want it to crash > out > > in the field on real users' devices. This means we can't rely on a developer > > ever seeing the logs, because users rarely report this kind of thing, and > rarely > > include logs. > > > > It needs to be a type of crash which is directly reported back to Google for > > users who have enabled crash reporting, and the automated report needs to > > include the right information to make it easy to do stats on. We want as much > of > > the information about why we crashed to be in the machine-parseable crash > > metadata, and not printed into logs (we do get logs in most cases, but maybe > not > > enough of the log, and it's harder to do analysis on). > > > > > How can I check how/if this type of crash will be reported? > > > > Two kinds of things are reported by breakpad, basically: > > > > 1) Native crashes caused by unhandled signals of a few types: SEGV, BUS, ILL > > (and others that indicate a memory/program error), as well as ABRT (caused by > > calls to abort() in the code, which is what CHECK() does). These are reported > > with their *native* crash stack, which is not a very convenient way to report > a > > Java exception; our crash backend won't be able to handle them sensibly and > > process their real, Java, cause. > > > > 2) Uncaught Java exceptions. If nobody catches an exception and it causes a > > thread to die, we report the exception, in a nice way that captures the Java > > stack and exception details properly. This is what would make Check valuable > and > > usable on the crash backend. > > > > Android also has its own crash reporting mechanism which we rely on as well as > > breakpad's own one (they have different coverage, since user consent is > handled > > in different ways), and it also reports these same two classes of thing in > > pretty much the same way. > > How about this, first it tries to make a JNI call to induce a `CHECK(FALSE)` > then throws if it can't? No, crashing via JNI will not report the java state, as I said - we will just get a native crash that doesn't tell us in what java callstack it died. You *must* throw an exception, nothing else will produce the right results. > This is in preference to NOTREACHED() since that will not crash on release > builds and we've already done the debug/release checking in java. If JNI hasn't > been set up yet, then I'll throw a Throwable and hope for the best. That way 99% > of the time we will get a forced crash and we have a much smaller risk that > crash can be caught. > > > > I'll change the tag to "CHROMIUM_CHECK_ASSERTION" > > > > Maximum tag length is 23 characters. I think we have a convention of using cr_ > > as a prefix, but i'm not sure what the current recommendations are; people > have > > been trying to make our tags consistent/rational. > > Chromium's logger already prepends "cr_" to the tag so I'll leave the tag as is.
Okay, I will convert the crashing to just throwing a Throwable to minimize the
catchability of it.
I used dexdump to get the following dex code conversions
if (context == null) {
throw new RuntimeException("Context must not be null");
}
0000: if-nez v2, 000b // +000b
0002: new-instance v0, Ljava/lang/RuntimeException; // type@0920
0004: const-string/jumbo v1, "Context must not be null" // string@00000b7f
0007: invoke-direct {v0, v1},
Ljava/lang/RuntimeException;.<init>:(Ljava/lang/String;)V // method@39be
000a: throw v0
Check.isNotNull(context, "Context must not be null");
0000: const-string/jumbo v0, "Context must not be null" // string@00000b7f
0003: const/4 v1, #int 0 // #0
0004: new-array v1, v1, [Ljava/lang/Object; // type@14e9
0006: invoke-static {v2, v0, v1},
Lorg/chromium/base/Check;.isNotNull:(Ljava/lang/Object;Ljava/lang/CharSequence;[Ljava/lang/Object;)V
// method@4623
On 2016/08/12 00:31:17, doloffd wrote:
> Okay, I will convert the crashing to just throwing a Throwable to minimize the
> catchability of it.
Thanks; throwing AssertionError is reasonable, anywhere that's catching that
either really means to catch everything (including SystemExit and other VM
conditions), or is wrong anyway :)
> I used dexdump to get the following dex code conversions
>
> if (context == null) {
> throw new RuntimeException("Context must not be null");
> }
>
> 0000: if-nez v2, 000b // +000b
> 0002: new-instance v0, Ljava/lang/RuntimeException; // type@0920
> 0004: const-string/jumbo v1, "Context must not be null" // string@00000b7f
> 0007: invoke-direct {v0, v1},
> Ljava/lang/RuntimeException;.<init>:(Ljava/lang/String;)V // method@39be
> 000a: throw v0
>
> Check.isNotNull(context, "Context must not be null");
>
> 0000: const-string/jumbo v0, "Context must not be null" // string@00000b7f
> 0003: const/4 v1, #int 0 // #0
> 0004: new-array v1, v1, [Ljava/lang/Object; // type@14e9
> 0006: invoke-static {v2, v0, v1},
>
Lorg/chromium/base/Check;.isNotNull:(Ljava/lang/Object;Ljava/lang/CharSequence;[Ljava/lang/Object;)V
> // method@4623
So this is for pretty much the most trivial case (a trivial check with a
message), and there's a definite runtime cost here: we're constructing an empty
array to indicate that we have no format parameters to substitute. I can't
actually tell the exact byte cost difference, because you haven't included the
next instruction (well, its offset) and I'm not sure how large that
invoke-static is encoding to :)
In theory we could avoid that case by specialcasing some arities of the vararg
function (zero for a start, maybe one or two?) which would remove the array
construction and just push the values directly to the stack - I'm not sure
whether Java will let you overload a function with varargs though, so doing that
might require expanding it out for all arities we intend to support. Either way
this is going to be a large and annoying copypaste job though, so we should
think about whether it's worth it first.
Yaron, Nicholas, what do you think about this in its current form? Are we
confident the performance overhead here is going to be reasonable if people
start using it in a similar way to CHECK() in native code? That's my major
concern at this point - if we imagine several hundred of these throughout our
java code, is that going to add up to a noticeable effect on performance or
binary size?
torne@chromium.org changed reviewers: + smaier@chromium.org
Also +smaier who has been working on proguard config, debug/release differences, and general size optimisation; any opinions about this change?
On 2016/08/16 12:50:15, Torne wrote: > Also +smaier who has been working on proguard config, debug/release differences, > and general size optimisation; any opinions about this change? I don't think I have anything to add with Check, but here are my thoughts on DCheck: 1) Jack is a while off. We shouldn't be counting on it for anything we want soon. It seems quite immature - a number of big clients are waiting for parity with ProGuard features, which is "very far out". 2) @RemovableInRelease now works in anywhere which uses base/android/base_proguard_config.flags (I believe this is Chrome, WebView, and very soon Cronet). I wouldn't be too worried about adding DCheck. 3) There is something we can do downstream to help guarantee DCheck is being removed. I have added the reviewers on this CL to the sample downstream review to avoid sharing the link publicly.
On 2016/08/16 16:03:07, smaier wrote: > On 2016/08/16 12:50:15, Torne wrote: > > Also +smaier who has been working on proguard config, debug/release > differences, > > and general size optimisation; any opinions about this change? > > I don't think I have anything to add with Check, but here are my thoughts on > DCheck: > > 1) Jack is a while off. We shouldn't be counting on it for anything we want > soon. It seems quite immature - a number of big clients are waiting for parity > with ProGuard features, which is "very far out". > > 2) @RemovableInRelease now works in anywhere which uses > base/android/base_proguard_config.flags (I believe this is Chrome, WebView, and > very soon Cronet). I wouldn't be too worried about adding DCheck. > > 3) There is something we can do downstream to help guarantee DCheck is being > removed. I have added the reviewers on this CL to the sample downstream review > to avoid sharing the link publicly. OK, so if we decide we're happy with Check, then I think we can also add a matching DCheck, which makes this a bit nicer. Thanks Sam. We still need to discuss Check though :)
On 2016/08/16 16:06:01, Torne wrote: > On 2016/08/16 16:03:07, smaier wrote: > > On 2016/08/16 12:50:15, Torne wrote: > > > Also +smaier who has been working on proguard config, debug/release > > differences, > > > and general size optimisation; any opinions about this change? > > > > I don't think I have anything to add with Check, but here are my thoughts on > > DCheck: > > > > 1) Jack is a while off. We shouldn't be counting on it for anything we want > > soon. It seems quite immature - a number of big clients are waiting for parity > > with ProGuard features, which is "very far out". > > > > 2) @RemovableInRelease now works in anywhere which uses > > base/android/base_proguard_config.flags (I believe this is Chrome, WebView, > and > > very soon Cronet). I wouldn't be too worried about adding DCheck. > > > > 3) There is something we can do downstream to help guarantee DCheck is being > > removed. I have added the reviewers on this CL to the sample downstream review > > to avoid sharing the link publicly. > > OK, so if we decide we're happy with Check, then I think we can also add a > matching DCheck, which makes this a bit nicer. Thanks Sam. We still need to > discuss Check though :) Some thoughts: - I think DCheck is a more valuable addition than Check (and a potential replacement for assert) but I'm still concerned about it. Developers don't expect it to add overhead in release but it sounds like it currently would. Sam's CL is nice in that it ensures DCheck itself doesn't add cost but doesn't help with evaluating parameters which is likely more expensive. He was going to check internal proguard to see if there's any help there but without it, I'm still concerned - Check seems like a fine addition - developers know it runs in release so are less likely and more thoughtful about adding it only when necessary. We should still minimize cost and that probably means doing the copy/paste dance Torne mentioned. - Per Sam's comments, Jack doesn't seem like a viable alternative and we definitely don't want a new keyword there :)
> - I think DCheck is a more valuable addition than Check (and a potential
> replacement for assert) but I'm still concerned about it. Developers don't
> expect it to add overhead in release but it sounds like it currently would.
> Sam's CL is nice in that it ensures DCheck itself doesn't add cost but doesn't
> help with evaluating parameters which is likely more expensive. He was going
to
> check internal proguard to see if there's any help there but without it, I'm
> still concerned
One final alternative I can see for DChecks, which is somewhat ugly but ensures
proper use could be to an extra method to DCheck
// Debug
private static final Set<Long> sOnCheckThreadIdSet =
Collections.synchronizedSet<Long>(new HashSet<Long>());
public static boolean on() {
final Long threadId = Thread.currentThread().getId();
if (sOnCheckThreadIdSet.contains(threadId)) {
fail("You already called on() but didn't perform any checks");
}
sOnCheckThreadIdSet.put(threadId());
return true;
}
public static void isCase(...) {
final Long threadId = Thread.currentThread().getId();
if (!sOnCheckThreadIdSet.contains(threadId)) {
fail("Always check if DCheck is on first");
}
sOnCheckThreadIdSet.remove(threadId);
// Do checking
}
// Release
@RemoveableInRelease
public static boolean on() {
return false;
}
@RemoveableInRelease
public static void isCase(...) { }
The usage would look like this
if (DCheck.on()) DCheck.isTrue(thing.expensive(), "Log statement %s",
thing.name());
I believe proguard can inline return values and if it has a guaranteed return
value, it should be able to remove if (false) { ... } blocks. I can try and
implement it and see what proguard ends up doing with the code. I could also
have on optionally take an int, which is the number of expected DChecks being
performed in the block and use a Map instead of a Set for that purpose. Would
having this system in place work (assuming proguard acts I suggested) or do you
think this adds too much extra code?
On 2016/08/16 18:34:18, doloffd wrote:
> > - I think DCheck is a more valuable addition than Check (and a potential
> > replacement for assert) but I'm still concerned about it. Developers don't
> > expect it to add overhead in release but it sounds like it currently would.
> > Sam's CL is nice in that it ensures DCheck itself doesn't add cost but
doesn't
> > help with evaluating parameters which is likely more expensive. He was going
> to
> > check internal proguard to see if there's any help there but without it, I'm
> > still concerned
>
> One final alternative I can see for DChecks, which is somewhat ugly but
ensures
> proper use could be to an extra method to DCheck
>
> // Debug
> private static final Set<Long> sOnCheckThreadIdSet =
> Collections.synchronizedSet<Long>(new HashSet<Long>());
>
> public static boolean on() {
> final Long threadId = Thread.currentThread().getId();
> if (sOnCheckThreadIdSet.contains(threadId)) {
> fail("You already called on() but didn't perform any checks");
> }
> sOnCheckThreadIdSet.put(threadId());
> return true;
> }
>
> public static void isCase(...) {
> final Long threadId = Thread.currentThread().getId();
> if (!sOnCheckThreadIdSet.contains(threadId)) {
> fail("Always check if DCheck is on first");
> }
> sOnCheckThreadIdSet.remove(threadId);
> // Do checking
> }
>
> // Release
>
> @RemoveableInRelease
> public static boolean on() {
> return false;
> }
>
> @RemoveableInRelease
> public static void isCase(...) { }
>
> The usage would look like this
>
> if (DCheck.on()) DCheck.isTrue(thing.expensive(), "Log statement %s",
> thing.name());
>
> I believe proguard can inline return values and if it has a guaranteed return
> value, it should be able to remove if (false) { ... } blocks. I can try and
> implement it and see what proguard ends up doing with the code. I could also
> have on optionally take an int, which is the number of expected DChecks being
> performed in the block and use a Map instead of a Set for that purpose. Would
> having this system in place work (assuming proguard acts I suggested) or do
you
> think this adds too much extra code?
ProGuard best practices suggest that you guard the logs with Log.isLoggable().
This is supposed to be the solution to this problem.
On 2016/08/16 18:55:49, smaier wrote:
> On 2016/08/16 18:34:18, doloffd wrote:
> > > - I think DCheck is a more valuable addition than Check (and a potential
> > > replacement for assert) but I'm still concerned about it. Developers don't
> > > expect it to add overhead in release but it sounds like it currently
would.
> > > Sam's CL is nice in that it ensures DCheck itself doesn't add cost but
> doesn't
> > > help with evaluating parameters which is likely more expensive. He was
going
> > to
> > > check internal proguard to see if there's any help there but without it,
I'm
> > > still concerned
> >
> > One final alternative I can see for DChecks, which is somewhat ugly but
> ensures
> > proper use could be to an extra method to DCheck
> >
> > // Debug
> > private static final Set<Long> sOnCheckThreadIdSet =
> > Collections.synchronizedSet<Long>(new HashSet<Long>());
> >
> > public static boolean on() {
> > final Long threadId = Thread.currentThread().getId();
> > if (sOnCheckThreadIdSet.contains(threadId)) {
> > fail("You already called on() but didn't perform any checks");
> > }
> > sOnCheckThreadIdSet.put(threadId());
> > return true;
> > }
> >
> > public static void isCase(...) {
> > final Long threadId = Thread.currentThread().getId();
> > if (!sOnCheckThreadIdSet.contains(threadId)) {
> > fail("Always check if DCheck is on first");
> > }
> > sOnCheckThreadIdSet.remove(threadId);
> > // Do checking
> > }
> >
> > // Release
> >
> > @RemoveableInRelease
> > public static boolean on() {
> > return false;
> > }
> >
> > @RemoveableInRelease
> > public static void isCase(...) { }
> >
> > The usage would look like this
> >
> > if (DCheck.on()) DCheck.isTrue(thing.expensive(), "Log statement %s",
> > thing.name());
> >
> > I believe proguard can inline return values and if it has a guaranteed
return
> > value, it should be able to remove if (false) { ... } blocks. I can try and
> > implement it and see what proguard ends up doing with the code. I could
also
> > have on optionally take an int, which is the number of expected DChecks
being
> > performed in the block and use a Map instead of a Set for that purpose.
Would
> > having this system in place work (assuming proguard acts I suggested) or do
> you
> > think this adds too much extra code?
>
> ProGuard best practices suggest that you guard the logs with Log.isLoggable().
> This is supposed to be the solution to this problem.
This is for logging - I'm not suggesting we guard our DChecks with isLoggable.
What I meant is that there is precedent for dealing with this issue by guarding
calls to debug only functions with flags that are turned off in release.
On 2016/08/16 18:55:49, smaier wrote:
> On 2016/08/16 18:34:18, doloffd wrote:
> > > - I think DCheck is a more valuable addition than Check (and a potential
> > > replacement for assert) but I'm still concerned about it. Developers don't
> > > expect it to add overhead in release but it sounds like it currently
would.
> > > Sam's CL is nice in that it ensures DCheck itself doesn't add cost but
> doesn't
> > > help with evaluating parameters which is likely more expensive. He was
going
> > to
> > > check internal proguard to see if there's any help there but without it,
I'm
> > > still concerned
> >
> > One final alternative I can see for DChecks, which is somewhat ugly but
> ensures
> > proper use could be to an extra method to DCheck
> >
> > // Debug
> > private static final Set<Long> sOnCheckThreadIdSet =
> > Collections.synchronizedSet<Long>(new HashSet<Long>());
> >
> > public static boolean on() {
> > final Long threadId = Thread.currentThread().getId();
> > if (sOnCheckThreadIdSet.contains(threadId)) {
> > fail("You already called on() but didn't perform any checks");
> > }
> > sOnCheckThreadIdSet.put(threadId());
> > return true;
> > }
> >
> > public static void isCase(...) {
> > final Long threadId = Thread.currentThread().getId();
> > if (!sOnCheckThreadIdSet.contains(threadId)) {
> > fail("Always check if DCheck is on first");
> > }
> > sOnCheckThreadIdSet.remove(threadId);
> > // Do checking
> > }
> >
> > // Release
> >
> > @RemoveableInRelease
> > public static boolean on() {
> > return false;
> > }
> >
> > @RemoveableInRelease
> > public static void isCase(...) { }
> >
> > The usage would look like this
> >
> > if (DCheck.on()) DCheck.isTrue(thing.expensive(), "Log statement %s",
> > thing.name());
> >
> > I believe proguard can inline return values and if it has a guaranteed
return
> > value, it should be able to remove if (false) { ... } blocks. I can try and
> > implement it and see what proguard ends up doing with the code. I could
also
> > have on optionally take an int, which is the number of expected DChecks
being
> > performed in the block and use a Map instead of a Set for that purpose.
Would
> > having this system in place work (assuming proguard acts I suggested) or do
> you
> > think this adds too much extra code?
>
> ProGuard best practices suggest that you guard the logs with Log.isLoggable().
> This is supposed to be the solution to this problem.
so then ya - it seems like this would be the way to go. It's kind of unfortunate
but seems like the one with no overhead. Why do you need the threadid checks?
i don't think we should worry about an integer variant.
On 2016/08/17 17:09:01, Yaron (limited availability) wrote:
> On 2016/08/16 18:55:49, smaier wrote:
> > On 2016/08/16 18:34:18, doloffd wrote:
> > > > - I think DCheck is a more valuable addition than Check (and a potential
> > > > replacement for assert) but I'm still concerned about it. Developers
don't
> > > > expect it to add overhead in release but it sounds like it currently
> would.
> > > > Sam's CL is nice in that it ensures DCheck itself doesn't add cost but
> > doesn't
> > > > help with evaluating parameters which is likely more expensive. He was
> going
> > > to
> > > > check internal proguard to see if there's any help there but without it,
> I'm
> > > > still concerned
> > >
> > > One final alternative I can see for DChecks, which is somewhat ugly but
> > ensures
> > > proper use could be to an extra method to DCheck
> > >
> > > // Debug
> > > private static final Set<Long> sOnCheckThreadIdSet =
> > > Collections.synchronizedSet<Long>(new HashSet<Long>());
> > >
> > > public static boolean on() {
> > > final Long threadId = Thread.currentThread().getId();
> > > if (sOnCheckThreadIdSet.contains(threadId)) {
> > > fail("You already called on() but didn't perform any checks");
> > > }
> > > sOnCheckThreadIdSet.put(threadId());
> > > return true;
> > > }
> > >
> > > public static void isCase(...) {
> > > final Long threadId = Thread.currentThread().getId();
> > > if (!sOnCheckThreadIdSet.contains(threadId)) {
> > > fail("Always check if DCheck is on first");
> > > }
> > > sOnCheckThreadIdSet.remove(threadId);
> > > // Do checking
> > > }
> > >
> > > // Release
> > >
> > > @RemoveableInRelease
> > > public static boolean on() {
> > > return false;
> > > }
> > >
> > > @RemoveableInRelease
> > > public static void isCase(...) { }
> > >
> > > The usage would look like this
> > >
> > > if (DCheck.on()) DCheck.isTrue(thing.expensive(), "Log statement %s",
> > > thing.name());
> > >
> > > I believe proguard can inline return values and if it has a guaranteed
> return
> > > value, it should be able to remove if (false) { ... } blocks. I can try
and
> > > implement it and see what proguard ends up doing with the code. I could
> also
> > > have on optionally take an int, which is the number of expected DChecks
> being
> > > performed in the block and use a Map instead of a Set for that purpose.
> Would
> > > having this system in place work (assuming proguard acts I suggested) or
do
> > you
> > > think this adds too much extra code?
> >
> > ProGuard best practices suggest that you guard the logs with
Log.isLoggable().
> > This is supposed to be the solution to this problem.
>
> so then ya - it seems like this would be the way to go. It's kind of
unfortunate
> but seems like the one with no overhead. Why do you need the threadid checks?
> i don't think we should worry about an integer variant.
The thread ID stuff looks like it's intended as a way to verify that all debug
checks are preceded by a "are debug checks on" call. If we wanted to do that, we
should probably use actual TLS instead of a synchronizedSet :)
> The thread ID stuff looks like it's intended as a way to verify that all debug > checks are preceded by a "are debug checks on" call. If we wanted to do that, we > should probably use actual TLS instead of a synchronizedSet :) I'm not sure what you mean by TLS? Do you mean make the set a regular Set and then make all the methods synchronized? I think that would slow down the code more than just synchronizing the set itself as all access of the set should be synchronized anyway but now more code will be within the synchronized blocks. In addition to that, it would further increase the differences between the Debug and Release versions of the class. Did I misunderstand what you meant?
On 2016/08/17 19:44:00, doloffd wrote: > > The thread ID stuff looks like it's intended as a way to verify that all debug > > checks are preceded by a "are debug checks on" call. If we wanted to do that, > we > > should probably use actual TLS instead of a synchronizedSet :) > > I'm not sure what you mean by TLS? Do you mean make the set a regular Set and > then make all the methods synchronized? I think that would slow down the code > more than just synchronizing the set itself as all access of the set should be > synchronized anyway but now more code will be within the synchronized blocks. In > addition to that, it would further increase the differences between the Debug > and Release versions of the class. Did I misunderstand what you meant? Can we just do: if (BuildConfig.isDebug) DCheck...
On 2016/08/17 19:51:54, Yaron (limited availability) wrote: > On 2016/08/17 19:44:00, doloffd wrote: > > > The thread ID stuff looks like it's intended as a way to verify that all > debug > > > checks are preceded by a "are debug checks on" call. If we wanted to do > that, > > we > > > should probably use actual TLS instead of a synchronizedSet :) > > > > I'm not sure what you mean by TLS? Do you mean make the set a regular Set and > > then make all the methods synchronized? I think that would slow down the code > > more than just synchronizing the set itself as all access of the set should be > > synchronized anyway but now more code will be within the synchronized blocks. > In > > addition to that, it would further increase the differences between the Debug > > and Release versions of the class. Did I misunderstand what you meant? > > Can we just do: > if (BuildConfig.isDebug) DCheck... That would work too, the concern I would have with it would be that if we wanted to add a command-line flag, to force DChecks on or off independent of the build type, checks that use that pattern would not be responsive to that flag. In addition to that, if we leave it as a function, then we'll be able to use the trick I proposed to help ensure developers always wrap their DChecks in an if-statement. Are these reasons not strong enough to keep it as a function? I compiled the release build of chrome_public_apk and proguard did not inline the return value or strip out the if-block even using @RemovableInRelease. Using BuildConfig.isDebug will compile out the code so that is definitely an advantage to using a constant over a function. Let me know which you prefer and if Check is stable enough now that I can re-add the DCheck class.
On 2016/08/17 21:55:02, doloffd wrote: > On 2016/08/17 19:51:54, Yaron (limited availability) wrote: > > On 2016/08/17 19:44:00, doloffd wrote: > > > > The thread ID stuff looks like it's intended as a way to verify that all > > debug > > > > checks are preceded by a "are debug checks on" call. If we wanted to do > > that, > > > we > > > > should probably use actual TLS instead of a synchronizedSet :) > > > > > > I'm not sure what you mean by TLS? Do you mean make the set a regular Set > and > > > then make all the methods synchronized? I think that would slow down the > code > > > more than just synchronizing the set itself as all access of the set should > be > > > synchronized anyway but now more code will be within the synchronized > blocks. > > In > > > addition to that, it would further increase the differences between the > Debug > > > and Release versions of the class. Did I misunderstand what you meant? > > > > Can we just do: > > if (BuildConfig.isDebug) DCheck... > > That would work too, the concern I would have with it would be that if we wanted > to add a command-line flag, to force DChecks on or off independent of the build > type, checks that use that pattern would not be responsive to that flag. I think you can already do that by setting dcheck_always_on = true (in GN) In > addition to that, if we leave it as a function, then we'll be able to use the > trick I proposed to help ensure developers always wrap their DChecks in an > if-statement. Are these reasons not strong enough to keep it as a function? > > I compiled the release build of chrome_public_apk and proguard did not inline > the return value or strip out the if-block even using @RemovableInRelease. Using > BuildConfig.isDebug will compile out the code so that is definitely an advantage > to using a constant over a function. If you get the function compiled out, then I'm supportive. Otherwise, I'd prefer it to remain a constant. > > Let me know which you prefer and if Check is stable enough now that I can re-add > the DCheck class. Sorry, what do you mean about Check being stable enough? As in the implementation of it?
On 2016/08/18 17:10:18, Yaron (limited availability) wrote: > On 2016/08/17 21:55:02, doloffd wrote: > > On 2016/08/17 19:51:54, Yaron (limited availability) wrote: > > > On 2016/08/17 19:44:00, doloffd wrote: > > > > > The thread ID stuff looks like it's intended as a way to verify that all > > > debug > > > > > checks are preceded by a "are debug checks on" call. If we wanted to do > > > that, > > > > we > > > > > should probably use actual TLS instead of a synchronizedSet :) > > > > > > > > I'm not sure what you mean by TLS? Do you mean make the set a regular Set > > and > > > > then make all the methods synchronized? I think that would slow down the > > code > > > > more than just synchronizing the set itself as all access of the set > should > > be > > > > synchronized anyway but now more code will be within the synchronized > > blocks. > > > In > > > > addition to that, it would further increase the differences between the > > Debug > > > > and Release versions of the class. Did I misunderstand what you meant? > > > > > > Can we just do: > > > if (BuildConfig.isDebug) DCheck... > > > > That would work too, the concern I would have with it would be that if we > wanted > > to add a command-line flag, to force DChecks on or off independent of the > build > > type, checks that use that pattern would not be responsive to that flag. > > I think you can already do that by setting dcheck_always_on = true (in GN) Would you prefer I expose it as // BuildConfig.template #if defined(DCHECK_ALWAYS_ON) || defined(DEBUG) public static final boolean DCHECK_ON = true; #else public static final boolean DCHECK_ON = false; or // BuildConfig.template #if defined(DCHECK_ALWAYS_ON) public static final boolean DCHECK_ALWAYS_ON = true; #else public static final boolean DCHECK_ALWAYS_ON = false; #endif // DCheck.java public static final boolean IS_ON = BuildConfig.DCHECK_ALWAYS_ON || BuildConfig.DEBUG; > > Let me know which you prefer and if Check is stable enough now that I can > re-add > > the DCheck class. > > Sorry, what do you mean about Check being stable enough? As in the > implementation of it? I was unclear, yes I meant the code and the method signatures were unlikely to change so I can copy the methods over.
On 2016/08/18 17:46:30, doloffd wrote: > On 2016/08/18 17:10:18, Yaron (limited availability) wrote: > > On 2016/08/17 21:55:02, doloffd wrote: > > > On 2016/08/17 19:51:54, Yaron (limited availability) wrote: > > > > On 2016/08/17 19:44:00, doloffd wrote: > > > > > > The thread ID stuff looks like it's intended as a way to verify that > all > > > > debug > > > > > > checks are preceded by a "are debug checks on" call. If we wanted to > do > > > > that, > > > > > we > > > > > > should probably use actual TLS instead of a synchronizedSet :) > > > > > > > > > > I'm not sure what you mean by TLS? Do you mean make the set a regular > Set > > > and > > > > > then make all the methods synchronized? I think that would slow down the > > > code > > > > > more than just synchronizing the set itself as all access of the set > > should > > > be > > > > > synchronized anyway but now more code will be within the synchronized > > > blocks. > > > > In > > > > > addition to that, it would further increase the differences between the > > > Debug > > > > > and Release versions of the class. Did I misunderstand what you meant? > > > > > > > > Can we just do: > > > > if (BuildConfig.isDebug) DCheck... > > > > > > That would work too, the concern I would have with it would be that if we > > wanted > > > to add a command-line flag, to force DChecks on or off independent of the > > build > > > type, checks that use that pattern would not be responsive to that flag. > > > > I think you can already do that by setting dcheck_always_on = true (in GN) > > Would you prefer I expose it as > > // BuildConfig.template > > #if defined(DCHECK_ALWAYS_ON) || defined(DEBUG) > public static final boolean DCHECK_ON = true; > #else > public static final boolean DCHECK_ON = false; > > or > > // BuildConfig.template > > #if defined(DCHECK_ALWAYS_ON) > public static final boolean DCHECK_ALWAYS_ON = true; > #else > public static final boolean DCHECK_ALWAYS_ON = false; > #endif > > // DCheck.java > > public static final boolean IS_ON = BuildConfig.DCHECK_ALWAYS_ON || > BuildConfig.DEBUG; > > > > > Let me know which you prefer and if Check is stable enough now that I can > > re-add > > > the DCheck class. > > > > Sorry, what do you mean about Check being stable enough? As in the > > implementation of it? > > I was unclear, yes I meant the code and the method signatures were unlikely to > change so I can copy the methods over. Sorry to come in late to this. Didn't notice that progress was being made :S. I just updated the bug with another idea of how to make asserts work: https://bugs.chromium.org/p/chromium/issues/detail?id=462676 wdyt about that approach? If we still want to add a custom class for this, I'd strongly prefer it to have just a single Check.isTrue() method and nothing else. That way the code size impact will be minimal.
See recent comment on crbug.com/462676 for yet another option for debug assertions. We should probably discuss this "centrally" somewhere to decide :)
Since we're going ahead in crbug.com/462676 with an assert-statement based approach for debug only checks, I'm not sure whether proceeding with this CL is worthwhile - we wouldn't use DCheck, and given the asymmetry I'm not sure if Check is particularly valuable versus the current approach of just throwing exceptions. Is anyone still interested in pursuing this, or should it be closed? |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
