Don't trigger HW acceleration from Toasts on low-end devices.

Don't trigger HW acceleration from Toasts on low-end devices.

BUG=520600
Committed: https://crrev.com/88634f4e2e63e13a71d94386dca77b3fc802077c
Cr-Commit-Position: refs/heads/master@{#343518}

[Dmitry Skiba, 2015-08-06 23:09:51]

dskiba@google.com changed reviewers:
+ jdduke@chromium.org, mariakhomenko@chromium.org

[jdduke (slow), 2015-08-07 00:10:11]

One question before we start, so we've already disabled HW acceleration for
low-end devices on L? Can you point me to the metrics that improved with that
change? I just want to make sure we have tests (perf or otherwise) in place that
would catch any regressions here. Otherwise this will be a never-ending
whack-a-mole =/.

[Maria, 2015-08-07 01:08:17]

Jared,

You can see the improvement on this graph here:
https://chromeperf.appspot.com/report?sid=bf05f9aee584a1525fe891212f9ddaae41c....
The sharp decline towards the end of the graph is where the change was
submitted. There are also instrumentation checks in place that start various
activities on low end devices and verify that RenderThread hasn't been created.

Dmitry,

Have you looked into enforcing the new toast class with a presubmit?

https://codereview.chromium.org/1276523003/diff/40001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/widget/Toast.java (right):

https://codereview.chromium.org/1276523003/diff/40001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/widget/Toast.java:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
2015

https://codereview.chromium.org/1276523003/diff/40001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/widget/Toast.java:49: mSWLayout = new
LinearLayout(new ContextWrapper(context) {
Does this have to be a linear layout? Seems like we just use it to wrap a single
view, frame layout is much more inexpensive.

https://codereview.chromium.org/1276523003/diff/40001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/widget/Toast.java:184: private ViewGroup
mSWLayout;
we generally put class members at the top of the class before constructor

[jdduke (slow), 2015-08-07 02:19:27]

I was also curious, have you looked at how the pull-to-refresh spinner performs
with HW acceleration off?

[jdduke (slow), 2015-08-07 02:24:29]

On 2015/08/07 01:08:17, Maria wrote:
> Jared,
> 
> You can see the improvement on this graph here:
>
https://chromeperf.appspot.com/report?sid=bf05f9aee584a1525fe891212f9ddaae41c....
> The sharp decline towards the end of the graph is where the change was
> submitted. There are also instrumentation checks in place that start various
> activities on low end devices and verify that RenderThread hasn't been
created.

Thanks, looks like a definite improvement, is 2-3MB about the savings we
expected?

[DmitrySkiba, 2015-08-07 15:33:09]

On 2015/08/07 01:08:17, Maria wrote:
> Dmitry,
> 
> Have you looked into enforcing the new toast class with a presubmit?

Not yet, but I will. I'll also write a test checking that triggering a Toast
doesn't create RenderThread.

[DmitrySkiba, 2015-08-07 15:39:13]

> Thanks, looks like a definite improvement, is 2-3MB about the savings we
> expected?

Hmm, on Nexus 4 it should be close to ~10 MiB, see my measurements in
https://code.google.com/p/chromium/issues/detail?id=482727#c106

I'll check with Maria on how to see other metrics from ClankInternal suite, for
now I can't even find it anywhere in the UI :)

[DmitrySkiba, 2015-08-07 15:43:36]

On 2015/08/07 02:19:27, jdduke wrote:
> I was also curious, have you looked at how the pull-to-refresh spinner
performs
> with HW acceleration off?

Yes, that was among items aelias@ wanted me to check in the original review
(https://codereview.chromium.org/1223263009):

"
Also, here is a list of UI features using Android Views that come to mind: URL
suggestions, permissions butterbars, contextual search, form autofill and
pull-to-refresh.  Could you test each of those things and verify it's in
software mode?  And if one of them turns out not to be and you change it, please
add a test specifically for that thing.
"

From those things only autofill suggestions list had issues, but I fixed them in
1252353004

[Maria, 2015-08-10 23:44:45]

lgtm

[Dmitry Skiba, 2015-08-11 23:49:51]

https://chromiumcodereview.appspot.com/1276523003/diff/120001/PRESUBMIT.py
File PRESUBMIT.py (right):

https://chromiumcodereview.appspot.com/1276523003/diff/120001/PRESUBMIT.py#ne...
PRESUBMIT.py:1334: '"android.widget.Toast" usage is detected. Android toasts use
hardware'
Example output:

** Presubmit Warnings **
"android.widget.Toast" usage is detected. Android toasts use hardware
acceleration, and can be
costly on low-end devices. If your code triggers toasts on low-end devices
(especially during
normal browsing workflow), you should use "org.chromium.ui.widget.Toast"
instead.
You don't need to bother if toast is shown only from an activity that is
hardware accelerated
(e.g. Preferences) or the one that is not visible to the end user (e.g.
ChromeShellActivity).
In any case, feel free to contact dskiba@chromium.org if you have any questions
or concerns.
 
chrome/android/javatests/src/org/chromium/chrome/browser/hardware_acceleration/Utils.java:9

[Maria, 2015-08-12 00:04:56]

On 2015/08/11 23:49:51, Dmitry Skiba wrote:
> https://chromiumcodereview.appspot.com/1276523003/diff/120001/PRESUBMIT.py
> File PRESUBMIT.py (right):
> 
>
https://chromiumcodereview.appspot.com/1276523003/diff/120001/PRESUBMIT.py#ne...
> PRESUBMIT.py:1334: '"android.widget.Toast" usage is detected. Android toasts
use
> hardware'
> Example output:
> 
> ** Presubmit Warnings **
> "android.widget.Toast" usage is detected. Android toasts use hardware
> acceleration, and can be
> costly on low-end devices. If your code triggers toasts on low-end devices
> (especially during
> normal browsing workflow), you should use "org.chromium.ui.widget.Toast"
> instead.
> You don't need to bother if toast is shown only from an activity that is
> hardware accelerated
> (e.g. Preferences) or the one that is not visible to the end user (e.g.
> ChromeShellActivity).
> In any case, feel free to contact mailto:dskiba@chromium.org if you have any
questions
> or concerns.
>  
>
chrome/android/javatests/src/org/chromium/chrome/browser/hardware_acceleration/Utils.java:9

I wonder if we should create a whitelist for existing exceptions in the check.
My worry is that people modifying the files that trigger this check will be
blocked from uploading their changes unless they force bypass (which sometimes
causes other things to go through), so I think it might be better to ensure that
people have a way to silence the warning.

The other question is, I noticed that you mention things in preferences not
needing this, but this change actually changes Toast to new one in many
preference classes. I think if we give guidance on where to use it, we should
follow it ourselves. Alternatively, we could just force this to be used across
the board since it just proxies to system toast in the base case.

[Dmitry Skiba, 2015-08-12 22:51:38]

On 2015/08/12 00:04:56, Maria wrote:
> On 2015/08/11 23:49:51, Dmitry Skiba wrote:
> > https://chromiumcodereview.appspot.com/1276523003/diff/120001/PRESUBMIT.py
> > File PRESUBMIT.py (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/1276523003/diff/120001/PRESUBMIT.py#ne...
> > PRESUBMIT.py:1334: '"android.widget.Toast" usage is detected. Android toasts
> use
> > hardware'
> > Example output:
> > 
> > ** Presubmit Warnings **
> > "android.widget.Toast" usage is detected. Android toasts use hardware
> > acceleration, and can be
> > costly on low-end devices. If your code triggers toasts on low-end devices
> > (especially during
> > normal browsing workflow), you should use "org.chromium.ui.widget.Toast"
> > instead.
> > You don't need to bother if toast is shown only from an activity that is
> > hardware accelerated
> > (e.g. Preferences) or the one that is not visible to the end user (e.g.
> > ChromeShellActivity).
> > In any case, feel free to contact mailto:dskiba@chromium.org if you have any
> questions
> > or concerns.
> >  
> >
>
chrome/android/javatests/src/org/chromium/chrome/browser/hardware_acceleration/Utils.java:9
> 
> I wonder if we should create a whitelist for existing exceptions in the check.
> My worry is that people modifying the files that trigger this check will be
> blocked from uploading their changes unless they force bypass (which sometimes
> causes other things to go through), so I think it might be better to ensure
that
> people have a way to silence the warning.
> 
> The other question is, I noticed that you mention things in preferences not
> needing this, but this change actually changes Toast to new one in many
> preference classes. I think if we give guidance on where to use it, we should
> follow it ourselves. Alternatively, we could just force this to be used across
> the board since it just proxies to system toast in the base case.

See updated PRESUBMIT script and two new Java files.
Now we filter out unrelated paths and return errors.

[Maria, 2015-08-12 23:18:29]

lgtm

[Dmitry Skiba, 2015-08-13 21:27:43]

The CQ bit was checked by dskiba@google.com

[commit-bot: I haz the power, 2015-08-13 21:28:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1276523003/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1276523003/160001

[commit-bot: I haz the power, 2015-08-13 21:38:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-13 21:38:17]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Dmitry Skiba, 2015-08-13 21:50:06]

dskiba@google.com changed reviewers:
+ dtrainor@chromium.org, phajdan.jr@chromium.org

[Dmitry Skiba, 2015-08-13 21:50:07]

phajdan.jr: please review PRESUBMIT.py

dtrainor: please review TracingControllerAndroid and files in ui/android/java

[David Trainor- moved to gerrit, 2015-08-14 00:35:10]

lgtm

[Paweł Hajdan Jr., 2015-08-14 09:32:40]

PRESUBMIT.py LGTM

[Dmitry Skiba, 2015-08-14 16:56:19]

The CQ bit was checked by dskiba@google.com

[commit-bot: I haz the power, 2015-08-14 16:56:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1276523003/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1276523003/160001

[commit-bot: I haz the power, 2015-08-14 17:45:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-14 17:45:14]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[Dmitry Skiba, 2015-08-14 19:09:58]

The CQ bit was checked by dskiba@google.com

[Dmitry Skiba, 2015-08-14 19:09:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org, mariakhomenko@chromium.org, phajdan.jr@chromium.org
Link to the patchset: https://codereview.chromium.org/1276523003/#ps180001
(title: "Workaround proguard issue")

[commit-bot: I haz the power, 2015-08-14 19:10:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1276523003/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1276523003/180001

[Dmitry Skiba, 2015-08-14 21:50:29]

The CQ bit was checked by dskiba@google.com

[Dmitry Skiba, 2015-08-14 21:50:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org, mariakhomenko@chromium.org, phajdan.jr@chromium.org
Link to the patchset: https://codereview.chromium.org/1276523003/#ps200001
(title: "Check Toast acceleration only on LOLLIPOP+")

[commit-bot: I haz the power, 2015-08-14 21:50:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1276523003/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1276523003/200001

[commit-bot: I haz the power, 2015-08-14 23:03:38]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-08-14 23:04:15]

Patchset 11 (id:??) landed as
https://crrev.com/88634f4e2e63e13a71d94386dca77b3fc802077c
Cr-Commit-Position: refs/heads/master@{#343518}