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

Camera App memory regression in FxOS v1.4

RESOLVED FIXED in Firefox OS v1.4

Status

Firefox OS
Gaia::Camera
P1
blocker
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: tkundu, Assigned: justindarc)

Tracking

({footprint, perf, regression})

unspecified
2.0 S6 (18july)
ARM
Gonk (Firefox OS)
footprint, perf, regression
Bug Flags:
in-moztrap -

Firefox Tracking Flags

(blocking-b2g:2.0+, b2g-v1.3 unaffected, b2g-v1.4 fixed, b2g-v2.0 fixed, b2g-v2.1 fixed)

Details

(Whiteboard: [c=memory p= s=2014.07.18.t u=2.0] [caf priority: p2][MemShrink:P2] [CR 690195] [landing eta 7/18])

Attachments

(9 attachments, 2 obsolete attachments)

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.
blocking-b2g: --- → 2.0?

Updated

3 years ago
blocking-b2g: 2.0? → 2.0+

Updated

3 years ago
Keywords: footprint, perf
Whiteboard: [MemShrink]

Updated

3 years ago
Keywords: regression

Comment 1

3 years ago
Justin,

Can you help investigate this on flame please?

Thanks
Hema
Flags: needinfo?(jdarcangelo)
(Assignee)

Updated

3 years ago
Assignee: nobody → jdarcangelo
Flags: needinfo?(jdarcangelo)
(Assignee)

Comment 2

3 years ago
(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.).
Flags: needinfo?(tkundu)
(Assignee)

Comment 3

3 years ago
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
I'll get some memory reports for this.
Flags: needinfo?(erahm)
(Assignee)

Comment 5

3 years ago
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.
(Assignee)

Comment 6

3 years ago
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.
Flags: needinfo?(mlee)
(Assignee)

Updated

3 years ago
See Also: → bug 1029856
(Assignee)

Comment 7

3 years ago
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
A ~50% increase in the PSS does seem like something we should look at.
Flags: needinfo?(mlee)
Whiteboard: [MemShrink] → [c=devtools p= s= u=] [MemShrink]
oops
Whiteboard: [c=devtools p= s= u=] [MemShrink] → [c=memory p= s= u=] [MemShrink]

Updated

3 years ago
Whiteboard: [c=memory p= s= u=] [MemShrink] → [c=memory p= s= u=2.0] [MemShrink]
(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.
Flags: needinfo?(tkundu)
(Assignee)

Comment 11

3 years ago
(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+?
(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
(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.
Flags: needinfo?(wchang)
Let's keep this on 2.0+ for now given we have 1.4+ on bug 1029856, where an actual problem is seen.
Flags: needinfo?(wchang)
(Assignee)

Comment 15

3 years ago
(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+

Updated

3 years ago
Target Milestone: --- → 2.0 S5 (4july)
(Assignee)

Comment 16

3 years ago
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.
(Assignee)

Comment 17

3 years ago
(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?
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.
Flags: needinfo?(mhabicher)
Flags: needinfo?(jdarcangelo)
(Assignee)

Comment 19

3 years ago
(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.
Flags: needinfo?(jdarcangelo)
(Assignee)

Comment 20

3 years ago
(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.
(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.
Flags: needinfo?(mhabicher)
Created attachment 8449714 [details]
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.
Flags: needinfo?(jdarcangelo)
Created attachment 8450276 [details]
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.
Attachment #8449714 - Attachment is obsolete: true
Attachment #8450276 - Flags: feedback?(jdarcangelo)
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.
Created attachment 8450488 [details]
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.
Flags: needinfo?(erahm)
(Assignee)

Comment 26

3 years ago
Created attachment 8451435 [details]
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.
Attachment #8450276 - Attachment is obsolete: true
Attachment #8450276 - Flags: feedback?(jdarcangelo)
Attachment #8451435 - Flags: feedback?(mhabicher)
(Assignee)

Comment 27

3 years ago
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).
Whiteboard: [c=memory p= s= u=2.0] [MemShrink] → [c=memory p= s= u=2.0] [MemShrink] [CR 690195]

Updated

3 years ago
Whiteboard: [c=memory p= s= u=2.0] [MemShrink] [CR 690195] → [caf priority: p1][c=memory p= s= u=2.0] [MemShrink] [CR 690195]

Updated

3 years ago
Whiteboard: [caf priority: p1][c=memory p= s= u=2.0] [MemShrink] [CR 690195] → [caf priority: p2][c=memory p= s= u=2.0] [MemShrink] [CR 690195]
Whiteboard: [caf priority: p2][c=memory p= s= u=2.0] [MemShrink] [CR 690195] → [caf priority: p2][c=memory p= s= u=2.0] [MemShrink:P2] [CR 690195]
(Assignee)

Comment 28

3 years ago
Created attachment 8453210 [details]
memory report 1.3

I generated this memory report using the v1.3 build that is included with the v122 base build.
Flags: needinfo?(jdarcangelo)
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 [+]
(Assignee)

Comment 30

3 years ago
Created attachment 8453229 [details]
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
(Assignee)

Comment 31

3 years ago
(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).
Flags: needinfo?(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.
Flags: needinfo?(erahm)
(Assignee)

Comment 33

3 years ago
(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.
(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 ?

Updated

3 years ago
Flags: needinfo?(jdarcangelo)
(Assignee)

Comment 35

3 years ago
(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.
Flags: needinfo?(jdarcangelo)
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.
(Assignee)

Comment 37

3 years ago
Created attachment 8453974 [details] [review]
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).
Attachment #8453974 - Flags: review?(dmarcos)
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.
Flags: needinfo?(n.nethercote)
Maybe the "require" stuff at the top is pulling in more source?  I have no idea how that works.
(Assignee)

Comment 40

3 years ago
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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 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.
Attachment #8453974 - Flags: review?(dmarcos) → review-
I think comment 40 addresses the needinfo from comment 38. Thanks, Justin.
Flags: needinfo?(n.nethercote)
(Assignee)

Comment 43

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

Diego: Please re-review when you get a chance. Addressed your PR comments. Thanks!
Attachment #8453974 - Flags: review- → review?(dmarcos)
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.
(Assignee)

Comment 45

3 years ago
(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?
Attachment #8453974 - Flags: review?(dmarcos) → review+
(Assignee)

Comment 46

3 years ago
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.
Flags: needinfo?(hkoka)
Flags: needinfo?(erahm)
(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.
Flags: needinfo?(jdarcangelo)

Comment 48

3 years ago
(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
Flags: needinfo?(hkoka)
(Assignee)

Comment 49

3 years ago
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.
status-b2g-v1.3: --- → unaffected
status-b2g-v1.4: --- → affected
status-b2g-v2.0: --- → affected
status-b2g-v2.1: --- → affected
Summary: Camera App memory regression in FFOS 2.0 → Camera App memory regression in FxOS v1.4
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?
(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.
Flags: needinfo?(erahm) → needinfo?(jfkthame)
(Assignee)

Comment 52

3 years ago
(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.
(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.
Flags: needinfo?(jfkthame)
(Assignee)

Comment 54

3 years ago
Created attachment 8456246 [details] [review]
pull-request (v2.0)

Patch for v2.0 branch, carrying R+ forward.
Attachment #8456246 - Flags: review+
Flags: needinfo?(jdarcangelo)

Updated

3 years ago
Target Milestone: 2.0 S5 (4july) → 2.0 S6 (18july)
(Assignee)

Comment 55

3 years ago
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
Attachment #8456246 - Flags: review+ → review?(dmarcos)
(Assignee)

Comment 56

3 years ago
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 on attachment 8456246 [details] [review]
pull-request (v2.0)

Looking good!
Attachment #8456246 - Flags: review?(dmarcos) → review+

Updated

3 years ago
Whiteboard: [caf priority: p2][c=memory p= s= u=2.0] [MemShrink:P2] [CR 690195] → [caf priority: p2][c=memory p= s= u=2.0] [MemShrink:P2] [CR 690195] [landing eta 7/18]
(Assignee)

Comment 58

3 years ago
Landed in v2.0:

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

Comment 59

3 years ago
Created attachment 8458292 [details] [review]
pull-request (master)

Pull Request for master (v2.1) -- Carrying R+ forward
Attachment #8458292 - Flags: review+
(Assignee)

Comment 60

3 years ago
Landed on master:

https://github.com/mozilla-b2g/gaia/commit/956b712ba7d4d1532e7099c6851db90cf3451130
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-b2g-v1.4: affected → fixed
status-b2g-v2.0: affected → fixed
status-b2g-v2.1: affected → fixed
Resolution: --- → FIXED

Updated

3 years ago
Whiteboard: [caf priority: p2][c=memory p= s= u=2.0] [MemShrink:P2] [CR 690195] [landing eta 7/18] → [c=memory p= s=2014.07.18.t u=2.0] [caf priority: p2][MemShrink:P2] [CR 690195] [landing eta 7/18]

Updated

3 years ago
Blocks: 984663

Updated

3 years ago
No longer blocks: 984663
Attachment #8451435 - Flags: feedback?(mhabicher) → feedback+
Flags: in-moztrap?(ychung)
No STR is present to create test case to address bug.
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
Flags: in-moztrap?(ychung)
Flags: in-moztrap-
You need to log in before you can comment on or make changes to this bug.