1029977 - Do not use data-uris for collection backgrounds

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Everything.me, defect, P1)

Description

[James Lal [:lightsofapollo]]

While debugging something else I noticed we are using data uris for the collections background image.... This has a number of performance problems and we should use blobs of images OR a straight source of an image if that is available.

This can have a significant memory impact.

Comment 1

[Jason Smith [:jsmith]]

Need to understand the depth of the memory impact here to make a blocking call.

Comment 2

[Jason Smith [:jsmith]]

James - Can you evaluate the memory impact here (i.e. how much memory is being used here)?

Comment 3

[James Lal [:lightsofapollo]]

Attachment: memory-reports

Memory report attached... It is hard to tell what the difference would be using a bloc or src but we currently use at least 1.8mb for the datauri.

Comment 4

[Jason Smith [:jsmith]]

Kyle - This might be one of the reasons why bug 1029902 is present. What's your take on a blocking decision on this bug from a MemShrink perspective? Do you need this fixed to help fix bug 1029902?

Comment 5

[James Lal [:lightsofapollo]]

This does not directly effect the memory of the homescreen this is allocated in the collections app (which unless I am mistaken) is not in the same process as the homescreen.

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

The 1.8 MB is the image created from the data URI, not the URI string itself.  We should fix this, but it's probably a relatively minor (e.g. < 10%) part of bug 1029902.

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Also this memory is in the "Smart Collections" process and I have no idea what that is.  Is there a design doc somewhere I can read?

Comment 8

[Kevin Grandon :kgrandon]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #7)
> Also this memory is in the "Smart Collections" process and I have no idea
> what that is.  Is there a design doc somewhere I can read?

No time for documentation this time around unfortunately, but basically we've split the homescreen and everything.me code up into different apps. The homescreen ships with a few "smart collections", and when you tap on one it will open an activity to view a smart collection. It's highly unlikely that this impacts bug 1029902, unless you were measuring with a smart collection open - so unblocking for now.

Also I just double checked and this is *exactly* what smart collections do in 1.4, so there shouldn't be much of a regression here.

Comment 9

[Kevin Grandon :kgrandon]

We should fix this, but I don't think it should block 2.0 as this is the exact same behavior as what we do in 1.4. Noming for 2.0? and suggesting that we don't block.

Comment 10

[Jason Smith [:jsmith]]

Okay, I'll kick this out of the blocking queue if it's long-standing.

Comment 11

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Kevin Grandon :kgrandon from comment #8)
> (In reply to Kyle Huey [:khuey] ([e-mail]) from comment #7)
> > Also this memory is in the "Smart Collections" process and I have no idea
> > what that is.  Is there a design doc somewhere I can read?
> 
> No time for documentation this time around unfortunately, but basically
> we've split the homescreen and everything.me code up into different apps.
> The homescreen ships with a few "smart collections", and when you tap on one
> it will open an activity to view a smart collection. It's highly unlikely
> that this impacts bug 1029902, unless you were measuring with a smart
> collection open - so unblocking for now.
> 
> Also I just double checked and this is *exactly* what smart collections do
> in 1.4, so there shouldn't be much of a regression here.
 
Sadly I did a measurement while the homescreen is starting up. At that point it seems that the base64 images are loaded both into the app window, but also into the the chrome scope.

It results into having the exact same amount of memory at the end, but it is two times biggers when the homescreen is starting up.
My current theory is that the base64 images are loaded once into the homescreen window, and once into the chrome scope by the datastore.
It means that when the homescreen is killed, it will consume 2 times more memory if the user has some collections on the homescreen...

Asking 2.0? again.

Comment 12

[Amir Nissim (:amirn)]

what is the "gaia way" for converting base64 image to blob?

Comment 13

[Amir Nissim (:amirn)]

Attachment: Pull Request

James, can you please run the memory profile again?

Comment 14

[Amir Nissim (:amirn)]

Comment on attachment 8456938 [details] [review]
Pull Request

f? on Vivien for memory profiling as well. thanks!

Comment 15

[Kevin Grandon :kgrandon]

Comment on attachment 8456938 [details] [review]
Pull Request

I'm guessing this should be much better. We do still need to process the base64 encoded image to generate the blob, but I guess there's no real way around it - though it should be relatively rare that we do it since we have the CDN images now I think?

Anyway, the code seems fine so R+. Would prefer to have Vivien or someone measure the difference in memory usage before landing though.

Comment 16

[Kevin Grandon :kgrandon]

Let's land and people can verify/measure that this is better. Thanks!

Master: https://github.com/mozilla-b2g/gaia/commit/a470c523d1f0d50c039f6a7e0326a06e2a4e7db3

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing for v2.0 uplift.

Comment 18

[Amir Nissim (:amirn)]

looks like we have conflicts because bug 1016221 is not in v2.0.
updated dependency.

Comment 19

[Amir Nissim (:amirn)]

Comment on attachment 8456938 [details] [review]
Pull Request

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Using base64 image src
[User impact] if declined: Memory usage is high
[Testing completed]: Yes
[Risk to taking this patch] (and alternatives if risky): Low
[String changes made]: switched to using blobs instead

Comment 20

[Kevin Grandon :kgrandon]

2.0: https://github.com/mozilla-b2g/gaia/commit/746701af0959a4a2c81be9ad4d09124fa6e3d2a6

Comment 21

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8456938 [details] [review]
Pull Request

I guess this does not need my f? anymore.

Comment 22

[James Lal [:lightsofapollo]]

clear ni