876029 - Gonk memory pressure watcher should use the ScheduleMemoryPressureEvent() instead of NS_DispatchToMainThread(…).

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Gonk, defect)

Description

[Nicolas B. Pierron [:nbp]]

Currently the MemoryPressureWatcher implemented in widget/gonk is using the Dispatch function to send events to other threads by queuing the event into the event list.  This means that nsThread::ProcessNextEvent will not see the memory pressure until it completes the execution of all events queued before the memory pressure event.

ScheduleMemoryPressureEvent() has a privileged place in nsThread::ProcessNextEvent as it provides a way to notify all observers watching for memory pressure events before any other event get processed.

Switching the the MemoryPressureWatcher of Gonk to use the ScheduleMemoryPressureEvent is a logical goal before trying to do any fast-notification during the execution of an event (Bug 869263, for doing that for JavaScript).

Comment 1

[Nicolas B. Pierron [:nbp]]

Attachment: Use ScheduleMemoryPressureEvent to bypass all events in the event loop.

Modifications:
 - Add a 0,1,2 states to the atomic variable in order to keep track if the memory pressure is ongoing or if this is a new one.
 - Move ScheduleMemoryPressureEvent to the idl file.
 - Replace the Dispatch function of GonkMemoryPressureMonitoring by NS_DispatchMemoryPressure, which is the side effect of avoiding allocations when we have a memory pressure event :)

Comment 2

[Justin Lebar (not reading bugmail)]

This is a good start!

>diff --git a/xpcom/threads/nsIThread.idl b/xpcom/threads/nsIThread.idl
>index feeae69..49ab595 100644
>--- a/xpcom/threads/nsIThread.idl
>+++ b/xpcom/threads/nsIThread.idl

>+%{C++
>+
>+/**
>+ * This function causes the main thread to fire a memory pressure event at its
>+ * next available opportunity.
>+ *
>+ * @param reminder
>+ *   The flag notes if the memory pressure is a reminder or a new one.
>+ *
>+ * You may call this function from any thread.
>+ */
>+void ScheduleMemoryPressureEvent(bool reminder = false);

It seems better to make this a static method on nsThread instead of making it a
global method.  In any case we don't put global C++ functions in IDL files like
this; it should go in nsThread.h.

Instead of "reminder", we've been saying "ongoing".  I think that's clearer.

As an alternative to taking a boolean argument, you could have two functions:

  ScheduleMemoryPressureEvent();
  ScheduleRepeatMemoryPressureEvent();

(I didn't use "ongoing" here because "ScheduleOngoingMemoryPressureEvent"
sounds like it's going to schedule multiple events.  We can figure out the
exact names once we settle on the API.)

If you do keep the function with the boolean arg, please comment all callsites, like

  ScheduleMemoryPressureEvent(/* ongoing */ true);

See http://jlebar.com/2011/12/16/Boolean_parameters_to_API_functions_considered_harmful..html.

>diff --git a/widget/gonk/GonkMemoryPressureMonitoring.cpp b/widget/gonk/GonkMemoryPressureMonitoring.cpp
>index 133b98e..003d2f8 100644
>--- a/widget/gonk/GonkMemoryPressureMonitoring.cpp
>+++ b/widget/gonk/GonkMemoryPressureMonitoring.cpp

>       // POLLPRI on lowMemFd indicates that we're in a low-memory situation.  We
>       // could read lowMemFd to double-check, but we've observed that the read
>       // sometimes completes after the memory-pressure event is over, so let's
>       // just believe the result of poll().
> 
>       // We use low-memory-no-forward because each process has its own watcher
>       // and thus there is no need for the main process to forward this event.
>-      Dispatch("memory-pressure",
>-               NS_LITERAL_STRING("low-memory-no-forward").get());
>+      NS_DispatchMemoryPressure(false);

This comment should now say that NS_DispatchMemoryPressure uses
low-memory-no-forward.

>diff --git a/xpcom/glue/nsThreadUtils.h b/xpcom/glue/nsThreadUtils.h
>index d28c6dc..4d7e9a9 100644
>--- a/xpcom/glue/nsThreadUtils.h
>+++ b/xpcom/glue/nsThreadUtils.h

>@@ -135,16 +135,25 @@ NS_DispatchToCurrentThread(nsIRunnable *event);
>  *
>  * @returns NS_ERROR_INVALID_ARG
>  *   If event is null.
>  */
> extern NS_COM_GLUE NS_METHOD
> NS_DispatchToMainThread(nsIRunnable *event,
>                         uint32_t dispatchFlags = NS_DISPATCH_NORMAL);
> 
>+/**
>+ * Dispatch a memory pressure event to the main thread.
>+ *
>+ * @param reminder
>+ *   The flag notes if the memory pressure is a reminder or a new one.
>+ */
>+extern NS_COM_GLUE void
>+NS_DispatchMemoryPressure(bool reminder);

Please add a comment noting that this can be called from any thread, and noting
exactly which observer notification is fired (no-forward, ongoing, etc).

Why do we need this wrapper function at all; why can't we continue using
mozilla::ScheduleMemoryPressureEvent directly?

>diff --git a/xpcom/threads/nsThread.cpp b/xpcom/threads/nsThread.cpp
>index 702a0df..9d51317 100644
>--- a/xpcom/threads/nsThread.cpp
>+++ b/xpcom/threads/nsThread.cpp

> /*
>  * It's important that this function not acquire any locks, nor do anything
>  * which might cause malloc to run.
>  */
>-void ScheduleMemoryPressureEvent()
>+void ScheduleMemoryPressureEvent(bool reminder)
> {
>-  PR_ATOMIC_SET(&sMemoryPressurePending, 1);
>+  PR_ATOMIC_SET(&sMemoryPressurePending, (reminder ? 1 : 2));
> }

If one thread calls ScheduleMemoryPressureEvent(false) and another thread calls
ScheduleMemoryPressureEvent(true), we'll do last writer wins.  But it seems
like in that case we actually want to fire a non-reminder event.  Or maybe you
could argue it should be a reminder event.  In any case it should not be last
writer wins, I think.

The atomic variables in mfbt/Atomics.h now have a compare-and-swap method which
could help with this.

> class nsThreadClassInfo : public nsIClassInfo {
> public:
>   NS_DECL_ISUPPORTS_INHERITED  // no mRefCnt
>   NS_DECL_NSICLASSINFO

>@@ -574,19 +579,27 @@ nsThread::ProcessNextEvent(bool mayWait, bool *result)
>     HangMonitor::Suspend();
> 
>   // Fire a memory pressure notification, if we're the main thread and one is
>   // pending.
>   if (MAIN_THREAD == mIsMainThread && !ShuttingDown()) {
>     bool mpPending = PR_ATOMIC_SET(&sMemoryPressurePending, 0);
>     if (mpPending) {
>       nsCOMPtr<nsIObserverService> os = services::GetObserverService();
>+
>+      // no-forward: Implies that the message will no be transfered to the
>+      // parent process. To be reactive, each process should have its own memory
>+      // watcher.

s/parent/child/.  Child processes don't send low-memory notifications up to the
parent.

The second sentence doesn't make much sense: nsThread doesn't know about
GonkMemoryPressureMonitoring.

Also please write the first sentence as a sentence; that is, say "no-forward
implies" or something instead of "no-forward: Implies".

Comment 3

[Justin Lebar (not reading bugmail)]

I just remembered that there's a bug in the code in the tree (and also the code in this patch): If we send a memory pressure event while the nsThread event loop is quiescent, we'll never receive the memory pressure event, because we only check the variable when we go around the event loop.

I think the right thing to do is just to enqueue a dummy event right after you set the variable.

Comment 4

[Nicolas B. Pierron [:nbp]]

(In reply to Justin Lebar [:jlebar] from comment #3)
> I just remembered that there's a bug in the code in the tree (and also the
> code in this patch): If we send a memory pressure event while the nsThread
> event loop is quiescent, we'll never receive the memory pressure event,
> because we only check the variable when we go around the event loop.
> 
> I think the right thing to do is just to enqueue a dummy event right after
> you set the variable.

I tested it by looking at the logcat at the same time.  All applications are getting the events even the background one and usually these events are processed in less than a second.

Still, I can reproduce what you describe by running the template application in the background, in which case no memory pressure GC happen after the event, but only when the application is resumed.

I will queue a dummy event after setting the flag.

Comment 5

[Justin Lebar (not reading bugmail)]

> I will queue a dummy event after setting the flag.

Unfortunately we can't do this unconditionally, because the Windows code that's calling this expects this function not to allocate.  So we need different functions or a flag or something.

Comment 6

[Nicolas B. Pierron [:nbp]]

(In reply to Justin Lebar [:jlebar] from comment #5)
> > I will queue a dummy event after setting the flag.
> 
> Unfortunately we can't do this unconditionally, because the Windows code
> that's calling this expects this function not to allocate.  So we need
> different functions or a flag or something.

As the event is just use to wake up the main thread, would it be safe to have either a static event or an event which is artificially (ref-count +1) kept alive on the main thread?

Comment 7

[Justin Lebar (not reading bugmail)]

I'd need to look more closely, but that would probably work!

Comment 8

[Justin Lebar (not reading bugmail)]

Erm, except merely enqueueing an event may allocate, right?

Comment 9

[Nicolas B. Pierron [:nbp]]

(In reply to Justin Lebar [:jlebar] from comment #7)
> I'd need to look more closely, but that would probably work!
> Erm, except merely enqueueing an event may allocate, right?

Right, the event queue might allocate a new page to store the new event.  It seems like there is small probability that we end-up with an empty queue which has no allocated space.

In which case we can make add a MaybePutEvent which is infallible, and which can be use to put the pre-allocated WakeUp event.

Comment 10

[Nicolas B. Pierron [:nbp]]

(In reply to Justin Lebar [:jlebar] from comment #2)
> >diff --git a/xpcom/glue/nsThreadUtils.h b/xpcom/glue/nsThreadUtils.h
> >index d28c6dc..4d7e9a9 100644
> >--- a/xpcom/glue/nsThreadUtils.h
> >+++ b/xpcom/glue/nsThreadUtils.h
> 
> >@@ -135,16 +135,25 @@ NS_DispatchToCurrentThread(nsIRunnable *event);
> >  *
> >  * @returns NS_ERROR_INVALID_ARG
> >  *   If event is null.
> >  */
> > extern NS_COM_GLUE NS_METHOD
> > NS_DispatchToMainThread(nsIRunnable *event,
> >                         uint32_t dispatchFlags = NS_DISPATCH_NORMAL);
> > 
> >+/**
> >+ * Dispatch a memory pressure event to the main thread.
> >+ *
> >+ * @param reminder
> >+ *   The flag notes if the memory pressure is a reminder or a new one.
> >+ */
> >+extern NS_COM_GLUE void
> >+NS_DispatchMemoryPressure(bool reminder);
> 
> Please add a comment noting that this can be called from any thread, and
> noting
> exactly which observer notification is fired (no-forward, ongoing, etc).
> 
> Why do we need this wrapper function at all; why can't we continue using
> mozilla::ScheduleMemoryPressureEvent directly?

Because this implies including xpcom/base/nsThread.h, I don't know if this would be the right way to go, but last time I had to touch any include list in Gecko, I had to patch 15 Makefiles.

When building b2g, I don't think that nsThread.h is provided in the list of included directories for building GonkMem….h

> >diff --git a/xpcom/threads/nsThread.cpp b/xpcom/threads/nsThread.cpp
> >index 702a0df..9d51317 100644
> >--- a/xpcom/threads/nsThread.cpp
> >+++ b/xpcom/threads/nsThread.cpp
> 
> > /*
> >  * It's important that this function not acquire any locks, nor do anything
> >  * which might cause malloc to run.
> >  */
> >-void ScheduleMemoryPressureEvent()
> >+void ScheduleMemoryPressureEvent(bool reminder)
> > {
> >-  PR_ATOMIC_SET(&sMemoryPressurePending, 1);
> >+  PR_ATOMIC_SET(&sMemoryPressurePending, (reminder ? 1 : 2));
> > }
> 
> If one thread calls ScheduleMemoryPressureEvent(false) and another thread
> calls
> ScheduleMemoryPressureEvent(true), we'll do last writer wins.  But it seems
> like in that case we actually want to fire a non-reminder event.  Or maybe
> you
> could argue it should be a reminder event.  In any case it should not be last
> writer wins, I think.
> 
> The atomic variables in mfbt/Atomics.h now have a compare-and-swap method
> which
> could help with this.

Ok, good to know.  Is there any use case where we can have multiple memory pressure?

We have optimizations to prevent ongoing memory pressure to trigger more CC, so this would imply that we should go in favor of giving a higher priority to ongoing memory pressure.  On the other hand, if there is a hang, an ongoing memory pressure can erase the first memory pressure event and cause us to avoid doing any CC at all.  So I think the best solution would be to give a higher priority to new memory pressure event.

Comment 11

[Justin Lebar (not reading bugmail)]

> Because this implies including xpcom/base/nsThread.h, I don't know if this would be the 
> right way to go, but last time I had to touch any include list in Gecko, I had to patch 
> 15 Makefiles.

We prefer cleanliness of code over the cleanliness of Makefiles.

btw I'm still not sure that you're going to be able to do the dummy event-enqueueing thing in a signal-safe way (which is necessary for the window low-memory monitoring).  I'd be OK with two separate paths (separate functions, or the same function with a bool arg, or whatever), one of which was signal safe and the other which was not and enqueued a dummy event.

In the Windows case, if we're low on memory, it's OK if we don't free anything until we start running again.  But in the B2G case we really do want to throw away memory as quickly as possible, even if we're asleep (which is quite possible).

Comment 12

[Justin Lebar (not reading bugmail)]

btw, nsThread.h is included in dom/camera and image/src, and it's listed in xpcom/threads/moz.build's EXPORTS variable.  So you probably can include it with no fuss from any file in the tree.

Comment 13

[Nicolas B. Pierron [:nbp]]

(In reply to Justin Lebar [:jlebar] from comment #11)
> In the Windows case, if we're low on memory, it's OK if we don't free
> anything until we start running again.  But in the B2G case we really do
> want to throw away memory as quickly as possible, even if we're asleep
> (which is quite possible).

That's what I was aiming for lately, to let Windows use the old interface for now.

Comment 14

[Nicolas B. Pierron [:nbp]]

I am totally lost, is there any good documentation explaining the linking of any C++ in gecko.  I tried to follow what was already in place, but apparently this does not work that well.

I did not added an include of nsThread.h, because widget/gonk/ is already using nsThreadUtils.h, and as I understand it we are supposed to use the glue and not something else out-side of xpcom … but as I am getting more and more compilation/linking issues I feel more and more lost with these modifications.

In addition, patches which are linking fine while building b2g, are not working at all while building on Linux.  Why do we have multiple linking strategies? where are these boundaries defined?

Worse, sometimes I feel I understand something until I notice that the only definition I can find is not enabled, such as nsRunnable which is conditionally defined in nsThreadUtils.h, but only if some preprocessing is defined.  I don't understand why I cannot add anything in nsThreadUtils which use an nsRunnable …

Lately, I managed to get:

arm-linux-androideabi/bin/ld: error: B2G/gecko/objdir-gecko/toolkit/library/../../js/xpconnect/loader/mozJSComponentLoader.o: multiple definition of 'mozJSComponentLoader::ModuleEntry::GetFactory(mozilla::Module const&, mozilla::Module::CIDEntry const&)'
arm-linux-androideabi/bin/ld: B2G/gecko/objdir-gecko/toolkit/library/../../js/xpconnect/loader/mozJSComponentLoader.o: previous definition here

which leave me with no clue why this .o file is included twice …

So as I have trouble getting stuff working inside the current headers you can understand that I am hardly willing to add a new file in the build system if this is to introduce new layers when I don't understand the current one.

Comment 15

[Nicolas B. Pierron [:nbp]]

(In reply to Nicolas B. Pierron [:nbp] from comment #14)
> Lately, I managed to get:
> 
> […]
> 
> which leave me with no clue why this .o file is included twice …

Ok, this was a bad merge where Makefile.in were kept across the recent build system changes, causing the *.o files to be included twice.

Comment 16

[Nicolas B. Pierron [:nbp]]

Attachment: Make Gonk memory pressure by-pass the event queue.

This patch makes the memory pressure of Gonk by-pass the event queue by using the memory pressure flag.

The memory pressure flag is changed to be an Atomic variable stored on the thread manager.  This flag can have 3 values:

  0. No memory pressure event.
  1. A new memory pressure is seen.
  2. Some repeated memory pressure.

A new memory pressure takes precedence over a repeated memory pressure.  The reason is that repeated memory pressure are doing less work than done during a new memory pressure.  So, if the event loop is not processed in-between, a new event will clean-up more than a repeated event.

The ThreadManager has 2 way to declare a new memory pressure, a slow one and a normal one.  The slow dispatch of the memory pressure event do not queue (allocate) a new event in the event loop of the main thread.  This event will only be caught when the next event of the main thread will be processed, which might happen later (slow) if the event loop is empty.

Q: How are we supposed to change the uuid of an idl?

Comment 17

[Nicholas Nethercote [inactive]]

> Q: How are we supposed to change the uuid of an idl?

Generated a new one and stick it in.  You can say |firebot: uuid| on an IRC channel that has firebot in it.  Or you can visit a site like http://www.uuidgenerator.net/.  (I just discovered there are "version 1" and "version 4" variants... I'm not sure if the difference is significant.)

Comment 18

[Justin Lebar (not reading bugmail)]

Or use uuidgen.

Comment 19

[Justin Lebar (not reading bugmail)]

I'd like for us to design this interface so that it has a minimal amount of
indirection.  This is important because DispatchSlowMemoryPressure() must meet
a complex requirement -- it cannot allocate anything, acquire any locks, etc.
It's easier to enforce this requirement in the future if the function is
implemented entirely in one place.

If we implemented this logic entirely in nsThreadUtils.cpp without involving
nsThreadManager, I think that would probably be the cleanest way to do it.
We'd have NS_DispatchMemoryPressure(), which would set a static global to 1 and
enqueue a dummy event on the main thread.  Then we'd have a
NS_GetMemoryPressure() or whatever function to get the value out of the global,
and so on.

I know that's not exactly how most of the functions in NS_ThreadUtils are
implemented.  The reason they're written like they are is that back in the dark
days of Gecko, we wanted to let other people swap out arbitrary XPCOM
components.  So someone else could implement their own nsIThreadManager
implementation.  In this world, you want bare methods like NS_NewThread() to do
as little as possible, since those can't be swapped out arbitrarily.

But at some point we realized that swapping out key interface implementations
was a crazy idea, and this level of modularity isn't something we aim for
anymore.

Alternatively, we could implement it entirely in the thread manager and access
all the functions as

  nsThreadManager::get()->DoFoo();

This is not so bad, but it's also not great, because I don't think of
dispatching memory pressure events as having much to do with the thread
manager, and also because it relies on the fact that get() never allocates
anything, so we again have to enforce that rule across multiple pieces of code.

>@@ -44,16 +44,43 @@ interface nsIThreadManager : nsISupports
>   /**
>+   * This function causes the main thread to fire a memory pressure event before
>+   * processing the next event. Wake-up the main thread by adding a dummy event
>+   * in its event loop.
>+   *
>+   * You may call this function from any thread.
>+   */
>+  [noscript] void dispatchMemoryPressure();
>+
>+  /**
>+   * This function causes the main thread to fire an ongoing memory pressure
>+   * event before processing the next event. Wake-up the main thread by adding a
>+   * dummy event in its event loop.
>+   *
>+   * You may call this function from any thread.
>+   */
>+  [noscript] void dispatchOngoingMemoryPressure();
>+
>+  /**
>+   * This function causes the main thread to fire a memory pressure event before
>+   * processing the next event. Wait until an event is schedule to take any
>+   * action.
>+   *
>+   * You may call this function from any thread.
>+   */
>+  [noscript] void dispatchSlowMemoryPressure();
>+
>+  /**
>    * Get the main thread.
>    */
>   readonly attribute nsIThread mainThread;
> 
>   /**
>    * Get the current thread.  If the calling thread does not already have a
>    * nsIThread associated with it, then a new nsIThread will be created and
>    * associated with the current PRThread.

Hopefully we can get away without modifying the nsThreadManager XPIDL.

>diff --git a/xpcom/threads/nsThread.cpp b/xpcom/threads/nsThread.cpp
>index 702a0df..0d785bc 100644
>--- a/xpcom/threads/nsThread.cpp
>+++ b/xpcom/threads/nsThread.cpp

>@@ -571,22 +560,28 @@ nsThread::ProcessNextEvent(bool mayWait, bool *result)
>   NS_ENSURE_STATE(PR_GetCurrentThread() == mThread);
> 
>   if (MAIN_THREAD == mIsMainThread && mayWait && !ShuttingDown())
>     HangMonitor::Suspend();
> 
>   // Fire a memory pressure notification, if we're the main thread and one is
>   // pending.
>   if (MAIN_THREAD == mIsMainThread && !ShuttingDown()) {
>-    bool mpPending = PR_ATOMIC_SET(&sMemoryPressurePending, 0);
>+    int32_t mpPending = nsThreadManager::get()->GetPendingMemoryPressureEvent();

I think the mozilla::Atomic interface is implemented such that you could use an
enum here.  That would be preferrable to magic numbers.

>     if (mpPending) {
>       nsCOMPtr<nsIObserverService> os = services::GetObserverService();
>+
>+      // Use no-forward to prevent the notifications to be transfered to the
>+      // children of this process.
>+      nsString lowMem = NS_LITERAL_STRING("low-memory-no-forward");
>+      if (mpPending == 2)
>+        lowMem = NS_LITERAL_STRING("low-memory-ongoing-no-forward");
>+
>       if (os) {
>-        os->NotifyObservers(nullptr, "memory-pressure",
>-                            NS_LITERAL_STRING("low-memory").get());
>+        os->NotifyObservers(nullptr, "memory-pressure", lowMem.get());
>       }
>       else {
>         NS_WARNING("Can't get observer service!");
>       }
>     }
>   }
> 
>   bool notifyMainThreadObserver =

>+  // Atomically updated variable to notify all observers of a memory pressure
>+  // event, before the processing of the next event in the queue.
>+  mozilla::Atomic<int32_t>     mMemoryPressurePending;

I think this can be weakly ordered?  That should make accessing it a bit
cheaper, which is nice.

Sorry to send this back for another re-design.  FWIW, I think the code is
right, so hopefully this is a simple matter of moving some things around.

Comment 20

[Nicolas B. Pierron [:nbp]]

Attachment: Make Gonk memory pressure by-pass the event queue.

Delta:
 - Move modifications of the Thread Manager to a new file (xpcom/threads/nsMemoryPressure.h)
 - Add an enum for the state of the memory pressure and give a MemoryPressureState argument to each function.

The atomic does not appreciate to be parametrized with an enum type, as it is considered as being a different type than an int.

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

Comment 21

[Justin Lebar (not reading bugmail)]

Based on a quick glance, the patch above looks really good to me; I'll do a proper a look whenever you're ready.

Comment 22

[Justin Lebar (not reading bugmail)]

Oh, I filed bug 855669 on getting atomics to work with enums, but we don't need to let that block you here.

Comment 23

[Justin Lebar (not reading bugmail)]

Whoops, I mean bug 888548.

Comment 24

[Justin Lebar (not reading bugmail)]

Comment on attachment 769190 [details] [diff] [review]
Make Gonk memory pressure by-pass the event queue.

>diff --git a/xpcom/threads/nsMemoryPressure.h b/xpcom/threads/nsMemoryPressure.h
>new file mode 100644
>index 0000000..24be7da
>--- /dev/null
>+++ b/xpcom/threads/nsMemoryPressure.h

>@@ -0,0 +1,44 @@
>+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* vim:set ts=2 sw=2 sts=2 et cindent: */
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this
>+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+#ifndef nsMemoryPressure_h__
>+#define nsMemoryPressure_h__
>+
>+#include "nscore.h"
>+
>+enum MemoryPressureState {
>+  MemPressure_None,   /* No memory pressure */
>+  MemPressure_New,    /* New memory pressure deteced */
>+  MemPressure_OnGoing /* Repeated memory pressure */
>+};

Nit: s/OnGoing/Ongoing/ (it's one word).

We should explain what the difference is between New and Ongoing.  (In
particular, we should explain how a New overrides an existing Ongoing when we
dispatch, and how New and Ongoing translate to memory-pressure event topics.)

>+/**
>+ * Return and erase the latest state of the memory pressure event set by any of
>+ * the corresponding dispatch function.
>+ */
>+XPCOM_API(MemoryPressureState)
>+NS_GetPendingMemoryPressure();

XPCOM_API gives these methods external visibility.  We don't need this anymore,
now that we compile Gecko as one big shared library, instead of as a bunch of
small ones, so I think we should get rid of XPCOM_API here and elsewhere.  That
might make these calls a bit faster on some platforms, which isn't irrelevant,
since we call this on every turn of the event loop.

>+/**
>+ * This function causes the main thread to fire a memory pressure event before
>+ * processing the next event. It is infallible and does not allocate any memory.

Nit: Let's be explicit and say something like

  "... before processing the next event, but if there are no events pending in
  the main thread's event queue, the memory pressure event won't be dispatched
  until one is enqueued."

>+ *
>+ * You may call this function from any thread.
>+ */
>+XPCOM_API(void)
>+NS_DispatchSlowMemoryPressure(MemoryPressureState state);

Nit: I think DispatchEventualMemoryPressure or something like that might be a
better name.  With "slow", it's not clear whether it's the method or the event
that's slow.  (In fact, the method is quite fast...)

>+/**
>+ * This function causes the main thread to fire a memory pressure event before
>+ * processing the next event. Wake-up the main thread by adding a dummy event in
>+ * its event loop.

s/Wake-up/We wake up/
s/in its event loop/to its event loop/
s/loop./loop, so, unlike with NS_DispatchEventualMemoryPressure, this
memory-pressure event is always fired relatively quickly, even if the event
loop is otherwise empty./

>+ * You may call this function from any thread.
>+ */
>+XPCOM_API(nsresult)
>+NS_DispatchMemoryPressure(MemoryPressureState state);
>+
>+#endif // nsMemoryPressure_h__

>diff --git a/xpcom/threads/nsMemoryPressure.cpp b/xpcom/threads/nsMemoryPressure.cpp
>new file mode 100644
>index 0000000..250fc57
>--- /dev/null
>+++ b/xpcom/threads/nsMemoryPressure.cpp

>@@ -0,0 +1,63 @@
>+#include "nsMemoryPressure.h"
>+#include "mozilla/Atomics.h"
>+
>+#include "nsThreadUtils.h"
>+
>+// This class is designed to be subclassed.

This isn't relevant here.

>+class nsWakeUpRunnable MOZ_FINAL : public nsIRunnable
>+{
>+public:
>+  NS_DECL_ISUPPORTS
>+  NS_DECL_NSIRUNNABLE
>+
>+  nsWakeUpRunnable() {
>+  }
>+
>+  virtual ~nsWakeUpRunnable() {
>+  }
>+};
>+
>+NS_IMPL_THREADSAFE_ISUPPORTS1(nsWakeUpRunnable, nsIRunnable)
>+
>+NS_IMETHODIMP
>+nsWakeUpRunnable::Run()
>+{
>+  // Do nothing
>+  return NS_OK;
>+}

Can we enqueue a new nsRunnable instead of creating this new class?
nsRunnable has an empty Run() method and everything, just like you want.

>+static mozilla::Atomic<int32_t, mozilla::Relaxed>
>+  sMemoryPressurePending(MemPressure_None);

I'm afraid that this will create a static constructor, which we don't like
because it slows down Firefox's startup.  If we use the default constructor, we
should be safe (or, if we still get a static constructor, it's the fault of the
Atomics header, not us), and the compiler will still initialize this to zero
for us.

Also, nit: In cpp files, we prefer |using mozilla| to explicit |mozilla::|.

>+MemoryPressureState
>+NS_GetPendingMemoryPressure()
>+{
>+  int32_t value = sMemoryPressurePending.exchange(MemPressure_None);
>+  return MemoryPressureState(value);
>+}
>+
>+void
>+NS_DispatchSlowMemoryPressure(MemoryPressureState state)
>+{
>+  // A new memory pressure erase an ongoing memory pressure.

s/erase/event erases/
s/./, but an existing "new" memory pressure event takes precedence over a new
"ongoing" memory pressure event./

>+  switch (state) {
>+    case MemPressure_None:
>+      sMemoryPressurePending = MemPressure_None;
>+      break;
>+    case MemPressure_New:
>+      sMemoryPressurePending = MemPressure_New;
>+      break;
>+    case MemPressure_OnGoing:
>+      sMemoryPressurePending.compareExchange(MemPressure_None, MemPressure_OnGoing);
>+      break;
>+  }
>+}

>diff --git a/xpcom/threads/nsThread.cpp b/xpcom/threads/nsThread.cpp
>index 702a0df..18fe870 100644
>--- a/xpcom/threads/nsThread.cpp
>+++ b/xpcom/threads/nsThread.cpp

>@@ -571,22 +555,28 @@ nsThread::ProcessNextEvent(bool mayWait, bool *result)
>   NS_ENSURE_STATE(PR_GetCurrentThread() == mThread);
> 
>   if (MAIN_THREAD == mIsMainThread && mayWait && !ShuttingDown())
>     HangMonitor::Suspend();
> 
>   // Fire a memory pressure notification, if we're the main thread and one is
>   // pending.
>   if (MAIN_THREAD == mIsMainThread && !ShuttingDown()) {
>-    bool mpPending = PR_ATOMIC_SET(&sMemoryPressurePending, 0);
>-    if (mpPending) {
>+    MemoryPressureState mpPending = NS_GetPendingMemoryPressure();
>+    if (mpPending != MemPressure_None) {
>       nsCOMPtr<nsIObserverService> os = services::GetObserverService();
>+
>+      // Use no-forward to prevent the notifications to be transfered to the
>+      // children of this process.

s/to be/from being/

Some verbs always take infinitives, and some always take gerunds.  "Prevent" is
in the latter class.

There are also verbs which can take either an infinitive or a gerund. Sometimes
there's no or little difference in meaning ("I like to run" vs. "I like
running"), but sometimes there is a difference ("I forgot to take out the
trash" vs. "I forgot taking out the trash").

s/transfered/transferred/

>+      nsString lowMem = NS_LITERAL_STRING("low-memory-no-forward");
>+      if (mpPending == MemPressure_OnGoing)
>+        lowMem = NS_LITERAL_STRING("low-memory-ongoing-no-forward");

I think using nsString forces a copy here.  But if we used nsAdoptingString,
that would avoid a copy.

Alternatively, you could do

  NS_NAMED_LITERAL_STRING(lowMem, "...");
  NS_NAMED_LITERAL_STRING(lowMemOngoing, "...");
  
  os->NotifyObservers(nullptr, "memory-pressure",
                      mpPending == MemPressure_New ? lowMem.get() :
                                                     lowMemOngoing.get());

>       if (os) {
>-        os->NotifyObservers(nullptr, "memory-pressure",
>-                            NS_LITERAL_STRING("low-memory").get());
>+        os->NotifyObservers(nullptr, "memory-pressure", lowMem.get());

r=me with these nits taken care of!

Comment 25

[Nicolas B. Pierron [:nbp]]

Try: https://tbpl.mozilla.org/?tree=Try&rev=14cde57e0a00
Inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/1c6223f7c74f

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Unfortunately, this was causing reproducible Android Armv6 mochitest-1 and mochitest-3 crashes, so it had to be backed out. Conveniently, they happened at the same point in the test run each time.
https://hg.mozilla.org/mozilla-central/rev/1c6223f7c74f

https://tbpl.mozilla.org/php/getParsedLog.php?id=24999543&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=24999565&tree=Mozilla-Inbound

Comment 27

[Ryan VanderMeulen [:RyanVM]]

...and then as magically (and reproducibly) as it appeared, it magically disappeared before the backout hit. So I guess run it through Try, retrigger the armv6 m1/m3 a bunch of times and push again if you're clear. Sorry for the churn :(

Comment 28

[Justin Lebar (not reading bugmail)]

I'd be pretty surprised if this had any effect on Android.  We shouldn't be firing memory-pressure events on Android.  Unless that got added recently or something.

Comment 29

[Nicolas B. Pierron [:nbp]]

https://tbpl.mozilla.org/?tree=Try&rev=bafe0ad175c9 (for M7 failures see Bug 881404)
https://hg.mozilla.org/integration/mozilla-inbound/rev/33935ee00c41

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/33935ee00c41

Comment 31

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

This should not have landed without an MPL header in nsMemoryPressure.cpp.

Comment 32

[Nicolas B. Pierron [:nbp]]

(In reply to :Ms2ger from comment #31)
> This should not have landed without an MPL header in nsMemoryPressure.cpp.

Thanks for noticing that.

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

Comment 33

[Ed Morley [:emorley]]

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