Closed Bug 668809 (MatchStartupMem) Opened 13 years ago Closed 9 years ago

[meta] Memory usage should match start-up levels after closing all tabs and triggering MP

Categories

(Core :: General, defect)

Product:
Core
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED WONTFIX

People

(Reporter: justin.lebar+bug, Unassigned)

References

(Depends on 3 open bugs)

Details

(Keywords: memory-footprint, meta)

Attachments

(2 files)

Sample about:memory
95.48 KB, text/plain
Details
Sample about:memory after minimize memory usage
94.35 KB, text/plain
Details 
This is bound to involve a lot of changes, but I think it's a goal worth working towards: If I close all tabs except about:blank and GC/CC/fire memory-pressure events (e.g. by clicking "minimize memory usage" in about:memory), Firefox should use about as much memory as it does on startup.

As proof that we're not anywhere near this right now, my nightly had an RSS of 1.4g last night.  When I closed all my tabs and clicked "minimize memory usage", we went down to 1.3g.
I agree that this is a worthy goal.  I don't think we'll ever hit it, but that doesn't mean that we shouldn't aim for it.

Things of note:

1. Fragmentation will cause some of this.
2. We need to hook up all of our caches to the memory pressure stuff.
3. We probably have actual leaks that need to be dealt with too.

There's an existing tool that can take diffs of the heap.  (diffbloatdump from trace-malloc, IIRC)  Taking a diff between startup and closing all the tabs/minimizing would probably be a good place to start here.
According to bug 666058, comment #17, that bug may help a lot here.
To expand slightly upon that:

1. Build with --enable-trace-malloc and --enable-cpp-rtti
2. Run the browser with --trace-malloc /dev/null
3. After startup, call window.TraceMallocDumpAllocations(before) on a window.
4. Do your browsing
5. Do your memory minimization
6. Call window.TraceMallocDumpAllocations(after)
7. Run http://mxr.mozilla.org/mozilla-central/source/tools/trace-malloc/diffbloatdump.pl?force=1 on the logs.
> 1. Fragmentation will cause some of this.

And as bug 666058 shows, we should remember that fragmentation exists outside the malloc heap.
> window.TraceMallocDumpAllocations(before)

For those following along at home, this should be

  window.TraceMallocDumpAllocations('before')

and you should run from the error console, which runs with chrome privs.
These tools may need a rewrite.  Just running fix-linux-stack.pl on dumps of 100M heaps before passing them to diffbloatdump takes more than 5 minutes.
Depends on: ZombieCompartments
(In reply to comment #0)
> This is bound to involve a lot of changes, but I think it's a goal worth
> working towards: If I close all tabs except about:blank and GC/CC/fire
> memory-pressure events (e.g. by clicking "minimize memory usage" in
> about:memory), Firefox should use about as much memory as it does on startup.

As Kyle says, it's probably unattainable.  One example off the top of my head:  the spell-checking dictionary is ~2MB, and once instantiated it lives forever, AFAICT.  The url-classifier complicates things too (bug 650649).  But you're right that we can do much better.
 
> As proof that we're not anywhere near this right now, my nightly had an RSS
> of 1.4g last night.  When I closed all my tabs and clicked "minimize memory
> usage", we went down to 1.3g.

Ouch.  Can you attach about:memory's output if you see that again?  I bet the JS heap was a big part of it, and the multiply mentioned bug 666058 should help a lot with that.
Depends on: 666058
Keywords: footprint
> Ouch.  Can you attach about:memory's output if you see that again?

Yes, for sure.  js heap was about half; the rest was unclassified.  It'll be better now that we track per-compartment stats.
Attached file Sample about:memory — 
Here's an example of an about:memory that's using a lot of RAM.  I ran it through the following sed script to strip out the full URLs:

  sed 's#\(https\?://[^/]\+\)/.*)#\1)#'

I had open gmail, bugzilla, and wiki.m.o.  I didn't have open nytimes, blogspot, mailinator, schneier, youtube, or many of the other sites listed there.  In fact, I hadn't been to those sites for hours.
This is attachment 543550 [details] after clicking "minimize memory usage".  You can see that it didn't do much at all.
What happens if you leave it alone for say ten minutes and then hit minimize memory usage a bunch of times?  In Bug 668871, the tbpl compartment seemed to go away after a few minutes.  Though I suppose for sites you hadn't been to for a few hours, whatever process is happening for tbpl should have already happened.
(In reply to comment #10)
> 
> This is attachment 543550 [details] after clicking "minimize memory usage". 
> You can see that it didn't do much at all.

I count 78 compartments.  Holy leak, Batman!

What we need now is to narrow it down, eg. to a single website.
> What we need now is to narrow it down, eg. to a single website.

We've done this in bug 668871.
(In reply to comment #13)
> > What we need now is to narrow it down, eg. to a single website.
> 
> We've done this in bug 668871.

Yes.  I changed the name of that bug to be site-specific and you reverted the change, not sure why.

Also, that one is looking like it might be invalid due to the timer issue.  But there are clearly lots of other pages for which this is happening, so we need to narrow it down to individual pages.  I'll take a look on Monday.
(In reply to comment #14)
> Yes.  I changed the name of that bug to be site-specific and you reverted
> the change, not sure why.

I think I thought I'd originally filed the bug as site-specific, and then I realized that plenty of other sites (not just tbpl) were holding onto compartments.  I thought I was reverting myself!  :)

> Also, that one is looking like it might be invalid due to the timer issue. 
> But there are clearly lots of other pages for which this is happening, so we
> need to narrow it down to individual pages.  I'll take a look on Monday.

I guess my (optimistic) assumption was that they're all happening for a similar reason.  But of course that's not necessarily the case!
I created bug 669116 to add a memory reporter for the spell-checking dictionary;  I think that once it's created it never goes away, and so is a factor in the "Firefox used less memory at startup" observation.
Depends on: 669116
Summary: If I close all tabs except about:blank and GC/CC/fire memory-pressure events, Firefox should use about as much memory as it does on startup → [meta] If I close all tabs except about:blank and GC/CC/fire memory-pressure events, Firefox should use about as much memory as it does on startup
Depends on: 669119
No longer depends on: 669119
Depends on: 669119
Whiteboard: [MemShrink] → [MemShrink:P1]
Depends on: 669545
Alias: MatchStartupMem
No longer depends on: 669545
Summary: [meta] If I close all tabs except about:blank and GC/CC/fire memory-pressure events, Firefox should use about as much memory as it does on startup → [meta] Memory usage should match start-up levels after closing all tabs and triggering MP
Depends on: 411894
Jesse sagely pointed out that every leak bug should block this one.  Should we restrict it to something like "memory pressure events should discard everything that wasn't present at startup"?
No longer depends on: 411894
I've been thinking of it as "leaks that persist through tab closing, but not shutdown".  It is nice because it is as quantifiable as shutdown leaks (though at this point not as automatic to find regressions on).  Yet another category would be leaks that go away when you close a tab.
(In reply to comment #18)
> I've been thinking of it as "leaks that persist through tab closing, but not
> shutdown".  It is nice because it is as quantifiable as shutdown leaks
> (though at this point not as automatic to find regressions on).  Yet another
> category would be leaks that go away when you close a tab.

Also fragmentation issues count against this bug.  But I'm still not sure where to draw the line -- should we really be counting every non-shutdown leak against this?
(In reply to comment #21)
> Also fragmentation issues count against this bug.  But I'm still not sure
> where to draw the line -- should we really be counting every non-shutdown
> leak against this?

I think my phrasing was jumbled, but not all non-shutdown leaks fall under here.  Any leak that is fixed by closing a tab won't be included, in the same way that leaks that are fixed by shutting down the browser aren't shutdown leaks.
Depends on: 669739
JS GC heap fragmentation is going to be a big obstacle for this goal.  Eg. see bug 670594 comment 23.
(In reply to comment #23)
> JS GC heap fragmentation is going to be a big obstacle for this goal.  Eg.
> see bug 670594 comment 23.

Shouldn't the GC heap free most everything after you close all the tabs?  The only compartment which should remain is the system compartment, right?  And even that should be smaller than normal?

It doesn't look like this is the case in bug 670594 comment 23 -- you're using 30mb of heap space, although I don't know how much is in the system compartment versus landfill.bugzilla.org.

Smaller chunks will help with this.  If we were to make them 4kb and allocate using memalign, I bet we'd get rid of most external fragmentation...
Depends on: 671702
Depends on: 636220
Depends on: 689914
No longer depends on: 689914
After looking at a lot of zombie-compartment bugs (and forums), and other bugs. With many users complaining of slowness and memory hogging. 

As a quickfix how about killing the compartments when a URL (or tab) is closed. This would help with addons that might have problems. Some addons which are already known while some might still be to be discovered. 

Also, I noticed that duplicate tabs use the same compartment. Let me know how I can help.
Forgot to add, this would also help in not forcing useful old-addons from being blacklisted. Which would prevent users from complaining about lost useful addons even though they are not often updated.
> As a quickfix how about killing the compartments when a URL (or tab) is closed.

That would break websites, no?
(In reply to mike from comment #28)
> As a quickfix how about killing the compartments when a URL (or tab) is
> closed. This would help with addons that might have problems. Some addons
> which are already known while some might still be to be discovered. 

The compartment isn't removed, because it's still in use (some dependency still exists). That's the whole point of bug 668871 - you can't remove them, because they're in use.

> 
> Also, I noticed that duplicate tabs use the same compartment. Let me know
> how I can help.

Compartments do not belong to a specific tab, but to a origin, so they're potentially shared among different tabs. That's one of the dependencies.
Note that zombie compartments are actually pretty rare these days if you're not using any extensions.  And if an extension is keeping a compartment alive, well, we'd have to kill both the extension and the compartment, which probably isn't what you want.
We won't make any progress here without measurements.  Bug 704646 comment 1 has some ideas.
Depends on: 746009
We're going through MemShrink:P1 bugs today.

Based on all our data, basically all that remains to be fixed here is fragmentation, in both the JS engine and in jemalloc.

The JS fragmentation is bug 619558, and the jemalloc fragmentation is bug 746009.  So we're going to dupe this.  I can only dupe this to one bug, so I'll arbitrarily pick bug 619558.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → DUPLICATE
No longer depends on: GenerationalGC
Depends on: 650161
Resolution: DUPLICATE → FIXED
This is far from fixed, at least on Windows.
Status: RESOLVED → REOPENED
Depends on: 1046169
Resolution: FIXED → ---
This would be nice but is close to impossible given that fragmentation is inevitable. The bug is very general and not serving any useful purpose at the moment so I will re-close it. There are numerous other open bugs for more specific issues.
Status: REOPENED → RESOLVED
Closed: 12 years ago → 9 years ago
Resolution: --- → WONTFIX
Whiteboard: [MemShrink:P1]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: