1015497 - Enable triggering memory report via fifo device for every Unix system

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Jan Beich]

Attachment: enable

+++ 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.

Comment 1

[Landry Breuil (:gaston)]

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 ?

Comment 2

[Nathan Froyd [:froydnj]]

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?

Comment 3

[Jan Beich]

(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

Comment 4

[Jan Beich]

wait for v2

Comment 5

[Jan Beich]

Attachment: enable v2

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

Comment 6

[Alphan Chen [: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?

Comment 7

[Landry Breuil (:gaston)]

(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 ?

Comment 8

[Jan Beich]

Attachment: enable v2

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.

Comment 9

[Landry Breuil (:gaston)]

https://tbpl.mozilla.org/?tree=Try&rev=6f840f4a8a87

Comment 10

[Landry Breuil (:gaston)]

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

[Eric Rahm [:erahm]]

(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.

Comment 12

[Jan Beich]

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.

Comment 13

[Landry Breuil (:gaston)]

Mostly offline until the 10, wont be able to do debugging

Comment 14

[Jan Beich]

Attachment: enable v2.1

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

Comment 15

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

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).

Comment 16

[Jan Beich]

Attachment: esr31 version

bug 798033 caused conflict in xpcom/base/nsDumpUtils.h

Comment 17

[Jan Beich]

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

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ac248c8f6861

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ac248c8f6861

Comment 20

[Ryan VanderMeulen (PTO, back 6-April)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/719c6742188c