Closed Bug 683517 Opened 13 years ago Closed 13 years ago

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

Categories

(Core :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla10

People

(Reporter: jdm, Assigned: deLta30)

Details

(Whiteboard: [good first bug] [mentor=jdm] [MemShrink:P2] [lang=c++])

Attachments

(1 file, 2 obsolete files)

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
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.
Whiteboard: [good first bug] [mentor=jdm] [MemShrink] → [good first bug] [mentor=jdm] [MemShrink:P2]
OS: Mac OS X → All
Hardware: x86 → All
Assignee: nobody → innomotive
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.
Whiteboard: [good first bug] [mentor=jdm] [MemShrink:P2] → [good first bug] [mentor=jdm] [MemShrink:P2] [lang=c++]
Assignee: innomotive → jitenmt
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).
Attachment #570040 - Flags: review?(josh)
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).
">+      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.
Wow, you're right. I feel quite silly for suggesting it now. You can ignore those comments.
(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.
This should be ready for tryserver now, I guess.
Attachment #570040 - Attachment is obsolete: true
You're missing actually creating the ExpirationTrackerObserver now. Currently this will crash because you're dereferencing a null pointer when calling Init.
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.
I have checked this and there is no crash on pressing 'Minimise memory usage' button on 'about:memory'.
Attachment #570214 - Attachment is obsolete: true
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
My mistake, the patch didn't apply cleanly. There's another try run with real results on its way.
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
Attachment #570442 - Flags: review?(roc)
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
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.
No, it won't work.Macro takes the parameter 'nsExpirationTracker<T, K>::ExpirationTrackerObserver' as 2 parameters instead of 1 because of ',' in between.
https://hg.mozilla.org/mozilla-central/rev/b316ac07d768
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
You need to log in before you can comment on or make changes to this bug.