Closed Bug 1015497 Opened 5 years ago Closed 5 years ago

Enable triggering memory report via fifo device for every Unix system

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32
Tracking Status
firefox31 --- fixed
firefox32 --- fixed

People

(Reporter: jbeich, Assigned: jbeich)

References

Details

(Whiteboard: [MemShrink:P3][qa-])

Attachments

(2 files, 3 obsolete files)

Attached patch enable (obsolete) — 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 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 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
wait for v2
Keywords: checkin-needed
Attached patch enable v2 (obsolete) — Splinter Review
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 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+
(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 ?
Attached patch enable v2 (obsolete) — Splinter Review
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
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 ?
(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.
Whiteboard: [MemShrink] → [MemShrink:P3]
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)
Mostly offline until the 10, wont be able to do debugging
Flags: needinfo?(landry)
Attached patch enable v2.1Splinter Review
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 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
Attached patch esr31 versionSplinter Review
bug 798033 caused conflict in xpcom/base/nsDumpUtils.h
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?
https://hg.mozilla.org/mozilla-central/rev/ac248c8f6861
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Attachment #8431341 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [MemShrink:P3] → [MemShrink:P3][qa-]
You need to log in before you can comment on or make changes to this bug.