Closed
Bug 953005
Opened 11 years ago
Closed 10 years ago
[B2G] enhance the initialization of MobileConnectionArray to improve performance
Categories
(Core :: DOM: Device Interfaces, defect, P3)
Tracking
()
People
(Reporter: hsinyi, Assigned: hsinyi)
References
Details
(Keywords: perf, Whiteboard: [c=progress p= s=2014.01.17 u=])
Attachments
(2 files, 3 obsolete files)
3.86 KB,
patch
|
khuey
:
review+
|
Details | Diff | Splinter Review |
2.01 KB,
patch
|
Details | Diff | Splinter Review |
This bug was originally reported on bug 948307. With tests, it is shown that the idea of bug 948037 comment 7 is feasible. This bug is filed to address that.
Assignee | ||
Comment 1•11 years ago
|
||
Delaying the initialization of mobileConnections till the first time querying IndexGetter.
Assignee | ||
Comment 2•10 years ago
|
||
Attachment #8351328 -
Attachment is obsolete: true
Assignee | ||
Comment 3•10 years ago
|
||
Comment on attachment 8355465 [details] [diff] [review] 953005.patch - v2 - delay initialization Settings App is relying on the length of navigator.mozMobileConnections to show the correct layout, e.g. for single sim or for dual sim. However, we are encountering a performance, i.e. SIM manager is loaded about half second later than other items in Settings when first launch the list (see bug 948307). This patch proposes a solution that in MobileConnectionArray constructor we only set the array length but don't init the elements to make function return earlier. Instead, we delaying the initialization of mobileConnections till the first time querying IndexGetter. Bug 948307 comment14 shows the idea of the patch works. Kyle, may I have your review on this? Thanks.
Attachment #8355465 -
Flags: review?(khuey)
Comment on attachment 8355465 [details] [diff] [review] 953005.patch - v2 - delay initialization Review of attachment 8355465 [details] [diff] [review]: ----------------------------------------------------------------- I believe there is a bug in this patch. If we never initialize the mobile connection array, we will crash in MobileConnectionArray::DropConnections when we try to call Shutdown on a null connection pointer. Please add a test for that too. ::: dom/network/src/MobileConnectionArray.cpp @@ +56,5 @@ > + > + for (uint32_t id = 0; id < mMobileConnections.Length(); id++) { > + nsRefPtr<MobileConnection> mobileConnection = new MobileConnection(id); > + mobileConnection->Init(mWindow); > + mMobileConnections.ReplaceElementAt(id, mobileConnection); Any reason not to just write mMobileConnections[id] = mobileConnection? ::: dom/network/src/MobileConnectionArray.h @@ +51,5 @@ > + void > + DropConnections(); > + > + bool > + mInitialized; One line.
Attachment #8355465 -
Flags: review?(khuey) → review-
Comment 5•10 years ago
|
||
Hi Hsin-Yi Tsai, Just assigning it to you since you seem to be working on it. Please re-assign if this isn't the case. Thanks!
Assignee: nobody → htsai
Status: NEW → ASSIGNED
Keywords: perf
Priority: -- → P3
Whiteboard: [c=progress p= s= u=]
Assignee | ||
Comment 6•10 years ago
|
||
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #4) > Comment on attachment 8355465 [details] [diff] [review] > 953005.patch - v2 - delay initialization > > Review of attachment 8355465 [details] [diff] [review]: > ----------------------------------------------------------------- > > I believe there is a bug in this patch. If we never initialize the mobile > connection array, we will crash in MobileConnectionArray::DropConnections > when we try to call Shutdown on a null connection pointer. Hi Kyle, Thanks for the review. According to [1] and [2], a default constructor will be used for the newly inserted element when setLength. And we also MOZ_ASSERT() the return value of setLength() so there won't be a crash case you are concerned. Please let me know if I miss or misunderstand anything. [1] http://dxr.mozilla.org/mozilla-central/source/xpcom/glue/nsTArray.h?from=nsTArray#1386 [2] http://dxr.mozilla.org/mozilla-central/source/xpcom/glue/nsTArray.h?from=nsTArray#1428 > > Please add a test for that too. > > ::: dom/network/src/MobileConnectionArray.cpp > @@ +56,5 @@ > > + > > + for (uint32_t id = 0; id < mMobileConnections.Length(); id++) { > > + nsRefPtr<MobileConnection> mobileConnection = new MobileConnection(id); > > + mobileConnection->Init(mWindow); > > + mMobileConnections.ReplaceElementAt(id, mobileConnection); > > Any reason not to just write mMobileConnections[id] = mobileConnection? > No. I will provide an update per your comment. > ::: dom/network/src/MobileConnectionArray.h > @@ +51,5 @@ > > + void > > + DropConnections(); > > + > > + bool > > + mInitialized; > > One line. Here, as well. Thanks!
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #6) > (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #4) > > Comment on attachment 8355465 [details] [diff] [review] > > 953005.patch - v2 - delay initialization > > > > Review of attachment 8355465 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > I believe there is a bug in this patch. If we never initialize the mobile > > connection array, we will crash in MobileConnectionArray::DropConnections > > when we try to call Shutdown on a null connection pointer. > > Hi Kyle, > > Thanks for the review. > > According to [1] and [2], a default constructor will be used for the newly > inserted element when setLength. And we also MOZ_ASSERT() the return value > of setLength() so there won't be a crash case you are concerned. > > Please let me know if I miss or misunderstand anything. > > [1] > http://dxr.mozilla.org/mozilla-central/source/xpcom/glue/nsTArray. > h?from=nsTArray#1386 > [2] > http://dxr.mozilla.org/mozilla-central/source/xpcom/glue/nsTArray. > h?from=nsTArray#1428 Sure. So that will call nsRefPtr's constructor which will initialize the pointer to nullptr. And if Init() is never called, when we get to http://mxr.mozilla.org/mozilla-central/source/dom/network/src/MobileConnectionArray.cpp#58 we will iterate through the array and call Shutdown on null pointers.
Assignee | ||
Comment 8•10 years ago
|
||
Comment 4 addressed.
Attachment #8355465 -
Attachment is obsolete: true
Assignee | ||
Comment 9•10 years ago
|
||
Comment on attachment 8359720 [details] [diff] [review] 953005-v3.patch -- delay init Review of attachment 8359720 [details] [diff] [review]: ----------------------------------------------------------------- Hi Kyle, I've addressed your comments. Regarding test, I've tried to come out one and tried your suggestions as below with extending marionette timeout -- ifr.onload = function() { connections = ifr.contentWindow.navigator.mozMobileConnecitions; ok(connections); ifr = null; connections = null; } However, I cannot see crashes on it even without Init(). Per our discussion on irc, I'd like to move forward without a test at the moment. Please help take a look at the revised patch, thank you.
Attachment #8359720 -
Flags: review?(khuey)
I forgot the appendChild call, which is what makes the entire test run :P Blake and I verified that this test runs and reproduces the crash with the original version of your patch.
Comment on attachment 8359720 [details] [diff] [review] 953005-v3.patch -- delay init Review of attachment 8359720 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the test I attached included. Thanks!
Attachment #8359720 -
Flags: review?(khuey) → review+
Component: RIL → DOM: Device Interfaces
Product: Firefox OS → Core
(That test might also need a trivial ok() statement added to make it pass, not sure. We only tested the failure)
Assignee | ||
Comment 13•10 years ago
|
||
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #10) > Created attachment 8360052 [details] > test_mobileconnections_uninitialized.js > > I forgot the appendChild call, which is what makes the entire test run :P I had appendChild but forgot the removeChild. :( > Blake and I verified that this test runs and reproduces the crash with the > original version of your patch. (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #12) > (That test might also need a trivial ok() statement added to make it pass, > not sure. We only tested the failure) Yes, we need at least one check function. I'll complete this. Thank you!
Assignee | ||
Comment 14•10 years ago
|
||
Complete attachment 8360052 [details] by adding ok() checks, and adding the file into manifest
Attachment #8360052 -
Attachment is obsolete: true
Assignee | ||
Comment 15•10 years ago
|
||
Try https://tbpl.mozilla.org/?tree=Try&rev=15a06c0aecbb looks good. Failure in mochitest-debug-1 was fixed in bug 883975. Should safe to go.
Assignee | ||
Comment 16•10 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/3ab1320d665f https://hg.mozilla.org/integration/b2g-inbound/rev/76a2d72ab483
Comment 17•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3ab1320d665f https://hg.mozilla.org/mozilla-central/rev/76a2d72ab483
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Updated•10 years ago
|
Whiteboard: [c=progress p= s= u=] → [c=progress p= s=2014.01.17 u=]
The test landed doesn't actually test lazy initialization, it just tests a regression that this patch would have introduced.
Flags: in-testsuite+
Assignee | ||
Comment 19•10 years ago
|
||
nominate as 1.3+ because because this issue directly affects user experience (see bug 948307 also)
blocking-b2g: --- → 1.3?
Comment 21•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/d17316ce716e https://hg.mozilla.org/releases/mozilla-aurora/rev/0c0e7c3fd0ef
status-b2g-v1.3:
--- → fixed
status-b2g-v1.4:
--- → fixed
status-firefox27:
--- → wontfix
status-firefox28:
--- → fixed
status-firefox29:
--- → fixed
Flags: in-testsuite+
Flags: in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•