1057915 - Return multiple thread/message records via MobileMessageCursorCallback

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Vicamo Yang [:vicamo][:vyang]]

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

From bug 1038176 comment 80, Julien suggested that we may return a chunk of thread records at once. In MobileMessage, both getThreads() and getMessages() creates an DOMCursor and the control paths are actually unified as MobileMessageCursorCallback.

From bug 1038176 comment 101, we see great total load time improvement with such change. Here is a convenient copy:

            aggressiveness | unlimited |   7 [3]|   1  |  0
  -------------------------+-----------+--------+------+-----
  first panel load time [1]|       9x0 |    760 |  860 |  880
  -------------------------+-----------+--------+------+-----
       cursor done time [2]|      2200 |   3340 | 6500 | 6600
  -------------------------+-----------+--------+------+-----
   all panels load time [4]|      2950 |    n/a |  n/a | 6750

  unit: ms
  reference workload: medium

This bug is therefore cloned to host the review/land process of those experimental patches.
 
[1]: From moz-chrome-dom-loaded to |fistPanelCount === 0|
[2]: From "Getting thread list" to "notifyCallback(results[0], 0)"
[3]: Flame shows 7 threads on thread list.
[4]: From moz-chrome-dom-loaded to onThreadsRendered

Comment 1

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 1/2: DOM and IPDL changes

With this patch, nsIMobileMessageCursorCallback::notifyCursorResult is modified to accept an array of nsISupports objects. Both class MobileMessageCursorCallback and class MobileMessageCursorParent are accommodated to this change.

In MobileMessageCursorParent, we have to determine the objects are actually threads or messages first and pack corresponding raw IPC data into MobileMessageCursorData and send that to child process.

In MobileMessageCursorCallback, we always fire a success event with the first element of the received results, and store the rest into MobileMessageCursor::mResults in reversed order for further reference. We also subclass DOMCursor as MobileMessageCursor because we now have a non-trivial Continue() call. With this patch, it only calls DOMCursor::Continue() when mResults is empty, or it just takes the last element away from mResults and fires a success event.

Please help review these changes.

Comment 2

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 2/2: Gonk mmdb read-ahead.

This patch implements read-ahead functionality for mmdb for both message and thread queries. Mmdb may now try to fetch message/thread records before it's requested explicitly.

The read ahead behavior can be controlled by an integer mozSettings entry "ril.sms.maxReadAheadEntries" as well as an integer holding preference "dom.sms.maxReadAheadEntries". The meanings are:

  positive: finite read-ahead entries,
  0: don't read ahead unless explicitly requested, (default)
  negative: read ahead all IDs if possible.

In detail, the ResultsCollector is renamed to IdCollector because it holds only message/thread IDs only. The ResultsCollector is re-invented to host read-ahead logic. The order of ID filtering objects are now:

  [UnionResultsCollector]
  +-> [IntersectionResultsCollector]
      +-> IdCollector
          +-> ResultsCollector

ResultsCollector has basically similar behaviour with IdCollector. When RC::squeeze() is called, either RC::drip() is called instantly if we have already fetched results available, or the request is kept and IC::squeeze() is called.

When RC::collect is called by IC::drip, it proceeds to fetch the corresponding record unless we have "dom.sms.maxReadAheadEntries" set to zero. After the message/thread record being fetched, ResultsCollector::drip is called if we have pending request. Anyway, RC::maybeSqueezeAgain is called to determine whether we need to call IC::squeeze again.

RC::squeeze is called when nsICursorContinueCallback::handleContinue() is called. ResultsCollector::drip will call to nsIMobileMessageCursorCallback::notifyFoo.

Please help review this patch.

Comment 3

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #2)
> When RC::collect is called by IC::drip, it proceeds to fetch the
> corresponding record unless we have "dom.sms.maxReadAheadEntries" set to
> zero.

Sorry, the 'unless ...' condition is not necessary.

RC::collect() is only called by IC::drip(), and IC::drip() is only invoked because IC::squeeze() had been previously invoked. Therefore whenever RC::collect() is called, that means RC is requested to give some results, so it will always proceed to fetch the specified record given that ID is neither an error nor an end mark.

Comment 4

[Vicamo Yang [:vicamo][:vyang]]

Passed local Firefox/Fennec build, try all: https://tbpl.mozilla.org/?tree=Try&rev=53244f094de7

Comment 5

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 1/2: DOM and IPDL changes : v2

Fix build failures, -Werror=overloaded-virtual, in Linux/Android by overriding |virtual nsresult Continue(void);| as well. Try in https://tbpl.mozilla.org/?tree=Try&rev=bbb670b920ee

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8478113 [details] [diff] [review]
part 2/2: Gonk mmdb read-ahead.


>     getRequest.onerror = function(event) {
>+      // Error reporting is done in GetMessagesCursor.notify.
>+      event.stopPropagation();
>+      event.reventDefault();
reventDefault()?
This code sure can't run without exceptions

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8478296 [details] [diff] [review]
part 1/2: DOM and IPDL changes : v2

>diff --git a/dom/mobilemessage/src/MobileMessageCursorCallback.cpp b/dom/mobilemessage/src/MobileMessageCursorCallback.cpp
>--- a/dom/mobilemessage/src/MobileMessageCursorCallback.cpp
>+++ b/dom/mobilemessage/src/MobileMessageCursorCallback.cpp
>@@ -3,23 +3,74 @@
>  * License, v. 2.0. If a copy of the MPL was not distributed with this
>  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
> #include "MobileMessageCursorCallback.h"
> #include "mozilla/dom/ScriptSettings.h"
> #include "nsIDOMDOMRequest.h"
> #include "nsIDOMMozSmsMessage.h"
> #include "nsIMobileMessageCallback.h"
>-#include "DOMCursor.h"
> #include "nsServiceManagerUtils.h"      // for do_GetService
> 
> namespace mozilla {
> namespace dom {
> namespace mobilemessage {
> 
>+NS_IMPL_CYCLE_COLLECTION_INHERITED(MobileMessageCursor, DOMCursor,
>+                                   mResults)
>+
>+NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION_INHERITED(MobileMessageCursor)
>+NS_INTERFACE_MAP_END_INHERITING(DOMCursor)
>+
>+NS_IMPL_ADDREF_INHERITED(MobileMessageCursor, DOMCursor)
>+NS_IMPL_RELEASE_INHERITED(MobileMessageCursor, DOMCursor)
>+
>+MobileMessageCursor::MobileMessageCursor(nsPIDOMWindow* aWindow,
>+                                         nsICursorContinueCallback *aCallback)
>+  : DOMCursor(aWindow, aCallback)
>+{
>+  MOZ_COUNT_CTOR(MobileMessageCursor);
>+}
>+
>+nsresult
>+MobileMessageCursor::Continue()
>+{
>+  return DOMCursor::Continue();
>+}
>+
>+void
>+MobileMessageCursor::Continue(ErrorResult& aRv)
>+{
>+  if (!mResults.Length()) {
>+    DOMCursor::Continue(aRv);
>+    return;
>+  }
>+
>+  nsCOMPtr<nsISupports> result = mResults.LastElement();
>+
>+  AutoJSAPI jsapi;
>+  if (NS_WARN_IF(!jsapi.Init(GetOwner()))) {
>+    aRv.Throw(NS_ERROR_FAILURE);
>+    return;
>+  }
>+  JSContext* cx = jsapi.cx();
>+
>+  JS::Rooted<JS::Value> val(cx);
>+  nsresult rv = nsContentUtils::WrapNative(cx, result, &val);
>+  if (NS_FAILED(rv)) {
>+    aRv.Throw(rv);
>+    return;
>+  }
>+
>+  mResults.RemoveElementAt(mResults.Length() - 1);
>+
>+  Reset();
>+  FireSuccess(val);
Am I missing something here...
Here we call FireSuccess (which DOMCursor::Continue doesn't do)
and pass the last element of mResults to it


>+MobileMessageCursorCallback::NotifyCursorResult(nsISupports** aResults,
>+                                                uint32_t aSize)
> {
>   MOZ_ASSERT(mDOMCursor);
>+  MOZ_ASSERT(aResults && *aResults && aSize);
> 
>   AutoJSAPI jsapi;
>   if (NS_WARN_IF(!jsapi.Init(mDOMCursor->GetOwner()))) {
>     return NS_ERROR_FAILURE;
>   }
>   JSContext* cx = jsapi.cx();
> 
>   JS::Rooted<JS::Value> wrappedResult(cx);
>-  nsresult rv = nsContentUtils::WrapNative(cx, aResult, &wrappedResult);
>+  nsresult rv = nsContentUtils::WrapNative(cx, aResults[0], &wrappedResult);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>+  // Push additional results in reversed order.
>+  while (aSize > 1) {
>+    --aSize;
>+    mDOMCursor->mResults.AppendElement(aResults[aSize]);
>+  }
>+
>   mDOMCursor->FireSuccess(wrappedResult);
Here we call FireSuccess and pass the first element from aResults.



>-MobileMessageCursorParent::NotifyCursorResult(nsISupports* aResult)
>+MobileMessageCursorParent::NotifyCursorResult(nsISupports** aResults,
>+                                              uint32_t aSize)
> {
>+  MOZ_ASSERT(aResults && *aResults && aSize);
Odd MOZ_ASSERT. Nothing prevents the caller to pass 0 size array.
So, NS_ENSURE_STATE(aResults && *aResults && aSize); perhaps


>+  nsCOMPtr<nsIDOMMozMobileMessageThread> iThread = do_QueryInterface(aResults[0]);
>+  if (iThread) {
>+    ThreadArrayData arrayData;
>+
>+    for (uint32_t i = 0; i < aSize; i++) {
>+      MobileMessageThread* thread = static_cast<MobileMessageThread*>(aResults[i]);
I would prefer if you'd QI all the items from aResults to nsIDOMMozMobileMessageThread
before doing static_cast. And null check, please.



So I really don't understand the setup here.
Why do we need to store the array in the cursor and not just notify about all the new data?
And if that is not wanted, could we at least do the array handling and nsContentUtils::WrapNative etc just in
one place

Comment 8

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Olli Pettay [:smaug] from comment #7)
> Comment on attachment 8478296 [details] [diff] [review]
> part 1/2: DOM and IPDL changes : v2
> 
> >diff --git a/dom/mobilemessage/src/MobileMessageCursorCallback.cpp b/dom/mobilemessage/src/MobileMessageCursorCallback.cpp
> >--- a/dom/mobilemessage/src/MobileMessageCursorCallback.cpp
> >+++ b/dom/mobilemessage/src/MobileMessageCursorCallback.cpp
> >@@ -3,23 +3,74 @@
> >  * License, v. 2.0. If a copy of the MPL was not distributed with this
> >  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > 
> > #include "MobileMessageCursorCallback.h"
> > #include "mozilla/dom/ScriptSettings.h"
> > #include "nsIDOMDOMRequest.h"
> > #include "nsIDOMMozSmsMessage.h"
> > #include "nsIMobileMessageCallback.h"
> >-#include "DOMCursor.h"
> > #include "nsServiceManagerUtils.h"      // for do_GetService
> > 
> > namespace mozilla {
> > namespace dom {
> > namespace mobilemessage {
> > 
> >+NS_IMPL_CYCLE_COLLECTION_INHERITED(MobileMessageCursor, DOMCursor,
> >+                                   mResults)
> >+
> >+NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION_INHERITED(MobileMessageCursor)
> >+NS_INTERFACE_MAP_END_INHERITING(DOMCursor)
> >+
> >+NS_IMPL_ADDREF_INHERITED(MobileMessageCursor, DOMCursor)
> >+NS_IMPL_RELEASE_INHERITED(MobileMessageCursor, DOMCursor)
> >+
> >+MobileMessageCursor::MobileMessageCursor(nsPIDOMWindow* aWindow,
> >+                                         nsICursorContinueCallback *aCallback)
> >+  : DOMCursor(aWindow, aCallback)
> >+{
> >+  MOZ_COUNT_CTOR(MobileMessageCursor);
> >+}
> >+
> >+nsresult
> >+MobileMessageCursor::Continue()
> >+{
> >+  return DOMCursor::Continue();
> >+}
> >+
> >+void
> >+MobileMessageCursor::Continue(ErrorResult& aRv)
> >+{
> >+  if (!mResults.Length()) {
> >+    DOMCursor::Continue(aRv);
> >+    return;
> >+  }
> >+
> >+  nsCOMPtr<nsISupports> result = mResults.LastElement();
> >+
> >+  AutoJSAPI jsapi;
> >+  if (NS_WARN_IF(!jsapi.Init(GetOwner()))) {
> >+    aRv.Throw(NS_ERROR_FAILURE);
> >+    return;
> >+  }
> >+  JSContext* cx = jsapi.cx();
> >+
> >+  JS::Rooted<JS::Value> val(cx);
> >+  nsresult rv = nsContentUtils::WrapNative(cx, result, &val);
> >+  if (NS_FAILED(rv)) {
> >+    aRv.Throw(rv);
> >+    return;
> >+  }
> >+
> >+  mResults.RemoveElementAt(mResults.Length() - 1);
> >+
> >+  Reset();
> >+  FireSuccess(val);
> Am I missing something here...
> Here we call FireSuccess (which DOMCursor::Continue doesn't do)
> and pass the last element of mResults to it
> 
> 
> >+MobileMessageCursorCallback::NotifyCursorResult(nsISupports** aResults,
> >+                                                uint32_t aSize)
> > {
> >   MOZ_ASSERT(mDOMCursor);
> >+  MOZ_ASSERT(aResults && *aResults && aSize);
> > 
> >   AutoJSAPI jsapi;
> >   if (NS_WARN_IF(!jsapi.Init(mDOMCursor->GetOwner()))) {
> >     return NS_ERROR_FAILURE;
> >   }
> >   JSContext* cx = jsapi.cx();
> > 
> >   JS::Rooted<JS::Value> wrappedResult(cx);
> >-  nsresult rv = nsContentUtils::WrapNative(cx, aResult, &wrappedResult);
> >+  nsresult rv = nsContentUtils::WrapNative(cx, aResults[0], &wrappedResult);
> >   NS_ENSURE_SUCCESS(rv, rv);
> > 
> >+  // Push additional results in reversed order.
> >+  while (aSize > 1) {
> >+    --aSize;
> >+    mDOMCursor->mResults.AppendElement(aResults[aSize]);
> >+  }
> >+
> >   mDOMCursor->FireSuccess(wrappedResult);
> Here we call FireSuccess and pass the first element from aResults.
> 
> 
> 
> >-MobileMessageCursorParent::NotifyCursorResult(nsISupports* aResult)
> >+MobileMessageCursorParent::NotifyCursorResult(nsISupports** aResults,
> >+                                              uint32_t aSize)
> > {
> >+  MOZ_ASSERT(aResults && *aResults && aSize);
> Odd MOZ_ASSERT. Nothing prevents the caller to pass 0 size array.
> So, NS_ENSURE_STATE(aResults && *aResults && aSize); perhaps

I placed a MOZ_ASSERT to clearly declare that passing an empty array is an error to this function. We should only call NotifyCursorResult when there is at least one valid result. Otherwise, call NotifyCursorDone instead.

> >+  nsCOMPtr<nsIDOMMozMobileMessageThread> iThread = do_QueryInterface(aResults[0]);
> >+  if (iThread) {
> >+    ThreadArrayData arrayData;
> >+
> >+    for (uint32_t i = 0; i < aSize; i++) {
> >+      MobileMessageThread* thread = static_cast<MobileMessageThread*>(aResults[i]);
> I would prefer if you'd QI all the items from aResults to
> nsIDOMMozMobileMessageThread
> before doing static_cast. And null check, please.

We have this bug because we want to improve the performance of these DOMCursor based APIs. The passed is always either an array of (SMS or MMS) or an array of threads. They never get mixed. Even they do, that's an fatal error and is considered a bug in MobileMessageDB.

How about having a check only in debug build?

> So I really don't understand the setup here.
> Why do we need to store the array in the cursor and not just notify about
> all the new data?

Because Gaia is expecting one object per onsuccess event. Please see bug 1038176 comment 97.

> And if that is not wanted, could we at least do the array handling and
> nsContentUtils::WrapNative etc just in
> one place

Will do.

Comment 9

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8478296 [details] [diff] [review]
part 1/2: DOM and IPDL changes : v2

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

r+ for nsIMobileMessageCursorCallback.idl, MobileMessageDB.jsm and mmdb_head.js. DOM and IPC need DOM peers review. Thank you!

Comment 10

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 1/2: DOM and IPDL changes : v3

Address previous review comments. May I have your review again?

Comment 11

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 2/2: Gonk mmdb read-ahead : v2

1) s/reventDefault/preventDefault/

2) Rename 'IdCollector' to 'IDsCollector' to follow naming of all other collector classes.

3) Rename 'ResultsCollector::maybeSqueezeAgain' to 'maybeSqueezeIdCollector' so that we don't confuse with collect to squeeze.

4) Add more comments.

5) Set default value of "dom.sms.maxReadAheadEntries" to 7 in B2G as suggested by :schung because we have usually 6 ~ 8 items in thread list view in Gaia Messages app.

Comment 12

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8478975 [details] [diff] [review]
part 2/2: Gonk mmdb read-ahead : v2

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

Nice patch to read ahead the threads/messages between cursor.continue() invocations.
Please see my questions/suggestions inline.

::: dom/mobilemessage/src/gonk/MobileMessageDB.jsm
@@ +3510,5 @@
> + *   +-> RC::drip
> + *       +-> RC::notifyCallback
> + *           +-> nsIMobileMessageCursorCallback::notifyFoo
> + *   +-> RC::maybeSqueezeIdCollector
> + *       +-> IC::squeeze

*       o-> IC::squeeze
Looks optional to me.

@@ +3732,5 @@
> +  drip: function(callback) {
> +    let results = this.results;
> +    this.results = [];
> +
> +    let lastId = this.done ? this.lastId : 0;

Looks not necessary to me.
By my understanding,
if !this.done, which implies this.lastId is equal to null. (Not set)
if this.done, this.lastId was set to either COLLECT_ID_END or COLLECT_ID_ERROR.

Hence, we can just use |this.done| + |this.lastId| in |notifyCallback()| to invoke either |callback.notifyCursorDone()| or |callback.notifyCursorError()| accordingly.

@@ +4071,3 @@
>      };
>      getRequest.onerror = function(event) {
> +      // Error reporting is done in GetMessagesCursor.notify.

GetMessagesCursor.notify is undefined.
Do you mean |ResultsCollector::notifyCallback()| instead? That's a little bit too far from here.

@@ +4071,5 @@
>      };
>      getRequest.onerror = function(event) {
> +      // Error reporting is done in GetMessagesCursor.notify.
> +      event.stopPropagation();
> +      event.preventDefault();

Can we have more specific comment of why event.stopPropagation() & event.preventDefault() is needed here?

@@ +4146,3 @@
>      };
>      getRequest.onerror = function(event) {
> +      // Error reporting is done in GetThreadCursor.notify.

GetThreadCursor.notify is undefined.
Do you mean |ResultsCollector::notifyCallback()| instead? That's a little bit too far from here.

@@ +4146,5 @@
>      };
>      getRequest.onerror = function(event) {
> +      // Error reporting is done in GetThreadCursor.notify.
> +      event.stopPropagation();
> +      event.preventDefault();

Can we have more specific comment of why event.stopPropagation() & event.preventDefault() is needed here?

Comment 13

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 2/2: Gonk mmdb read-ahead : v3

Address previous review comments.

Comment 14

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8479629 [details] [diff] [review]
part 2/2: Gonk mmdb read-ahead : v3

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

Thank you!

Comment 15

[Ken Chang[:kenkai]]

This bug blocks a 2.0+ bug. That means it is a 2.0+ blocker as well.

Comment 16

[Ken Chang[:kenkai]]

This is a 2.0+ blocker. Please have reviews on patches. Thanks very much.

Comment 17

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

Could you please rebase your patch for v2.0 ?

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

(No idea what the needinfo was for. This is in my review queue sure, but that queue happens to be exceptionally long atm.)

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

 
> > >-MobileMessageCursorParent::NotifyCursorResult(nsISupports* aResult)
> > >+MobileMessageCursorParent::NotifyCursorResult(nsISupports** aResults,
> > >+                                              uint32_t aSize)
> > > {
> > >+  MOZ_ASSERT(aResults && *aResults && aSize);
> > Odd MOZ_ASSERT. Nothing prevents the caller to pass 0 size array.
> > So, NS_ENSURE_STATE(aResults && *aResults && aSize); perhaps
> 
> I placed a MOZ_ASSERT to clearly declare that passing an empty array is an
> error to this function. We should only call NotifyCursorResult when there is
> at least one valid result. Otherwise, call NotifyCursorDone instead.
Remember that MOZ_ASSERT doesn't do anything in release builds.


> We have this bug because we want to improve the performance of these
> DOMCursor based APIs. The passed is always either an array of (SMS or MMS)
> or an array of threads. They never get mixed. Even they do, that's an fatal
> error and is considered a bug in MobileMessageDB.
> 
> How about having a check only in debug build?
Not too happy. If the QI ever fails, it means a critical security bug in the non-debug builds.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8478973 [details] [diff] [review]
part 1/2: DOM and IPDL changes : v3

>+MobileMessageCursor::MobileMessageCursor(nsPIDOMWindow* aWindow,
>+                                         nsICursorContinueCallback *aCallback)
nsICursorContinueCallback* aCallback


>+MobileMessageCursor::Continue()
>+{
>+  return DOMCursor::Continue();
So this will end up calling the Continue method below, right?


>+}
>+
>+void
>+MobileMessageCursor::Continue(ErrorResult& aRv)
>+{
>+  if (!mResults.Length()) {
>+    DOMCursor::Continue(aRv);
>+    return;
>+  }
>+
>+  nsCOMPtr<nsISupports> result = mResults.LastElement();
>+  mResults.RemoveElementAt(mResults.Length() - 1);
>+
>+  Reset();
This Reset needs a comment.


>+MobileMessageCursorCallback::NotifyCursorResult(nsISupports** aResults,
>+                                                uint32_t aSize)
> {
>   MOZ_ASSERT(mDOMCursor);
>+  MOZ_ASSERT(aResults && *aResults && aSize);
> 
>-  AutoJSAPI jsapi;
>-  if (NS_WARN_IF(!jsapi.Init(mDOMCursor->GetOwner()))) {
>-    return NS_ERROR_FAILURE;
>+  nsresult rv = mDOMCursor->FireResult(aResults[0]);
So why do we not need Reset() here?

>+  if (NS_FAILED(rv)) {
>+    NotifyCursorError(nsIMobileMessageCallback::INTERNAL_ERROR);
>+    return rv;
>   }
>-  JSContext* cx = jsapi.cx();
> 
>-  JS::Rooted<JS::Value> wrappedResult(cx);
>-  nsresult rv = nsContentUtils::WrapNative(cx, aResult, &wrappedResult);
>-  NS_ENSURE_SUCCESS(rv, rv);
>+  // Push additional results in reversed order.
>+  while (aSize > 1) {
>+    --aSize;
>+    mDOMCursor->mResults.AppendElement(aResults[aSize]);
>+  }
I'd expect us to first append all the elements to results, and then call FireResult.
That way things should work correctly even in the unusual case when FireResult ends up spinning event loop.
>+MobileMessageCursorChild::DoNotifyResult(const nsTArray<MobileMessageData>& aDataArray)
>+{
>+  const uint32_t length = aDataArray.Length();
>+  MOZ_ASSERT(length);
>+
>+  if (length == 1) {
>+    nsCOMPtr<nsISupports> message = CreateMessageFromMessageData(aDataArray[0]);
>+    nsISupports* ptr = message.get();
>+    mCursorCallback->NotifyCursorResult(&ptr, 1);
>+    return;
>+  }
>+
>+  nsAutoArrayPtr<nsISupports*> autoArray(new nsISupports* [length]);
I would make this nsAutoTArray<nsISupports*>, 1> 
and then call SetCapacity(length)


>+
>+  FallibleTArray<nsCOMPtr<nsISupports>> messages;
And this nsAutoTArray<nsCOMPtr<nsISupports>, 1>

and then not special case length == 1.
(nsAutoTArray has internal buffer, so in case of just one element in the array it wouldn't do any malloc/free)


>+  for (uint32_t i = 0; i < length; i++) {
>+    nsCOMPtr<nsISupports> message = CreateMessageFromMessageData(aDataArray[i]);
>+    NS_ENSURE_TRUE_VOID(messages.AppendElement(message));
>+
>+    autoArray[i] = message.get();
>+  }
>+
>+  mCursorCallback->NotifyCursorResult(autoArray, length);
that would mean you'd need to pass autoArray.Elements() here, not autoArray.


>+void
>+MobileMessageCursorChild::DoNotifyResult(const nsTArray<ThreadData>& aDataArray)
>+{
>+  const uint32_t length = aDataArray.Length();
>+  MOZ_ASSERT(length);
>+
>+  if (length == 1) {
>+    nsCOMPtr<nsISupports> thread = new MobileMessageThread(aDataArray[0]);
>+    nsISupports* ptr = thread.get();
>+    mCursorCallback->NotifyCursorResult(&ptr, 1);
>+    return;
>+  }
>+
>+  nsAutoArrayPtr<nsISupports*> autoArray(new nsISupports* [length]);
>+
>+  FallibleTArray<nsCOMPtr<nsISupports>> threads;
>+  for (uint32_t i = 0; i < length; i++) {
>+    nsCOMPtr<nsISupports> thread = new MobileMessageThread(aDataArray[i]);
>+    NS_ENSURE_TRUE_VOID(threads.AppendElement(thread));
>+
>+    autoArray[i] = thread.get();
>+  }
>+
>+  mCursorCallback->NotifyCursorResult(autoArray, length);
similar stuff in this method.


I think those done, r+, but I could look at still the new patch.

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8479629 [details] [diff] [review]
part 2/2: Gonk mmdb read-ahead : v3

>@@ -3933,16 +3933,22 @@ pref("dom.image.picture.enabled", false)
> pref("dom.sms.enabled", false);
> // Enable Latin characters replacement with corresponding ones in GSM SMS
> // 7-bit default alphabet.
> pref("dom.sms.strict7BitEncoding", false);
> pref("dom.sms.requestStatusReport", true);
> // Numeric default service id for SMS API calls with |serviceId| parameter
> // omitted.
> pref("dom.sms.defaultServiceId", 0);
>+// MobileMessage GetMessages/GetThreads read ahead aggressiveness.
>+//
>+// positive: finite read-ahead entries,
>+// 0: don't read ahead unless explicitly requested, (default)
>+// negative: read ahead all IDs if possible.
>+pref("dom.sms.maxReadAheadEntries", 0);
I'd prefer to not have this at all, and only if the pref is set to >0, use readahead


Other that that, I'm really not familiar with this code, so don't feel qualified to give r+, or r-.
(If that is needed, I could read all the MobileMessageDB.jsm and related code, but that takes quite a bit time.)

Is btseng's review enough for this?

Comment 22

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Olli Pettay [:smaug] from comment #21)
> Comment on attachment 8479629 [details] [diff] [review]
> part 2/2: Gonk mmdb read-ahead : v3
> 
> >@@ -3933,16 +3933,22 @@ pref("dom.image.picture.enabled", false)
> > pref("dom.sms.enabled", false);
> > // Enable Latin characters replacement with corresponding ones in GSM SMS
> > // 7-bit default alphabet.
> > pref("dom.sms.strict7BitEncoding", false);
> > pref("dom.sms.requestStatusReport", true);
> > // Numeric default service id for SMS API calls with |serviceId| parameter
> > // omitted.
> > pref("dom.sms.defaultServiceId", 0);
> >+// MobileMessage GetMessages/GetThreads read ahead aggressiveness.
> >+//
> >+// positive: finite read-ahead entries,
> >+// 0: don't read ahead unless explicitly requested, (default)
> >+// negative: read ahead all IDs if possible.
> >+pref("dom.sms.maxReadAheadEntries", 0);
> I'd prefer to not have this at all, and only if the pref is set to >0, use
> readahead

Please see the comment 0. The aggressiveness of this mechanism may affect the content page load time vastly. For B2G, most of our devices have 7 to 9 items in messages app thread list UI, but that does not mean all. Tarako, an extreme low cost device, has even less items due to a smaller screen and has greater memory pressure in comparison to others because it has only 128 MB ram. I left a configurable preference on purpose so that Gaia customization process may have a way to tweak it.

> Other that that, I'm really not familiar with this code, so don't feel
> qualified to give r+, or r-.
> (If that is needed, I could read all the MobileMessageDB.jsm and related
> code, but that takes quite a bit time.)
> 
> Is btseng's review enough for this?

Yes. I thought you're interested in mmdb as well since you left a comment in comment 6.

Comment 23

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Olli Pettay [:smaug] from comment #20)
> Comment on attachment 8478973 [details] [diff] [review]
> part 1/2: DOM and IPDL changes : v3
> 
> >+MobileMessageCursor::MobileMessageCursor(nsPIDOMWindow* aWindow,
> >+                                         nsICursorContinueCallback *aCallback)
> nsICursorContinueCallback* aCallback
> 
> 
> >+MobileMessageCursor::Continue()
> >+{
> >+  return DOMCursor::Continue();
> So this will end up calling the Continue method below, right?

Yes. We have originally:

  DOMCursor::Continue()
  +-> DOMCursor::Continue(ErrorResult& aRv)

Now it becomes:

  MobileMessageCursor::Continue()
  +-> DOMCursor::Continue()
      +-> MobileMessageCursor::Continue(ErrorResult& aRv)
          o-> DOMCursor::Continue(ErrorResult& aRv)
          o-> MobileMessageCursor::FireResult()

> >+}
> >+
> >+void
> >+MobileMessageCursor::Continue(ErrorResult& aRv)
> >+{
> >+  if (!mResults.Length()) {
> >+    DOMCursor::Continue(aRv);
> >+    return;
> >+  }
> >+
> >+  nsCOMPtr<nsISupports> result = mResults.LastElement();
> >+  mResults.RemoveElementAt(mResults.Length() - 1);
> >+
> >+  Reset();
> This Reset needs a comment.
> 
> 
> >+MobileMessageCursorCallback::NotifyCursorResult(nsISupports** aResults,
> >+                                                uint32_t aSize)
> > {
> >   MOZ_ASSERT(mDOMCursor);
> >+  MOZ_ASSERT(aResults && *aResults && aSize);
> > 
> >-  AutoJSAPI jsapi;
> >-  if (NS_WARN_IF(!jsapi.Init(mDOMCursor->GetOwner()))) {
> >-    return NS_ERROR_FAILURE;
> >+  nsresult rv = mDOMCursor->FireResult(aResults[0]);
> So why do we not need Reset() here?

We don't call to Reset() because when nsIMobileMessageCursorCallback::NotifyCursorResult() has been invoked previously in DOMCursor::Continue().

  DOMCursor::Continue()
  +-> DOMCursor::Reset()
  +-> nsICursorContinueCallback::HandleContinue()
      +-> nsIMobileMessageCursorCallback::NotifyCursorResult()
          +-> DOMCursor::FireSuccess()

I'll add all these as comments.

Comment 24

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 1/2: DOM and IPDL changes : v4

1) Rebase after bug 1058101.

2) Address previous review comments in comment 20.

3) Fix Windows build errors.

4) Move MobileMessageCursor up to namespace mozilla::dom because it's an DOM component returned to content page.

See try results in
https://tbpl.mozilla.org/?tree=Try&rev=57b600c3d3a7 and https://tbpl.mozilla.org/?tree=Try&rev=5b85213f86a4

Comment 25

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 2/2: Gonk mmdb read-ahead : v4

Rebase after bug 1058101, bug 878533.

Comment 26

[Vicamo Yang [:vicamo][:vyang]]

Attachment: v2.0 part 1/2: DOM and IPDL changes

Comment 27

[Vicamo Yang [:vicamo][:vyang]]

Attachment: v2.0 part 2/2: Gonk mmdb read-ahead

Comment 28

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 1/2: DOM and IPDL changes : v5

Fix debug build.

Comment 29

[Vicamo Yang [:vicamo][:vyang]]

Attachment: v2.0 part 1/2: DOM and IPDL changes : v2

Fix debug build.

Comment 30

[Olli Pettay [:smaug][[e-mail]]]

> 
> Yes. I thought you're interested in mmdb as well since you left a comment in
> comment 6.
Oh, I was just trying to see the big picture :)

Comment 31

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8482616 [details] [diff] [review]
part 1/2: DOM and IPDL changes : v5

>+MobileMessageCursorChild::DoNotifyResult(const nsTArray<MobileMessageData>& aDataArray)
>+{
>+  const uint32_t length = aDataArray.Length();
>+  MOZ_ASSERT(length);
>+
>+  if (length == 1) {
>+    nsCOMPtr<nsISupports> message = CreateMessageFromMessageData(aDataArray[0]);
>+    nsISupports* ptr = message.get();
>+    mCursorCallback->NotifyCursorResult(&ptr, 1);
>+    return;
>+  }
You could just drop the whole if (length == 1) {
The autoTArray<,1> should deal with that case efficiently enough.

>+
>+  AutoFallibleTArray<nsISupports*, 1> autoArray;
>+  NS_ENSURE_TRUE_VOID(autoArray.SetCapacity(aDataArray.Length()));
>+
>+  FallibleTArray<nsCOMPtr<nsISupports>> messages;
Make also this Auto*TArray <*, 1>.


>+MobileMessageCursorChild::DoNotifyResult(const nsTArray<ThreadData>& aDataArray)
>+{
>+  const uint32_t length = aDataArray.Length();
>+  MOZ_ASSERT(length);
>+
>+  if (length == 1) {
>+    nsCOMPtr<nsISupports> thread = new MobileMessageThread(aDataArray[0]);
>+    nsISupports* ptr = thread.get();
>+    mCursorCallback->NotifyCursorResult(&ptr, 1);
>+    return;
>+  }
>+
>+  AutoFallibleTArray<nsISupports*, 1> autoArray;
>+  NS_ENSURE_TRUE_VOID(autoArray.SetCapacity(aDataArray.Length()));
>+
>+  FallibleTArray<nsCOMPtr<nsISupports>> threads;
>+  for (uint32_t i = 0; i < length; i++) {
>+    nsCOMPtr<nsISupports> thread = new MobileMessageThread(aDataArray[i]);
>+    NS_ENSURE_TRUE_VOID(threads.AppendElement(thread));
>+    NS_ENSURE_TRUE_VOID(autoArray.AppendElement(thread.get()));
>+  }
Same things here. Drop the 'if' and us Auto*TArray for both arrays

Comment 32

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Olli Pettay [:smaug] from comment #31)
> Comment on attachment 8482616 [details] [diff] [review]
> part 1/2: DOM and IPDL changes : v5
> 
> >+  AutoFallibleTArray<nsISupports*, 1> autoArray;
> >+  NS_ENSURE_TRUE_VOID(autoArray.SetCapacity(aDataArray.Length()));
> >+
> >+  FallibleTArray<nsCOMPtr<nsISupports>> messages;
> Make also this Auto*TArray <*, 1>.

That follows we may be owning an array of dangling pointers if some error occurs during the process. The previously collected messages/threads objects won't be automatically freed.

Comment 33

[Ken Chang[:kenkai]]

(In reply to Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me) from comment #17)
> Could you please rebase your patch for v2.0 ?
Vicamo had provided patches for 2.0, can you please try them first? Thanks.

Comment 34

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

I picked 2 patches from Comment 27 and Comment 29. I am asking our internal test team to test SMS launch latency again. Please let me know if I need to pickup any other patches .

Comment 35

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #32)
> (In reply to Olli Pettay [:smaug] from comment #31)
> > Comment on attachment 8482616 [details] [diff] [review]
> > part 1/2: DOM and IPDL changes : v5
> > 
> > >+  AutoFallibleTArray<nsISupports*, 1> autoArray;
> > >+  NS_ENSURE_TRUE_VOID(autoArray.SetCapacity(aDataArray.Length()));
> > >+
> > >+  FallibleTArray<nsCOMPtr<nsISupports>> messages;
> > Make also this Auto*TArray <*, 1>.
> 
> That follows we may be owning an array of dangling pointers if some error
> occurs during the process. The previously collected messages/threads objects
> won't be automatically freed.

Auto* variant of TArray just makes the TArray to have internal buffer so that it doesn't need to
do malloc/free when few enough items are added to it. 
So where would the dangling points come here?

Oh, I didn't mean AutoFallibleTArray <nsISupports*, 1>, but
AutoFallibleTArray<nsCOMPtr<nsISupports>> messages;

Comment 36

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Olli Pettay [:smaug] from comment #35)
> Oh, I didn't mean AutoFallibleTArray <nsISupports*, 1>, but
> AutoFallibleTArray<nsCOMPtr<nsISupports>> messages;

Then this makes sense. Updating patches ...

Comment 37

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 1/2: DOM and IPDL changes : v6

Address review comment 31.

Comment 38

[Vicamo Yang [:vicamo][:vyang]]

Attachment: v2.0 part 1/2: DOM and IPDL changes : v3

Address review comment 31.

Comment 39

[Vicamo Yang [:vicamo][:vyang]]

https://hg.mozilla.org/integration/b2g-inbound/rev/0eb6e8c124c8
https://hg.mozilla.org/integration/b2g-inbound/rev/4e0b98f7ed73

Comment 40

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0eb6e8c124c8
https://hg.mozilla.org/mozilla-central/rev/4e0b98f7ed73

Comment 41

[Ryan VanderMeulen [:RyanVM]]

Note that due to recent policy changes, all patches need approval for uplift regardless of blocking status. Please request b2g32 and aurora approval on these when you get a chance. Sorry for the inconvenience :(

Comment 42

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #41)
> Note that due to recent policy changes, all patches need approval for uplift
> regardless of blocking status. Please request b2g32 and aurora approval on
> these when you get a chance. Sorry for the inconvenience :(

Not at all. I was waiting it being merged to m-c before filing approval requests.

Comment 43

[Ken Chang[:kenkai]]

(In reply to Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me) from comment #34)
> I picked 2 patches from Comment 27 and Comment 29. I am asking our internal
> test team to test SMS launch latency again. Please let me know if I need to
> pickup any other patches .

Do you have the test result?

Comment 44

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

Hi Ken,

I provided my observation in bug 1038176 comment 110. Thanks for the good work !

Comment 45

[Ryan VanderMeulen [:RyanVM]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #42)
> Not at all. I was waiting it being merged to m-c before filing approval
> requests.

How about now? :)

Comment 46

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #45)
> (In reply to Vicamo Yang [:vicamo][:vyang] from comment #42)
> > Not at all. I was waiting it being merged to m-c before filing approval
> > requests.
> 
> How about now? :)

With bug 1038176 comment 112, bug 1038176 comment 113, Yes. But I'd like to try again with Flame Gonk 123 to prevent bug 1062990 from happening again.

Comment 47

[Vicamo Yang [:vicamo][:vyang]]

Attachment: v2.0 part 1/2: DOM and IPDL changes : v4

Rebase after bug 1046026.

Comment 48

[Vicamo Yang [:vicamo][:vyang]]

Attachment: v2.0 part 2/2: Gonk mmdb read-ahead : v2

Rebase after bug 1046026.

Comment 49

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8486167 [details] [diff] [review]
v2.0 part 1/2: DOM and IPDL changes : v4

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Unknown. See bug 1038176.

User impact if declined:
According to bug 1038176 comment 112, this patch brings 300ms launch time improvement for sms app.

Testing completed:
1) flash gecko & gaia only to Flame with Gonk v123
2) flash whole system to Flame with Gonk v123

Risk to taking this patch (and alternatives if risky):
Interface changes, binary incompatible with existing vendor RIL. No alternatives so far because such mechanism change is what this bug is meant for.

String or UUID changes made by this patch:
8fd0dba2-032e-4190-a751-07cc3782e93e (nsIMobileMessageCursorCallback) => 134a6958-543b-46e2-b419-4631a2314164

Comment 50

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8486168 [details] [diff] [review]
v2.0 part 2/2: Gonk mmdb read-ahead : v2

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
See comment 49.

Comment 51

[Ryan VanderMeulen [:RyanVM]]

Does this need to land on Aurora for v2.1 as well?

Comment 52

[bhavana bajaj [:bajaj]]

Comment on attachment 8486168 [details] [diff] [review]
v2.0 part 2/2: Gonk mmdb read-ahead : v2

Vicamo, not sure if the same patches apply cleanly on aurora, or what the plan is there, can you comment.

Comment 53

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/1268579cb432
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/d7daa6137200

Comment 54

[Kai-Zhen Li [:seinlin][:kli]]

http://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/1268579cb432
http://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/d7daa6137200

Comment 55

[William Lachance (:wlach)]

FYI, eideticker detected a substantial improvement after this landed:

http://eideticker.mozilla.org/#/b2g/flame-319/2.0/b2g-messages-startup/timetostableframe/90

Comment 56

[Ryan VanderMeulen [:RyanVM]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #51)
> Does this need to land on Aurora for v2.1 as well?

(In reply to bhavana bajaj [:bajaj] from comment #52)
> Vicamo, not sure if the same patches apply cleanly on aurora, or what the
> plan is there, can you comment.

????

Comment 57

[Julien Wajsberg [:julienw]]

I'm quite sure we want this in v2.1 given the performance improvements. I think Vicamo added a NI on himself to try the patches on aurora (and possibly do branch-specific patches).

Comment 58

[Vicamo Yang [:vicamo][:vyang]]

Attachment: aurora part 1/2: DOM and IPDL changes

1) Identical to attachment 8482555 [details] [diff] [review] except for the commit summary (with a=bajaj).

2) Applies to both aurora and b2g-v2.1 because their dom/mobilemessage are identical.

Comment 59

[Vicamo Yang [:vicamo][:vyang]]

Attachment: aurora part 2/2: Gonk mmdb read-ahead

1) Identical to attachment 8482555 [details] [diff] [review] except for the commit summary (with a=bajaj).

2) Applies to both aurora and b2g-v2.1 because their dom/mobilemessage are identical.

Comment 60

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #58)
> Created attachment 8494943 [details] [diff] [review]
> aurora part 1/2: DOM and IPDL changes
> 
> 1) Identical to attachment 8482555 [details] [diff] [review] except for the commit summary (with a=bajaj).

Oops, I mean attachment 8483917 [details] [diff] [review] for part 1 and attachment 8482555 [details] [diff] [review] for part 2.

Trying ...

Comment 61

[Vicamo Yang [:vicamo][:vyang]]

Flashing to Flame v2.1 with v123 Gonk works fine except for bug 1072929.

Comment 62

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8494943 [details] [diff] [review]
aurora part 1/2: DOM and IPDL changes

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Unknown. See bug 1038176.

[User impact] if declined: According to bug 1038176 comment 112, this patch brings 300ms launch time improvement for sms app.

[Testing completed]:
1) flash gecko & gaia only to Flame with Gonk v123. Needed work-around from attachment 8495242 [details] [diff] [review] or `make -C gaia reset-gaia` for bug 1072929.
2) flash whole system to Flame with Gonk v123

[Risk to taking this patch] (and alternatives if risky):
Interface changes, binary incompatible with existing vendor RIL. No alternatives so far because such mechanism change is what this bug is meant for.

[String changes made]:
8fd0dba2-032e-4190-a751-07cc3782e93e (nsIMobileMessageCursorCallback) => 134a6958-543b-46e2-b419-4631a2314164

Comment 63

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8494953 [details] [diff] [review]
aurora part 2/2: Gonk mmdb read-ahead

[Approval Request Comment]
See comment 62.

Comment 64

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8494943 [details] [diff] [review]
aurora part 1/2: DOM and IPDL changes

This is not the approval flag you're looking for :)

Comment 65

[bhavana bajaj [:bajaj]]

Comment on attachment 8494943 [details] [diff] [review]
aurora part 1/2: DOM and IPDL changes

Given the improvement and keeping in mind we've already uplifted this to 2.0, approving this for aurora to help in 2.1

Comment 66

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9a17dc3fa1c8
https://hg.mozilla.org/releases/mozilla-aurora/rev/38ad67d1f185

Comment 67

[Ryan VanderMeulen [:RyanVM]]

Should have been the following 2 commits, sorry:

https://hg.mozilla.org/releases/mozilla-aurora/rev/465cb43ed447
https://hg.mozilla.org/releases/mozilla-aurora/rev/8d66c0771ce6