1028253 - Camera App memory regression in FxOS v1.4

Status: RESOLVED FIXED

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

Description

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

Camera app shows 7244 KB USS and 8920 KB PSS in FFOS 1.3 

But it shows following  13068 KB USS and 14829 KB PSS in FFOS 2.0

This data is collected by running |b2g-procrank| command on msm8610 device.

This can become a bottleneck for 256MB FFOS 2.0 device.

Comment 1

[Hema Koka [:hema]]

Justin,

Can you help investigate this on flame please?

Thanks
Hema

Comment 2

[Justin D'Arcangelo [:justindarc]]

(In reply to Tapas Kumar Kundu from comment #0)
> Camera app shows 7244 KB USS and 8920 KB PSS in FFOS 1.3 
> 
> But it shows following  13068 KB USS and 14829 KB PSS in FFOS 2.0
> 
> This data is collected by running |b2g-procrank| command on msm8610 device.
> 
> This can become a bottleneck for 256MB FFOS 2.0 device.

Do you happen to have mem usage numbers for FFOS 1.4 ?

I'm going to start looking into this today to see if I can find any contributing factors that may have changed in 2.0, but I would almost think that a memory increase would be expected when comparing v1.3 and v1.4/v2.0. The Camera app starting in v1.4 is almost a completely new app due to refactoring and a ton of new features. Whereas the Camera in v1.3 is extremely basic (no zoom, no tap-to-focus, no face detection, no settings menu, etc.).

Comment 3

[Justin D'Arcangelo [:justindarc]]

I don't believe this is a v2.0 regression. If anything, it is a v1.4 regression. However, the Camera (starting with v1.4) is an entirely different app than the one that shipped in v1.3. Also, in testing using `b2g-info` (which replaced b2g-procrank), my PSS numbers in v2.0 are actually slightly *lower* than the ones in v1.4. Here's the results I got (all numbers reported are on the Flame device):

v1.3: 11.1 MB
v1.4: 17.9 MB
v2.0: 17.1 MB
v2.1: 17.5 MB

Comment 4

[Eric Rahm [:erahm]]

I'll get some memory reports for this.

Comment 5

[Justin D'Arcangelo [:justindarc]]

Here's the PSS numbers I got for Hamachi (averaging 5 runs by running `b2g-info`):

v1.3: 10.5 MB
v1.4: 16.8 MB
v2.0: 16.9 MB
v2.1: 16.8 MB

It seems like the app's memory usage is pretty consistent between v1.4 and v2.1, but with a fairly consistent ~6 MB increase between v1.3 and v1.4. This is likely due to the increase in size of the new Camera application in v1.4.

Comment 6

[Justin D'Arcangelo [:justindarc]]

Mike: See the above Comment 3 and Comment 5 -- Is this increase between v1.3 and v1.4 acceptable? The codebase for the Camera has probably increased 4x in v1.4 to accommodate all of the new features.

Comment 7

[Justin D'Arcangelo [:justindarc]]

Just added a See Also for Bug 1029856 -- Appears to be a memory leak related to HWC when running the Camera app. This may account for some of the increase in v1.4

Comment 8

[Dave Huseby [:huseby]]

A ~50% increase in the PSS does seem like something we should look at.

Comment 9

[Dave Huseby [:huseby]]

oops

Comment 10

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

(In reply to Justin D'Arcangelo [:justindarc] from comment #2)
> (In reply to Tapas Kumar Kundu from comment #0)
> > Camera app shows 7244 KB USS and 8920 KB PSS in FFOS 1.3 
> > 
> > But it shows following  13068 KB USS and 14829 KB PSS in FFOS 2.0
> > 
> > This data is collected by running |b2g-procrank| command on msm8610 device.
> > 
> > This can become a bottleneck for 256MB FFOS 2.0 device.
> 
> Do you happen to have mem usage numbers for FFOS 1.4 ?
> 

Numbers of FFOS 1.4 is matching with numbers from FFOS 2.0. Both are very close. 

#comment 8 seems to me interesting. We may gain something by digging in that direction.

Comment 11

[Justin D'Arcangelo [:justindarc]]

(In reply to Tapas Kumar Kundu from comment #10)
> Numbers of FFOS 1.4 is matching with numbers from FFOS 2.0. Both are very
> close. 

Shouldn't this bug then be re-classified as a 1.4+ blocker instead of 2.0+?

Comment 12

[Michael Vines [:m1] [:evilmachines]]

(In reply to Justin D'Arcangelo [:justindarc] from comment #11)
> 
> Shouldn't this bug then be re-classified as a 1.4+ blocker instead of 2.0+?

The fix is not required on v1.4 from our POV

Comment 13

[Jason Smith [:jsmith]]

(In reply to Justin D'Arcangelo [:justindarc] from comment #11)
> (In reply to Tapas Kumar Kundu from comment #10)
> > Numbers of FFOS 1.4 is matching with numbers from FFOS 2.0. Both are very
> > close. 
> 
> Shouldn't this bug then be re-classified as a 1.4+ blocker instead of 2.0+?

Let me ask Wayne to find out if our partner wants this or not.

Comment 14

[Wayne Chang [:wchang]]

Let's keep this on 2.0+ for now given we have 1.4+ on bug 1029856, where an actual problem is seen.

Comment 15

[Justin D'Arcangelo [:justindarc]]

(In reply to Wayne Chang [:wchang] from comment #14)
> Let's keep this on 2.0+ for now given we have 1.4+ on bug 1029856, where an
> actual problem is seen.

Looks like Bug 1029856 is resolved -- Going to re-check the numbers today to see if we still have a regression in v1.4+

Comment 16

[Justin D'Arcangelo [:justindarc]]

Bug 1029856 has landed, but it doesn't seem to have affected Camera's memory utilization. I'll dig around a little more on the JS side of things to see if there's anything that can be done to reduce memory consumption.

Comment 17

[Justin D'Arcangelo [:justindarc]]

(In reply to Eric Rahm [:erahm] from comment #4)
> I'll get some memory reports for this.

Eric: Have you made any progress on this?

Comment 18

[David Flanagan [:djf]]

Justin: does our preview stream match the screen size or are we taking a large one and scaling it down to match? You might try forcing the code to use an unrealistically small preview stream to see if that has any impact on memory use.  (Or check memory usage when viewing a photo without the preview stream running.)

There have been substantial gecko changes since 1.3 also. Mike: can you think of any gecko changes that would cause substantially increased memory usage by the camera?

It might also be interesting to know what the % increase in JS source code is between 1.3 and 2.0.

Really, though, I think we shouldn't be speculating based on b2g-ps and procrank output. We need about:memory reports for 1.3 vs 2.0 here.

Comment 19

[Justin D'Arcangelo [:justindarc]]

(In reply to David Flanagan[OOO until July 14] [:djf] from comment #18)
> Justin: does our preview stream match the screen size or are we taking a
> large one and scaling it down to match? You might try forcing the code to
> use an unrealistically small preview stream to see if that has any impact on
> memory use.  (Or check memory usage when viewing a photo without the preview
> stream running.)
> 
> There have been substantial gecko changes since 1.3 also. Mike: can you
> think of any gecko changes that would cause substantially increased memory
> usage by the camera?
> 
> It might also be interesting to know what the % increase in JS source code
> is between 1.3 and 2.0.
> 
> Really, though, I think we shouldn't be speculating based on b2g-ps and
> procrank output. We need about:memory reports for 1.3 vs 2.0 here.

I already tried forcing the smallest-possible preview stream on my Flame yesterday and there was very little change in the procrank reports. I'll do a JS source code size analysis between 1.3 and 1.4 later today. The size of the codebase has at least doubled (if not more) to accommodate all of the new features.

Comment 20

[Justin D'Arcangelo [:justindarc]]

(In reply to David Flanagan[OOO until July 14] [:djf] from comment #18)
> Really, though, I think we shouldn't be speculating based on b2g-ps and
> procrank output. We need about:memory reports for 1.3 vs 2.0 here.

Also, note that this bug is filed specifically to address the increase in the procrank data. I agree that we really do need to see the memory reports to determine if there is an unusually-large increase in utilization. I also think that it would be unreasonable to assume that we would have zero increase between 1.3 and 1.4 given that they are not even close to the same app.

Comment 21

[Mike Habicher [:mikeh] (high bugzilla latency)]

(In reply to David Flanagan[OOO until July 14] [:djf] from comment #18)
> 
> There have been substantial gecko changes since 1.3 also. Mike: can you
> think of any gecko changes that would cause substantially increased memory
> usage by the camera?

The changes and new features in Gecko since 1.3 would have an effect measured in tens-of-kilobytes, maybe 100KiB at the -very- outside. I can try to code up a minimal Camera app (viewfinder-only) that works on all of our trees to get a more solid idea.

Comment 22

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: Mini-Camera app -- only opens the viewfinder, v1

Here's a(n untested) mini-Camera app. All it does is open the viewfinder, and it should work on all versions of the CameraControl API. Need to figure out how to push it onto a real device.

Comment 23

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: Mini-Camera app -- only opens the viewfinder, v2

Update:
- create a manifest, move script to separate file, add icon
- fix getCamera() signature checking

This does nothing but open the camera and throw up the viewfinder.

Comment 24

[Mike Habicher [:mikeh] (high bugzilla latency)]

Gaia      d7a517f0bde32072f1799e4a47ea34c6d786c287
Gecko     https://hg.mozilla.org/mozilla-central/rev/b7b20af4a4fb
BuildID   20140703040209
Version   33.0a1
ro.build.version.incremental=108
ro.build.date=Tue Jun 10 19:40:40 CST 201

14:16:03 ➜  ~  adb shell b2g-procrank
APPLICATION        PID      Vss      Rss      Pss      Uss  cmdline
b2g                289  118468K   92508K   75201K   66968K  /system/b2g/b2g
Homescreen        1131   50604K   43628K   25879K   18812K  /system/b2g/plugin-container
Minimal Camera    1438   44796K   31224K   17441K   14404K  /system/b2g/plugin-container
Smart Collectio   1149   22792K   22780K    9999K    7416K  /system/b2g/plugin-container
Built-in Keyboa    988   22176K   22164K    9549K    7048K  /system/b2g/plugin-container
(Preallocated a   1678   19020K   19008K    7902K    5840K  /system/b2g/plugin-container
(Nuwa)             917   20076K   20064K    6665K    2364K  /system/b2g/plugin-container
                                           ------   ------  ------
                                          183529K  148224K  TOTAL

-----
Gaia      7ad00b0bd84d5d97e0801e3b3ceaac33fcd90e05
Gecko     https://hg.mozilla.org/releases/mozilla-aurora/rev/e87f7b398fce
BuildID   20140703000248
Version   32.0a2
ro.build.version.incremental=108
ro.build.date=Tue Jun 10 19:40:40 CST 2014

14:22:34 ➜  ~  adb shell b2g-procrank
APPLICATION        PID      Vss      Rss      Pss      Uss  cmdline
b2g                304  131492K   91224K   73359K   65200K  /system/b2g/b2g
Homescreen        1094   51040K   44004K   25790K   19280K  /system/b2g/plugin-container
Minimal Camera    1305   43540K   29968K   15941K   13248K  /system/b2g/plugin-container
Settings          1128   34556K   29584K   15281K   12520K  /system/b2g/plugin-container
Smart Collectio   1107   22740K   22728K    9736K    7572K  /system/b2g/plugin-container
Built-in Keyboa    980   21856K   21844K    9048K    6904K  /system/b2g/plugin-container
(Preallocated a   1466   18888K   18876K    7386K    5572K  /system/b2g/plugin-container
(Nuwa)             917   20484K   20472K    6551K    2212K  /system/b2g/plugin-container
                                           ------   ------  ------
                                          199039K  163000K  TOTAL

-----
Gaia      f250750c9b8381560fa33b2383737736e4dbdff5
Gecko     https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/4c05c3fb440d
BuildID   20140703000230
Version   30.0
ro.build.version.incremental=108
ro.build.date=Tue Jun 10 19:40:40 CST 2014

...apparently |navigator.mozCameras| doesn't exist in this build? (Though the camera app works just fine.) Until I get this sorted out, there is definitely an across-the-board numbers increase in memory usage even for the Minimal Camera app between v2.1 (m-c) and v2.0 (aurora) -- though there are no significant changes in the Gecko camera code between these two versions.

Comment 25

[Eric Rahm [:erahm]]

Attachment: memory report 1.4

I'm unable to get a report for 1.3 currently (there doesn't appear to be flame support), I'm attaching a memory report for 1.4. There's nothing really outstanding to me in it, it would probably be better with a diff.

If someone has has access to a 1.3 device you can get a memory report with the following command:
  MOZ_IGNORE_NUWA_PROCESS=1 tools/get_about_memory.py --minimize

And if it's not a flame we'll probably want a 1.4 report from that device as well.

Comment 26

[Justin D'Arcangelo [:justindarc]]

Attachment: Mini-Camera app -- only opens the viewfinder, v3

Mike: I've wrapped your mini camera app up into an app template generated by the shiny new WebIDE :-D

You can add this and deploy it to the device using WebIDE in Nightly. Also, there was an issue with the API version detection in your code that is now fixed.

Comment 27

[Justin D'Arcangelo [:justindarc]]

Here are the procrank (b2g-info) numbers for the Mike's mini camera app (same app, only opens viewfinder on v1.3 through v2.1):

             NAME  PID PPID CPU(s) NICE  USS  PSS  RSS VSIZE OOM_ADJ USER   
1.3   Camera Test 2197 1482    0.8    1  5.5  8.9 20.0  75.0       2 u0_a2197
1.4   Camera Test 1510  897    0.9    1  6.1 10.2 20.2  76.0       2 u0_a1510
2.0   Camera Test 1922  908    1.0    1  6.1  8.4 21.6  78.4       2 u0_a1922
2.1   Camera Test 2169 1691    1.1    1  6.4  9.0 22.1  80.7       2 u0_a2169

As you can see, not much of a change between v1.3 and v1.4 (slight increase). I still need to try and get a v1.3 memory report from either my Flame or perhaps I'll just try comparing v1.3 and v1.4 on a Hamachi. Once I can compare the memory reports with v1.3, we might be able to get to the bottom of the cause of the increase. Because there isn't much of a difference in terms of memory usage in the mini camera app (viewfinder only), I'm going to assume that our increase can be attributed to the UI refresh (introduced in v1.4) as well as the entire-app overhaul (all the new features were introduced in v1.4 as well which also increased the size of the codebase significantly).

Comment 28

[Justin D'Arcangelo [:justindarc]]

Attachment: memory report 1.3

I generated this memory report using the v1.3 build that is included with the v122 base build.

Comment 29

[Eric Rahm [:erahm]]

Attachment: Camera 1.3 -> 1.4 memory report diff

This diff show's a lot of small increases. Of note (to me at least):

│  ├──1.51 MB (36.48%) -- active/window(app://camera.gaiamobile.org/index.html)
│  │  ├──1.25 MB (30.18%) -- js-compartment(app://camera.gaiamobile.org/index.html)

├──0.82 MB (19.86%) -- js-non-window
│  │  ├──0.18 MB (04.34%) -- strings
│  ├──0.50 MB (11.99%) -- runtime
│  │  ├──0.23 MB (05.60%) ── script-data
│  │  ├──0.11 MB (02.59%) -- script-sources

├──0.46 MB (11.15%) -- media
│  ├──0.25 MB (06.09%) ── libogg [+]
│  ├──0.11 MB (02.69%) ── decoded/audio
│  └──0.10 MB (02.37%) ── resources [+]

Comment 30

[Justin D'Arcangelo [:justindarc]]

Attachment: Memory Report DIFF (v1.3 - v1.4)

Here is a diff of the about:memory reports for the Camera app between v1.3 and v1.4

Comment 31

[Justin D'Arcangelo [:justindarc]]

(In reply to Eric Rahm [:erahm] from comment #29)
> Created attachment 8453228 [details]
> Camera 1.3 -> 1.4 memory report diff
> 
> This diff show's a lot of small increases. Of note (to me at least):
> 
> │  ├──1.51 MB (36.48%) --
> active/window(app://camera.gaiamobile.org/index.html)
> │  │  ├──1.25 MB (30.18%) --
> js-compartment(app://camera.gaiamobile.org/index.html)
> 
> ├──0.82 MB (19.86%) -- js-non-window
> │  │  ├──0.18 MB (04.34%) -- strings
> │  ├──0.50 MB (11.99%) -- runtime
> │  │  ├──0.23 MB (05.60%) ── script-data
> │  │  ├──0.11 MB (02.59%) -- script-sources
> 
> ├──0.46 MB (11.15%) -- media
> │  ├──0.25 MB (06.09%) ── libogg [+]
> │  ├──0.11 MB (02.69%) ── decoded/audio
> │  └──0.10 MB (02.37%) ── resources [+]

Eric: Is it safe to assume that this is due to the large number of new features and UI refresh? (the v1.3 and v1.4 Camera apps are not even close to the same thing).

Comment 32

[Eric Rahm [:erahm]]

(In reply to Justin D'Arcangelo [:justindarc] from comment #31)
> 
> Eric: Is it safe to assume that this is due to the large number of new
> features and UI refresh? (the v1.3 and v1.4 Camera apps are not even close
> to the same thing).

I think overall that makes sense. The main increases we're seeing are all JS related. It's possible we can reduce the overall script size which could be a win, and it's possible we're keeping JS-related things around longer than we need to. We can probably do some things to cleanup the media usage (we might have some media elements hanging around that we don't need).

Someone from the camera team should probably chime in on this.

Comment 33

[Justin D'Arcangelo [:justindarc]]

(In reply to Eric Rahm [:erahm] from comment #32)
> (In reply to Justin D'Arcangelo [:justindarc] from comment #31)
> > 
> > Eric: Is it safe to assume that this is due to the large number of new
> > features and UI refresh? (the v1.3 and v1.4 Camera apps are not even close
> > to the same thing).
> 
> I think overall that makes sense. The main increases we're seeing are all JS
> related. It's possible we can reduce the overall script size which could be
> a win, and it's possible we're keeping JS-related things around longer than
> we need to. We can probably do some things to cleanup the media usage (we
> might have some media elements hanging around that we don't need).
> 
> Someone from the camera team should probably chime in on this.

Well, that would be myself, Diego or Wilson. We are usually discarding things (media elements, etc.) that aren't in use.. but I can try a couple things out to see if it helps any. For the most part though, we throw away views when they're done being used and we lazy-load as much as possible. I'm not sure a whole lot can be done to reduce the increase.

Comment 34

[bhavana bajaj [:bajaj]]

(In reply to Eric Rahm [:erahm] from comment #29)
> Created attachment 8453228 [details]
> Camera 1.3 -> 1.4 memory report diff
> 
> This diff show's a lot of small increases. Of note (to me at least):
> 
> │  ├──1.51 MB (36.48%) --
> active/window(app://camera.gaiamobile.org/index.html)
> │  │  ├──1.25 MB (30.18%) --
> js-compartment(app://camera.gaiamobile.org/index.html)
> 
> ├──0.82 MB (19.86%) -- js-non-window
> │  │  ├──0.18 MB (04.34%) -- strings
> │  ├──0.50 MB (11.99%) -- runtime
> │  │  ├──0.23 MB (05.60%) ── script-data
> │  │  ├──0.11 MB (02.59%) -- script-sources
> 
> ├──0.46 MB (11.15%) -- media
> │  ├──0.25 MB (06.09%) ── libogg [+]
> │  ├──0.11 MB (02.69%) ── decoded/audio
> │  └──0.10 MB (02.37%) ── resources [+]

Can we generate a similar diff for 1.3->2.0 or 1.4->2.0 to see if we can analyze this better which was the original idea behind filling this bug from CAF ?

Comment 35

[Justin D'Arcangelo [:justindarc]]

(In reply to bhavana bajaj [:bajaj] [NOT reading Bugmail, needInfo please] from comment #34)
> (In reply to Eric Rahm [:erahm] from comment #29)
> > Created attachment 8453228 [details]
> > Camera 1.3 -> 1.4 memory report diff
> > 
> > This diff show's a lot of small increases. Of note (to me at least):
> > 
> > │  ├──1.51 MB (36.48%) --
> > active/window(app://camera.gaiamobile.org/index.html)
> > │  │  ├──1.25 MB (30.18%) --
> > js-compartment(app://camera.gaiamobile.org/index.html)
> > 
> > ├──0.82 MB (19.86%) -- js-non-window
> > │  │  ├──0.18 MB (04.34%) -- strings
> > │  ├──0.50 MB (11.99%) -- runtime
> > │  │  ├──0.23 MB (05.60%) ── script-data
> > │  │  ├──0.11 MB (02.59%) -- script-sources
> > 
> > ├──0.46 MB (11.15%) -- media
> > │  ├──0.25 MB (06.09%) ── libogg [+]
> > │  ├──0.11 MB (02.69%) ── decoded/audio
> > │  └──0.10 MB (02.37%) ── resources [+]
> 
> Can we generate a similar diff for 1.3->2.0 or 1.4->2.0 to see if we can
> analyze this better which was the original idea behind filling this bug from
> CAF ?

Comment 0 reports an increase in the b2g-procrank numbers between v1.3 and v2.0. However, the procrank numbers for v1.4, v2.0 and v2.1 are *very* stable. The reported increase occurs between v1.3 and v1.4. I'm not sure how comparing with v2.0 is going to reveal anything different than what the v1.4 about:memory diff is already showing. Especially since v1.4 is when the UI refresh and majority of new features landed, not v2.0.

Comment 36

[Nicholas Nethercote [inactive]]

Increases in "script-data" and "script-sources" are almost certainly due to having more JS code. And it's likely that the "strings" increase has the same cause, though I can't say definitively.

Comment 37

[Justin D'Arcangelo [:justindarc]]

Attachment: pull-request (v1.4)

This patch gives me a memory savings of around 0.5 MB in v1.4. It converts all of our sounds to .opus format (which is supposed to be slightly more efficient than .ogg). It also discards any HTML template strings that are no longer used in views after initialization (there was an increase in the amount of memory used for strings in v1.4).

Comment 38

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

How is script-sources for app://camera.gaiamobile.org/js/main.js 180 KB when http://mxr.mozilla.org/gaia/source/apps/camera/js/main.js is less than 1 KB?

The amount of memory we're spending on function objects seems kind of high too.

18% heap-unclassified.

Beyond that I don't think there's much actionable.

Comment 39

[Andrew McCreight [:mccr8]]

Maybe the "require" stuff at the top is pulling in more source?  I have no idea how that works.

Comment 40

[Justin D'Arcangelo [:justindarc]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #38)
> How is script-sources for app://camera.gaiamobile.org/js/main.js 180 KB when
> http://mxr.mozilla.org/gaia/source/apps/camera/js/main.js is less than 1 KB?
> 

The Makefile concatenates all critical startup path sources into a single large main.js. Everything else gets lazy-loaded with RequireJS after the critical startup path finishes (once the viewfinder is live and the basic camera controls are accessible).

> The amount of memory we're spending on function objects seems kind of high
> too.
> 
> 18% heap-unclassified.
> 

Yes, this can definitely be attributed to new features. Everything is broken out into maintainable components; so, yes, there are a lot of functions.

> Beyond that I don't think there's much actionable.

I agree. There are no obvious, alarming leaks as far as I can tell. We can also be fairly certain that the Gecko-side Camera API changes have not introduced any significant memory regressions from running Mike's mini-camera app (viewfinder only, no controls) across v1.3 - v2.1 (see Comment 27).

Not to underplay the importance of the additional memory overhead, but... the original Camera app in v1.3 (and earlier) was *extremely* barebones (only take photo, record video and display thumbnail strip). We now have an all new UI design, new preview gallery, real-time pinch-to-zoom capability, HDR, self-timer, settings menu, and a lot more other small odds and ends. The total increase is around 4 MB. Given that the previous app was only using around 8 MB, it appears like a 50% increase. The "50% increase" sounds alarming, but given that the previous app was extremely limited, it also had a *very* low memory requirement. Because of that, I think that this "regression" seems more drastic than it probably is when you look at it from a percentages point-of-view.

The bottom line is: if we absolutely need to ship a "low-memory" version, then we will need to eliminate features from the build. Likely candidates for features to strip out would be: pinch-to-zoom and the settings menu (which includes HDR, self-timer and grid lines toggles). However, if we can live with a ~3.5 MB increase, then we can land my patch that removes some of the strings overhead and optimizes our sound assets.

Comment 41

[Diego Marcos [:dmarcos]]

Comment on attachment 8453974 [details] [review]
pull-request (v1.4)

Just made a couple of suggestions to move some function calls to a different place. Almost ready.

Comment 42

[Nicholas Nethercote [inactive]]

I think comment 40 addresses the needinfo from comment 38. Thanks, Justin.

Comment 43

[Justin D'Arcangelo [:justindarc]]

Comment on attachment 8453974 [details] [review]
pull-request (v1.4)

Diego: Please re-review when you get a chance. Addressed your PR comments. Thanks!

Comment 44

[Eric Rahm [:erahm]]

Attachment: DMD, GC, and CC logs

There's not a ton of big ticket items in the DMD report. I did notice a fair amount of font-related entries in the top 25 accounting for ~172KB.

Comment 45

[Justin D'Arcangelo [:justindarc]]

(In reply to Eric Rahm [:erahm] from comment #44)
> Created attachment 8454586 [details]
> DMD, GC, and CC logs
> 
> There's not a ton of big ticket items in the DMD report. I did notice a fair
> amount of font-related entries in the top 25 accounting for ~172KB.

We do use an icon-font for all the icons in the app. We currently inline the icon font as a base-64 encoded string in the CSS. Are there any known memory-related pros/cons to doing that versus loading via separate request?

Comment 46

[Justin D'Arcangelo [:justindarc]]

Landed on v1.4:

https://github.com/mozilla-b2g/gaia/commit/b7d36622c7df92c976c37520ccab25199c7ada91

Eric/Hema: Who makes the ultimate decision as to whether or not to call this a blocking regression? This patch should bring a little bit of memory savings to Camera, but it would be impossible to not have any increase going from v1.3 to v1.4. I will leave this bug open until I hear your input on this.

Comment 47

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

(In reply to Justin D'Arcangelo [:justindarc] from comment #46)
> Landed on v1.4:
> 
> https://github.com/mozilla-b2g/gaia/commit/
> b7d36622c7df92c976c37520ccab25199c7ada91
> 
> Eric/Hema: Who makes the ultimate decision as to whether or not to call this
> a blocking regression? This patch should bring a little bit of memory
> savings to Camera, 

Could you please give us a PR for ffos 2.0 ? 


> but it would be impossible to not have any increase going
> from v1.3 to v1.4. I will leave this bug open until I hear your input on
> this.

Could you please send us a test patch which will disable certain features in current gaia camera app in FFOS 2.0 to remove this memory regression fully ? 

Based on your test patch, we can decide whether to keep those features in v2.0 or not. 
Thanks a lot for your help.

Comment 48

[Hema Koka [:hema]]

(In reply to Justin D'Arcangelo [:justindarc] from comment #46)
> Landed on v1.4:
> 
> https://github.com/mozilla-b2g/gaia/commit/
> b7d36622c7df92c976c37520ccab25199c7ada91
> 
> Eric/Hema: Who makes the ultimate decision as to whether or not to call this
> a blocking regression? This patch should bring a little bit of memory
> savings to Camera, but it would be impossible to not have any increase going
> from v1.3 to v1.4. I will leave this bug open until I hear your input on
> this.

Justin, 

We have a couple of memory regression bugs opened tracking zoom, HDR etc. on flame (configured at 273MB) and QRD (at 256MB). Diego has put in a patch here: https://bugzilla.mozilla.org/show_bug.cgi?id=1024692 to restrict to 3MP images and Mike is working on disabling HDR for devices less than 512MB mem (https://bugzilla.mozilla.org/show_bug.cgi?id=1036637). 

The new camera app since 1.4 has several new features that attribute to the increase. We will probably need to disable mem intense camera features such as pinch-to-zoom, HDR on low-mem devices. Can you help run the memory report with patches from 1024692 and 1036637 and your patch on 2.0 Flame (273MB)? The results will help decide. 

Thanks
Hema

Comment 49

[Justin D'Arcangelo [:justindarc]]

Updating title/tracking flags to be clearer about which versions are affected and when the memory utilization increased (it didn't start in v2.0).

Going to migrate this v1.4 patch to v2.0 and re-run the memory reports incorporating the patches from Bug 1024692 and Bug 1036637 to see where we're at now.

Comment 50

[Eric Rahm [:erahm]]

From purely a memory usage (and I guess usability) perspective it seems like it will be worth the effort to add the ability to pref off certain high memory camera features. Perhaps it would make sense to track that in another bug?

Comment 51

[Eric Rahm [:erahm]]

(In reply to Justin D'Arcangelo [:justindarc] from comment #45)
> (In reply to Eric Rahm [:erahm] from comment #44)
> > Created attachment 8454586 [details]
> > DMD, GC, and CC logs
> > 
> > There's not a ton of big ticket items in the DMD report. I did notice a fair
> > amount of font-related entries in the top 25 accounting for ~172KB.
> 
> We do use an icon-font for all the icons in the app. We currently inline the
> icon font as a base-64 encoded string in the CSS. Are there any known
> memory-related pros/cons to doing that versus loading via separate request?

A bit of a tangent:
It looks like bug 1008458 was going to allow us to load the icon font from the system, which seems like it would have been a good thing memory-wise but that got nixed. In bug 1030829 we added the ability to at least look up cached fonts by data URIs, so in theory we're avoiding the loading overhead (as long as the URIs match of course). In general I think that's a loss for us memory-wise as we're now keeping the base64-encoded font in memory (twice I'm guessing, once in the font cache and once in the CSS). Also that just landed in 33 so, we're not going to see the performance win in 1.4/2.0 unless it gets uplifted.

On to the original question: embedding as base64 vs dynamically loading. I would guess loading via a separate request is better memory-wise, but worse performance-wise. Without memory reports comparing the two or a more intimate knowledge of font-loading that's bout all I can say.

Johnathan seems to know a bit more about fonts (or at least has some background on the bugs I mentioned), perhaps he could weigh in on this.

Comment 52

[Justin D'Arcangelo [:justindarc]]

(In reply to Tapas Kumar Kundu from comment #47)
> (In reply to Justin D'Arcangelo [:justindarc] from comment #46)
> > Landed on v1.4:
> > 
> > https://github.com/mozilla-b2g/gaia/commit/
> > b7d36622c7df92c976c37520ccab25199c7ada91
> > 
> > Eric/Hema: Who makes the ultimate decision as to whether or not to call this
> > a blocking regression? This patch should bring a little bit of memory
> > savings to Camera, 
> 
> Could you please give us a PR for ffos 2.0 ? 
> 
> 
> > but it would be impossible to not have any increase going
> > from v1.3 to v1.4. I will leave this bug open until I hear your input on
> > this.
> 
> Could you please send us a test patch which will disable certain features in
> current gaia camera app in FFOS 2.0 to remove this memory regression fully ? 
> 
> Based on your test patch, we can decide whether to keep those features in
> v2.0 or not. 
> Thanks a lot for your help.

Even if we disable features and don't load the modules required for those features, we still incur some additional memory overhead from the overall increase in size of the codebase as well as parts of the new UI design that landed in v1.4. Simply put, the app is more sophisticated than it was in v1.3, so while we may be able to reduce the memory usage by disabling some features, the amount by which we will save in disabling them will not likely get us all the way back to our v1.3 numbers.

Comment 53

[Jonathan Kew [:jfkthame]]

(In reply to Eric Rahm [:erahm] from comment #51)
> (In reply to Justin D'Arcangelo [:justindarc] from comment #45)
> > (In reply to Eric Rahm [:erahm] from comment #44)
> > > Created attachment 8454586 [details]
> > > DMD, GC, and CC logs
> > > 
> > > There's not a ton of big ticket items in the DMD report. I did notice a fair
> > > amount of font-related entries in the top 25 accounting for ~172KB.
> > 
> > We do use an icon-font for all the icons in the app. We currently inline the
> > icon font as a base-64 encoded string in the CSS. Are there any known
> > memory-related pros/cons to doing that versus loading via separate request?
> 
> A bit of a tangent:
> It looks like bug 1008458 was going to allow us to load the icon font from
> the system, which seems like it would have been a good thing memory-wise but
> that got nixed. In bug 1030829 we added the ability to at least look up
> cached fonts by data URIs, so in theory we're avoiding the loading overhead
> (as long as the URIs match of course). In general I think that's a loss for
> us memory-wise as we're now keeping the base64-encoded font in memory (twice
> I'm guessing, once in the font cache and once in the CSS).

Yes, unfortunately; although the preloading of the font into cache happens before the Nuwa fork, so my understanding is that all processes should be sharing the same memory pages for this.

AFAICT the camera app isn't currently taking advantage of this, however; it's simply loading its own embedded gaia-icons, not using the preloaded font from the system.

> Also that just
> landed in 33 so, we're not going to see the performance win in 1.4/2.0
> unless it gets uplifted.
> 
> On to the original question: embedding as base64 vs dynamically loading. I
> would guess loading via a separate request is better memory-wise, but worse
> performance-wise.

That's correct, in general. Linking to a separate file allows FreeType to mmap() the font, whereas using embedded base64 means we have to decode this to an in-memory buffer (in addition to the base64 data hanging around in the resource's URI) that we can then hand to FreeType in its entirety.

Linking to a separate file means that the font doesn't get loaded at all until layout encounters a character styled to use that font; then it kicks off an asynchronous resource fetch. Later, once the font is loaded, we reflow again to render using it. So there's some potential lag here. OTOH, a base64 data: URI is loaded synchronously when we need the font, so it blocks that reflow but does not trigger a re-reflow later. And if the data: URI corresponds to a pre-cached font (as per bug 1030829), the load is almost instant.

Comment 54

[Justin D'Arcangelo [:justindarc]]

Attachment: pull-request (v2.0)

Patch for v2.0 branch, carrying R+ forward.

Comment 55

[Justin D'Arcangelo [:justindarc]]

Comment on attachment 8456246 [details] [review]
pull-request (v2.0)

Diego: This patch for v2.0 has changed a little more than the previous one you reviewed for v1.4, so I'm resetting the flag to R? -- I was able to get about ~0.5 MB savings by *NOT* pre-loading our sound assets. The total savings from this patch is now about 0.77 MB:

-0.77 MB (100.0%) -- explicit
├──-0.54 MB (70.99%) -- media
│  ├──-0.41 MB (53.00%) ── libogg
│  ├──-0.13 MB (16.91%) ── resources [-]
│  └──-0.01 MB (01.08%) ── decoded/audio
├──-0.11 MB (13.82%) ── heap-unclassified
├──-0.09 MB (12.25%) -- window-objects/top(app://camera.gaiamobile.org/index.html, id=1)
│  ├──-0.08 MB (010.00%) -- active/window(app://camera.gaiamobile.org/index.html)
│  │  ├──-0.03 MB (04.42%) -- js-compartment(app://camera.gaiamobile.org/index.html)
│  │  │  ├──-0.02 MB (02.54%) -- shapes
│  │  │  │  ├──-0.01 MB (01.39%) -- gc-heap
│  │  │  │  │  ├──-0.01 MB (01.44%) -- tree
│  │  │  │  │  │  ├──-0.01 MB (01.38%) ── global-parented
│  │  │  │  │  │  └──-0.00 MB (00.05%) ── non-global-parented
│  │  │  │  │  └───0.00 MB (-0.05%) ++ (2 tiny)
│  │  │  │  └──-0.01 MB (01.15%) -- malloc-heap
│  │  │  │     ├──-0.01 MB (01.17%) ── tree-tables
│  │  │  │     └───0.00 MB (-0.02%) ++ (2 tiny)
│  │  │  ├──-0.01 MB (01.36%) ++ objects
│  │  │  └──-0.00 MB (00.52%) ++ (3 tiny)
│  │  ├──-0.03 MB (03.28%) -- dom
│  │  │  ├──-0.02 MB (02.22%) ── element-nodes
│  │  │  └──-0.01 MB (01.06%) ++ (2 tiny)
│  │  └──-0.02 MB (02.30%) -- layout
│  │     ├──-0.01 MB (01.24%) ++ (2 tiny)
│  │     └──-0.01 MB (01.06%) ── rule-nodes
│  └──-0.02 MB (02.26%) -- js-zone(0xNNN)
│     ├──-0.01 MB (01.02%) ── type-pool
│     ├──-0.01 MB (01.02%) ── unused-gc-things
│     └──-0.00 MB (00.22%) ++ (3 tiny)
├──-0.02 MB (02.01%) -- heap-overhead
│  ├──-0.01 MB (01.02%) ── bookkeeping
│  └──-0.01 MB (00.99%) ── bin-unused
├──-0.01 MB (00.92%) -- js-non-window
│  ├──-0.00 MB (00.52%) -- runtime
│  │  ├──-0.00 MB (00.03%) -- script-sources
│  │  │  ├──-0.01 MB (01.26%) -- source(scripts=92, <non-notable files>)
│  │  │  │  ├──-0.01 MB (01.21%) ── misc [-]
│  │  │  │  └──-0.00 MB (00.05%) ── uncompressed [-]
│  │  │  └───0.01 MB (-1.23%) -- source(scripts=90, <non-notable files>)
│  │  │      ├──0.01 MB (-1.19%) ── misc [+]
│  │  │      └──0.00 MB (-0.04%) ── uncompressed [+]
│  │  └──-0.00 MB (00.50%) ── script-data
│  └──-0.00 MB (00.40%) ++ zones/zone(0xNNN)
└──-0.00 MB (00.01%) ── atom-tables

Comment 56

[Justin D'Arcangelo [:justindarc]]

I should clarify that the memory diff report in Comment 55 is comparing the v2.0 branch *WITHOUT* the patch to the v2.0 branch *WITH* the patch.

Comment 57

[Diego Marcos [:dmarcos]]

Comment on attachment 8456246 [details] [review]
pull-request (v2.0)

Looking good!

Comment 58

[Justin D'Arcangelo [:justindarc]]

Landed in v2.0:

https://github.com/mozilla-b2g/gaia/commit/8a8622d58af2c24ef8df1c7fc7aa9cda50ac0085

Comment 59

[Justin D'Arcangelo [:justindarc]]

Attachment: pull-request (master)

Pull Request for master (v2.1) -- Carrying R+ forward

Comment 60

[Justin D'Arcangelo [:justindarc]]

Landed on master:

https://github.com/mozilla-b2g/gaia/commit/956b712ba7d4d1532e7099c6851db90cf3451130

Comment 61

[Yeojin Chung [:YeojinC]]

No STR is present to create test case to address bug.