909577 - API for reading process info

Status: NEW

(Toolkit :: General, defect)

Description

[Gregory Szorc [:gps]]

:mihneadb is working on instrumenting our test harnesses to record process resource usage (CPU, I/O, memory, etc). After much mucking around, it's becoming clearer that obtaining resource usage from within the XPCOM process is simpler than obtaining it from an external process (such as the Python test runner).

I'm proposing adding functionality to Gecko to quickly and easily access the current process's basic resource info. Initially, I'm thinking this will be a JSM. Eventually, we can convert it to an XPCOM component and/or rewrite it in C++.

https://hg.mozilla.org/mozilla-central/file/14b1e8c2957e/toolkit/components/telemetry/TelemetryPing.js#l104 is some existing code that does something similar.

Comment 1

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: API for reading process info

First iteration

Comment 2

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: API for reading process info

Comment 3

[David Teller [:Yoric] - still alive but not very active]

I am suspicious of the idea of reading a file (even one from /proc) on the main thread. Can anyone tell with absolute certainty that this will not cause main thread I/O?

Comment 4

[Mihnea Dobrescu-Balaur (:mihneadb)]

When you'd use the numbers to compare two processes against each other you will basically have the same thing in both, so I'd say it is negligible.

Or maybe I didn't get the problem you are referring to?

Comment 5

[Mihnea Dobrescu-Balaur (:mihneadb)]

(Just for the record, this module's purpose is profiling resource usage. If you are worried about loss of smoothness, I'd say this is not an issue for this use case.)

Comment 6

[Gregory Szorc [:gps]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #3)
> I am suspicious of the idea of reading a file (even one from /proc) on the
> main thread. Can anyone tell with absolute certainty that this will not
> cause main thread I/O?

We should have a conversation about this!

I advised Mihnea to use synchronous/main thread I/O for reading from /proc because I don't believe access to the /proc/self "files" here will delay the main thread because /proc is a virtual filesystem that effectively provides access to kernel data structures. Therefore, I view reading content from *certain* /proc "files" as the equivalent of calling an API like getrusage(), something we wouldn't need to offload to a child thread.

I'm assuming that under the hood nsIFile is just a wrapper around the open/read/close system calls and doesn't do anything complicated like marshal requests to different threads (like OS.File).

Furthermore, I think that obtaining resource usage should have as little probe overhead as possible. OS.File with it's model of moving requests to a different thread - a good model for most scenarios - just seems overkill for this.

Also, we don't *currently* intend for this API to be used in shipping code - just by test harnesses. However, once the cat is out of the bag and this module ships, all bets are off.

All that being said, I would like sign-off from someone on the performance team that synchronous I/O for reading select files from /proc is acceptable. I hope I've presented sufficient justification for it not being an issue.

Comment 7

[Gregory Szorc [:gps]]

Comment on attachment 795816 [details] [diff] [review]
API for reading process info

Review of attachment 795816 [details] [diff] [review]:
-----------------------------------------------------------------

Nice!

I think you need to work on the API. Specifically, we don't want callers to be concerned with the platform type - we should expose a high-level "getIOCounters" etc that "just works." This likely means normalizing the returned data to a common format similar to how psutil does it.

We'll also need some form of error handling. On Linux, I'm pretty sure the format of the /proc/self/* structures you are reading are different in older kernel versions. We should raise a proper exception type (not file not found) if we have errors reading these.

::: toolkit/modules/ProcessInfo.jsm
@@ +13,5 @@
> +
> +
> +function getFileFromPath(path) {
> +  let file = Cc["@mozilla.org/file/local;1"]
> +             .createInstance(Ci.nsILocalFile);

Nit: Align .createInstance under the [ (2 space indent):

Cc["@mozilla.org/file/local;1"]
  .createInstance(Ci.nsILocalFile)
  .foo(xxx);

@@ +22,5 @@
> +function readFileAsLines(path) {
> +  let file = getFileFromPath(path);
> +  // open an input stream from file
> +  let istream = Cc["@mozilla.org/network/file-input-stream;1"].
> +    createInstance(Ci.nsIFileInputStream);

Nit: Put the . on the 2nd line and align under the [ from Cc[ (it's how we do things - don't ask).

@@ +38,5 @@
> +
> +  return lines;
> +}
> +
> +function Linux__getIOCounters() {

We typically don't use underscores in function names. I've never seen double underscore used.

@@ +45,5 @@
> +  let data = {};
> +  lines.map(function(line) {
> +    let [key, value] = line.split(":");
> +    data[key] = value | 0;
> +  });

Since we are doing this from performance-sensitive code, might want to avoid the .map() and just iterate over the lines.

@@ +69,5 @@
> +  let startPos = line.indexOf(")") + 2;
> +  let values = line.substring(startPos).split(" ");
> +  return {
> +    utime: values[11] | 0,
> +    stime: values[12] | 0

cutime and cstime would also be interesting so we can look at low-level cycle counts over time.

Comment 8

[David Teller [:Yoric] - still alive but not very active]

Well, if this code isn't meant to be shipped in production, I guess that everything is permitted.

However, if it does perform main thread I/O (even something as simple as open/read/close), you should remember that every operation has an unbounded and unpredictable duration. On a busy system, this can be orders of magnitude worse than sending a message to a dedicated thread (seconds vs. milliseconds).

If you consider that the overhead of OS.File is unacceptable, you should write the code in C++ and communicate with Dispatch.

Comment 9

[Gregory Szorc [:gps]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #8)
> Well, if this code isn't meant to be shipped in production, I guess that
> everything is permitted.

Well, it needs to be shipped (unless we make it a testing-only module). Perhaps we should add a prominent comment and/or a console.warning when the file is loaded to help get the message across?

> If you consider that the overhead of OS.File is unacceptable, you should
> write the code in C++ and communicate with Dispatch.

In the ideal world, yeah. I'd like this to be an XPCOM component implemented in C++. However, Mihnea's just trying to get a test harness feature implemented, learning XPCOM/C++ is beyond the scope of that feature, and perfect is the enemy of done, so JS implementation is good for now! I was thinking that we could at least try to ensure API compatibility with the future XPCOM component, which is why I'd like to avoid OS.File, promises, and callbacks. Who knows. The consumers of this should be small, so we can always throw everything away later once something better is available.

Comment 10

[Gregory Szorc [:gps]]

Perhaps we should integrate this into https://mxr.mozilla.org/mozilla-central/source/toolkit/components/perf/PerfMeasurement.jsm

Comment 11

[Justin Dolske [:Dolske]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #8)

> However, if it does perform main thread I/O (even something as simple as
> open/read/close), you should remember that every operation has an unbounded
> and unpredictable duration. On a busy system, this can be orders of
> magnitude worse than sending a message to a dedicated thread (seconds vs.
> milliseconds).

But isn't gps' argument (which makes sense to me) that this doesn't apply? Stuff in /proc doesn't map to anything on a physical disk (well, generally), so the usual I/O arguments don't apply. It's more akin to reading from /dev/zero or writing to /dev/null.

That said, unless there's a compelling reason not to, we should probably be in the habit of doing async file operations in every case. If for no other reason than avoiding the need to justify it, the inevitable "bugs" that will be filed about it in the future, and poor cut'n'paste habits. :)

Comment 12

[David Teller [:Yoric] - still alive but not very active]

> But isn't gps' argument (which makes sense to me) that this doesn't apply? Stuff in /proc doesn't map to anything on a physical disk (well, generally), so the usual I/O arguments don't apply. It's more akin to reading from /dev/zero or writing to /dev/null.

Well, hence the *if*. Everybody here is almost convinced that this is not the case. We just need the reassurance that we are right.

Comment 13

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: API for reading process info

Work in progress.

Comment 14

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: API for reading process info

Will leave memory for later.

Comment 15

[Mihnea Dobrescu-Balaur (:mihneadb)]

Comment on attachment 799193 [details] [diff] [review]
API for reading process info

Fire away

Comment 16

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: API for reading process info

Reindented & stuff.

Comment 17

[Gregory Szorc [:gps]]

Comment on attachment 799224 [details] [diff] [review]
API for reading process info

Review of attachment 799224 [details] [diff] [review]:
-----------------------------------------------------------------

Looking good!

It would be nice if the API could be more standardized between platforms. Although, there are so many subtle differences. I have a feeling you'd end with a dict with sparsely populated fields due to the different semantics.

::: toolkit/modules/ProcessInfo.jsm
@@ +32,5 @@
> +  let file = getFileFromPath(path);
> +  // open an input stream from file
> +  let istream = Cc["@mozilla.org/network/file-input-stream;1"]
> +                  .createInstance(Ci.nsIFileInputStream);
> +  istream.init(file, 0x01, 0444, 0);

You should use a constant instead of 0x01.

@@ +68,5 @@
> +                                                IO_COUNTERS.ptr); // lpIoCounters
> +
> +    let GetCurrentProcess = kernel32.declare('GetCurrentProcess',
> +                                             ctypes.winapi_abi,
> +                                             ctypes.voidptr_t); // return type

You probably want to cache some of these ctypes data structures. I'd have a setup function at module scope that returns early if things are cached.

@@ +76,5 @@
> +
> +    let data = {
> +      readBytes: parseInt(io.readBytes),
> +      writeBytes: parseInt(io.writeBytes),
> +      otherBytes: parseInt(io.otherBytes)

Always pass a second argument to parseInt.

@@ +139,5 @@
> +    let data = {
> +      user: parseInt(processedUserTime.wSecond) +
> +            parseInt(processedUserTime.wMilliseconds) / 1000,
> +      sys: parseInt(processedKernelTime.wSecond) +
> +           parseInt(processedKernelTime.wMilliseconds) / 1000

What abouts minutes, hours, days, ...?

Why can't you just do math on the FILETIME struct? It's just a big integer.

@@ +189,5 @@
> +    let sysconf = libc.declare('sysconf',
> +                               ctypes.default_abi,
> +                               ctypes.long, // return type
> +                               ctypes.int); // name
> +    let _SC_CLK_TCK = sysconf(3); // #define _SC_CLK_TCK 3

Is SC_CLK_TCK constant or can it change while the machine is running (e.g. CPU frequency is adjusted)?

@@ +207,5 @@
> +
> +let OSX = {
> +  getIOCounters: function () {
> +    // not available on OS X
> +    return {total: 0};

total: 0 implies there's a value. I'd just return an empty object or an object indicating a special state. e.g. {unavailable: true}

@@ +217,5 @@
> +    return {
> +      peak: parseInt(usage.ru_maxrss),
> +      shared: parseInt(usage.ru_ixrss),
> +      unshared_data: parseInt(usage.ru_idrss),
> +      unshared_stack: parseInt(usage.ru_isrss),

parseInt(value, base)

@@ +279,5 @@
> +  this.ProcessInfo = Linux;
> +} else if (OS.name == 'Windows_NT') {
> +  this.ProcessInfo = Windows;
> +} else {
> +  this.ProcessInfo = null;

We should probably have a dummy object that implements the API so callers don't have to check this.

Comment 18

[Mihnea Dobrescu-Balaur (:mihneadb)]

(In reply to Gregory Szorc [:gps] from comment #17)
> Comment on attachment 799224 [details] [diff] [review]
> API for reading process info
> 
> Review of attachment 799224 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looking good!
> 
> It would be nice if the API could be more standardized between platforms.
> Although, there are so many subtle differences. I have a feeling you'd end
> with a dict with sparsely populated fields due to the different semantics.

Well the function calls are the same, and all the returning dicts have a "total" field. Not sure we can do more than that.

> 
> ::: toolkit/modules/ProcessInfo.jsm
> @@ +32,5 @@
> > +  let file = getFileFromPath(path);
> > +  // open an input stream from file
> > +  let istream = Cc["@mozilla.org/network/file-input-stream;1"]
> > +                  .createInstance(Ci.nsIFileInputStream);
> > +  istream.init(file, 0x01, 0444, 0);
> 
> You should use a constant instead of 0x01.

Problem is, the snippet[1] uses 0x01 and the docs[2] don't list any constant with that value. Magic!

> 
> @@ +68,5 @@
> > +                                                IO_COUNTERS.ptr); // lpIoCounters
> > +
> > +    let GetCurrentProcess = kernel32.declare('GetCurrentProcess',
> > +                                             ctypes.winapi_abi,
> > +                                             ctypes.voidptr_t); // return type
> 
> You probably want to cache some of these ctypes data structures. I'd have a
> setup function at module scope that returns early if things are cached.

I don't see this as perf-sensitive code since it is enabled only to get data and that's under a mach flag, but sure, will do.


> @@ +139,5 @@
> > +    let data = {
> > +      user: parseInt(processedUserTime.wSecond) +
> > +            parseInt(processedUserTime.wMilliseconds) / 1000,
> > +      sys: parseInt(processedKernelTime.wSecond) +
> > +           parseInt(processedKernelTime.wMilliseconds) / 1000
> 
> What abouts minutes, hours, days, ...?

I could add minutes, we won't have hours days etc since we have timeouts.

> 
> Why can't you just do math on the FILETIME struct? It's just a big integer.

I was trying to stick to the API. I can do that, sure.

> 
> @@ +189,5 @@
> > +    let sysconf = libc.declare('sysconf',
> > +                               ctypes.default_abi,
> > +                               ctypes.long, // return type
> > +                               ctypes.int); // name
> > +    let _SC_CLK_TCK = sysconf(3); // #define _SC_CLK_TCK 3
> 
> Is SC_CLK_TCK constant or can it change while the machine is running (e.g.
> CPU frequency is adjusted)?

From what I read it is constant, yes.

> @@ +279,5 @@
> > +  this.ProcessInfo = Linux;
> > +} else if (OS.name == 'Windows_NT') {
> > +  this.ProcessInfo = Windows;
> > +} else {
> > +  this.ProcessInfo = null;
> 
> We should probably have a dummy object that implements the API so callers
> don't have to check this.

I'm not sure I see the problem. This is handled in the module, so a caller will only use ProcessInfo.getSomething.

[1] https://developer.mozilla.org/en-US/docs/Code_snippets/File_I_O#Line_by_line
[2] https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIFileInputStream

Comment 19

[Mihnea Dobrescu-Balaur (:mihneadb)]

BTW, I did test runs with and without resource logging on Linux (where we read on the main thread) and there was no noticeable perf difference.

Comment 20

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: API for reading process info

Adressed most of the comments, (together with the previous reply).

I'd rather not manually do the math on the Windows struct since
they have the API for this. I find it a bit more readable (and more..
idiomatic?).

Comment 21

[Gregory Szorc [:gps]]

Comment on attachment 801955 [details] [diff] [review]
API for reading process info

Review of attachment 801955 [details] [diff] [review]:
-----------------------------------------------------------------

Looking good!

Please add some basic usage docs to ProcessInfo.jsm. See Sqlite.jsm (in the same directory) for examples.

::: toolkit/modules/ProcessInfo.jsm
@@ +2,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +this.EXPORTED_SYMBOLS = [ "ProcessInfo" ];

Nit: Cuddle braces.

@@ +40,5 @@
> +  let line = {}, lines = [], hasmore;
> +  do {
> +    hasmore = istream.readLine(line);
> +    lines.push(line.value);
> +  } while(hasmore);

This probably fails on empty lines (""), no? Do we care?

@@ +105,5 @@
> +
> +  getIOCounters: function () {
> +    this._setupCTypes();
> +    let io = new this._IO_COUNTERS();
> +    this._GetProcessIoCounters(this._GetCurrentProcess(), io.address());

Always check the return value and handle errors!

@@ +110,5 @@
> +
> +    let data = {
> +      readBytes: parseInt(io.readBytes, 10),
> +      writeBytes: parseInt(io.writeBytes, 10),
> +      otherBytes: parseInt(io.otherBytes, 10)

Might as well capture all the fields in the struct, no?

@@ +112,5 @@
> +      readBytes: parseInt(io.readBytes, 10),
> +      writeBytes: parseInt(io.writeBytes, 10),
> +      otherBytes: parseInt(io.otherBytes, 10)
> +    };
> +    data.total = data.readBytes + data.writeBytes + data.otherBytes;

data.totalBytes

@@ +127,5 @@
> +    this._GetProcessTimes(this._GetCurrentProcess(),
> +                  creationTime.address(),
> +                  exitTime.address(),
> +                  kernelTime.address(),
> +                  userTime.address());

Nit: This indenting is weird. Either align with paren or do standard 2 space indent.

@@ +142,5 @@
> +            parseInt(processedUserTime.wMilliseconds, 10) / 1000,
> +      sys: parseInt(processedKernelTime.wMinute, 10) * 60 +
> +           parseInt(processedKernelTime.wSecond, 10) +
> +           parseInt(processedKernelTime.wMilliseconds, 10) / 1000
> +    };

What about hours? I'd just perform math on the FILETIME struct. Something like:

let ticks = (kernelTime.dwHighDateTime << 32) + kernelTime.dwLowDateTime;
let milliseconds = ticks / 10000;

@@ +157,5 @@
> +
> +
> +let Linux = {
> +  _setupCTypes: function () {
> +    this._libc = ctypes.open('libc.so.6');

Can we just use libc.so? That should be symlinked to the latest libc on the system, no?

@@ +195,5 @@
> +
> +  getCPUTimes: function () {
> +    this._setupCTypes();
> +    // using ctypes to get value of _SC_CLK_TCK
> +    let _SC_CLK_TCK = this._sysconf(2); // #define _SC_CLK_TCK 2

This seems a bit fragile, but sadly it's probably the best way to do it. Can you at least throw a comment in here saying why you do it this way? Should save a bit of bugzilla foraging when someone looks at this code in the future.

@@ +202,5 @@
> +    let startPos = line.indexOf(")") + 2;
> +    let values = line.substring(startPos).split(" ");
> +    let data = {
> +      user: (values[11] | 0) / _SC_CLK_TCK,
> +      sys: (values[12] | 0) / _SC_CLK_TCK

Is there any chance the clk could be reported as 0? If so, we have a divide by 0 exception.

@@ +295,5 @@
> +  this.ProcessInfo = Linux;
> +} else if (OS.name == 'Windows_NT') {
> +  this.ProcessInfo = Windows;
> +} else {
> +  this.ProcessInfo = null;

I'd assign a dummy object that returns empty values when the public APIs are called.

::: toolkit/modules/tests/xpcshell/test_ProcessInfo.js
@@ +5,5 @@
> +
> +function run_test() {
> +  dump(JSON.stringify(ProcessInfo.getCPUTimes()) + "\n");
> +  dump(JSON.stringify(ProcessInfo.getMemoryUsage()) + "\n");
> +  dump(JSON.stringify(ProcessInfo.getIOCounters()) + "\n");

Can you add some more intelligent tests? Maybe add a sleep() or some I/O and verify counters (when present) increment.

Comment 22

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: API for reading process info

* cuddled braces
* added comment for the reason we get SC_CLK_TCK
* fixed indentation in Ctypes code

Comment 23

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: API for reading process info

* dummy object fallback

Comment 24

[Mihnea Dobrescu-Balaur (:mihneadb)]

Sorry for the delay, been traveling lately. Comments inline.

(In reply to Gregory Szorc [:gps] from comment #21)
> Comment on attachment 801955 [details] [diff] [review]
> API for reading process info
> 
> Review of attachment 801955 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looking good!
> 
> Please add some basic usage docs to ProcessInfo.jsm. See Sqlite.jsm (in the
> same directory) for examples.

Will do.

> 
> @@ +40,5 @@
> > +  let line = {}, lines = [], hasmore;
> > +  do {
> > +    hasmore = istream.readLine(line);
> > +    lines.push(line.value);
> > +  } while(hasmore);
> 
> This probably fails on empty lines (""), no? Do we care?

We don't care. I'll try to change it to use the stream's hasAvailable (or whatever it's called) method.

> 
> @@ +105,5 @@
> > +
> > +  getIOCounters: function () {
> > +    this._setupCTypes();
> > +    let io = new this._IO_COUNTERS();
> > +    this._GetProcessIoCounters(this._GetCurrentProcess(), io.address());
> 
> Always check the return value and handle errors!

Hm, this is the same behavior as in TelemetryPing.js. I'm not sure we have to do all this for this module, but if you think we really should then I will. I'm afraid the ctype parts of the code will become horrifying.

> 
> @@ +110,5 @@
> > +
> > +    let data = {
> > +      readBytes: parseInt(io.readBytes, 10),
> > +      writeBytes: parseInt(io.writeBytes, 10),
> > +      otherBytes: parseInt(io.otherBytes, 10)
> 
> Might as well capture all the fields in the struct, no?

Fixed.

> 
> @@ +112,5 @@
> > +      readBytes: parseInt(io.readBytes, 10),
> > +      writeBytes: parseInt(io.writeBytes, 10),
> > +      otherBytes: parseInt(io.otherBytes, 10)
> > +    };
> > +    data.total = data.readBytes + data.writeBytes + data.otherBytes;
> 
> data.totalBytes

Ok, will do. The point was to have a "total" field in all the platform-specific implementation so that we could have a common way to look at the data across platforms (i.e. the web viewer). I could change the names to totalBytes and totalTime, np.

> @@ +142,5 @@
> > +            parseInt(processedUserTime.wMilliseconds, 10) / 1000,
> > +      sys: parseInt(processedKernelTime.wMinute, 10) * 60 +
> > +           parseInt(processedKernelTime.wSecond, 10) +
> > +           parseInt(processedKernelTime.wMilliseconds, 10) / 1000
> > +    };
> 
> What about hours? I'd just perform math on the FILETIME struct. Something
> like:
> 
> let ticks = (kernelTime.dwHighDateTime << 32) + kernelTime.dwLowDateTime;
> let milliseconds = ticks / 10000;

From MDN: "The operands of all bitwise operators are converted to signed 32-bit integers "
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Bitwise_Operators

I don't think we need hours.. but I can add it, it's just a two line change.

> 
> @@ +157,5 @@
> > +
> > +
> > +let Linux = {
> > +  _setupCTypes: function () {
> > +    this._libc = ctypes.open('libc.so.6');
> 
> Can we just use libc.so? That should be symlinked to the latest libc on the
> system, no?

I remember having trouble, will check again, need a linux laptop handy. Basically you get libc.so.VER_NUMBER symlinked to your lib path.
http://stackoverflow.com/questions/6495817/why-glibc-binary-is-called-libc-so-6-not-a-libc-so-1-or-libc-so-4

> @@ +202,5 @@
> > +    let startPos = line.indexOf(")") + 2;
> > +    let values = line.substring(startPos).split(" ");
> > +    let data = {
> > +      user: (values[11] | 0) / _SC_CLK_TCK,
> > +      sys: (values[12] | 0) / _SC_CLK_TCK
> 
> Is there any chance the clk could be reported as 0? If so, we have a divide
> by 0 exception.

I haven't found any docs saying that it could be zero/subunitary.

> 
> ::: toolkit/modules/tests/xpcshell/test_ProcessInfo.js
> @@ +5,5 @@
> > +
> > +function run_test() {
> > +  dump(JSON.stringify(ProcessInfo.getCPUTimes()) + "\n");
> > +  dump(JSON.stringify(ProcessInfo.getMemoryUsage()) + "\n");
> > +  dump(JSON.stringify(ProcessInfo.getIOCounters()) + "\n");
> 
> Can you add some more intelligent tests? Maybe add a sleep() or some I/O and
> verify counters (when present) increment.

Absolutely, will do.

Comment 25

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 812613 [details] [diff] [review]
API for reading process info

Review of attachment 812613 [details] [diff] [review]:
-----------------------------------------------------------------

Bug 926990 strongly suggests that reading /proc/meminfo and should really be done on the main thread.
Consequently, I very strongly suggest an async API.

Comment 26

[Mihnea Dobrescu-Balaur (:mihneadb)]

I see your point, however I still think that only for benchmarking/monitoring reasons it would be fine sync. I do understand that we have no control over where the code would end up being used after it lands in the tree. :(

Comment 27

[David Teller [:Yoric] - still alive but not very active]

I believe that you have two possibilities:
- either pref-off this API (and pref it back on in the test suite); or
- make this API async.

Otherwise, we are going to end up with add-ons (or worse, front-end code) that misuses it.

Comment 28

[Gregory Szorc [:gps]]

Or we just make it a testing-only JSM that is only distributed in test archives.

Comment 29

[David Teller [:Yoric] - still alive but not very active]

That works for me, too.
Whichever of the three you guys prefer.

Comment 30

[Mihnea Dobrescu-Balaur (:mihneadb)]

(In reply to Gregory Szorc [:gps] from comment #28)
> Or we just make it a testing-only JSM that is only distributed in test
> archives.

How do we do achieve this?

Comment 31

[Gregory Szorc [:gps]]

See TESTING_JS_MODULES in services/healthreport/Makefile.in.

Comment 32

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.