944224 - Considerable amounts of images as dataURLs

Status: RESOLVED FIXED

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

Description

[Ting-Yuan Huang]

Attachment: memory-reports

The (various sizes of) icons of marketplace are encoded as dataURLs in
external-apps/marketplace.firefox.com/update.webapp
test_external_apps/marketplace.allizom.org/manifest.webapp
test_external_apps/marketplace-dev.allizom.org/update.webapp

There are more than 2MB runtime memory consumed and can't be collected. For example,
737,472 B (04.35%) -- string(length=28946, copies=12, "data:image/png;base64,iVBORw0KG...

Please find the complete memory report attached for more details.

Comment 1

[Jason Smith [:jsmith]]

We need to switch to using direct icon assets. data URIs are known to consume far more memory than direct use of image assets.

Comment 2

[Christopher Van Wiemeersch [:cvan]]

(In reply to Jason Smith [:jsmith] from comment #1)
> We need to switch to using direct icon assets. data URIs are known to
> consume far more memory than direct use of image assets.

Can we easily do that we apps in `external-apps` directory? That is, how do we link to images on the file system?

Once installed, the packaged app that gets updated links to images in the packaged app:

https://github.com/mozilla/fireplace/blob/master/yulelog/manifest.webapp#L15-L21

Comment 3

[Jason Smith [:jsmith]]

(In reply to Christopher Van Wiemeersch [:cvan] from comment #2)
> (In reply to Jason Smith [:jsmith] from comment #1)
> > We need to switch to using direct icon assets. data URIs are known to
> > consume far more memory than direct use of image assets.
> 
> Can we easily do that we apps in `external-apps` directory? That is, how do
> we link to images on the file system?
> 
> Once installed, the packaged app that gets updated links to images in the
> packaged app:
> 
> https://github.com/mozilla/fireplace/blob/master/yulelog/manifest.webapp#L15-
> L21

Yuren - Do you know? How can we reference direct image assets in the update.webapp in the external apps directory?

Comment 4

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

for packaged app, yes we can. but for hosted app, it would be better if we use dataurl for preloading app because we need show icon even user can't access internet.

Comment 5

[Kan-Ru Chen [:kanru] (UTC+9)]

Is the markeplace a preloaded hosted app? Could we insert the image into the preloaded apps db?

Comment 6

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

markplace is a packaged app so it should be okay if use icon path in update.webapp.

Comment 7

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Kan-Ru Chen [:kanru] from comment #5)
> Is the markeplace a preloaded hosted app? Could we insert the image into the
> preloaded apps db?

We still need to address the preloaded hosted app use case, maybe in another bug.

Comment 8

[Tomasz Szatkowski]

How about:
1. Load img with dataurl.
2. Copy image content.
3. And let GC sweep old image and dataurl.

Comment 9

[Tomasz Szatkowski]

Better idea:
1. Convert dataurl to TypedArray (data)
2. blob = new Blob([data])
3. url = URL.createObjectURL(blob)
4. image.src = url

Still it would probably takea only ~20% less memory.

Comment 10

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Note that these are JS strings, so base64 means storing 6 bits in a 16-bit character.  Also, the copies=12 in comment #0 is a source of concern — it looks like we could save 660 KiB just by not having the 11 extra copies.

Comment 11

[Tomasz Szatkowski]

(In reply to Jed Davis [:jld] from comment #10)
> Note that these are JS strings, so base64 means storing 6 bits in a 16-bit
> character.  Also, the copies=12 in comment #0 is a source of concern — it
> looks like we could save 660 KiB just by not having the 11 extra copies.

In first step I meant decoding base64 to bytes array (Uint8Array). Of course it doesn't resolve problem of extra copies. BTW, why JS engine doesn't share identical strings?

Comment 12

[Joe Cheng [:jcheng] (please needinfo)]

triage: 1.3+ to benefit tarako. unless after review we believe this is too risky to uplift then we can discuss further

Comment 13

[Joe Cheng [:jcheng] (please needinfo)]

hi cvan, do you mind updating the bug so the latest status of this bug can be understood? thanks

Comment 14

[Andreas Gal :gal]

cvan, whats the status here? This was assigned to you over a month ago.

Comment 15

[Andreas Gal :gal]

bwalker, can you please make this a top priority. This is a huge memory hog and I am not seeing progress in this bug despite multiple needinfos.

Comment 16

[Andreas Gal :gal]

fabrice, should we add a setter in the app code to automatically parse and turn into a blob any data urls and when you use the getter we turn int back into a data url. That would work around anyone else doing this insanity with data urls.

Comment 17

[Andreas Gal :gal]

Fabrice, I think we should also hook up the OOM notification to purge the manifest cache.

Comment 18

[Wil Clouser [:clouserw]]

(In reply to Jason Smith [:jsmith] from comment #3)
> (In reply to Christopher Van Wiemeersch [:cvan] from comment #2)
> > Can we easily do that we apps in `external-apps` directory? That is, how do
> > we link to images on the file system?
> Yuren - Do you know? How can we reference direct image assets in the
> update.webapp in the external apps directory?

(In reply to Yuren Ju [:yurenju] from comment #4)
> for packaged app, yes we can. but for hosted app, it would be better if we
> use dataurl for preloading app because we need show icon even user can't
> access internet.

Can you expand on the method of doing this?  I think you're saying it's possible but we don't know the details.  The Marketplace is a packaged app. Thanks

Comment 19

[[:fabrice] Fabrice Desré]

(In reply to Andreas Gal :gal from comment #16)
> fabrice, should we add a setter in the app code to automatically parse and
> turn into a blob any data urls and when you use the getter we turn int back
> into a data url. That would work around anyone else doing this insanity with
> data urls.

I'd like to keep them urls, so I'll try to turn them into blob uris. The tricky part is that blob uris are bound to a window.

(In reply to Andreas Gal :gal from comment #17)
> Fabrice, I think we should also hook up the OOM notification to purge the
> manifest cache.

Yes, good catch. Big wins with this one in bug 956833

Comment 20

[Andreas Gal :gal]

As long we flush the cache we might not need to fiddle with this at the gaia level too much. But the marketplace folks should definitely stop doing this since they are on every home screen.

Comment 21

[Christopher Van Wiemeersch [:cvan]]

Attachment: https://bugzilla.mozilla.org/show_bug.cgi?id=944224

Comment 22

[Christopher Van Wiemeersch [:cvan]]

Attachment: https://github.com/mozilla-b2g/gaia/pull/15052

Comment 23

[[:fabrice] Fabrice Desré]

Comment on attachment 8356333 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/15052

Thanks!

Comment 24

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

Hi Will, Christopher's pull request use the right way to access images in packaged app.

Comment 25

[Christopher Van Wiemeersch [:cvan]]

Fabrice, can you please merge this in? I don't have commit privileges.

Comment 26

[Tim Guan-tin Chien [:timdream] (please needinfo)]

https://github.com/mozilla-b2g/gaia/commit/43382bab150125ba0817f4c18e5d76d6d028e8a5

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Needinfo jhford for the 1.3 uplift.

Comment 28

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

[v1.3 482cff8] Merge pull request #15052 from cvan/mkt-icons

I can't seem to find the status flag on this bug :/

Comment 29

[[:fabrice] Fabrice Desré]

Tim, can you uplift on 1.3t? thanks!

Comment 30

[Tim Guan-tin Chien [:timdream] (please needinfo)]

This is a 1.3+ bug.

Comment 31

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Actually, I can verify this bug is in v1.3t because it was merged to v1.3 before branching.

Comment 32

[Joe Cheng [:jcheng] (please needinfo)]

 status-b2g-v1.3T:  fixed. remove tarako