As a follow up to bug #960933, implement a memory watcher using bug #923275's memory actor to display memory widgets in Firefox OS's devtools layers.

Tagging with perf as the team is likely interested in this one.

Actually I have some concerns about the performance impact of the tool itself. It should probably wait until tracked metrics are configured with prefs.

Comment on attachment 8370791 [details] [diff] [review] Implement an App Memory Watcher for Firefox OS. Nicholas, I'd like to display app memory info in Firefox OS, using the "totalSize" provided by Panos' memory actor (bug #923275), which is using nsIMemoryReporterManager. I'm interested in general feedback about this patch: 1. Do you think that polling for memory usage every 3 seconds on all running apps is the right approach? 2. Could that have a bad impact on the device's performance? 3. Does "totalSize" really represent all the memory currently used by an app? What about USS information?

Comment on attachment 8368591 [details] Display app memory information. Would like to see the new code to format memory.

> 1. Do you think that polling for memory usage every 3 seconds on all running > apps is the right approach? > 2. Could that have a bad impact on the device's performance? > 3. Does "totalSize" really represent all the memory currently used by an > app? What about USS information? SizeOfTab() does some internal measurements of heap allocations (i.e. malloc, new, etc) and mmap() allocations. Some notable characteristics of it: - It measures a single window. (On desktop, this lets us measure a single tab.) - It breaks the window's consumption up into half a dozen groups: DOM, style, JS, etc. - While it's fairly fast, it's not trivial to compute -- there is lots of data structure traversal. - It doesn't know about any sharing between processes (e.g. the kind that Nuwa introduces). But if you're trying to just get per-app measurements, it's not a good match -- you don't need the first two characteristics, and the latter two characteristics are a disadvantage. If you just want USS (or PSS, or RSS) for each app, you'd be better off just reading data from /proc. You should look at bug 945973, where I did just that. But I think you should also describe exactly what you want this feature to look like. There are lots of different memory metrics. And it's not clear to me how a developer would use this feature. In my experience, if you give somebody coarse-grained memory profiling, they'll use it and then almost immediately ask for more fine-grained memory profiling. For example, knowing that memory consumption spiked isn't that useful if you don't know what caused it to spike.

Comment on attachment 8370791 [details] [diff] [review] Implement an App Memory Watcher for Firefox OS. I don't know much about the memory reporter code, :njn will know better, be here is a guess: If I'm not mistaken, jsMilliseconds + nonJSMilliseconds will tell you how long it takes to run `measure()`, right? Also, I guess the memory reporter code runs in the main thread (I'm not sure), so you could adapt the timeout to ensure that it we won't slow down the event loop of more than 5%. Basically, instead of an interval, use a timeout: lastDuration = jsMilliseconds + nonJSMilliseconds; nextTick = 20 * lastDuration; setTimeout(measure, nextTick); Does it make sense?

Comment on attachment 8370791 [details] [diff] [review] Implement an App Memory Watcher for Firefox OS. Thanks a lot for the review and great feedback! My goal is to display simple per-app memory counters (or graphs) on the device. While I agree that coarse-grained memory profiling has limited use, I think that such a widget can help notice memory problems. These problems can then be investigated with more precise tools, like a detailed DOM, style, JS, etc consumption view, or an on-device about:memory equivalent. I also think that both SizeOfTab() and PSS (or USS with Nuwa) will have their uses. The simple SizeOfTab widget of this patch has already proven useful to some gaia developers, but we're also going to be interested in USS at some point. I'll add USS in a follow-up, once widgets are configurable, in order not to confuse people with two different memory metrics.

I think USS is the most desirable metric, since this is what we get back once the app is killed. That's what we will soon get on datazilla too, so that would be nice to not compare apples to oranges ;)

Before the weekend, I had the chance to poll the gaia team on what they thought was a useful memory metric. We reached the conclusion that we needed both SizeOfTab and USS. I'll go ahead and land this with SizeOfTab (once I've addressed the nits), and file a new bug to track USS, blocked on bug #968220 so that it lands after metrics are configurable.

(In reply to Nicholas Nethercote [:njn] from comment #9) > But I think you should also describe exactly what you want this feature to > look like. There are lots of different memory metrics. And it's not clear to > me how a developer would use this feature. In my experience, if you give > somebody coarse-grained memory profiling, they'll use it and then almost > immediately ask for more fine-grained memory profiling. For example, knowing > that memory consumption spiked isn't that useful if you don't know what > caused it to spike. I think this tool should stay a high overview, and its really not a replacement for about:memory. This is more a 'warning' tool that tells you that you may want to use about:memory. It also allow Gaia devs (or any third party devs) to quickly do a patch / push it / run and see the effects without having to dig into about:memory (which I have been told is hard to read for most of our web developers).

That said an to be honest I already asked Janx if we can have some checkboxes to select the type of memory that is displayed (DOM, style, images, JS, etc...). That would let us see if the system app does not consume memory for images for example when it should not (it should only be the icons of the status bar). For sure I can use about:memory for that, but with such a tool it should be easy to connect the device to the system app via the app manager and play with the dom in real time and observe changes.

I will try to achieve something akin to this screenshot. - "Process memory" will display Unique Set Size. - "App memory" will display app window memory. When activated, the options you see below will appear, allowing more fine-grained tracking of memory. I think this is the best we can achieve for a high-level memory overview. Once a memory problem is detected, if it's not a trivial fix, it can be investigated more using in-depth memory tools like the App Manager and about:memory.

Vivien, what do you think about that approach? Do you think I should group "JS objects", "JS strings" and "JS other" into a single "JS" category?

(In reply to Jan Keromnes [:janx] from comment #17) > Vivien, what do you think about that approach? Do you think I should group > "JS objects", "JS strings" and "JS other" into a single "JS" category? I'm a bit uncertain about grouping them yet. Do you know what is hiding behing the JS other ? One of the nice use case for this tool would be to be able to dump the memory used by images. Do you know if images manipulated by JS fits into this category ?

> - "Process memory" will display Unique Set Size. Please call it "Unique Set Size". There are lots of different memory measurements with standard names. If you use something unclear like "process memory" everyone just has to scratch their head and wonder what it actually mean.

(In reply to Nicholas Nethercote [:njn] from comment #19) > Please call it "Unique Set Size". There are lots of different memory > measurements with standard names. If you use something unclear like "process > memory" everyone just has to scratch their head and wonder what it actually > mean. I completely agree and already renamed it to "Unique set size" locally. I'm still pondering the "App memory" name though.

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #18) > I'm a bit uncertain about grouping them yet. Do you know what is hiding > behing the JS other? I'm going to not group them, the feedback I received was pretty much unanimous about this. I guess "JS other" means all the non-object, non-strings things like gc metadata, source data, math cache, etc. Nicholas probably knows more about this. > One of the nice use case for this tool would be to be able to dump the > memory used by images. Do you know if images manipulated by JS fits into > this category ? I agree about the use case, but I don't know which category images get classified into, if at all. 'Needinfoing' Nicholas.

> I guess "JS other" means all the non-object, > non-strings things like gc metadata, source data, math cache, etc. Nicholas > probably knows more about this. Yes. It's everything we can report about JS other than objects and strings. Look at the "js-main-runtime" tree in about:memory, there's heaps of stuff there. The reason I split out objects and strings is that they are language-level things that a JS programmer has clear control over, as opposed to things like shapes and type inference data and a zillion other things that are concepts internal to the JS engine. > > One of the nice use case for this tool would be to be able to dump the > > memory used by images. Do you know if images manipulated by JS fits into > > this category ? All the image data for a single process is currently reported in the "images" sub-tree in about:memory. For desktop Firefox, this means that we unfortunately don't have per-tab image measurements (bug 921300 is open for that). For FxOS apps this perhaps isn't a problem, since apps are their own processes. nsIMemoryReporterManager already has a |imagesContentUsedUncompressed| member which you could use; it's implemented here: http://dxr.mozilla.org/mozilla-central/source/image/src/imgLoader.cpp#131. Or if you want to report all aspects of the image data (both raw and uncompressed, content and chrome) you could add another, similar member.

Comment on attachment 8380558 [details] [diff] [review] Add memory tracking to Firefox OS's Developer HUD. r=vingtetun Punting USS and images to follow-ups.

Comment on attachment 8368591 [details] Display app memory information. r+ with one nit.

Comment on attachment 8380558 [details] [diff] [review] Add memory tracking to Firefox OS's Developer HUD. r=vingtetun Review of attachment 8380558 [details] [diff] [review]: ----------------------------------------------------------------- LGTM.

Comment on attachment 8382376 [details] [diff] [review] Add memory tracking to Firefox OS's Developer HUD. r=vingtetun Rebase and keep r+.

`checking-needed` for the Gecko patch. Please don't merge the Gaia pull request before the Gecko patch has landed! (It would break the Developer HUD).

Do you realize that you just asked localizers to translate all dev settings strings again? If so, why?

Axel, I thought that Firefox OS translations were done once the tree was forked into a branch. The Developer HUD being new in 1.4, I expected the new strings to be translated only once when the 1.4 branch will be created. The only properties that existed before 1.4 and that I modified are "hud-ttl" and "hud-fps", and I was requested to change their IDs because I modified the English translation. Are properties actually translated into every language as soon as any change to them lands in the tree? If that is the case, I'm sorry for the mess.

We should be in a state where all teams consistently translate fx os during the development. We're still far away from that sadly. Right now it's some 10 teams that are more or less up-to-date. The idea of the 1.4 schedule just didn't have time to wait for the code to be done. I'm specifically talking about https://github.com/mozilla-b2g/gaia/pull/15870/files#diff-3, which removed the developer- prefix on existing strings. Let's keep things as they are now, different toolchains are going to be differently impacted, some not at all, some only a little. Some will end up re-translating the 8 strings. Note, I don't see the change in -ttl and -fps?

Ok, thanks for the details. The -ttl and -fps were changed in a previous patch. I was in the hope that changing them again soon after wouldn't cause double re-translation. In the future, I'll refrain from renaming l10n keys like that.

This feature should be documented at https://developer.mozilla.org/en-US/Firefox_OS/Debugging/Developer_settings

Adding this bug to Firefox OS, based on comment #37. If you consider that we should verify this on Desktop, please flip this back.