Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2263333002:
Disable StrictMode for disk reads during Snippet formatting. (Closed)
DescriptionDisable StrictMode for disk reads during Snippet formatting.
During Snippet formatting we call DateUtils.getRelativeTimeSpanString(...), which itself calls through to TimeZone.getDefault(). If this has never been called before it loads the current time zone from disk. In most likelihood this will have been called previously and the current time zone will have been cached, but in some cases (eg instrumentation tests) it will cause a strict mode violation.
BUG=639877
Committed: https://crrev.com/f0418916f751a8e1d1854f37c2aa06d07456376c
Cr-Commit-Position: refs/heads/master@{#413613}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Timed only text formatting. #
Messages
Total messages: 19 (8 generated)
peconn@chromium.org changed reviewers: + bauerb@chromium.org, holte@chromium.org, mvanouwerkerk@chromium.org, wnwen@chromium.org
holte@, could you please review the change in histograms.xml. Others, could you please review the change in SnippetArticleViewHolder.java.
https://codereview.chromium.org/2263333002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/2263333002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:303: mPublisherTextView.setText(publisherAttribution); We could actually make this asynchronous - create an AsyncTask that on completion sets the mPublisherTextView.
lgtm https://codereview.chromium.org/2263333002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/2263333002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:303: mPublisherTextView.setText(publisherAttribution); On 2016/08/22 17:22:53, PEConn wrote: > We could actually make this asynchronous - create an AsyncTask that on > completion sets the mPublisherTextView. I'd be a bit worried about UI jumpiness in that case… Do we know how often the strict mode violation happens in the real world? I guess we don't, since we can't really measure it :-/
https://codereview.chromium.org/2263333002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/2263333002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:303: mPublisherTextView.setText(publisherAttribution); On 2016/08/22 18:00:37, Bernhard Bauer wrote: > On 2016/08/22 17:22:53, PEConn wrote: > > We could actually make this asynchronous - create an AsyncTask that on > > completion sets the mPublisherTextView. > > I'd be a bit worried about UI jumpiness in that case… > > Do we know how often the strict mode violation happens in the real world? I > guess we don't, since we can't really measure it :-/ You can look at the histogram for Android.StrictMode.NotificationUIBuildTime for a rough idea. I don't know how long a Notification takes to build normally, but the results for that show that ~90% are within 3ms, then has a bit of a long tail. Besides though it's kinda scary that this piece of code happens every onBindViewHolder, it really should only occur once per RecyclerView.
lgtm % comment https://codereview.chromium.org/2263333002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/2263333002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:303: mPublisherTextView.setText(publisherAttribution); On 2016/08/22 18:11:24, PEConn wrote: > On 2016/08/22 18:00:37, Bernhard Bauer wrote: > > On 2016/08/22 17:22:53, PEConn wrote: > > > We could actually make this asynchronous - create an AsyncTask that on > > > completion sets the mPublisherTextView. > > > > I'd be a bit worried about UI jumpiness in that case… > > > > Do we know how often the strict mode violation happens in the real world? I > > guess we don't, since we can't really measure it :-/ > > You can look at the histogram for Android.StrictMode.NotificationUIBuildTime for > a rough idea. I don't know how long a Notification takes to build normally, but > the results for that show that ~90% are within 3ms, then has a bit of a long > tail. > > Besides though it's kinda scary that this piece of code happens every > onBindViewHolder, it really should only occur once per RecyclerView. Yes, just for the other call site, P90 is at 6.5ms, P99 is at 52ms, so not too bad. Worthwhile to consider warming it on idle as there's now a couple places where we hit this, but probably not worth incurring a separate AsyncTask if this is the only problem. This histogram will help us determine this too. Also, can setText come after the timing?
The CQ bit was checked by peconn@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: This issue passed the CQ dry run.
https://codereview.chromium.org/2263333002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/2263333002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:303: mPublisherTextView.setText(publisherAttribution); On 2016/08/22 19:33:00, Peter Wen wrote: > On 2016/08/22 18:11:24, PEConn wrote: > > On 2016/08/22 18:00:37, Bernhard Bauer wrote: > > > On 2016/08/22 17:22:53, PEConn wrote: > > > > We could actually make this asynchronous - create an AsyncTask that on > > > > completion sets the mPublisherTextView. > > > > > > I'd be a bit worried about UI jumpiness in that case… > > > > > > Do we know how often the strict mode violation happens in the real world? I > > > guess we don't, since we can't really measure it :-/ > > > > You can look at the histogram for Android.StrictMode.NotificationUIBuildTime > for > > a rough idea. I don't know how long a Notification takes to build normally, > but > > the results for that show that ~90% are within 3ms, then has a bit of a long > > tail. > > > > Besides though it's kinda scary that this piece of code happens every > > onBindViewHolder, it really should only occur once per RecyclerView. > > Yes, just for the other call site, P90 is at 6.5ms, P99 is at 52ms, so not too > bad. Worthwhile to consider warming it on idle as there's now a couple places > where we hit this, but probably not worth incurring a separate AsyncTask if this > is the only problem. This histogram will help us determine this too. > > Also, can setText come after the timing? Done.
lgtm
The CQ bit was checked by peconn@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, wnwen@chromium.org Link to the patchset: https://codereview.chromium.org/2263333002/#ps20001 (title: "Timed only text formatting.")
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 #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Disable StrictMode for disk reads during Snippet formatting. During Snippet formatting we call DateUtils.getRelativeTimeSpanString(...), which itself calls through to TimeZone.getDefault(). If this has never been called before it loads the current time zone from disk. In most likelihood this will have been called previously and the current time zone will have been cached, but in some cases (eg instrumentation tests) it will cause a strict mode violation. BUG=639877 ========== to ========== Disable StrictMode for disk reads during Snippet formatting. During Snippet formatting we call DateUtils.getRelativeTimeSpanString(...), which itself calls through to TimeZone.getDefault(). If this has never been called before it loads the current time zone from disk. In most likelihood this will have been called previously and the current time zone will have been cached, but in some cases (eg instrumentation tests) it will cause a strict mode violation. BUG=639877 Committed: https://crrev.com/f0418916f751a8e1d1854f37c2aa06d07456376c Cr-Commit-Position: refs/heads/master@{#413613} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/f0418916f751a8e1d1854f37c2aa06d07456376c Cr-Commit-Position: refs/heads/master@{#413613}
Message was sent while issue was closed.
lgtm |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
