Closed
Bug 1015497
Opened 10 years ago
Closed 10 years ago
Enable triggering memory report via fifo device for every Unix system
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla32
People
(Reporter: jbeich, Assigned: jbeich)
References
Details
(Whiteboard: [MemShrink:P3][qa-])
Attachments
(2 files, 3 obsolete files)
5.33 KB,
patch
|
jld
:
review+
|
Details | Diff | Splinter Review |
5.35 KB,
patch
|
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
+++ This bug was initially created as a clone of Bug #1000461 +++ mkfifo(2) is defined in POSIX.1 or later. A few more XP_UNIX platforms can benefit: Solaris, DragonFly, NetBSD, OpenBSD.
Attachment #8428153 -
Flags: review?(nfroyd)
Attachment #8428153 -
Flags: feedback?(landry)
Comment 1•10 years ago
|
||
Comment on attachment 8428153 [details] [diff] [review] enable This build and runs for me on OpenBSD, but how can one actually test it at runtime ?
Attachment #8428153 -
Flags: feedback?(landry) → feedback+
Comment 2•10 years ago
|
||
Comment on attachment 8428153 [details] [diff] [review] enable Review of attachment 8428153 [details] [diff] [review]: ----------------------------------------------------------------- WFM. Are you able to test on, say, Solaris and NetBSD (maybe OpenBSD is a sufficient proxy for NetBSD), or are we just relying on standardization here?
Attachment #8428153 -
Flags: review?(nfroyd) → review+
(In reply to Landry Breuil (:gaston) from comment #1) > This build and runs for me on OpenBSD, but how can one actually test it at > runtime ? 1/ toggle memory_info_dumper.watch_fifo.enabled 2/ echo "memory report" >/tmp/debug_info_trigger 3/ notice new /tmp/unified-memory-*.json.gz (In reply to Nathan Froyd (:froydnj) from comment #2) > Comment on attachment 8428153 [details] [diff] [review] > WFM. Are you able to test on, say, Solaris and NetBSD (maybe OpenBSD is a > sufficient proxy for NetBSD), or are we just relying on standardization here? Standards after looking at the quirks in gnulib. http://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=blob_plain;f=m4/mkfifo.m4
Keywords: checkin-needed
Keywords: checkin-needed
I've missed "status report" code. It uses FifoWatcher but bug 1000461 forgot to update the ifdef. Either XP_UNIX or MOZ_WIDGET_GONK (in a new bug). commit message updated accordingly
Attachment #8428153 -
Attachment is obsolete: true
Attachment #8428506 -
Flags: review?(alchen)
Comment 6•10 years ago
|
||
Comment on attachment 8428506 [details] [diff] [review] enable v2 Review of attachment 8428506 [details] [diff] [review]: ----------------------------------------------------------------- I'm fine with this changes. Is try server result available?
Attachment #8428506 -
Flags: review?(alchen) → review+
Comment 7•10 years ago
|
||
(In reply to Jan Beich from comment #3) > (In reply to Landry Breuil (:gaston) from comment #1) > > This build and runs for me on OpenBSD, but how can one actually test it at > > runtime ? > > 1/ toggle memory_info_dumper.watch_fifo.enabled > 2/ echo "memory report" >/tmp/debug_info_trigger > 3/ notice new /tmp/unified-memory-*.json.gz Doesnt seem to work here even after a restart, no socket/fifo - is firefox itself supposed to create the fifo if memory_info_dumper.watch_fifo.enabled is true ?
Cosmetic fix to commit message for better wording. Carrying over r+ from :froydnj and :Alphan. Landry, can you push to Try as build-only ? At least OS X and Windows because of a style change in nsStatusReporterManager.cpp. (In reply to Landry Breuil (:gaston) from comment #7) > (In reply to Jan Beich from comment #3) > > (In reply to Landry Breuil (:gaston) from comment #1) > > > This build and runs for me on OpenBSD, but how can one actually test it at > > > runtime ? > > > > 1/ toggle memory_info_dumper.watch_fifo.enabled > > 2/ echo "memory report" >/tmp/debug_info_trigger > > 3/ notice new /tmp/unified-memory-*.json.gz > > Doesnt seem to work here even after a restart, no socket/fifo - is firefox > itself supposed to create the fifo if memory_info_dumper.watch_fifo.enabled > is true ? mkfifo(2) call in firefox process creates the fifo immediately after the pref change (unless bug 1016078). The fifo is not removed upon shutdown. Adding the following line to xpcom/base/nsDumpUtils.h may help to diagnose your issue. #define LOG(...) printf(__VA_ARGS__); printf("\n") On my box with 1/ and 2/ it prints Fifo watcher disabled via pref. memory_info_dumper.watch_fifo.enabled changed FifoWatcher::OpenFifo unlink failed; errno=2. Continuing despite error. FifoWatcher(command:memory report) dispatching memory report runnable. FifoWatcher closing and re-opening fifo. but "status report" only adds Got unexpected value from fifo; ignoring it. FifoWatcher closing and re-opening fifo.
Attachment #8428506 -
Attachment is obsolete: true
Comment 9•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=6f840f4a8a87
Comment 10•10 years ago
|
||
Is there a reason for keeping #define MOZ_SUPPORTS_RT_SIGNALS 1 only in #if defined(XP_LINUX) || defined(__FreeBSD__), and could it be the reason i failed to see it on openbsd ?
Comment 11•10 years ago
|
||
(In reply to Landry Breuil (:gaston) from comment #10) > Is there a reason for keeping #define MOZ_SUPPORTS_RT_SIGNALS 1 only in #if > defined(XP_LINUX) || defined(__FreeBSD__), and could it be the reason i > failed to see it on openbsd ? |MOZ_SUPPORTS_RT_SIGNALS| refers to using real-time signals to trigger memory reports, it shouldn't affect FIFO behavior. For openbsd it's possible we're not actually writing to /tmp, in |FifoWatcher::OpenFd()| in nsDumpUtils.cpp you can set a breakpoint/add a printf to dump the actual file name. Also, Jan, nice improvement! Good catch on updating the status report code as well.
Updated•10 years ago
|
Whiteboard: [MemShrink] → [MemShrink:P3]
Assignee | ||
Comment 12•10 years ago
|
||
Pending Landry to do some debugging (comment 8 and comment 11). Otherwise, the patch can land regardless because it's unlikely to break build and even exposes more code for testing.
Flags: needinfo?(landry)
Comment 13•10 years ago
|
||
Mostly offline until the 10, wont be able to do debugging
Flags: needinfo?(landry)
Assignee | ||
Comment 14•10 years ago
|
||
The following looks wrong even without the patch. Do you think nsMemoryInfoDumper.h should be included for MOZ_DMD, too? xpcom/base/nsMemoryReporterManager.cpp: #if defined(XP_LINUX) || defined(__FreeBSD__) || defined(XP_MACOSX) #include "nsMemoryInfoDumper.h" #endif ... #ifdef MOZ_DMD if (!aDMDDumpIdent.IsEmpty()) { return nsMemoryInfoDumper::DumpDMD(aDMDDumpIdent); } #endif
Attachment #8428895 -
Attachment is obsolete: true
Attachment #8430755 -
Flags: review?(jld)
Comment 15•10 years ago
|
||
Comment on attachment 8430755 [details] [diff] [review] enable v2.1 Review of attachment 8430755 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/base/nsMemoryReporterManager.cpp @@ +17,5 @@ > #include "nsPIDOMWindow.h" > #include "nsIObserverService.h" > #include "nsIGlobalObject.h" > #include "nsIXPConnect.h" > +#if defined(XP_UNIX) || defined(MOZ_DMD) Thanks for fixing this. I think the only reason this ever worked (after bug 946407) without the MOZ_DMD test is because unified builds wind up concatenating this file with nsMemoryInfoDumper.cpp and thereby pulling in its include of nsMemoryInfoDumper.h, and nobody does non-unified DMD builds. Note that Windows DMD is currently also broken for other reasons (bug 1017911).
Attachment #8430755 -
Flags: review?(jld) → review+
Keywords: checkin-needed
Assignee | ||
Comment 16•10 years ago
|
||
bug 798033 caused conflict in xpcom/base/nsDumpUtils.h
Assignee | ||
Comment 17•10 years ago
|
||
Comment on attachment 8431341 [details] [diff] [review] esr31 version Mostly to benefit downstream by chasing bug 1000461 in esr31. [Approval Request Comment] Bug caused by (feature/regressing bug #): none User impact if declined: not directly, broken --enable-dmd on Windows and disabled "status report" fifo command on OS X Testing completed (on m-c, etc.): soon Risk to taking this patch (and alternatives if risky): At most, broken build. String or IDL/UUID changes made by this patch: None
Attachment #8431341 -
Flags: approval-mozilla-aurora?
Comment 18•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac248c8f6861
Assignee: nobody → jbeich
Keywords: checkin-needed
Comment 19•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ac248c8f6861
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Updated•10 years ago
|
status-firefox31:
--- → affected
status-firefox32:
--- → fixed
Updated•10 years ago
|
Attachment #8431341 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in
before you can comment on or make changes to this bug.
Description
•