970895 - Run MemoryPressureWatcher on I/O thread

Status: RESOLVED INVALID

(Firefox OS Graveyard :: General, defect)

Description

[Thomas Zimmermann [:tzimmermann] [:tdz]]

I think we can remove the thread for memory-pressure events and run the code on the I/O thread. The code base will be a bit cleaner (one hand-written poll loop less) and we can safe the thread's resources.

Comment 1

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 970895: Use I/O loop for polling memory-pressure events

I tested this patch on the Hamachi with the test case from bug 771195. When I run it I can see 'Memory pressure detected' and 'MemoryPressure is over' in the logcat.

Comment 2

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: Some basic memory measurements

I find it hard to get some useful memory measurements from the system. I measured memory consumption several times after rebooting when the login screen was shown. The patched version always uses less physical memory at this point, but it's hard to say if the patch has any influence and the difference of ~6MiB is probably unrelated. After 5 min, memory consumption seems more or less the same.

One thing I noticed was that the memory consumption for page tables and kernel stack was always less with the patch applied. And this is where the patch most certainly makes a difference.

Comment 3

[Thomas Zimmermann [:tzimmermann] [:tdz]]

ping for review

Comment 4

[Gabriele Svelto [:gsvelto]]

+1 for this, it's an obvious win and the cleaner, simpler code is a very nice improvement. Consider nom'ing this for 1.3T as this saves memory and doesn't seem too risky as the logic is mostly the same as before.

Comment 5

[Dave Hylands [:dhylands]]

Comment on attachment 8374031 [details] [diff] [review]
[01] Bug 970895: Use I/O loop for polling memory-pressure events

Review of attachment 8374031 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

Most of my comments are nits.

r=me with the other issues addressed.

::: widget/gonk/GonkMemoryPressureMonitoring.cpp
@@ +23,5 @@
>  
>  namespace {
>  
> +//
> +// MemoryPressureWatcher watches on the I/O tread for changes to the

nit: spelling of thread

@@ +24,5 @@
>  namespace {
>  
> +//
> +// MemoryPressureWatcher watches on the I/O tread for changes to the
> +// lowmemkiller's sysfs interface. If the system run's low on memory,

nit: run's s/b runs

@@ +58,5 @@
> +    static void* operator new(size_t aSize);
> +    static void operator delete(void* aMem, size_t aSize);
> +
> +    void Run() MOZ_OVERRIDE
> +    {

nit: assert that you're on the I/O thread

@@ +78,5 @@
> +  {
> +  public:
> +    void* Alloc()
> +    {
> +      return mMem;

since this is supposed to be a singleton, I think it would be worthwhile to add an allocated boolean and in alloc assert that its not allocated, and in release assert that it is allocated.

@@ +120,5 @@
> +      fd = open("/sys/kernel/mm/lowmemkiller/notify_trigger_active",
> +                O_RDONLY | O_CLOEXEC);
> +    } while (fd == -1 && errno == EINTR);
> +
> +    NS_ENSURE_STATE(fd != -1);

nit: This particular macro has been deprecated. See bug 672843 and https://groups.google.com/forum/#!topic/mozilla.dev.platform/1clMLuuhtWQ

Assume for the entire review. I marked a few, but didn't mark them all.

I think that not all kernels have the lowmemkiller compiled in, so we should make sure that everything works properly if the open fails.

@@ +133,5 @@
>  
> +    int res;
> +
> +    do {
> +      res = close(mFd);

If, for some reason, Open failed, then mFd will be == -1, so we should check and not call close (i.e. just return early)

@@ +137,5 @@
> +      res = close(mFd);
> +    } while (res == -1 && errno == EINTR);
> +
> +    mFd = -1;
> +    NS_ENSURE_TRUE_VOID(res != -1);

nit: NS_ENSURE_xxx is deprecated

@@ +145,5 @@
> +  {
> +    MessageLoopForIO* ioLoop = MessageLoopForIO::current();
> +    MOZ_ASSERT(ioLoop == mIOLoop);
> +    ioLoop->WatchFileDescriptor(mFd, true, MessageLoopForIO::WATCH_READ,
> +                                 &mReadWatcher, this);

nit: indentation (need to remove 1 space)

@@ +208,3 @@
>  
> +    off_t off = lseek(mFd, 0, SEEK_SET);
> +    NS_ENSURE_STATE(!off);

nit: NS_ENSURE_xxx is deprecated

@@ +258,5 @@
> +    MOZ_ASSERT(mWatcher);
> +  }
> +
> +  void Run() MOZ_OVERRIDE
> +  {

nit: Assert that you're on the I/O thread

@@ +260,5 @@
> +
> +  void Run() MOZ_OVERRIDE
> +  {
> +    nsresult rv = mWatcher->Open();
> +    NS_ENSURE_SUCCESS_VOID(rv);

nit: NS_ENSURE_xxx is deprecated

@@ +280,5 @@
> +    MOZ_ASSERT(mWatcher);
> +  }
> +
> +  void Run() MOZ_OVERRIDE
> +  {

nit: Assert that you're on the I/O thread

@@ +330,5 @@
>  {
> +  MessageLoop* ioLoop = XRE_GetIOMessageLoop();
> +  uint32_t pollMS =
> +    Preferences::GetUint("gonk.systemMemoryPressureRecoveryPollMS", 5000);
> +  MemoryPressureWatcher* watcher = new MemoryPressureWatcher(ioLoop, pollMS);

Should we allow pollMS of zero to mean - don't use?

@@ +338,5 @@
> +  watcher->GetIOLoop()->PostTask(FROM_HERE, task);
> +
> +  // Install shutdown observer
> +  nsCOMPtr<nsIObserverService> os = services::GetObserverService();
> +  NS_ENSURE_TRUE_VOID(os);

nit: NS_ENSURE_xxx is deprectated.

@@ +339,5 @@
> +
> +  // Install shutdown observer
> +  nsCOMPtr<nsIObserverService> os = services::GetObserverService();
> +  NS_ENSURE_TRUE_VOID(os);
> +  nsCOMPtr<ShutdownObserver> observer = new ShutdownObserver(watcher);

nit: Use nsRefPtr for concrete classes.

Comment 6

[Gabriele Svelto [:gsvelto]]

(In reply to Dave Hylands [:dhylands] from comment #5)
> @@ +120,5 @@
> > +      fd = open("/sys/kernel/mm/lowmemkiller/notify_trigger_active",
> > +                O_RDONLY | O_CLOEXEC);
> > +    } while (fd == -1 && errno == EINTR);
> > +
> > +    NS_ENSURE_STATE(fd != -1);
> 
> I think that not all kernels have the lowmemkiller compiled in, so we should
> make sure that everything works properly if the open fails.

Yes, some lack the low memory notification trigger such as the Unagi's kernel and from some comments in other bugs the Tarako's might also be missing it.

Comment 7

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Hi Dave,

Thanks for reviewing the patch. See my replies below.

> 
> ::: widget/gonk/GonkMemoryPressureMonitoring.cpp
> @@ +23,5 @@
> >  
> >  namespace {
> >  
> > +//
> > +// MemoryPressureWatcher watches on the I/O tread for changes to the
> 
> nit: spelling of thread

Done

> @@ +24,5 @@
> >  namespace {
> >  
> > +//
> > +// MemoryPressureWatcher watches on the I/O tread for changes to the
> > +// lowmemkiller's sysfs interface. If the system run's low on memory,
> 
> nit: run's s/b runs

Done

> @@ +58,5 @@
> > +    static void* operator new(size_t aSize);
> > +    static void operator delete(void* aMem, size_t aSize);
> > +
> > +    void Run() MOZ_OVERRIDE
> > +    {
> 
> nit: assert that you're on the I/O thread

Done
 
> @@ +78,5 @@
> > +  {
> > +  public:
> > +    void* Alloc()
> > +    {
> > +      return mMem;
> 
> since this is supposed to be a singleton, I think it would be worthwhile to
> add an allocated boolean and in alloc assert that its not allocated, and in
> release assert that it is allocated.

Done

> @@ +120,5 @@
> > +      fd = open("/sys/kernel/mm/lowmemkiller/notify_trigger_active",
> > +                O_RDONLY | O_CLOEXEC);
> > +    } while (fd == -1 && errno == EINTR);
> > +
> > +    NS_ENSURE_STATE(fd != -1);
> 
> nit: This particular macro has been deprecated. See bug 672843 and
> https://groups.google.com/forum/#!topic/mozilla.dev.platform/1clMLuuhtWQ
> 
> Assume for the entire review. I marked a few, but didn't mark them all.

Bug 672843 looks like the perfect example for bike shedding. ;) I replaced all obsolete macros by the new ones.

> 
> I think that not all kernels have the lowmemkiller compiled in, so we should
> make sure that everything works properly if the open fails.

I tested on the Hamachi (with sysfs interface) and Unagi (without sysfs interface) and both worked fine. As before, the Unagi prints a warning about fd being -1.

> @@ +133,5 @@
> >  
> > +    int res;
> > +
> > +    do {
> > +      res = close(mFd);
> 
> If, for some reason, Open failed, then mFd will be == -1, so we should check
> and not call close (i.e. just return early)

Calling close for a non-open file descriptor could be a bug, so I added a warning as well.

> @@ +137,5 @@
> > +      res = close(mFd);
> > +    } while (res == -1 && errno == EINTR);
> > +
> > +    mFd = -1;
> > +    NS_ENSURE_TRUE_VOID(res != -1);
> 
> nit: NS_ENSURE_xxx is deprecated

Done

> @@ +145,5 @@
> > +  {
> > +    MessageLoopForIO* ioLoop = MessageLoopForIO::current();
> > +    MOZ_ASSERT(ioLoop == mIOLoop);
> > +    ioLoop->WatchFileDescriptor(mFd, true, MessageLoopForIO::WATCH_READ,
> > +                                 &mReadWatcher, this);
> 
> nit: indentation (need to remove 1 space)

Done

> @@ +208,3 @@
> >  
> > +    off_t off = lseek(mFd, 0, SEEK_SET);
> > +    NS_ENSURE_STATE(!off);
> 
> nit: NS_ENSURE_xxx is deprecated

Done

> @@ +258,5 @@
> > +    MOZ_ASSERT(mWatcher);
> > +  }
> > +
> > +  void Run() MOZ_OVERRIDE
> > +  {
> 
> nit: Assert that you're on the I/O thread

Done

> @@ +260,5 @@
> > +
> > +  void Run() MOZ_OVERRIDE
> > +  {
> > +    nsresult rv = mWatcher->Open();
> > +    NS_ENSURE_SUCCESS_VOID(rv);
> 
> nit: NS_ENSURE_xxx is deprecated

Done

> @@ +280,5 @@
> > +    MOZ_ASSERT(mWatcher);
> > +  }
> > +
> > +  void Run() MOZ_OVERRIDE
> > +  {
> 
> nit: Assert that you're on the I/O thread

Done
 
> @@ +330,5 @@
> >  {
> > +  MessageLoop* ioLoop = XRE_GetIOMessageLoop();
> > +  uint32_t pollMS =
> > +    Preferences::GetUint("gonk.systemMemoryPressureRecoveryPollMS", 5000);
> > +  MemoryPressureWatcher* watcher = new MemoryPressureWatcher(ioLoop, pollMS);
> 
> Should we allow pollMS of zero to mean - don't use?

It's not in the old code. If this is useful, we should implement it in a new bug.

> @@ +338,5 @@
> > +  watcher->GetIOLoop()->PostTask(FROM_HERE, task);
> > +
> > +  // Install shutdown observer
> > +  nsCOMPtr<nsIObserverService> os = services::GetObserverService();
> > +  NS_ENSURE_TRUE_VOID(os);
> 
> nit: NS_ENSURE_xxx is deprectated.

Done

> @@ +339,5 @@
> > +
> > +  // Install shutdown observer
> > +  nsCOMPtr<nsIObserverService> os = services::GetObserverService();
> > +  NS_ENSURE_TRUE_VOID(os);
> > +  nsCOMPtr<ShutdownObserver> observer = new ShutdownObserver(watcher);
> 
> nit: Use nsRefPtr for concrete classes.

Done

Comment 8

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Hi Gabriele

(In reply to Gabriele Svelto [:gsvelto] from comment #6)
> (In reply to Dave Hylands [:dhylands] from comment #5)
> > @@ +120,5 @@
> > > +      fd = open("/sys/kernel/mm/lowmemkiller/notify_trigger_active",
> > > +                O_RDONLY | O_CLOEXEC);
> > > +    } while (fd == -1 && errno == EINTR);
> > > +
> > > +    NS_ENSURE_STATE(fd != -1);
> > 
> > I think that not all kernels have the lowmemkiller compiled in, so we should
> > make sure that everything works properly if the open fails.
> 
> Yes, some lack the low memory notification trigger such as the Unagi's
> kernel and from some comments in other bugs the Tarako's might also be
> missing it.

Right. Having the sysfs interface is actually the exception, rather than the norm. We don't even have this patch on the emulator; something I wanted to fix for quite some time now.

Comment 9

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 970895: Use I/O loop for polling memory-pressure events (v2)

Comment 10

[Thomas Zimmermann [:tzimmermann] [:tdz]]

https://hg.mozilla.org/integration/b2g-inbound/rev/e9055e7476f1

Comment 11

[Thomas Zimmermann [:tzimmermann] [:tdz]]

https://tbpl.mozilla.org/?tree=B2g-Inbound&rev=e9055e7476f1

Comment 12

[Phil Ringnalda (:philor)]

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

Comment 13

[Ed Morley [:emorley]]

Backed out out jsmith's request for causing bug 973824:

remote:   https://hg.mozilla.org/mozilla-central/rev/eb675c2b6dee

Comment 14

[Thomas Zimmermann [:tzimmermann] [:tdz]]

We cannot fix this bug. See discussion in bug 973824.