Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1188623002:
Avoid redundant Looper tracing (Closed)
|
Created:
5 years, 6 months ago by paulmiller Modified:
5 years, 6 months ago CC:
chromium-reviews, erikwright+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAvoid redundant Looper tracing
internal bug 21374535
Committed: https://crrev.com/76de16b533909638d2f0eec8f8d632cd0f7ec6b6
Cr-Commit-Position: refs/heads/master@{#335180}
Patch Set 1 #
Total comments: 11
Patch Set 2 : prettify comments #
Total comments: 5
Patch Set 3 : sATraceEnabled conditional #Patch Set 4 : prettify comments #
Total comments: 1
Patch Set 5 : you have got to be kidding me #
Total comments: 3
Patch Set 6 : early-out #Messages
Total messages: 30 (7 generated)
paulmiller@chromium.org changed reviewers: + boliu@chromium.org
PTAL
boliu@chromium.org changed reviewers: + nyquist@chromium.org
+nyquist for owners lg2m
https://codereview.chromium.org/1188623002/diff/1/base/android/java/src/org/c... File base/android/java/src/org/chromium/base/TraceEvent.java (right): https://codereview.chromium.org/1188623002/diff/1/base/android/java/src/org/c... base/android/java/src/org/chromium/base/TraceEvent.java:23: private static volatile boolean sATraceEnabled = false; What is ATrace? Could you add documentation? https://codereview.chromium.org/1188623002/diff/1/base/android/java/src/org/c... base/android/java/src/org/chromium/base/TraceEvent.java:178: public static void setEnabled(boolean enabled) { If this is only supposed to be called from native, could this be private? https://codereview.chromium.org/1188623002/diff/1/base/android/java/src/org/c... base/android/java/src/org/chromium/base/TraceEvent.java:180: // Android M+ systrace logs this on its own. We should log it only if we are not writing to Nit: Do not use pronouns in comments. https://codereview.chromium.org/1188623002/diff/1/base/android/java/src/org/c... base/android/java/src/org/chromium/base/TraceEvent.java:195: sATraceEnabled = enabled; If I do: setEnabled(true); setATraceEnabled(true); this method will bail out early, and sATraceEnabled will not be set. Is that intentional?
https://codereview.chromium.org/1188623002/diff/1/base/android/java/src/org/c... File base/android/java/src/org/chromium/base/TraceEvent.java (right): https://codereview.chromium.org/1188623002/diff/1/base/android/java/src/org/c... base/android/java/src/org/chromium/base/TraceEvent.java:178: public static void setEnabled(boolean enabled) { On 2015/06/15 23:25:17, nyquist wrote: > If this is only supposed to be called from native, could this be private? Not really. In theory chrome trace can be turned on from java code. It's just no caller (that can I find from 5 seconds of grepping). Probably doesn't hurt. https://codereview.chromium.org/1188623002/diff/1/base/android/java/src/org/c... base/android/java/src/org/chromium/base/TraceEvent.java:195: sATraceEnabled = enabled; On 2015/06/15 23:25:17, nyquist wrote: > If I do: > > setEnabled(true); > setATraceEnabled(true); > > this method will bail out early, and sATraceEnabled will not be set. Is that > intentional? Can assume chrome trace and android systrace will never happen at the same time (since it's just not supported period). But this does look bad. Should update line 194 to sATraceEnabled
https://codereview.chromium.org/1188623002/diff/1/base/android/java/src/org/c... File base/android/java/src/org/chromium/base/TraceEvent.java (right): https://codereview.chromium.org/1188623002/diff/1/base/android/java/src/org/c... base/android/java/src/org/chromium/base/TraceEvent.java:23: private static volatile boolean sATraceEnabled = false; On 2015/06/15 23:25:17, nyquist wrote: > What is ATrace? Could you add documentation? Acknowledged. https://codereview.chromium.org/1188623002/diff/1/base/android/java/src/org/c... base/android/java/src/org/chromium/base/TraceEvent.java:178: public static void setEnabled(boolean enabled) { Either way, it's sort of outside this change. https://codereview.chromium.org/1188623002/diff/1/base/android/java/src/org/c... base/android/java/src/org/chromium/base/TraceEvent.java:180: // Android M+ systrace logs this on its own. We should log it only if we are not writing to On 2015/06/15 23:25:17, nyquist wrote: > Nit: Do not use pronouns in comments. Acknowledged. https://codereview.chromium.org/1188623002/diff/1/base/android/java/src/org/c... base/android/java/src/org/chromium/base/TraceEvent.java:195: sATraceEnabled = enabled; I'm just trying to be consistent with the current behavior. I don't know why the original author decided to silently no-op in this case. Of course, if you call setEnabled first, then the Looper logging has already been set.
nyquist@chromium.org changed reviewers: + hush@chromium.org
hush: Any thoughts on the behavior you added in https://codereview.chromium.org/247143006 and the comments in this CL?
https://codereview.chromium.org/1188623002/diff/1/base/android/java/src/org/c... File base/android/java/src/org/chromium/base/TraceEvent.java (right): https://codereview.chromium.org/1188623002/diff/1/base/android/java/src/org/c... base/android/java/src/org/chromium/base/TraceEvent.java:195: sATraceEnabled = enabled; On 2015/06/15 23:48:17, paulmiller wrote: > I'm just trying to be consistent with the current behavior. I don't know why the > original author decided to silently no-op in this case. Of course, if you call > setEnabled first, then the Looper logging has already been set. The early exit was just to save unnecessary work. I am the person who added line 194 above, at which time there was no extra state "sATraceEnabled". There was only sEnabled. So with this new state introduced, I think what Bo and Tommy said makes sense: we should set sATraceEnabled in line 194, and not early out.
You need to comment when you upload a new patch set. Unlike gerrit, uploading a patch set does *not* send mail to anyway https://codereview.chromium.org/1188623002/diff/20001/base/android/java/src/o... File base/android/java/src/org/chromium/base/TraceEvent.java (right): https://codereview.chromium.org/1188623002/diff/20001/base/android/java/src/o... base/android/java/src/org/chromium/base/TraceEvent.java:23: // Write Chrome traces to Android systrace? Question in comment doesn't really help, even a rhetorical one.. https://codereview.chromium.org/1188623002/diff/20001/base/android/java/src/o... base/android/java/src/org/chromium/base/TraceEvent.java:194: if (sEnabled == enabled) return; can this be sATraceEnabled == enabled?
https://codereview.chromium.org/1188623002/diff/20001/base/android/java/src/o... File base/android/java/src/org/chromium/base/TraceEvent.java (right): https://codereview.chromium.org/1188623002/diff/20001/base/android/java/src/o... base/android/java/src/org/chromium/base/TraceEvent.java:23: // Write Chrome traces to Android systrace? This comment describes the purpose of sATraceEnabled, as per Tom's request. If you'd like a different wording, please suggest one. https://codereview.chromium.org/1188623002/diff/20001/base/android/java/src/o... base/android/java/src/org/chromium/base/TraceEvent.java:194: if (sEnabled == enabled) return; On 2015/06/17 16:22:16, boliu wrote: > can this be sATraceEnabled == enabled? Acknowledged.
https://codereview.chromium.org/1188623002/diff/20001/base/android/java/src/o... File base/android/java/src/org/chromium/base/TraceEvent.java (right): https://codereview.chromium.org/1188623002/diff/20001/base/android/java/src/o... base/android/java/src/org/chromium/base/TraceEvent.java:23: // Write Chrome traces to Android systrace? On 2015/06/18 17:43:51, paulmiller wrote: > This comment describes the purpose of sATraceEnabled, as per Tom's request. If > you'd like a different wording, please suggest one. "True when taking an Android systrace"
new patch set
tommy: ptal https://codereview.chromium.org/1188623002/diff/60001/base/android/java/src/o... File base/android/java/src/org/chromium/base/TraceEvent.java (right): https://codereview.chromium.org/1188623002/diff/60001/base/android/java/src/o... base/android/java/src/org/chromium/base/TraceEvent.java:23: private static volatile boolean sATraceEnabled = false; // True when taking an Android systrace period at end of comment.
This is the last nitpick I will entertain. Take it or leave it.
lgtm https://codereview.chromium.org/1188623002/diff/80001/base/android/java/src/o... File base/android/java/src/org/chromium/base/TraceEvent.java (right): https://codereview.chromium.org/1188623002/diff/80001/base/android/java/src/o... base/android/java/src/org/chromium/base/TraceEvent.java:181: if (!sATraceEnabled) { Could you change this to if (sATraceEnabled) return; ? I find non-indented code easier to read.
The CQ bit was checked by paulmiller@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1188623002/80001
The CQ bit was unchecked by boliu@chromium.org
Wait, fix Tommy's last comment first. That wasn't optional
I don't mean to be rude, but I honestly can't tell if I'm being trolled here. If you don't like indentation, I don't know what to tell you. I already said I will not entertain any more pointless nitpicking. I give up.
Message was sent while issue was closed.
On 2015/06/18 22:10:06, paulmiller wrote: > I don't mean to be rude, but I honestly can't tell if I'm being trolled here. If > you don't like indentation, I don't know what to tell you. I already said I will > not entertain any more pointless nitpicking. I give up. It's not a joke. Keeping consistent style and high quality comments is important for long term maintenability of the code. Everything else in chrome depends on base/, so quality bar for base is super high.
I apologize for my impatience. Here's my reasoning. https://codereview.chromium.org/1188623002/diff/80001/base/android/java/src/o... File base/android/java/src/org/chromium/base/TraceEvent.java (right): https://codereview.chromium.org/1188623002/diff/80001/base/android/java/src/o... base/android/java/src/org/chromium/base/TraceEvent.java:181: if (!sATraceEnabled) { The sATraceEnabled check is specifically for the code inside the if body. Indentation makes this relationship clear. If we did an early-out, and someone were to add some code to this function in the future, they would have to change it back to an indented block because the new code would probably not depend on sATraceEnabled.
boliu is right in that comments that are not marked as optional should not be ignored. Anyway; I don't think any of us would want to spend more time on this than necessary, and if you want to land the code as is that would be OK with me this time. https://codereview.chromium.org/1188623002/diff/80001/base/android/java/src/o... File base/android/java/src/org/chromium/base/TraceEvent.java (right): https://codereview.chromium.org/1188623002/diff/80001/base/android/java/src/o... base/android/java/src/org/chromium/base/TraceEvent.java:181: if (!sATraceEnabled) { On 2015/06/18 23:07:05, paulmiller wrote: > The sATraceEnabled check is specifically for the code inside the if body. > Indentation makes this relationship clear. If we did an early-out, and someone > were to add some code to this function in the future, they would have to change > it back to an indented block because the new code would probably not depend on > sATraceEnabled. Yes, they would have to change the code at that point. That is not how the code looks like now though. Luckily this setter-method hasn't become a cesspool of hard to understand constructs yet, so that someone who might change it in the future should be able to do so even with an early out here. Unless it leads us very much down the wrong path, code in chromium is usually written for the present, or at least very near or planned future (say as part of a bugfix/feature that is implemented across several CLs). I didn't know of any upcoming plans of using this setter for more than is already the case, which is why I added the comment.
The CQ bit was checked by paulmiller@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nyquist@chromium.org Link to the patchset: https://codereview.chromium.org/1188623002/#ps100001 (title: "early-out")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1188623002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/76de16b533909638d2f0eec8f8d632cd0f7ec6b6 Cr-Commit-Position: refs/heads/master@{#335180} |
