Closed
Bug 999473
Opened 11 years ago
Closed 11 years ago
report number of open file descriptions (fd) in about:memory
Categories
(Toolkit :: about:memory, defect)
Tracking
()
RESOLVED
FIXED
mozilla32
People
(Reporter: bkelly, Assigned: erahm)
References
Details
(Whiteboard: [MemShrink:P2])
Attachments
(1 file, 1 obsolete file)
4.27 KB,
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•11 years ago
|
||
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
Updated•11 years ago
|
Assignee: nobody → erahm
Whiteboard: [MemShrink] → [MemShrink:P2]
Comment 2•11 years ago
|
||
erahm and I discussed this, and listing file paths seems very useful.
Assignee | ||
Updated•11 years ago
|
OS: All → Linux
Assignee | ||
Comment 3•11 years ago
|
||
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
Assignee | ||
Comment 4•11 years ago
|
||
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 5•11 years ago
|
||
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+
Comment 6•11 years ago
|
||
Or the "System" section could just represent "everything we get from /proc", in which case your patch is fine... I'm undecided.
Assignee | ||
Comment 7•11 years ago
|
||
(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"
Reporter | ||
Comment 8•11 years ago
|
||
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 9•11 years ago
|
||
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+
Assignee | ||
Comment 10•11 years ago
|
||
try looks happy: https://tbpl.mozilla.org/?tree=Try&rev=28e98fe92d4e
Assignee | ||
Comment 11•11 years ago
|
||
Comment 12•11 years ago
|
||
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
Comment 13•11 years ago
|
||
> 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.
Assignee | ||
Comment 14•11 years ago
|
||
Unfortunately that's exactly what happened, although actually looking at that bug I should have realized it had nothing to do with linux.
Assignee | ||
Comment 15•11 years ago
|
||
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)
Assignee | ||
Updated•11 years ago
|
Attachment #8414115 -
Attachment is obsolete: true
Assignee | ||
Comment 16•11 years ago
|
||
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 17•11 years ago
|
||
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+
Assignee | ||
Comment 18•11 years ago
|
||
try is actually happy: https://tbpl.mozilla.org/?tree=Try&rev=5a06b0150a73
Assignee | ||
Comment 19•11 years ago
|
||
Comment 20•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in
before you can comment on or make changes to this bug.
Description
•