Closed Bug 958433 Opened 7 years ago Closed 7 years ago

avoid using data url for default bookmark icons, wasting memory

Categories

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

x86
macOS
defect

Tracking

(Not tracked)

RESOLVED WONTFIX
1.4 S1 (14feb)

People

(Reporter: jj.evelyn, Assigned: huseby)

References

Details

(Keywords: perf, Whiteboard: [c=memory p= s= u=tarako],[MemShrink:P2])

Similar to bug 944224, if we can use png file directly instead of hardcode base64 data, that should cost less memory.
Whiteboard: [tarako]
If this isn't needed for 1.3 then please be aware that bookmarks should be moving to the homescreen in 1.4 so there might not be any point in doing this. It is worth thinking about for default homescreen bookmarks though.

I'd be surprised if these base64 strings are being held in memory all the time the browser is open. It should only be on first run when the default data is populated and then as the bookmarks list is being generated. I'd be surprised if those strings couldn't be garbage collected. The actual icons are stored and rendered as blobs and object URLs, the string is only used as a key to identify them.

If they are indeed being held in memory all the time, that may be a bug.
(In reply to Ben Francis [:benfrancis] from comment #1)
> If this isn't needed for 1.3 then please be aware that bookmarks should be
> moving to the homescreen in 1.4 so there might not be any point in doing
> this. It is worth thinking about for default homescreen bookmarks though.

It's tracked by tarako which means we are looking for a proper solution for 1.3. (also nominate 1.3?) I know there will be a big change in browser app in 1.4, so now I'm thinking simply replacing these data urls as local image files.

> I'd be surprised if these base64 strings are being held in memory all the
> time the browser is open. It should only be on first run when the default
> data is populated and then as the bookmarks list is being generated. I'd be
> surprised if those strings couldn't be garbage collected. The actual icons
> are stored and rendered as blobs and object URLs, the string is only used as
> a key to identify them.
> 
> If they are indeed being held in memory all the time, that may be a bug.

I didn't see the memory profiling result, but from tracing the code, I agree with you that these default data should be loaded on the first run. No matter GC happens or not, the memory peak may cause OOM on a device with very limited memory (128M). At this moment, we have 3 copies on storage and 2 copies on memory[1] for each icon, it's absolutely can be improved. Also, using these base64 strings as keys to identify blobs doesn't optimize for memory usage. IMO, we should abandon these base64 strings once they are converted to blobs.

[1] a base64 string in init.json, a base64 string as key and a blob as its value in indexed DB; for each DB query, both key and value occupy memory.
blocking-b2g: --- → 1.3?
(In reply to Evelyn Hung [:evelyn] from comment #2)
> (In reply to Ben Francis [:benfrancis] from comment #1)
> > If this isn't needed for 1.3 then please be aware that bookmarks should be
> > moving to the homescreen in 1.4 so there might not be any point in doing
> > this. It is worth thinking about for default homescreen bookmarks though.
> 
> It's tracked by tarako which means we are looking for a proper solution for
> 1.3. (also nominate 1.3?) I know there will be a big change in browser app
> in 1.4, so now I'm thinking simply replacing these data urls as local image
> files.

That's not completely simple, but it may be possible. The reason they're base64 encoded strings is that the icons are specified in applications-data.js in the build system which creates an init.json file in the browser at build time. What we'd need to do is to get the build system to copy the icon binary files into the browser app folder at build time, then they could be referenced by an app://browser.gaiamobile.org or http://browser.gaiamobile.org (it would need to support both) URL instead of a data URL.

It's important that the key is a real URL which points at a real icon because the browser app's cache system will try to re-download the icon from that URL once the cache has expired. If the web site itself references a favicon on the server, the browser will get a mozbrowsericonchange event and the data URL will be replaced by the real favicon URL and icon when the bookmark is first loaded.

> I didn't see the memory profiling result, but from tracing the code, I agree
> with you that these default data should be loaded on the first run. No
> matter GC happens or not, the memory peak may cause OOM on a device with
> very limited memory (128M). At this moment, we have 3 copies on storage and
> 2 copies on memory[1] for each icon, it's absolutely can be improved. Also,
> using these base64 strings as keys to identify blobs doesn't optimize for
> memory usage. IMO, we should abandon these base64 strings once they are
> converted to blobs.

The base64 encoded strings are used as keys because the key needs to be a URL which resolves to an actual icon. The icons are not very large in size, but there is room for optimisation here by using the approach I describe above. However, I don't think it would save very much memory and there's nothing to stop third party web sites specifying data URLs for their own favicons, so that could still lead to an OOM. Note that we limit icon blob sizes to 100kB with the MAX_ICON_SIZE constant, which can be tweaked.

If you'd like to work on this Evelyn it would be good to see before and after memory profiles to see whether the memory saving justifies the risk of a making this change in 1.3.
Keywords: perf
Whiteboard: [tarako] → [tarako],[MemShrink]
(In reply to Ben Francis [:benfrancis] from comment #3) 
> If you'd like to work on this Evelyn it would be good to see before and
> after memory profiles to see whether the memory saving justifies the risk of
> a making this change in 1.3.

Ben, thanks for your detailed explanation. I made a WIP patch and did memory profiles before and after applying my patch. As we expected, it's just 40 - 50k memory saving. I was told that even the benefit is small, we should try to improve it.(I still think it isn't too hard to avoid base64 in the build script, is it?) But yes, you are right, if the change is risky in 1.3 and there will be a huge change in 1.4, it may not be a good time point to do this. 

I will leave this bug open, if moving to homescreen in 1.4 won't have the same problem, feel free to close it. Thanks.
moving to 1.4? for new feature and per comment 4
blocking-b2g: 1.3? → 1.4?
Whiteboard: [tarako],[MemShrink] → [c=memory p= s= u=tarako] [tarako],[MemShrink]
Whiteboard: [c=memory p= s= u=tarako] [tarako],[MemShrink] → [c=memory p= s= u=tarako] [tarako],[MemShrink:P2]
Assignee: nobody → dhuseby
Priority: -- → P2
Status: NEW → ASSIGNED
Priority: P2 → P1
Whiteboard: [c=memory p= s= u=tarako] [tarako],[MemShrink:P2] → [c=memory p=2 s= u=tarako] [tarako],[MemShrink:P2]
triage: wont fix for tarako.
since browser has moved to system in 1.4, no point in fixing browser app in 1.4
close bug as wont fix
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
Whiteboard: [c=memory p=2 s= u=tarako] [tarako],[MemShrink:P2] → [c=memory p=2 s= u=tarako],[MemShrink:P2]
blocking-b2g: 1.4? → ---
Whiteboard: [c=memory p=2 s= u=tarako],[MemShrink:P2] → [c=memory p= s= u=tarako],[MemShrink:P2]
Target Milestone: --- → 1.4 S1 (14feb)
You need to log in before you can comment on or make changes to this bug.