Last Comment Bug 683517 - Make nsExpirationTracker expire all tracked objects when "memory-pressure" notification is observed
: Make nsExpirationTracker expire all tracked objects when "memory-pressure" no...
Status: RESOLVED FIXED
[good first bug] [mentor=jdm] [MemShr...
:
Product: Core
Classification: Components
Component: General (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla10
Assigned To: Jiten [:deLta30]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-31 08:03 PDT by Josh Matthews [:jdm] (away until 9/3)
Modified: 2011-11-02 06:30 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Created class ExpirationTrackerObserver inside nsExpirationTracker which calls AgeAllGenerations() when "memory-pressure" message is observed. (5.90 KB, patch)
2011-10-27 11:19 PDT, Jiten [:deLta30]
no flags Details | Diff | Splinter Review
Created class ExpirationTrackerObserver inside nsExpirationTracker which calls AgeAllGenerations() when "memory-pressure" message is observed. (5.91 KB, patch)
2011-10-28 03:35 PDT, Jiten [:deLta30]
no flags Details | Diff | Splinter Review
Created class ExpirationTrackerObserver inside nsExpirationTracker which calls AgeAllGenerations() when "memory-pressure" message is observed. (5.95 KB, patch)
2011-10-28 18:36 PDT, Jiten [:deLta30]
roc: review+
Details | Diff | Splinter Review

Description Josh Matthews [:jdm] (away until 9/3) 2011-08-31 08:03:56 PDT
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] (away until 9/3) 2011-08-31 09:12:38 PDT
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] (away until 9/3) 2011-09-18 11:40:17 PDT
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] 2011-10-27 11:19:42 PDT
Created attachment 570040 [details] [diff] [review]
Created class ExpirationTrackerObserver inside nsExpirationTracker which calls AgeAllGenerations() when "memory-pressure" message is observed.
Comment 4 Josh Matthews [:jdm] (away until 9/3) 2011-10-27 12:09:42 PDT
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] (away until 9/3) 2011-10-27 12:13:05 PDT
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] 2011-10-27 13:58:05 PDT
">+      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] (away until 9/3) 2011-10-27 14:32:29 PDT
Wow, you're right. I feel quite silly for suggesting it now. You can ignore those comments.
Comment 8 Nicholas Nethercote [:njn] 2011-10-27 14:53:42 PDT
(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] 2011-10-28 03:35:01 PDT
Created attachment 570214 [details] [diff] [review]
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] (away until 9/3) 2011-10-28 06:50:47 PDT
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] (away until 9/3) 2011-10-28 06:58:29 PDT
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] 2011-10-28 18:36:27 PDT
Created attachment 570442 [details] [diff] [review]
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 2011-10-29 20:50:25 PDT
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/josh@joshmatthews.net-817131fda48c
Comment 14 Josh Matthews [:jdm] (away until 9/3) 2011-10-29 21:12:21 PDT
My mistake, the patch didn't apply cleanly. There's another try run with real results on its way.
Comment 15 Mozilla RelEng Bot 2011-10-30 01:00:38 PDT
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/josh@joshmatthews.net-e79d83d83953
Comment 16 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-10-30 14:00:41 PDT
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] (away until 9/3) 2011-10-30 14:25:29 PDT
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] 2011-10-30 16:28:45 PDT
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] (away until 9/3) 2011-11-01 22:15:09 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/b316ac07d768
Comment 20 Ed Morley [:emorley] 2011-11-02 06:30:29 PDT
https://hg.mozilla.org/mozilla-central/rev/b316ac07d768

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