If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Make Android UI nsThread and MessageLoop creation asynchronous

RESOLVED FIXED in Firefox 53

Status

()

Firefox for Android
GeckoView
RESOLVED FIXED
9 months ago
8 months ago

People

(Reporter: rbarker, Assigned: rbarker)

Tracking

Trunk
Firefox 53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

9 months ago
Currently instantiating the nsThread and MessageLoop for the Android UI thread requires the main thread to block while they are created. It would be better to try and asynchronously create them and then only block if they are queried before they have been created on the Android UI thread.
(Assignee)

Updated

9 months ago
Assignee: nobody → rbarker
(Assignee)

Updated

9 months ago
Summary: Make Android UI nsThread and MessageLoop creation Asynchronous → Make Android UI nsThread and MessageLoop creation asynchronous
(Assignee)

Comment 1

9 months ago
Created attachment 8823884 [details] [diff] [review]
0001-Bug-1328747-Make-Android-UI-nsThread-and-MessageLoop-creation-asynchronous-r-17010416-05cd641.patch
(Assignee)

Updated

9 months ago
Attachment #8823884 - Flags: review?(nfroyd)
(Assignee)

Updated

9 months ago
Blocks: 1328752
(Assignee)

Updated

9 months ago
Depends on: 1319850
Comment on attachment 8823884 [details] [diff] [review]
0001-Bug-1328747-Make-Android-UI-nsThread-and-MessageLoop-creation-asynchronous-r-17010416-05cd641.patch

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

That is some gnarly code.
Attachment #8823884 - Flags: review?(nfroyd) → review+

Comment 3

9 months ago
Pushed by rbarker@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ba48de0e021
Make Android UI nsThread and MessageLoop creation asynchronous r=froydnj

Comment 4

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7ba48de0e021
Status: NEW → RESOLVED
Last Resolved: 8 months ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
I see a perf win on some autophone devices:
== Change summary for alert #4774 (as of January 11 2017 17:42 UTC) ==

Improvements:

  5%  remote-twitter summary android-4-2-armv7-api15 opt     2722.36 -> 2587.08
  3%  remote-blank summary android-6-0-armv8-api15 opt       869.46 -> 843.8

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=4774
Comment on attachment 8823884 [details] [diff] [review]
0001-Bug-1328747-Make-Android-UI-nsThread-and-MessageLoop-creation-asynchronous-r-17010416-05cd641.patch

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

::: widget/android/AndroidUiThread.cpp
@@ +217,5 @@
> +  if (!sInitialized) {
> +    return nullptr;
> +  }
> +
> +  if (!sThread) {

Drive-by: this is potentially racy btw. you could be returning an uninitialized nsThread here. It's best to always take the lock or add memory barriers.

https://en.wikipedia.org/wiki/Double-checked_locking
(Assignee)

Comment 7

8 months ago
(In reply to Jim Chen [:jchen] [:darchons] from comment #6)
> Comment on attachment 8823884 [details] [diff] [review]
> 0001-Bug-1328747-Make-Android-UI-nsThread-and-MessageLoop-creation-
> asynchronous-r-17010416-05cd641.patch
> 
> Review of attachment 8823884 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/android/AndroidUiThread.cpp
> @@ +217,5 @@
> > +  if (!sInitialized) {
> > +    return nullptr;
> > +  }
> > +
> > +  if (!sThread) {
> 
> Drive-by: this is potentially racy btw. you could be returning an
> uninitialized nsThread here. It's best to always take the lock or add memory
> barriers.
> 
> https://en.wikipedia.org/wiki/Double-checked_locking

Good catch, I think I can also fix it by not assigning to the static vars until they have been initialized:

RefPtr<nsThread> thread = new nsThread;
// Initialize
sThread = thread;
// notify all

I'll create a quick patch and post.
(Assignee)

Updated

8 months ago
Blocks: 1331055
You need to log in before you can comment on or make changes to this bug.