Closed
Bug 1328747
Opened 8 years ago
Closed 8 years ago
Make Android UI nsThread and MessageLoop creation asynchronous
Categories
(GeckoView :: General, defect)
GeckoView
General
Tracking
(firefox53 fixed)
RESOLVED
FIXED
mozilla53
Tracking | Status | |
---|---|---|
firefox53 | --- | fixed |
People
(Reporter: rbarker, Assigned: rbarker)
References
Details
Attachments
(1 file)
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•8 years ago
|
Assignee: nobody → rbarker
Assignee | ||
Updated•8 years ago
|
Summary: Make Android UI nsThread and MessageLoop creation Asynchronous → Make Android UI nsThread and MessageLoop creation asynchronous
Assignee | ||
Comment 1•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Attachment #8823884 -
Flags: review?(nfroyd)
Comment 2•8 years ago
|
||
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+
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 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Comment 5•8 years ago
|
||
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 6•8 years ago
|
||
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 years 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.
Updated•6 years ago
|
Product: Firefox for Android → GeckoView
Updated•6 years ago
|
Target Milestone: Firefox 53 → mozilla53
You need to log in
before you can comment on or make changes to this bug.
Description
•