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.

blocks leo+ bug 882094 -> leo+ plus, the profile shows that this only low/medium-risk fix can result in a big improvement in perf.

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.

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 :)

https://github.com/mozilla-b2g/gaia/pull/10843 Same patch as for bug 882094.

Comment on attachment 773465 [details] link to pull request comments on the github PR please ask me for review when you're ready :) thanks !

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

Comment on attachment 773465 [details] link to pull request r=me with nits

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.

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 on attachment 774622 [details] link to pull request Carrying r=julienw, merging on master: https://bugzilla.mozilla.org/show_bug.cgi?id=889899#c9

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

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.

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

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

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.

Obvious perf win here, blocking.

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.

Uplifted bd5dc592b4a6c3f46f1cbd2aca5d820414074320 to: v1-train: 919e9af2ec6c039c9631203d0f2c22165a85c533

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.

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

(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.

v1.1.0hd: 919e9af2ec6c039c9631203d0f2c22165a85c533