Closed Bug 889899 Opened 7 years ago Closed 7 years ago

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

Categories

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

x86
macOS
defect

Tracking

(blocking-b2g:leo+, b2g18 fixed, b2g-v1.1hd fixed)

RESOLVED FIXED
1.1 QE4 (15jul)
blocking-b2g leo+
Tracking Status
b2g18 --- fixed
b2g-v1.1hd --- fixed

People

(Reporter: mbudzynski, Assigned: kaze)

References

Details

(Keywords: perf, Whiteboard: u=commsapps-user c=messaging p=2, [LeoVB+] )

Attachments

(2 files, 1 obsolete file)

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.
Keywords: perf
blocks leo+ bug 882094 -> leo+

plus, the profile shows that this only low/medium-risk fix can result in a big improvement in perf.
blocking-b2g: --- → leo?
Assignee: nobody → felash
Whiteboard: u=commsapps-user c=messaging p=2
blocking-b2g: leo? → leo+
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 :)
Assignee: felash → kaze
Attached file link to pull request (obsolete) —
https://github.com/mozilla-b2g/gaia/pull/10843

Same patch as for bug 882094.
Attachment #773465 - Flags: review?(felash)
Comment on attachment 773465 [details]
link to pull request

comments on the github PR

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

thanks !
Attachment #773465 - Flags: review?(felash)
Blocks: 889902
Attachment #773465 - Flags: review?(felash)
Comment on attachment 773465 [details]
link to pull request

r=me with nits
Attachment #773465 - Flags: review?(felash) → review+
Duplicate of this bug: 882094
blocking-b2g: leo+ → leo?
Whiteboard: u=commsapps-user c=messaging p=2 → u=commsapps-user c=messaging p=2,[leo-triage]
Priority: -- → P1
Target Milestone: --- → 1.1 QE5
Attached file 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.
Attachment #773465 - Attachment is obsolete: true
Attachment #774622 - Flags: review?(felash)
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
Attachment #774622 - Flags: review?(felash) → review+
Merged on master:
https://github.com/mozilla-b2g/gaia/commit/bd5dc592b4a6c3f46f1cbd2aca5d820414074320
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: 1.1 QE5 → 1.1 QE4 (15jul)
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.
Flags: needinfo?(gsvelto)
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.
Flags: needinfo?(gsvelto)
Obvious perf win here, blocking.
blocking-b2g: leo? → leo+
Whiteboard: u=commsapps-user c=messaging p=2,[leo-triage] → u=commsapps-user c=messaging p=2
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.
Whiteboard: u=commsapps-user c=messaging p=2 → u=commsapps-user c=messaging p=2, [LeoVB+]
v1.1.0hd: 919e9af2ec6c039c9631203d0f2c22165a85c533
You need to log in before you can comment on or make changes to this bug.