If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in Firefox OS v1.1hd

Status

Firefox OS
Gaia::SMS
P1
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: michalbe, Assigned: kaze)

Tracking

({perf})

unspecified
1.1 QE4 (15jul)
x86
Mac OS X
Dependency tree / graph

Firefox Tracking Flags

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

Details

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

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
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.
(Reporter)

Updated

4 years ago
Blocks: 888135
Keywords: perf
Blocks: 887196
Blocks: 882094
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+
Blocks: 889805
(Assignee)

Comment 2

4 years ago
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
Blocks: 875338
(Assignee)

Comment 4

4 years ago
Created attachment 773465 [details]
link to pull request

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)
I guess https://bugzilla.mozilla.org/show_bug.cgi?id=889902 depends on this one.
(Reporter)

Updated

4 years ago
Blocks: 889902
(Assignee)

Updated

4 years ago
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

Updated

4 years ago
blocking-b2g: leo+ → leo?
Whiteboard: u=commsapps-user c=messaging p=2 → u=commsapps-user c=messaging p=2,[leo-triage]

Updated

4 years ago
Priority: -- → P1
Target Milestone: --- → 1.1 QE5
(Assignee)

Comment 9

4 years ago
Created attachment 774622 [details]
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.
(Assignee)

Comment 11

4 years ago
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+
(Assignee)

Comment 12

4 years ago
Merged on master:
https://github.com/mozilla-b2g/gaia/commit/bd5dc592b4a6c3f46f1cbd2aca5d820414074320
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED

Updated

4 years ago
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)

Comment 14

4 years ago
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
status-b2g18: --- → fixed
Created attachment 776370 [details]
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

4 years ago
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.

Updated

4 years ago
Whiteboard: u=commsapps-user c=messaging p=2 → u=commsapps-user c=messaging p=2, [LeoVB+]
v1.1.0hd: 919e9af2ec6c039c9631203d0f2c22165a85c533
status-b2g-v1.1hd: --- → fixed
You need to log in before you can comment on or make changes to this bug.