889899 - [SMS] change attachment's iframe to div+img

Status: RESOLVED FIXED

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

Description

[Michał Budzyński (:michalbe)]

Iframes are slower and more expensive than just div with image inside. We should however leave iframes while composing new sms, because of the reason in the comment form the file: 

> The render method creates an iframe to represent the attachment in the message 
> composition area. An iframe is used because Gecko will still try to put the 
> cursor into elements with [contentEditable=false]. Instead of a bunch of 
> JavaScript to manage where the caret is and what to delete on backspace, the 
> contentEditable region treats the iframe as a simple block. Win.

Comment 1

[Julien Wajsberg [:julienw]]

blocks leo+ bug 882094 -> leo+

plus, the profile shows that this only low/medium-risk fix can result in a big improvement in perf.

Comment 2

[Fabien Cazenave [:kaze]]

Julien, please assign this bug to me if you haven’t worked on this yet. I have a WIP patch for this, attached to bug 882094.

Comment 3

[Julien Wajsberg [:julienw]]

I haven't worked on this yet but I think Michal has at least a prototype in https://github.com/michalbe/gaia/blob/mms-thumbnails/apps/sms/js/attachment.js (if you want to check this out).

I'm happy if you steal this bug from me :)

Comment 4

[Fabien Cazenave [:kaze]]

Attachment: link to pull request

https://github.com/mozilla-b2g/gaia/pull/10843

Same patch as for bug 882094.

Comment 5

[Julien Wajsberg [:julienw]]

Comment on attachment 773465 [details]
link to pull request

comments on the github PR

please ask me for review when you're ready :)

thanks !

Comment 6

[Jakub Siemiatkowski (:sasklacz)]

I guess https://bugzilla.mozilla.org/show_bug.cgi?id=889902 depends on this one.

Comment 7

[Julien Wajsberg [:julienw]]

Comment on attachment 773465 [details]
link to pull request

r=me with nits

Comment 9

[Fabien Cazenave [:kaze]]

Attachment: link to pull request

https://github.com/mozilla-b2g/gaia/pull/10943

I’m sorry I couldn’t merge the previous PR properly after addressing your latest nits. Here’s a fresh PR.

Comment 10

[Julien Wajsberg [:julienw]]

I don't know why this got nominated again. This is necessary to solve a _big_ performance issue when displaying several MMS in a thread. And this is a blocker to other leo+ bugs. And a patch is ready.

Comment 11

[Fabien Cazenave [:kaze]]

Comment on attachment 774622 [details]
link to pull request

Carrying r=julienw, merging on master:
https://bugzilla.mozilla.org/show_bug.cgi?id=889899#c9

Comment 12

[Fabien Cazenave [:kaze]]

Merged on master:
https://github.com/mozilla-b2g/gaia/commit/bd5dc592b4a6c3f46f1cbd2aca5d820414074320

Comment 13

[Julien Wajsberg [:julienw]]

Gabriele, would you please profile a version with this patch in, on the MMS-only thread in the heavy workload, to see if there are improvements over the "CSP" function usage ?

Leo triagers, I'd really would like to have this as leo+ if Gabriele's profile shows the perf improvement I'm expecting.

Comment 14

[Leo]

Leo also wants this to be Leo+ if we see improvement.

Comment 15

[Gabriele Svelto [:gsvelto]]

I'm testing the effect of the patch right now. I'll be using mozilla-b2g18 for Gecko and I'm testing the following:

- I'll measure the time it takes to open the MMS thread in the x-heavy reference workload with and w/o the patch
- I'll grab full profiles of the opening and compare them (if it takes less than 2-3 minutes due to the profiler's limits)
- I'll try to measure FPS while scrolling the thread both with and w/o the patch

Comment 16

[Gabriele Svelto [:gsvelto]]

OK, here's the measurements; I didn't do a full profile because there's no need to: the difference is night and day! We might want to do one on the changed code if we feel we want to push it even further but as it is the improvement is already tremendous.

The conditions for testing are what I described in comment 15; before applying the patch we had:

- 2m 10s to open the thread
- ~175MiB of memory used by the application once the thread is opened
- ~200+MiB memory usage peak while opening the thread
- 35-45FPS while scrolling with frequent glitches making the scrolling stutter

After the patch we now have:

- 50s to open the thread
- ~40MiB of memory used by the application once the thread is opened
- ~80MiB memory usage peak while opening the thread
- 45-55FPS while scrolling with the overall behavior feeling being much smoother than before

To stress the data above, besides being much faster this allows us to open the large MMS thread on 256MiB devices while before the app would get killed on those devices before having finished to load all messages.

I *highly* recommend to make this a leo+; big kudos to :kaze for making this happen.

Comment 17

[Lukas Blakk [:lsblakk] use ?needinfo]

Obvious perf win here, blocking.

Comment 18

[Jakub Siemiatkowski (:sasklacz)]

This patch was supposed to change the old iframe approach to iframe/div+img. But it still uses image as a background for div instead of img node. I'll need to change this for #889902.

Comment 19

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

Uplifted bd5dc592b4a6c3f46f1cbd2aca5d820414074320 to:
v1-train: 919e9af2ec6c039c9631203d0f2c22165a85c533

Comment 20

[Gabriele Svelto [:gsvelto]]

Attachment: Single large MMS thread database

This is the database I've used for the test in comment 16. It contains a single very large thread of MMSes. To use it copy the contents of this archive in the /data/local/indexedDB/chrome/ directory on the device before opening the SMS app.

Comment 21

[Leo]

Hi,
After applying this patch,Found Side a effect that recorded video is not attached to MMS composer
Please check.
Thanks,

Comment 22

[Gabriele Svelto [:gsvelto]]

(In reply to Leo from comment #21)
> After applying this patch,Found Side a effect that recorded video is not
> attached to MMS composer

Could you please open a new bug with the detailed STR of the problem you're seeing? Since this bug is closed it's better not to add too much activity on it.

Comment 23

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

v1.1.0hd: 919e9af2ec6c039c9631203d0f2c22165a85c533