Closed Bug 1075644 Opened 10 years ago Closed 3 years ago

Follow-up to initializing Gecko thread sooner

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED INCOMPLETE

People

(Reporter: jchen, Unassigned)

References

Details

Attachments

(3 files, 2 obsolete files)

We want to reduce the Nexus S throbber start regression after bug 888482 landed.
I think these are safe to move around without risking regression. They are part of startup anyways, and moving them to after Gecko start can let us parallelize better.
Attachment #8498339 - Flags: review?(mark.finkle)
We should reduce Gecko thread priority at the very start. ThreadUtils.reduceGeckoPriority already takes care of checking for single-core devices.
Attachment #8498345 - Flags: review?(snorp)
Comment on attachment 8498339 [details] [diff] [review]
Move more things to after Gecko start (v1)

Worth a try
Attachment #8498339 - Flags: review?(mark.finkle) → review+
Comment on attachment 8498345 [details] [diff] [review]
Reduce Gecko thread priority at very start (v1)

Review of attachment 8498345 [details] [diff] [review]:
-----------------------------------------------------------------

Drive-by comments:
* The priority gets reset here: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/TopSitesPanel.java#402. That is, TopSitesPanel is responsible for both lowering and resetting the priority. Moving part of that to GeckoThread makes us lose that parity.
* There's no guarantee TopSitesPanel gets shown during startup -- consider session restore, for example. That means it will always take 10 seconds (the timeout) to reset the priority in these cases.
* A related point: since there's little UI shown for session restore startups, do we even want to lower the Gecko priority?
(In reply to Brian Nicholson (:bnicholson) from comment #4)
> Comment on attachment 8498345 [details] [diff] [review]
> Reduce Gecko thread priority at very start (v1)
> 
> Review of attachment 8498345 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Drive-by comments:
> * The priority gets reset here:
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/
> TopSitesPanel.java#402. That is, TopSitesPanel is responsible for both
> lowering and resetting the priority. Moving part of that to GeckoThread
> makes us lose that parity.

I don't think we need parity within TopSitesPanel here. It's easy enough to see who's calling reduceGeckoPriority/resetGeckoPriority.

> * There's no guarantee TopSitesPanel gets shown during startup -- consider
> session restore, for example. That means it will always take 10 seconds (the
> timeout) to reset the priority in these cases.

I see. How about we also reset priority at other places (e.g. at throbber start)?

> * A related point: since there's little UI shown for session restore
> startups, do we even want to lower the Gecko priority?

We still load a lot of UI (inflating views, constructing classes, etc.) despite not showing it, so we do want to lower the Gecko priority.
Comment on attachment 8498345 [details] [diff] [review]
Reduce Gecko thread priority at very start (v1)

Review of attachment 8498345 [details] [diff] [review]:
-----------------------------------------------------------------

I agree with :bnicholson's points, we should make sure we're upping the priority in the right spots.
Attachment #8498345 - Flags: review?(snorp)
What about we separately reduce Gecko priority during UI loading? This way thumbnail loading is not affected -- when loading UI, we reduce priority, then reset it; then when loading thumbnail later, we reduce priority again, then reset it again.
Attachment #8498345 - Attachment is obsolete: true
Attachment #8498998 - Flags: review?(snorp)
Attachment #8498998 - Flags: review?(bnicholson)
Oops, the last patch didn't include the reduceGeckoPriority change in TopSitesPanel.java.
Attachment #8498998 - Attachment is obsolete: true
Attachment #8498998 - Flags: review?(snorp)
Attachment #8498998 - Flags: review?(bnicholson)
Attachment #8499003 - Flags: review?(snorp)
Attachment #8499003 - Flags: review?(bnicholson)
Comment on attachment 8499003 [details] [diff] [review]
Reduce Gecko thread priority at very start (v3)

Review of attachment 8499003 [details] [diff] [review]:
-----------------------------------------------------------------

I think this looks OK. Is this a speculative fix, or have you already confirmed that this helps the throbber times?
Attachment #8499003 - Flags: review?(bnicholson) → review+
(In reply to Brian Nicholson (:bnicholson) from comment #9)
> Comment on attachment 8499003 [details] [diff] [review]
> Reduce Gecko thread priority at very start (v3)
> 
> Review of attachment 8499003 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think this looks OK. Is this a speculative fix, or have you already
> confirmed that this helps the throbber times?

Speculative mostly, since autophone only monitors things that have landed.
Attachment #8499003 - Flags: review?(snorp) → review+
I suspect that these intermittent crashes were yours as well:
https://treeherder.mozilla.org/ui/logviewer.html#?job_id=831255&repo=fx-team
Looks like this regressed autophone for Nexus S devices. Other devices had a small improvement. I wonder if it's the GeckoThread priority change? Could we check for single processor devices and skip the thread priority change? Is it worth it?
mfinkle, it regressed total throbber time but that was partly due to the huge improvement in throbber start time with an unchanged throbber stop time.
(In reply to Bob Clary [:bc:] from comment #15)
> mfinkle, it regressed total throbber time but that was partly due to the
> huge improvement in throbber start time with an unchanged throbber stop time.

Oh damn, Bob, you're right! Ignore me.
These tests were failing because they assume the test page is loaded when the title changes. However, a page can still be loading when that happens. This patch changes them to wait for first paint.
Attachment #8505640 - Flags: review?(mark.finkle)
Comment on attachment 8505640 [details] [diff] [review]
Wait for page to fully load in context menu tests (v1)

Seems sane to me
Attachment #8505640 - Flags: review?(mark.finkle) → review+
Backed out for being the likely cause of bug 1085627.
https://hg.mozilla.org/integration/fx-team/rev/f48ccb17a9e1
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Firefox 36 → ---
Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/f48ccb17a9e1
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
*sigh*
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Firefox 36 → ---
Assignee: nchen → nobody
We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.
Status: REOPENED → RESOLVED
Closed: 10 years ago3 years ago
Resolution: --- → INCOMPLETE
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.