985369 - Writing into the URL bar consumes 100% CPU time

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::Browser, defect, P1)

Description

[Gabriele Svelto [:gsvelto]]

+++ This bug was initially created as a clone of Bug #980070 +++

While investigating the behavior described in bug 980070 comment 36 I've noticed that when we type some characters in the URL bar the suggestions/history elements appearing below are refreshed for every character. This drives CPU utilization to 100% and when typing fast causes the updates to pile up and be redrawn with a measurable delay.

Could we batch up those updates so that if the user is typing too fast for us to keep up we skip the completions we can't come up with fast enough?

Quickly looking at the code in apps/browser/js/awesomescreen.js I think this might be achieved by canceling the call to AwesomeScreen.populateResults() if AwesomeScreen.update() is invoked again before the former has had time to repopulate the list of results.

Comment 1

[Jason Smith [:jsmith]]

leaving qawanted here to see if this problem is present on 1.1

Comment 2

[Peter Bylenga [:PBylenga]]

On Buri v1.1 I see the CPU float around 91-94% during writing into the URL bar.

Prerequisites:
use 'adb shell top -m 10' to observe CPU usage

STRs
1. Launch Browser App
2. Activate URL bar
3. Begin typing quickly and observe CPU consumption

Actual Results:
91-94%

v1.1 Environmental Variables:
Device: Buri v1.1 MOZ
BuildID: 20140318041202
Gaia: 44a2ddf63373f8e95c784faf4ed4d60081699c61
Gecko: 2c70ef07c5b3
Version: 18.0
Firmware Version: v1.2-device.cfg

Comment 3

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH 1/2] Drop updates to the suggestions that have already been superseded

This patch implements what I described in comment 0. Basically every time an update to the suggestions is triggered we check if another one is already undergoing. If it is, we flag the new update as pending and wait for the previous one to finish before executing it. Since at all times we can have only one pending update, updates arriving rapidly one after the other are automatically skipped reducing the CPU load.

Comment 4

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH 2/2] Clean up all linting issues

Since I was already working on this and I'm kind-of a linter freak I took the liberty of preparing a patch to make this file jshint-clean too. The changes are minimal.

Comment 5

[Gabriele Svelto [:gsvelto]]

Attachment: [PULL REQUEST] Improve URL input

And here comes the pull request with both patches included

Comment 6

[Ben Francis [:benfrancis]]

Comment on attachment 8397915 [details] [diff] [review]
[PATCH 1/2] Drop updates to the suggestions that have already been superseded

That's great, thanks :)

Comment 7

[Ben Francis [:benfrancis]]

Merged into master https://github.com/mozilla-b2g/gaia/commit/71909da8662ed3b600a04dd70c91aa567566cd55

Comment 8

[Kevin Grandon :kgrandon]

Oops - looks like this cross paths with another merge before it landed and broke the linter. Follow-up landed here: https://github.com/mozilla-b2g/gaia/commit/b6ba925df0afa3a1d5d5a197457b9ce51abe5fc1

Comment 9

[Ben Francis [:benfrancis]]

Ah, thanks Kevin!

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b215fd6c2296

Comment 11

[Gabriele Svelto [:gsvelto]]

The bug this one was cloned from was a 1.3 blocker so since we already have a fix here and it's pretty straightforward let's nom this for 1.4.

Comment 12

[Jason Smith [:jsmith]]

(In reply to Gabriele Svelto [:gsvelto] from comment #11)
> The bug this one was cloned from was a 1.3 blocker so since we already have
> a fix here and it's pretty straightforward let's nom this for 1.4.

That sounds like a case that we should actually ask for approval, rather than block. This is mainly because this has already been present on 1.1 & 1.3, so we've already shipped with this problem previously. So I'd suggest asking for 1.4 approval.

Comment 13

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8397915 [details] [diff] [review]
[PATCH 1/2] Drop updates to the suggestions that have already been superseded

(In reply to Jason Smith [:jsmith] from comment #12)
> That sounds like a case that we should actually ask for approval, rather
> than block. This is mainly because this has already been present on 1.1 &
> 1.3, so we've already shipped with this problem previously. So I'd suggest
> asking for 1.4 approval.

Good point, here's the request:

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): this has always been the default behavior
[User impact] if declined: higher CPU consumption when typing in the URL bar, no functional issues
[Testing completed]: tested on the emulator and on a device, landed on master and stuck (until now at least)
[Risk to taking this patch] (and alternatives if risky): there might be an impact on the suggestions presented when typing an URL
[String changes made]: none

Comment 14

[bhavana bajaj [:bajaj]]

(In reply to Gabriele Svelto [:gsvelto] from comment #13)
> Comment on attachment 8397915 [details] [diff] [review]
> [PATCH 1/2] Drop updates to the suggestions that have already been superseded
> 
> (In reply to Jason Smith [:jsmith] from comment #12)
> > That sounds like a case that we should actually ask for approval, rather
> > than block. This is mainly because this has already been present on 1.1 &
> > 1.3, so we've already shipped with this problem previously. So I'd suggest
> > asking for 1.4 approval.
> 
> Good point, here's the request:
> 
> [Approval Request Comment]
> [Bug caused by] (feature/regressing bug #): this has always been the default
> behavior
> [User impact] if declined: higher CPU consumption when typing in the URL
> bar, no functional issues
> [Testing completed]: tested on the emulator and on a device, landed on
> master and stuck (until now at least)
> [Risk to taking this patch] (and alternatives if risky): there might be an
> impact on the suggestions presented when typing an URL
Could you elaborate on this ? I am requesting QA to verify this patch before uplifting due to the outlined risk or get some exploratory testing here and may be any input on testcases might be helpful to them here.

Comment 15

[Gabriele Svelto [:gsvelto]]

(In reply to bhavana bajaj [:bajaj] from comment #14)
> Could you elaborate on this ? I am requesting QA to verify this patch before
> uplifting due to the outlined risk or get some exploratory testing here and
> may be any input on testcases might be helpful to them here.

One of the earlier versions of my patch had an issue which caused some suggestions not to show up. I noticed it because the text typed in the URL and the one looked up in the suggestions did not match so I fixed it. I still wanted to mention this because I'm fairly confident that the current patch doesn't have that problem but it's always better to be safe than sorry with testing.

Comment 16

[bhavana bajaj [:bajaj]]

Leaving a NI on Peter to see if he can help verify this bug on master/1.5.

Comment 17

[Peter Bylenga [:PBylenga]]

On today's Master/1.5 when I type in the URL usage floats between 62-74% using steps outlined in Comment 2.

v1.5 Environmental Variables:
Device: Buri v1.5 MOZ
BuildID: 20140408040204
Gaia: 1958454595b1fa0e061f0652ae965629993f5708
Gecko: 8883360b1edb
Version: 31.0a1
Firmware Version: v1.2-device.cfg

Comment 18

[Gabriele Svelto [:gsvelto]]

Cherry-picked to gaia/v1.4 6c4319c95114dc4bd18b2497306b6cbac3b29ad9

https://github.com/mozilla-b2g/gaia/commit/6c4319c95114dc4bd18b2497306b6cbac3b29ad9