683517 - Make nsExpirationTracker expire all tracked objects when "memory-pressure" notification is observed

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Josh Matthews [:jdm]]

http://mxr.mozilla.org/mozilla-central/source/xpcom/ds/nsExpirationTracker.h

nsExpirationTracker conveniently defines an AgeAllGenerations function that should force its tracked objects to expire. An enterprising contributor could step up and add the goop necessary to turn the class into an XPCOM observer (http://mxr.mozilla.org/mozilla-central/source/xpcom/ds/nsIObserver.idl). http://mxr.mozilla.org/mozilla-central/source/gfx/src/nsDeviceContext.cpp looks like a good example of how to do this (search for "memory-pressure" for relevant code).

http://mxr.mozilla.org/mozilla-central/source/xpcom/ds/nsExpirationTracker.h#225

Comment 1

[Josh Matthews [:jdm]]

To note, this will not be a simple one-liner patch. Besides adding the regular XPCOM goop like NS_IMPL/DECL_ISUPPORTS, we'll also need to ensure that any code that uses a pointer to a class that inherits from nsExpirationTracker (http://mxr.mozilla.org/mozilla-central/search?string=nsexpirationtracker) uses proper refcounting pointers (either nsCOMPtr<nsIObserver> or nsRefPtr<Whatever>). If they're already using nsRefPtr, there shouldn't be any problems, but I haven't looked into whether that's the case or not.

Regardless, this shouldn't deter someone who's willing to put in the work. This is a nice, meaty bug that could help control Firefox memory usage.

Comment 2

[Josh Matthews [:jdm]]

While talking with Hari, I realized that it would be a lot easier to store a member object that implements nsIObserver inside nsExpirationTracker, rather than make nsExpirationTracker implement nsIObserver itself.

Comment 3

[Jiten [:deLta30]]

Attachment: Created class ExpirationTrackerObserver inside nsExpirationTracker which calls AgeAllGenerations() when "memory-pressure" message is observed.

Comment 4

[Josh Matthews [:jdm]]

Comment on attachment 570040 [details] [diff] [review]
Created class ExpirationTrackerObserver inside nsExpirationTracker which calls AgeAllGenerations() when "memory-pressure" message is observed.

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

Great! There's one important problem right now that will cause crashes every time an nsExpirationTracker object is destroyed, so I'd like to see another patch that we can send to the tryserver and make sure that nothing breaks.

::: xpcom/ds/nsExpirationTracker.h
@@ -269,0 +274,8 @@
> > +    
> > +    /**
> > +     * Whenever "memory-pressure" is observed, it calls AgeAllGenerations()
> > +     * to minimize memory usage.
NaN more ...

Make this private, and call it mOwner.

@@ -269,0 +274,9 @@
> > +    
> > +    /**
> > +     * Whenever "memory-pressure" is observed, it calls AgeAllGenerations()
> > +     * to minimize memory usage.
NaN more ...

Make this be the constructor - there's no need to have an explicit Init/Delete.

@@ -269,0 +274,16 @@
> > +    
> > +    /**
> > +     * Whenever "memory-pressure" is observed, it calls AgeAllGenerations()
> > +     * to minimize memory usage.
NaN more ...

This can be the destructor.

@@ +328,5 @@
>      nsCOMPtr<nsITimer> mTimer;
>      PRUint32           mTimerPeriod;
>      PRUint32           mNewestGeneration;
>      bool               mInAgeOneGeneration;
> +    ExpirationTrackerObserver mObserver;

This will break when Destroy() is run, because nsIObserver is refcounted. That means that RemoveObserver will release the last reference, so ExpirationTrackerObserver::Release will run and try to delete |this|. Instead, make mObserver by an nsRefPtr<ExpirationTrackerObserver>, and create it with |new ExpirationTrackerObserver(this)|.

@@ -327,0 +356,8 @@
> > +template<class T, PRUint32 K>
> > +NS_IMETHODIMP
> > +nsExpirationTracker<T, K>::ExpirationTrackerObserver::Observe(nsISupports     *aSubject,
> > +                                                              const char      *aTopic,
NaN more ...

mobj-> instead of (*mobj).

Comment 5

[Josh Matthews [:jdm]]

Sorry, the review tool destroyed my review comments like usual. Here's the actual code references:

>+    public:
>+      nsExpirationTracker<T,K> *mobj;

Make this private, and call it mOwner.

>+      void Init(nsExpirationTracker<T,K> *obj) {
>+        mobj = obj;

Make this be the constructor - there's no need to have an explicit Init/Delete.

>+      void Destroy() {

This can be the destructor.

>      nsCOMPtr<nsITimer> mTimer;
>      PRUint32           mTimerPeriod;
>      PRUint32           mNewestGeneration;
>      bool               mInAgeOneGeneration;
> +    ExpirationTrackerObserver mObserver;

This will break when Destroy() is run, because nsIObserver is refcounted. That means that RemoveObserver will release the last reference, so ExpirationTrackerObserver::Release will run and try to delete |this|. Instead, make mObserver by an nsRefPtr<ExpirationTrackerObserver>, and create it with |new ExpirationTrackerObserver(this)|.

>+    (*mobj).AgeAllGenerations();

mobj-> instead of (*mobj).

Comment 6

[Jiten [:deLta30]]

">+      void Init(nsExpirationTracker<T,K> *obj) {
>+        mobj = obj;

Make this be the constructor - there's no need to have an explicit Init/Delete."

According to comment here http://mxr.mozilla.org/mozilla-central/source/gfx/src/nsDeviceContext.cpp#118 ,it's risky to call AddObserver or RemoveObserver from constructor or destructor respectively.

Comment 7

[Josh Matthews [:jdm]]

Wow, you're right. I feel quite silly for suggesting it now. You can ignore those comments.

Comment 8

[Nicholas Nethercote [inactive]]

(In reply to Josh Matthews [:jdm] from comment #5)
> Sorry, the review tool destroyed my review comments like usual.

Wherever you click to insert a comment, Splinter will include that line and the four above it as context.  Just remember that and you'll be fine.

Comment 9

[Jiten [:deLta30]]

Attachment: Created class ExpirationTrackerObserver inside nsExpirationTracker which calls AgeAllGenerations() when "memory-pressure" message is observed.

This should be ready for tryserver now, I guess.

Comment 10

[Josh Matthews [:jdm]]

You're missing actually creating the ExpirationTrackerObserver now. Currently this will crash because you're dereferencing a null pointer when calling Init.

Comment 11

[Josh Matthews [:jdm]]

And just to be clear - the best way to test this yourself is to start up the browser and make sure it runs, then press the "Minimize memory usage" button in about:memory. That sends the memory-presure notification that these objects are responding to.

Comment 12

[Jiten [:deLta30]]

Attachment: Created class ExpirationTrackerObserver inside nsExpirationTracker which calls AgeAllGenerations() when "memory-pressure" message is observed.

I have checked this and there is no crash on pressing 'Minimise memory usage' button on 'about:memory'.

Comment 13

[Mozilla RelEng Bot]

Try run for 817131fda48c is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=817131fda48c
Results (out of 18 total builds):
    failure: 18
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-817131fda48c

Comment 14

[Josh Matthews [:jdm]]

My mistake, the patch didn't apply cleanly. There's another try run with real results on its way.

Comment 15

[Mozilla RelEng Bot]

Try run for e79d83d83953 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=e79d83d83953
Results (out of 257 total builds):
    success: 252
    warnings: 5
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-e79d83d83953

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 570442 [details] [diff] [review]
Created class ExpirationTrackerObserver inside nsExpirationTracker which calls AgeAllGenerations() when "memory-pressure" message is observed.

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

Can't we use an NS_IMPL_ISUPPORTS macro here?

::: xpcom/ds/nsExpirationTracker.h
@@ +331,5 @@
>      nsCOMPtr<nsITimer> mTimer;
>      PRUint32           mTimerPeriod;
>      PRUint32           mNewestGeneration;
>      bool               mInAgeOneGeneration;
> +    nsRefPtr<ExpirationTrackerObserver> mObserver;

Move this up next to mTimer to avoid space wastage

Comment 17

[Josh Matthews [:jdm]]

The nsISupports methods need to be templated, and I don't believe there's any way to use the macros on templated classes. Although, now that I think of it, you might be able to get away with

template <class T, class K> NS_IMPL_ADDREF(nsExpirationTracker<T, K>::ExpirationTrackerObserver)

and similarly for NS_IMPL_RELEASE and NS_IMPL_QUERYINTERFACE1.

Comment 18

[Jiten [:deLta30]]

No, it won't work.Macro takes the parameter 'nsExpirationTracker<T, K>::ExpirationTrackerObserver' as 2 parameters instead of 1 because of ',' in between.

Comment 19

[Josh Matthews [:jdm]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/b316ac07d768

Comment 20

[Ed Morley [:emorley]]

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