report number of open file descriptions (fd) in about:memory

RESOLVED FIXED in mozilla32

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bkelly, Assigned: erahm)

Tracking

unspecified
mozilla32
All
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(1 attachment, 1 obsolete attachment)

We seem to be leaking file descriptors in bug 998504.  To help catch this sort of thing faster in the future it would be nice to report open fd's in about:memory.

As a side effect, this would also provide an easier way to instrument the current open fd's in code as well.
How detailed do we want to go?
  1) Just FD count
  2) List all open FDs
  3) List all open FDs and the file path

On linux/b2g any of these should be easy to accomplish by looking at |/proc/<pid>/fd|

We could do something slightly more clever which would group by path (so ashmem handles, pmem handles, etc would be grouped), as follows:
  /open_fds/dev/ashmem/<fd>
  /open_fds/dev/pmem/<fd>

And then when looking at a report we'd get something like:
1,591 (100.0%) -- open_fds
└──1,591 (100.0%) -- dev
   ├──1,323 (83.16%) ── ashmem
   └────268 (16.84%) -- pmem
Assignee: nobody → erahm
Whiteboard: [MemShrink] → [MemShrink:P2]
erahm and I discussed this, and listing file paths seems very useful.
OS: All → Linux
This patch adds support for reporting the open file descriptors for each process on the system.

It breaks out the file descriptors into 4 categories:
- files
- sockets
- pipes
- anon_inodes

Output looks similar to this:

473 (100.0%) -- open_fds
├──147 (31.08%) -- process(/system/b2g/b2g, pid=2755)
│  ├───92 (19.45%) -- files
│  │   ├──65 (13.74%) -- dev
│  │   │  ├──18 (03.81%) -- ashmem
│  │   │  │  ├───1 (00.21%) ── 103
│  │   │  │  ├───1 (00.21%) ── 104
│  │   │  ├───9 (01.90%) -- pmem
│  │   │  │   ├──1 (00.21%) ── 109
│  │   │  │   ├──1 (00.21%) ── 110
│  │   │  ├───8 (01.69%) -- genlock
│  │   │  │   ├──1 (00.21%) ── 111
│  │   │  │   ├──1 (00.21%) ── 124
│  │   │  ├───8 (01.69%) -- log
│  │   │  ├───5 (01.06%) -- graphics/fb0
│  │   ├──14 (02.96%) ++ data
│  │   ├───6 (01.27%) -- system/fonts
│  │   │   ├──1 (00.21%) ── FiraSansOT-Light.otf/86
│  │   │   ├──1 (00.21%) ── FiraSansOT-Medium.otf/81
│  │   │   ├──1 (00.21%) ── FiraSansOT-MediumItalic.otf/116
│  │   │   ├──1 (00.21%) ── FiraSansOT-Regular.otf/74
│  │   │   ├──1 (00.21%) ── FiraSansOT-RegularItalic.otf/87
│  │   │   └──1 (00.21%) ── MTLmr3m.ttf/107
│  ├───25 (05.29%) -- sockets
│  │   ├───1 (00.21%) ── socket:[15885]/27
│  │   ├───1 (00.21%) ── socket:[15886]/28
│  ├───16 (03.38%) ++ anon_inodes
│  └───14 (02.96%) -- pipes
│      ├───2 (00.42%) -- pipe:[15887]
│      │   ├──1 (00.21%) ── 29
│      │   └──1 (00.21%) ── 30
Comment on attachment 8414115 [details] [diff] [review]
report number of open file descriptions (fd) in about:memory

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

Nick can you take a look? Ben can you let me know if this format is what you were hoping for?

::: xpcom/base/SystemMemoryReporter.cpp
@@ +275,5 @@
>              break;
>          }
>          fclose(f);
> +
> +        // Report the open file descriptors for this process.

In case it's not clear, this is in the existing process enumeration function.

@@ +743,5 @@
> +    // anon_inode, or possibly an uncategorized device.
> +    const char kFilePrefix[] = "/";
> +    const char kSocketPrefix[] = "socket:";
> +    const char kPipePrefix[] = "pipe:";
> +    const char kAnonInodePrefix[] = "anon_inode:";

I'm open to not categorizing / changing the names. If we don't categorize there will be a lot of socket/pipe/anon_inode entries seemingly in the root of the file system.

@@ +800,5 @@
> +            "open_fds/%s/%s%s/%s", processName.get(), category, linkPath, fd);
> +        nsPrintfCString entryDescription(
> +            "%s file descriptor opened by the process", descriptionPrefix);
> +        REPORT_WITH_CLEANUP(
> +            entryPath, UNITS_COUNT, 1, entryDescription, closedir(d));

I'm not totally sure this is the best unit/value, but it's the only one that really makes sense.
Attachment #8414115 - Flags: review?(n.nethercote)
Attachment #8414115 - Flags: feedback?(bkelly)
Comment on attachment 8414115 [details] [diff] [review]
report number of open file descriptions (fd) in about:memory

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

This looks good except for one thing: I was imagining that the open fds for each process would be reported in the process's section in about:memory, not in the "System" section. The system memory reporter does multi-process reporting because for the PSS values that's pretty much the only way that makes sense -- we have a fixed amount of memory and want to indicate how much each process is taking up and how much is left over. But fds don't have that property. Also, we want to report PSS values for non-Gecko processes, but I don't think we want/need that for fds.

But I see why you did it this way -- SystemMemoryReporter already iterates over /proc, so it fits here nicely. How hard it would be to put this new code into a new OpenFdReporter class and register one per Gecko process? If that's too hard, maybe you could keep this structure but just change the |process| and |path| args when calling the report callback... that might require filtering out fds for non-Gecko processes, though.

::: xpcom/base/SystemMemoryReporter.cpp
@@ +796,5 @@
> +#undef CHECK_PREFIX
> +
> +        const nsCString processName(aProcessName);
> +        nsPrintfCString entryPath(
> +            "open_fds/%s/%s%s/%s", processName.get(), category, linkPath, fd);

Nit: use "open-fds" for consistency with other reporter paths.
Attachment #8414115 - Flags: review?(n.nethercote) → feedback+
Or the "System" section could just represent "everything we get from /proc", in which case your patch is fine... I'm undecided.
(In reply to Nicholas Nethercote [:njn] from comment #6)
> Or the "System" section could just represent "everything we get from /proc",
> in which case your patch is fine... I'm undecided.

Both sides have merit. On non-B2G it makes sense to not report for all processes. On B2G it probably is useful. Considering this request came from working on a B2G bug, I'm leaning towards going with System representing "everything we get from /proc"
Comment on attachment 8414115 [details] [diff] [review]
report number of open file descriptions (fd) in about:memory

I did not look at the code, but the output format looks great!

I do think having the fd's open for each parent/child process is important.
Attachment #8414115 - Flags: feedback?(bkelly) → feedback+
Comment on attachment 8414115 [details] [diff] [review]
report number of open file descriptions (fd) in about:memory

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

Ok, dumping it in "System" seems reasonable.
Attachment #8414115 - Flags: feedback+ → review+
This was the patch responsible for the test failures that bug 1001419 got backed out for. *sigh*
https://hg.mozilla.org/integration/mozilla-inbound/rev/977cf6a69034
> try looks happy: https://tbpl.mozilla.org/?tree=Try&rev=28e98fe92d4e

Nope :(

You may have been mislead by the orange matching -- the failure was this:

> 3401 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/toolkit/components/aboutmemory/tests/test_aboutmemory5.xul | system-wide reporter

and it suggested this as a match:

> Bug 937078 - Intermittent aboutmemory/tests/test_aboutmemory5.xul | Assertion count 1 is greater than expected range 0-0 assertions. Due to negative "gfx-quartz-surface" value.

Same test, but different failure.
Unfortunately that's exactly what happened, although actually looking at that bug I should have realized it had nothing to do with linux.
This fixes errors on plain-old-linux (b2g was fine). We need to make sure we can actually access the /proc/pid/fd directory and ignore it if we can't.
Attachment #8416229 - Flags: review?(n.nethercote)
Attachment #8414115 - Attachment is obsolete: true
Comment on attachment 8416229 [details] [diff] [review]
report number of open file descriptions (fd) in about:memory.

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

::: xpcom/base/SystemMemoryReporter.cpp
@@ +748,5 @@
> +
> +    const nsCString procPath(aProcPath);
> +    DIR* d = opendir(procPath.get());
> +    if (!d) {
> +      if (NS_WARN_IF(errno != ENOENT && errno != EACCES)) {

"&& errno != EACCES" is the only change here.
Comment on attachment 8416229 [details] [diff] [review]
report number of open file descriptions (fd) in about:memory.

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

FWIW, I probably wouldn't have requested re-review for a fix this small and obvious. But no harm in doing so, either :)
Attachment #8416229 - Flags: review?(n.nethercote) → review+
https://hg.mozilla.org/mozilla-central/rev/5fec1b48b68d
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.