Last Comment Bug 970895 - Run MemoryPressureWatcher on I/O thread
: Run MemoryPressureWatcher on I/O thread
Status: RESOLVED INVALID
:
Product: Firefox OS
Classification: Client Software
Component: General (show other bugs)
: unspecified
: ARM Gonk (Firefox OS)
-- normal (vote)
: ---
Assigned To: Thomas Zimmermann [:tzimmermann] [:tdz]
:
:
Mentors:
Depends on: 973824
Blocks:
  Show dependency treegraph
 
Reported: 2014-02-11 06:00 PST by Thomas Zimmermann [:tzimmermann] [:tdz]
Modified: 2014-02-19 00:22 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
[01] Bug 970895: Use I/O loop for polling memory-pressure events (17.04 KB, patch)
2014-02-11 06:03 PST, Thomas Zimmermann [:tzimmermann] [:tdz]
dhylands: review+
Details | Diff | Splinter Review
Some basic memory measurements (28.94 KB, text/plain)
2014-02-11 06:12 PST, Thomas Zimmermann [:tzimmermann] [:tdz]
no flags Details
[01] Bug 970895: Use I/O loop for polling memory-pressure events (v2) (17.90 KB, patch)
2014-02-17 03:32 PST, Thomas Zimmermann [:tzimmermann] [:tdz]
tzimmermann: review+
Details | Diff | Splinter Review

Description User image Thomas Zimmermann [:tzimmermann] [:tdz] 2014-02-11 06:00:57 PST
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 User image Thomas Zimmermann [:tzimmermann] [:tdz] 2014-02-11 06:03:56 PST
Created attachment 8374031 [details] [diff] [review]
[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 User image Thomas Zimmermann [:tzimmermann] [:tdz] 2014-02-11 06:12:22 PST
Created attachment 8374037 [details]
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 User image Thomas Zimmermann [:tzimmermann] [:tdz] 2014-02-13 23:04:25 PST
ping for review
Comment 4 User image Gabriele Svelto [:gsvelto] 2014-02-14 06:34:22 PST
+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 User image Dave Hylands [:dhylands] 2014-02-14 17:34:38 PST
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 User image Gabriele Svelto [:gsvelto] 2014-02-15 00:13:47 PST
(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 User image Thomas Zimmermann [:tzimmermann] [:tdz] 2014-02-17 03:28:41 PST
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 User image Thomas Zimmermann [:tzimmermann] [:tdz] 2014-02-17 03:30:15 PST
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 User image Thomas Zimmermann [:tzimmermann] [:tdz] 2014-02-17 03:32:40 PST
Created attachment 8377069 [details] [diff] [review]
[01] Bug 970895: Use I/O loop for polling memory-pressure events (v2)
Comment 10 User image Thomas Zimmermann [:tzimmermann] [:tdz] 2014-02-17 03:33:46 PST
https://hg.mozilla.org/integration/b2g-inbound/rev/e9055e7476f1
Comment 11 User image Thomas Zimmermann [:tzimmermann] [:tdz] 2014-02-17 03:34:20 PST
https://tbpl.mozilla.org/?tree=B2g-Inbound&rev=e9055e7476f1
Comment 12 User image Phil Ringnalda (:philor) 2014-02-17 15:18:42 PST
https://hg.mozilla.org/mozilla-central/rev/e9055e7476f1
Comment 13 User image Ed Morley [:emorley] 2014-02-18 09:53:20 PST
Backed out out jsmith's request for causing bug 973824:

remote:   https://hg.mozilla.org/mozilla-central/rev/eb675c2b6dee
Comment 14 User image Thomas Zimmermann [:tzimmermann] [:tdz] 2014-02-19 00:22:03 PST
We cannot fix this bug. See discussion in bug 973824.

Note You need to log in before you can comment on or make changes to this bug.