Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1486053003:
Android: Add support for TraceEvent before the native library is loaded. (Closed)
DescriptionAndroid: Add support for TraceEvent before the native library is loaded.
Currently TraceEvents recorded before the native library load are
silently discarded. This CL adds support for such "early" events.
The events are buffered in Java, and then sent to the native side once
it is available. This support is intentionally simple now. The aim is to
ease performance investigation of early startup, and to ease tracking of
early startup performance regression, since timings are then accessible
to TBM tests.
Also use the newly-introduced events in a couple places, and fix a bug
in the TraceEvent we use to record the launcher activity lifetime (when
an Activity calls finish from onCreate(), onPause() is not called).
BUG=564041
Committed: https://crrev.com/fb0784294541c53f12e283124672750ab2e291f1
Cr-Commit-Position: refs/heads/master@{#404635}
Patch Set 1 #Patch Set 2 : Proper locking. #
Total comments: 18
Patch Set 3 : Address comments. #Patch Set 4 : Rebase. #Patch Set 5 : Add a comment. #
Total comments: 14
Patch Set 6 : Address comments. #Patch Set 7 : . #Patch Set 8 : . #Patch Set 9 : Rebase. #
Total comments: 4
Patch Set 10 : Rebase + comments. #
Total comments: 4
Patch Set 11 : Address comments. #Patch Set 12 : Rebase + fix test. #Messages
Total messages: 45 (15 generated)
lizeb@chromium.org changed reviewers: + pasko@chromium.org
Hello pasko, Here is a CL introducing trace events in Java before the native library is loaded. Compared with the previous CL doing that (https://codereview.chromium.org/874543003/), the main difference is that the native side change are much smaller. This is because instead of the native side pulling the Java TraceEvents in one call, the java side is pushing them to the native side, one call per event. This simplifies the code (since there is no need to do an array-of-structs to struct-of-array style conversion), at the cost of some efficiency, but I believe that this is not critical, since we can only cram so many events in the few hundreds of ms before the native library is loaded. Furthermore, if this turns about to be an issue, we can still improve it later. Finally, usual TraceEvents require two JNI calls per event, so the total cost should not be much higher. PTAL, thanks.
I would like to postpone reviewing this to next week, when we have The Fixit. Please ;)
I am wondering whether we should/can do the change in TraceEvent.setEnabled() differently. See the corresponding comment below. Other suggestions are more straightforward or trivial, or just nits. https://codereview.chromium.org/1486053003/diff/20001/base/android/java/src/o... File base/android/java/src/org/chromium/base/EarlyTraceEvent.java (right): https://codereview.chromium.org/1486053003/diff/20001/base/android/java/src/o... base/android/java/src/org/chromium/base/EarlyTraceEvent.java:22: * - Two events with the same name can be in progress at the same time. s/can/cannot/ https://codereview.chromium.org/1486053003/diff/20001/base/android/java/src/o... base/android/java/src/org/chromium/base/EarlyTraceEvent.java:107: // TODO(lizeb): support multiple events with the same name if necessary. We do not support it for "EarlyJava", and we think it is a reasonable tradeoff. So please remove this TODO, as it is not really a TODO. https://codereview.chromium.org/1486053003/diff/20001/base/android/java/src/o... File base/android/java/src/org/chromium/base/TraceEvent.java (right): https://codereview.chromium.org/1486053003/diff/20001/base/android/java/src/o... base/android/java/src/org/chromium/base/TraceEvent.java:20: * be ignored. (Perhaps we could devise to buffer them up in future?). after the change this comment will no longer be correct https://codereview.chromium.org/1486053003/diff/20001/base/android/java/src/o... base/android/java/src/org/chromium/base/TraceEvent.java:182: if (enabled) EarlyTraceEvent.disable(); Some observations: 1. we may miss a few early traces this way (early is off, regular tracing is not on yet), though the flakiness would be rare 2. someone may call this function from Java without realizing that it can stop tracing _sometimes_. Probably a comment at the top of this method would be sufficient: "Does not work correctly when called from Java". 3. it makes it quite possible for setEnabled(true) to take a long time so that in the meantime setEnabled(false) can execute, what a confusion I think we cannot solve (1) in a robust way, unfortunately. Something that seems to solve (2) and (3) (and maybe slightly better (1)) is in setEnabled(true): * crash if native library is not loaded * ONCE: atomically (with barriers, etc etc) switch the trace collection mechanism from early traces to regular ones * ONCE: _asynchronously_ dump the "EarlyJava" traces. We would not cause any problems when doing a dump because the new trace events would have all different categories. Some racy behavior: when we begin in EarlyJava and end in Java, the events would be broken. We can probably "fixup" them when writing the output trace somewhere, not in this change. WDYT? I might be missing something this evening. https://codereview.chromium.org/1486053003/diff/20001/base/base.gypi File base/base.gypi (right): https://codereview.chromium.org/1486053003/diff/20001/base/base.gypi#newcode41 base/base.gypi:41: 'android/early_trace_event_binding.cc', the usual naming convention seems to be like: early_trace_event_android.cc also, in chromium codebase "binding" is a bit overloaded please rename https://codereview.chromium.org/1486053003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java (right): https://codereview.chromium.org/1486053003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java:208: if (CommandLine.getInstance().hasSwitch("trace-startup")) { This should also check for /data/local/chrome-trace-config.json. I *think* we can speculatively start early tracing if the file exists and later based on the contents decide whether we need the data. Another option: do early tracing all the time, there are not too many events to collect (yet). See for details: https://docs.google.com/document/d/1yRCXhrQ-0rsfUgNHt9T4YdnmJYrXKN6aK56Ozk3kP... https://codereview.chromium.org/1486053003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/document/ChromeLauncherActivity.java (right): https://codereview.chromium.org/1486053003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/document/ChromeLauncherActivity.java:232: public void onDestroy() { changing onPause() to onDestroy() looks like fixing a separate bug. Did it break something for you? I would prefer landing this separately to avoid risks, prefer to revert small things than large (you know, android app lifetime == complicated) https://codereview.chromium.org/1486053003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitializationActivity.java (right): https://codereview.chromium.org/1486053003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitializationActivity.java:186: } finally { I think a separate method would be slightly more readable: TraceEvent.begin("AsyncInitializationActivity.onCreate()"); onCreateInternal(savedInstanceState); TraceEvent.end("AsyncInitializationActivity.onCreate()"); throwing an exception from any of the invoked stuff here is unlikely, and compared to that a broken TraceEvent is not a problem. this is somewhat minor, open for debate
Thanks for the review. Sorry about the large diff, I should have rebased the patch separately... PTAL. https://codereview.chromium.org/1486053003/diff/20001/base/android/java/src/o... File base/android/java/src/org/chromium/base/EarlyTraceEvent.java (right): https://codereview.chromium.org/1486053003/diff/20001/base/android/java/src/o... base/android/java/src/org/chromium/base/EarlyTraceEvent.java:22: * - Two events with the same name can be in progress at the same time. On 2015/12/15 17:52:59, pasko wrote: > s/can/cannot/ Good catch, thanks! Done. https://codereview.chromium.org/1486053003/diff/20001/base/android/java/src/o... base/android/java/src/org/chromium/base/EarlyTraceEvent.java:107: // TODO(lizeb): support multiple events with the same name if necessary. On 2015/12/15 17:52:59, pasko wrote: > We do not support it for "EarlyJava", and we think it is a reasonable tradeoff. > > So please remove this TODO, as it is not really a TODO. Done. https://codereview.chromium.org/1486053003/diff/20001/base/android/java/src/o... File base/android/java/src/org/chromium/base/TraceEvent.java (right): https://codereview.chromium.org/1486053003/diff/20001/base/android/java/src/o... base/android/java/src/org/chromium/base/TraceEvent.java:20: * be ignored. (Perhaps we could devise to buffer them up in future?). On 2015/12/15 17:52:59, pasko wrote: > after the change this comment will no longer be correct Done. https://codereview.chromium.org/1486053003/diff/20001/base/android/java/src/o... base/android/java/src/org/chromium/base/TraceEvent.java:182: if (enabled) EarlyTraceEvent.disable(); On 2015/12/15 17:52:59, pasko wrote: > Some observations: > > 1. we may miss a few early traces this way (early is off, regular tracing is not > on yet), though the flakiness would be rare > > 2. someone may call this function from Java without realizing that it can stop > tracing _sometimes_. Probably a comment at the top of this method would be > sufficient: "Does not work correctly when called from Java". > > 3. it makes it quite possible for setEnabled(true) to take a long time so that > in the meantime setEnabled(false) can execute, what a confusion > > I think we cannot solve (1) in a robust way, unfortunately. > > Something that seems to solve (2) and (3) (and maybe slightly better (1)) is in > setEnabled(true): > > * crash if native library is not loaded > > * ONCE: atomically (with barriers, etc etc) switch the trace collection > mechanism from early traces to regular ones > > * ONCE: _asynchronously_ dump the "EarlyJava" traces. We would not cause any > problems when doing a dump because the new trace events would have all different > categories. > > Some racy behavior: when we begin in EarlyJava and end in Java, the events would > be broken. We can probably "fixup" them when writing the output trace somewhere, > not in this change. > > WDYT? I might be missing something this evening. Per offline discussion, my understanding is that we agree that the current solution is fine. https://codereview.chromium.org/1486053003/diff/20001/base/base.gypi File base/base.gypi (right): https://codereview.chromium.org/1486053003/diff/20001/base/base.gypi#newcode41 base/base.gypi:41: 'android/early_trace_event_binding.cc', On 2015/12/15 17:52:59, pasko wrote: > the usual naming convention seems to be like: > early_trace_event_android.cc > > also, in chromium codebase "binding" is a bit overloaded > > please rename This is named this way to be consistent with base/android/trace_event_binding.cc. So I would prefer to keep the naming as it is. https://codereview.chromium.org/1486053003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java (right): https://codereview.chromium.org/1486053003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java:208: if (CommandLine.getInstance().hasSwitch("trace-startup")) { On 2015/12/15 17:52:59, pasko wrote: > This should also check for /data/local/chrome-trace-config.json. > > I *think* we can speculatively start early tracing if the file exists and later > based on the contents decide whether we need the data. > > Another option: do early tracing all the time, there are not too many events to > collect (yet). > > See for details: > https://docs.google.com/document/d/1yRCXhrQ-0rsfUgNHt9T4YdnmJYrXKN6aK56Ozk3kP... Done. https://codereview.chromium.org/1486053003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/document/ChromeLauncherActivity.java (right): https://codereview.chromium.org/1486053003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/document/ChromeLauncherActivity.java:232: public void onDestroy() { On 2015/12/15 17:52:59, pasko wrote: > changing onPause() to onDestroy() looks like fixing a separate bug. Did it break > something for you? > Yes, as mentioned in the CL description. Basically, the "end" was not always called. > I would prefer landing this separately to avoid risks, prefer to revert small > things than large (you know, android app lifetime == complicated) This is code that I (oops) added, and since the method doesn't do anything else aside from calling super(), this is fine I guess. And it only matters starting from this patch. https://codereview.chromium.org/1486053003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitializationActivity.java (right): https://codereview.chromium.org/1486053003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitializationActivity.java:186: } finally { On 2015/12/15 17:52:59, pasko wrote: > I think a separate method would be slightly more readable: > > TraceEvent.begin("AsyncInitializationActivity.onCreate()"); > onCreateInternal(savedInstanceState); > TraceEvent.end("AsyncInitializationActivity.onCreate()"); > > throwing an exception from any of the invoked stuff here is unlikely, and > compared to that a broken TraceEvent is not a problem. > > this is somewhat minor, open for debate You're right, throwing from onCreate() is deadly, so no need for the try { } finally { }. Thanks!
lgtm with a request for slightly more documentation about concurrency that might turn out to be tricky https://codereview.chromium.org/1486053003/diff/20001/base/android/java/src/o... File base/android/java/src/org/chromium/base/TraceEvent.java (right): https://codereview.chromium.org/1486053003/diff/20001/base/android/java/src/o... base/android/java/src/org/chromium/base/TraceEvent.java:182: if (enabled) EarlyTraceEvent.disable(); On 2016/01/06 12:50:06, Benoit L wrote: > On 2015/12/15 17:52:59, pasko wrote: > > Some observations: > > > > 1. we may miss a few early traces this way (early is off, regular tracing is > not > > on yet), though the flakiness would be rare > > > > 2. someone may call this function from Java without realizing that it can stop > > tracing _sometimes_. Probably a comment at the top of this method would be > > sufficient: "Does not work correctly when called from Java". > > > > 3. it makes it quite possible for setEnabled(true) to take a long time so that > > in the meantime setEnabled(false) can execute, what a confusion > > > > I think we cannot solve (1) in a robust way, unfortunately. > > > > Something that seems to solve (2) and (3) (and maybe slightly better (1)) is > in > > setEnabled(true): > > > > * crash if native library is not loaded > > > > * ONCE: atomically (with barriers, etc etc) switch the trace collection > > mechanism from early traces to regular ones > > > > * ONCE: _asynchronously_ dump the "EarlyJava" traces. We would not cause any > > problems when doing a dump because the new trace events would have all > different > > categories. > > > > Some racy behavior: when we begin in EarlyJava and end in Java, the events > would > > be broken. We can probably "fixup" them when writing the output trace > somewhere, > > not in this change. > > > > WDYT? I might be missing something this evening. > > Per offline discussion, my understanding is that we agree that the current > solution is fine. I do not remember the details :( Can you outline the concurrency issues at the top of EarlyTraceEvent.java with quick resolution why it is good enough? https://codereview.chromium.org/1486053003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/document/ChromeLauncherActivity.java (right): https://codereview.chromium.org/1486053003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/document/ChromeLauncherActivity.java:232: public void onDestroy() { On 2016/01/06 12:50:06, Benoit L wrote: > > I would prefer landing this separately to avoid risks, prefer to revert small > > things than large (you know, android app lifetime == complicated) > > This is code that I (oops) added, and since the method doesn't do anything else > aside from calling super(), this is fine I guess. > And it only matters starting from this patch. OK, worksforme, thank you for explanation
lizeb@chromium.org changed reviewers: + yfriedman@chromium.org
Hello yfriedman Here is a CL introducing trace events in Java before the native library is loaded. Compared with the previous CL doing that (https://codereview.chromium.org/874543003/), the main difference is that the native side change are much smaller. This is because instead of the native side pulling the Java TraceEvents in one call, the java side is pushing them to the native side, one call per event. This simplifies the code (since there is no need to do an array-of-structs to struct-of-array style conversion), at the cost of some efficiency, but I believe that this is not critical, since we can only cram so many events in the few hundreds of ms before the native library is loaded. Furthermore, if this turns about to be an issue, we can still improve it later. Finally, usual TraceEvents require two JNI calls per event, so the total cost should not be much higher. PTAL, thanks.
https://codereview.chromium.org/1486053003/diff/80001/base/android/java/src/o... File base/android/java/src/org/chromium/base/EarlyTraceEvent.java (right): https://codereview.chromium.org/1486053003/diff/80001/base/android/java/src/o... base/android/java/src/org/chromium/base/EarlyTraceEvent.java:73: /** @see TraceEvent#MaybeEnableEarlytracing(). Tracing https://codereview.chromium.org/1486053003/diff/80001/base/android/java/src/o... base/android/java/src/org/chromium/base/EarlyTraceEvent.java:77: || (new File("/data/local/chrome-trace-config.json")).exists())) { This appears to be a StrictMode violation (and disk IO) on start-up for most users. Why is it necessary? https://codereview.chromium.org/1486053003/diff/80001/base/android/java/src/o... base/android/java/src/org/chromium/base/EarlyTraceEvent.java:107: if (sState != STATE_ENABLED) return; Hmm, while I get the point of this, I'm surprised that findbugs didn't complain about inconsistent locking - have you run tryjobs? This may need to be exempted https://codereview.chromium.org/1486053003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java (right): https://codereview.chromium.org/1486053003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java:199: initCommandLine(); Can you not do this here? We're actually in the process of trying to trim down our application classes: crbug/560466 I think on the renderer this is less important too so this could live in ChromeBrowserInitializer
https://codereview.chromium.org/1486053003/diff/80001/base/android/java/src/o... File base/android/java/src/org/chromium/base/EarlyTraceEvent.java (right): https://codereview.chromium.org/1486053003/diff/80001/base/android/java/src/o... base/android/java/src/org/chromium/base/EarlyTraceEvent.java:77: || (new File("/data/local/chrome-trace-config.json")).exists())) { On 2016/01/08 19:42:18, Yaron wrote: > This appears to be a StrictMode violation (and disk IO) on start-up for most > users. Why is it necessary? Yes, this reaction matches my first thoughts as well. We decided that Telemetry should control tracing for startup by introducing this file into the filesystem. We consider that checking for file in a recently used directory is cheap. Also, Benoit told me that stat(3) without reading/writing does not trigger a StrictMode violation. I did not verify that myself. (https://docs.google.com/document/d/1yRCXhrQ-0rsfUgNHt9T4YdnmJYrXKN6aK56Ozk3kP...)
https://codereview.chromium.org/1486053003/diff/80001/base/android/java/src/o... File base/android/java/src/org/chromium/base/EarlyTraceEvent.java (right): https://codereview.chromium.org/1486053003/diff/80001/base/android/java/src/o... base/android/java/src/org/chromium/base/EarlyTraceEvent.java:77: || (new File("/data/local/chrome-trace-config.json")).exists())) { On 2016/01/08 19:51:10, pasko wrote: > On 2016/01/08 19:42:18, Yaron wrote: > > This appears to be a StrictMode violation (and disk IO) on start-up for most > > users. Why is it necessary? > > Yes, this reaction matches my first thoughts as well. > > We decided that Telemetry should control tracing for startup by introducing this > file into the filesystem. We consider that checking for file in a recently used > directory is cheap. Also, Benoit told me that stat(3) without reading/writing > does not trigger a StrictMode violation. I did not verify that myself. > > (https://docs.google.com/document/d/1yRCXhrQ-0rsfUgNHt9T4YdnmJYrXKN6aK56Ozk3kP...) Perhaps it's specific to the version of Android but I don't think it's universally true: crbug/562165
Thanks for the review. Please take a look. https://codereview.chromium.org/1486053003/diff/80001/base/android/java/src/o... File base/android/java/src/org/chromium/base/EarlyTraceEvent.java (right): https://codereview.chromium.org/1486053003/diff/80001/base/android/java/src/o... base/android/java/src/org/chromium/base/EarlyTraceEvent.java:73: /** @see TraceEvent#MaybeEnableEarlytracing(). On 2016/01/08 19:42:18, Yaron wrote: > Tracing Good catch, thanks! Done. https://codereview.chromium.org/1486053003/diff/80001/base/android/java/src/o... base/android/java/src/org/chromium/base/EarlyTraceEvent.java:77: || (new File("/data/local/chrome-trace-config.json")).exists())) { On 2016/01/08 19:59:29, Yaron wrote: > On 2016/01/08 19:51:10, pasko wrote: > > On 2016/01/08 19:42:18, Yaron wrote: > > > This appears to be a StrictMode violation (and disk IO) on start-up for most > > > users. Why is it necessary? > > > > Yes, this reaction matches my first thoughts as well. > > > > We decided that Telemetry should control tracing for startup by introducing > this > > file into the filesystem. We consider that checking for file in a recently > used > > directory is cheap. Also, Benoit told me that stat(3) without reading/writing > > does not trigger a StrictMode violation. I did not verify that myself. > > > > > (https://docs.google.com/document/d/1yRCXhrQ-0rsfUgNHt9T4YdnmJYrXKN6aK56Ozk3kP...) > > Perhaps it's specific to the version of Android but I don't think it's > universally true: crbug/562165 pasko: Sorry, wasn't clear enough. The command line access is an "unavoidable" StrictMode violation, since we need to read the command line file very early on. Fortunately this happens only once. Checking whether the file exists is done a bit later on for every startup, here: https://code.google.com/p/chromium/codesearch#chromium/src/content/app/conten... As pasko@ said, we unfortunately need to check this file. Can we suppress the StrictMode violation then? (even though I believe this wouldn't cause one, as we setup the policy later, unless maybeEnable() is called several times). https://codereview.chromium.org/1486053003/diff/80001/base/android/java/src/o... base/android/java/src/org/chromium/base/EarlyTraceEvent.java:107: if (sState != STATE_ENABLED) return; On 2016/01/08 19:42:18, Yaron wrote: > Hmm, while I get the point of this, I'm surprised that findbugs didn't complain > about inconsistent locking - have you run tryjobs? This may need to be exempted Surprinsingly, findbugs doesn't complain about it. See the tryjob run below. https://codereview.chromium.org/1486053003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java (right): https://codereview.chromium.org/1486053003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java:199: initCommandLine(); On 2016/01/08 19:42:18, Yaron wrote: > Can you not do this here? We're actually in the process of trying to trim down > our application classes: crbug/560466 > > I think on the renderer this is less important too so this could live in > ChromeBrowserInitializer We want to track things starting from the Java entry point (as you see below with the TraceEvent call). ChromeBrowserInitializer is called from AsyncInitializationActivity IIRC, which is already fairly late (for instance, this doesn't allow us to capture the overhead of the "trampoline" activity ChromeLauncherActivity). Any suggestion?
https://codereview.chromium.org/1486053003/diff/80001/base/android/java/src/o... File base/android/java/src/org/chromium/base/EarlyTraceEvent.java (right): https://codereview.chromium.org/1486053003/diff/80001/base/android/java/src/o... base/android/java/src/org/chromium/base/EarlyTraceEvent.java:77: || (new File("/data/local/chrome-trace-config.json")).exists())) { On 2016/01/13 09:35:28, Benoit L wrote: > On 2016/01/08 19:59:29, Yaron wrote: > > On 2016/01/08 19:51:10, pasko wrote: > > > On 2016/01/08 19:42:18, Yaron wrote: > > > > This appears to be a StrictMode violation (and disk IO) on start-up for > most > > > > users. Why is it necessary? > > > > > > Yes, this reaction matches my first thoughts as well. > > > > > > We decided that Telemetry should control tracing for startup by introducing > > this > > > file into the filesystem. We consider that checking for file in a recently > > used > > > directory is cheap. Also, Benoit told me that stat(3) without > reading/writing > > > does not trigger a StrictMode violation. I did not verify that myself. > > > > > > > > > (https://docs.google.com/document/d/1yRCXhrQ-0rsfUgNHt9T4YdnmJYrXKN6aK56Ozk3kP...) > > > > Perhaps it's specific to the version of Android but I don't think it's > > universally true: crbug/562165 > > pasko: Sorry, wasn't clear enough. The command line access is an "unavoidable" > StrictMode violation, since we need to read the command line file very early on. > Fortunately this happens only once. > > Checking whether the file exists is done a bit later on for every startup, here: > https://code.google.com/p/chromium/codesearch#chromium/src/content/app/conten... > > As pasko@ said, we unfortunately need to check this file. Can we suppress the > StrictMode violation then? (even though I believe this wouldn't cause one, as we > setup the policy later, unless maybeEnable() is called several times). I think the policy can be set externally as well, so it would be more robust to just ignore it? https://codereview.chromium.org/1486053003/diff/80001/base/android/java/src/o... base/android/java/src/org/chromium/base/EarlyTraceEvent.java:107: if (sState != STATE_ENABLED) return; On 2016/01/13 09:35:28, Benoit L wrote: > On 2016/01/08 19:42:18, Yaron wrote: > > Hmm, while I get the point of this, I'm surprised that findbugs didn't > complain > > about inconsistent locking - have you run tryjobs? This may need to be > exempted > > Surprinsingly, findbugs doesn't complain about it. See the tryjob run below. not sure how surprising it is: accessing a volatile field is one of the synchronization primitives in Java
https://codereview.chromium.org/1486053003/diff/80001/base/android/java/src/o... File base/android/java/src/org/chromium/base/EarlyTraceEvent.java (right): https://codereview.chromium.org/1486053003/diff/80001/base/android/java/src/o... base/android/java/src/org/chromium/base/EarlyTraceEvent.java:77: || (new File("/data/local/chrome-trace-config.json")).exists())) { On 2016/01/13 16:00:47, pasko wrote: > On 2016/01/13 09:35:28, Benoit L wrote: > > On 2016/01/08 19:59:29, Yaron wrote: > > > On 2016/01/08 19:51:10, pasko wrote: > > > > On 2016/01/08 19:42:18, Yaron wrote: > > > > > This appears to be a StrictMode violation (and disk IO) on start-up for > > most > > > > > users. Why is it necessary? > > > > > > > > Yes, this reaction matches my first thoughts as well. > > > > > > > > We decided that Telemetry should control tracing for startup by > introducing > > > this > > > > file into the filesystem. We consider that checking for file in a recently > > > used > > > > directory is cheap. Also, Benoit told me that stat(3) without > > reading/writing > > > > does not trigger a StrictMode violation. I did not verify that myself. > > > > > > > > > > > > > > (https://docs.google.com/document/d/1yRCXhrQ-0rsfUgNHt9T4YdnmJYrXKN6aK56Ozk3kP...) > > > > > > Perhaps it's specific to the version of Android but I don't think it's > > > universally true: crbug/562165 > > > > pasko: Sorry, wasn't clear enough. The command line access is an "unavoidable" > > StrictMode violation, since we need to read the command line file very early > on. > > Fortunately this happens only once. > > > > Checking whether the file exists is done a bit later on for every startup, > here: > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/app/conten... > > > > As pasko@ said, we unfortunately need to check this file. Can we suppress the > > StrictMode violation then? (even though I believe this wouldn't cause one, as > we > > setup the policy later, unless maybeEnable() is called several times). With all due respect: what's not to believe? The bug I linked is clear evidence that File.exists is flagged by android's strictmode due to causing disk IO. Or are you saying that we already have that IO on the main thread in native code so this would just incur the same overhead but in a different place and the subsequent call would likely be hot/cached? As for the linked code, that's explicitly for !android > > I think the policy can be set externally as well, so it would be more robust to > just ignore it? You can certainly whitelist the strictmode violation locally but I don't understand why we choose to do that when you can specify a command line flag and no overhead for real users. Added a comment on the doc https://codereview.chromium.org/1486053003/diff/80001/base/android/java/src/o... base/android/java/src/org/chromium/base/EarlyTraceEvent.java:107: if (sState != STATE_ENABLED) return; On 2016/01/13 16:00:47, pasko wrote: > On 2016/01/13 09:35:28, Benoit L wrote: > > On 2016/01/08 19:42:18, Yaron wrote: > > > Hmm, while I get the point of this, I'm surprised that findbugs didn't > > complain > > > about inconsistent locking - have you run tryjobs? This may need to be > > exempted > > > > Surprinsingly, findbugs doesn't complain about it. See the tryjob run below. > > not sure how surprising it is: accessing a volatile field is one of the > synchronization primitives in Java oops, missed that it was volatile, agree that this is fine.
Hi, Reviving this CL because of crbug.com/622213. yfriedman@, PTAL. From the comment you had back in January the main concern was aroung the way we decide whether we do tracing. WDYT?
ya, I think my only concern was the disk access. in the intervening time, we've come to understand these better so I reluctantly don't object too strongly anymore. we're already affected by lots of small reads, this one shouldn't be too much additional trouble. I do think you need to whitelist it though or it'll get flagged. https://codereview.chromium.org/1486053003/diff/160001/base/android/java/src/... File base/android/java/src/org/chromium/base/EarlyTraceEvent.java (right): https://codereview.chromium.org/1486053003/diff/160001/base/android/java/src/... base/android/java/src/org/chromium/base/EarlyTraceEvent.java:81: if (!(CommandLine.getInstance().hasSwitch("trace-startup") so this still need a strictmode exemption, right? https://codereview.chromium.org/1486053003/diff/160001/base/base.gypi File base/base.gypi (right): https://codereview.chromium.org/1486053003/diff/160001/base/base.gypi#newcode45 base/base.gypi:45: 'android/early_trace_event_binding.cc', gyp changes probably aren't necessary anymore
Thanks! All done. https://codereview.chromium.org/1486053003/diff/160001/base/android/java/src/... File base/android/java/src/org/chromium/base/EarlyTraceEvent.java (right): https://codereview.chromium.org/1486053003/diff/160001/base/android/java/src/... base/android/java/src/org/chromium/base/EarlyTraceEvent.java:81: if (!(CommandLine.getInstance().hasSwitch("trace-startup") On 2016/06/29 21:27:34, Yaron wrote: > so this still need a strictmode exemption, right? Done. https://codereview.chromium.org/1486053003/diff/160001/base/base.gypi File base/base.gypi (right): https://codereview.chromium.org/1486053003/diff/160001/base/base.gypi#newcode45 base/base.gypi:45: 'android/early_trace_event_binding.cc', On 2016/06/29 21:27:34, Yaron wrote: > gyp changes probably aren't necessary anymore Indeed, thanks! Done.
The CQ bit was checked by lizeb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by lizeb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
lgtm https://codereview.chromium.org/1486053003/diff/180001/base/android/early_tra... File base/android/early_trace_event_binding.h (right): https://codereview.chromium.org/1486053003/diff/180001/base/android/early_tra... base/android/early_trace_event_binding.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. 2016 now https://codereview.chromium.org/1486053003/diff/180001/base/android/early_tra... base/android/early_trace_event_binding.h:13: extern bool RegisterEarlyTraceEvent(JNIEnv* env); Why extern?
lizeb@chromium.org changed reviewers: + jam@chromium.org
Thanks! All done. Adding jam@ for base/BUILD.gn. PTAL, thanks. https://codereview.chromium.org/1486053003/diff/180001/base/android/early_tra... File base/android/early_trace_event_binding.h (right): https://codereview.chromium.org/1486053003/diff/180001/base/android/early_tra... base/android/early_trace_event_binding.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/07/05 16:43:56, Yaron wrote: > 2016 now Done. https://codereview.chromium.org/1486053003/diff/180001/base/android/early_tra... base/android/early_trace_event_binding.h:13: extern bool RegisterEarlyTraceEvent(JNIEnv* env); On 2016/07/05 16:43:56, Yaron wrote: > Why extern? Done.
The CQ bit was checked by lizeb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
On 2016/07/06 14:00:33, Benoit L wrote: > Thanks! > All done. > > > Adding jam@ for base/BUILD.gn. PTAL, thanks. lgtm since this is trivial. in the future, please pick an owner from base/owners.
The CQ bit was checked by lizeb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pasko@chromium.org, yfriedman@chromium.org Link to the patchset: https://codereview.chromium.org/1486053003/#ps200001 (title: "Address comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by lizeb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pasko@chromium.org, yfriedman@chromium.org, jam@chromium.org Link to the patchset: https://codereview.chromium.org/1486053003/#ps220001 (title: "Rebase + fix test.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Android: Add support for TraceEvent before the native library is loaded. Currently TraceEvents recorded before the native library load are silently discarded. This CL adds support for such "early" events. The events are buffered in Java, and then sent to the native side once it is available. This support is intentionally simple now. The aim is to ease performance investigation of early startup, and to ease tracking of early startup performance regression, since timings are then accessible to TBM tests. Also use the newly-introduced events in a couple places, and fix a bug in the TraceEvent we use to record the launcher activity lifetime (when an Activity calls finish from onCreate(), onPause() is not called). BUG=564041 ========== to ========== Android: Add support for TraceEvent before the native library is loaded. Currently TraceEvents recorded before the native library load are silently discarded. This CL adds support for such "early" events. The events are buffered in Java, and then sent to the native side once it is available. This support is intentionally simple now. The aim is to ease performance investigation of early startup, and to ease tracking of early startup performance regression, since timings are then accessible to TBM tests. Also use the newly-introduced events in a couple places, and fix a bug in the TraceEvent we use to record the launcher activity lifetime (when an Activity calls finish from onCreate(), onPause() is not called). BUG=564041 Committed: https://crrev.com/fb0784294541c53f12e283124672750ab2e291f1 Cr-Commit-Position: refs/heads/master@{#404635} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/fb0784294541c53f12e283124672750ab2e291f1 Cr-Commit-Position: refs/heads/master@{#404635}
Message was sent while issue was closed.
A revert of this CL (patchset #12 id:220001) has been created in https://codereview.chromium.org/2145443002/ by wychen@chromium.org. The reason for reverting is: https://crbug.com/627142. |
