888715 - [SMS] [perf] Messages app is CPU hungry with huge SMS database

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::SMS, defect, P1)

Description

[:gerard-majax]

Attachment: Profiling of the Messages app.

With quite big SMS database (10000+ messages, most of then in one thread), the SMS app is eating a LOT of CPU.

Profiling attached.

Comment 1

[Julien Wajsberg [:julienw]]

Yep we saw this with Gabriele, the main problem is with CSS, and one of the possible solutions is bug 887198 along with bug 865743.

another solution is find which css selectors suck up the CPU time.

Comment 2

[:gerard-majax]

(In reply to Julien Wajsberg [:julienw] from comment #1)
> Yep we saw this with Gabriele, the main problem is with CSS, and one of the
> possible solutions is bug 887198 along with bug 865743.
> 
> another solution is find which css selectors suck up the CPU time.

Maybe I'm gonna say something stupid, but I only see one query selector call in this area:  var messageDOM = this.container.querySelector(
  1104       '[data-message-id="' + message.id + '"]');

Comment 3

[:gerard-majax]

FYI,

E/GeckoConsole( 2334): Content JS WARN at app://sms.gaiamobile.org/js/thread_ui.js:1103 in thui_appendMessage: appendMessage: querySelector('[data-message-id="9959"]')

and going on forever as soon as I open a thread. So with a lot of messages inside one thread, cursor keeps getting to this callback.

Comment 4

[Julien Wajsberg [:julienw]]

I was more thinking about the internal CSS parser, but what you found is actually a very good lead !

Could be useful to try to replace this line with a getElementById instead, as we _also_ have a correct id which is "message-<id>" (see https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/js/thread_ui.js#L1078).

Comment 5

[Julien Wajsberg [:julienw]]

If a so simple change can fix this problem, I think this is worth being leo+.

Comment 6

[:gerard-majax]

(In reply to Julien Wajsberg [:julienw] from comment #4)
> I was more thinking about the internal CSS parser, but what you found is
> actually a very good lead !
> 
> Could be useful to try to replace this line with a getElementById instead,
> as we _also_ have a correct id which is "message-<id>" (see
> https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/js/thread_ui.
> js#L1078).

I'll test it right now.

Comment 7

[:gerard-majax]

Attachment: Profiling of the Messages app after getElementById().

Please find attached the profiling of Messages app after switching to getElementById(). This seems to have already a nice impact.

Comment 8

[:gerard-majax]

Limiting the amount of messages to retrieve at first, with the following change, gives of course another increment in perfs. I suspect this is the behavior expected out of bug 887198 (but with the ability to scroll back the full history, which the following change removes):

diff --git a/apps/sms/js/thread_ui.js b/apps/sms/js/thread_ui.js
index d90a2ca..50068ed 100644
--- a/apps/sms/js/thread_ui.js
+++ b/apps/sms/js/thread_ui.js
@@ -989,6 +989,7 @@ var ThreadUI = global.ThreadUI = {
         self.appendMessage(message,/*hidden*/ true);
         self.messageIndex++;
         if (self.messageIndex === self.CHUNK_SIZE) {
+          self.stopRendering();
           self.showFirstChunk();
         }
         return true;

Comment 9

[Julien Wajsberg [:julienw]]

Yep, that's it :)

Gabriele, could you try to profile with and without this change and see if this lowers the "Styles" functions usage ?

Alexandre, would you please add unit tests around this, to check that trying to append twice the same message won't append it twice.

Comment 10

[Julien Wajsberg [:julienw]]

Alex, could you please add a patch or a PR so that Gabriele can profile ? thanks !

Comment 11

[:gerard-majax]

(In reply to Julien Wajsberg [:julienw] from comment #9)
> Yep, that's it :)
> 
> Gabriele, could you try to profile with and without this change and see if
> this lowers the "Styles" functions usage ?
> 
> Alexandre, would you please add unit tests around this, to check that trying
> to append twice the same message won't append it twice.

Sure, I'll try to do it today on spare time. As far as I've been using this since yesterday, no issue.

Comment 12

[Gabriele Svelto [:gsvelto]]

I'll run a profile as soon as I'll have all the pieces in place, what do you want me to profile exactly? Opening the thread? Scrolling once the thread is open?

When we looked at the profiles last week we identified multiple issues (cost of applying CSS rules, thumbnail previews, actual retrieval of the messages, etc...) it might be worth doing some further analysis in this bug and then opening dependent bugs for the various aspects we might want to address.

Comment 13

[Julien Wajsberg [:julienw]]

Gabriele, investigation is probably more to be done in Bug 888135.

I think that what's changed here is the appendThread call, therefore merely opening the thread and waiting for it to settle (using eg top) is enough.

Alexandre, if you're not supposed to work this week I can take over this, just tell me.

Comment 14

[Julien Wajsberg [:julienw]]

Alexandre is not supposed to work so I'm taking it for the week then.

Comment 15

[Julien Wajsberg [:julienw]]

Attachment: patch v1

add a patch so that gabriele can start profiling

Comment 16

[Julien Wajsberg [:julienw]]

I'd like to see how the "show thread" action behaves in the x-heavy workload, for the only SMS thread. Wait that the thread list stops loading (the scrollbar would hide) and then open that thread to get the profile for this.

Please profile with and without that patch :)

thanks !

Comment 17

[Gabriele Svelto [:gsvelto]]

I've got a couple of profiles finally, this is before applying the patch:

http://people.mozilla.com/~bgirard/cleopatra/#report=e739aa83599ec8b0af20c4107579631031aa81b1

And this one is after:

http://people.mozilla.com/~bgirard/cleopatra/#report=14d91328e6945ebd0e28e18e25493b1909ab4076

In both cases I've opened the large SMS thread from the x-heavy workload after having deleted all other threads (in order to minimize their influence on the profile).

A crude evaluation of the total time spent opening the thread puts it at ~17s w/o the patch and 13.5s with it. The difference seems to come almost entirely from thui_appendMessage() which is made three times faster by the patch. This is consistent with what the patch is doing, previously we were spending an enormous amount of time in Gecko code used for matching elements (essentially the implementation of document.querySelector(selectors)).

Comment 18

[Julien Wajsberg [:julienw]]

That means that this single change cut 25% of the time on the x-heavy workload. The gain are probably not that impressive on smaller workloads, but I'd like a leo+ on such a non-risky small big-value patch.

Comment 19

[:gerard-majax]

Looking into it to add unit tests.

Comment 20

[Julien Wajsberg [:julienw]]

Comment on attachment 769774 [details] [diff] [review]
patch v1

seems like there is already a test for the double-insert case, so requesting review.

pull request is https://github.com/mozilla-b2g/gaia/pull/10830

Comment 21

[Borja Salguero [:borjasalguero]]

Comment on attachment 769774 [details] [diff] [review]
patch v1

I've been testing this and we have a better performance, so for me it's R+.

Comment 22

[Lukas Blakk [:lsblakk] use ?needinfo]

This will affect power users and likely come back to us via OEMs so blocking for the value win here.

Comment 23

[Julien Wajsberg [:julienw]]

master: b6ea8c37ff3659a56e27c6b2b68c925a118b1b51

Comment 24

[John Ford [:jhford] CET/CEST Berlin Time]

Uplifted b6ea8c37ff3659a56e27c6b2b68c925a118b1b51 to:
v1-train: 6f1bf145b7b7075cc8f104b150dde78e06c16d01

Comment 25

[John Ford [:jhford] CET/CEST Berlin Time]

v1.1.0hd: 6f1bf145b7b7075cc8f104b150dde78e06c16d01