The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla10

Status

()

Core
General
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jdm, Assigned: deLta30)

Tracking

unspecified
mozilla10
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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
(Reporter)

Comment 1

6 years ago
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]
(Reporter)

Updated

6 years ago
OS: Mac OS X → All
Hardware: x86 → All

Updated

6 years ago
Assignee: nobody → innomotive
(Reporter)

Comment 2

6 years ago
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.
(Reporter)

Updated

6 years ago
Whiteboard: [good first bug] [mentor=jdm] [MemShrink:P2] → [good first bug] [mentor=jdm] [MemShrink:P2] [lang=c++]
(Assignee)

Updated

6 years ago
Assignee: innomotive → jitenmt
(Assignee)

Comment 3

6 years ago
Created attachment 570040 [details] [diff] [review]
Created class ExpirationTrackerObserver inside nsExpirationTracker which calls AgeAllGenerations() when "memory-pressure" message is observed.
Attachment #570040 - Flags: review?(josh)
(Reporter)

Comment 4

6 years ago
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)
(Reporter)

Comment 5

6 years ago
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).
(Assignee)

Comment 6

6 years ago
">+      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.
(Reporter)

Comment 7

6 years ago
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.
(Assignee)

Comment 9

6 years ago
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.
Attachment #570040 - Attachment is obsolete: true
(Reporter)

Comment 10

6 years ago
You're missing actually creating the ExpirationTrackerObserver now. Currently this will crash because you're dereferencing a null pointer when calling Init.
(Reporter)

Comment 11

6 years ago
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.
(Assignee)

Comment 12

6 years ago
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'.
Attachment #570214 - Attachment is obsolete: true

Comment 13

6 years ago
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
(Reporter)

Comment 14

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

Comment 15

6 years ago
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
(Reporter)

Updated

6 years ago
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
(Reporter)

Comment 17

6 years ago
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.
(Assignee)

Comment 18

6 years ago
No, it won't work.Macro takes the parameter 'nsExpirationTracker<T, K>::ExpirationTrackerObserver' as 2 parameters instead of 1 because of ',' in between.
Attachment #570442 - Flags: review?(roc) → review+
(Reporter)

Comment 19

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/b316ac07d768
https://hg.mozilla.org/mozilla-central/rev/b316ac07d768
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
You need to log in before you can comment on or make changes to this bug.